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

> 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.