Re: [PATCH 0/2] RFC: implement new zdiff3 conflict style

[Elijah Newren <newren@gmail.com>]

On Wed, Jun 16, 2021 at 1:57 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 15/06/2021 20:35, Elijah Newren wrote:
> > On Tue, Jun 15, 2021 at 2:43 AM Jeff King <peff@peff.net> wrote:
> >>
> >> On Tue, Jun 15, 2021 at 05:16:08AM +0000, Elijah Newren via GitGitGadget wrote:
> >>
> >>> Implement a zealous diff3, or "zdiff3". This new mode is identical to
> >>> ordinary diff3 except that it allows compaction of common lines between the
> >>> two sides of history, if those common lines occur at the beginning or end of
> >>> a conflict hunk.
> >>>
> >>> This is just RFC, because I need to add tests. Also, while I've remerged
> >>> every merge, revert, or duly marked cherry-pick from both git.git and
> >>> linux.git with this patch using the new zdiff3 mode, that only shows it
> >>> doesn't segfault. (Though I also reran 10% of the linux remerges with zdiff3
> >>> under valgrind without issues.) I looked through some differences between
> >>> --remerge-diff with diff3 and --remerge-diff with zdiff3, but those are
> >>> essentially diffs of a diff of a diff, which I found hard to read. I'd like
> >>> to look through more examples, and use it for a while before submitting the
> >>> patches without the RFC tag.
> >>
> >> I did something similar (but I wasn't smart enough to try your
> >> remerge-diff, and just re-ran a bunch of merges).
> >
> > What I did was great for testing if there were funny merges that might
> > cause segfaults or such with zdiff3, but not so clever for viewing the
> > direct output from zdiff3.  Using remerge-diff in this way does not
> > show the [z]diff3 output directly, but a diff of that output against
> > what was ultimately recorded in the merge, forcing me to attempt to
> > recreate the original in my head.
> >
> > (And, of course, I made it even worse by taking the remerge-diff
> > output with diff3, and the remerge-diff output with zdiff3, and then
> > diffing those, resulting in yet another layer of diffs that I'd have
> > to undo in my head to attempt to construct the original.)
> >
> >> Skimming over the results, I didn't see anything that looked incorrect.
> >> Many of them are pretty non-exciting, though. A common case seems to be
> >> ones like 01a2a03c56 (Merge branch 'jc/diff-filter-negation',
> >> 2013-09-09), where two sides both add functions in the same place, and
> >> the common lines are just the closing "}" followed by a blank line.
> >>
> >> Removing those shared lines actually makes things less readable, IMHO,
> >> but I don't think it's the wrong thing to do. The usual "merge" zealous
> >> minimization likewise produces the same unreadability. If we want to
> >> address that, I think the best way would be by teaching the minimization
> >> some heuristics about which lines are trivial.
> >>
> >> Here's another interesting one. In 0c52457b7c (Merge branch
> >> 'nd/daemon-informative-errors-typofix', 2014-01-10), the diff3 looks
> >> like:
> >>
> >>    <<<<<<< ours
> >>                    if (starts_with(arg, "--informative-errors")) {
> >>    ||||||| base
> >>                    if (!prefixcmp(arg, "--informative-errors")) {
> >>    =======
> >>                    if (!strcmp(arg, "--informative-errors")) {
> >>    >>>>>>> theirs
> >>                            informative_errors = 1;
> >>                            continue;
> >>                    }
> >>    <<<<<<< ours
> >>                    if (starts_with(arg, "--no-informative-errors")) {
> >>    ||||||| base
> >>                    if (!prefixcmp(arg, "--no-informative-errors")) {
> >>    =======
> >>                    if (!strcmp(arg, "--no-informative-errors")) {
> >>    >>>>>>> theirs
> >>
> >> A little clunky, but it's easy-ish to see what's going on. With zdiff3,
> >> the context between the two hunks is rolled into a single hunk:
> >>
> >>    <<<<<<< ours
> >>                    if (starts_with(arg, "--informative-errors")) {
> >>                            informative_errors = 1;
> >>                            continue;
> >>                    }
> >>                    if (starts_with(arg, "--no-informative-errors")) {
> >>    ||||||| base
> >>                    if (!prefixcmp(arg, "--informative-errors")) {
> >>    =======
> >>                    if (!strcmp(arg, "--informative-errors")) {
> >>                            informative_errors = 1;
> >>                            continue;
> >>                    }
> >>                    if (!strcmp(arg, "--no-informative-errors")) {
> >>    >>>>>>> theirs
> >>
> >> which seems worse. I haven't dug/thought carefully enough into your
> >> change yet to know if this is expected, or if there's a bug.
>
> XDL_MERGE_ZEALOUS coalesces adjacent conflicts that are separated by
> fewer than four lines. Unfortunately the existing code in
> xdl_merge_two_conflicts() only coalesces 'ours' and 'theirs', not
> 'base'. Applying
>
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index b1dc9df7ea..5f76957169 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -455,6 +455,7 @@ static int lines_contain_alnum(xdfenv_t *xe, int i,
> int chg)
>   static void xdl_merge_two_conflicts(xdmerge_t *m)
>   {
>          xdmerge_t *next_m = m->next;
> +       m->chg0 = next_m->i0 + next_m->chg0 - m->i0;
>          m->chg1 = next_m->i1 + next_m->chg1 - m->i1;
>          m->chg2 = next_m->i2 + next_m->chg2 - m->i2;
>          m->next = next_m->next;
>
> and running
>      git checkout 0c52457b7c^ &&
>      bin-wrappers/git -c merge.conflictstyle=zdiff3 merge 0c52457b7c^2
> gives
>
> <<<<<<< HEAD
>                 if (starts_with(arg, "--informative-errors")) {
>                         informative_errors = 1;
>                         continue;
>                 }
>                 if (starts_with(arg, "--no-informative-errors")) {
> ||||||| 2f93541d88
>                 if (!prefixcmp(arg, "--informative-errors")) {
>                         informative_errors = 1;
>                         continue;
>                 }
>                 if (!prefixcmp(arg, "--no-informative-errors")) {
> =======
>                 if (!strcmp(arg, "--informative-errors")) {
>                         informative_errors = 1;
>                         continue;
>                 }
>                 if (!strcmp(arg, "--no-informative-errors")) {
>  >>>>>>> 0c52457b7c^2
>
> Which I think is correct. Whether combining single line conflicts in
> this way is useful is a different question (and is independent of your
> patch). I can see that with larger conflicts it is worth it but here we
> end up with conflicts where 60% of the lines are from the base version.
> One the other hand there are fewer conflicts to resolve - I'm not sure
> which I prefer.

Oh, sweet, thanks for tracking this down!  I'll try to find some time
to play with it some more.