Merge lp:~cjwatson/launchpad/git-patch-headers into lp:launchpad

Proposed by Colin Watson
Status: Merged
Merged at revision: 17835
Proposed branch: lp:~cjwatson/launchpad/git-patch-headers
Merge into: lp:launchpad
Diff against target: 89 lines (+20/-5)
2 files modified
lib/lp/code/mail/patches.py (+11/-0)
lib/lp/code/mail/tests/test_codereviewcomment.py (+9/-5)
To merge this branch: bzr merge lp:~cjwatson/launchpad/git-patch-headers
Reviewer Review Type Date Requested Status
Kit Randel (community) Approve
Review via email: mp+275792@code.launchpad.net

Commit message

Parse extended header lines in git diffs correctly.

Description of the change

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".

To post a comment you must log in.
Revision history for this message
Kit Randel (blr) wrote :

That seems like a reasonably future-proof approach.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 13:03:57 +0000
@@ -360,6 +360,7 @@
360 dirty_head = []360 dirty_head = []
361 orig_range = 0361 orig_range = 0
362 beginning = True362 beginning = True
363 in_git_patch = False
363364
364 dirty_headers = ('=== ', 'diff ', 'index ')365 dirty_headers = ('=== ', 'diff ', 'index ')
365 for line in iter_lines:366 for line in iter_lines:
@@ -372,7 +373,16 @@
372 dirty_head = []373 dirty_head = []
373 else:374 else:
374 yield saved_lines375 yield saved_lines
376 in_git_patch = False
375 saved_lines = []377 saved_lines = []
378 if line.startswith('diff --git'):
379 in_git_patch = True
380 dirty_head.append(line)
381 continue
382 if in_git_patch and line and line[0].islower():
383 # Extended header line in a git diff. All extended header lines
384 # currently start with a lower-case character, and nothing else
385 # in the patch before the next "diff" header line can do so.
376 dirty_head.append(line)386 dirty_head.append(line)
377 continue387 continue
378 if line.startswith('*** '):388 if line.startswith('*** '):
@@ -395,6 +405,7 @@
395 dirty_head = []405 dirty_head = []
396 else:406 else:
397 yield saved_lines407 yield saved_lines
408 in_git_patch = False
398 saved_lines = []409 saved_lines = []
399 elif line.startswith('@@'):410 elif line.startswith('@@'):
400 hunk = hunk_from_header(line)411 hunk = hunk_from_header(line)
401412
=== 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 13:03:57 +0000
@@ -468,7 +468,9 @@
468 "-bar\n"468 "-bar\n"
469 "+baz\n"469 "+baz\n"
470 "diff --git a/fulano b/fulano\n"470 "diff --git a/fulano b/fulano\n"
471 "index 5716ca5..7601807 100644\n"471 "old mode 100644\n"
472 "new mode 100755\n"
473 "index 5716ca5..7601807\n"
472 "--- a/fulano\n"474 "--- a/fulano\n"
473 "+++ b/fulano\n"475 "+++ b/fulano\n"
474 "@@ -1,3 +1,3 @@\n"476 "@@ -1,3 +1,3 @@\n"
@@ -556,7 +558,7 @@
556 self.getSection(comments).splitlines()[7:11])558 self.getSection(comments).splitlines()[7:11])
557559
558 def test_comments_in_git_diff(self):560 def test_comments_in_git_diff(self):
559 comments = {'1': 'foo', '5': 'bar', '15': 'baz'}561 comments = {'1': 'foo', '5': 'bar', '17': 'baz'}
560 section = self.getSection(comments, diff_text=self.git_diff_text)562 section = self.getSection(comments, diff_text=self.git_diff_text)
561 self.assertEqual(563 self.assertEqual(
562 map(unicode, [564 map(unicode, [
@@ -574,7 +576,9 @@
574 "> -bar",576 "> -bar",
575 "> +baz",577 "> +baz",
576 "> diff --git a/fulano b/fulano",578 "> diff --git a/fulano b/fulano",
577 "> index 5716ca5..7601807 100644",579 "> old mode 100644",
580 "> new mode 100755",
581 "> index 5716ca5..7601807",
578 "> --- a/fulano",582 "> --- a/fulano",
579 "> +++ b/fulano",583 "> +++ b/fulano",
580 "> @@ -1,3 +1,3 @@",584 "> @@ -1,3 +1,3 @@",
@@ -585,7 +589,7 @@
585 "baz",589 "baz",
586 "",590 "",
587 "> +zutano"]),591 "> +zutano"]),
588 section.splitlines()[4:29])592 section.splitlines()[4:31])
589593
590 def test_commentless_hunks_ignored(self):594 def test_commentless_hunks_ignored(self):
591 # Hunks without inline comments are not returned in the diff text.595 # Hunks without inline comments are not returned in the diff text.
@@ -694,7 +698,7 @@
694 self.getSection(comments).splitlines()[6:12])698 self.getSection(comments).splitlines()[6:12])
695699
696 def test_multiple_comments(self):700 def test_multiple_comments(self):
697 # Multiple inline comments are redered appropriately.701 # Multiple inline comments are rendered appropriately.
698 comments = {'4': 'Foo', '5': 'Bar'}702 comments = {'4': 'Foo', '5': 'Bar'}
699 self.assertEqual(703 self.assertEqual(
700 ['> +++ bar.py\t1969-12-31 19:00:00.000000000 -0500',704 ['> +++ bar.py\t1969-12-31 19:00:00.000000000 -0500',