Merge lp:~gz/brz/py3_errors into lp:brz

[Martin Packman]

Improve Python 3 compatibility of errors module

Selection of changes that move breezy.errors towards Python 3 compatibility.

Implement __hash__ which is now required on classes that have an __eq__.

Only provide __str__/__unicode__ methods as appropriate.

In the tests, moved out errors that actually require a transport to a separate class, and fix a few other suspect assertions.

[Jelmer Vernooij (jelmer)]

review: Approve

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
http://10.242.247.184:8080/job/brz-dev/97/

[Martin Packman (gz)]

Ended up doing rather more changed than intended to inventory_delta based on the one test failure there, so this branch could probably do with another quick review.

=== modified file 'breezy/errors.py'
--- breezy/errors.py	2017-05-22 00:56:52 +0000
+++ breezy/errors.py	2017-06-09 16:32:43 +0000
@@ -19,6 +19,10 @@
 
 from __future__ import absolute_import
 
+from .sixish import (
+    PY3,
+    )
+
 # TODO: is there any value in providing the .args field used by standard
 # python exceptions?   A list of values with no names seems less useful
 # to me.
@@ -87,6 +91,7 @@
         if s is not None:
             # contains a preformatted message
             return s
+        err = None
         try:
             fmt = self._get_format_string()
             if fmt:
@@ -96,34 +101,20 @@
                 # never a 'unicode' object.
                 return s
         except Exception as e:
-            pass # just bind to 'e' for formatting below
-        else:
-            e = None
+            err = e
         return 'Unprintable exception %s: dict=%r, fmt=%r, error=%r' \
             % (self.__class__.__name__,
                self.__dict__,
                getattr(self, '_fmt', None),
-               e)
-
-    def __unicode__(self):
-        u = self._format()
-        if isinstance(u, str):
-            # Try decoding the str using the default encoding.
-            u = unicode(u)
-        elif not isinstance(u, unicode):
-            # Try to make a unicode object from it, because __unicode__ must
-            # return a unicode object.
-            u = unicode(u)
-        return u
-
-    def __str__(self):
-        s = self._format()
-        if isinstance(s, unicode):
-            s = s.encode('utf8')
-        else:
-            # __str__ must return a str.
-            s = str(s)
-        return s
+               err)
+
+    if PY3:
+        __str__ = _format
+    else:
+        def __str__(self):
+            return self._format().encode('utf-8')
+
+        __unicode__ = _format
 
     def __repr__(self):
         return '%s(%s)' % (self.__class__.__name__, str(self))
@@ -133,13 +124,16 @@
         fmt = getattr(self, '_fmt', None)
         if fmt is not None:
             from breezy.i18n import gettext
-            return gettext(unicode(fmt)) # _fmt strings should be ascii
+            return gettext(fmt) # _fmt strings should be ascii
 
     def __eq__(self, other):
         if self.__class__ is not other.__class__:
             return NotImplemented
         return self.__dict__ == other.__dict__
 
+    def __hash__(self):
+        return id(self)
+
 
 class InternalBzrError(BzrError):
     """Base class for errors that are internal in nature.
@@ -665,30 +659,30 @@
     def __repr__(self):
         return '<%s %r>' % (self.__class__.__name__, self.__dict__)
 
-    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.
+    def _get_format_string(self):
+        # GZ 2017-06-08: Not the best place to lazy fill detail in.
         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'
+           self.detail = self._get_detail()
+        return super(NotBranchError, self)._get_format_string()
+
+    def _get_detail(self):
+        if self.bzrdir is not None:
+            try:
+                self.bzrdir.open_repository()
+            except NoRepositoryPresent:
+                return ''
+            except Exception as e:
+                # 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.
+                return ': ' + e.__class__.__name__
             else:
-                self.detail = ''
-        return PathError._format(self)
+                return ': location is a repository'
+        return ''
 
 
 class NoSubmitBranch(PathError):

=== modified file 'breezy/inventory_delta.py'
--- breezy/inventory_delta.py	2017-05-25 01:35:55 +0000
+++ breezy/inventory_delta.py	2017-06-09 16:32:43 +0000
@@ -36,12 +36,17 @@
 
 class InventoryDeltaError(errors.BzrError):
     """An error when serializing or deserializing an inventory delta."""
-    
+
     # Most errors when serializing and deserializing are due to bugs, although
     # damaged input (i.e. a bug in a different process) could cause
     # deserialization errors too.
     internal_error = True
 
+    def __init__(self, format_string, **kwargs):
+        # Let each error supply a custom format string and arguments.
+        self._fmt = format_string
+        super(InventoryDeltaError, self).__init__(**kwargs)
+
 
 class IncompatibleInventoryDelta(errors.BzrError):
     """The delta could not be deserialised because its contents conflict with
