Merge lp:~slyguy/bzr/bug-440952-bzrdir into lp:bzr

This branch contains an attempt to tackle bug 440952 (https://bugs.launchpad.net/bugs/440952). This bug is based on a complaint where a user tried to branch from a repository directory. Bzr's error message was pretty opaque: the problem was that the location provided was a repository, not a branch.

Following a suggestion from John, I've modified the BzrDir.open_branch() and BzrDir.get_branch_reference() to, upon a NotBranchError, do a further probe to see if the location may actually be a repository. If it is a repository, the NotBranchError includes additional detail "location is a repository", which will be visible to the user when reported. For example:

  $ ./bzr branch http://bzr.savannah.nongnu.org/r/man-db
  http://bzr.savannah.nongnu.org/r/man-db/ is permanently redirected to http+urllib://bzr.savannah.gnu.org/r/man-db/
  bzr: ERROR: Not a branch: "http+urllib://bzr.savannah.gnu.org/r/man-db/.bzr/branch/": location is a repository.

This change is broken into 4 parts:

  1) Extend NotBranchError to hold an extra field, "detail". If present, this field provides a meaningful explanation to the user why the location is not a branch. I couldn't use the existing "extra" field as the superclass uses it to hold a nicely-formatted variant of whatever is passed (e.g., it prefixes whatever is provided with ": ").

  2) There are many variants of BzrDir, but as pointed out by John, the only one that actually seemed to need modification is BzrDirMeta1. We modify open_branch() and get_branch_reference() to catch the NotBranchError and do a further probe. To simplify the code, I've introduced BzrDir.has_repository(); its default implementation calls open_repository() and returns true if successful, and false if it fails. bzr-svn's SvnRemoteAccess subclasses BzrDir directly, and so should be insulated from this change.

  3) [This is the sketchiest part of my solution.] I've modified the HPSS to
    a) do equivalent repository-probing on a NotBranchError
    b) to serialize the NotBranchError.detail if present
    c) to pick out the NotBranchError.detail on deserialization
   I'm not sure if there is a better way.

  4) I've added tests to ensure the detail is reported when attempting to branch from a repository to bzrlib/tests/blackbox/test_shared_repository.py and bzrlib/tests/test_smart.py. I'm not positive that these locations are the best locations, but they seemed to have similar style tests.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review needs-fixing

Brian de Alwis wrote:
> Brian de Alwis has proposed merging lp:~slyguy/bzr/bug-440952-bzrdir into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This branch contains an attempt to tackle bug 440952 (https://bugs.launchpad.net/bugs/440952). This bug is based on a complaint where a user tried to branch from a repository directory. Bzr's error message was pretty opaque: the problem was that the location provided was a repository, not a branch.
>
> Following a suggestion from John, I've modified the BzrDir.open_branch() and BzrDir.get_branch_reference() to, upon a NotBranchError, do a further probe to see if the location may actually be a repository. If it is a repository, the NotBranchError includes additional detail "location is a repository", which will be visible to the user when reported. For example:

This looks like it would cause Branch.open_containing to do twice as
many probes. Is that true?

Ideally, we would do this probe at the highest level, since
NotBranchError isn't always user-visible. It would be better to do it
in Branch.open, or maybe even by catching NotBranchError in the command
runner and testing for repositories there.

> $ ./bzr branch http://bzr.savannah.nongnu.org/r/man-db
> http://bzr.savannah.nongnu.org/r/man-db/ is permanently redirected to http+urllib://bzr.savannah.gnu.org/r/man-db/
> bzr: ERROR: Not a branch: "http+urllib://bzr.savannah.gnu.org/r/man-db/.bzr/branch/": location is a repository.
>
> This change is broken into 4 parts:
>
> 1) Extend NotBranchError to hold an extra field, "detail". If present, this field provides a meaningful explanation to the user why the location is not a branch. I couldn't use the existing "extra" field as the superclass uses it to hold a nicely-formatted variant of whatever is passed (e.g., it prefixes whatever is provided with ": ").
>
> 2) There are many variants of BzrDir, but as pointed out by John, the only one that actually seemed to need modification is BzrDirMeta1. We modify open_branch() and get_branch_reference() to catch the NotBranchError and do a further probe. To simplify the code, I've introduced BzrDir.has_repository(); its default implementation calls open_repository() and returns true if successful, and false if it fails. bzr-svn's SvnRemoteAccess subclasses BzrDir directly, and so should be insulated from this change.
>
> 3) [This is the sketchiest part of my solution.] I've modified the HPSS to
> a) do equivalent repository-probing on a NotBranchError
> b) to serialize the NotBranchError.detail if present
> c) to pick out the NotBranchError.detail on deserialization
> I'm not sure if there is a better way.
>
> 4) I've added tests to ensure the detail is reported when attempting to branch from a repository to bzrlib/tests/blackbox/test_shared_repository.py and bzrlib/tests/test_smart.py. I'm not positive that these locations are the best locations, but they seemed to have similar style tests.
>
>

