From owner-svn-src-head@FreeBSD.ORG Thu Nov 29 07:53:19 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 5E109827; Thu, 29 Nov 2012 07:53:19 +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 1379B8FC08; Thu, 29 Nov 2012 07:53:19 +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 7E40F46B09; Thu, 29 Nov 2012 02:53:17 -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=us-ascii From: "Robert N. M. Watson" In-Reply-To: <20121129070234.9C68458094@chaos.jnpr.net> Date: Thu, 29 Nov 2012 07:53:15 +0000 Content-Transfer-Encoding: quoted-printable Message-Id: <49B76E37-D3AB-4FCE-85F4-34A7864117D8@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> <20121129070234.9C68458094@chaos.jnpr.net> To: Simon J. Gerraty X-Mailer: Apple Mail (2.1283) Cc: "src-committers@freebsd.org" , Andre Oppermann , Peter Wemm , Garrett Cooper , "svn-src-all@freebsd.org" , Alfred Perlstein , "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:53:19 -0000 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=