Merge lp:~spiv/bzr/notbrancherror-recursion-2.2 into lp:bzr/2.2

[Andrew Bennetts]

Don't propagate unexpected exceptions during NotBranchError.__str__

This adds a bit of safety to NotBranchError. Tim Penhey encountered a situation where the Launchpad codehosting server would fall into an infinite loop because str() of a NotBranchError would trigger another NotBranchError.

Strictly speaking an infinite loop in that situation is a bug in the caller, but I don't see any harm in suppressing unexpected errors at that point. The open_repository call is only there to provide a hint in the str-ified error, and an error is enough to decide the hint isn't needed. And not obliterating the original error is likely to be more useful.

If we wanted to be extra conservative, we could just trap NotBranchError rather than Exception at that point, to only deal with the specific reported issue. But I think we may as well fix the slightly more general issue.

[Martin Pool (mbp)]

review: Approve

[Andrew Bennetts (spiv)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-10-08 10:09:37 +0000
+++ NEWS	2010-10-11 05:14:40 +0000
@@ -19,6 +19,10 @@
 Bug Fixes
 *********
 
+* ``NotBranchError`` no longer allows errors from calling
+  ``bzrdir.open_repository()`` to propagate.  This is unhelpful at best,
+  and at worst can trigger infinite loops in callers.  (Andrew Bennetts)
+  
 * Skip tests that needs a bzr source tree when there isn't one. This is
   needed to succesfully run the test suite for installed versions.
   (Vincent Ladeuil, #644855).

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-09-13 02:32:44 +0000
+++ bzrlib/errors.py	2010-10-11 05:14:40 +0000
@@ -723,6 +723,15 @@
                     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:

=== modified file 'bzrlib/tests/test_errors.py'
--- bzrlib/tests/test_errors.py	2010-09-13 02:32:44 +0000
+++ bzrlib/tests/test_errors.py	2010-10-11 05:14:40 +0000
@@ -672,6 +672,15 @@
         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):