Merge lp:~mbp/bzr/687653-notbrancherror into lp:bzr/2.2

[Martin Pool]

As discussed in <https://bugs.launchpad.net/bzr/+bug/687653> I think doing (possibly network) IO from an error while it's formatted is a pretty bad idea, and likely to lead to cascading errors. Instead this checks for a repository at the time we fail to open the branch. It's possible this sometimes does too much work, but I think this shouldn't be a very common case, since it will only be hit when we already have found a bzrdir.

I'm targeting 2.2 so this can be rolled out on Launchpad.

This is a slight change in API behaviour because the bzrdir parameter is now ignored, but I think not really a break.

[lp:~mbp/bzr/687653-notbrancherror]

5119. By Martin Pool on 2010-12-10

Import urlutils just once into errors.py

[Andrew Bennetts (spiv)]

I'm a bit worried about the performance impact. grepping for places that catch NotBranchError, and then eyeballing them to ignore places that are actually catching NotBranchError from opening a BzrDir rather than a branch, I see a fair few places that are likely to be incurring extra round-trips. The most serious is probably cmd_switch. Other places are less common and/or performance sensitive situations, like cmd_pack, cmd_whoami, BzrDir.sprout where the bzrdir has a repo but no branch.

I think all the HPSS effort tests still pass, so that's a good sign. But it definitely smells a bit, so we're trading one smell for another... probably an ok tradeoff, but it does make me hesitate.

On the plus side you can remove the str(e) call from SmartServerRequestOpenBranchV3.

If the test suite passes it's definitely safe for Launchpad to cherrypick.

[Andrew Bennetts (spiv)]

An alternative fix... just add:

    def __repr__(self):
        return '<%s %r>' % (self.__class__.__name__, self.__dict__)

The problem is that repr ought to be safe (and thus simple), and currently it can trigger arbitrary work, which allows this bug to happen.

Code that calls str(e) will still get a string suitable for displaying to users — including the "is a repository" hint.

[Martin Pool (mbp)]

I'm going to reject this in favor of spiv's fix in 2.2. In 2.3, I'm inclined to think we should just rip it out; the ui benefit is small and doing things from an exception class is bad.

Unmerged revisions

5119. By Martin Pool on 2010-12-10

Import urlutils just once into errors.py

5118. By Martin Pool on 2010-12-10

Smooth off handling of details and bzrdir in NotBranchError and update tests

5117. By Martin Pool on 2010-12-10

Remove misleading unused variables

5116. By Martin Pool on 2010-12-10

Trim unused import

5115. By Martin Pool on 2010-12-10

Don't probe for a repository from within formatting of a `NotBranchError`,
because this can cause knock-on errors. Instead, in cases where it would be
useful to tell the users that a directory contains a repository but not a
branch, check for it when raising the error.

=== modified file 'NEWS'
--- NEWS	2010-12-02 09:29:41 +0000
+++ NEWS	2010-12-10 03:22:11 +0000
@@ -25,6 +25,13 @@
 Bug Fixes
 *********
 
+* Don't probe for a repository from within formatting of a
+  `NotBranchError`, because this can cause knock-on errors.  Instead, in
+  cases where it would be useful to tell the users that a directory
+  contains a repository but not a branch, check for it when raising the
+  error.
+  (Martin Pool, 687653)
+
 * RevisionTree.is_executable no longer returns None for directories and
   symlinks.  Instead, it returns False, like other Trees and methods.
   (Aaron Bentley, #681885)

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2010-08-13 07:32:06 +0000
+++ bzrlib/branch.py	2010-12-10 03:22:11 +0000
@@ -35,7 +35,6 @@
         rio,
         symbol_versioning,
         transport,
-        tsort,
         ui,
         urlutils,
         )
@@ -1555,7 +1554,15 @@
                 return format()
             return format
         except errors.NoSuchFile:
-            raise errors.NotBranchError(path=transport.base, bzrdir=a_bzrdir)
+            try:
+                # Give a clearer message if the bzrdir has a repository but no
+                # branch: see bug 440952.
+                a_bzrdir.open_repository()
+            except errors.NoRepositoryPresent:
+                detail = None
+            else:
+                detail = 'location is a repository'
+            raise errors.NotBranchError(path=transport.base, detail=detail)
         except KeyError:
             raise errors.UnknownFormatError(format=format_string, kind='branch')
 
@@ -2069,7 +2076,15 @@
                               _repository=a_bzrdir.find_repository(),
                               ignore_fallbacks=ignore_fallbacks)
         except errors.NoSuchFile:
-            raise errors.NotBranchError(path=transport.base, bzrdir=a_bzrdir)
+            try:
+                # Give a clearer message if the bzrdir has a repository but no
+                # branch: see bug 440952.
+                a_bzrdir.open_repository()
+            except errors.NoRepositoryPresent:
+                detail = None
+            else:
+                detail = 'location is a repository'
+            raise errors.NotBranchError(path=transport.base, detail=detail)
 
     def __init__(self):
         super(BranchFormatMetadir, self).__init__()
@@ -2255,7 +2270,7 @@
     def set_reference(self, a_bzrdir, name, to_branch):
         """See BranchFormat.set_reference()."""
         transport = a_bzrdir.get_branch_transport(None, name=name)
-        location = transport.put_bytes('location', to_branch.base)
+        transport.put_bytes('location', to_branch.base)
 
     def initialize(self, a_bzrdir, name=None, target_branch=None):
         """Create a branch of this format in a_bzrdir."""