> @@ -1465,7 +1478,11 @@
> return not isinstance(self._format, format.__class__)
>
> def open_branch(self, unsup...

Thanks for the speedy review, Aaron.

On 15-Oct-2009, at 3:15 PM, Aaron Bentley wrote:
> This looks like it would cause Branch.open_containing to do twice as
> many probes. Is that true?

So not that I've seen: to experiment, I placed a mutter in the
exception code and then tried running various bzr commands from
various locations to affect various locations. Basically the code is
only invoked whenever I specify a location that is a repository — I
guess most commands must have something that filters out non-.bzr
containing locations.

Branch.open_containing is a bit tortuous, but it seems to do its work
using transport objects, and doesn't seem to use BzrDir.open_branch.

> Ideally, we would do this probe at the highest level, since
> NotBranchError isn't always user-visible. It would be better to do it
> in Branch.open, or maybe even by catching NotBranchError in the
> command
> runner and testing for repositories there.

So attempting to restrict changes to Branch.open doesn't seem to be
possible as there is a lot of the code that uses BzrDir.open_branch()
instead.

Placing the test in the command runner (I assume you mean something
like Command.run_argv_aliases?) is a bit difficult. Although
NotBranchError.path provides the path/URL, it isn't necessarily the
path actually provided by the user. The example from the motivating
use case demonstrates this: the exception reports the path as ".../man-
db/.bzr/branch":

>> $ ./bzr branch http://bzr.savannah.nongnu.org/r/man-db
>> http://bzr.savannah.nongnu.org/r/man-db/ is permanently redirected
>> to http+urllib://bzr.savannah.gnu.org/r/man-db/
>> bzr: ERROR: Not a branch: "http+urllib://bzr.savannah.gnu.org/r/man-db/.bzr/branch/
>> ": location is a repository.

Finally, I think there are two compelling reasons to recommend such a
test happen as close to the error as possible. First, it avoids an
additional probe when using HPSS. Second, it's entirely possible that
there will be some foreign VCS that won't have a concept of a
repository; bzr-svn, for example, provides its own BzrDir subclass and
will never perform the has_repository tests.

Re: your code comments:

>> def open_branch(self, unsupported=False, ignore_fallbacks=False):
>> - """See BzrDir.open_branch."""
>> + """See BzrDir.open_branch.
>> +
>> + Note: Test for repository on failure is not done here as
>> + BzrDirPreSplitOut cannot have a repository.
>
> This is false: BzrDirPreSplitOut *always* has a repository, and it
> *always* has a branch. There's no point testing for a repository
> because if there's no branch, there's no BzrDirPreSplitOut and no
> repository.

I've amended the comment.

>> + def test_notif_on_branch_from_repo(self):
>
> ^^^ typo

And changed all tests to use consistent method names.

I've updated the code. I hope this addresses your comments.

Brian.

--
Brian de Alwis
<email address hidden>

Overall, this is a good patch, and remarkably thorough. Thank you!

I like the approach that only looks for the repository in the error case. I'm a
little concerned that there may be a case that calls open_containing or
something that will get the NotBranchError, do the extra lookup, then ignore the
detail because it just tries the parent directory. I think that's not very
common, and at worst it will only happen for every BzrDir found in while walking
parent dirs. So I think that's ok. Perhaps if we wanted to be really fancy
we'd actually make NotBranchError.detail a lazy attribute so that if nothing
looks at it or stringifies the error, we don't pay the cost, but I'm ok with
worrying about that later (if at all).

Because Aaron's already reviewed this I've focused my comments on the HPSS code,
which he didn't really comment on.

Finally, I realise we're asking for a fair bit extra work on top of the
obviously large amount you've already done. If you don't feel able to do this
work soon let us know. I'm happy to finish this work if you're too busy,
although if you can do it soon I'm sure it will get done sooner than if I add it
my todo list :)

Brian de Alwis wrote:
[...]
> 3) [This is the sketchiest part of my solution.] I've modified the HPSS to
> a) do equivalent repository-probing on a NotBranchError
> b) to serialize the NotBranchError.detail if present
> c) to pick out the NotBranchError.detail on deserialization
> I'm not sure if there is a better way.

I think that's a reasonable approach. I suspect eventually we'll get to the
point where we simply do a single HPSS call that asks for the details of all of
BzrDir, Repository, Branch (and maybe WorkingTree) that are present at a
location... if we did that then we'd simply infer this situation from the result
of that rather than NotBranchError. But until then I think this is a simple and
effective solution.

> 4) I've added tests to ensure the detail is reported when attempting to
> branch from a repository to bzrlib/tests/blackbox/test_shared_repository.py
> and bzrlib/tests/test_smart.py. I'm not positive that these locations are
> the best locations, but they seemed to have similar style tests.

Not the best, but a great start! I've given some suggestions below.

[...]
> def get_branch_reference(self):
> """See BzrDir.get_branch_reference()."""
> - from bzrlib.branch import BranchFormat
> - format = BranchFormat.find_format(self)
> - return format.get_reference(self)
> + try:
> + from bzrlib.branch import BranchFormat
> + format = BranchFormat.find_format(self)
> + return format.get_reference(self)
> + except errors.NotBranchError, e:
> + # provide indication to the user if they're actually
> + # opening a repository (see bug 440952)
> + if self.has_repository():
> + raise errors.NotBranchError(path=e.path, detail='location is a repository')
> + raise e

I wonder if it would be better to do:

    if e.detail is None and self.has_repository():
        e.detail = 'location is a repository'
    raise e

I think i...

On 16-Oct-2009, at 1:48 AM, Andrew Bennetts wrote:
> Finally, I realise we're asking for a fair bit extra work on top of
> the
> obviously large amount you've already done. If you don't feel able
> to do this
> work soon let us know. I'm happy to finish this work if you're too
> busy,
> although if you can do it soon I'm sure it will get done sooner than
> if I add it
> my todo list :)

Since most of the extra work are a bit beyond my ken, if you wouldn't
mind taking it on, I would be most appreciative if you took it on :-)

Thanks Andrew!

Brian.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review needs-fixing

Brian de Alwis wrote:
> Branch.open_containing is a bit tortuous, but it seems to do its work
> using transport objects, and doesn't seem to use BzrDir.open_branch.

It absolutely does. First it acquires a BzrDir using
BzrDir.open_containing, and then it tries to open it using
BzrDir.open_branch.

>> Ideally, we would do this probe at the highest level, since
>> NotBranchError isn't always user-visible. It would be better to do it
>> in Branch.open, or maybe even by catching NotBranchError in the
>> command
>> runner and testing for repositories there.
>
> So attempting to restrict changes to Branch.open doesn't seem to be
> possible as there is a lot of the code that uses BzrDir.open_branch()
> instead.

I didn't mean that. I meant that ideally we would only probe for a
repository when we know that NotBranchError will be displayed to the
user, and when we know that the location was user-entered.

> Placing the test in the command runner (I assume you mean something
> like Command.run_argv_aliases?) is a bit difficult. Although
> NotBranchError.path provides the path/URL, it isn't necessarily the
> path actually provided by the user.

Why is that relevant? When you probe for the repository, you're not
using the path entered by the user anyhow.

> Finally, I think there are two compelling reasons to recommend such a
> test happen as close to the error as possible. First, it avoids an
> additional probe when using HPSS.

I think that a single additional probe will not be visible to users. If
there is a slight delay introduced, remember that it will only happen
when there is a error is displayed to users.

Your proposed change will cause additional probes to happen in every
case. On the local filesystem, the low-latency IO means that they will
not be observable, and on HPSS, the probes will be does as part of a
single operation, so they will not cause additional roundtrips. But
over dumb transports such as http and sftp, they can cause observable
lag. Many times, NotBranchErrors are caught and handled, and when they
are, these probes are unnecessary. It's possible for many
NotBranchErrors may be encountered during a successful bzr run.

To summarize: your change can cause multiple, observable probes on dumb
transports, but avoids an additional probe on smart transports when
displaying the error to the user. My suggestion would cause a single
additional probe on smart transports when displaying an error to the
user. I think that is better from the perspective of minimizing probes.

> Second, it's entirely possible that
> there will be some foreign VCS that won't have a concept of a
> repository; bzr-svn, for example, provides its own BzrDir subclass and
> will never perform the has_repository tests.

I think what you're trying to say here is that we would reopen the
BzrDir unnecessarily in that case? We could certainly attach the BzrDir
to the NotBranchError to avoid that. But again, slightly more lag
during error handling isn't the end of the world.

> === modified file 'bzrlib/bzrdir.py'
> --- bzrlib/bzrdir.py 2009-09-24 09:45:23 +0000
>...

On Thu, 2009-10-15 at 17:05 +0000, Brian de Alwis wrote:
> Brian de Alwis has proposed merging lp:~slyguy/bzr/bug-440952-bzrdir into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This branch contains an attempt to tackle bug 440952 (https://bugs.launchpad.net/bugs/440952).

I see you already have some excellent reviews. I wonder perhaps if
changing open_branch/get_branch_reference is overkill, given the
complexity it adds? It makes a clearly defined and *predictable*
overhead function sometimes twice as expensive. And the function can be
a good fraction of a second.

I'd rather not see 'has_repository' as a public method. I don't see
*bzr* repo's suffering from many extra probes - bzrdirs without branches
that are not repositories are pretty rare. However, the svn mapping
creates a svndir object at every path under an svn repo and at every dir
in an svn tree, which we then loop up on - so the performance impact
there could be substantial.

So in balance, I'd also rather see this done from the command runner or
Branch.open. For the latter, just putting the bzrdir that was found *if*
one was found in NotBranchError would let
Branch.open/Branch.open_containing do the translation without altering
the core logic to be unpredictable. Doing it from the command runner is
probably too high up.

 review: needsfixing

Also, Brian, can you complete a contributor agreement? <http://www.canonical.com/contributors>

Thanks very much.

On 17-Nov-2009, at 8:01 PM, Andrew Bennetts wrote:
> Also, Brian, can you complete a contributor agreement? <http://www.canonical.com/contributors>

Done. I thought I had done this previously, but can't find any record of having sent it.

Brian, Andrew, can you update this merge proposal so we know who is working on what ?

This seems a bit stalled because there is no clear next step. It looks like Andrew has offered to take it from here, which slyguy has accepted. Also, Robert has some additional comments. Since Andrew's patch pilot for the first week of 2010, I'm going to set this back to work in progress, and mark 440952 as in progress and assigned to him, and hopefully he can take it from there.