Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern)

[Robert N. M. Watson]

On 29 Nov 2012, at 04:17, Alfred Perlstein wrote:

> I've seen what happens with large groups, it doesn't scale and basically you wind up with the following type of reviewers:

I think we have to assume best intent here -- the goal of code review in complex critical components is not to eliminate creativity, but to reduce the occurrence of errors and improve the extent to which larger architectural goals are applied and understood in substantive changes. I would much rather we agree by consensus that this sort of code requires mutual code review than chase the "stick" aspect -- as I've said, I feel strongly that code review applied in the last few years has improved code quality markedly in this area, and prevented a lot of problems from being shipped in production kernels. Obviously, you've had some bad experiences with code review, and I have sympathies -- however, if we grant commit bits on the basis of not just technical competence and independence, but also their ability to work well with others, then we need to trust them to perform code reviews in a mature way. And by "code review", I'm incorporating the idea of "design review" as well -- some collaboration throughout development of non-trivial changes (e.g., in a branch) really does improve not just quality of local areas of code, but the overall design.

Robert


> 
> 1) whitespace nazis
> 2) rubber stampers
> 3) forever absentee reviewers
> 4) name nazis
> 
> The whitespace nazis will never give review approval until you've gone through 600 iterations of the code.  Imagine Bruce... but if he wasn't pragmatic and awesome and didn't have other amazing skills.
> 
> The rubber stampers are friends or people that feel bad about what's happening to you by the whitespace nazis and name nazis so they just glance and approve based on the fact that you're a good guy who has committed to the same place before and usually doesn't break stuff.
> 
> The forever absentee reviewers are the ones that sign up to review, but never are around to review, or just don't have the time. Eventually an entire subsystem becomes "owned" by absentee reviewers and then it's a huge, delicate and annoying process to get that part of the code under some other list or remove the absentee people.
> 
> Finally the name nazis, these will debate you on if your code should be of the name kern_foo.c, subr_foo.c or maybe in the new-name-ng should actually be called kernel_subr_foo.c, but whatever you name something, they will make you change it, because you are wrong.
> 
> Anyhow, that is my opinion on heavy handed reviews in a large project.
> 
> I've been fortunate enough to work on one project where luckily it was dominated by rubber stampers, that was silly but awesome because at least we could get stuff done...
> 
> Unfortunately I then worked on quite a few dominated by a mix of all the four I mentioned.  The result sucked.
> 
> -Alfred
> 
>