Code review comment for lp:~slyguy/bzr/bug-440952-bzrdir

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 in practical terms there's not much difference, but this way feels a
touch nicer to me, and won't needlessly modify the traceback.

I suppose one advantage is that if NotBranchError ever grows more attributes
then my way will cope automatically, whereas your code will silently discard
some.

> === modified file 'bzrlib/remote.py'
> --- bzrlib/remote.py 2009-10-15 04:01:26 +0000
> +++ bzrlib/remote.py 2009-10-15 17:05:32 +0000
> @@ -2814,6 +2814,9 @@
> raise NoSuchRevision(find('repository'), err.error_args[0])
> elif err.error_tuple == ('nobranch',):
> raise errors.NotBranchError(path=find('bzrdir').root_transport.base)
> + elif err.error_verb == 'nobranch':
> + raise errors.NotBranchError(path=find('bzrdir').root_transport.base,
> + detail=err.error_args[0])
> elif err.error_verb == 'norepository':
> raise errors.NoRepositoryPresent(find('bzrdir'))
> elif err.error_verb == 'LockContention':
Hmm, d'oh. Newer servers that send the 'detail' field will cause
UnknownErrorFromSmartServer in old clients. Probably we should sniff the client
version header in the server and strip out that field :/

I'm happy to take care of this part, it's overdue infrastructural work, but
should be straighforward...

Hmm. The other option is to make a new revision of the BzrDir.open_branch verb,
and only send the detail to clients that use that. Given that so few verbs are
affected that might be simpler. Either way, I can take care of this bit if you
want.

> === modified file 'bzrlib/smart/bzrdir.py'
> --- bzrlib/smart/bzrdir.py 2009-09-22 00:34:10 +0000
> +++ bzrlib/smart/bzrdir.py 2009-10-15 17:05:32 +0000
> @@ -88,8 +88,10 @@
> try:
> self._bzrdir = BzrDir.open_from_transport(
> self.transport_from_client_path(path))
> - except errors.NotBranchError:
> - return FailedSmartServerResponse(('nobranch', ))
> + except errors.NotBranchError, e:
> + if e.detail is None:
> + return FailedSmartServerResponse(('nobranch',))
> + return FailedSmartServerResponse(('nobranch', e.detail))
> return self.do_bzrdir_request(*args)

Rather than adjust multiple places in bzrlib/smart/bzrdir.py, remove these
except blocks entirely and put the serialisation logic in _translate_error
bzrlib/smart/request.py. (Obviously, don't remove the cases that don't translate
NotBranchError into a 'nobranch' failure.)

(The only reason those try/except blocks still exist is that the generic error
serialisation in request.py is relatively new.)

> === modified file 'bzrlib/tests/blackbox/test_shared_repository.py'
> --- bzrlib/tests/blackbox/test_shared_repository.py 2009-09-23 23:58:10 +0000
> +++ bzrlib/tests/blackbox/test_shared_repository.py 2009-10-15 17:05:32 +0000
> @@ -120,3 +120,12 @@
> # become necessary for this use case. Please do not adjust this number
> # upwards without agreement from bzr's network support maintainers.
> self.assertLength(15, self.hpss_calls)
> +
> + def test_notification_on_branch_from_repository(self):
> + out, err = self.run_bzr("init-repository -q a")
> + self.assertEqual(out, "")
> + self.assertEqual(err, "")
> + dir = BzrDir.open('a')
> + self.assertIs(dir.has_repository(), True)
> + e = self.assertRaises(errors.NotBranchError, dir.open_branch)
> + self.assertEqual(e.detail, "location is a repository")

Hmm... this isn't really the right place for this test. You're asserting something
about the open_branch API here, not about part of the UI, which is mainly what
we use blackbox tests for.

The other thing we use blackbox tests for is integration tests/smoke tests. I
guess this does roughly fit that, but it isn't quite right. Specifically:
 - I don't think you should assert anything about the stdout or stderr of the
   init-repository command in this test. This test isn't about that.
 - Consequently, don't use the UI to make the shared repo, just call the API
   directly.
 - Also, I think we want to assert that the "location is a repository" text is
   something that does reach the UI. So instead of assertRaises that
   open_branch has that in an attribute, do something like
   run_bzr("bzr branch a b") and assert that that text appears in the output.

Additionally, I think we should add a per_bzrdir test that does the
assertRaises and assertEqual that you're currently doing here.

Does that testing strategy make sense to you?

> === modified file 'bzrlib/tests/test_smart.py'

The tests you've added here are good. TestErrorTranslationSuccess in
test_remote should also be updated. And if you take my suggestion to put the
error serialisation one place, then TestSmartRequestHandlerErrorTranslation in
test_smart_request.py should be updated too.

-Andrew.