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

[simo]

On Fri, 2012-10-12 at 16:48 +0200, Jelmer Vernooij wrote:
> 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.

+1

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>