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

On 16/06/2021 11:31, Jeff King wrote:

On Wed, Jun 16, 2021 at 09:57:49AM +0100, Phillip Wood wrote:

quoted

quoted

quoted

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")) {

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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