launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Review: Needs Information

Chris,

The code seems to be working as expected, code hunks without comments are suppressed in the email.

However, there are few points that needs consideration IMO:

* Code copied from bzrlib needs to be removed (reused from bzrlib, maybe with accessors) or, in last instance, copied to a different and isolated place.

* Test coverage seems to be enough but the testing infrastructure (bzrlib TestCase should not be needed) and test names and descriptions needs update.

* The way we load test inputs and outputs from files is nice to make the tests less noisy, but at the same time makes it very hard to understand. I don't think there is an easy solution here, but surely the problem can be alleviated with better test description(i.e. 'binary file mentions in diff are properly presented.')

* Finally, the core code changes (build_inline_comments_section) can be simplified by replacing the boolean flags with continue and some use some local functions. That would make the code clearer.



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-07-10 17:05:25 +0000
> @@ -11,6 +11,17 @@
>      ]
>  
>  
> +import re
> +from bzrlib.patches import (
> +    iter_lines_handle_nl,
> +    iter_hunks,
> +    binary_files_re,
> +    )
> +from bzrlib.errors import (
> +    BzrError,
> +    MalformedPatchHeader,
> +    PatchSyntax,
> +    )
>  from zope.component import getUtility
>  from zope.security.proxy import removeSecurityProxy
>  
> @@ -28,6 +39,8 @@
>      )
>  from lp.services.webapp import canonical_url
>  
> +regex = re.compile(binary_files_re)
> +
>  
>  def send(comment, event):
>      """Send a copy of the code review comments to branch subscribers."""
> @@ -169,18 +182,207 @@
>                  content, content_type=content_type, filename=filename)
>  
>  
> +class BinaryFiles(BzrError):
> +
> +    _fmt = 'Binary files section encountered.'
> +
> +    def __init__(self, comments, orig_name, mod_name):
> +        self.comments = comments
> +        self.orig_name = orig_name
> +        self.mod_name = mod_name
> +
> +
> +class BinaryPatch(object):
> +    def __init__(self, comments, oldname, newname):
> +        self.oldname = oldname
> +        self.newname = newname
> +        self.comments = comments
> +        self.hunks = []
> +
> +    def __str__(self):
> +        return self.get_header()
> +
> +    def get_header(self):
> +        header = 'Binary files %s and %s differ\n' % (
> +            self.oldname, self.newname)
> +        comments = ''.join(self.comments + [header])
> +        return "%s" % comments
> +
> +
> +class Patch(BinaryPatch):
> +
> +    def __init__(self, comments, oldname, newname):
> +        BinaryPatch.__init__(self, comments, oldname, newname)
> +        self.hunks = []
> +        self.comments = comments
> +
> +    def __str__(self):
> +        ret = self.get_header()
> +        ret += "".join([str(h) for h in self.hunks])
> +        return ret
> +
> +    def get_header(self):
> +        comments = ''.join(self.comments)
> +        return "%s--- %s\n+++ %s\n" % (comments, self.oldname, self.newname)
> +
> +
> +def get_patch_names(iter_lines):
> +    comments = []
> +    line = iter_lines.next()
> +    while ((line == "\n") or line.startswith('=== ')
> +            or line.startswith('*** ') or line.startswith('#')):
> +        comments.append(line)
> +        line = iter_lines.next()
> +    try:
> +        match = re.match(binary_files_re, line)
> +        if match is not None:
> +            raise BinaryFiles(comments, match.group(1), match.group(2))
> +        if not line.startswith("--- "):
> +            raise MalformedPatchHeader("No orig name", line)
> +        else:
> +            orig_name = line[4:].rstrip("\n")
> +    except StopIteration:
> +        raise MalformedPatchHeader("No orig line", "")
> +    try:
> +        line = iter_lines.next()
> +        if not line.startswith("+++ "):
> +            raise PatchSyntax("No mod name")
> +        else:
> +            mod_name = line[4:].rstrip("\n")
> +    except StopIteration:
> +        raise MalformedPatchHeader("No mod line", "")
> +    return (comments, orig_name, mod_name)
> +
> +
> +def iter_file_patch(iter_lines, allow_dirty=False):
> +    '''
> +    :arg iter_lines: iterable of lines to parse for patches
> +    :kwarg allow_dirty: If True, allow comments and other non-patch text
> +        before the first patch.  Note that the algorithm here can only find
> +        such text before any patches have been found.  Comments after the
> +        first patch are stripped away in iter_hunks() if it is also passed
> +        allow_dirty=True.  Default False.
> +    '''
> +    ### FIXME: Docstring is not quite true.  We allow certain comments no
> +    # matter what, If they startwith '===', '***', or '#' Someone should
> +    # reexamine this logic and decide if we should include those in
> +    # allow_dirty or restrict those to only being before the patch is found
> +    # (as allow_dirty does).
> +    saved_lines = []
> +    keeper = []
> +    for line in iter_lines:
> +        # if it's a === or binary it's a new patch, but we might have
> +        # both together.
> +        if line.startswith('=== '):
> +            # assume a new patch until we know differently
> +            if len(saved_lines) > 0:
> +                if saved_lines[-1] == "\n":
> +                    keeper.append(saved_lines[-1])
> +                yield saved_lines
> +                saved_lines = []
> +            keeper.append(line)
> +        elif regex.match(line):
> +            ("REGEX'ed: %s" % line)
> +            # check for preceeding '=== ' line
> +            # a binary patch always yields the previous patch
> +            if len(saved_lines) > 0:
> +                if saved_lines[-1][:4] == '=== ':
> +                    saved_lines.append(line)
> +                    yield saved_lines
> +                else:
> +                    yield saved_lines
> +                    yield [line]
> +            else:
> +                yield [line]
> +            saved_lines = []
> +        else:
> +            saved_lines.append(line)
> +        if len(keeper) > 0:
> +            saved_lines += keeper
> +            keeper = []
> +    if len(saved_lines) > 0:
> +        yield saved_lines
> +
> +
> +def parse_patch(iter_lines, allow_dirty=False):
> +    '''
> +    :arg iter_lines: iterable of lines to parse
> +    :kwarg allow_dirty: If True, allow the patch to have trailing junk.
> +        Default False
> +    '''
> +    iter_lines = iter_lines_handle_nl(iter_lines)
> +    try:
> +        (comments, orig_name, mod_name) = get_patch_names(iter_lines)
> +    except BinaryFiles, e:
> +        return BinaryPatch(e.comments, e.orig_name, e.mod_name)
> +    else:
> +        patch = Patch(comments, orig_name, mod_name)
> +        patch.comments = comments
> +        for hunk in iter_hunks(iter_lines, allow_dirty):
> +            patch.hunks.append(hunk)
> +        return patch
> +
> +
> +def parse_patches(iter_lines, allow_dirty=False):
> +    '''
> +    :arg iter_lines: iterable of lines to parse for patches
> +    :kwarg allow_dirty: If True, allow text that's not part of the patch at
> +        selected places.  This includes comments before and after a patch
> +        for instance.  Default False.
> +    '''
> +    patches = []
> +    for f in iter_file_patch(iter_lines, allow_dirty):
> +        patch = parse_patch(f.__iter__(), allow_dirty)
> +        patches.append(patch)
> +
> +    return patches
> +
> +

