launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-patch-headers into lp:launchpad.

Commit message:
Parse extended header lines in git diffs correctly.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1510337 in Launchpad itself: "Extended headers in git diffs break patch parsing"
  https://bugs.launchpad.net/launchpad/+bug/1510337

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-patch-headers/+merge/275792

Parse extended header lines in git diffs correctly.

https://git.kernel.org/cgit/git/git.git/tree/Documentation/diff-generate-patch.txt documents the file format here.  Rather than either (a) hardcoding the longish list of possible extended header lines or (b) being more liberal with all "dirty header" lines and risking disturbing parsing of Bazaar diffs, I opted for (c) instead, namely to accept anything starting with a lower-case letter as an extended header line provided that it hasn't already matched one of dirty_headers, but only if it's after a line starting with "diff --git".
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-patch-headers into lp:launchpad.

=== modified file 'lib/lp/code/mail/patches.py'
--- lib/lp/code/mail/patches.py	2015-07-09 05:40:01 +0000
+++ lib/lp/code/mail/patches.py	2015-10-27 01:49:46 +0000
@@ -360,6 +360,7 @@
     dirty_head = []
     orig_range = 0
     beginning = True
+    in_git_patch = False
 
     dirty_headers = ('=== ', 'diff ', 'index ')
     for line in iter_lines:
@@ -372,7 +373,14 @@
                     dirty_head = []
                 else:
                     yield saved_lines
+                in_git_patch = False
                 saved_lines = []
+            if line.startswith('diff --git'):
+                in_git_patch = True
+            dirty_head.append(line)
+            continue
+        if in_git_patch and line and line[0].islower():
+            # Extended header line in a git diff.
             dirty_head.append(line)
             continue
         if line.startswith('*** '):
@@ -395,6 +403,7 @@
                     dirty_head = []
                 else:
                     yield saved_lines
+                in_git_patch = False
             saved_lines = []
         elif line.startswith('@@'):
             hunk = hunk_from_header(line)

=== modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
--- lib/lp/code/mail/tests/test_codereviewcomment.py	2015-09-11 12:20:23 +0000
+++ lib/lp/code/mail/tests/test_codereviewcomment.py	2015-10-27 01:49:46 +0000
@@ -468,7 +468,9 @@
         "-bar\n"
         "+baz\n"
         "diff --git a/fulano b/fulano\n"
-        "index 5716ca5..7601807 100644\n"
+        "old mode 100644\n"
+        "new mode 100755\n"
+        "index 5716ca5..7601807\n"
         "--- a/fulano\n"
         "+++ b/fulano\n"
         "@@ -1,3 +1,3 @@\n"
@@ -556,7 +558,7 @@
             self.getSection(comments).splitlines()[7:11])
 
     def test_comments_in_git_diff(self):
-        comments = {'1': 'foo', '5': 'bar', '15': 'baz'}
+        comments = {'1': 'foo', '5': 'bar', '17': 'baz'}
         section = self.getSection(comments, diff_text=self.git_diff_text)
         self.assertEqual(
             map(unicode, [
@@ -574,7 +576,9 @@
                 "> -bar",
                 "> +baz",
                 "> diff --git a/fulano b/fulano",
-                "> index 5716ca5..7601807 100644",
+                "> old mode 100644",
+                "> new mode 100755",
+                "> index 5716ca5..7601807",
                 "> --- a/fulano",
                 "> +++ b/fulano",
                 "> @@ -1,3 +1,3 @@",
@@ -585,7 +589,7 @@
                 "baz",
                 "",
                 "> +zutano"]),
-            section.splitlines()[4:29])
+            section.splitlines()[4:31])
 
     def test_commentless_hunks_ignored(self):
         # Hunks without inline comments are not returned in the diff text.
@@ -694,7 +698,7 @@
             self.getSection(comments).splitlines()[6:12])
 
     def test_multiple_comments(self):
-        # Multiple inline comments are redered appropriately.
+        # Multiple inline comments are rendered appropriately.
         comments = {'4': 'Foo', '5': 'Bar'}
         self.assertEqual(
             ['> +++ bar.py\t1969-12-31 19:00:00.000000000 -0500',