[3008] in BarnOwl Developers
Re: Thoughts on our current code review policy
daemon@ATHENA.MIT.EDU (Nelson Elhage)
Thu Mar 24 10:32:23 2011
Date: Thu, 24 Mar 2011 10:32:18 -0400
From: Nelson Elhage <nelhage@nelhage.com>
To: David A Benjamin <davidben@mit.edu>
Cc: Anders Kaseorg <andersk@mit.edu>, barnowl-dev@mit.edu
In-Reply-To: <alpine.DEB.1.10.1103240003160.25741@dr-wily.mit.edu>
On Thu, Mar 24, 2011 at 12:38:33AM -0400, David A Benjamin wrote:
> I definitely agree with Nelson that our code review policy isn't
> working right now. I would love to have more eyes on every change,
> but we really don't have many available. Particularly not student
> ones, and I get the impression people in the Real World have less
> spare time. (This is probably more than somewhat my fault for not
> pulling many new SIPB folks in. Though, one of these days, dxiao and
> I are going to successfully schedule time to work on fmtext/style
> stuff, really.)
>
> The small, trivial fixes and cleanups tend to get bit by this most.
> I often start little janitor projects when I'm bored, or do another
> sweep with valgrind. I'll get bogged down later because I have this
> or that patch in flight, and I don't really want to keep rebasing
> patches between each other. (Amazing as git as, it gets a little
> silly when the conflict 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 more interested in seeing code review (though perhaps design
> review is what I'm more interested in, if that's a word). Also ones
> with rather visible behavior changes. Those seem to also stall (I
> sent out g_main_loop three weeks ago?), but I guess I'm less unhappy
> about them, being more interested in the review.
"design review" is totally a thing that people talk about, and I totally support
design review for larger structural changes. I definitely want to make it easier
for small janitor projects to get in, since I feel like the review process is
really hurting there. There's a lot of middle ground between those; My
personally feeling is that we're in a position where erring on the side of just
getting stuff pushed is better than not.
- Nelson
>
> I would be interested in toying with putting out weekly or nightly
> builds for testing. The :viewuser fix in 1.7.1 would probably have
> been caught earlier (it was found rather soon after 1.7.0 was
> released, as I recall), and did slip by review. Though I don't know
> if we'd get all the benefit we may otherwise; restarting a BarnOwl
> session drops a lot of state, so people aren't too willing to do it.
> (Until we get persistent logging 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’re 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 mandatory review on everything. And maybe some other
> system with a more 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
> 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 than
> >one person to be familiar with the code.
>
> I feel like code review is not the best way to get more people
> familiar with code. If I were new, I would not feel comfortable
> giving a review on code I am unfamiliar with and lack context for.
> And, as a coder, if it were subtle code that I really wanted another
> opinion on, I'm not sure I'm looking for such a review either. I
> guess it's useful for existing developers, but I think our problem
> is much more the headcount than the sum of their contents.
>
>
> David
