On Sunday, 17 June 2007 20:52, Andrew Morton wrote:
> On Sun, 17 Jun 2007 20:53:41 +0200 Bartlomiej Zolnierkiewicz <[email protected]> wrote:
>
> >
> >
> > IMO we should concentrate more on preventing regressions than on fixing them.
> > In the long-term preventing bugs is cheaper than fixing them afterwards.
> >
> > First let me tell you all a little story...
> >
> > Over two years ago I've reviewed some _cleanup_ patch and noticed three bugs
> > in it (in other words I potentially prevented three regressions). I also
> > asked for more thorough verification of the patch as I suspected that it may
> > have more problems. The author fixed the issues and replied that he hasn't
> > done the full verification yet but he doesn't suspect any problems...
> >
> > Fast forward...
> >
> > Year later I discover that the final version of the patch hit the mainline.
> > I don't remember ever seeing the final version in my mailbox (there are no
> > cc: lines in the patch description) and I saw that I'm not credited in the
> > patch description. However the worse part is that it seems that the full
> > verification has never been done. The result? Regression in the release
> > kernel (exactly the issue that I was worried about) which required three
> > patches and over a month to be fixed completely. It seems that a year
> > was not enough to get this ~70k _cleanup_ patch fully verified and tested
> > (it hit -mm soon before being merged)...
>
> crap. Commit ID, please ;)
>
> > >From reviewer's POV: I have invested my time into review, discovered real
> > issues and as a reward I got no credit et all and extra frustration from the
> > fact that part of my review was forgotten/ignored (the part which resulted in
> > real regression in the release kernel)... Oh and in the past the said
> > developer has already been asked (politely in private message) to pay more
> > attention to his changes (after I silently fixed some other regression caused
> > by his other patch).
> >
> > But wait there is more, I happend to be the maintainer of the subsystem which
> > got directly hit by the issue and I was getting bugreports from the users about
> > the problem... :-)
> >
> > It wasn't my first/last bad experience as a reviewer... finally I just gave up
> > on reviewing other people patches unless they are stricly for IDE subsystem.
> >
> > The moral of the story is that currently it just doesn't pay off to do
> > code reviews.
>
> I dunno. I suspect (hope) that this was an exceptional case, hence one
> should not draw general conclusions from it. It certainly sounds very bad.
>
> > From personal POV it pays much more to wait until buggy patch
> > hits the mainline and then fix the issues yourself (at least you will get
> > some credit). To change this we should put more ephasize on the importance
> > of code reviews by "rewarding" people investing their time into reviews
> > and "rewarding" developers/maintainers taking reviews seriously.
> >
> > We should credit reviewers more, sometimes it takes more time/knowledge to
> > review the patch than to make it so getting near to zero credit for review
> > doesn't sound too attractive. Hmm, wait it can be worse - your review
> > may be ignored... ;-)
> >
> > >From my side I think I'll start adding less formal "Reviewed-by" to IDE
> > patches even if the review resulted in no issues being found (in additon to
> > explicit "Acked-by" tags and crediting people for finding real issues - which
> > I currently always do as a way for showing my appreciation for their work).
>
> yup, Reviewed-by: is good and I do think we should start adopting it,
> although I haven't thought through exactly how.
>
> On my darker days I consider treating a Reviewed-by: as a prerequisite for
> merging. I suspect that would really get the feathers flying.
How about the following "algorithm":
* Step 1: Send a patch as an RFC to the relevant lists/people and only if there
are no negative comments within at least n days, you are allowed to proceed
to the next step. If anyone has reviewed/acked the patch, add their names
and email addresses as "Reviewed-by"/"Acked-by" to the patch in the next
step.
* Step 2: Send the patch as an RC to the relevant lists/people _and_ LKML and
if there are no negative comments within at least n days, you can proceed to
the next step. If anyone has reviewed/acked the patch, add their names
and email addresses as "Reviewed-by"/"Acked-by" to the patch in the next
step.
* Step 3: Submit the patch for merging to the right maintainer (keeping the
previous CC list).
where n is a number that needs to be determined (I think that n could be 3).
Well, "negative comments" should also be defined more precisely. ;-)
> > I also encourage other maintainers/developers to pay more attention to
> > adding "Acked-by"/"Reviewed-by" tags and crediting reviewers. I hope
> > that maintainers will promote changes that have been reviewed by others
> > by giving them priority over other ones (if the changes are on more-or-less
> > the same importance level of course, you get the idea).
> >
> > Now what to do with people who ignore reviews and/or have rather high
> > regressions/patches ratio?
>
> Ignoring a review would be a wildly wrong thing to do. It's so unusual
> that I'd be suspecting a lost email or an i-sent-the-wrong-patch.
>
> As for high regressions/patches ratio: that'll be hard to calculate and
> tends to be dependent upon the code which is being altered rather than who
> is doing the altering: some stuff is just fragile, for various reasons.
>
> One ratio which we might want to have a think about is the patches-sent
> versus reviews-done ratio ;)
>
> > I think that we should have info about regressions integrated into SCM,
> > i.e. in git we should have optional "fixes-commit" tag and we should be
> > able to do some reverse data colletion. This feature combined with
> > "Author:" info after some time should give us some very interesting
> > statistics (Top Ten "Regressors"). It wouldn't be ideal (ie. we need some
> > patches threshold to filter out people with 1 patch and >= 1 regression(s),
> > we need to remember that some code areas are more difficult than the others
> > and that patches are not equal per se etc.) however I believe than making it
> > into Top Ten "Regressors" should give the winners some motivation to improve
> > their work ethic. Well, in the worst case we would just get some extra
> > trivial/documentation patches. ;-)
>
> We of course do want to minimise the amount of overhead for each developer.
> I'm a strong believer in specialisation: rather than requiring that *every*
> developer/maintainer integrate new steps in their processes it would be
> better to allow them to proceed in a close-to-usual fashion and to provide
> for a specialist person (or team) to do the sorts of things which you're
> thinking about.
Still, even very experienced developers make trivial mistakes, so there should
be a way to catch such things before they hit -rc or even -mm kernels
Greetings,
Rafael
--
"Premature optimization is the root of all evil." - Donald Knuth
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
