Bazaar

Merge lp:~gerard-/bzr/update into lp:bzr

  1. update
  2. Merge into bzr.dev
Proposed by Gerard Krol
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~gerard-/bzr/update
Merge into: lp:bzr
Diff against target: 312 lines (+180/-49)
4 files modified
NEWS (+4/-0)
bzrlib/tests/blackbox/test_commit.py (+7/-0)
bzrlib/tests/blackbox/test_update.py (+131/-8)
bzrlib/workingtree.py (+38/-41)
To merge this branch: bzr merge lp:~gerard-/bzr/update
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
John A Meinel Approve
Review via email: mp+18464@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Gerard Krol (gerard-) wrote :

I don't really proposing to merge this as-is, but I think I solved the double merge problem update had for a long time. It will now perform the merges in a order that will minimize conflicts (first the local branch, then the master) and also stop after it encounters a conflict for the first merge. The only problem is really that I don't understand how it works...

I also added some tests for the new behaviour.

Please comment!

Revision history for this message
Martin Pool (mbp) wrote :

Thanks for tackling this!

Are there specific bugs that you think this will fix?

Revision history for this message
Gerard Krol (gerard-) wrote :

Just bug #113809 (update performs two merges) + the 6 duplicates bug #116655, bug #175589, bug #228506, bug #236724, bug #270665 and bug #395514

Revision history for this message
Martin Pool (mbp) wrote :

just that hey? :-)

--
Martin <http://launchpad.net/~mbp/>

Revision history for this message
Vincent Ladeuil (vila) wrote :

I let John review this as he's more familiar that code.

Just a couple of comments on some details.

While you're tweaking, can you fix the lines too long
(see doc/developers/HAKCING.txt Coding Style Guidelines) ?

Also, if you disagree with some comments, update them instead
of deleting them (bzrlib/tests/blackbox/test_update.py).

Can you also have a look at http://www.canonical.com/contributors so we can merge your patch
once it is approved ?

You may also add a NEWS entry mentioning the bugs you're addressing.
Make sure you have tests reproducing them.

Revision history for this message
John A Meinel (jameinel) wrote :
Download full text (5.3 KiB)

8 + a_file = file('u1/hosts', 'rt')
9 + text = a_file.read()
10 + a_file.close()
11 + self.assertEqual(text, '<<<<<<< TREE\nfirst offline change in u1=======\naltered in u2>>>>>>> MERGE-SOURCE\n')
12 +

^- I think this is better written as

self.assertFileEqual('u1/hosts', '<<<<<< ...')

If that doesn't work because of line endings (it may be done in 'b' instead of
't' mode), then at least use:

 self.assertEqualDiff("""<<<<<<< TREE
first offline change in u1...
""", text)

We generally put 'expected' first, and assertEqualDiff gives nice output when
they aren't equal.

You have this a few times, and the lines are longer than 80 chars. (And some
other comments, etc are also >80 chars.)

I think this would do well with a clear statement of steps. Probably in a
comment somewhere. At least, it helped me sort it out. The old code did:

1) old_tip = self.branch.update() (go from branch.last_revision() =>
   master_branch.last_revision(), return old last_revision())
2) last_rev = basis_revision for current working tree
3) revision = new master_branch.last_revision()
4) merge_inner(last_rev => revision)
5) set_parent_trees(revision + existing_merges)
6) base_rev_id = unique_lca(revision, old_tip)
7) merge_inner(base_rev_id => old_tip)

Which thinking about it, does seem a bit backwards. In the above analysis we
have these revisions at play

W wt.last_revision()
B wt.branch.last_revision()
M wt.branch.get_master_branch().last_revision()
P wt.get_parent_ids()[1:], things that claim to be merged into the WT
    already

The goal is to get from W to M in the most logical procession, but then also
handle that some of W (or B) may not be in M, so they have to be re-introduced.
Actually, I think it is that revisions in B that are not in M need to be
re-introduced, but I'm not sure if we care about revisions in W that aren't
in B or M. (though we certainly do care about P.)

