Code review required for commits - Discuss.

Jelmer Vernooij jelmer at
Fri Oct 12 07:42:37 MDT 2012

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

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <>

More information about the samba-technical mailing list