| Message ID | 20210613225836.1009569-5-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | merge: cleanups and fix | expand |
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? (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/ > 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
Elijah Newren wrote: > 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. > 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. > 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. > 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
On Tue, Jun 15, 2021 at 12:51 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Elijah Newren wrote: > > On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > ... > > 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". The difference here is that Jeff reported segfaults after Uwe sent the patch. Uwe could not have read those reports before he posted the patch. You did read those emails before re-posting the patch. > > 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. You knew he had reported it as a possibility. That behooved you to try to investigate, either by asking him for details to try to reproduce, or attempt it on a large range of merges, and then to report the results when you reposted the patch. Or, at the absolute minimum, to at least report Peff's report along with your reposting of the patch. > > 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. Not when folks spent several emails in response to the original about the finer points of how splitting should or should not work. Nor when folks had reported segfaults with the original patch. With that background, I'd say it's completely unreasonable to assume that a reboot of the series comes without any tests unless marked as RFC. > > 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. I did not assume the worst with your patches. I assumed good faith until I was provided with strong counter-evidence as perhaps best summarized at https://lore.kernel.org/git/YMbexfeUG78yBix4@coredump.intra.peff.net/. That made it clear something was heavily problematic. If you hadn't reposted patches for which there were reported segfaults without saying anything about those reports, I would have asked you none of those pointed questions. In fact, I probably wouldn't have asked them if your original response to Peff were along the lines of "sorry, won't do that again" rather than "I don't know how I was supposed to investigate the few segfaults you mentioned. All you said is that you never tracked the bug." It was precisely that cavalier attitude that made me question this patch in this manner. > 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. I was not deriding your work. I was asking deep and pointed questions due to the cavalier manner in which you were posting, and which you seem to be doubling down on. I would say that after https://lore.kernel.org/git/YMhx2BFlwUxZ2aFJ@coredump.intra.peff.net/, I'd have to agree with Peff that you have displayed a level of carelessness with which I am not comfortable for the project. That kind of carelessness leads to skepticism in others, moreso when you double down on it. It makes me think that if I review any of your future patches, I need to check them far more rigorously because you are willing to eschew what I view as even the most basic of responsibilities in ensuring you are submitting valid patches, and even worse, you are unwilling to change how you approach the project even when those are pointed out to you.
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;
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. Let's initialize it to be safe. 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(-)