@@ -2959,7 +2974,6 @@
 
     def set_bound_location(self, location):
         """See Branch.set_push_location."""
-        result = None
         config = self.get_config()
         if location is None:
             if config.get_user_option('bound') != 'True':

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-10-11 05:06:26 +0000
+++ bzrlib/errors.py	2010-12-10 03:22:11 +0000
@@ -20,6 +20,7 @@
 from bzrlib import (
     osutils,
     symbol_versioning,
+    urlutils,
     )
 from bzrlib.patches import (
     MalformedHunkHeader,
@@ -312,7 +313,6 @@
     _fmt = 'There is no public branch set for "%(branch_url)s".'
 
     def __init__(self, branch):
-        import bzrlib.urlutils as urlutils
         public_location = urlutils.unescape_for_display(branch.base, 'ascii')
         BzrError.__init__(self, branch_url=public_location)
 
@@ -698,45 +698,27 @@
 
 
 # TODO: This is given a URL; we try to unescape it but doing that from inside
-# the exception object is a bit undesirable.
-# TODO: Probably this behavior of should be a common superclass
+# the exception object is a bit undesirable.  We should instead either tell
+# the exception the encoding we want it formatted into, or let the UIFactory
+# perform the encoding (if any.)
 class NotBranchError(PathError):
 
     _fmt = 'Not a branch: "%(path)s"%(detail)s.'
 
     def __init__(self, path, detail=None, bzrdir=None):
-       import bzrlib.urlutils as urlutils
-       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)
+        """Construct an error.
 
-    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 = ''
-                except Exception:
-                    # Just ignore unexpected errors.  Raising arbitrary errors
-                    # during str(err) can provoke strange bugs.  Concretely
-                    # Launchpad's codehosting managed to raise NotBranchError
-                    # here, and then get stuck in an infinite loop/recursion
-                    # trying to str() that error.  All this error really cares
-                    # about that there's no working repository there, and if
-                    # open_repository() fails, there probably isn't.
-                    self.detail = ''
-                else:
-                    self.detail = ': location is a repository'
-            else:
-                self.detail = ''
-        return PathError._format(self)
+        :param detail: Extra string to append to the message, explaining 
+            why it was not a branch.
+        :param bzrdir: Deprecated/ignored.
+        """
+        path = urlutils.unescape_for_display(path, 'ascii')
+        if detail is not None:
+            self.detail = ': ' + detail
+        else:
+            self.detail = ''
+        self.bzrdir = bzrdir
+        PathError.__init__(self, path=path)
 
 
 class NoSubmitBranch(PathError):
@@ -744,7 +726,6 @@
     _fmt = 'No submit branch available for branch "%(path)s"'
 
     def __init__(self, branch):
-       import bzrlib.urlutils as urlutils
        self.path = urlutils.unescape_for_display(branch.base, 'ascii')
 
 
@@ -1218,7 +1199,6 @@
            ' branch "%(location)s".')
 
     def __init__(self, location):
-       import bzrlib.urlutils as urlutils
        location = urlutils.unescape_for_display(location, 'ascii')
        BzrError.__init__(self, location=location)
 
@@ -2168,7 +2148,6 @@
         '"%(revstring)s".'
 
     def __init__(self, public_location, revstring):
-        import bzrlib.urlutils as urlutils
         public_location = urlutils.unescape_for_display(public_location,
                                                         'ascii')
         BzrError.__init__(self, public_location=public_location,
@@ -2780,7 +2759,6 @@
 class BzrDirError(BzrError):
 
     def __init__(self, bzrdir):
-        import bzrlib.urlutils as urlutils
         display_url = urlutils.unescape_for_display(bzrdir.user_url,
                                                     'ascii')
         BzrError.__init__(self, bzrdir=bzrdir, display_url=display_url)
@@ -2793,7 +2771,6 @@
 
     def __init__(self, bzrdir, target_branch):
         BzrDirError.__init__(self, bzrdir)
-        import bzrlib.urlutils as urlutils
         self.target_url = urlutils.unescape_for_display(target_branch.base,
                                                         'ascii')
 
@@ -2860,7 +2837,6 @@
             more = ''
         else:
             more = ' ' + more
-        import bzrlib.urlutils as urlutils
         display_url = urlutils.unescape_for_display(
             tree.user_url, 'ascii')
         BzrError.__init__(self, tree=tree, display_url=display_url, more=more)

=== modified file 'bzrlib/tests/test_errors.py'
--- bzrlib/tests/test_errors.py	2010-12-02 09:23:10 +0000
+++ bzrlib/tests/test_errors.py	2010-12-10 03:22:11 +0000
@@ -661,43 +661,11 @@
         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)
+    def test_not_branch_bzrdir_with_details(self):
+        err = errors.NotBranchError('path', detail='location is a repository')
         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_bzrdir_with_recursive_not_branch_error(self):
-        class FakeBzrDir(object):
-            def open_repository(self):
-                # str() on the NotBranchError will trigger a call to this,
-                # which in turn will another, identical NotBranchError.
-                raise errors.NotBranchError('path', bzrdir=FakeBzrDir())
-        err = errors.NotBranchError('path', bzrdir=FakeBzrDir())
-        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)
-
     def test_invalid_pattern(self):
         error = errors.InvalidPattern('Bad pattern msg.')
         self.assertEqualDiff("Invalid pattern(s) found. Bad pattern msg.",