Merge lp:~cjwatson/bzr/fix-keep-dirty into lp:bzr

Proposed by Colin Watson on 2015-07-02
Status: Merged
Approved by: Vincent Ladeuil on 2015-07-02
Approved revision: 6605
Merged at revision: 6605
Proposed branch: lp:~cjwatson/bzr/fix-keep-dirty
Merge into: lp:bzr
Diff against target: 46 lines (+17/-1)
2 files modified
bzrlib/patches.py (+8/-0)
bzrlib/tests/test_patches.py (+9/-1)
To merge this branch: bzr merge lp:~cjwatson/bzr/fix-keep-dirty
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve on 2015-07-02
William Grant 2015-07-02 Approve on 2015-07-02
Review via email: mp+263632@code.launchpad.net

Commit message

Avoid associating dirty patch headers with the previous file in the patch.

Description of the change

The keep_dirty feature added in https://code.launchpad.net/~blr/bzr/bug-1400567/+merge/244531 turns out to be slightly buggy. You can see the effect by inspecting the quoted "===" lines in http://paste.ubuntu.com/11809068/ - all dirty headers except the first are wrongly associated with the previous file in the patch.

To fix this, when we encounter a "===" line, check whether there are any saved lines and if so emit them at that point before continuing.

I extended the existing tests slightly to cover this (./bzr selftest -v -s bt.test_patches), and haven't bothered adding a release note since this is a bug-fix to a change that as yet hasn't been part of a bzr release.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve
Vincent Ladeuil (vila) wrote :

Thanks for fixing that.

Just a minor nit if you don't mind ;) see inline comments.

Good to land otherwise.

review: Needs Fixing
lp:~cjwatson/bzr/fix-keep-dirty updated on 2015-07-02
6605. By Colin Watson on 2015-07-02

Use assertLength.

Vincent Ladeuil (vila) :
review: Approve
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Vincent Ladeuil (vila) wrote :

sent to pqm by email

Vincent Ladeuil (vila) wrote :

sent to pqm by email

Richard Wilbur (richard-wilbur) wrote :

I have an open rt ticket with ubuntu.com regarding pqm. I am bugging them again about it because they said they found an issue 10 days ago and would get back to me when they had it resolved.

(By the way, thanks Colin for the fix. And thanks Vincent and William for participating in the review process.)

Richard Wilbur (richard-wilbur) wrote :

The open rt ticket regarding pqm is #26672 at ubuntu.com.

Vincent Ladeuil (vila) 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 2014-12-12 03:59:25 +0000
3+++ bzrlib/patches.py 2015-07-02 11:31:08 +0000
4@@ -358,6 +358,14 @@
5
6 for line in iter_lines:
7 if line.startswith('=== '):
8+ if len(saved_lines) > 0:
9+ if keep_dirty and len(dirty_head) > 0:
10+ yield {'saved_lines': saved_lines,
11+ 'dirty_head': dirty_head}
12+ dirty_head = []
13+ else:
14+ yield saved_lines
15+ saved_lines = []
16 dirty_head.append(line)
17 continue
18 if line.startswith('*** '):
19
20=== modified file 'bzrlib/tests/test_patches.py'
21--- bzrlib/tests/test_patches.py 2014-12-12 03:59:25 +0000
22+++ bzrlib/tests/test_patches.py 2015-07-02 11:31:08 +0000
23@@ -67,14 +67,22 @@
24 lines = ["=== added directory 'foo/bar'\n",
25 "=== modified file 'orig/commands.py'\n",
26 "--- orig/commands.py\n",
27- "+++ mod/dommands.py\n"]
28+ "+++ mod/dommands.py\n",
29+ "=== modified file 'orig/another.py'\n",
30+ "--- orig/another.py\n",
31+ "+++ mod/another.py\n"]
32 patches = parse_patches(
33 lines.__iter__(), allow_dirty=True, keep_dirty=True)
34+ self.assertLength(2, patches)
35 self.assertEqual(patches[0]['dirty_head'],
36 ["=== added directory 'foo/bar'\n",
37 "=== modified file 'orig/commands.py'\n"])
38 self.assertEqual(patches[0]['patch'].get_header().splitlines(True),
39 ["--- orig/commands.py\n", "+++ mod/dommands.py\n"])
40+ self.assertEqual(patches[1]['dirty_head'],
41+ ["=== modified file 'orig/another.py'\n"])
42+ self.assertEqual(patches[1]['patch'].get_header().splitlines(True),
43+ ["--- orig/another.py\n", "+++ mod/another.py\n"])
44
45 def testValidPatchHeader(self):
46 """Parse a valid patch header"""