From owner-svn-src-head@FreeBSD.ORG Thu Nov 29 07:44:00 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id E3E69438; Thu, 29 Nov 2012 07:44:00 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 9A5358FC12; Thu, 29 Nov 2012 07:44:00 +0000 (UTC) Received: from [192.168.2.119] (host86-146-118-26.range86-146.btcentralplus.com [86.146.118.26]) by cyrus.watson.org (Postfix) with ESMTPSA id 2A0AB46B17; Thu, 29 Nov 2012 02:43:58 -0500 (EST) Subject: Re: Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern) Mime-Version: 1.0 (Apple Message framework v1283) Content-Type: text/plain; charset=iso-8859-1 From: "Robert N. M. Watson" In-Reply-To: <50B6E1DD.2030908@mu.org> Date: Thu, 29 Nov 2012 07:43:57 +0000 Content-Transfer-Encoding: quoted-printable Message-Id: <450D9522-1878-41C7-B1E1-3C5388B5A2A4@FreeBSD.org> References: <201211272004.qARK4qS8047209@svn.freebsd.org> <50B54180.5020608@freebsd.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> <50B6E1DD.2030908@mu.org> To: Alfred Perlstein X-Mailer: Apple Mail (2.1283) Cc: "src-committers@freebsd.org" , Andre Oppermann , Peter Wemm , Garrett Cooper , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Nov 2012 07:44:01 -0000 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 >=20 > 1) whitespace nazis > 2) rubber stampers > 3) forever absentee reviewers > 4) name nazis >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > Anyhow, that is my opinion on heavy handed reviews in a large project. >=20 > 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... >=20 > Unfortunately I then worked on quite a few dominated by a mix of all = the four I mentioned. The result sucked. >=20 > -Alfred >=20 >=20