-----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, 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. > @@ -1690,6 +1714,15 @@ > pass > return self.transport.clone('checkout') > > + def has_repository(self): > + """See BzrDir.has_repository().""" > + from bzrlib.repository import RepositoryFormat > + try: > + RepositoryFormat.find_format(self) > + return True > + except errors.NoRepositoryPresent: > + return False > + > def has_workingtree(self): > """Tell if this bzrdir contains a working tree. > > @@ -1743,9 +1776,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) > + 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 > > def open_repository(self, unsupported=False): > """See BzrDir.open_repository.""" > > === modified file 'bzrlib/errors.py' > --- bzrlib/errors.py 2009-08-30 21:34:42 +0000 > +++ bzrlib/errors.py 2009-10-15 17:05:32 +0000 > @@ -702,11 +702,14 @@ > # TODO: Probably this behavior of should be a common superclass > class NotBranchError(PathError): > > - _fmt = 'Not a branch: "%(path)s".' > + _fmt = 'Not a branch: "%(path)s"%(extra)s.' > > - def __init__(self, path): > + def __init__(self, path, detail=None): > import bzrlib.urlutils as urlutils > - self.path = urlutils.unescape_for_display(path, 'ascii') > + path = urlutils.unescape_for_display(path, 'ascii') > + # remember the detail in case of remote serialization > + self.detail = detail > + PathError.__init__(self, path=path, extra=self.detail) > > > class NoSubmitBranch(PathError): > > === 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': > > === 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) > > 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 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") > > === 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 17:05:32 +0000 > @@ -511,6 +511,14 @@ > self.assertEqual(SmartServerResponse(('ok', reference_url)), > request.execute('reference')) > > + def test_notif_on_branch_from_repo(self): ^^^ typo > + """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')), > + request.execute('')) > + > > class TestSmartServerRequestOpenBranchV2(TestCaseWithChrootedTransport): > > @@ -562,6 +570,14 @@ > response) > self.assertLength(1, opened_branches) > > + def test_notif_on_branch_from_repo(self): ^^ typo > + """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')), > + request.execute('')) > + > > class TestSmartServerRequestRevisionHistory(tests.TestCaseWithMemoryTransport): > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkrXdIYACgkQ0F+nu1YWqI33wACePiuea+l3D7DkXW5cpGs/FaAh imMAnRdQvcAWMKhEZvJnRI/Uf22b+3pp =RGas -----END PGP SIGNATURE-----