Merge lp:~cjohnston/launchpad/short-ic-emails into lp:launchpad

[Chris Johnston]

First pass at reducing the amount of code that is emailed out when someone saves diff comments

This branch makes a first pass at reducing the amount of code that is emailed out when someone saves diff comments. The way this is done is by discarding hunks that do not have diff comments.

[William Grant (wgrant)]

review: Needs Fixing (code)

[Celso Providelo (cprov)]

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.

review: Needs Information

[Celso Providelo (cprov)]

lib/lp/services/patches.py is a much better place, the code style there still alien to LP, it reads very differently. But I am not opposed to landing it this way if you think it will be closer to the original bzrlib, thus easier to maintain up to date.

Looks good for landing and get some feedback from users, possibly experimenting with including previous comments too.

Good work!

review: Approve

[William Grant (wgrant)]

lp.services.patches should say somewhere that it's derived from bzrlib.patches, what the changes are, and why. It might also be possible to push these improvements back to bzrlib.patches in a backward-compatible way later.

The new set of test files doesn't give any tests for inline comments across multiple files, just multiple hunks. That should probably be fixed.

[William Grant (wgrant)]

Consider '\n'.join(whatever) instead of '{}{}{}'.format(whatever) to get a more realistic diff with blank lines between files.

[William Grant (wgrant)]

Line numbers jump ahead by one at the boundary between file, as visible in expected-6: the comment in the second file is rendered a line too late, and the comment in the third is two lines late. I assume the trailing newline isn't being counted.

Also, it probably makes sense to have a gap between hunks, at least when one is skipped. In expected-6, the hunk at 70 seems to lead directly into 88, but in the original there's one at 80.

Additionally, does expected-1 give any value over expected-6?

review: Needs Fixing (code)

[Chris Johnston (cjohnston)]

> Also, it probably makes sense to have a gap between hunks, at least when one
> is skipped. In expected-6, the hunk at 70 seems to lead directly into 88, but
> in the original there's one at 80.

How do you suggest doing the gap? A blank line or a line that starts with '>', or maybe even some sort of text to indicate that a hunk is skipped? Also, how do you want to handle if multiple hunks in a row (or even a whole file) are mising? One line for each hunk missing, one line total, if a file is missing and it's the first file should we start out with a blank line? I almost think that we should see what type of feedback we get the way things are and then iterate again.

> Additionally, does expected-1 give any value over expected-6?

Just the fact that it's a very simple diff with only one file, so just trying to cover one more case where things are different.

[William Grant (wgrant)]

This is now broken in the opposite direction. If there is no gap between
two files, the comments end up a line too early.

On 16/07/14 03:35, Chris Johnston wrote:
>> Also, it probably makes sense to have a gap between hunks, at least when one
>> is skipped. In expected-6, the hunk at 70 seems to lead directly into 88, but
>> in the original there's one at 80.
>
> How do you suggest doing the gap? A blank line or a line that starts with '>', or maybe even some sort of text to indicate that a hunk is skipped? Also, how do you want to handle if multiple hunks in a row (or even a whole file) are mising? One line for each hunk missing, one line total, if a file is missing and it's the first file should we start out with a blank line? I almost think that we should see what type of feedback we get the way things are and then iterate again.

I think just a blank line makes sense. A line starting with ">" would
indicate that it was quoting the diff, which is false.

[Chris Johnston (cjohnston)]

I have updated the MP to handle a nl between patches or no nl between patches. I also added a blank line when there is a skipped hunk.

[William Grant (wgrant)]

This breaks three tests in TestInlineCommentsSection, and I have a few other comments.

review: Needs Fixing (code)