This entire hunk of code seems to be copied from bzrlib. Can we first investigate if it can be reused from there. If it cannot be reused (which is unlikely) we should, at least, copy those functions into an isolated file, IMO.

>  def build_inline_comments_section(comments, diff_text):
>      """Return a formatted text section with contextualized comments."""
> -    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)
> +    hunk_code = []
> +    lines = diff_text.splitlines(True)
> +    patches = parse_patches(lines, allow_dirty=True)
> +    line_count = 0
> +    for patch in patches:
> +        patch_code = []
> +        patch_has_ic = False
> +        headers = patch.get_header().rstrip('\n')
> +        has_header_ic = False
> +        header_code = []
> +        for line in headers.split('\n'):
> +            line_count += 1
> +            comment = comments.get(str(line_count))
> +            header_code.append(u'> {0}'.format(
> +                line.decode('utf-8', 'replace')))
> +            if comment is not None:
> +                header_code.append('')
> +                header_code.extend(comment.splitlines())
> +                header_code.append('')
> +                has_header_ic = True
> +
> +        for p in patch.hunks:
> +            this_hunk_code = []
> +            has_ic = False
> +            # This uses split('\n') due to the requirement of keeping the blank
> +            # line after a hunk
> +            diff_code = str(p).rstrip('\n').split('\n')
> +            for num, line in enumerate(diff_code, 1):
> +                line_count += 1
> +                this_hunk_code.append(u'> {0}'.format(
> +                    line.decode('utf-8', 'replace')))
> +                comment = comments.get(str(line_count))
> +                if comment is not None:
> +                    this_hunk_code.append('')
> +                    this_hunk_code.extend(comment.splitlines())
> +                    this_hunk_code.append('')
> +                    has_ic = True
> +            if has_ic:
> +                patch_code += this_hunk_code
> +                patch_has_ic = True
> +        if has_header_ic or patch_has_ic:
> +            hunk_code += header_code + patch_code
> +
> +    result_text = u'\n'.join(hunk_code)
>  
>      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-05-21 09:33:04 +0000
> +++ lib/lp/code/mail/tests/test_codereviewcomment.py	2014-07-10 17:05:25 +0000
> @@ -3,9 +3,15 @@
>  
>  """Test CodeReviewComment emailing functionality."""
>  
> -
> +import os
>  import testtools
>  import transaction
> +
> +from bzrlib.tests import TestCase
> +from bzrlib.patches import (
> +    difference_index,
> +    )
> +from bzrlib.errors import MalformedPatchHeader
>  from zope.component import getUtility
>  from zope.security.proxy import removeSecurityProxy
>  
> @@ -16,7 +22,12 @@
>      )
>  from lp.code.mail.codereviewcomment import (
>      build_inline_comments_section,
> +    BinaryPatch,
>      CodeReviewCommentMailer,
> +    Patch,
> +    get_patch_names,
> +    parse_patch,
> +    parse_patches,
>      )
>  from lp.services.mail.sendmail import format_address
>  from lp.services.messages.interfaces.message import IMessageSet
> @@ -31,6 +42,126 @@
>  from lp.testing.layers import LaunchpadFunctionalLayer
>  
>  
> +def datafile(filename):
> +    data_path = os.path.join(os.path.dirname(__file__),
> +                             "test_patches_data", filename)
> +    return file(data_path, "rb")
> +
> +
> +class TestBzrLibCode(TestCase):
> +    """Test code rewritten from bzrlib.patches to work with diff comment
> +    emails"""

