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

[Elijah Newren <newren@gmail.com>]

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.

Yeah, I don't know for sure either but that does look buggy to me.
Thanks for digging up these examples.  I'm a bit overdrawn on time for
this, so I'll pick it back up in a week or two and investigate this
case further.