=== 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-29 15:41:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Email notifications for code review comments."""
@@ -26,6 +26,7 @@
     append_footer,
     format_address,
     )
+from lp.services.patches import parse_patches
 from lp.services.webapp import canonical_url
 
 
@@ -171,16 +172,50 @@
 
 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)
+    result_diff = []
+    lines = diff_text.splitlines(True)
+    patches = parse_patches(lines, allow_dirty=True)
+    line_count = 0
+    for patch in patches:
+        patch_lines = []
+        patch_has_ic = False
+        headers = patch.get_header()
+        has_header_ic = False
+        header_lines = []
+        for line in headers.splitlines():
+            line_count += 1
+            comment = comments.get(str(line_count))
+            header_lines.append(u'> {0}'.format(
+                line.decode('utf-8', 'replace')))
+            if comment is not None:
+                header_lines.append('')
+                header_lines.extend(comment.splitlines())
+                header_lines.append('')
+                has_header_ic = True
+
+        for p in patch.hunks:
+            hunk_lines = []
+            has_ic = False
+            raw_diff = str(p).splitlines()
+            for num, line in enumerate(raw_diff, 1):
+                line_count += 1
+                hunk_lines.append(u'> {0}'.format(
+                    line.decode('utf-8', 'replace')))
+                comment = comments.get(str(line_count))
+                if comment is not None:
+                    hunk_lines.append('')
+                    hunk_lines.extend(comment.splitlines())
+                    hunk_lines.append('')
+                    has_ic = True
+            if has_ic:
+                patch_lines += hunk_lines
+                patch_has_ic = True
+            else:
+                if not patch_lines or patch_lines[-1] != u'':
+                    patch_lines.append('')
+        if has_header_ic or patch_has_ic:
+            result_diff += header_lines + patch_lines
+
+    result_text = u'\n'.join(result_diff)
 
     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-29 15:41:46 +0000
@@ -1,8 +1,9 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test CodeReviewComment emailing functionality."""
 
+import os
 
 import testtools
 import transaction
@@ -31,6 +32,12 @@
 from lp.testing.layers import LaunchpadFunctionalLayer
 
 
+def datafile(filename):
+    data_path = os.path.join(os.path.dirname(__file__),
+                             "../../../services/tests/data", filename)
+    return file(data_path, "rb")
+
+
 class TestCodeReviewComment(TestCaseWithFactory):
     """Test that comments are generated as expected."""
 
@@ -215,13 +222,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 +243,180 @@
             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')
