Merge lp:~blr/bzr/bug-1400567 into lp:bzr

[Kit Randel]

Added keep_dirty kwarg to bzrlib.patches.parse_patches() allowing preservation of dirty patch headers.

Summary

Launchpad currently provides emails to users when inline diff comments are made in the context of a branch merge proposal. These emails tend to be very verbose (https://bugs.launchpad.net/launchpad/+bug/1334577) and a branch has been provided to try to address this (https://code.launchpad.net/~blr/launchpad/bug-1334577-verbose-diff/+merge/243751), by discarding patches and hunks without comments. The implementation parses launchpad diffs with bzrlib.patches, however in order for comment line numbers and diff line numbers to correspond correctly, bzrlib.patches need to be aware of dirty headers such as file 'state' headers that may precede a patch e.g.

    "=== added directory 'bish/bosh'"
    "=== modified file 'foo/bar.py'"

Proposed fix
The solution proposed in this branch is to add a keep_dirty kwarg to parse_patches(). When this is set to True, parse_patches will return a dict containing both the patch and its associated dirty_headers. This will allow us to reconstruct the original diff correctly in launchpad.

Tests
A relevant test has been added in test_patches.PatchesTester.test_preserve_dirty_head and can be run with:

./bzr selftest -v patches.PatchesTester

[Vincent Ladeuil (vila)]

> ./bzr selftest -v patches.PatchesTester

Thanks so much for telling how to test the MP !

As a thank you hint: try './bzr selftest -v -s bt.test_patches' next time ;) The difference is that your command is less precise as it can match more tests, -s (--start-with) also avoid loading tests that won't be run

Overall, your approach is very clean and well tested.

I don't really like changing the return signature depending on an input parameter (especially for a generator) but I agree with your simpler approach, we can do refactoring later if needed (which is unclear) so that all call sites use your better returned values.

Thanks for the pep8 cleanups too which account for half of this proposal. Very much appreciated.

review: Approve

[Vincent Ladeuil (vila)]

Oh, forgot one bit: you need to add an news entry for that bugfix.

[Kit Randel (blr)]

Thanks Vincent!

Regarding the return signature, I felt similarly, but was concerned about
breaking the API.

In terms of adding a news entry, presumably I
ammend doc/en/release-notes/bzr-2.7.txt?

On Tue, Dec 16, 2014 at 12:55 AM, Vincent Ladeuil <email address hidden>
wrote:
>
> Oh, forgot one bit: you need to add an news entry for that bugfix.
> --
> https://code.launchpad.net/~blr/bzr/bug-1400567/+merge/244531
> You are the owner of lp:~blr/bzr/bug-1400567.
>

--
Kit Randel
Canonical - Ubuntu Engineering - Continuous Integration Team

[Kit Randel (blr)]

Added a release note under 'improvements' for 2.7b.

[Richard Wilbur (richard-wilbur)]

Thanks for the news entry, the code cleanup (pep8), and the fix!
I approve.
+1

review: Approve

[Richard Wilbur (richard-wilbur)]

sent to pqm by email

=== modified file 'bzrlib/patches.py'
--- bzrlib/patches.py	2011-12-18 15:28:38 +0000
+++ bzrlib/patches.py	2014-12-15 20:26:37 +0000
@@ -31,9 +31,10 @@
 
 binary_files_re = 'Binary files (.*) and (.*) differ\n'
 
+
 def get_patch_names(iter_lines):
+    line = iter_lines.next()
     try:
-        line = iter_lines.next()
         match = re.match(binary_files_re, line)
         if match is not None:
             raise BinaryFiles(match.group(1), match.group(2))
@@ -317,7 +318,6 @@
                 if isinstance(line, ContextLine):
                     pos += 1
 
-
 def parse_patch(iter_lines, allow_dirty=False):
     '''
     :arg iter_lines: iterable of lines to parse
@@ -336,7 +336,7 @@
         return patch
 
 
-def iter_file_patch(iter_lines, allow_dirty=False):
+def iter_file_patch(iter_lines, allow_dirty=False, keep_dirty=False):
     '''
     :arg iter_lines: iterable of lines to parse for patches
     :kwarg allow_dirty: If True, allow comments and other non-patch text
@@ -352,10 +352,15 @@
     # (as allow_dirty does).
     regex = re.compile(binary_files_re)
     saved_lines = []
+    dirty_head = []
     orig_range = 0
     beginning = True
+
     for line in iter_lines:
-        if line.startswith('=== ') or line.startswith('*** '):
+        if line.startswith('=== '):
+            dirty_head.append(line)
+            continue
+        if line.startswith('*** '):
             continue
         if line.startswith('#'):
             continue
@@ -369,14 +374,23 @@
                 # parse the patch
                 beginning = False
             elif len(saved_lines) > 0:
-                yield saved_lines
+                if keep_dirty and len(dirty_head) > 0:
+                    yield {'saved_lines': saved_lines,
+                           'dirty_head': dirty_head}
+                    dirty_head = []
+                else:
+                    yield saved_lines
             saved_lines = []
         elif line.startswith('@@'):
             hunk = hunk_from_header(line)
             orig_range = hunk.orig_range
         saved_lines.append(line)
     if len(saved_lines) > 0:
-        yield saved_lines
+        if keep_dirty and len(dirty_head) > 0:
+            yield {'saved_lines': saved_lines,
+                   'dirty_head': dirty_head}
+        else:
+            yield saved_lines
 
 
 def iter_lines_handle_nl(iter_lines):
@@ -400,15 +414,24 @@
         yield last_line
 
 
