Merge lp:~blr/launchpad/bug-1334577-verbose-diff into lp:launchpad

[Kit Randel]

Check for inline comments associated with patch headers, hunk headers (hunk context lines) and hunk lines, and discard irrelevant bits in branch merge proposal mail.

Summary
Emails notifications of inline comments on a merge proposal are currently sent with the entirety of the diff, which is needlessly verbose and can make it difficult to find the relevant code. This branch provides a re-implementation of the code responsible for this and discards patches and diff hunks that do not have associated inline comments.

Proposed fix
Check for comments associated with patch headers, hunk headers (hunk context lines) and hunk lines, and discard irrelevant bits.

Implementation details
Currently the diff text is represented as a simple list. In order to have a structural representation of the diff, parse the diff text with bzrlib.patches. Ideally bzrlib.patches would provide line numbers, but as it does not wgrant and blr decided a less intrusive approach would be better than submitting a MP for bzrlib, even if it makes the mailer code more verbose.

LOC Rationale
More lines of code here to reduce lines of code elsewhere (email!). I think we're breaking even.

Tests
./bin/test -c -m lp.code.mail.tests.test_codereviewcomment

Demo and Q/A
Running the tests locally is the best option, unless you have an MTA configured in your dev environment. I'm hoping we can test this with genuine emails on staging?

Lint
Lint free

[William Grant (wgrant)]

review: Needs Fixing (code)

[Colin Watson (cjwatson)]

This looks almost right now, but there seems to be a minor issue or two left over from William's earlier review, and I spotted what looks like a logic bug that you'll want to fix.

review: Needs Fixing

[Colin Watson (cjwatson)]

One more edge case that you need to fix, but feel free to land once you've done that. Thanks!

review: Approve

[Kit Randel (blr)]

Reply to inline comment.

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'lib/lp/code/mail/codereviewcomment.py'
--- lib/lp/code/mail/codereviewcomment.py	2014-05-21 08:52:53 +0000
+++ lib/lp/code/mail/codereviewcomment.py	2015-06-30 08:52:09 +0000
@@ -10,7 +10,7 @@
     'CodeReviewCommentMailer',
     ]
 
-
+from bzrlib import patches
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -169,18 +169,92 @@
                 content, content_type=content_type, filename=filename)
 
 
+def format_comment(comment):
+    """Returns a list of correctly formatted comment(s)."""
+    comment_lines = []
+    if comment is not None:
+        comment_lines.append('')
+        comment_lines.extend(comment.splitlines())
+        comment_lines.append('')
+    return comment_lines
+
+
 def build_inline_comments_section(comments, diff_text):
-    """Return a formatted text section with contextualized comments."""
+    """Return a formatted text section with contextualized comments.
+
+    Hunks without comments are skipped to limit verbosity.
+    Comments can be rendered after patch headers, hunk context lines,
+    and hunk lines.
+    """
+    diff_lines = diff_text.splitlines(True)
+
+    diff_patches = patches.parse_patches(
+        diff_lines, allow_dirty=True, keep_dirty=True)
     result_lines = []
