Date: Thu, 29 Nov 2012 07:43:57 +0000 From: "Robert N. M. Watson" <rwatson@FreeBSD.org> To: Alfred Perlstein <bright@mu.org> 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>, "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: <450D9522-1878-41C7-B1E1-3C5388B5A2A4@FreeBSD.org> In-Reply-To: <50B6E1DD.2030908@mu.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?450D9522-1878-41C7-B1E1-3C5388B5A2A4>