Merge lp:~jelmer/brz/bad-bug-url into lp:brz

[Jelmer Vernooij]

Improve handling of bad bug urls:

* Raise an exception when invalid bug URLs are specified to 'brz commit'
* Print exceptions when customer log handlers raise an exception
  rather than aborting altogether.

[Martin Packman (gz)]

Looks reasonable, see two inline comments. Also, decoded bug urls should probably be defined as either url-escaped ascii or cleanly decoded to unicode, but that can change later.

review: Approve

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

Running landing tests failed
https://ci.breezy-vcs.org/job/brz/job/brz-land/69/

=== modified file 'breezy/bugtracker.py'
--- breezy/bugtracker.py	2018-11-16 11:37:47 +0000
+++ breezy/bugtracker.py	2018-11-17 17:17:17 +0000
@@ -186,6 +186,14 @@
         self.line = line
 
 
+class InvalidBugUrl(errors.BzrError):
+
+    _fmt = "Invalid bug URL: %(url)s"
+
+    def __init__(self, url):
+        self.url = url
+
+
 class InvalidBugStatus(errors.BzrError):
 
     _fmt = ("Invalid bug status: '%(status)s'")
@@ -420,4 +428,25 @@
     :return: A string that will be set as the 'bugs' property of a revision
         as part of a commit.
     """
-    return '\n'.join(('%s %s' % (url, tag)) for (url, tag) in bug_urls)
+    lines = []
+    for (url, tag) in bug_urls:
+        if ' ' in url:
+            raise InvalidBugUrl(url)
+        lines.append('%s %s' % (url, tag))
+    return '\n'.join(lines)
+
+
+def decode_bug_urls(bug_text):
+    """Decode a bug property text.
+
+    :param bug_text: Contents of a bugs property
+    :return: iterator over (url, status) tuples
+    """
+    for line in bug_text.splitlines():
+        try:
+            url, status = line.split(None, 2)
+        except ValueError:
+            raise InvalidLineInBugsProperty(line)
+        if status not in ALLOWED_BUG_STATUSES:
+            raise InvalidBugStatus(status)
+        yield url, status

=== modified file 'breezy/log.py'
--- breezy/log.py	2018-11-16 19:09:31 +0000
+++ breezy/log.py	2018-11-17 17:17:17 +0000
@@ -75,6 +75,7 @@
     errors,
     registry,
     revisionspec,
+    trace,
     )
 from .osutils import (
     format_date,
@@ -1522,7 +1523,11 @@
         """
         lines = self._foreign_info_properties(revision)
         for key, handler in properties_handler_registry.iteritems():
-            lines.extend(self._format_properties(handler(revision)))
+            try:
+                lines.extend(self._format_properties(handler(revision)))
+            except Exception:
+                trace.log_exception_quietly()
+                trace.print_exception(sys.exc_info(), self.to_file)
         return lines
 
     def _foreign_info_properties(self, rev):
@@ -2156,21 +2161,21 @@
 
 
 def _bugs_properties_handler(revision):
+    fixed_bug_urls = []
+    related_bug_urls = []
+    for bug_url, status in revision.iter_bugs():
+        if status == 'fixed':
+            fixed_bug_urls.append(bug_url)
+        elif status == 'related':
+            related_bug_urls.append(bug_url)
     ret = {}
-    if 'bugs' in revision.properties:
-        bug_lines = revision.properties['bugs'].split('\n')
-        bug_rows = [line.split(' ', 1) for line in bug_lines]
-        fixed_bug_urls = [row[0] for row in bug_rows if
-                          len(row) > 1 and row[1] == 'fixed']
-        related_bug_urls = [row[0] for row in bug_rows if
-                            len(row) > 1 and row[1] == 'related']
-        if fixed_bug_urls:
-            text = ngettext('fixes bug', 'fixes bugs', len(fixed_bug_urls))
-            ret[text] = ' '.join(fixed_bug_urls)
-        if related_bug_urls:
-            text = ngettext('related bug', 'related bugs',
-                            len(related_bug_urls))
-            ret[text] = ' '.join(related_bug_urls)
+    if fixed_bug_urls:
+        text = ngettext('fixes bug', 'fixes bugs', len(fixed_bug_urls))
+        ret[text] = ' '.join(fixed_bug_urls)
+    if related_bug_urls:
+        text = ngettext('related bug', 'related bugs',
+                        len(related_bug_urls))
+        ret[text] = ' '.join(related_bug_urls)
     return ret
 
 

=== modified file 'breezy/revision.py'
--- breezy/revision.py	2018-11-16 12:32:43 +0000
+++ breezy/revision.py	2018-11-17 17:17:17 +0000
@@ -147,15 +147,8 @@
         """Iterate over the bugs associated with this revision."""
         bug_property = self.properties.get('bugs', None)
         if bug_property is None:
-            return
-        for line in bug_property.splitlines():
-            try:
-                url, status = line.split(None, 2)
-            except ValueError:
-                raise bugtracker.InvalidLineInBugsProperty(line)
-            if status not in bugtracker.ALLOWED_BUG_STATUSES:
-                raise bugtracker.InvalidBugStatus(status)
-            yield url, status
+            return iter([])
+        return bugtracker.decode_bug_urls(bug_property)
 
 
 def iter_ancestors(revision_id, revision_source, only_present=False):

