merge3 automerges wrongly unlike weave

[GuilhemBichot]

Hello. I know each merge algorithm has its strengths and weaknesses, but for this case I couldn't find an explanation so I'm passing it to you. If you could have a look and say if this is a merge3 fixable bug or just something we have to live with. Thanks.
Grab this branch:
https://code.launchpad.net/~mysql/mysql-server/mysql-next
let's say you have it in a directory named "mysql-next"
and create two identical directories by branching "mysql-next" at revid <email address hidden> , call the first one "merge3" and the other "weave".
do
cd merge3
bzr merge ../mysql-next -r 'revid:<email address hidden>
(20 conflicts)
cd ../weave
bzr merge ../mysql-next -r 'revid:<email address hidden> --weave
(0 conflicts)
and then do a diff (like with kdiff3) of "merge3" and "weave" and look at file mysql-test/lib/mtr_cases.pm. It's "weave" which has it correct (so does --lca) and "merge3" which is wrong by having such line:
  my %found_suites;
(and the few lines further which reference found_suites are also wrong).
Let me explain.
That "%found_suites" line existed, then we found that it caused a bug, and it was removed by <email address hidden> ; that bugfix revision is present in the MERGE_SOURCE branch (alik) and not in TREE (kristofer), the buggy code is absent from alik and present in kristofer. The logic is that the merge imports the bugfix revision and so removes the buggy code from TREE. That's what weave does, but not merge3 (and as it's a silent automerge by merge3, the colleague didn't notice it and the bug came back to life again http://bugs.mysql.com/bug.php?id=42807).
I figured it could be a case of old LCA which didn't have the buggy code either, so we would have a 3-way merge with "ancestor and MERGE_SOURCE don't have the code, TREE has it, so it's new in TREE and TREE wins". But maybe I'm oversimplifying. I couldn't reproduce this with a simple testcase with small few-revision branches.
Anyway that worries me, because so far I thought that merge3 was *only* introducing excess conflicts in criss-cross, but it also seems to automerge wrongly. Some of my colleagues seem to use merge3 in criss-cross merges unless there are too many conflicts (then fall back to weave), but this now looks wrong, it looks like they should always use weave in criss-cross merge even if few conflicts?
I wonder, would there be a way to force a logic in "bzr merge":
"if the merge is criss-cross then refuse to merge with anything else than weave, or automatically fall back to weave"?

[John A Meinel (jameinel)]

I'm pretty sure this is the same per-file graph issue, and that merge3 is selecting too old of a base for the merge.

[GuilhemBichot (guilhem-bichot)]

Minor correction: merge3 does not give 20 conflicts but 2 (such a low acceptable number likely explains why colleagues didn't switch to --weave for this merge).
I'm very probably going to force colleagues to use --weave as soon as the merge is criss-cross, no matter the number of conflicts (until this merge3 bug is fixed if it's a fixable bug), because silent wrong automerges are too harmful.
To that aim, I believe I'll modify a MySQL-grown plugin which all colleagues have: the patch I quickly made is to add this to our plugin's init code:

OLD_MERGE_CLASS = get_cmd_object("merge", True).__class__

cmd_merge_original_do_merge= OLD_MERGE_CLASS._do_merge

def cmd_merge_do_merge_and_require_weave_for_criss_cross(self, merger, change_reporter, allow_pending, verified):
    import bzrlib.merge
    assert (not merger._is_criss_cross or \
            (merger.merge_type is bzrlib.merge.WeaveMerger)), \
            "for criss-cross merges you must use the --weave option: 'bzr merge --weave'"
    return cmd_merge_original_do_merge(self,merger,change_reporter,allow_pending,verified)

OLD_MERGE_CLASS._do_merge= cmd_merge_do_merge_and_require_weave_for_criss_cross

It seems to work according to my quick testing, however could you please have a quick look and tell me if you spot a potential problem in this method? I would be grateful.

[Vincent Ladeuil (vila)]

You use assert which nasty users can avoid by running python -O.
No other eye-catcher.

[Martin Pool (mbp)]

2009/6/25 GuilhemBichot <email address hidden>:
> Minor correction: merge3 does not give 20 conflicts but 2 (such a low acceptable number likely explains why colleagues didn't switch to --weave for this merge).
> I'm very probably going to force colleagues to use --weave as soon as the merge is criss-cross, no matter the number of conflicts (until this merge3 bug is fixed if it's a fixable bug), because silent wrong automerges are too harmful.
> To that aim, I believe I'll modify a MySQL-grown plugin which all colleagues have: the patch I quickly made is to add this to our plugin's init code:
>
> OLD_MERGE_CLASS = get_cmd_object("merge", True).__class__
>
> cmd_merge_original_do_merge= OLD_MERGE_CLASS._do_merge
>
> def cmd_merge_do_merge_and_require_weave_for_criss_cross(self, merger, change_reporter, allow_pending, verified):
>    import bzrlib.merge
>    assert (not merger._is_criss_cross or \
>            (merger.merge_type is bzrlib.merge.WeaveMerger)), \
>            "for criss-cross merges you must use the --weave option: 'bzr merge --weave'"
>    return cmd_merge_original_do_merge(self,merger,change_reporter,allow_pending,verified)
>
> OLD_MERGE_CLASS._do_merge=
> cmd_merge_do_merge_and_require_weave_for_criss_cross
>
> It seems to work according to my quick testing, however could you please
> have a quick look and tell me if you spot a potential problem in this
> method? I would be grateful.

Hm, I might be misunderstanding this, but it looks like you're
checking the _is_criss_cross attribute of the Merger object before you
start the merge, at which point it's None. Whether the merge will
have a criss-cross situation is not a static property of the Merger
class but rather depends on what data we actually observe.

Therefore I think your code here will never fire. Also, as vila
mentioned, you should use if/raise BzrCommandError otherwise firstly
people can turn off the assertion and secondly they will get an ugly
traceback rather than just a message.

I think what you have to do here is: try the merge, and if it detects
a criss-cross situation, restart the merge using another merger.

You could probably do that by calling find_base() on the merger object
here, then observing the is_criss_cross property.

It might be nice if there was a hook by which plugin code could be
told (through a reporter or result pattern) about the progress of the
merge, then interrupt it or otherwise override what would otherwise
happen - we could file a separate bug for that.

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

[GuilhemBichot (guilhem-bichot)]

Hi Martin.
Thanks very much for the review! Actually it works now. It checks _is_criss_cross in cmd_merge_do_merge_and_require_weave_for_criss_cross(), and that function replaces cmd_merge._do_merge() (the replacement is done when plugin is loaded, before merge starts).
So when cmd_merge.run() executes (in bzrlib.builtins), it sets _is_criss_cross, then calls self._do_merge(), which is actually my function, which then checks self._is_criss_cross. I tested and it works (in criss-cross merge, --merge3 and --lca are refused, --weave is accepted). You are right that it would have been even better to automatically restart with --weave instead of just bailing out if --merge3 or --lca is used, but that was too difficult for me :-)
I replaced the assertion with a BzrError, it is indeed better-looking.