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

simo idra at samba.org
Fri Oct 12 08:06:21 MDT 2012


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.

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.
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.

Simo.

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



More information about the samba-technical mailing list