-def parse_patches(iter_lines, allow_dirty=False):
+def parse_patches(iter_lines, allow_dirty=False, keep_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.
+    :kwarg keep_dirty: If True, returns a dict of patches with dirty headers.
+        Default False.
     '''
-    return [parse_patch(f.__iter__(), allow_dirty) for f in
-                        iter_file_patch(iter_lines, allow_dirty)]
+    patches = []
+    for patch_lines in iter_file_patch(iter_lines, allow_dirty, keep_dirty):
+        if 'dirty_head' in patch_lines:
+            patches.append({'patch': parse_patch(
+                patch_lines['saved_lines'], allow_dirty),
+                            'dirty_head': patch_lines['dirty_head']})
+        else:
+            patches.append(parse_patch(patch_lines, allow_dirty))
+    return patches
 
 
 def difference_index(atext, btext):
 
=== modified file 'bzrlib/tests/test_patches.py'
--- bzrlib/tests/test_patches.py	2010-05-20 18:23:17 +0000
+++ bzrlib/tests/test_patches.py	2014-12-15 20:26:37 +0000
@@ -58,10 +58,24 @@
         # https://bugs.launchpad.net/bzr/+bug/502076
         # https://code.launchpad.net/~toshio/bzr/allow-dirty-patches/+merge/18854
         lines = ["diff -pruN commands.py",
-            "--- orig/commands.py",
-            "+++ mod/dommands.py"]
+                 "--- orig/commands.py",
+                 "+++ mod/dommands.py"]
         bits = parse_patches(iter(lines), allow_dirty=True)
 
+    def test_preserve_dirty_head(self):
+        """Parse a patch containing a dirty header, and preserve lines"""
+        lines = ["=== added directory 'foo/bar'\n",
+                 "=== modified file 'orig/commands.py'\n",
+                 "--- orig/commands.py\n",
+                 "+++ mod/dommands.py\n"]
+        patches = parse_patches(
+            lines.__iter__(), allow_dirty=True, keep_dirty=True)
+        self.assertEqual(patches[0]['dirty_head'],
+                         ["=== added directory 'foo/bar'\n",
+                          "=== modified file 'orig/commands.py'\n"])
+        self.assertEqual(patches[0]['patch'].get_header().splitlines(True),
+                         ["--- orig/commands.py\n", "+++ mod/dommands.py\n"])
+
     def testValidPatchHeader(self):
         """Parse a valid patch header"""
         lines = "--- orig/commands.py\n+++ mod/dommands.py\n".split('\n')
@@ -78,7 +92,7 @@
     def testValidHunkHeader(self):
         """Parse a valid hunk header"""
         header = "@@ -34,11 +50,6 @@\n"
-        hunk = hunk_from_header(header);
+        hunk = hunk_from_header(header)
         self.assertEqual(hunk.orig_pos, 34)
         self.assertEqual(hunk.orig_range, 11)
         self.assertEqual(hunk.mod_pos, 50)
@@ -88,7 +102,7 @@
     def testValidHunkHeader2(self):
         """Parse a tricky, valid hunk header"""
         header = "@@ -1 +0,0 @@\n"
-        hunk = hunk_from_header(header);
+        hunk = hunk_from_header(header)
         self.assertEqual(hunk.orig_pos, 1)
         self.assertEqual(hunk.orig_range, 1)
         self.assertEqual(hunk.mod_pos, 0)
@@ -117,7 +131,7 @@
         self.makeMalformed("@@ -34,11 +50,6.5 @@\n")
         self.makeMalformed("@@ -34,11 +50,-6 @@\n")
 
-    def lineThing(self,text, type):
+    def lineThing(self, text, type):
         line = parse_line(text)
         self.assertIsInstance(line, type)
         self.assertEqual(str(line), text)
@@ -146,7 +160,7 @@
         i = difference_index(patchtext, pstr)
         if i is not None:
             print "%i: \"%s\" != \"%s\"" % (i, patchtext[i], pstr[i])
-        self.assertEqual (patchtext, str(patch))
+        self.assertEqual(patchtext, str(patch))
 
     def testAll(self):
         """Test parsing a whole patch"""
@@ -161,7 +175,7 @@
         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')
+                              '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"))
@@ -170,7 +184,7 @@
         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')
+                              'Binary files bar\t.* and qux\t.* differ\n')
 
     def test_roundtrip_binary(self):
         patchtext = ''.join(self.data_lines("binary.patch"))
@@ -228,7 +242,6 @@
             mod_lines = list(self.datafile(mod))
 
             patched_file = IterableFile(iter_patched(orig_lines, patch))
-            lines = []
             count = 0
             for patch_line in patched_file:
                 self.assertEqual(patch_line, mod_lines[count])
@@ -239,7 +252,6 @@
         binary_lines = self.data_lines('binary.patch')
         e = self.assertRaises(BinaryFiles, iter_patched, [], binary_lines)
 
-
     def test_iter_patched_from_hunks(self):
         """Test a few patch files, and make sure they work."""
         files = [
@@ -256,7 +268,6 @@
             mod_lines = list(self.datafile(mod))
             iter_patched = iter_patched_from_hunks(orig_lines, parsed.hunks)
             patched_file = IterableFile(iter_patched)
-            lines = []
             count = 0
             for patch_line in patched_file:
                 self.assertEqual(patch_line, mod_lines[count])
 
=== modified file 'doc/en/release-notes/bzr-2.7.txt'
--- doc/en/release-notes/bzr-2.7.txt	2014-06-19 09:42:08 +0000
+++ doc/en/release-notes/bzr-2.7.txt	2014-12-15 20:26:37 +0000
@@ -26,6 +26,10 @@
 .. Improvements to existing commands, especially improved performance 
    or memory usage, or better results.
 
+* bzrlib.patches.parse_patches can optionally return a list of 'dirty'
+  patch headers (prefixed with '===').
+  (Kit Randel, #1400567)
+
 Bug Fixes
 *********