On Tue, Jun 15, 2021 at 2:16 AM Felipe Contreras <felipe.contreras@xxxxxxxxx> wrote: > > Elijah Newren wrote: > > On Mon, Jun 14, 2021 at 7:07 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > > > > > Felipe Contreras <felipe.contreras@xxxxxxxxx> 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? I did a simple grep to see where "diff3" was mentioned in the codebase to see if any of those needed a "zdiff3". Among the things I found was that although the original patch updated git-completion.bash, there were additional locations within a current git-completion.bash that referred to "diff3" that should also have a "zdiff3". I know you understand that part of the code. > > * 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 The discussion ended with no resolution in part because there were multiple items discussed that would need to be addressed. Including the one reiterated at the end of the discussion. >, 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. That wasn't the end of the discussion. The email you are referencing occurred here: https://lore.kernel.org/git/20130307185046.GA11622@xxxxxxxxxxxxxxxxxxxxx/. The end of the discussion was Junio quoting himself in order to reiterate that "As long as we clearly present the users what the option does and what its implications are, it is not bad to have such an option, I think." See https://lore.kernel.org/git/7vip42gfjc.fsf@xxxxxxxxxxxxxxxxxxxxxxxx/ and check the timestamps in the threadlist. > > * 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. This statement from Peff was incorrect; the zdiff3 patches made the code do splitting of conflict hunks. I would normally understand if perhaps you didn't know his statement was incorrect and wouldn't have had a way to know, *except* for the fact that this exact patch we are commenting on that you posted is modifying the code that does conflict hunk splitting. Further, you stated at https://lore.kernel.org/git/60c8758c80e13_e633208f7@natae.notmuch/ that you wanted to see conflict hunk splitting in a zdiff3 mode and expected it. So clearly conflict hunk splitting is relevant to you even if it wasn't to Peff. Peff and Junio spent several emails discussing conflict hunk splitting in the original thread (with Junio raising the question multiple times showing it was a concern of his), and Peff spent several emails discussing that topic even assuming that code was never triggered. In contrast to Peff, you know that conflict hunk splitting is relevant since you wanted it to occur, you saw the old thread where they discussed that topic and length, and yet you made no attempt to include a testcase (perhaps even using the one they discussed) to show how the splitting works? I find that negligent. > > * the patches were posted despite knowing they caused segfaults, and > > without even stating as much![2] > > Whomever *knew* that, it wasn't me. You knew that Peff had reported they caused segfaults. He pointed it out after making you aware of the zdiff3 patch; see https://lore.kernel.org/git/YMI+R5LFTj7ezlZE@xxxxxxxxxxxxxxxxxxxxxxx/. You also acknowledged having known of Peff's reports before reposting the patches at https://lore.kernel.org/git/60c82a622ae66_e5292087f@natae.notmuch/ You may be correct to point out that you only knew Peff had reported segfaults, rather than having verified for yourself that there were segfaults. But the fact that you took no action on the knowledge you did have, neither trying to verify, nor asking if the segfaults still occurred, nor even relaying those reports when reposting the patch, is exactly the problem at stake here. I find the lack of action with respect to the segfault report to be reckless. > > * 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. Ah, so your plan was to post a v2 with the fix as well as *also* post that fix elsewhere? Okay, that makes me feel better about this item, so I retract it. > > In my opinion, these submissions were egregiously cavalier. > > If you make unwarranted assumptions everything is possible. Which assumptions? That you were splitting the segfault fixes into a separate series and not also including them with the patch that introduces the segfault? That does seem unusual and would have been nice if you had communicated your plans somewhere so others wouldn't have to worry about that particular issue, but I agree that your explanation does invalidate the fifth item from my list as a concern. Unfortunately, that still leaves the other four. It's unfair to reviewers to post patches if you have not done due diligence. I've read other patches of yours and commented that I thought they looked good, so I'm not just trying to pick on you. You clearly have talent. With regards to the zdiff3 patches, I've stated above why I think you haven't done your due diligence. Sometimes people make mistakes; that's something that can be corrected. What I find egregious here is that even when Peff and I have pointed out how more due diligence is expected and needed, you've dug in to explain why you think your course of action was reasonable (both here and in https://lore.kernel.org/git/60c82a622ae66_e5292087f@natae.notmuch/). That in my mind raises your submissions from careless to glaringly cavalier. Further, it makes me suspect we may continue to see you repeat such behavior. That worries me.