Why do we need the bzrlib TestCase here ?

Also PEP-8 demands single-line docstring headers. There are many other occurrences in this patch, please adjust them too.

> +
> +    def data_lines(self, filename):
> +        data = datafile(filename)
> +        try:
> +            return data.readlines()
> +        finally:
> +            data.close()
> +
> +    def testValidPatchHeaderWithComments(self):
> +        """Parse a valid patch header"""
> +        expected_comments = ["=== modified file 'commands.py'"]
> +        lines = ("=== modified file 'commands.py'\n--- orig/commands.py\n"
> +                 "+++ mod/dommands.py\n").split('\n')
> +        (comments, orig, mod) = get_patch_names(lines.__iter__())
> +        self.assertEqual(comments, expected_comments)
> +        self.assertEqual(orig, "orig/commands.py")
> +        self.assertEqual(mod, "mod/dommands.py")
> +
> +    def testValidPatchHeader(self):
> +        """Parse a valid patch header"""
> +        expected_comments = []
> +        lines = ("--- orig/commands.py\n+++ mod/dommands.py\n").split('\n')
> +        (comments, orig, mod) = get_patch_names(lines.__iter__())
> +        self.assertEqual(comments, expected_comments)
> +        self.assertEqual(orig, "orig/commands.py")
> +        self.assertEqual(mod, "mod/dommands.py")
> +
> +    def testInvalidPatchHeader(self):
> +        """Parse an invalid patch header"""
> +        lines = "-- orig/commands.py\n+++ mod/dommands.py".split('\n')
> +        self.assertRaises(MalformedPatchHeader, get_patch_names,
> +                          lines.__iter__())
> +
> +    def compare_parsed(self, patchtext):
> +        lines = patchtext.splitlines(True)
> +        patch = parse_patch(lines.__iter__())
> +        pstr = str(patch)
> +        i = difference_index(patchtext, pstr)
> +        if i is not None:
> +            print "%i: \"%s\" != \"%s\"" % (i, patchtext[i], pstr[i])
> +        self.assertEqual(patchtext, str(patch))
> +
> +    def testAll(self):
> +        """Test parsing a whole patch"""
> +        patchtext = datafile("patchtext.patch").read()
> +        self.compare_parsed(patchtext)
> +
> +    def test_parse_binary(self):
> +        """Test parsing a whole patch"""
> +        patches = parse_patches(self.data_lines("binary.patch"))
> +        self.assertIs(BinaryPatch, patches[0].__class__)
> +        self.assertIs(Patch, patches[1].__class__)
> +        self.assertContainsRe(patches[0].oldname, '^bar\t')
> +        self.assertContainsRe(patches[0].newname, '^qux\t')
> +        self.assertContainsRe(str(patches[0]),
> +                              'Binary files bar\t.* and qux\t.* differ\n')
> +
> +    def test_parse_binary_after_normal(self):
> +        patches = parse_patches(self.data_lines("binary-after-normal.patch"))
> +        self.assertIs(BinaryPatch, patches[1].__class__)
> +        self.assertIs(Patch, patches[0].__class__)
> +        self.assertContainsRe(patches[1].oldname, '^bar\t')
> +        self.assertContainsRe(patches[1].newname, '^qux\t')
> +        self.assertContainsRe(str(patches[1]),
> +                              'Binary files bar\t.* and qux\t.* differ\n')
> +
> +    def test_roundtrip_binary(self):
> +        patchtext = ''.join(self.data_lines("binary.patch"))
> +        patches = parse_patches(patchtext.splitlines(True))
> +        self.assertEqual(patchtext, ''.join(str(p) for p in patches))
> +
> +    def testParsePatchesWithComments(self):
> +        """Make sure file names can be extracted from tricky unified diffs
> +        with comment lines"""
> +        patchtext = \
> +"""=== orig-7
> +--- orig-7
> ++++ mod-7
> +@@ -1,10 +1,10 @@
> + -- a
> +--- b
> ++++ c
> + xx d
> + xx e
> + ++ f
> +-++ g
> ++-- h
> + xx i
> + xx j
> + -- k
> +--- l
> ++++ m
> +=== orig-8
> +--- orig-8
> ++++ mod-8
> +@@ -1 +1 @@
> +--- A
> ++++ B
> +@@ -1 +1 @@
> +--- C
> ++++ D
> +"""

