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

[Robert N. M. Watson]

On 29 Nov 2012, at 07:02, Simon J. Gerraty wrote:

> On Wed, 28 Nov 2012 20:17:33 -0800, Alfred Perlstein writes:
>> I've seen what happens with large groups, it doesn't scale and basically 
>> you wind up with the following type of reviewers:
> 
> The issues you cite, are the result of taking a good idea too far.
> Mandating reviews for all changes - does not make sense, and all the
> ills you describe are natural consequences.
> 
> But AFAIK that wasn't what Robert was talking about.
> 
> Some bits of code do warrant extra care when touching.
> Unless you are the sole author and maintainer (and sometimes even then)
> getting a 2nd opinion (from someone familiar with the code) makes sense.
> 
> That does not have to map to a requirement for formal reviewers, review
> tracking etc - none of which would scale in a volunteer organization
> anyway.

Yes, exactly so. Perhaps I took too much for granted in my original e-mail, and too much focus was placed on the original commit that triggered the larger thread, so I should be more explicit:

- I have in mind developers increasing their practice of seeking code review when touching critical and extremely sensitive subsystems in the network stack -- e.g., socket buffers, inter-layer locking, TCP structure, etc, rather than an edge-case device drivers.
- When I say "mandatory", what I mean is a mutual consensus that everyone involved in that area thinks that review is an important part of working on it, and seeks review as part of their development process.
- That this obviously applies to non-trivial changes rather than every tweak to a comment.
- That it be done sensibly and with good intentions by people who understand the code.
- That it occur in a culture of mutual review: if we have four active developers in the socket code, then they all agree to participate in both sides of the process. E.g., I suggest code review to Andre and agree to do some, but in return, he reviews some of my changes. An imbalance in code production and review leads to dissatisfaction for everyone, and is not sustainable.

Looking back through svn log on netinet over the last thousand changes, excluding the last month and a half, SCTP, and mechanical/cosmetic changes, we can see that roughly half of changes are "reviewed by" or "discussed with", and there are patterns of mutual review, especially for major changes. I suspect more were reviewed but that it wasn't specifically tagged in commit messages. That doesn't mean errors don't creep in, changes don't have to be backed out, etc, but it has significant benefits.

Robert