Messages in this thread | | | From | Nick Piggin <> | Subject | Re: [git pull] core fixes | Date | Mon, 18 Aug 2008 16:17:14 +1000 |
| |
On Monday 18 August 2008 15:22, Nick Piggin wrote: > On Friday 15 August 2008 22:58, Ingo Molnar wrote: > > * Nick Piggin <nickpiggin@yahoo.com.au> wrote: > > > The argument that we lose the "thought process" of coming up with a > > > correct patch I don't really buy into either. We want to document the > > > thought process of well thought out, well reviewed and tested patches > > > -- if they get merged and subsequently found to be broken, it is > > > really nice to be able to look back at how and why they went wrong[*]. > > > > generally i agree and replace patches - but in this case i went for the > > delta because -rc3 was imminent and the previous patch was already > > well-tested with practical workloads, even though broken in the > > slowpath. > > But the patch author in this case hadn't proposed it as a fix or > added his SOB. > > > Also, in terms of judging risks, it was easier to look at the > > delta between the two commits and say "that obviously cannot make it > > worse than the current code". But it's all a special-case really. > > Which delta do you mean? My first patch did make the current code worse. > The delta between that and the corrected one fairly obviously didn't > make it worse because my first patch broke it so badly -- not something > I think needs to be reflected in upstream changeset. > > > > But merging bits and pieces of such raw patches IMO just adds too much > > > noise to the tree, and breaks bisection too easily. In the case of my > > > patch, the kernel will still build and mostly run, but that is > > > actually even a worse way to break the biesection if you are hunting > > > for some obscure and hard to reproduce bug. > > > > Note that the two commits were kept together tightly so the chance of > > bisection going in the middle of it _and_ hitting the obscure slowpath > > is reasonably small. > > Note I hit the obscure slowpath numerous times while testing other > patches. It wasn't particularly difficult. > > > What is wrong is to keep them apart (say merge a > > full upstream release between them) and break bisection on a wide basis. > > Slipperly slope, right? It is slippery in multiple ways. How "obscure" > and rare does the problem have to be? How large a window is tolerable > between broken patch and correction? How many people are allowed to > commit this type of broken patch? (because if everybody does then the > kernel is always broken regardless if how soon they are corrected). > > It is far better just to avoid the whole issue and do the right thing > and not merge commits like this, than to have a broken tree and justify > it because the commits are close together.
OK, Ingo pointed out that his first pull request did not contain both the initial broken fix and the corrected delta. I thought this is what happened but I was mistaken about that.
I still didn't actually have much confidence in the patch and didn't want it to be merged. But I maybe didn't make that clear.
So if this is a one off unfortunate event then yeah it is nothing to get too worked up about.
| |