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

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!

Thanks for tackling this!

Are there specific bugs that you think this will fix?

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

just that hey? :-)

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

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.

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

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)

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

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

Fixed

> 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

I FINALLY understand the issue (thanks John!)

I think that this is ready for the merge now.

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.

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

Signed it.

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.

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.