Code review required for commits - Discuss.

[Michael Adam]

On 2012-10-11 at 12:52 -0700, Jeremy Allison 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.
> > 
> > Personally:
> > 
> > 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.
> > 
> > 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. ;)

I.e. if the patch carries a "Pair-Programmed-With: .." tag?!

> > 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.
> 
> Thanks for writing this up so well Ira.
> 
> A big +1 from me.
> 
> If a majority of Team members vote +1 on this, I'd like
> to see it adopted as policy immediately.

+1

This will give us much more protection from prematurely pushed
patches. It will create a certain additional work and a certain
additional latency for patches to hit master, but I think it is
worth it. We need to exercise this. Authors need to excercise
their patience, and reviewer need to learn that spending time
on careful reviewing is a serious and important part of our work.

Cheers - Michael

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20121012/76d27525/attachment.pgp>