Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 29 Nov 2012 07:53:15 +0000
From:      "Robert N. M. Watson" <rwatson@FreeBSD.org>
To:        Simon J. Gerraty <sjg@juniper.net>
Cc:        "src-committers@freebsd.org" <src-committers@FreeBSD.org>, Andre Oppermann <andre@FreeBSD.org>, Peter Wemm <peter@wemm.org>, Garrett Cooper <yanegomi@gmail.com>, "svn-src-all@freebsd.org" <svn-src-all@FreeBSD.org>, Alfred Perlstein <bright@mu.org>, "svn-src-head@freebsd.org" <svn-src-head@FreeBSD.org>
Subject:   Re: Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern)
Message-ID:  <49B76E37-D3AB-4FCE-85F4-34A7864117D8@FreeBSD.org>
In-Reply-To: <20121129070234.9C68458094@chaos.jnpr.net>
References:  <201211272004.qARK4qS8047209@svn.freebsd.org> <CAGE5yCpxOdsjefe6quR_gjs82pk9a2e_H_WUNUWhUGA3WZPJaw@mail.gmail.com> <50B54180.5020608@freebsd.org> <alpine.BSF.2.00.1211272246560.37292@fledge.watson.org> <50B54492.5040100@freebsd.org> <956CE44A-BA0F-4FE4-AA38-F4B90C85ECBA@FreeBSD.org> <50B54CE0.6080008@freebsd.org> <2A12C740-1D72-4D30-B663-47A37AAC2FF3@FreeBSD.org> <50B5C4F1.1020002@freebsd.org> <50B64C43.50001@mu.org> <E11B3FAA-440E-492A-A8DA-FFD376E9B176@gmail.com> <50B6E1DD.2030908@mu.org> <20121129070234.9C68458094@chaos.jnpr.net>

next in thread | previous in thread | raw e-mail | index | archive | help

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=20
>> you wind up with the following type of reviewers:
>=20
> 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.
>=20
> But AFAIK that wasn't what Robert was talking about.
>=20
> 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.
>=20
> 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=



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?49B76E37-D3AB-4FCE-85F4-34A7864117D8>