-    diff_lines = diff_text.splitlines()
-    for num, line in enumerate(diff_lines, 1):
-        result_lines.append(u'> {0}'.format(line.decode('utf-8', 'replace')))
-        comment = comments.get(str(num))
-        if comment is not None:
-            result_lines.append('')
-            result_lines.extend(comment.splitlines())
-            result_lines.append('')
-
-    result_text = u'\n'.join(result_lines)
-
+    line_count = 0  # track lines in original diff
+
+    for patch in diff_patches:
+        patch_lines = []
+        dirty_head = []
+        dirty_comment = False
+        patch_comment = False
+
+        if isinstance(patch, dict) and 'dirty_head' in patch:
+            for line in patch['dirty_head']:
+                dirty_head.append(u'> %s' % line.rstrip('\n'))
+                line_count += 1  # inc for dirty headers
+                comment = comments.get(str(line_count))
+                if comment:
+                    dirty_head.extend(format_comment(comment))
+                    dirty_comment = True
+            patch = patch['patch']
+
+        for ph in patch.get_header().splitlines():
+            line_count += 1  # inc patch headers
+            comment = comments.get(str(line_count))
+
+            patch_lines.append('> {0}'.format(ph))
+            if comment:
+                patch_lines.extend(format_comment(comment))
+                patch_comment = True
+
+        keep_hunks = []  # preserve hunks with comments
+        for hunk in patch.hunks:
+            hunk_lines = []
+            hunk_comment = False
+
+            # add context line (hunk header)
+            line_count += 1  # inc hunk context line
+            hunk_lines.append(u'> %s' % hunk.get_header().rstrip('\n'))
+
+            # comment for context line (hunk header)
+            comment = comments.get(str(line_count))
+            if comment:
+                hunk_lines.extend(format_comment(comment))
+                hunk_comment = True
+
+            for line in hunk.lines:
+                line_count += 1  # inc hunk lines
+
+                #  line is a ContextLine/ReplaceLine
+                hunk_lines.append(u'> %s' % str(line).rstrip('\n').decode(
+                    'utf-8', 'replace'))
+                comment = comments.get(str(line_count))
+                if comment:
+                    hunk_lines.extend(format_comment(comment))
+                    hunk_comment = True
+
+            # preserve hunks for context if comment in patch header
+            if patch_comment or hunk_comment:
+                keep_hunks.extend(hunk_lines)
+
+        # Add entire patch and hunks to result if comment found
+        if patch_comment or keep_hunks:
+            result_lines.extend(dirty_head)
+            result_lines.extend(patch_lines)
+            result_lines.extend(keep_hunks)
+        elif dirty_comment:
+            result_lines.extend(dirty_head)
+
+    result_text = '\n'.join(result_lines)
     return '\n\nDiff comments:\n\n%s\n\n' % result_text

=== modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
--- lib/lp/code/mail/tests/test_codereviewcomment.py	2014-08-19 04:56:39 +0000
+++ lib/lp/code/mail/tests/test_codereviewcomment.py	2015-06-30 08:52:09 +0000
@@ -91,7 +91,7 @@
             comment.branch_merge_proposal, mailer.merge_proposal)
         sender = comment.message.owner
         sender_address = format_address(sender.displayname,
-            sender.preferredemail.email)
+                                        sender.preferredemail.email)
         self.assertEqual(sender_address, mailer.from_address)
         self.assertEqual(comment, mailer.code_review_comment)
 
@@ -145,9 +145,10 @@
         source_branch = mailer.merge_proposal.source_branch
         branch_name = source_branch.bzr_identity
         self.assertEqual(
-            ctrl.body.splitlines()[-3:], ['-- ',
-            canonical_url(mailer.merge_proposal),
-            'You are subscribed to branch %s.' % branch_name])
+            ctrl.body.splitlines()[-3:], [
+                '-- ', canonical_url(mailer.merge_proposal),
+                'You are subscribed to branch %s.' % branch_name
+                ])
         rationale = mailer._recipients.getReason('[e-mail]')[1]
         expected = {'X-Launchpad-Branch': source_branch.unique_name,
                     'X-Launchpad-Message-Rationale': rationale,
@@ -216,7 +217,8 @@
                                       inline_comments=None):
         """Create a `CodeReviewComment` with inline (diff) comments."""
         bmp = self.factory.makeBranchMergeProposal()
