Re: [PATCH] xdiff: implement a zealous diff3

[Felipe Contreras <felipe.contreras@gmail.com>]

Elijah Newren wrote:
> 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)

What makes you think so?

>   * no attempt was made to address missing items pointed out in
> response to the original submission[1]

The original submission caused a discussion with no resolution, and
edned with Jeff saying he wanted to try real use-cases and that that he
wanted to use it in practice for a while.

The purpose v1 of this series was to respark the discussion and see if
any of the original parties had changed their minds.

People do change their minds after 8 years.

>   * no attempt was made to handle or even test particular cases
> pointed out in response to the original submission (see [1] and below)

Those were sent *after* the series, except [4], which clearly states the
*opposite* of there being a deal-breaker:

  But again, we don't do this splitting now. So I don't think it's
  something that should make or break a decision to have zdiff3. Without
  the splitting, I can see it being quite useful.

>   * the patches were posted despite knowing they caused segfaults, and
> without even stating as much![2]

Whomever *knew* that, it wasn't me.

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

My v2 includes the patch.

Just because a patch is in one series that doesn't preclude it from
being in another series. `git merge` and `git rebase` are smart enough
to handle such cases.

There is no risk of that happening (unless there's plans of merging v1
as-is).

> In my opinion, these submissions were egregiously cavalier.

If you make unwarranted assumptions everything is possible.

> I'll submit a patch (or perhaps a few) soon that has a functioning
> zdiff3.

I already have 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.

Yes, that is *one* opinion. The jury is still out on what is the best
approach. Junio and Jeff did not agree on that.


The whole point of "zdiff3" was to have something closer to "merge",
even if it wasn't 100% correct. Your approach maybe more correct, but
correctness was never the point.

Either way, I have more rewarding things to focus on, so good luck with
that.

> [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/

-- 
Felipe Contreras