So the current code does:
merge_inner(W => M)
    Change the workingtree directly into the new master revision
merge_inner(ancestor(W,B) => W)
    And re-merge the changes in the Branch that were not present in the Master

The new code does:

1) [same] old_tip = self.branch.update()
2) [same] last_rev = basis_revision for current working tree
3) [same] revision = master_branch.last_revision()
4) if old_tip != revision
     merge_inner(ancestor(revision, old_tip) => old_tip)
   if conflicts => stop (mark old_tip as merged into current tree)
5) if not merged:
    merge_inner(last_rev => revision)
   else:
    merge_inner(ancestor(revision, last_rev) => revision)

So I think this is:
merge_inner(ancestor(M,B) => B)
merge_inner(ancestor(M,W) => M)

# Note: I think one thing that neither code handles is when some of the
# revisions in M are already part of P, and thus probably shouldn't be
# re-introduced

So what does that do with a real graph

    1
    |\
    2 3
    | |
  B-4 5-M
    |
    W

The old code would do:
    apply the difference from W to M (revert 2,4, apply 3,5)
    merge (1 => B) to M (apply 2,4)

Which should change the graph to look like:

   1
   |\
   3 2
   | |
MB-5 4
   |/
   W

Which is ~ what I would expect. It probably suffers from the probl...

Read more...

review: Needs Information
Revision history for this message
John A Meinel (jameinel) wrote :

Just a thought....

Maybe what we want is:

if wt.get_parent_ids()[0] != wt.branch.last_revision():
  merge_inner(wt.get_parent_ids()[0] => wt.branch.last_revision())
merge_inner(ancestor(wt.branch.last_revision(), revision), revision)

Revision history for this message
Gerard Krol (gerard-) wrote :

> Also, if you disagree with some comments, update them instead
> of deleting them (bzrlib/tests/blackbox/test_update.py).

Those comments were identical to each other and to the comments on the test they were copied from. The best I can do at this point is remove them, as inventing my own will not add any information beyond what is already visible from the code. I might even misunderstand the code, and wrong comments are even worse than none.

Revision history for this message
Gerard Krol (gerard-) wrote :

> While you're tweaking, can you fix the lines too long

Fixed

Revision history for this message
Gerard Krol (gerard-) wrote :