@@ -70,7 +75,8 @@
         exec_bytes = ''
     size_exec_sha = (entry.text_size, exec_bytes, entry.text_sha1)
     if None in size_exec_sha:
-        raise InventoryDeltaError('Missing size or sha for %s' % entry.file_id)
+        raise InventoryDeltaError(
+            'Missing size or sha for %(fileid)r', fileid=entry.file_id)
     return "file\x00%d\x00%s\x00%s" % size_exec_sha
 
 
@@ -81,7 +87,8 @@
     """
     target = entry.symlink_target
     if target is None:
-        raise InventoryDeltaError('Missing target for %s' % entry.file_id)
+        raise InventoryDeltaError(
+            'Missing target for %(fileid)r', fileid=entry.file_id)
     return "link\x00%s" % target.encode('utf8')
 
 
@@ -93,7 +100,7 @@
     tree_revision = entry.reference_revision
     if tree_revision is None:
         raise InventoryDeltaError(
-            'Missing reference revision for %s' % entry.file_id)
+            'Missing reference revision for %(fileid)r', fileid=entry.file_id)
     return "tree\x00%s" % tree_revision
 
 
@@ -180,9 +187,10 @@
         to_line = self._delta_item_to_line
         for delta_item in delta_to_new:
             line = to_line(delta_item, new_name)
-            if line.__class__ != str:
+            # GZ 2017-06-09: Not really worth asserting this here
+            if line.__class__ != bytes:
                 raise InventoryDeltaError(
-                    'to_line generated non-str output %r' % lines[-1])
+                    'to_line gave non-bytes output %(line)r', line=lines[-1])
             lines.append(line)
         lines.sort()
         lines[0] = "format: %s\n" % FORMAT_1
@@ -236,10 +244,12 @@
                 # xml5 serializer.
                 if last_modified != new_version:
                     raise InventoryDeltaError(
-                        'Version present for / in %s (%s != %s)'
-                        % (file_id, last_modified, new_version))
+                        'Version present for / in %(fileid)r'
+                        ' (%(last)r != %(new)r)',
+                        fileid=file_id, last=last_modified, new=new_version)
             if last_modified is None:
-                raise InventoryDeltaError("no version for fileid %s" % file_id)
+                raise InventoryDeltaError(
+                    "no version for fileid %(fileid)r", fileid=file_id)
             content = self._entry_to_content[entry.kind](entry)
         return ("%s\x00%s\x00%s\x00%s\x00%s\x00%s\n" %
             (oldpath_utf8, newpath_utf8, file_id, parent_id, last_modified,
@@ -265,7 +275,7 @@
         elif value == "false":
             return False
         else:
-            raise InventoryDeltaError("value %r is not a bool" % (value,))
+            raise InventoryDeltaError("value %(val)r is not a bool", val=value)
 
     def parse_text_bytes(self, bytes):
         """Parse the text bytes of a serialized inventory delta.
