[MERGE] Fix bug #235407, when the same revision is merged twice

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
| On Mon, 2008-06-02 at 13:46 +1000, Martin Pool wrote:
|> On Mon, Jun 2, 2008 at 1:41 PM, Robert Collins
|> <robertc at robertcollins.net> wrote:
|>> Robert Collins has voted reject.
|>> Status is now: Vetoed
|>> Comment:
|>> merge is definitely doing the wrong thing and should not be any harder to
|>> fix: there were tests at one point that add_parent_ids would normalise any
|>> new parents so only the heads were listed.
|> I believe it does have that check but --force bypasses it.  (I haven't
|> checked the code.)  If that's true I can't see any really good reason
|> to remove it and would support removing it.  If you can get there
|> without --force I'd agree that's a bug.
|
| --force does not bypass the low level tree parent setting code which is
| where I believe the bug lies.
|
|> If we want to enforce it in another place you could make commit check
|> it, though I think at the moment it avoids scanning history and it
|> would be a shame to enforce it.
|>
|> At any rate: having got a tree into this state, it is not very nice to
|> just leave the user with an error.
|
| revert should get the tree out of the state, and then merging with a
| fixed bzr would do the right thing.
|
| -Rob

I still disagree that we should allow status to fail under these conditions.
Whether the actual test will be valid.

As for add_parent_id, what about set_parent_trees, etc. Are you sure that they
should all be doing a heads() call? It seems like we could have the caller be
responsible.

As for --force/--no-force. You can't *do* another merge without --force. So
there is no way to cause that effect from the command line.

I personally feel like the check should be happening much earlier in 'merge'
when it is looking for the common ancestor, so that it can give you 'Nothing to
do'. Not to mention give you a better merge base regardless.

My quick probe into it makes it somewhat non-trivial. The problem is that a lot
of the merge code is written in terms of branches, rather than in terms of
trees. In other places we have faked a Graph with a parent provider that gives
'CURRENT => (merge-parents)' which works well for finding the LCA with multiple
merge parents. But at the time 'find_lca' is being called, we only have branches.

So... I'll see if I can get merge fixed still, but I still feel this is a
reasonable fix.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkhDf5QACgkQJdeBCYSNAAMVmgCg1RfKNBkPxDTkSFZP19mc+aJf
u+AAnivs7Faktb4npFJ10mDA7qTbbGQD
=1T/q
-----END PGP SIGNATURE-----