Bazaar

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

  1. bug-1400567
  2. Merge into bzr.dev
Proposed by Kit Randel
Status: Merged
Approved by: Richard Wilbur
Approved revision: no longer in the source branch.
Merged at revision: 6602
Proposed branch: lp:~blr/bzr/bug-1400567
Merge into: lp:bzr
Diff against target: 226 lines (+58/-20)
3 files modified
bzrlib/patches.py (+32/-9)
bzrlib/tests/test_patches.py (+22/-11)
doc/en/release-notes/bzr-2.7.txt (+4/-0)
To merge this branch: bzr merge lp:~blr/bzr/bug-1400567
Reviewer Review Type Date Requested Status
Richard Wilbur Approve
Vincent Ladeuil Approve
Review via email: mp+244531@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

> ./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
Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Kit Randel (blr) wrote :

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

Revision history for this message
Kit Randel (blr) wrote :

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

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

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

review: Approve
Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/patches.py'
2--- bzrlib/patches.py 2011-12-18 15:28:38 +0000
3+++ bzrlib/patches.py 2014-12-15 20:26:37 +0000
4@@ -31,9 +31,10 @@
5
6 binary_files_re = 'Binary files (.*) and (.*) differ\n'
7
8+
9 def get_patch_names(iter_lines):
10+ line = iter_lines.next()
11 try:
12- line = iter_lines.next()
13 match = re.match(binary_files_re, line)
14 if match is not None:
15 raise BinaryFiles(match.group(1), match.group(2))
16@@ -317,7 +318,6 @@
17 if isinstance(line, ContextLine):
18 pos += 1
19
20-
21 def parse_patch(iter_lines, allow_dirty=False):
22 '''
23 :arg iter_lines: iterable of lines to parse
24@@ -336,7 +336,7 @@
25 return patch
26
27
28-def iter_file_patch(iter_lines, allow_dirty=False):
29+def iter_file_patch(iter_lines, allow_dirty=False, keep_dirty=False):
30 '''
31 :arg iter_lines: iterable of lines to parse for patches
32 :kwarg allow_dirty: If True, allow comments and other non-patch text
33@@ -352,10 +352,15 @@
34 # (as allow_dirty does).
35 regex = re.compile(binary_files_re)
36 saved_lines = []
37+ dirty_head = []
38 orig_range = 0
39 beginning = True
40+
41 for line in iter_lines:
42- if line.startswith('=== ') or line.startswith('*** '):
43+ if line.startswith('=== '):
44+ dirty_head.append(line)
45+ continue
46+ if line.startswith('*** '):
47 continue
48 if line.startswith('#'):
49 continue
50@@ -369,14 +374,23 @@
51 # parse the patch
52 beginning = False
53 elif len(saved_lines) > 0:
54- yield saved_lines
55+ if keep_dirty and len(dirty_head) > 0:
56+ yield {'saved_lines': saved_lines,
57+ 'dirty_head': dirty_head}
58+ dirty_head = []
59+ else:
60+ yield saved_lines
61 saved_lines = []
62 elif line.startswith('@@'):
63 hunk = hunk_from_header(line)
64 orig_range = hunk.orig_range
65 saved_lines.append(line)
66 if len(saved_lines) > 0:
67- yield saved_lines
68+ if keep_dirty and len(dirty_head) > 0:
69+ yield {'saved_lines': saved_lines,
70+ 'dirty_head': dirty_head}
71+ else:
72+ yield saved_lines
73
74
75 def iter_lines_handle_nl(iter_lines):
76@@ -400,15 +414,24 @@
77 yield last_line
78
79
80-def parse_patches(iter_lines, allow_dirty=False):
81+def parse_patches(iter_lines, allow_dirty=False, keep_dirty=False):
82 '''
83 :arg iter_lines: iterable of lines to parse for patches
84 :kwarg allow_dirty: If True, allow text that's not part of the patch at
85 selected places. This includes comments before and after a patch
86 for instance. Default False.
87+ :kwarg keep_dirty: If True, returns a dict of patches with dirty headers.
88+ Default False.
89 '''
90- return [parse_patch(f.__iter__(), allow_dirty) for f in
91- iter_file_patch(iter_lines, allow_dirty)]
92+ patches = []
93+ for patch_lines in iter_file_patch(iter_lines, allow_dirty, keep_dirty):
94+ if 'dirty_head' in patch_lines:
95+ patches.append({'patch': parse_patch(
96+ patch_lines['saved_lines'], allow_dirty),
97+ 'dirty_head': patch_lines['dirty_head']})
98+ else:
99+ patches.append(parse_patch(patch_lines, allow_dirty))
100+ return patches
101
102
103 def difference_index(atext, btext):
104
105=== modified file 'bzrlib/tests/test_patches.py'
106--- bzrlib/tests/test_patches.py 2010-05-20 18:23:17 +0000
107+++ bzrlib/tests/test_patches.py 2014-12-15 20:26:37 +0000
108@@ -58,10 +58,24 @@
109 # https://bugs.launchpad.net/bzr/+bug/502076
110 # https://code.launchpad.net/~toshio/bzr/allow-dirty-patches/+merge/18854
111 lines = ["diff -pruN commands.py",
112- "--- orig/commands.py",
113- "+++ mod/dommands.py"]
114+ "--- orig/commands.py",
115+ "+++ mod/dommands.py"]
116 bits = parse_patches(iter(lines), allow_dirty=True)
117
118+ def test_preserve_dirty_head(self):
119+ """Parse a patch containing a dirty header, and preserve lines"""
120+ lines = ["=== added directory 'foo/bar'\n",
121+ "=== modified file 'orig/commands.py'\n",
122+ "--- orig/commands.py\n",
123+ "+++ mod/dommands.py\n"]
124+ patches = parse_patches(
125+ lines.__iter__(), allow_dirty=True, keep_dirty=True)
126+ self.assertEqual(patches[0]['dirty_head'],
127+ ["=== added directory 'foo/bar'\n",
128+ "=== modified file 'orig/commands.py'\n"])
129+ self.assertEqual(patches[0]['patch'].get_header().splitlines(True),
130+ ["--- orig/commands.py\n", "+++ mod/dommands.py\n"])
131+
132 def testValidPatchHeader(self):
133 """Parse a valid patch header"""
134 lines = "--- orig/commands.py\n+++ mod/dommands.py\n".split('\n')
135@@ -78,7 +92,7 @@
136 def testValidHunkHeader(self):
137 """Parse a valid hunk header"""
138 header = "@@ -34,11 +50,6 @@\n"
139- hunk = hunk_from_header(header);
140+ hunk = hunk_from_header(header)
141 self.assertEqual(hunk.orig_pos, 34)
142 self.assertEqual(hunk.orig_range, 11)
143 self.assertEqual(hunk.mod_pos, 50)
144@@ -88,7 +102,7 @@
145 def testValidHunkHeader2(self):
146 """Parse a tricky, valid hunk header"""
147 header = "@@ -1 +0,0 @@\n"
148- hunk = hunk_from_header(header);
149+ hunk = hunk_from_header(header)
150 self.assertEqual(hunk.orig_pos, 1)
151 self.assertEqual(hunk.orig_range, 1)
152 self.assertEqual(hunk.mod_pos, 0)
153@@ -117,7 +131,7 @@
154 self.makeMalformed("@@ -34,11 +50,6.5 @@\n")
155 self.makeMalformed("@@ -34,11 +50,-6 @@\n")
156
157- def lineThing(self,text, type):
158+ def lineThing(self, text, type):
159 line = parse_line(text)
160 self.assertIsInstance(line, type)
161 self.assertEqual(str(line), text)
162@@ -146,7 +160,7 @@
163 i = difference_index(patchtext, pstr)
164 if i is not None:
165 print "%i: \"%s\" != \"%s\"" % (i, patchtext[i], pstr[i])
166- self.assertEqual (patchtext, str(patch))
167+ self.assertEqual(patchtext, str(patch))
168
169 def testAll(self):
170 """Test parsing a whole patch"""
171@@ -161,7 +175,7 @@
172 self.assertContainsRe(patches[0].oldname, '^bar\t')
173 self.assertContainsRe(patches[0].newname, '^qux\t')
174 self.assertContainsRe(str(patches[0]),
175- 'Binary files bar\t.* and qux\t.* differ\n')
176+ 'Binary files bar\t.* and qux\t.* differ\n')
177
178 def test_parse_binary_after_normal(self):
179 patches = parse_patches(self.data_lines("binary-after-normal.patch"))
180@@ -170,7 +184,7 @@
181 self.assertContainsRe(patches[1].oldname, '^bar\t')
182 self.assertContainsRe(patches[1].newname, '^qux\t')
183 self.assertContainsRe(str(patches[1]),
184- 'Binary files bar\t.* and qux\t.* differ\n')
185+ 'Binary files bar\t.* and qux\t.* differ\n')
186
187 def test_roundtrip_binary(self):
188 patchtext = ''.join(self.data_lines("binary.patch"))
189@@ -228,7 +242,6 @@
190 mod_lines = list(self.datafile(mod))
191
192 patched_file = IterableFile(iter_patched(orig_lines, patch))
193- lines = []
194 count = 0
195 for patch_line in patched_file:
196 self.assertEqual(patch_line, mod_lines[count])
197@@ -239,7 +252,6 @@
198 binary_lines = self.data_lines('binary.patch')
199 e = self.assertRaises(BinaryFiles, iter_patched, [], binary_lines)
200
201-
202 def test_iter_patched_from_hunks(self):
203 """Test a few patch files, and make sure they work."""
204 files = [
205@@ -256,7 +268,6 @@
206 mod_lines = list(self.datafile(mod))
207 iter_patched = iter_patched_from_hunks(orig_lines, parsed.hunks)
208 patched_file = IterableFile(iter_patched)
209- lines = []
210 count = 0
211 for patch_line in patched_file:
212 self.assertEqual(patch_line, mod_lines[count])
213
214=== modified file 'doc/en/release-notes/bzr-2.7.txt'
215--- doc/en/release-notes/bzr-2.7.txt 2014-06-19 09:42:08 +0000
216+++ doc/en/release-notes/bzr-2.7.txt 2014-12-15 20:26:37 +0000
217@@ -26,6 +26,10 @@
218 .. Improvements to existing commands, especially improved performance
219 or memory usage, or better results.
220
221+* bzrlib.patches.parse_patches can optionally return a list of 'dirty'
222+ patch headers (prefixed with '===').
223+ (Kit Randel, #1400567)
224+
225 Bug Fixes
226 *********
227