@@ -281,10 +291,12 @@
         """
         if bytes[-1:] != '\n':
             last_line = bytes.rsplit('\n', 1)[-1]
-            raise InventoryDeltaError('last line not empty: %r' % (last_line,))
+            raise InventoryDeltaError(
+                'last line not empty: %(line)r', line=last_line)
         lines = bytes.split('\n')[:-1] # discard the last empty line
         if not lines or lines[0] != 'format: %s' % FORMAT_1:
-            raise InventoryDeltaError('unknown format %r' % lines[0:1])
+            raise InventoryDeltaError(
+                'unknown format %(line)r', line=lines[0:1])
         if len(lines) < 2 or not lines[1].startswith('parent: '):
             raise InventoryDeltaError('missing parent: marker')
         delta_parent_id = lines[1][8:]
@@ -310,22 +322,24 @@
             parent_id = parent_id or None
             if file_id in seen_ids:
                 raise InventoryDeltaError(
-                    "duplicate file id in inventory delta %r" % lines)
+                    "duplicate file id %(fileid)r", fileid=file_id)
             seen_ids.add(file_id)
             if (newpath_utf8 == '/' and not delta_versioned_root and
                 last_modified != delta_version_id):
                     # Delta claims to be not have a versioned root, yet here's
                     # a root entry with a non-default version.
-                    raise InventoryDeltaError("Versioned root found: %r" % line)
+                    raise InventoryDeltaError(
+                        "Versioned root found: %(line)r", line=line)
             elif newpath_utf8 != 'None' and last_modified[-1] == ':':
                 # Deletes have a last_modified of null:, but otherwise special
                 # revision ids should not occur.
-                raise InventoryDeltaError('special revisionid found: %r' % line)
+                raise InventoryDeltaError(
+                    'special revisionid found: %(line)r', line=line)
             if content.startswith('tree\x00'):
                 if delta_tree_references is False:
                     raise InventoryDeltaError(
                             "Tree reference found (but header said "
-                            "tree_references: false): %r" % line)
+                            "tree_references: false): %(line)r", line=line)
                 elif not self._allow_tree_references:
                     raise IncompatibleInventoryDelta(
                         "Tree reference not allowed")
@@ -333,8 +347,8 @@
                 oldpath = None
             elif oldpath_utf8[:1] != '/':
                 raise InventoryDeltaError(
-                    "oldpath invalid (does not start with /): %r"
-                    % (oldpath_utf8,))
+                    "oldpath invalid (does not start with /): %(path)r",
+                    path=oldpath_utf8)
             else:
                 oldpath_utf8 = oldpath_utf8[1:]
                 oldpath = oldpath_utf8.decode('utf8')
@@ -342,8 +356,8 @@
                 newpath = None
             elif newpath_utf8[:1] != '/':
                 raise InventoryDeltaError(
-                    "newpath invalid (does not start with /): %r"
-                    % (newpath_utf8,))
+                    "newpath invalid (does not start with /): %(path)r",
+                    path=newpath_utf8)
             else:
                 # Trim leading slash
                 newpath_utf8 = newpath_utf8[1:]

=== modified file 'breezy/tests/test_errors.py'
--- breezy/tests/test_errors.py	2017-05-22 00:56:52 +0000
+++ breezy/tests/test_errors.py	2017-06-09 16:32:43 +0000
@@ -25,16 +25,15 @@
     controldir,
     errors,
     osutils,
+    tests,
     urlutils,
     )
-from . import (
-    TestCase,
-    TestCaseWithTransport,
-    TestSkipped,
+from ..sixish import (
+    text_type,
     )
 
 
-class TestErrors(TestCaseWithTransport):
+class TestErrors(tests.TestCase):
 
     def test_no_arg_named_message(self):
         """Ensure the __init__ and _fmt in errors do not have "message" arg.
