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

[Phillip Wood <phillip.wood123@gmail.com>]

On 16/06/2021 11:31, Jeff King wrote:
> On Wed, Jun 16, 2021 at 09:57:49AM +0100, Phillip Wood wrote:
> 
>>>> 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
>> [...]
>> 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.
> 
> Thanks for figuring that out. I agree that the output after the patch
> you showed is correct, in the sense that the common lines show up in the
> base now. It does feel like it's working against the point of zdiff3,
> though, which is to reduce the number of common lines shown in the
> "ours" and "theirs" hunks.

I agree - the output is longer rather than shorter. As we only want to 
trim the common prefix and suffix from the conflicts I wonder if it 
would be better to special case zdiff3 rather than piggy backing on the 
existing XDL_MERGE_ZEALOUS implementation. We can trim the common lines 
by looping over the begging and end of the hunk comparing the lines 
with xdl_recmatch() without going to the trouble of diffing them as 
XDL_MERGE_ZEALOUS does. I don't think we need to worry about coalescing 
adjacent conflicts for zdiff3. It makes sense to coalesce in the 
XDL_MERGE_ZEALOUS case as it can potentially split a  N line conflict 
hunk into N/2 single line conflict hunks but zdiff3 does not split 
conflict hunks.

> Likewise, I think this coalescing makes things worse even for "merge",
> where you get:
> 
>    <<<<<<< ours
>                    if (starts_with(arg, "--informative-errors")) {
>                            informative_errors = 1;
>                            continue;
>                    }
>                    if (starts_with(arg, "--no-informative-errors")) {
>    =======
>                    if (!strcmp(arg, "--informative-errors")) {
>                            informative_errors = 1;
>                            continue;
>                    }
>                    if (!strcmp(arg, "--no-informative-errors")) {
>    >>>>>>> theirs
> 
> and have to figure out manually that those interior lines are common.
> But I imagine there are cases where you have a large number of
> uninteresting lines (blank lines, "}", etc that create a lot of noise > in the output by breaking up the actual changed lines into tiny hunks
> that are hard to digest on their own.

Yes, I think the heuristic for coalescing conflict hunks could be 
improved. It would be fairly simple to only join two hunks if the 
conflicts are longer that the context between them and the existing 
XDL_MERGE_ZEALOUS_ALNUM logic allows conflicts with more context between 
them to be coalesced if the context lines are uninteresting. I think 
XDL_MERGE_ZEALOUS_ALNUM is only used by `git merge-file` at the moment, 
with everything else going through ll_merge() which uses XDL_MERGE_ZEALOUS

Best Wishes

Phillip

> 
> -Peff
>