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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Jun 15, 2021, 07:51 UTC
Message-ID: <60c85bfd112a9_e633208d5@natae.notmuch>
In-Reply-To: <CABPp-BGZ2H1MVgw9RvSdogLMdqsX3n89NkkDYDa2VM3TRHn7tg@mail.gmail.com>

Elijah Newren wrote:

Show 14 quoted lines

> On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > 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?

Because I found the fix *after* I sent the patch, and after Jeff posted a way to reproduce the issue he experienced in the past.

v2 of the series has the fix included, but I was waiting for feedback on v1.

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

More than 50% of my patches get either completely ignored, or commented, and then ignored. The vast majority of them don't have a valid reason for rejection.

I thought this series had a better chance of being merged before the zdiff3 series, and the chg0 is orthogonal to the zdiff3 series anyway (in my view).

Plus, I don't see the harm in sending the same patch in two different series. Especially if the chances of both series being merged any time soon is very low.

> > Let's initialize it to be safe.
> 
> Is it being safe, or is it hacking around a hypothetical future
> segfault?

It's being safe.

> Are these values reasonable,

I spent about two hours of my own free time--with no corporation backing up my git work--to familiarize myself with the code and the values are as reasonable as I could make them.

If m->chg0 is 0, xdl_orig_copy will ignore the chunk, so m->i0 is irrelevant, but just to be consistent I copied the original i0.

Once again: the only value that matters is m->chg0, and it's completely safe to set it to 0.

Much more reasonable than garbage, like 1774234 which we currently have sometimes.

> or did you just initialize them to known garbage rather than letting
> them be unknown garbage?

No.

> Are there other values that need to be changed for the xdiff data
> structures to be consistent?

No.

> Will future code readers be helped by this initialization,

Yes.

> or will it introduce inconsistencies (albeit initialized ones) in
> existing data structures that make it harder for future readers to
> reason about?

No.

> I'm somewhat worried by the cavalier posting of the zdiff3 patch[1],

When Uwe sent the patch [1] nobody said it was a "cavalier posting". Jeff King said "I think the patch is correct".

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

Nobody pays me to work on git. I comment when I have time. Yesterday was my birthday and I spent my free time doing other things.

When I sent that mail it was "Sun, 13 Jun 2021 16:24:50 -0500", when I sent the patch it was "Sun, 13 Jun 2021 17:58:36 -0500". After spending more than an hour and a half reading the code, and trying different approaches I became more confident that there was no better approach. At least not one that was easily found.

I am 99% certain that m->chg0 = 0 is safe, and produces a reasonable output.

What I didn't know at 16:24 is if there was something better we could do. But by 17:58 I became much more certain that there wasn't, although I'm still not sure.

Degrees of confidence vary with time.

> Peff already pointed out that you reposted the zdiff3 patch that had
> knonw segfaults without even stating as much[3].

He knew that. I didn't.

Show 5 quoted lines

> 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],

That thread included many things, including links to other threads.

> and discussed some concerns about the implementation that would need
> to be addressed[5].

That link is a mail from Jeff stating that he disagrees with Junio in at least one point, and said:

  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.

Show 6 quoted lines

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

I didn't want to override Uwe's wording, especially since both Junio and Jeff often complain about my commit messages, and it isn't rare that they end up rewritten.

> 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,

It is not uncommon for v1 of a patch series to be missing something. Uwe's patch series [1] did not include tests either, and nobody focused on that. It is completely reasonable to assume that a reboot of the series wouldn't initially focus on that either.

That being said, I did test zdiff3 with the whole testing framework, and I did explain how.

Show 7 quoted lines

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

I typically don't send cover letters for single patches (just like Uwe didn't [1]), but I did add an explanation of what tests I ran below the commit message of the patch, as is customary.

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

I had not planned to do any additional work, as I don't usually plan how I spend my free time. I happened to have free time when I saw Jeff mail about a way to reproduce and I felt motivated to investigate further. One thing lead to another and fortunately I found the fix quickly enough.

> 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?

By assuming good faith [2], and simply asking questions. I don't think assuming the worst and calling into question the competence of a contributor is a good approach.

Morevoer, this is not *my* patch, this is a patch from the community, to the community, and it's in the best interest of the community that it gets developed *collaboratively*.

I took Uwe's patch and added my two cents, not for me, but for the community. I don't need zdiff3, I'm perfectly fine with diff3, but I would like a better default conflict style than merge, for the community. That's why I spend a considerable amount of time reading the old threads, and familiarizing myself with the xdiff/xmerge code of which I basically knew nothing about.

This is something I wish more people did, and then perhaps we wouldn't need to wait 8 years for a simple crash fix for a feature many people probably would like, which again, I'm 99% certain is correct.

To be crystal clear: this is not *my itch*, I did the patch altrusticially for the community. The fix correct--at least to the best knowledge of everyone involved so far. If you want to deride the hours I spent working for the community for free which resulted in a patch that most likely is a net positive, feel free, I think this doesn't help the community, which needs more of this kind of work, not less.

Cheers.

[1] https://lore.kernel.org/git/1362602202-29749-1-git-send-email-u.kleine-koenig@pengutronix.de/ [2] https://en.wikipedia.org/wiki/Wikipedia:Assume_good_faith

-- 
Felipe Contreras