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>