[3007] in BarnOwl Developers
Re: Thoughts on our current code review policy
daemon@ATHENA.MIT.EDU (Nelson Elhage)
Thu Mar 24 10:22:24 2011
Date: Thu, 24 Mar 2011 10:22:17 -0400
From: Nelson Elhage <nelhage@nelhage.com>
To: Anders Kaseorg <andersk@MIT.edu>
Cc: barnowl-dev@MIT.edu
In-Reply-To: <alpine.DEB.2.02.1103232303340.4468@dr-wily.mit.edu>
On Wed, Mar 23, 2011 at 11:23:59PM -0400, Anders Kaseorg wrote:
> On Wed, 23 Mar 2011, Nelson Elhage wrote:
> > I'm curious what people about how our current code review policy is
> > working out.
>
> I haven’t personally had a problem with it on the coder side. I’ve seen
> some things get forgotten about, but that tends to get fixed when I send
> out a zephyr poke.
>
> 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.
One possibility here I would support would be to submit most branches as pull
requests on github. Their code review infrastructure keeps getting better --
they have line-by-line comment support now, for example -- and since most of us
develop on github anyways, it would be pretty easy to give it a try.
- Nelson
>
> If we had that, it would be much easier to draw conclusions about whether
> code review is stifling development. As things stand, I’m not really
> sure.
>
> By the way, it would also be great if when you forked barnowl on Github,
> it didn’t make a copy of every damn branch. It’s hard to see anything
> about what’s going on when
> $ git branch -r | wc -l
> 164
> (You can help! If you have a barnowl fork on Github, please delete all
> the branches from it that aren’t yours.)
>
> > 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.
>
> Anders
