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
