Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 28 Nov 2012 19:49:40 -0800
From:      Garrett Cooper <yanegomi@gmail.com>
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>, "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:   Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern)
Message-ID:  <E11B3FAA-440E-492A-A8DA-FFD376E9B176@gmail.com>
In-Reply-To: <50B64C43.50001@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>

next in thread | previous in thread | raw e-mail | index | archive | help
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:
>>>=20
>>> On 27 Nov 2012, at 23:29, Andre Oppermann wrote:
>>>=20
>>>>>>>>> Andre.. this breaks incoming connections.  TCP is immediately rese=
t and never even gets to the
>>>>>>>>> listener process.  You need to back out of fix this urgently pleas=
e.
>>>>>>>>=20
>>>>>>>> I just found out and fixed it.  Sorry for the breakage.
>>>>>>>=20
>>>>>>> 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 ov=
er 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 peo=
ple with expertise in it.
>>>>>>=20
>>>>>> Good to see you becoming more active again. :-)  And yes,
>>>>>> you have a point there.
>>>>>=20
>>>>> 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 o=
bligations :-).
>>>>=20
>>>> Just saw that I did indeed send you a review request three weeks ago. ;=
-)
>>>> At the end of a rather long email though.
>>>=20
>>> Yes, indeed -- no patch was attached, and it followed quite a long e-mai=
l on your plans to rewrite the TCP stack. I'm afraid that went onto the "rea=
d 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 t=
he patch rather than trying a bit harder to find a reviewer isn't the right a=
nswer either. To maximise the likelihood of a review, construct an e-mail wi=
th a subject line like "Review request: (patch description)", attach the pat=
ch, and include a proposed commit message.
>>=20
>> Yes, and I didn't really expect you to answer (at least quickly) during
>> your FreeBSD hiatus.  So it was seeking review by chance.
>>=20
>> 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.
>>=20
>> 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 fee=
t when developing FreeBSD.
>=20
> Mistakes will happen, they will happen in head.  Slowing down the process t=
o eliminate mistakes only works to slow down change and give a false sense o=
f "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 ex=
tension of the maintainers concept applied in a better fashion because it en=
courages several developers to provide input on a series of commits instead o=
f one.

I know it's sort of done in some groups [based on commit messages], but it w=
ould be nice to have it better formalized and socialized as well. Things lik=
e 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 b=
ased review is doable and a lot of OSS groups use it, but there are some nic=
e 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 m=
oved 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 w=
ell. Plus it integrates well with p4, and does pretty well with git and svn.=


Thanks!
-Garrett=



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?E11B3FAA-440E-492A-A8DA-FFD376E9B176>