Elijah Newren wrote: > On Sun, Jun 13, 2021 at 4:04 PM Felipe Contreras > <felipe.contreras@xxxxxxxxx> 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@xxxxxxxxxxxxxx/ [2] https://en.wikipedia.org/wiki/Wikipedia:Assume_good_faith -- Felipe Contreras
