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

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

This is a substantial reworking of <https://code.edge.launchpad.net/~slyguy/bzr/bug-440952-bzrdir/+merge/13431>.

The basic mechanism now is that probing for a repository is delayed until the NotBranchError is stringified. So for non-UI cases, the only extra cost should be that NotBranchError will hold a reference to a bzrdir that is never used.

This also adds a BzrDir.open_branchV3 verb so that this extra field can be passed via the smart protocol. Perhaps the 'nobranch' case should be handled via more generic error translation instead, but this patch was already large enough.

I think this resolves the various objections raised in earlier reviews: it doesn't probe unnecessarily, it doesn't add a has_repository method, it doesn't break HPSS backwards compatibility, etc. I'm willing to be proven wrong, of course! So, please review :)

[John A Meinel (jameinel)]

The final output seems to be:
$ awbzr branch ../bzr
bzr: ERROR: Not a branch: "C:/Users/jameinel/dev/bzr/.bzr/branch/": location is a repository.

I'm not 100% happy with that, but it is better than nothing. The code looks good.

review: Approve

=== modified file 'NEWS'
--- NEWS	2010-01-15 04:06:45 +0000
+++ NEWS	2010-01-15 05:14:10 +0000
@@ -116,6 +116,10 @@
   ``try``/``finally`` blocks where applicable as it is simpler and more
   robust.  (Andrew Bennetts)
 