+    # For the test_inline_comments_email_* tests, please see
+    # `build_inline_comments_section` tests for formatting details.
+
+    def test_inline_comments_email_text(self):
+        # Testing that a single file with three hunks is parsed properly and
+        # only the header/hunk combinations with comments are provided.
+
+        # 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_inline_comments_email_text_binary(self):
+        # Testing that a single file with three hunks and a binary file is
+        # parsed properly and only the binary header with comments are provided
+
+        # 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'\n'.join([diff_1, diff_2])
+        expected = datafile("expected-2").read().decode().splitlines()
+
+        comment = self.makeCommentWithInlineComments(
+            inline_comments={'33': 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_inline_comments_email_binary(self):
+        # Testing that a binary file is parsed properly and only the binary
+        # header with comments are provided
+
+        # 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_inline_comments_email_text_binary_text(self):
+        # Testing that a single file with three hunks, a binary file and a
+        # text file is parsed properly and only the binary header with comments
+        # are provided
+
+        # 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_3 = datafile("diff-3").read()
+        diff = u'\n'.join([diff_1, diff_3])
+        expected = datafile("expected-3").read().decode().splitlines()
+
+        comment = self.makeCommentWithInlineComments(
+            inline_comments={'42': 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_inline_comments_email_text_binary_text_includes_two_files(self):
+        # Testing that a single file with three hunks, a binary file and a
+        # text file is parsed properly and a single hunk from the first file
+        # plus a single hunk from the second file with comments are provided
+
+        # 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_3 = datafile("diff-3").read()
+        diff = u'\n'.join([diff_1, diff_3])
+        expected = datafile("expected-5").read().decode().splitlines()
+
+        comment = self.makeCommentWithInlineComments(
+            inline_comments={'18': u'Why?',
+                             '42': u'Good catch!'},
+            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_inline_comments_email_text_binary_text_includes_three_files(self):
+        # Testing that a single file with three hunks, a binary file and a
+        # text file is parsed properly and two hunks from the first file,
+        # the second file and a single hunk from the second file with comments
+        # are provided
+
+        # 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_3 = datafile("diff-3").read()
+        diff = u'\n'.join([diff_1, diff_3])
+        expected = datafile("expected-6").read().decode().splitlines()
+
+        comment = self.makeCommentWithInlineComments(
+            inline_comments={'27': u'+1',
+                             '33': u'Do we need this?',
+                             '42': u'Can we change 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_inline_comments_email_text_binary_text_no_nl_files(self):
+        # Testing that a single file with three hunks, a binary file and a
+        # text file which do not have a new line between each other is parsed
+        # properly and two hunks from the first file, the second file and a
+        # single hunk from the second file with comments are provided
+
+        # 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_3 = datafile("diff-3").read()
+        diff = u''.join([diff_1, diff_3])
+        expected = datafile("expected-7").read().decode().splitlines()
+
+        comment = self.makeCommentWithInlineComments(
+            inline_comments={'9': u'Good stuff',
+                             '27': u'+1',
+                             '32': u'Do we need this?',
+                             '41': u'Can we change 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_inline_comments_email_text_comment_in_header(self):
+        # Testing that a single file with comments in the header is parsed
+        # properly and only the headers is provided
+
+        # 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('utf-8').splitlines()
 
         comment = self.makeCommentWithInlineComments(
             inline_comments={'2': u'Is this from Planet Earth\xa9 ?'})
@@ -251,20 +425,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."""
@@ -425,24 +586,6 @@
             diff_text = self.diff_text
         return build_inline_comments_section(comments, diff_text)
 
-    def test_section_header_and_footer(self):
-        # 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]
-        self.assertEqual(
-            ['',
-             '',
-             'Diff comments:',
-             '',
-             '> --- bar\t2009-08-26 15:53:34.000000000 -0400'],
-            header)
-        footer = section[-2:]
-        self.assertEqual(
-            ['> +e',
-             ''],
-            footer)
-
     def test_single_line_comment(self):
         # The inline comments are correctly contextualized in the diff.
         # and prefixed with '>>> '
@@ -452,8 +595,8 @@
              '',
              u'\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae',
              '',
-             '> @@ -1,3 +0,0 @@',
-             u'> -\xe5'],
+             '',
+             u''],
             self.getSection(comments).splitlines()[5:11])
 
     def test_multi_line_comment(self):
@@ -465,7 +608,7 @@
              'Foo',
              'Bar',
              '',
-             '> @@ -1,3 +0,0 @@'],
+             ''],
             self.getSection(comments).splitlines()[5:11])
 
     def test_multiple_comments(self):
 
=== added file 'lib/lp/services/patches.py'
--- lib/lp/services/patches.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/patches.py	2014-07-29 15:41:46 +0000
@@ -0,0 +1,215 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# This file is derived from bzrlib.patches in order to extend some
+# functionality for use by the Launchpad inline comment feature.
+
+# In order to reduce the size of a diff being emailed with inline comments
+# to just the relevant hunks of code, it was needed to break the diff down
+# and then put the required pieces back together. To do this, the notion of
+# 'comments' was added to Patch and BinaryPatch in order to return any lines
+# beginning with '===' which is later added to the file headers and the line
+# counted.
+
+import re
+
+from bzrlib.errors import (
+    BzrError,
+    MalformedHunkHeader,
+    MalformedPatchHeader,
+    PatchSyntax,
+    )
+from bzrlib.patches import (
+    binary_files_re,
+    ContextLine,
+    hunk_from_header,
+    InsertLine,
+    iter_lines_handle_nl,
+    parse_line,
+    RemoveLine,
+    )
+
+
+binary_regex = re.compile(binary_files_re)
+
+
+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)
+        return ''.join(self.comments + [header])
+
+
+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 = []
+    for line in iter_lines:
+        if (line == "\n" or line.startswith('=== ') or line.startswith('*** ')
+            or line.startswith('#')):
+            comments.append(line)
+        else:
+            break
+    try:
+        match = binary_regex.match(line)
+        if match is not None:
+            # For binary packages we need to check and see if there is a \n
+            # that follows the header info. If so, we need to carry it.
+            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_hunks(iter_lines, allow_dirty=False):
+    '''
+    :arg iter_lines: iterable of lines to parse for hunks
+    :kwarg allow_dirty: If True, when we encounter something that is not
+        a hunk header when we're looking for one, assume the rest of the lines
+        are not part of the patch (comments or other junk).  Default False
+    '''
+    hunk = None
+    for line in iter_lines:
+        if line == "\n":
+            if hunk is not None:
+                hunk.lines.append(line)
+                yield hunk
+                hunk = None
+            continue
+        if hunk is not None:
+            yield hunk
+        try:
+            hunk = hunk_from_header(line)
+        except MalformedHunkHeader:
+            if allow_dirty:
+                # If the line isn't a hunk header, then we've reached the end
+                # of this patch and there's "junk" at the end.  Ignore the
+                # rest of this patch.
+                return
+            raise
+        orig_size = 0
+        mod_size = 0
+        while orig_size < hunk.orig_range or mod_size < hunk.mod_range:
+            hunk_line = parse_line(iter_lines.next())
+            hunk.lines.append(hunk_line)
+            if isinstance(hunk_line, (RemoveLine, ContextLine)):
+                orig_size += 1
+            if isinstance(hunk_line, (InsertLine, ContextLine)):
+                mod_size += 1
+    if hunk is not None:
+        yield hunk
+
+
+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 = []
+    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:
+                yield saved_lines
+                saved_lines = []
+            saved_lines.append(line)
+        elif binary_regex.match(line):
+            # check for preceeding '=== ' line
+            # a binary patch always yields the previous patch
+            if len(saved_lines) > 0:
+                saved_lines.append(line)
+            else:
+                yield [line]
+        else:
+            saved_lines.append(line)
+    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)
+        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
 
=== added directory 'lib/lp/services/tests/data'
=== added file 'lib/lp/services/tests/data/diff-1'
--- lib/lp/services/tests/data/diff-1	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/diff-1	2014-07-29 15:41:46 +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/services/tests/data/diff-2'
--- lib/lp/services/tests/data/diff-2	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/diff-2	2014-07-29 15:41:46 +0000
@@ -0,0 +1,2 @@
+=== nqqrq svyr 'jrohv/obql_ot.wct'
+Binary files 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/services/tests/data/diff-3'
--- lib/lp/services/tests/data/diff-3	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/diff-3	2014-07-29 15:41:46 +0000
@@ -0,0 +1,14 @@
+=== nqqrq svyr 'jrohv/obql_ot.wct'
+Binary files jrohv/obql_ot.wct1970-01-01 00:00:00 +0000 and jrohv/obql_ot.wct19702014-01-31 16:57:19 +0000 differ
+=== 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/services/tests/data/expected-1'
--- lib/lp/services/tests/data/expected-1	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/expected-1	2014-07-29 15:41:46 +0000
@@ -0,0 +1,35 @@
+
+
+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/services/tests/data/expected-2'
--- lib/lp/services/tests/data/expected-2	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/expected-2	2014-07-29 15:41:46 +0000
@@ -0,0 +1,12 @@
+
+
+Diff comments:
+
+> === nqqrq svyr 'jrohv/obql_ot.wct'
+> Binary files 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/services/tests/data/expected-3'
--- lib/lp/services/tests/data/expected-3	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/expected-3	2014-07-29 15:41:46 +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/services/tests/data/expected-4'
--- lib/lp/services/tests/data/expected-4	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/expected-4	2014-07-29 15:41:46 +0000
@@ -0,0 +1,14 @@
+
+
+Diff comments:
+
+> === zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'
+> --- yvo/yc/pbqr/vagresnprf/qvss.cl      2009-10-01 13:25:12 +0000
+
+Is this from Planet Earth© ?
+
+> +++ yvo/yc/pbqr/vagresnprf/qvss.cl      2010-02-02 15:48:56 +0000
+
+
+
+-- 
 
=== added file 'lib/lp/services/tests/data/expected-5'
--- lib/lp/services/tests/data/expected-5	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/expected-5	2014-07-29 15:41:46 +0000
@@ -0,0 +1,39 @@
+
+
+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
+
+> @@ -80,7 +80,7 @@
+>                   'crefba': crefba_bow,
+>                   'grkg': 'Guvf vf terng.',
+>                   'qngr': (arj Qngr(abj - 12600000)),
+> -                },
+> +                }
+
+Why?
+
+>              ];
+>              zbpxvb.fhpprff({
+>                  erfcbafrGrkg: L.WFBA.fgevatvsl(choyvfurq_pbzzragf),
+
+> === 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();
+
+Good catch!
+
+>      grfgf.fhvgr.nqq(arj L.Grfg.Pnfr({
+>          anzr: 'grfg_nccebkvzngrqngr',
+>  
+
+
+-- 
 
=== added file 'lib/lp/services/tests/data/expected-6'
--- lib/lp/services/tests/data/expected-6	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/expected-6	2014-07-29 15:41:46 +0000
@@ -0,0 +1,44 @@
+
+
+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
+
+> @@ -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');
+
++1
+
+>              L.Nffreg.nerRdhny(
+>                  'Sbb One (anzr16) jebgr ba 2012-08-12:',
+>                  svefg.bar('.obneqPbzzragQrgnvyf').trg('grkg'));
+> 
+> === nqqrq svyr 'jrohv/obql_ot.wct'
+> Binary files jrohv/obql_ot.wct1970-01-01 00:00:00 +0000 and jrohv/obql_ot.wct19702014-01-31 16:57:19 +0000 differ
+
+Do we need this?
+
+> === 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();
+
+Can we change this?
+
+>      grfgf.fhvgr.nqq(arj L.Grfg.Pnfr({
+>          anzr: 'grfg_nccebkvzngrqngr',
+>  
+
+
+-- 
 
=== added file 'lib/lp/services/tests/data/expected-7'
--- lib/lp/services/tests/data/expected-7	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/expected-7	2014-07-29 15:41:46 +0000
@@ -0,0 +1,55 @@
+
+
+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();
+
+Good stuff
+
+>              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');
+
++1
+
+>              L.Nffreg.nerRdhny(
+>                  'Sbb One (anzr16) jebgr ba 2012-08-12:',
+>                  svefg.bar('.obneqPbzzragQrgnvyf').trg('grkg'));
+> === nqqrq svyr 'jrohv/obql_ot.wct'
+> Binary files jrohv/obql_ot.wct1970-01-01 00:00:00 +0000 and jrohv/obql_ot.wct19702014-01-31 16:57:19 +0000 differ
+
+Do we need this?
+
+> === 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();
+
+Can we change this?
+
+>      grfgf.fhvgr.nqq(arj L.Grfg.Pnfr({
+>          anzr: 'grfg_nccebkvzngrqngr',
+>  
+
+
+-- 
 
=== added file 'lib/lp/services/tests/data/insert_top.patch'
--- lib/lp/services/tests/data/insert_top.patch	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/insert_top.patch	2014-07-29 15:41:46 +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/services/tests/data/tricky-diff.patch'
--- lib/lp/services/tests/data/tricky-diff.patch	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/data/tricky-diff.patch	2014-07-29 15:41:46 +0000
@@ -0,0 +1,26 @@
+=== 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
 
=== added file 'lib/lp/services/tests/test_patches.py'
--- lib/lp/services/tests/test_patches.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/test_patches.py	2014-07-29 15:41:46 +0000
@@ -0,0 +1,127 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import os
+import re
+
+from bzrlib.errors import MalformedPatchHeader
+from bzrlib.patches import difference_index
+
+from lp.services.patches import (
+    BinaryPatch,
+    get_patch_names,
+    parse_patch,
+    parse_patches,
+    Patch,
+    )
+from lp.testing import TestCase
+
+
+def datafile(filename):
+    data_path = os.path.join(os.path.dirname(__file__),
+                             "data", filename)
+    return file(data_path, "rb")
+
+
+class TestBzrLibCode(TestCase):
+    """Test code rewritten from bzrlib.patches for diff comment emails"""
+
+    def assertContainsRe(self, haystack, needle_re, flags=0):
+        """Assert that a contains something matching a regular expression."""
+        if not re.search(needle_re, haystack, flags):
+            if '\n' in haystack or len(haystack) > 60:
+                # a long string, format it in a more readable way
+                raise AssertionError(
+                    'pattern "%s" not found in\n"""\\\n%s"""\n'
+                    % (needle_re, haystack))
+            else:
+                raise AssertionError('pattern "%s" not found in "%s"'
+                                     % (needle_re, haystack))
+
+    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("diff-1").read()
+        self.compare_parsed(patchtext)
+
+    def test_parse_binary(self):
+        """Test parsing a whole patch"""
+        diff_1 = datafile("diff-1").read()
+        diff_2 = datafile("diff-2").read()
+        diff = '\n'.join([diff_2, diff_1]).splitlines(True)
+        patches = parse_patches(diff)
+        self.assertIs(BinaryPatch, patches[0].__class__)
+        self.assertIs(Patch, patches[1].__class__)
+        self.assertContainsRe(
+            str(patches[0]),
+            'Binary files jrohv/obql_ot.wct1970-01-01.* and '
+            'jrohv/obql_ot.wct19702014-01-31.* differ\n')
+
+    def test_parse_binary_after_normal(self):
+        diff_1 = datafile("diff-1").read()
+        diff_2 = datafile("diff-2").read()
+        diff = '\n'.join([diff_1, diff_2]).splitlines(True)
+        patches = parse_patches(diff)
+        self.assertIs(BinaryPatch, patches[1].__class__)
+        self.assertIs(Patch, patches[0].__class__)
+        self.assertContainsRe(
+            str(patches[1]),
+            'Binary files jrohv/obql_ot.wct1970-01-01.* and '
+            'jrohv/obql_ot.wct19702014-01-31.* differ\n')
+
+    def test_roundtrip_binary(self):
+        diff = datafile("diff-3").read()
+        patches = parse_patches(diff.splitlines(True))
+        self.assertEqual(diff, ''.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 = ''.join(self.data_lines("tricky-diff.patch"))
+        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)
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2014-07-18 07:43:06 +0000
+++ lib/lp/testing/factory.py	2014-07-29 15:41:46 +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()