launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

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.

Diff comments:

> === 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	2014-12-18 00:33:16 +0000
> @@ -10,7 +10,7 @@
>      'CodeReviewCommentMailer',
>      ]
>  
> -
> +from bzrlib import patches
>  from zope.component import getUtility
>  from zope.security.proxy import removeSecurityProxy
>  
> @@ -169,18 +169,85 @@
>                  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 = []
> +        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
> +            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 is not None:
> +                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 is not None:
> +                hunk_lines.extend(format_comment(comment))
> +                hunk_comment = True
> +
> +            for line in hunk.lines:
> +                line_count = line_count + 1  # inc hunk lines

line_count += 1, for consistency with elsewhere.

> +
> +                hunk_lines.append(u'> %s' % str(
> +                    line).rstrip('\n').decode('utf-8', 'replace'))

As William noted in a previous review, str(line) is either the identity function if isinstance(line, str) or buggy if isinstance(line, unicode).  Just make that u'> %s' % line instead.

> +                comment = comments.get(str(line_count))
> +                if comment is not None:
> +                    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 hunk_comment:

hunk_comment is reset to False at the start of each run through the "for hunk in patch.hunks" loop above, so this only tests the last hunk, which seems wrong.  How about just dropping this condition and moving the block inside it out an indentation level?  list.extend([]) is a no-op, so you can just rely on patch_lines and keep_hunks being empty lists if there's nothing to do.

Please do add an explicit test for this bug, though: I believe it will be triggered if there's a comment on a non-last hunk of a patch, and no comment on the header.

> +            if len(dirty_head) > 0:
> +                result_lines.extend(dirty_head)

Likewise, you don't need this explicit test, because result_lines.extend(dirty_head) is a no-op if dirty_head is the empty list.

> +            result_lines.extend(patch_lines)
> +            result_lines.extend(keep_hunks)
> +
> +    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	2014-12-18 00:33:16 +0000
> @@ -239,7 +239,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 +249,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])
>  
> @@ -378,6 +378,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 +391,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 +413,90 @@
>          # 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_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',
> +            map(unicode, ['> --- bar\t2009-08-26 15:53:34.000000000 -0400',
> +             '> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
>               '',
>               'Foo',
>               'Bar',
> -             '',
> -             '> @@ -1,3 +0,0 @@'],
> -            self.getSection(comments).splitlines()[5:11])
> +             '']),
> +            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 +506,4 @@
>               '',
>               'Bar',
>               ''],
> -            self.getSection(comments).splitlines()[5:13])
> +            self.getSection(comments).splitlines()[7:15])
> 
> === modified file 'versions.cfg'
> --- versions.cfg	2014-12-08 02:23:41 +0000
> +++ versions.cfg	2014-12-18 00:33:16 +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
> 


-- 
https://code.launchpad.net/~blr/launchpad/bug-1334577-verbose-diff/+merge/243751
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.