+* Attempts to open a shared repository as a branch (e.g. ``bzr branch
+  path/to/repo``) will now include "location is a repository" as a hint in
+  the error message.  (Brian de Alwis, Andrew Bennetts, #440952)
+
 * Push will now inform the user when they are trying to push to a foreign 
   VCS for which roundtripping is not supported, and will suggest them to 
   use dpush. (Jelmer Vernooij)
@@ -166,6 +170,10 @@
 Internals
 *********
 
+* Added ``BzrDir.open_branchV3`` smart server request, which can receive
+  a string of details (such as "location is a repository") as part of a
+  ``nobranch`` response.  (Andrew Bennetts, #440952)
+  
 * New helper osutils.UnicodeOrBytesToBytesWriter which encodes unicode
   objects but passes str objects straight through. This is used for
   selftest but may be useful for diff and other operations that generate

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2010-01-08 05:28:17 +0000
+++ bzrlib/branch.py	2010-01-15 05:14:10 +0000
@@ -1441,7 +1441,7 @@
             format_string = transport.get_bytes("format")
             return klass._formats[format_string]
         except errors.NoSuchFile:
-            raise errors.NotBranchError(path=transport.base)
+            raise errors.NotBranchError(path=transport.base, bzrdir=a_bzrdir)
         except KeyError:
             raise errors.UnknownFormatError(format=format_string, kind='branch')
 
@@ -1796,7 +1796,7 @@
                               _repository=a_bzrdir.find_repository(),
                               ignore_fallbacks=ignore_fallbacks)
         except errors.NoSuchFile:
-            raise errors.NotBranchError(path=transport.base)
+            raise errors.NotBranchError(path=transport.base, bzrdir=a_bzrdir)
 
     def __init__(self):
         super(BranchFormatMetadir, self).__init__()

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-01-08 06:33:05 +0000
+++ bzrlib/errors.py	2010-01-15 05:14:10 +0000
@@ -702,11 +702,32 @@
 # 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"%(detail)s.'
 
-    def __init__(self, path):
+    def __init__(self, path, detail=None, bzrdir=None):
        import bzrlib.urlutils as urlutils
-       self.path = urlutils.unescape_for_display(path, 'ascii')
+       path = urlutils.unescape_for_display(path, 'ascii')
+       if detail is not None:
+           detail = ': ' + detail
+       self.detail = detail
+       self.bzrdir = bzrdir
+       PathError.__init__(self, path=path)
+
+    def _format(self):
+        # XXX: Ideally self.detail would be a property, but Exceptions in
+        # Python 2.4 have to be old-style classes so properties don't work.
+        # Instead we override _format.
+        if self.detail is None:
+            if self.bzrdir is not None:
+                try:
+                    self.bzrdir.open_repository()
+                except NoRepositoryPresent:
+                    self.detail = ''
+                else:
+                    self.detail = ': location is a repository'
+            else:
+                self.detail = ''
+        return PathError._format(self)
 
 
 class NoSubmitBranch(PathError):

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2009-12-15 20:32:34 +0000
+++ bzrlib/remote.py	2010-01-15 05:14:10 +0000
@@ -284,21 +284,32 @@
     def _get_branch_reference(self):
         path = self._path_for_remote_call(self._client)
         medium = self._client._medium
-        if not medium._is_remote_before((1, 13)):
+        candidate_calls = [
+            ('BzrDir.open_branchV3', (2, 1)),
+            ('BzrDir.open_branchV2', (1, 13)),
+            ('BzrDir.open_branch', None),
+            ]
+        for verb, required_version in candidate_calls:
+            if required_version and medium._is_remote_before(required_version):
+                continue
             try:
-                response = self._call('BzrDir.open_branchV2', path)
-                if response[0] not in ('ref', 'branch'):
-                    raise errors.UnexpectedSmartServerResponse(response)
-                return response
+                response = self._call(verb, path)
             except errors.UnknownSmartMethod:
-                medium._remember_remote_is_before((1, 13))
-        response = self._call('BzrDir.open_branch', path)
-        if response[0] != 'ok':
+                if required_version is None:
+                    raise
+                medium._remember_remote_is_before(required_version)
+            else:
+                break
+        if verb == 'BzrDir.open_branch':
+            if response[0] != 'ok':
+                raise errors.UnexpectedSmartServerResponse(response)
+            if response[1] != '':
+                return ('ref', response[1])
+            else:
+                return ('branch', '')
+        if response[0] not in ('ref', 'branch'):
             raise errors.UnexpectedSmartServerResponse(response)
-        if response[1] != '':
-            return ('ref', response[1])
-        else:
-            return ('branch', '')
+        return response
 
     def _get_tree_branch(self):
         """See BzrDir._get_tree_branch()."""
@@ -2823,8 +2834,13 @@
         raise NoSuchRevision(find('branch'), err.error_args[0])
     elif err.error_verb == 'nosuchrevision':
         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':
+        if len(err.error_args) >= 1:
+            extra = err.error_args[0]
+        else:
+            extra = None
+        raise errors.NotBranchError(path=find('bzrdir').root_transport.base,
+            detail=extra)
     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	2010-01-15 05:14:10 +0000
@@ -88,8 +88,8 @@
         try:
             self._bzrdir = BzrDir.open_from_transport(
                 self.transport_from_client_path(path))
-        except errors.NotBranchError:
-            return FailedSmartServerResponse(('nobranch', ))
+        except errors.NotBranchError, e:
+            return FailedSmartServerResponse(('nobranch',))
         return self.do_bzrdir_request(*args)
 
     def _boolean_to_yes_no(self, a_boolean):
@@ -465,8 +465,8 @@
                 return SuccessfulSmartServerResponse(('ok', ''))
             else:
                 return SuccessfulSmartServerResponse(('ok', reference_url))
-        except errors.NotBranchError:
-            return FailedSmartServerResponse(('nobranch', ))
+        except errors.NotBranchError, e:
+            return FailedSmartServerResponse(('nobranch',))
 
 
 class SmartServerRequestOpenBranchV2(SmartServerRequestBzrDir):
@@ -481,5 +481,39 @@
                 return SuccessfulSmartServerResponse(('branch', format))
             else:
                 return SuccessfulSmartServerResponse(('ref', reference_url))
-        except errors.NotBranchError:
-            return FailedSmartServerResponse(('nobranch', ))
+        except errors.NotBranchError, e:
+            return FailedSmartServerResponse(('nobranch',))
+
+
+class SmartServerRequestOpenBranchV3(SmartServerRequestBzrDir):
+
+    def do_bzrdir_request(self):
+        """Open a branch at path and return the reference or format.
+        
+        This version introduced in 2.1.
+
+        Differences to SmartServerRequestOpenBranchV2:
+          * can return 2-element ('nobranch', extra), where 'extra' is a string
+            with an explanation like 'location is a repository'.  Previously
+            a 'nobranch' response would never have more than one element.
+        """
+        try:
+            reference_url = self._bzrdir.get_branch_reference()
+            if reference_url is None:
+                br = self._bzrdir.open_branch(ignore_fallbacks=True)
+                format = br._format.network_name()
+                return SuccessfulSmartServerResponse(('branch', format))
+            else:
+                return SuccessfulSmartServerResponse(('ref', reference_url))
+        except errors.NotBranchError, e:
+            # Stringify the exception so that its .detail attribute will be
+            # filled out.
+            str(e)
+            resp = ('nobranch',)
+            detail = e.detail
+            if detail:
+                if detail.startswith(': '):
+                    detail = detail[2:]
+                resp += (detail,)
+            return FailedSmartServerResponse(resp)
+

=== modified file 'bzrlib/smart/request.py'
--- bzrlib/smart/request.py	2009-12-21 17:00:29 +0000
+++ bzrlib/smart/request.py	2010-01-15 05:14:10 +0000
@@ -561,6 +561,9 @@
     'BzrDir.open_branchV2', 'bzrlib.smart.bzrdir',
     'SmartServerRequestOpenBranchV2')
 request_handlers.register_lazy(
+    'BzrDir.open_branchV3', 'bzrlib.smart.bzrdir',
+    'SmartServerRequestOpenBranchV3')
+request_handlers.register_lazy(
     'delete', 'bzrlib.smart.vfs', 'DeleteRequest')
 request_handlers.register_lazy(
     'get', 'bzrlib.smart.vfs', 'GetRequest')

