-----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 > +++ bzrlib/bzrdir.py 2009-10-15 20:21:14 +0000 > @@ -1034,6 +1034,19 @@ > except errors.NotBranchError: > return False > > + def has_repository(self): > + """Tell if this bzrdir contains a repository. > + > + Note: if you're going to open the repository, you should just go ahead > + and try, and not ask permission first. (This method just opens the > + repository and discards it, and that's somewhat expensive.) > + """ > + try: > + self.open_repository() > + return True > + except errors.NoRepositoryPresent: > + return False > + This seems like a function that would rarely be used, and could be misused by people who use "look before you leap" instead of "easier to ask forgiveness than permission". It's not very long, and it changes the API. Do we really want it? > @@ -1642,9 +1660,16 @@ > > 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 This seems irrelevant to the effect you're trying to achieve, and I can't see any tests for it. Do you really need it? Also, it exceeds 79 characters. Please re-wrap it. (Sorry, I didn't notice this before.) > @@ -1743,9 +1777,16 @@ > > def open_branch(self, unsupported=False, ignore_fallbacks=False): > """See BzrDir.open_branch.""" > - format = self.find_branch_format() > - self._check_supported(format, unsupported) > - return format.open(self, _found=True, ignore_fallbacks=ignore_fallbacks) > + try: > + format = self.find_branch_format() > + self._check_supported(format, unsupported) > + return format.open(self, _found=True, ignore_fallbacks=ignore_fallbacks) This line exceeds 79 characters. Please re-wrap it. > + 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') This line exceeds 79 characters. Please re-wrap it. > + raise e > > === modified file 'bzrlib/remote.py' > --- bzrlib/remote.py 2009-10-15 04:01:26 +0000 > +++ bzrlib/remote.py 2009-10-15 20:21:14 +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': > > === modified file 'bzrlib/smart/bzrdir.py' > --- bzrlib/smart/bzrdir.py 2009-09-22 00:34:10 +0000 > +++ bzrlib/smart/bzrdir.py 2009-10-15 20:21:14 +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) > > def _boolean_to_yes_no(self, a_boolean): > @@ -465,8 +467,10 @@ > return SuccessfulSmartServerResponse(('ok', '')) > else: > return SuccessfulSmartServerResponse(('ok', reference_url)) > - except errors.NotBranchError: > - return FailedSmartServerResponse(('nobranch', )) > + except errors.NotBranchError, e: > + if e.detail is None: > + return FailedSmartServerResponse(('nobranch',)) > + return FailedSmartServerResponse(('nobranch', e.detail)) > > > class SmartServerRequestOpenBranchV2(SmartServerRequestBzrDir): > @@ -481,5 +485,7 @@ > return SuccessfulSmartServerResponse(('branch', format)) > else: > return SuccessfulSmartServerResponse(('ref', reference_url)) > - except errors.NotBranchError: > - return FailedSmartServerResponse(('nobranch', )) > + except errors.NotBranchError, e: > + if e.detail is None: > + return FailedSmartServerResponse(('nobranch',)) > + return FailedSmartServerResponse(('nobranch', e.detail)) > > === 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 20:21:14 +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") > > === modified file 'bzrlib/tests/test_smart.py' > --- bzrlib/tests/test_smart.py 2009-09-24 05:31:23 +0000 > +++ bzrlib/tests/test_smart.py 2009-10-15 20:21:14 +0000 > @@ -511,6 +511,14 @@ > self.assertEqual(SmartServerResponse(('ok', reference_url)), > request.execute('reference')) > > + def test_notification_on_branch_from_repository(self): > + """When there is a repository, the error should return details.""" > + backing = self.get_transport() > + request = smart.bzrdir.SmartServerRequestOpenBranch(backing) > + repo = self.make_repository('.') > + self.assertEqual(SmartServerResponse(('nobranch', 'location is a repository')), This line exceeds 79 characters. Please re-wrap it. > + request.execute('')) > + > > class TestSmartServerRequestOpenBranchV2(TestCaseWithChrootedTransport): > > @@ -562,6 +570,14 @@ > response) > self.assertLength(1, opened_branches) > > + def test_notification_on_branch_from_repository(self): > + """When there is a repository, the error should return details.""" > + backing = self.get_transport() > + request = smart.bzrdir.SmartServerRequestOpenBranchV2(backing) > + repo = self.make_repository('.') > + self.assertEqual(SmartServerResponse(('nobranch', 'location is a repository')), This line exceeds 79 characters. Please re-wrap it. Aaron -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEUEARECAAYFAkrkiJUACgkQ0F+nu1YWqI1ahgCfSdgVCjvmIS24R/GqcN53uhQk DpEAmIrV0cCqYF59pXUHlMl7xGEIRtg= =wsWG -----END PGP SIGNATURE-----