=== modified file 'breezy/tests/test_bugtracker.py'
--- breezy/tests/test_bugtracker.py	2018-11-16 11:37:47 +0000
+++ breezy/tests/test_bugtracker.py	2018-11-17 17:17:17 +0000
@@ -342,3 +342,36 @@
             bugtracker.encode_fixes_bug_urls(
                 [('http://example.com/bugs/1', 'fixed'),
                  ('http://example.com/bugs/2', 'related')]))
+
+    def test_encoding_with_space(self):
+        self.assertRaises(
+            bugtracker.InvalidBugUrl,
+            bugtracker.encode_fixes_bug_urls,
+            [('http://example.com/bugs/ 1', 'fixed')])
+
+
+class TestPropertyDecoding(TestCase):
+    """Tests for parsing bug revision properties."""
+
+    def test_decoding_one(self):
+        self.assertEqual(
+            [('http://example.com/bugs/1', 'fixed')],
+            list(bugtracker.decode_bug_urls(
+                'http://example.com/bugs/1 fixed')))
+
+    def test_decoding_zero(self):
+        self.assertEqual([], list(bugtracker.decode_bug_urls('')))
+
+    def test_decoding_two(self):
+        self.assertEqual(
+            [('http://example.com/bugs/1', 'fixed'),
+             ('http://example.com/bugs/2', 'related')],
+            list(bugtracker.decode_bug_urls(
+                'http://example.com/bugs/1 fixed\n'
+                'http://example.com/bugs/2 related')))
+
+    def test_decoding_invalid(self):
+        self.assertRaises(
+            bugtracker.InvalidLineInBugsProperty,
+            list,
+            bugtracker.decode_bug_urls('http://example.com/bugs/ 1 fixed\n'))

=== modified file 'breezy/tests/test_log.py'
--- breezy/tests/test_log.py	2018-11-11 04:08:32 +0000
+++ breezy/tests/test_log.py	2018-11-17 17:17:17 +0000
@@ -15,6 +15,7 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 import os
+import re
 
 from .. import (
     branchbuilder,
@@ -728,7 +729,9 @@
         log.properties_handler_registry.register(
             'trivial_custom_prop_handler',
             trivial_custom_prop_handler)
-        self.assertRaises(Exception, log.show_log, wt.branch, formatter,)
+        log.show_log(wt.branch, formatter)
+        self.assertContainsRe(
+            sio.getvalue(), b'brz: ERROR: Exception: a test error')
 
     def test_properties_handler_bad_argument(self):
         wt = self.make_standard_commit('bad_argument',
@@ -748,7 +751,7 @@
 
         revision = wt.branch.repository.get_revision(wt.branch.last_revision())
         formatter.show_properties(revision, '')
-        self.assertEqualDiff(b'''custom_prop_name: test_value\n''',
+        self.assertEqualDiff(b'custom_prop_name: test_value\n',
                              sio.getvalue())
 
     def test_show_ids(self):
@@ -1395,6 +1398,35 @@
                                  'test://bug/2 fixed'})
         return tree
 
+    def test_bug_broken(self):
+        tree = self.make_branch_and_tree(u'.')
+        self.build_tree(['a', 'b'])
+        tree.add('a')
+        self.wt_commit(tree, 'simple log message', rev_id=b'a1',
+                       revprops={u'bugs': 'test://bua g/id fixed'})
+
+        logfile = self.make_utf8_encoded_stringio()
+        formatter = log.LongLogFormatter(to_file=logfile)
+        log.show_log(tree.branch, formatter)
+
+        self.assertContainsRe(
+            logfile.getvalue(),
+            b'brz: ERROR: breezy.bugtracker.InvalidLineInBugsProperty: '
+            b'Invalid line in bugs property: \'test://bua g/id fixed\'')
+
+        text = logfile.getvalue()
+        self.assertEqualDiff(
+            text[text.index(b'-' * 60):],
+            b"""\
+------------------------------------------------------------
+revno: 1
+committer: Joe Foo <[e-mail]>
+branch nick: work
+timestamp: Tue 2005-11-22 00:00:00 +0000
+message:
+  simple log message
+""")
+
     def test_long_bugs(self):
         tree = self.make_commits_with_bugs()
         self.assertFormatterResult(b"""\
@@ -1441,12 +1473,22 @@
         self.build_tree(['foo'])
         self.wt_commit(tree, 'simple log message', rev_id=b'a1',
                        revprops={u'bugs': 'test://bug/id invalid_value'})
-        self.assertFormatterResult(b"""\
-    1 Joe Foo\t2005-11-22
-      simple log message
-
-""",
-                                   tree.branch, log.ShortLogFormatter)
+
+        logfile = self.make_utf8_encoded_stringio()
+        formatter = log.ShortLogFormatter(to_file=logfile)
+        log.show_log(tree.branch, formatter)
+
+        lines = logfile.getvalue().splitlines()
+
+        self.assertEqual(
+            lines[0], b'    1 Joe Foo\t2005-11-22')
+
+        self.assertEqual(
+            lines[1],
+            b'brz: ERROR: breezy.bugtracker.InvalidBugStatus: Invalid '
+            b'bug status: \'invalid_value\'')
+
+        self.assertEqual(lines[-2], b"      simple log message")
 
     def test_bugs_handler_present(self):
         self.properties_handler_registry.get('bugs_properties_handler')