Code review tools [WAS: Re: Code review required for commits - Discuss.]

[Jelmer Vernooij]

On Fri, 2012-10-12 at 10:06 -0400, simo wrote:
> On Fri, 2012-10-12 at 15:42 +0200, Jelmer Vernooij wrote:
> > On Thu, Oct 11, 2012 at 03:48:53PM -0400, Ira Cooper wrote:
> > > Jeremy and Simo brought this up in another topic, but it deserves its
> > > own thread really.
> > > 
> > > Also, in most serious work I've done, I've always had reviewers before
> > > committing my code, provided I'm not the only one working on it.  It
> > > is just standard procedure.
> > > 
> > > So consider this my +1, to their ideas, and giving them a "rule".
> > > 
> > > Actual formal suggestion:
> > > 
> > > No team member commits their own code.  All code will be "signed off"
> > > by two team members, as a team member you may sign off your own code.
> > > The "non-author" team member will be responsible for pushing the code.
> > >  If there are two they can agree among themselves. ;)
> > > 
> > > Release branches work as they do today, though the + should be treated
> > > as a sign-off in bugzilla.  All bug commits should now contain their
> > > bug number in them, so we can track back what happened.
> > 
> > In general, I'm in favor of requiring reviews. However, I also think we
> > should be explicit about what the benefits and costs are.
> > 
> > We'll spend more time discussing changes with others, more time
> > chasing reviewers and getting code in better shape to land. That'll
> > help quality and spread the overall understanding of the
> > code over more people, but it'll also have an impact on our
> > overall output. I think reviews can certainly have a positive impact,
> > but let's keep the process lightweight and not bureaucratic.
> > 
> > > I have a rate of "bit rot" I expect on Illumos/Solaris.  It's usually
> > > 1-2 build breaks, and a few minor issues if I walk away for 6 months.
> > > Yes... I have expectations on just how broken things get.  Sometimes
> > > we all exceed them.  Sometimes not.  But the rule of thumb: "Master
> > > won't build 100% right if I haven't touched it in 6 months." is right.
> > 
> > Reviews won't fix portability issues. Maybe we'll catch one or two
> > more because of reviewers that are aware of specific portability
> > issues. But I don't see how reviews will significantly help with this
> > problem.
> > 
> > > No team member commits their own code.
> > I'm puzzled by this. Do we not trust team members to be able to commit
> > their code and follow the policy? This just seems like something that
> > will make landing code harder without any benefits.
> > 
> > Also, can we switch to a different way of doing reviews, like
> > Gerrit? Bugzilla is horrible for this kind of thing.
> 
> We are not proposing to use bugzilla for the reviews for the master
> tree.
I didn't see any reference to what specific tools you were proposing to
use in Ira's original email so I assumed it was bugzilla. Was the
suggestion just to use the mailing list or just $undefined?

I would actually prefer not defining any specific tools, but just saying
that commits need to be signed off by two people - and leaving it up to
them to decide how they want to coordinate. That gives people freedom to
experiment with different tools, and using whatever they feel most
comfortable with.

> Changing subject to avoid hijacking the main thread and keep it to
> process and not tools.
> 
> I do like the concept of gerrit, though I do not like the premises of
> the tool, I have already seen people having issue because (afaik) gerrit
> doesn't use the official git server but they built their own in order to
> do special things, this is a *bad* idea imo.
Gerrit is written in Java and uses JGit, which is different from the
main "C Git" codebase. They don't require a custom server AFAIK, but
it's been a while since I've run gerrit.

> However I would be amenable to the idea on 2 conditions:
> 1. reviews can be acted upon exclusively via email (a mandatory web ui
> is a full nack from me)
> 2. we do not let gerrit own the master branch. The gerrit branch get
> pushed to the real master.
Not letting gerrit own the master branch, and just having it available
for those who want to use it would work for me too.

Cheers,

Jelmer