Can't we have this diff in a testing file as well ?

> +        filenames = [('orig-7', 'mod-7'),
> +                     ('orig-8', 'mod-8')]
> +        patches = parse_patches(patchtext.splitlines(True))
> +        patch_files = []
> +        for patch in patches:
> +            patch_files.append((patch.oldname, patch.newname))
> +        self.assertEqual(patch_files, filenames)
> +
> +
>  class TestCodeReviewComment(TestCaseWithFactory):
>      """Test that comments are generated as expected."""
>  
> @@ -215,13 +346,14 @@
>                           mailer.message.text_contents.splitlines())
>  
>      def makeCommentWithInlineComments(self, subject=None, content=None,
> -                                      inline_comments=None):
> +                                      inline_comments=None, diff_text=None):
>          """Create a `CodeReviewComment` with inline (diff) comments."""
>          bmp = self.factory.makeBranchMergeProposal()
>          bmp.source_branch.subscribe(bmp.registrant,
>              BranchSubscriptionNotificationLevel.NOEMAIL, None,
>              CodeReviewNotificationLevel.FULL, bmp.registrant)
> -        previewdiff = self.factory.makePreviewDiff(merge_proposal=bmp)
> +        previewdiff = self.factory.makePreviewDiff(merge_proposal=bmp,
> +                                                   diff_text=diff_text)
>          transaction.commit()
>          if subject is None:
>              subject = 'A comment'
> @@ -235,14 +367,116 @@
>              inline_comments=inline_comments)
>          return comment
>  
> -    def test_generateEmailWithInlineComments(self):
> -        """Review comments emails consider the inline comments.
> -
> -        See `build_inline_comments_section` tests for formatting details.
> -        """
> -        # Enabled corresponding feature flag.
> -        self.useContext(feature_flags())
> -        set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> +    def test_generateEmailWithInlineCommentsDiff1(self):