@@ -45,10 +44,6 @@
         See bug #603461
         """
         fmt_pattern = re.compile("%\(message\)[sir]")
-        subclasses_present = getattr(errors.BzrError, '__subclasses__', None)
-        if not subclasses_present:
-            raise TestSkipped('__subclasses__ attribute required for classes. '
-                'Requires Python 2.5 or later.')
         for c in errors.BzrError.__subclasses__():
             init = getattr(c, '__init__', None)
             fmt = getattr(c, '_fmt', None)
@@ -62,11 +57,11 @@
                     '"errors.%s._fmt"' % c.__name__))
 
     def test_bad_filename_encoding(self):
-        error = errors.BadFilenameEncoding('bad/filen\xe5me', 'UTF-8')
-        self.assertEqualDiff(
-            "Filename 'bad/filen\\xe5me' is not valid in your current"
-            " filesystem encoding UTF-8",
-            str(error))
+        error = errors.BadFilenameEncoding(b'bad/filen\xe5me', 'UTF-8')
+        self.assertContainsRe(
+            str(error),
+            "^Filename b?'bad/filen\\\\xe5me' is not valid in your current"
+            " filesystem encoding UTF-8$")
 
     def test_corrupt_dirstate(self):
         error = errors.CorruptDirstate('path/to/dirstate', 'the reason why')
@@ -199,19 +194,6 @@
         self.assertEqualDiff(
             "The medium 'a medium' is not connected.", str(error))
 
-    def test_no_public_branch(self):
-        b = self.make_branch('.')
-        error = errors.NoPublicBranch(b)
-        url = urlutils.unescape_for_display(b.base, 'ascii')
-        self.assertEqualDiff(
-            'There is no public branch set for "%s".' % url, str(error))
-
-    def test_no_repo(self):
-        dir = controldir.ControlDir.create(self.get_url())
-        error = errors.NoRepositoryPresent(dir)
-        self.assertNotEqual(-1, str(error).find((dir.transport.clone('..').base)))
-        self.assertEqual(-1, str(error).find((dir.transport.base)))
-
     def test_no_smart_medium(self):
         error = errors.NoSmartMedium("a transport")
         self.assertEqualDiff("The transport 'a transport' cannot tunnel the "
@@ -305,19 +287,11 @@
             "The branch format someformat is already at the most "
             "recent format.", str(error))
 
-    def test_corrupt_repository(self):
-        repo = self.make_repository('.')
-        error = errors.CorruptRepository(repo)
-        self.assertEqualDiff("An error has been detected in the repository %s.\n"
-                             "Please run brz reconcile on this repository." %
-                             repo.bzrdir.root_transport.base,
-                             str(error))
-
     def test_read_error(self):
         # a unicode path to check that %r is being used.
         path = u'a path'
         error = errors.ReadError(path)
-        self.assertEqualDiff("Error reading from u'a path'.", str(error))
+        self.assertContainsRe(str(error), "^Error reading from u?'a path'.$")
 
     def test_bad_index_format_signature(self):
         error = errors.BadIndexFormatSignature("foo", "bar")
@@ -445,14 +419,6 @@
             'See "brz help bugs" for more information on this feature.',
             str(error))
 
-    def test_unknown_bug_tracker_abbreviation(self):
-        """Test the formatting of UnknownBugTrackerAbbreviation."""
-        branch = self.make_branch('some_branch')
-        error = errors.UnknownBugTrackerAbbreviation('xxx', branch)
-        self.assertEqual(
-            "Cannot find registered bug tracker called xxx on %s" % branch,
-            str(error))
-
     def test_unexpected_smart_server_response(self):
         e = errors.UnexpectedSmartServerResponse(('not yes',))
         self.assertEqual(
@@ -495,14 +461,12 @@
 
     def test_duplicate_record_name_error(self):
         """Test the formatting of DuplicateRecordNameError."""
-        e = errors.DuplicateRecordNameError(u"n\xe5me".encode('utf-8'))
+        e = errors.DuplicateRecordNameError(b"n\xc3\xa5me")
         self.assertEqual(
-            "Container has multiple records with the same name: n\xc3\xa5me",
-            str(e))
+            u"Container has multiple records with the same name: n\xe5me",
+            text_type(e))
 
     def test_check_error(self):
-        # This has a member called 'message', which is problematic in
-        # python2.5 because that is a slot on the base Exception class
         e = errors.BzrCheckError('example check failure')
         self.assertEqual(
             "Internal check failed: example check failure",
@@ -534,7 +498,7 @@
             str(err))
         err = errors.UnableCreateSymlink(path=u'\xb5')
         self.assertEqual(
-            "Unable to create symlink u'\\xb5' on this platform",
+            "Unable to create symlink %s on this platform" % repr(u'\xb5'),
             str(err))
 
     def test_invalid_url_join(self):
@@ -569,10 +533,7 @@
         err = errors.TipChangeRejected(u'Unicode message\N{INTERROBANG}')
         self.assertEqual(
             u'Tip change rejected: Unicode message\N{INTERROBANG}',
-            unicode(err))
-        self.assertEqual(
-            'Tip change rejected: Unicode message\xe2\x80\xbd',
-            str(err))
+            text_type(err))
 
     def test_error_from_smart_server(self):
         error_tuple = ('error', 'tuple')
@@ -647,17 +608,6 @@
         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_bzrdir_with_recursive_not_branch_error(self):
         class FakeBzrDir(object):
             def open_repository(self):
@@ -665,24 +615,7 @@
                 # 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)
+        self.assertEqual('Not a branch: "path": NotBranchError.', str(err))
 
     def test_invalid_pattern(self):
         error = errors.InvalidPattern('Bad pattern msg.')
@@ -721,7 +654,7 @@
     __doc__ = """This class has a docstring but no format string."""
 
 
-class TestErrorFormatting(TestCase):
+class TestErrorFormatting(tests.TestCase):
 
     def test_always_str(self):
         e = PassThroughError(u'\xb5', 'bar')
@@ -764,3 +697,64 @@
         self.assertEqual(
             u"Failed to rename from to to: readonly file",
             str(e))
+
+
+class TestErrorsUsingTransport(tests.TestCaseWithMemoryTransport):
+    """Tests for errors that need to use a branch or repo."""
+
+    def test_no_public_branch(self):
+        b = self.make_branch('.')
+        error = errors.NoPublicBranch(b)
+        url = urlutils.unescape_for_display(b.base, 'ascii')
+        self.assertEqualDiff(
+            'There is no public branch set for "%s".' % url, str(error))
+
+    def test_no_repo(self):
+        dir = controldir.ControlDir.create(self.get_url())
+        error = errors.NoRepositoryPresent(dir)
+        self.assertNotEqual(-1, str(error).find((dir.transport.clone('..').base)))
+        self.assertEqual(-1, str(error).find((dir.transport.base)))
+
+    def test_corrupt_repository(self):
+        repo = self.make_repository('.')
+        error = errors.CorruptRepository(repo)
+        self.assertEqualDiff("An error has been detected in the repository %s.\n"
+                             "Please run brz reconcile on this repository." %
+                             repo.bzrdir.root_transport.base,
+                             str(error))
+
+    def test_unknown_bug_tracker_abbreviation(self):
+        """Test the formatting of UnknownBugTrackerAbbreviation."""
+        branch = self.make_branch('some_branch')
+        error = errors.UnknownBugTrackerAbbreviation('xxx', branch)
+        self.assertEqual(
+            "Cannot find registered bug tracker called xxx on %s" % branch,
+            str(error))
+
+    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)

=== modified file 'breezy/tests/test_inventory_delta.py'
--- breezy/tests/test_inventory_delta.py	2017-05-22 00:56:52 +0000
+++ breezy/tests/test_inventory_delta.py	2017-06-09 16:32:43 +0000
@@ -354,7 +354,7 @@
             versioned_root=True, tree_references=True)
         err = self.assertRaises(InventoryDeltaError,
             serializer.delta_to_lines, NULL_REVISION, 'entry-version', delta)
-        self.assertEqual(str(err), 'no version for fileid id')
+        self.assertContainsRe(str(err), "^no version for fileid b?'id'$")
 
     def test_richroot_unversioned_root_errors(self):
         old_inv = Inventory(None)
@@ -366,7 +366,8 @@
             versioned_root=True, tree_references=True)
         err = self.assertRaises(InventoryDeltaError,
             serializer.delta_to_lines, NULL_REVISION, 'entry-version', delta)
-        self.assertEqual(str(err), 'no version for fileid TREE_ROOT')
+        self.assertContainsRe(
+            str(err), "no version for fileid b?'TREE_ROOT'$")
 
     def test_nonrichroot_versioned_root_errors(self):
         old_inv = Inventory(None)
@@ -379,7 +380,8 @@
             versioned_root=False, tree_references=True)
         err = self.assertRaises(InventoryDeltaError,
             serializer.delta_to_lines, NULL_REVISION, 'entry-version', delta)
-        self.assertStartsWith(str(err), 'Version present for / in TREE_ROOT')
+        self.assertContainsRe(
+            str(err), "^Version present for / in b?'TREE_ROOT'")
 
     def test_unknown_kind_errors(self):
         old_inv = Inventory(None)