-        bmp.source_branch.subscribe(bmp.registrant,
+        bmp.source_branch.subscribe(
+            bmp.registrant,
             BranchSubscriptionNotificationLevel.NOEMAIL, None,
             CodeReviewNotificationLevel.FULL, bmp.registrant)
         previewdiff = self.factory.makePreviewDiff(merge_proposal=bmp)
@@ -239,7 +241,7 @@
         See `build_inline_comments_section` tests for formatting details.
         """
         comment = self.makeCommentWithInlineComments(
-            inline_comments={'2': u'Is this from Planet Earth\xa9 ?'})
+            inline_comments={'3': 'Is this from Pl\u0060net Earth ?'})
         mailer = CodeReviewCommentMailer.forCreation(comment)
         commenter = comment.branch_merge_proposal.registrant
         ctrl = mailer.generateEmail(
@@ -249,14 +251,14 @@
             '',
             'Diff comments:',
             '',
-            "> === zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'",
-            '> --- yvo/yc/pbqr/vagresnprf/qvss.cl      '
-            '2009-10-01 13:25:12 +0000',
-            '',
-            u'Is this from Planet Earth\xa9 ?',
-            '',
-            '> +++ yvo/yc/pbqr/vagresnprf/qvss.cl      '
-            '2010-02-02 15:48:56 +0000'
+            ("> === zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'"),
+            ('> --- yvo/yc/pbqr/vagresnprf/qvss.cl      '
+             '2009-10-01 13:25:12 +0000'),
+            ('> +++ yvo/yc/pbqr/vagresnprf/qvss.cl      '
+             '2010-02-02 15:48:56 +0000'),
+            '',
+            'Is this from Pl\u0060net Earth ?',
+            '',
         ]
         self.assertEqual(expected_lines, ctrl.body.splitlines()[1:10])
 
@@ -317,7 +319,8 @@
         bmp = self.factory.makeBranchMergeProposal(registrant=proposer)
         commenter = self.factory.makePerson(
             email='[e-mail]', displayname='Commenter')
-        bmp.source_branch.subscribe(commenter,
+        bmp.source_branch.subscribe(
+            commenter,
             BranchSubscriptionNotificationLevel.NOEMAIL, None,
             CodeReviewNotificationLevel.FULL, commenter)
         comment = bmp.createComment(commenter, 'hello')
@@ -378,6 +381,8 @@
     """Tests for `build_inline_comments_section`."""
 
     diff_text = (
+        "=== added directory 'foo/bar'\n"
+        "=== modified file 'foo/bar/baz.py'\n"
         "--- bar\t2009-08-26 15:53:34.000000000 -0400\n"
         "+++ bar\t1969-12-31 19:00:00.000000000 -0500\n"
         "@@ -1,3 +0,0 @@\n"
@@ -389,6 +394,9 @@
         "@@ -0,0 +1,2 @@\n"
         "+a\n"
         "+b\n"
+        "@@ -1,2 +0,0 @@\n"
+        "-x\n"
+        "-y\n"
         "--- foo\t2009-08-26 15:53:23.000000000 -0400\n"
         "+++ foo\t2009-08-26 15:56:43.000000000 -0400\n"
         "@@ -1,3 +1,4 @@\n"
@@ -408,48 +416,119 @@
         # The inline comments section starts with a 4-lines header
         # (empty lines and title) and ends with an empty line.
         section = self.getSection({}).splitlines()
-        header = section[:5]
+        header = section[:4]
         self.assertEqual(
             ['',
              '',
              'Diff comments:',
-             '',
-             '> --- bar\t2009-08-26 15:53:34.000000000 -0400'],
-            header)
-        footer = section[-2:]
+             ''], header)
+        footer = section[-1:]
         self.assertEqual(
-            ['> +e',
-             ''],
+            [''],
             footer)
 
     def test_single_line_comment(self):
         # The inline comments are correctly contextualized in the diff.
         # and prefixed with '>>> '
-        comments = {'2': u'\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae'}
-        self.assertEqual(
-            ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
-             '',
-             u'\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae',
-             '',
-             '> @@ -1,3 +0,0 @@',
-             u'> -\xe5'],
-            self.getSection(comments).splitlines()[5:11])
+        comments = {'4': '\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae'}
+        self.assertEqual(
+            map(unicode, [
+                '> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
+                '',
+                '\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae',
+                '']),
+            self.getSection(comments).splitlines()[7:11])
+
+    def test_commentless_hunks_ignored(self):
+        # Hunks without inline comments are not returned in the diff text.
+        comments = {'16': 'A comment', '21': 'Another comment'}
+        self.assertEqual(
+            map(unicode, [
+                '> --- baz\t1969-12-31 19:00:00.000000000 -0500',
+                '> +++ baz\t2009-08-26 15:53:57.000000000 -0400',
+                '> @@ -1,2 +0,0 @@',
+                '> -x',
+                '> -y',
+                '',
+                'A comment',
+                '',
+                '> --- foo\t2009-08-26 15:53:23.000000000 -0400',
+                '> +++ foo\t2009-08-26 15:56:43.000000000 -0400',
+                '> @@ -1,3 +1,4 @@',
+                '>  a',
+                '> -b',
+                '',
+                'Another comment',
+                '',
+                '>  c',
+                '> +d',
+                '> +e']),
+            self.getSection(comments).splitlines()[4:23])
+
+    def test_patch_header_comment(self):
+        # Inline comments in patch headers are rendered correctly and
+        # include the patch's hunk(s).
+        comments = {'17': 'A comment in the patch header', '18': 'aardvark'}
+        self.assertEqual(
+            map(unicode, [
+                '> --- foo\t2009-08-26 15:53:23.000000000 -0400',
+                '',
+                'A comment in the patch header',
+                '',
+                '> +++ foo\t2009-08-26 15:56:43.000000000 -0400',
+                '',
+                'aardvark',
+                '',
+                '> @@ -1,3 +1,4 @@',
+                '>  a',
+                '> -b',
+                '>  c',
+                '> +d',
+                '> +e']),
+            self.getSection(comments).splitlines()[4:18])
+
+    def test_dirty_header_comment(self):
+        # Inline comments in dirty headers e.g. 'added file/modified file'
+        # are rendered correctly
+        comments = {'1': 'A comment for a dirty header'}
+        self.assertEqual(
+            map(unicode, [
+                "> === added directory 'foo/bar'",
+                '',
+                'A comment for a dirty header',
+                '']),
+            self.getSection(comments).splitlines()[4:8])
+
+    def test_non_last_hunk_comment(self):
+        comments = {'12': 'A comment in the non-last hunk'}
+        self.assertEqual(
+            map(unicode, [
+                '> --- baz\t1969-12-31 19:00:00.000000000 -0500',
+                '> +++ baz\t2009-08-26 15:53:57.000000000 -0400',
+                '> @@ -0,0 +1,2 @@',
+                '> +a',
+                '',
+                'A comment in the non-last hunk',
+                '',
+                '> +b']),
+            self.getSection(comments).splitlines()[4:12])
 
     def test_multi_line_comment(self):
         # Inline comments with multiple lines are rendered appropriately.
-        comments = {'2': 'Foo\nBar'}
+        comments = {'4': 'Foo\nBar'}
         self.assertEqual(
-            ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
-             '',
-             'Foo',
-             'Bar',
-             '',
-             '> @@ -1,3 +0,0 @@'],
-            self.getSection(comments).splitlines()[5:11])
+            map(unicode, [
+                '> --- bar\t2009-08-26 15:53:34.000000000 -0400',
+                '> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
+                '',
+                'Foo',
+                'Bar',
+                '']),
+            self.getSection(comments).splitlines()[6:12])
 
     def test_multiple_comments(self):
         # Multiple inline comments are redered appropriately.
-        comments = {'2': 'Foo', '3': 'Bar'}
+        comments = {'4': 'Foo', '5': 'Bar'}
         self.assertEqual(
             ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
              '',
@@ -459,4 +538,4 @@
              '',
              'Bar',
              ''],
-            self.getSection(comments).splitlines()[5:13])
+            self.getSection(comments).splitlines()[7:15])

=== modified file 'versions.cfg'
--- versions.cfg	2015-06-24 07:45:11 +0000
+++ versions.cfg	2015-06-30 08:52:09 +0000
@@ -16,7 +16,7 @@
 auditorfixture = 0.0.5
 BeautifulSoup = 3.2.1
 bson = 0.3.3
-bzr = 2.6.0
+bzr = 2.6.0.lp.1
 celery = 2.5.1
 Chameleon = 2.11
 cssselect = 0.9.1