Re: [PATCH] xdiff: implement a zealous diff3

[Elijah Newren <newren@gmail.com>]

On Mon, Jun 14, 2021 at 7:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > I found the problem, m->chg0 was not initialized in xdl_refine_conflicts.
> >
> > I'm not familiar with the area so I don't know if the following makes
> > sense, but it fixes the crash:
>
> Unlike the remainder of the xdiff/ directory, xdiff/xmerge.c was
> Dscho's brainchild if I am not mistaken, so I'm CCing him for
> input.

This is going to sound harsh, but people shouldn't waste (any more)
time reviewing the patches in this thread or the "merge: cleanups and
fix" series submitted elsewhere.  They should all just be rejected.

I do not think it is reasonable to expect reviewers to spend time
responding to re-posted patches when:
  * no attempt was made to make sure they were up-to-date with current
code beyond compiling (see below)
  * no attempt was made to address missing items pointed out in
response to the original submission[1]
  * no attempt was made to handle or even test particular cases
pointed out in response to the original submission (see [1] and below)
  * the patches were posted despite knowing they caused segfaults, and
without even stating as much![2]
  * the segfault "fixes" are submitted as a separate series from the
patch introducing the segfault[3], raising the risk that one gets
picked up without the other.

In my opinion, these submissions were egregiously cavalier.  I'll
submit a patch (or perhaps a few) soon that has a functioning zdiff3.

However, since I've already put in the time to understand it, let me
explain what is wrong with this patch.  This particular change is in
the area of the code that splits conflict regions when there are
portions of the sides (not the base) that match.  Doing such splitting
makes sense with "merge" conflictStyle since the base is never shown;
this splitting can allow pulling the common lines out of the conflict
region.  However, with diff3 or zdiff3, the original text does not
match the sides and by splitting the conflict region, we are forced to
decide how or where to split the original text among the various
conflict (and non-conflict?) regions.  This is pretty haphazard, and
the effect of this patch is to assign all of the original text to the
first conflict region in the split, and make all other regions have
empty base text.

This exact scenario was discussed by you and Peff back when zdiff3 was
originally introduced in the thread where Felipe got the patch that he
started this thread with.  In that thread, Peff explained how zdiff3
should only try to move common lines at the beginning or end of the
conflict hunk outside the conflict region, without doing any splitting
of the conflict region (this particular issue took about 1/3 to 1/2 of
the original thread, but I think [4] has a good hilight).
Additionally, a quick grep through the code showed that there are
additional places in bash/zsh completion that need to be fixed to use
the new option besides the locations modified in the original zdiff3
patch.  See [1] and [2] for various other things overlooked.


[1] https://lore.kernel.org/git/CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com/
[2] https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/
[3] https://lore.kernel.org/git/20210613225836.1009569-5-felipe.contreras@gmail.com/
[4] https://lore.kernel.org/git/20130307180157.GA6604@sigill.intra.peff.net/