Date: Wed, 28 Nov 2012 20:17:33 -0800 From: Alfred Perlstein <bright@mu.org> To: Garrett Cooper <yanegomi@gmail.com> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, Andre Oppermann <andre@freebsd.org>, Peter Wemm <peter@wemm.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "Robert N. M. Watson" <rwatson@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: <50B6E1DD.2030908@mu.org> In-Reply-To: <E11B3FAA-440E-492A-A8DA-FFD376E9B176@gmail.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/28/12 7:49 PM, Garrett Cooper wrote: > On Nov 28, 2012, at 9:39 AM, Alfred Perlstein <bright@mu.org> wrote: > >> On 11/28/12 12:01 AM, Andre Oppermann wrote: >>> On 28.11.2012 00:59, Robert N. M. Watson wrote: >>>> On 27 Nov 2012, at 23:29, Andre Oppermann wrote: >>>> >>>>>>>>>> Andre.. this breaks incoming connections. TCP is immediately reset and never even gets to the >>>>>>>>>> listener process. You need to back out of fix this urgently please. >>>>>>>>> I just found out and fixed it. Sorry for the breakage. >>>>>>>> I'd like to see a much more thorough use of "Reviewed by:" in socket and TCP-related commits -- this >>>>>>>> is very sensitive code, and a second pair of eyes is always valuable. Post-commit review is not a >>>>>>>> substitute. Looking back over similar changes in the socket code over the last two years, I see >>>>>>>> that almost all have reviewers, so I think it would be reasonable to consider it mandatory for these >>>>>>>> subsystems at this point. The good news is that we have lots of people with expertise in it. >>>>>>> Good to see you becoming more active again. :-) And yes, >>>>>>> you have a point there. >>>>>> Yes -- this is only about three weeks old, however; for the prior six-twelve months, I've been fairly non-existent in FreeBSD-land due to outside obligations :-). >>>>> Just saw that I did indeed send you a review request three weeks ago. ;-) >>>>> At the end of a rather long email though. >>>> Yes, indeed -- no patch was attached, and it followed quite a long e-mail on your plans to rewrite the TCP stack. I'm afraid that went onto the "read this later as time permits" pile as I was at a conference, rather than the fast-path "oh, quickly review this patch" pile. However, simply committing the patch rather than trying a bit harder to find a reviewer isn't the right answer either. To maximise the likelihood of a review, construct an e-mail with a subject line like "Review request: (patch description)", attach the patch, and include a proposed commit message. >>> Yes, and I didn't really expect you to answer (at least quickly) during >>> your FreeBSD hiatus. So it was seeking review by chance. >>> >>> Alas I found and fixed the bug myself within 2.5hrs. While not optimal, >>> a sign of poor prior testing and too much trust into the submitter of >>> the patch it wasn't an earth shattering event. Doesn't distract from >>> the fact that it was mea culpa in any case though. >>> >>> For prior review of kern_socket* and netinet/tcp_* related changes it has >>> been on and off by various committers over the past year. If we do have >>> a policy of prior review required then it should be made official and >>> codified in MAINTAINERS and universally applied to all. >> Personally I don't think we need any more anchors attached to people's feet when developing FreeBSD. >> >> Mistakes will happen, they will happen in head. Slowing down the process to eliminate mistakes only works to slow down change and give a false sense of "fixing stability" when in fact the only thing "stable" is the slowness of submitting code. > Organizing cabals of people to review code is the best way to go. It's an extension of the maintainers concept applied in a better fashion because it encourages several developers to provide input on a series of commits instead of one. > > I know it's sort of done in some groups [based on commit messages], but it would be nice to have it better formalized and socialized as well. Things like this are helpful for other potential freebsd contributors/developers (like some folks I work with for instance!). > > An extension of this code review idea would probably be reviewboard. Email based review is doable and a lot of OSS groups use it, but there are some nice points to using a more advanced tool, in particular: > 1. Colorized diffs. > 2. Various diff niceties (hide white space changes, etc). > 3. It does a reasonable job at establishing code context (code block A has moved to B). > > There are 2 FreeBSD corporate consumers that use it, with at least one other group considering it, and there are probably more groups that using it as well. Plus it integrates well with p4, and does pretty well with git and svn. > > I've seen what happens with large groups, it doesn't scale and basically you wind up with the following type of reviewers: 1) whitespace nazis 2) rubber stampers 3) forever absentee reviewers 4) name nazis 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. 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. 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. 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. Anyhow, that is my opinion on heavy handed reviews in a large project. 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... Unfortunately I then worked on quite a few dominated by a mix of all the four I mentioned. The result sucked. -Alfred
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?50B6E1DD.2030908>