> 8 + a_file = file('u1/hosts', 'rt')
> 9 + text = a_file.read()
> 10 + a_file.close()
> 11 + self.assertEqual(text, '<<<<<<< TREE\nfirst offline change in
> u1=======\naltered in u2>>>>>>> MERGE-SOURCE\n')
> 12 +
>
>
> ^- I think this is better written as
>
> self.assertFileEqual('u1/hosts', '<<<<<< ...')

Fixed

Revision history for this message
Gerard Krol (gerard-) wrote :

I FINALLY understand the issue (thanks John!)

I think that this is ready for the merge now.

Revision history for this message
Gerard Krol (gerard-) wrote :

I'll talk with my boss about signing the "Canonical Contributor Agreement" this Monday.

If there are any additional changes needed to the patch or actions to take please let me know.

Revision history for this message
Gerard Krol (gerard-) wrote :

> I'll talk with my boss about signing the "Canonical Contributor Agreement"
> this Monday.

Signed it.

Revision history for this message
John A Meinel (jameinel) wrote :

This fits my expectations. If one of the merging steps creates conflicts, it stops as expected. It also looks like it should not spuriously remove committed changes, causing conflicts in uncommitted changes when bringing in the updates from the master branch.

We should still get a second review, I think.

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

I've cleanup the tests a bit to be real whitebox ones based on branchbuilder.
I still the feeling that we don't have enough tests here but that's good enough for now.

I'll merge.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-02-05 10:57:26 +0000
3+++ NEWS 2010-02-06 21:29:15 +0000
4@@ -67,6 +67,10 @@
5 the same value to avoid confusing ``make`` and other date-based build
6 systems. (Robert Collins, #515631)
7
8+* ``bzr update`` performs the two merges in a more logical order and will stop
9+ when it encounters conflicts.
10+ (Gerard Krol, #113809)
11+
12 Improvements
13 ************
14
15
16=== modified file 'bzrlib/tests/blackbox/test_commit.py'
17--- bzrlib/tests/blackbox/test_commit.py 2010-01-25 17:48:22 +0000
18+++ bzrlib/tests/blackbox/test_commit.py 2010-02-06 21:29:15 +0000
19@@ -359,6 +359,13 @@
20 # work as a merge
21 # retcode 1 as we expect a text conflict
22 self.run_bzr('update u1', retcode=1)
23+ self.assertFileEqual('<<<<<<< TREE\n'
24+ 'first offline change in u1'
25+ '=======\n'
26+ 'altered in u2'
27+ '>>>>>>> MERGE-SOURCE\n',
28+ 'u1/hosts')
29+
30 self.run_bzr('resolved u1/hosts')
31 # add a text change here to represent resolving the merge conflicts in
32 # favour of a new version of the file not identical to either the u1
33
34=== modified file 'bzrlib/tests/blackbox/test_update.py'
35--- bzrlib/tests/blackbox/test_update.py 2009-12-23 06:31:19 +0000
36+++ bzrlib/tests/blackbox/test_update.py 2010-02-06 21:29:15 +0000
37@@ -171,9 +171,9 @@
38 # get all three files and a pending merge.
39 out, err = self.run_bzr('update checkout')
40 self.assertEqual('', out)
41- self.assertEqualDiff("""+N file
42+ self.assertEqualDiff("""+N file_b
43 All changes applied successfully.
44-+N file_b
45++N file
46 All changes applied successfully.
47 Updated to revision 1 of branch %s
48 Your local commits will now show as pending merges with 'bzr status', and can be committed with 'bzr commit'.
49@@ -242,9 +242,6 @@
50 self.run_bzr('update checkout')
51
52 def test_update_dash_r(self):
53- # Test that 'bzr update' works correctly when you have
54- # an update in the master tree, and a lightweight checkout
55- # which has merged another branch
56 master = self.make_branch_and_tree('master')
57 os.chdir('master')
58 self.build_tree(['./file1'])
59@@ -266,9 +263,6 @@
60 self.assertEquals(['m1'], master.get_parent_ids())
61
62 def test_update_dash_r_outside_history(self):
63- # Test that 'bzr update' works correctly when you have
64- # an update in the master tree, and a lightweight checkout
65- # which has merged another branch
66 master = self.make_branch_and_tree('master')
67 self.build_tree(['master/file1'])
68 master.add(['file1'])
69@@ -315,3 +309,132 @@
70 2>All changes applied successfully.
71 2>Updated to revision 2 of branch .../master
72 ''')
73+
74+ def test_update_checkout_prevent_double_merge(self):
75+ """"Launchpad bug 113809 in bzr "update performs two merges"
76+ https://launchpad.net/bugs/113809"""
77+ master = self.make_branch_and_tree('master')
78+ self.build_tree_contents([('master/file', 'initial contents')])
79+ master.add(['file'])
80+ master.commit('one', rev_id='m1')
81+
82+ checkout = master.branch.create_checkout('checkout')
83+ lightweight = checkout.branch.create_checkout('lightweight',
84+ lightweight=True)
85+
86+ # time to create a mess
87+ # add a commit to the master
88+ self.build_tree_contents([('master/file', 'master')])
89+ master.commit('two', rev_id='m2')
90+ self.build_tree_contents([('master/file', 'master local changes')])
91+
92+ # local commit on the checkout
93+ self.build_tree_contents([('checkout/file', 'checkout')])
94+ checkout.commit('tree', rev_id='c2', local=True)
95+ self.build_tree_contents([('checkout/file', 'checkout local changes')])
96+
97+ # lightweight
98+ self.build_tree_contents([('lightweight/file',
99+ 'lightweight local changes')])
100+
101+ # now update (and get conflicts)
102+ out, err = self.run_bzr('update lightweight', retcode=1)
103+ self.assertEqual('', out)
104+ self.assertFileEqual('<<<<<<< TREE\n'
105+ 'lightweight local changes'
106+ '=======\n'
107+ 'checkout'
108+ '>>>>>>> MERGE-SOURCE\n',
109+ 'lightweight/file')
110+
111+ # resolve it
112+ self.build_tree_contents([('lightweight/file',
113+ 'lightweight+checkout')])
114+ self.run_bzr('resolved lightweight/file')
115+
116+ # check we get the second conflict
117+ out, err = self.run_bzr('update lightweight', retcode=1)
118+ self.assertEqual('', out)
119+ self.assertFileEqual('<<<<<<< TREE\n'
120+ 'lightweight+checkout'
121+ '=======\n'
122+ 'master'
123+ '>>>>>>> MERGE-SOURCE\n',
124+ 'lightweight/file')
125+
126+ def test_update_remove_commit(self):
127+ """Update should remove revisions when the branch has removed some commits
128+ 1
129+ |\
130+ B-2 3
131+ | |
132+ 4 5-M
133+ |
134+ W
135+
136+ What you want is to revert 4, as in this graph the final result should
137+ be equivalent to:
138+
139+ 1
140+ |\
141+ 3 2
142+ | |\
143+ MB-5 | 4
144+ |/
145+ W
146+
147+ And the changes in 4 have been removed from the WT.
148+ """
149+
150+ # building the picture
151+ M = self.make_branch_and_tree('M')
152+ self.build_tree_contents([('M/file1', '1')])
153+ M.add(['file1'])
154+ M.commit('one', rev_id='c1')
155+
156+ B = M.branch.create_checkout('B')
157+ self.build_tree_contents([('B/file2', '2')])
158+ B.add(['file2'])
159+ B.commit('two', rev_id='c2', local=True)
160+ self.build_tree_contents([('B/file4', '4')])
161+ B.add(['file4'])
162+ B.commit('four', rev_id='c4', local=True)
163+
164+ W = B.branch.create_checkout('W', lightweight=True)
165+ # move B back to 2
166+ B.pull(B.branch, local=True, stop_revision='c2', overwrite=True)
167+
168+ self.build_tree_contents([('M/file3', '3')])
169+ M.add(['file3'])
170+ M.commit('three', rev_id='c3')
171+ self.build_tree_contents([('M/file5', '5')])
172+ M.add(['file5'])
173+ M.commit('five', rev_id='c5')
174+
175+ # now check if everything is as we expect it to be (test the test)
176+ # M has 1,3,5
177+ self.failUnlessExists('M/file1')
178+ self.failIfExists( 'M/file2')
179+ self.failUnlessExists('M/file3')
180+ self.failIfExists( 'M/file4')
181+ self.failUnlessExists('M/file5')
182+ # B has 1,2
183+ self.failUnlessExists('B/file1')
184+ self.failUnlessExists('B/file2')
185+ self.failIfExists( 'B/file3')
186+ self.failIfExists( 'B/file4')
187+ self.failIfExists( 'B/file5')
188+ # W has 1,2,4
189+ self.failUnlessExists('W/file1')
190+ self.failUnlessExists('W/file2')
191+ self.failIfExists( 'W/file3')
192+ self.failUnlessExists('W/file4')
193+ self.failIfExists( 'W/file5')
194+
195+ out, err = self.run_bzr('update W', retcode=0)
196+ self.assertEqual('', out)
197+ self.failUnlessExists('W/file1')
198+ self.failUnlessExists('W/file2')
199+ self.failUnlessExists('W/file3')
200+ self.failIfExists('W/file4')
201+ self.failUnlessExists('W/file5')
202
203=== modified file 'bzrlib/workingtree.py'
204--- bzrlib/workingtree.py 2010-02-03 07:16:18 +0000
205+++ bzrlib/workingtree.py 2010-02-06 21:29:15 +0000
206@@ -2274,26 +2274,53 @@
207 else:
208 if revision not in self.branch.revision_history():
209 raise errors.NoSuchRevision(self.branch, revision)
210+
211+ old_tip = old_tip or _mod_revision.NULL_REVISION
212+
213+ if not _mod_revision.is_null(old_tip) and old_tip != last_rev:
214+ # the branch we are bound to was updated
215+ # merge those changes in first
216+ base_tree = self.basis_tree()
217+ other_tree = self.branch.repository.revision_tree(old_tip)
218+ result = merge.merge_inner(
219+ self.branch,
220+ other_tree,
221+ base_tree,
222+ this_tree=self,
223+ change_reporter=change_reporter)
224+ if result > 0:
225+ self.add_parent_tree((old_tip, other_tree))
226+ trace.note('Rerun update after fixing the conflicts.')
227+ return result
228+
229 if last_rev != _mod_revision.ensure_null(revision):
230- # merge tree state up to specified revision.
231+ # the working tree is up to date with the branch
232+ # we can merge the specified revision from master
233+ to_tree = self.branch.repository.revision_tree(revision)
234+ to_root_id = to_tree.get_root_id()
235+
236 basis = self.basis_tree()
237 basis.lock_read()
238 try:
239- to_tree = self.branch.repository.revision_tree(revision)
240- to_root_id = to_tree.get_root_id()
241 if (basis.inventory.root is None
242 or basis.inventory.root.file_id != to_root_id):
243 self.set_root_id(to_root_id)
244 self.flush()
245- result += merge.merge_inner(
246- self.branch,
247- to_tree,
248- basis,
249- this_tree=self,
250- change_reporter=change_reporter)
251- self.set_last_revision(revision)
252 finally:
253 basis.unlock()
254+
255+ # determine the branch point
256+ graph = self.branch.repository.get_graph()
257+ base_rev_id = graph.find_unique_lca(self.branch.last_revision(), last_rev)
258+ base_tree = self.branch.repository.revision_tree(base_rev_id)
259+
260+ result = merge.merge_inner(
261+ self.branch,
262+ to_tree,
263+ base_tree,
264+ this_tree=self,
265+ change_reporter=change_reporter)
266+ self.set_last_revision(revision)
267 # TODO - dedup parents list with things merged by pull ?
268 # reuse the tree we've updated to to set the basis:
269 parent_trees = [(revision, to_tree)]
270@@ -2306,41 +2333,11 @@
271 for parent in merges:
272 parent_trees.append(
273 (parent, self.branch.repository.revision_tree(parent)))
274- if (old_tip is not None and not _mod_revision.is_null(old_tip)):
275+ if not _mod_revision.is_null(old_tip):
276 parent_trees.append(
277 (old_tip, self.branch.repository.revision_tree(old_tip)))
278 self.set_parent_trees(parent_trees)
279 last_rev = parent_trees[0][0]
280- else:
281- # the working tree had the same last-revision as the master
282- # branch did. We may still have pivot local work from the local
283- # branch into old_tip:
284- if (old_tip is not None and not _mod_revision.is_null(old_tip)):
285- self.add_parent_tree_id(old_tip)
286- if (old_tip is not None and not _mod_revision.is_null(old_tip)
287- and old_tip != last_rev):
288- # our last revision was not the prior branch last revision
289- # and we have converted that last revision to a pending merge.
290- # base is somewhere between the branch tip now
291- # and the now pending merge
292-
293- # Since we just modified the working tree and inventory, flush out
294- # the current state, before we modify it again.
295- # TODO: jam 20070214 WorkingTree3 doesn't require this, dirstate
296- # requires it only because TreeTransform directly munges the
297- # inventory and calls tree._write_inventory(). Ultimately we
298- # should be able to remove this extra flush.
299- self.flush()
300- graph = self.branch.repository.get_graph()
301- base_rev_id = graph.find_unique_lca(revision, old_tip)
302- base_tree = self.branch.repository.revision_tree(base_rev_id)
303- other_tree = self.branch.repository.revision_tree(old_tip)
304- result += merge.merge_inner(
305- self.branch,
306- other_tree,
307- base_tree,
308- this_tree=self,
309- change_reporter=change_reporter)
310 return result
311
312 def _write_hashcache_if_dirty(self):