Re: [PATCH 4/4] xdiff/xmerge: fix chg0 initialization

From: Elijah Newren <newren@gmail.com>
Date: Jun 14, 2021, 15:34 UTC
Message-ID: <CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com>
In-Reply-To: <20210613225836.1009569-5-felipe.contreras@gmail.com>

On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 8 quoted lines

>
> chg0 is needed when style is XDL_MERGE_DIFF3, xdl_refine_conflicts()
> currently is only called when level >= XDL_MERGE_ZEALOUS, which cannot
> happen since the level is capped to XDL_MERGE_EAGER.
>
> However, if at some point in the future we decide to not cap diff3, or
> introduce a similar uncapped mode, an uninitialized chg0 would cause a
> crash.

If this cannot happen now, and is needed by your other posted patch, why aren't these two patches either combined or posted as part of the same series? (I think a separate submission _only_ makes sense if you explicitly withdraw the other patch from consideration, otherwise it feels dangerous to submit patches this way).

> Let's initialize it to be safe.

Is it being safe, or is it hacking around a hypothetical future segfault? Are these values reasonable, or did you just initialize them to known garbage rather than letting them be unknown garbage? Are there other values that need to be changed for the xdiff data structures to be consistent? Will future code readers be helped by this initialization, or will it introduce inconsistencies (albeit initialized ones) in existing data structures that make it harder for future readers to reason about?

I'm somewhat worried by the cavalier posting of the zdiff3 patch[1], and am worried you're continuing more of the same here, especially since in your previous post[2] you said that you didn't know whether this particular change made sense and haven't posted anything yet to state why it does. Peff already pointed out that you reposted the zdiff3 patch that had knonw segfaults without even stating as much[3]. That's one thing that needs to be corrected, but I think there are more that should be pointed out. Peff linked to the old thread which is how you found out about the patches, but the old thread also included things that Junio wanted to see if zdiff3 were to be added as an option[4], and discussed some concerns about the implementation that would need to be addressed[5]. Given the fact that Peff liked the original zdiff3 patch and tried it for over a month and took time to report on it and argue for such a feature, I would have expected that when you reposted the zdiff3 patch that you would have provided some more detailed explanation of how it works and why it's right (and included some fixes with it rather than separate), and that you would have included multiple new testcases to the testsuite both to make sure we don't cause any obvious bugs and to provide some samples of how the option functions, and also likely include in the cover letter some kind of explanation of additional testing done to try to assure us that the new mode is safe to use (e.g. "I ran this on hundreds of linux kernel commits, with X% of them showing a difference. They typically looked like <Y>" or "I've run with this for a month without issue, and occasionally have re-checked a merge afterwards and found that the conflicts were much easier to view because of..."). Short of all that, I would have at least expected the patches to be posted as RFC and stating any known shortcomings and additional work you planned on doing after gathering some feedback.

You didn't do any of that, making me wonder whether this patch is a solid fix, or whether it represents just hacking around the first edge case you ran into and posting a shot in the dark. It could well be either; how are we to know whether we should trust these patches?

(Having read the old zdiff3 thread after Peff pointed to it, I really like the idea of such an option. I'd like to see it exist and I would use it. But I think it needs to be introduced appropriately, otherwise it makes it even less likely we'll end up with such a thing.)

[1] https://lore.kernel.org/git/20210613143155.836591-1-felipe.contreras@gmail.com/ [2] https://lore.kernel.org/git/60c677a2c2d24_f5651208cf@natae.notmuch/ [3] https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/ [4] https://lore.kernel.org/git/7vip42gfjc.fsf@alter.siamese.dyndns.org/ [5] https://lore.kernel.org/git/20130307180157.GA6604@sigill.intra.peff.net/

Show 30 quoted lines

> Cc: Jeff King <peff@peff.net>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  xdiff/xmerge.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index b5b3f56f92..d9b3a0dccd 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -333,7 +333,7 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,
>                 mmfile_t t1, t2;
>                 xdfenv_t xe;
>                 xdchange_t *xscr, *x;
> -               int i1 = m->i1, i2 = m->i2;
> +               int i0 = m->i0, i1 = m->i1, i2 = m->i2;
>
>                 /* let's handle just the conflicts */
>                 if (m->mode)
> @@ -384,6 +384,8 @@ static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,
>                         m->next = m2;
>                         m = m2;
>                         m->mode = 0;
> +                       m->i0 = i0;
> +                       m->chg0 = 0;
>                         m->i1 = xscr->i1 + i1;
>                         m->chg1 = xscr->chg1;
>                         m->i2 = xscr->i2 + i2;
> --
> 2.32.0