Numeric suffix are not a good idea for test names. Please use descriptive term(s) in the test name.
You can also replace the test docstring by a multi-line code comment describing the test in more detail (not need for the previous boilerplates).

> +        """Review comments emails consider the inline comments.
> +
> +        See `build_inline_comments_section` tests for formatting details.
> +
> +        Testing that a single file with three hunks only emails the two hunks
> +        with diff comments.
> +        """
> +        # Enabled corresponding feature flag.
> +        self.useContext(feature_flags())
> +        set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> +        diff = datafile("diff-1").read()
> +        expected = datafile("expected-1").read().decode().splitlines()
> +
> +        comment = self.makeCommentWithInlineComments(
> +            inline_comments={'9': u'Cool',
> +                             '27': u'Are you sure?'},
> +            diff_text=diff)
> +        mailer = CodeReviewCommentMailer.forCreation(comment)
> +        commenter = comment.branch_merge_proposal.registrant
> +        ctrl = mailer.generateEmail(
> +            commenter.preferredemail.email, commenter)
> +
> +        self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> +    def test_generateEmailWithInlineCommentsDiff2(self):
> +        """Review comments emails consider the inline comments.
> +
> +        See `build_inline_comments_section` tests for formatting details.
> +
> +        Testing that a file with three hunks and a binary file emails as only
> +        the binary file with diff comments.
> +        """
> +        # Enabled corresponding feature flag.
> +        self.useContext(feature_flags())
> +        set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> +        diff_1 = datafile("diff-1").read()
> +        diff_2 = datafile("diff-2").read()
> +        diff = u"{}{}".format(diff_1, diff_2)
> +        expected = datafile("expected-2").read().decode().splitlines()
> +
> +        comment = self.makeCommentWithInlineComments(
> +            inline_comments={'32': u'Why are you adding this?'},
> +            diff_text=diff)
> +        mailer = CodeReviewCommentMailer.forCreation(comment)
> +        commenter = comment.branch_merge_proposal.registrant
> +        ctrl = mailer.generateEmail(
> +            commenter.preferredemail.email, commenter)
> +
> +        self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> +    def test_generateEmailWithInlineCommentsDiff3(self):
> +        """Review comments emails consider the inline comments.
> +
> +        See `build_inline_comments_section` tests for formatting details.
> +
> +        Testing that a binary file emails as the binary file with diff
> +        comments.
> +        """
> +        # Enabled corresponding feature flag.
> +        self.useContext(feature_flags())
> +        set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> +        diff = datafile("diff-2").read()
> +        expected = datafile("expected-2").read().decode().splitlines()
> +
> +        comment = self.makeCommentWithInlineComments(
> +            inline_comments={'2': u'Why are you adding this?'},
> +            diff_text=diff)
> +        mailer = CodeReviewCommentMailer.forCreation(comment)
> +        commenter = comment.branch_merge_proposal.registrant
> +        ctrl = mailer.generateEmail(
> +            commenter.preferredemail.email, commenter)
> +
> +        self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> +    def test_generateEmailWithInlineCommentsDiff4(self):
> +        """Review comments emails consider the inline comments.
> +
> +        See `build_inline_comments_section` tests for formatting details.
> +
> +        Testing that a text file with three hunks, a binary file, and another
> +        """
> +        # Enabled corresponding feature flag.
> +        self.useContext(feature_flags())
> +        set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> +        diff_1 = datafile("diff-1").read()
> +        diff_2 = datafile("diff-2").read()
> +        diff_3 = datafile("diff-3").read()
> +        diff = u"{}{}{}".format(diff_1, diff_2, diff_3)
> +        expected = datafile("expected-3").read().decode().splitlines()
> +
> +        comment = self.makeCommentWithInlineComments(
> +            inline_comments={'41': u'What is this?'},
> +            diff_text=diff)
> +        mailer = CodeReviewCommentMailer.forCreation(comment)
> +        commenter = comment.branch_merge_proposal.registrant
> +        ctrl = mailer.generateEmail(
> +            commenter.preferredemail.email, commenter)
> +
> +        self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> +    def test_generateEmailWithInlineCommentsDiff5(self):
> +        """Review comments emails consider the inline comments.
> +
> +        See `build_inline_comments_section` tests for formatting details.
> +        """
> +        # Enabled corresponding feature flag.
> +        self.useContext(feature_flags())
> +        set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> +        expected = datafile("expected-4").read().decode().splitlines()
>  
>          comment = self.makeCommentWithInlineComments(
>              inline_comments={'2': u'Is this from Planet Earth\xa9 ?'})
> @@ -251,20 +485,7 @@
>          ctrl = mailer.generateEmail(
>              commenter.preferredemail.email, commenter)
>  
> -        expected_lines = [
> -            '',
> -            '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'
> -        ]
> -        self.assertEqual(expected_lines, ctrl.body.splitlines()[1:10])
> +        self.assertEqual(expected, ctrl.body.splitlines()[:-2])
>  
>      def test_generateEmailWithInlineComments_feature_disabled(self):
>          """Inline comments are not considered if the flag is not enabled."""
> 
> === added directory 'lib/lp/code/mail/tests/test_patches_data'
> === added file 'lib/lp/code/mail/tests/test_patches_data/binary-after-normal.patch'
> --- lib/lp/code/mail/tests/test_patches_data/binary-after-normal.patch	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/binary-after-normal.patch	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,6 @@
> +--- baz	2009-10-14 19:49:59 +0000
> ++++ quxx	2009-10-14 19:51:00 +0000
> +@@ -1 +1 @@
> +-hello
> ++goodbye
> +Binary files bar	2009-10-14 19:49:59 +0000 and qux	2009-10-14 19:50:35 +0000 differ
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/binary.patch'
> --- lib/lp/code/mail/tests/test_patches_data/binary.patch	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/binary.patch	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,6 @@
> +Binary files bar	2009-10-14 19:49:59 +0000 and qux	2009-10-14 19:50:35 +0000 differ
> +--- baz	2009-10-14 19:49:59 +0000
> ++++ quxx	2009-10-14 19:51:00 +0000
> +@@ -1 +1 @@
> +-hello
> ++goodbye
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/diff-1'
> --- lib/lp/code/mail/tests/test_patches_data/diff-1	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/diff-1	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,30 @@
> +=== zbqvsvrq svyr 'yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf'
> +--- yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 12:32:44 +0000
> ++++ yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 23:47:16 +0000
> +@@ -70,7 +70,7 @@
> +                 "jf.bc=trgVayvarPbzzragf&cerivrjqvss_vq=1",
> +                 zbpxvb.erdhrfgf[0].pbasvt.qngn);
> +             zbpxvb.ynfg_erdhrfg = zbpxvb.erdhrfgf[0];
> +-            ine abj = (arj Qngr).inyhrBs()
> ++            ine abj = (arj Qngr).inyhrBs();
> +             choyvfurq_pbzzragf = [
> +                 {'yvar_ahzore': '2',
> +                  'crefba': crefba_bow,
> +@@ -80,7 +80,7 @@
> +                  'crefba': crefba_bow,
> +                  'grkg': 'Guvf vf terng.',
> +                  'qngr': (arj Qngr(abj - 12600000)),
> +-                },
> ++                }
> +             ];
> +             zbpxvb.fhpprff({
> +                 erfcbafrGrkg: L.WFBA.fgevatvsl(choyvfurq_pbzzragf),
> +@@ -88,7 +88,7 @@
> + 
> +             // Choyvfurq pbzzrag vf qvfcynlrq.
> +             ine svefg_pbzzragf = L.bar('#qvss-yvar-2').arkg().bar('qvi');
> +-            ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq')
> ++            ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq');
> +             L.Nffreg.nerRdhny(
> +                 'Sbb One (anzr16) jebgr ba 2012-08-12:',
> +                 svefg.bar('.obneqPbzzragQrgnvyf').trg('grkg'));
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/diff-2'
> --- lib/lp/code/mail/tests/test_patches_data/diff-2	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/diff-2	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,2 @@
> +=== nqqrq svyr 'jrohv/obql_ot.wct'
> +Binary files svyrf jrohv/obql_ot.wct1970-01-01 00:00:00 +0000 and jrohv/obql_ot.wct19702014-01-31 16:57:19 +0000 differ
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/diff-3'
> --- lib/lp/code/mail/tests/test_patches_data/diff-3	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/diff-3	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,12 @@
> +=== zbqvsvrq svyr 'yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf'
> +--- yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf    2014-07-08 12:32:44 +0000
> ++++ yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf    2014-07-08 23:47:16 +0000
> +@@ -3,7 +3,7 @@
> +     ine grfgf = L.anzrfcnpr('qngr.grfg');
> +     grfgf.fhvgr = arj L.Grfg.Fhvgr("qngr grfgf");
> + 
> +-    ine abj = (arj Qngr).inyhrBs()
> ++    ine abj = (arj Qngr).inyhrBs();
> +     grfgf.fhvgr.nqq(arj L.Grfg.Pnfr({
> +         anzr: 'grfg_nccebkvzngrqngr',
> +
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/expected-1'
> --- lib/lp/code/mail/tests/test_patches_data/expected-1	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/expected-1	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,34 @@
> +
> +
> +Diff comments:
> +
> +> === zbqvsvrq svyr 'yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf'
> +> --- yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 12:32:44 +0000
> +> +++ yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 23:47:16 +0000
> +> @@ -70,7 +70,7 @@
> +>                  "jf.bc=trgVayvarPbzzragf&cerivrjqvss_vq=1",
> +>                  zbpxvb.erdhrfgf[0].pbasvt.qngn);
> +>              zbpxvb.ynfg_erdhrfg = zbpxvb.erdhrfgf[0];
> +> -            ine abj = (arj Qngr).inyhrBs()
> +> +            ine abj = (arj Qngr).inyhrBs();
> +
> +Cool
> +
> +>              choyvfurq_pbzzragf = [
> +>                  {'yvar_ahzore': '2',
> +>                   'crefba': crefba_bow,
> +> @@ -88,7 +88,7 @@
> +>  
> +>              // Choyvfurq pbzzrag vf qvfcynlrq.
> +>              ine svefg_pbzzragf = L.bar('#qvss-yvar-2').arkg().bar('qvi');
> +> -            ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq')
> +> +            ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq');
> +
> +Are you sure?
> +
> +>              L.Nffreg.nerRdhny(
> +>                  'Sbb One (anzr16) jebgr ba 2012-08-12:',
> +>                  svefg.bar('.obneqPbzzragQrgnvyf').trg('grkg'));
> +
> +
> +-- 
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/expected-2'
> --- lib/lp/code/mail/tests/test_patches_data/expected-2	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/expected-2	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,12 @@
> +
> +
> +Diff comments:
> +
> +> === nqqrq svyr 'jrohv/obql_ot.wct'
> +> Binary files svyrf jrohv/obql_ot.wct1970-01-01 00:00:00 +0000 and jrohv/obql_ot.wct19702014-01-31 16:57:19 +0000 differ
> +
> +Why are you adding this?
> +
> +
> +
> +-- 
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/expected-3'
> --- lib/lp/code/mail/tests/test_patches_data/expected-3	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/expected-3	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,22 @@
> +
> +
> +Diff comments:
> +
> +> === zbqvsvrq svyr 'yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf'
> +> --- yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf    2014-07-08 12:32:44 +0000
> +> +++ yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf    2014-07-08 23:47:16 +0000
> +> @@ -3,7 +3,7 @@
> +>      ine grfgf = L.anzrfcnpr('qngr.grfg');
> +>      grfgf.fhvgr = arj L.Grfg.Fhvgr("qngr grfgf");
> +>  
> +> -    ine abj = (arj Qngr).inyhrBs()
> +> +    ine abj = (arj Qngr).inyhrBs();
> +
> +What is this?
> +
> +>      grfgf.fhvgr.nqq(arj L.Grfg.Pnfr({
> +>          anzr: 'grfg_nccebkvzngrqngr',
> +>  
> +
> +
> +-- 
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/expected-4'
> --- lib/lp/code/mail/tests/test_patches_data/expected-4	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/expected-4	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,20 @@
> +
> +Diff comments:
> +
> +> === 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 Planet Earth\xa9 ?
> +
> +> @@ -121,6 +121,10 @@
> +>                  'Gur pbasyvpgf grkg qrfpevovat nal cngu be grkg pbasyvpgf.'),
> +>               ernqbayl=Gehr))
> +> 
> +> +    unf_pbasyvpgf = Obby(
> +> +        gvgyr=_('Unf pbasyvpgf'), ernqbayl=Gehr,
> +> +        qrfpevcgvba=_('Gur cerivrjrq zretr cebqhprf pbasyvpgf.'))
> +> +
> +>      # Gur fpurzn sbe gur Ersrerapr trgf cngpurq va _fpurzn_pvephyne_vzcbegf.
> +>      oenapu_zretr_cebcbfny = rkcbegrq(
> +>          Ersrerapr(
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/insert_top.patch'
> --- lib/lp/code/mail/tests/test_patches_data/insert_top.patch	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/insert_top.patch	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,7 @@
> +--- orig/pylon/patches.py
> ++++ mod/pylon/patches.py
> +@@ -1,3 +1,4 @@
> ++#test
> + import util
> + import sys
> + class PatchSyntax(Exception):
> 
> === added file 'lib/lp/code/mail/tests/test_patches_data/patchtext.patch'
> --- lib/lp/code/mail/tests/test_patches_data/patchtext.patch	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/mail/tests/test_patches_data/patchtext.patch	2014-07-10 17:05:25 +0000
> @@ -0,0 +1,25 @@
> +--- orig/commands.py
> ++++ mod/commands.py
> +@@ -1337,7 +1337,8 @@
> + 
> +     def set_title(self, command=None):
> +         try:
> +-            version = self.tree.tree_version.nonarch
> ++            version = pylon.alias_or_version(self.tree.tree_version, self.tree,
> ++                                             full=False)
> +         except:
> +             version = "[no version]"
> +         if command is None:
> +@@ -1983,7 +1984,11 @@
> +                                          version)
> +         if len(new_merges) > 0:
> +             if cmdutil.prompt("Log for merge"):
> +-                mergestuff = cmdutil.log_for_merge(tree, comp_version)
> ++                if cmdutil.prompt("changelog for merge"):
> ++                    mergestuff = "Patches applied:\\n"
> ++                    mergestuff += pylon.changelog_for_merge(new_merges)
> ++                else:
> ++                    mergestuff = cmdutil.log_for_merge(tree, comp_version)
> +                 log.description += mergestuff
> +         log.save()
> +     try:
> 
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2014-07-08 06:34:13 +0000
> +++ lib/lp/testing/factory.py	2014-07-10 17:05:25 +0000
> @@ -1540,8 +1540,11 @@
>              Diff.fromFile(StringIO(diff_text), len(diff_text)))
>  
>      def makePreviewDiff(self, conflicts=u'', merge_proposal=None,
> -                        date_created=None):
> -        diff = self.makeDiff()
> +                        date_created=None, diff_text=None):
> +        if diff_text is None:
> +            diff = self.makeDiff()
> +        else:
> +            diff = self.makeDiff(diff_text)
>          if merge_proposal is None:
>              merge_proposal = self.makeBranchMergeProposal()
>          preview_diff = PreviewDiff()
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/short-ic-emails/+merge/225095
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.