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

[Gerard Krol]

[Gerard Krol (gerard-)]

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!

[Martin Pool (mbp)]

Thanks for tackling this!

Are there specific bugs that you think this will fix?

[Gerard Krol (gerard-)]

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

[Martin Pool (mbp)]

just that hey? :-)

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

[lp:~gerard-/bzr/update]

4993. By Gerard Krol on 2010-02-05

Remove an unused local variable that was still lingering

[Vincent Ladeuil (vila)]

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.

[John A Meinel (jameinel)]

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

review: Needs Information

[John A Meinel (jameinel)]

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)

[Gerard Krol (gerard-)]

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

[lp:~gerard-/bzr/update]

4994. By Gerard Krol on 2010-02-06

Use assertFileEqual in the tests, instead of opening them and looking at the contents. TODO: check if this does work on Windows (due to possible \r\n line endings)?

4995. By Gerard Krol on 2010-02-06

Reformat long lines

[Gerard Krol (gerard-)]

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

Fixed

[Gerard Krol (gerard-)]

> 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

[lp:~gerard-/bzr/update]

4996. By Gerard Krol on 2010-02-06

No need to use "result +=" anymore

4997. By Gerard Krol on 2010-02-06

Test for a case where update should remove commits

4998. By Gerard Krol on 2010-02-06

Pass the new test by doing the first merge with self.basis_tree() as base

4999. By Gerard Krol on 2010-02-06

Don't compare old_tip to revision, just do the merge.

5000. By Gerard Krol on 2010-02-06

Now correctly determine the branch point (instead of some lucky guesses)

[Gerard Krol (gerard-)]

I FINALLY understand the issue (thanks John!)

I think that this is ready for the merge now.

[lp:~gerard-/bzr/update]

5001. By Gerard Krol on 2010-02-06

NEWS item

[Gerard Krol (gerard-)]

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.

[Gerard Krol (gerard-)]

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

Signed it.

[John A Meinel (jameinel)]

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

[Vincent Ladeuil (vila)]

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

