[2994] in BarnOwl Developers
Re: Thoughts on our current code review policy
daemon@ATHENA.MIT.EDU (David A Benjamin)
Thu Mar 24 00:38:37 2011
Date: Thu, 24 Mar 2011 00:38:33 -0400 (EDT)
From: David A Benjamin <davidben@MIT.EDU>
To: Anders Kaseorg <andersk@mit.edu>
cc: Nelson Elhage <nelhage@nelhage.com>, barnowl-dev@mit.edu
In-Reply-To: <alpine.DEB.2.02.1103232303340.4468@dr-wily.mit.edu>
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
---1257051904-408196712-1300940110=:25741
Content-Type: TEXT/PLAIN; CHARSET=ISO-8859-7; FORMAT=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
Content-ID: <alpine.DEB.1.10.1103240015281.25741@dr-wily.mit.edu>
I definitely agree with Nelson that our code review policy isn't working=20
right now. I would love to have more eyes on every change, but we really=20
don't have many available. Particularly not student ones, and I get the=20
impression people in the Real World have less spare time. (This is=20
probably more than somewhat my fault for not pulling many new SIPB folks=20
in. Though, one of these days, dxiao and I are going to successfully=20
schedule time to work on fmtext/style stuff, really.)
The small, trivial fixes and cleanups tend to get bit by this most. I=20
often start little janitor projects when I'm bored, or do another sweep=20
with valgrind. I'll get bogged down later because I have this or that=20
patch in flight, and I don't really want to keep rebasing patches between=
=20
each other. (Amazing as git as, it gets a little silly when the conflict=20
resolution amounts to redoing all two lines of the change.)
My larger, more invasive branches (e.g. g_main_loop) are things I'd be=20
more interested in seeing code review (though perhaps design review is=20
what I'm more interested in, if that's a word). Also ones with rather=20
visible behavior changes. Those seem to also stall (I sent out g_main_loop=
=20
three weeks ago?), but I guess I'm less unhappy about them, being more=20
interested in the review.
I would be interested in toying with putting out weekly or nightly builds=
=20
for testing. The :viewuser fix in 1.7.1 would probably have been caught=20
earlier (it was found rather soon after 1.7.0 was released, as I recall),=
=20
and did slip by review. Though I don't know if we'd get all the benefit we=
=20
may otherwise; restarting a BarnOwl session drops a lot of state, so=20
people aren't too willing to do it. (Until we get persistent logging=20
pony.)
On Wed, 23 Mar 2011, Anders Kaseorg wrote:
> On the reviewer side, it would be great if I could click on something and
> see all the commits that are still waiting for review. Some kind of
> Gerrit, for instance. And we=A2re far from the only greater-SIPB-related
> project that has this need, so we could do a lot of good if we either
> figure out how to get something like Gerrit working on Scripts, or set it
> up on an XVM that several projects can use.
Agreed. Some setup of gerrit or so would be worthwhile, even we don't have=
=20
mandatory review on everything. And maybe some other system with a more=20
well-defined workflow would help with things slipping between the cracks.
(Didn't we decide at the zephyr meetup that zephyr was going to try=20
buildbot and gerrit instances? Possibly worth coordinating there.)
>> so I'm not convinced anyone can effectively review changes for e.g.
>> subtle interactions with other pieces of the code,
>
> This argument cuts both ways: code review is a good way to cause more tha=
n
> one person to be familiar with the code.
I feel like code review is not the best way to get more people familiar=20
with code. If I were new, I would not feel comfortable giving a review on=
=20
code I am unfamiliar with and lack context for. And, as a coder, if it=20
were subtle code that I really wanted another opinion on, I'm not sure I'm=
=20
looking for such a review either. I guess it's useful for existing=20
developers, but I think our problem is much more the headcount than the=20
sum of their contents.
David
---1257051904-408196712-1300940110=:25741--
