On Thu, Jul 21, 2022 at 11:14:57AM +0200, Omar Polo wrote:
> I did something similar to what you suggest, but instead I'm swapping
> tmpfile (which is the "new" file) and the ancestor: this way the merge3
> is done with the patched file against the old blob (left) and the
> current revision (right).  (yeah, it gets confusing easily)

Our diff3 is very weird about which file is the ancestor.
This has confused me before.

Diff3 as described in the "diff3 analysis" paper does a 'diff O A'
and a diff 'O B', where O is the original file, and files A and B
were both derived from O.

The code we inherited from the OpenRCS merge code does a diff '1 3'
and diff '2 3'. Which seems wrong, since the O being used is 1 in
the first diff, and 2 in the second diff. It would seem that perhaps
3 is base and it somehow works in reverse, but I recall trying to
pass the base for '3' and ending up breaking things.

This could be the reason for mis-merges we have seen while using the
Myers diff algorithm. Using Patience diff seems to be good enough to
work around such issues, but it is not a proper fix.

I recall trying to fix it properly and not getting anywhere.
Whenever I dive into diff3.c I come back out wanting to burn it all
down and rewrite it. This code is very hard to follow, with all the
ed-script nonsense and all the non-informative naming of things.

> To do this, i'm postponing reverse_patch in the diff3 case: we don't
> need to reverse it unless we don't find the blob and fall back to the
> search/replace.
> 
> The diff is a bit longer because i had to move reverse_patch early for
> apply_patch to see it, and i've added a (simple) regress.  (would a more
> complex example help there?  I'm already ensuring we're using the
> merge.)

Yes, please test your fix with some non-trivial patches. The test you
are adding is so simple that it is perhaps working inspite of arguments
not being in the right order. Ideally, you should try to trigger a
conflict and then check whether the content in left/base/right matches
what you would expect.

It might be useful to have a test case which triggers a merge conflict
and verifies that the result matches the result of 'got backout' (for
patch -R) and 'got cherrypick' (for regular patch).

Regardless, your patch is a good first step; ok stsp@