=== modified file 'NEWS'
--- NEWS	2010-02-05 10:57:26 +0000
+++ NEWS	2010-02-06 21:29:15 +0000
@@ -67,6 +67,10 @@
   the same value to avoid confusing ``make`` and other date-based build
   systems. (Robert Collins, #515631)
 
+* ``bzr update`` performs the two merges in a more logical order and will stop
+  when it encounters conflicts.  
+  (Gerard Krol, #113809)
+
 Improvements
 ************
 

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2010-01-25 17:48:22 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2010-02-06 21:29:15 +0000
@@ -359,6 +359,13 @@
         # work as a merge
         # retcode 1 as we expect a text conflict
         self.run_bzr('update u1', retcode=1)
+        self.assertFileEqual('<<<<<<< TREE\n'
+                             'first offline change in u1'
+                             '=======\n'
+                             'altered in u2'
+                             '>>>>>>> MERGE-SOURCE\n',
+                             'u1/hosts')
+
         self.run_bzr('resolved u1/hosts')
         # add a text change here to represent resolving the merge conflicts in
         # favour of a new version of the file not identical to either the u1

=== modified file 'bzrlib/tests/blackbox/test_update.py'
--- bzrlib/tests/blackbox/test_update.py	2009-12-23 06:31:19 +0000
+++ bzrlib/tests/blackbox/test_update.py	2010-02-06 21:29:15 +0000
@@ -171,9 +171,9 @@
         # get all three files and a pending merge.
         out, err = self.run_bzr('update checkout')
         self.assertEqual('', out)
-        self.assertEqualDiff("""+N  file
+        self.assertEqualDiff("""+N  file_b
 All changes applied successfully.
-+N  file_b
++N  file
 All changes applied successfully.
 Updated to revision 1 of branch %s
 Your local commits will now show as pending merges with 'bzr status', and can be committed with 'bzr commit'.
@@ -242,9 +242,6 @@
         self.run_bzr('update checkout')
 
     def test_update_dash_r(self):
-        # Test that 'bzr update' works correctly when you have
-        # an update in the master tree, and a lightweight checkout
-        # which has merged another branch
         master = self.make_branch_and_tree('master')
         os.chdir('master')
         self.build_tree(['./file1'])
@@ -266,9 +263,6 @@
         self.assertEquals(['m1'], master.get_parent_ids())
 
     def test_update_dash_r_outside_history(self):
-        # Test that 'bzr update' works correctly when you have
-        # an update in the master tree, and a lightweight checkout
-        # which has merged another branch
         master = self.make_branch_and_tree('master')
         self.build_tree(['master/file1'])
         master.add(['file1'])
@@ -315,3 +309,132 @@
 2>All changes applied successfully.
 2>Updated to revision 2 of branch .../master
 ''')
+
+    def test_update_checkout_prevent_double_merge(self):
+        """"Launchpad bug 113809 in bzr "update performs two merges"
+        https://launchpad.net/bugs/113809"""
+        master = self.make_branch_and_tree('master')
+        self.build_tree_contents([('master/file', 'initial contents')])
+        master.add(['file'])
+        master.commit('one', rev_id='m1')
+
+        checkout = master.branch.create_checkout('checkout')
+        lightweight = checkout.branch.create_checkout('lightweight',
+                                                      lightweight=True)
+        
+        # time to create a mess
+        # add a commit to the master
+        self.build_tree_contents([('master/file', 'master')])
+        master.commit('two', rev_id='m2')
+        self.build_tree_contents([('master/file', 'master local changes')])
+
+        # local commit on the checkout
+        self.build_tree_contents([('checkout/file', 'checkout')])
+        checkout.commit('tree', rev_id='c2', local=True)
+        self.build_tree_contents([('checkout/file', 'checkout local changes')])
+
+        # lightweight 
+        self.build_tree_contents([('lightweight/file',
+                                   'lightweight local changes')])
+
+        # now update (and get conflicts)
+        out, err = self.run_bzr('update lightweight', retcode=1)
+        self.assertEqual('', out)
+        self.assertFileEqual('<<<<<<< TREE\n'
+                             'lightweight local changes'
+                             '=======\n'
+                             'checkout'
+                             '>>>>>>> MERGE-SOURCE\n',
+                             'lightweight/file')
+        
+        # resolve it
+        self.build_tree_contents([('lightweight/file',
+                                   'lightweight+checkout')])
+        self.run_bzr('resolved lightweight/file')
+
+        # check we get the second conflict
+        out, err = self.run_bzr('update lightweight', retcode=1)
+        self.assertEqual('', out)
+        self.assertFileEqual('<<<<<<< TREE\n'
+                             'lightweight+checkout'
+                             '=======\n'
+                             'master'
+                             '>>>>>>> MERGE-SOURCE\n',
+                             'lightweight/file')
+
+    def test_update_remove_commit(self):
+        """Update should remove revisions when the branch has removed some commits
+            1
+            |\
+          B-2 3
+            | |
+            4 5-M
+            |
+            W
+
+        What you want is to revert 4, as in this graph the final result should
+        be equivalent to:
+
+           1
+           |\
+           3 2
+           | |\
+        MB-5 | 4
+           |/
+           W
+
+        And the changes in 4 have been removed from the WT.
+        """
+        
+        # building the picture
+        M = self.make_branch_and_tree('M')
+        self.build_tree_contents([('M/file1', '1')])
+        M.add(['file1'])
+        M.commit('one', rev_id='c1')
+
+        B = M.branch.create_checkout('B')
+        self.build_tree_contents([('B/file2', '2')])
+        B.add(['file2'])
+        B.commit('two', rev_id='c2', local=True)
+        self.build_tree_contents([('B/file4', '4')])
+        B.add(['file4'])
+        B.commit('four', rev_id='c4', local=True)
+        
+        W = B.branch.create_checkout('W', lightweight=True)
+        # move B back to 2
+        B.pull(B.branch, local=True, stop_revision='c2', overwrite=True)
+
+        self.build_tree_contents([('M/file3', '3')])
+        M.add(['file3'])
+        M.commit('three', rev_id='c3')
+        self.build_tree_contents([('M/file5', '5')])
+        M.add(['file5'])
+        M.commit('five', rev_id='c5')
+        
+        # now check if everything is as we expect it to be (test the test)
+        # M has 1,3,5
+        self.failUnlessExists('M/file1')
+        self.failIfExists(    'M/file2')
+        self.failUnlessExists('M/file3')
+        self.failIfExists(    'M/file4')
+        self.failUnlessExists('M/file5')
+        # B has 1,2
+        self.failUnlessExists('B/file1')
+        self.failUnlessExists('B/file2')
+        self.failIfExists(    'B/file3')
+        self.failIfExists(    'B/file4')
+        self.failIfExists(    'B/file5')
+        # W has 1,2,4
+        self.failUnlessExists('W/file1')
+        self.failUnlessExists('W/file2')
+        self.failIfExists(    'W/file3')
+        self.failUnlessExists('W/file4')
+        self.failIfExists(    'W/file5')
+
+        out, err = self.run_bzr('update W', retcode=0)
+        self.assertEqual('', out)
+        self.failUnlessExists('W/file1')
+        self.failUnlessExists('W/file2')
+        self.failUnlessExists('W/file3')
+        self.failIfExists('W/file4')
+        self.failUnlessExists('W/file5')

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2010-02-03 07:16:18 +0000
+++ bzrlib/workingtree.py	2010-02-06 21:29:15 +0000
@@ -2274,26 +2274,53 @@
         else:
             if revision not in self.branch.revision_history():
                 raise errors.NoSuchRevision(self.branch, revision)
+                
+        old_tip = old_tip or _mod_revision.NULL_REVISION
+        
+        if not _mod_revision.is_null(old_tip) and old_tip != last_rev:
+            # the branch we are bound to was updated
+            # merge those changes in first
+            base_tree  = self.basis_tree()
+            other_tree = self.branch.repository.revision_tree(old_tip)
+            result = merge.merge_inner(
+                                  self.branch,
+                                  other_tree,
+                                  base_tree,
+                                  this_tree=self,
+                                  change_reporter=change_reporter)
+            if result > 0:
+                self.add_parent_tree((old_tip, other_tree))
+                trace.note('Rerun update after fixing the conflicts.')
+                return result
+
         if last_rev != _mod_revision.ensure_null(revision):
-            # merge tree state up to specified revision.
+            # the working tree is up to date with the branch
+            # we can merge the specified revision from master
+            to_tree = self.branch.repository.revision_tree(revision)
+            to_root_id = to_tree.get_root_id()
+
             basis = self.basis_tree()
             basis.lock_read()
             try:
-                to_tree = self.branch.repository.revision_tree(revision)
-                to_root_id = to_tree.get_root_id()
                 if (basis.inventory.root is None
                     or basis.inventory.root.file_id != to_root_id):
                     self.set_root_id(to_root_id)
                     self.flush()
-                result += merge.merge_inner(
-                                      self.branch,
-                                      to_tree,
-                                      basis,
-                                      this_tree=self,
-                                      change_reporter=change_reporter)
-                self.set_last_revision(revision)
             finally:
                 basis.unlock()
+
+            # determine the branch point
+            graph = self.branch.repository.get_graph()
+            base_rev_id = graph.find_unique_lca(self.branch.last_revision(), last_rev)
+            base_tree = self.branch.repository.revision_tree(base_rev_id)
+
+            result = merge.merge_inner(
+                                  self.branch,
+                                  to_tree,
+                                  base_tree,
+                                  this_tree=self,
+                                  change_reporter=change_reporter)
+            self.set_last_revision(revision)
             # TODO - dedup parents list with things merged by pull ?
             # reuse the tree we've updated to to set the basis:
             parent_trees = [(revision, to_tree)]
@@ -2306,41 +2333,11 @@
             for parent in merges:
                 parent_trees.append(
                     (parent, self.branch.repository.revision_tree(parent)))
-            if (old_tip is not None and not _mod_revision.is_null(old_tip)):
+            if not _mod_revision.is_null(old_tip):
                 parent_trees.append(
                     (old_tip, self.branch.repository.revision_tree(old_tip)))
             self.set_parent_trees(parent_trees)
             last_rev = parent_trees[0][0]
-        else:
-            # the working tree had the same last-revision as the master
-            # branch did. We may still have pivot local work from the local
-            # branch into old_tip:
-            if (old_tip is not None and not _mod_revision.is_null(old_tip)):
-                self.add_parent_tree_id(old_tip)
-        if (old_tip is not None and not _mod_revision.is_null(old_tip)
-            and old_tip != last_rev):
-            # our last revision was not the prior branch last revision
-            # and we have converted that last revision to a pending merge.
-            # base is somewhere between the branch tip now
-            # and the now pending merge
-
-            # Since we just modified the working tree and inventory, flush out
-            # the current state, before we modify it again.
-            # TODO: jam 20070214 WorkingTree3 doesn't require this, dirstate
-            #       requires it only because TreeTransform directly munges the
-            #       inventory and calls tree._write_inventory(). Ultimately we
-            #       should be able to remove this extra flush.
-            self.flush()
-            graph = self.branch.repository.get_graph()
-            base_rev_id = graph.find_unique_lca(revision, old_tip)
-            base_tree = self.branch.repository.revision_tree(base_rev_id)
-            other_tree = self.branch.repository.revision_tree(old_tip)
-            result += merge.merge_inner(
-                                  self.branch,
-                                  other_tree,
-                                  base_tree,
-                                  this_tree=self,
-                                  change_reporter=change_reporter)
         return result
 
     def _write_hashcache_if_dirty(self):