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

On 15/06/2021 20:35, Elijah Newren wrote:

quoted

On Tue, Jun 15, 2021 at 2:43 AM Jeff King [off-list ref] wrote:

quoted

On Tue, Jun 15, 2021 at 05:16:08AM +0000, Elijah Newren via GitGitGadget wrote:

quoted

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

quoted

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.

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

diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index b1dc9df7ea..5f76957169 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c

@@ -455,6 +455,7 @@ static int lines_contain_alnum(xdfenv_t *xe, int i,

int chg)
  static void xdl_merge_two_conflicts(xdmerge_t *m)
  {
         xdmerge_t *next_m = m->next;
+       m->chg0 = next_m->i0 + next_m->chg0 - m->i0;
         m->chg1 = next_m->i1 + next_m->chg1 - m->i1;
         m->chg2 = next_m->i2 + next_m->chg2 - m->i2;
         m->next = next_m->next;

and running
     git checkout 0c52457b7c^ &&
     bin-wrappers/git -c merge.conflictstyle=zdiff3 merge 0c52457b7c^2
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.