Merge lp:~chy-causer/bzr-bookmarks/short-form into lp:bzr-bookmarks

[ChrisCauser]

This is a very small change that I find useful. Basically if I issue the command,

$ bzr bookmark wibble .

I expect the bookmark wibble to refer to the branch at $PWD at the time of inserting the bookmark, rather than at the time of dereferencing it.

Like before, the plugin makes no check for the presence of a branch at the location, but I think this new behaviour is more intuitive and useful to others.

This will not do anything for people who have already bookmarked "." or for example "../../trunk", although it may be potentially confusing if they bookmark something again in future expecting the same result (which incidentally, in the case of the latter example is a silly usecase because the dereferencing command would have to be given in the same directory as when the bookmark insertion was made.) I'm not sure what to do about that.

[Jelmer Vernooij (jelmer)]

Hi Chris,

Rather than doing this in just one of the backends, I think this should be handled by the caller of .set_location.

review: Needs Fixing

[ChrisCauser (chy-causer)]

> Rather than doing this in just one of the backends, I think this should be
> handled by the caller of .set_location.

I am happy to do this, but I fear that a backward-incompatible change like this may not sit well with other plugins, although I can't say that with any authority. What's your take on it? Should it be passed as an option, for example .set_location(..., make_absolute=True)?

[Jelmer Vernooij (jelmer)]

On 31/08/11 18:15, ChrisCauser wrote:
>> Rather than doing this in just one of the backends, I think this should be
>> handled by the caller of .set_location.
> I am happy to do this, but I fear that a backward-incompatible change like this may not sit well with other plugins, although I can't say that with any authority. What's your take on it? Should it be passed as an option, for example .set_location(..., make_absolute=True)?
bzr-bookmarks is the only plugin that writes to that location so I don't
think there is a problem with writing an absolete location now. Can you
think of reasons why users might not want an absolute location there?

I do wonder if rather than *storing* a absolute location if it would
make sense to always interpret locations relative to the URL of the path
against which they're set.

Cheers,

Jelmer

[lp:~chy-causer/bzr-bookmarks/short-form]

21. By Chris Causer <email address hidden> on 2011-09-01

Update last commit so that absolute urls are stored for branch configs as well as global

[ChrisCauser (chy-causer)]

> bzr-bookmarks is the only plugin that writes to that location so I don't
> think there is a problem with writing an absolete location now. Can you
> think of reasons why users might not want an absolute location there?

Sorry, I'm going to have to rewind here. I thought set_location was part of bzrlib core but I've just realized that it doesn't exist. What function are we talking about here? Is it cmd_bookmark.run? If so I've just made another commit reflecting that.

>
> I do wonder if rather than *storing* a absolute location if it would
> make sense to always interpret locations relative to the URL of the path
> against which they're set.
>
Interesting. This would involve storing both the bookmark and the place where the bookmark was set. This might allow bookmarks to be cloned with a repository clone, although the number of people requiring that functionality would be pretty small, and I'm sure people coming back to this plugin in years' time would think "now why did they do that?" !

[Jelmer Vernooij (jelmer)]

Hi Chris,

On 01/09/11 10:39, ChrisCauser wrote:
>> bzr-bookmarks is the only plugin that writes to that location so I don't
>> think there is a problem with writing an absolete location now. Can you
>> think of reasons why users might not want an absolute location there?
> Sorry, I'm going to have to rewind here. I thought set_location was part of bzrlib core but I've just realized that it doesn't exist. What function are we talking about here? Is it cmd_bookmark.run? If so I've just made another commit reflecting that.
Yeah, cmd_bookmark.run.
>> I do wonder if rather than *storing* a absolute location if it would
>> make sense to always interpret locations relative to the URL of the path
>> against which they're set.
>>
> Interesting. This would involve storing both the bookmark and the place where the bookmark was set. This might allow bookmarks to be cloned with a repository clone, although the number of people requiring that functionality would be pretty small, and I'm sure people coming back to this plugin in years' time would think "now why did they do that?" !
>
Aren't some of the bookmarks set in the repository itself rather than in
the global configuration? I'm not very familiar with the bookmarks
plugin. If there is just the global configuration, then I think
defaulting to absolute paths always makes sense.

Cheers,

Jelmer

[ChrisCauser (chy-causer)]

> Hi Chris,
> Aren't some of the bookmarks set in the repository itself rather than in
> the global configuration? I'm not very familiar with the bookmarks
> plugin. If there is just the global configuration, then I think
> defaulting to absolute paths always makes sense.

Yes, you are correct that bookmarks can be both repo and global in scope. Hopefully this new diff saves the absolute path for both cases. People who have already saved bookmarks will notice no change in functionality until they create a new bookmark.

[Jelmer Vernooij (jelmer)]

On 06/09/11 17:32, ChrisCauser wrote:
>> Hi Chris,
>> Aren't some of the bookmarks set in the repository itself rather than in
>> the global configuration? I'm not very familiar with the bookmarks
>> plugin. If there is just the global configuration, then I think
>> defaulting to absolute paths always makes sense.
> Yes, you are correct that bookmarks can be both repo and global in scope. Hopefully this new diff saves the absolute path for both cases. People who have already saved bookmarks will notice no change in functionality until they create a new bookmark.
Hmm, interesting. I don't really want to bikeshed about this, but I
wonder if we should do the following:

For per-repo bookmarks, allow relative paths and absolute paths and
resolve those paths relative to the repository path.

For global bookmarks, allow only absolute paths, and always store
absolute paths.

Does that make sense?

Cheers,

Jelmer

[ChrisCauser (chy-causer)]

On 06/09/11 20:21, Jelmer Vernooij wrote:
> Does that make sense?
Yup, perfect sense. I'll update the merge proposal when I've done that.

Unmerged revisions

21. By Chris Causer <email address hidden> on 2011-09-01

Update last commit so that absolute urls are stored for branch configs as well as global

20. By Chris Causer <email address hidden> on 2011-08-30

Absolute paths are generated from relative ones

=== modified file '__init__.py'
--- __init__.py	2011-02-04 11:40:36 +0000
+++ __init__.py	2011-09-01 08:36:25 +0000
@@ -197,6 +197,9 @@
             if not location:
                 raise errors.BzrCommandError(
                     'no location provided for bookmark "%s"' % (name,))
+            if location.startswith('.'):
+                import os.path
+                location = os.path.abspath(location)
             provider.set_bookmark(name, location)