=== 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	2010-01-15 05:14:10 +0000
@@ -18,7 +18,7 @@
 
 import os
 
-from bzrlib.bzrdir import BzrDir
+from bzrlib.bzrdir import BzrDir, BzrDirMetaFormat1
 import bzrlib.errors as errors
 from bzrlib.tests import TestCaseInTempDir
 
@@ -120,3 +120,21 @@
         # 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')
+        dir.open_repository() # there is a repository there
+        e = self.assertRaises(errors.NotBranchError, dir.open_branch)
+        self.assertContainsRe(str(e), "location is a repository")
+
+    def test_notification_on_branch_from_nonrepository(self):
+        fmt = BzrDirMetaFormat1()
+        t = self.get_transport()
+        t.mkdir('a')
+        dir = fmt.initialize_on_transport(t.clone('a'))
+        self.assertRaises(errors.NoRepositoryPresent, dir.open_repository)
+        e = self.assertRaises(errors.NotBranchError, dir.open_branch)
+        self.assertNotContainsRe(str(e), "location is a repository")

=== modified file 'bzrlib/tests/per_branch/test_push.py'
--- bzrlib/tests/per_branch/test_push.py	2009-09-24 05:31:23 +0000
+++ bzrlib/tests/per_branch/test_push.py	2010-01-15 05:14:10 +0000
@@ -414,7 +414,7 @@
         self.empty_branch.push(target)
         self.assertEqual(
             ['BzrDir.open_2.1',
-             'BzrDir.open_branchV2',
+             'BzrDir.open_branchV3',
              'BzrDir.find_repositoryV3',
              'Branch.get_stacked_on_url',
              'Branch.lock_write',

=== modified file 'bzrlib/tests/test_errors.py'
--- bzrlib/tests/test_errors.py	2010-01-08 06:33:05 +0000
+++ bzrlib/tests/test_errors.py	2010-01-15 05:14:10 +0000
@@ -623,6 +623,38 @@
         self.assertEqual(
             'Repository dummy repo cannot suspend a write group.', str(err))
 
+    def test_not_branch_no_args(self):
+        err = errors.NotBranchError('path')
+        self.assertEqual('Not a branch: "path".', str(err))
+
+    def test_not_branch_bzrdir_with_repo(self):
+        bzrdir = self.make_repository('repo').bzrdir
+        err = errors.NotBranchError('path', bzrdir=bzrdir)
+        self.assertEqual(
+            'Not a branch: "path": location is a repository.', str(err))
+
+    def test_not_branch_bzrdir_without_repo(self):
+        bzrdir = self.make_bzrdir('bzrdir')
+        err = errors.NotBranchError('path', bzrdir=bzrdir)
+        self.assertEqual('Not a branch: "path".', str(err))
+
+    def test_not_branch_laziness(self):
+        real_bzrdir = self.make_bzrdir('path')
+        class FakeBzrDir(object):
+            def __init__(self):
+                self.calls = []
+            def open_repository(self):
+                self.calls.append('open_repository')
+                raise errors.NoRepositoryPresent(real_bzrdir)
+        fake_bzrdir = FakeBzrDir()
+        err = errors.NotBranchError('path', bzrdir=fake_bzrdir)
+        self.assertEqual([], fake_bzrdir.calls)
+        str(err)
+        self.assertEqual(['open_repository'], fake_bzrdir.calls)
+        # Stringifying twice doesn't try to open a repository twice.
+        str(err)
+        self.assertEqual(['open_repository'], fake_bzrdir.calls)
+
 
 class PassThroughError(errors.BzrError):
 

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2009-12-16 22:29:31 +0000
+++ bzrlib/tests/test_remote.py	2010-01-15 05:14:10 +0000
@@ -440,7 +440,7 @@
             'BzrDir.cloning_metadir', ('quack/', 'False'),
             'error', ('BranchReference',)),
         client.add_expected_call(
-            'BzrDir.open_branchV2', ('quack/',),
+            'BzrDir.open_branchV3', ('quack/',),
             'success', ('ref', self.get_url('referenced'))),
         a_bzrdir = RemoteBzrDir(transport, remote.RemoteBzrDirFormat(),
             _client=client)
@@ -531,7 +531,7 @@
         self.make_branch('.')
         a_dir = BzrDir.open(self.get_url('.'))
         self.reset_smart_call_log()
-        verb = 'BzrDir.open_branchV2'
+        verb = 'BzrDir.open_branchV3'
         self.disable_verb(verb)
         format = a_dir.open_branch()
         call_count = len([call for call in self.hpss_calls if
@@ -547,7 +547,7 @@
         transport = transport.clone('quack')
         client = FakeClient(transport.base)
         client.add_expected_call(
-            'BzrDir.open_branchV2', ('quack/',),
+            'BzrDir.open_branchV3', ('quack/',),
             'success', ('branch', branch_network_name))
         client.add_expected_call(
             'BzrDir.find_repositoryV3', ('quack/',),
@@ -572,7 +572,7 @@
             _client=client)
         self.assertRaises(errors.NotBranchError, bzrdir.open_branch)
         self.assertEqual(
-            [('call', 'BzrDir.open_branchV2', ('quack/',))],
+            [('call', 'BzrDir.open_branchV3', ('quack/',))],
             client._calls)
 
     def test__get_tree_branch(self):
@@ -602,7 +602,7 @@
         network_name = reference_format.network_name()
         branch_network_name = self.get_branch_format().network_name()
         client.add_expected_call(
-            'BzrDir.open_branchV2', ('~hello/',),
+            'BzrDir.open_branchV3', ('~hello/',),
             'success', ('branch', branch_network_name))
         client.add_expected_call(
             'BzrDir.find_repositoryV3', ('~hello/',),
@@ -1190,7 +1190,7 @@
         client = FakeClient(self.get_url())
         branch_network_name = self.get_branch_format().network_name()
         client.add_expected_call(
-            'BzrDir.open_branchV2', ('stacked/',),
+            'BzrDir.open_branchV3', ('stacked/',),
             'success', ('branch', branch_network_name))
         client.add_expected_call(
             'BzrDir.find_repositoryV3', ('stacked/',),
@@ -1226,7 +1226,7 @@
         client = FakeClient(self.get_url())
         branch_network_name = self.get_branch_format().network_name()
         client.add_expected_call(
-            'BzrDir.open_branchV2', ('stacked/',),
+            'BzrDir.open_branchV3', ('stacked/',),
             'success', ('branch', branch_network_name))
         client.add_expected_call(
             'BzrDir.find_repositoryV3', ('stacked/',),
@@ -2775,6 +2775,15 @@
         expected_error = errors.NotBranchError(path=bzrdir.root_transport.base)
         self.assertEqual(expected_error, translated_error)
 
+    def test_nobranch_one_arg(self):
+        bzrdir = self.make_bzrdir('')
+        translated_error = self.translateTuple(
+            ('nobranch', 'extra detail'), bzrdir=bzrdir)
+        expected_error = errors.NotBranchError(
+            path=bzrdir.root_transport.base,
+            detail='extra detail')
+        self.assertEqual(expected_error, translated_error)
+
     def test_LockContention(self):
         translated_error = self.translateTuple(('LockContention',))
         expected_error = errors.LockContention('(remote lock)')

=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py	2009-10-29 00:16:50 +0000
+++ bzrlib/tests/test_smart.py	2010-01-15 05:14:10 +0000
@@ -524,6 +524,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',)),
+            request.execute(''))
+
 
 class TestSmartServerRequestOpenBranchV2(TestCaseWithChrootedTransport):
 
@@ -575,6 +583,74 @@
             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',)),
+            request.execute(''))
+
+
+class TestSmartServerRequestOpenBranchV3(TestCaseWithChrootedTransport):
+
+    def test_no_branch(self):
+        """When there is no branch, ('nobranch', ) is returned."""
+        backing = self.get_transport()
+        self.make_bzrdir('.')
+        request = smart.bzrdir.SmartServerRequestOpenBranchV3(backing)
+        self.assertEqual(SmartServerResponse(('nobranch',)),
+            request.execute(''))
+
+    def test_branch(self):
+        """When there is a branch, 'ok' is returned."""
+        backing = self.get_transport()
+        expected = self.make_branch('.')._format.network_name()
+        request = smart.bzrdir.SmartServerRequestOpenBranchV3(backing)
+        self.assertEqual(SuccessfulSmartServerResponse(('branch', expected)),
+            request.execute(''))
+
+    def test_branch_reference(self):
+        """When there is a branch reference, the reference URL is returned."""
+        self.vfs_transport_factory = local.LocalURLServer
+        backing = self.get_transport()
+        request = smart.bzrdir.SmartServerRequestOpenBranchV3(backing)
+        branch = self.make_branch('branch')
+        checkout = branch.create_checkout('reference',lightweight=True)
+        reference_url = BranchReferenceFormat().get_reference(checkout.bzrdir)
+        self.assertFileEqual(reference_url, 'reference/.bzr/branch/location')
+        self.assertEqual(SuccessfulSmartServerResponse(('ref', reference_url)),
+            request.execute('reference'))
+
+    def test_stacked_branch(self):
+        """Opening a stacked branch does not open the stacked-on branch."""
+        trunk = self.make_branch('trunk')
+        feature = self.make_branch('feature')
+        feature.set_stacked_on_url(trunk.base)
+        opened_branches = []
+        Branch.hooks.install_named_hook('open', opened_branches.append, None)
+        backing = self.get_transport()
+        request = smart.bzrdir.SmartServerRequestOpenBranchV3(backing)
+        request.setup_jail()
+        try:
+            response = request.execute('feature')
+        finally:
+            request.teardown_jail()
+        expected_format = feature._format.network_name()
+        self.assertEqual(
+            SuccessfulSmartServerResponse(('branch', expected_format)),
+            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.SmartServerRequestOpenBranchV3(backing)
+        repo = self.make_repository('.')
+        self.assertEqual(
+            SmartServerResponse(('nobranch', 'location is a repository')),
+            request.execute(''))
+
 
 class TestSmartServerRequestRevisionHistory(tests.TestCaseWithMemoryTransport):
 
@@ -1771,6 +1847,8 @@
             smart.bzrdir.SmartServerRequestOpenBranch)
         self.assertHandlerEqual('BzrDir.open_branchV2',
             smart.bzrdir.SmartServerRequestOpenBranchV2)
+        self.assertHandlerEqual('BzrDir.open_branchV3',
+            smart.bzrdir.SmartServerRequestOpenBranchV3)
         self.assertHandlerEqual('PackRepository.autopack',
             smart.packrepository.SmartServerPackRepositoryAutopack)
         self.assertHandlerEqual('Repository.gather_stats',