Code review required for commits - Discuss.

[simo]

On Fri, 2012-10-12 at 16:08 +0200, Jelmer Vernooij wrote:
> On Fri, 2012-10-12 at 09:11 -0400, simo wrote:
> > On Fri, 2012-10-12 at 18:18 +1100, Andrew Bartlett wrote:
> > > On Fri, 2012-10-12 at 08:56 +0200, Michael Adam wrote:
> > > > On 2012-10-12 at 08:43 +0200, Volker Lendecke wrote:
> > > > > On Thu, Oct 11, 2012 at 04:07:34PM -0700, Jeremy Allison wrote:
> > > > > > On Thu, Oct 11, 2012 at 06:54:50PM -0400, Ira Cooper wrote:
> > > > > > 
> > > > > > > I think Matthieu has a point here, and it is important.  Timely review
> > > > > > > is going to be key.
> > > > > > > 
> > > > > > > And I'm going to amend myself:  If there is code that has sat for 3
> > > > > > > business days, the author/sponsor may push.  A simple flag for review
> > > > > > > will buy time to do the review on complex code.  Use your brain.  But
> > > > > > > if there are no flags... that is consent from "samba-technical".
> > > > > > 
> > > > > > I'm ok with that. If someone ignores a patch on samba-technical for
> > > > > > 3 days without even saying "I want to review this" then it's probably
> > > > > > good to go.
> > > > > 
> > > > > Just for my understanding (lots of mails to read...): Does
> > > > > this mean that if I ask someone for review and I get no
> > > > > response whatsoever, I am free to push it without review? Or
> > > > > do I have to get someone else to review it?
> > > > 
> > > > My feeling is that 3 days might not be enough.
> > > > I think we could do the following:
> > > > 
> > > > * sending a patch to samba-technical with request for review
> > > >   is the call for review to everyone.
> > > > * if there is no reaction, author should ping again after 2-3
> > > >   days (workdays).
> > > > * I would give a free push pass not earlier then 1 week after
> > > >   requesting review
> > > > 
> > > > Generally, it should work out in not more than 2-3 days of course.
> > > 
> > > As Ira said before, and I tend to agree with him on this much, if nobody
> > > is interested after 3 business days, then nobody is interested.
> > > Stretching this process out for a week is only going to cause
> > > aggravation for no additional benefit. 
> > 
> > Can you explain what is the aggravation ?
> > 
> > I *really* do not get it.
> > 
> > I have kept 80+ patchsets in my tree for *months* in all major works
> > I've done in the past couple of years and I haven't had a single issue
> > with it.
> > 
> > So I really don't get why you keep saying this is disruptive ?
> > *How* exactly is this model disruptive to you ?
> > *What* exactly prevents you from doing ?
> > 
> > Yes reviews will force you to create better patches and sometimes to
> > rework them, what is wrong with better code quality ?
> 
> All of this takes quite a bit of extra time - rebasing, resolving
> conflicts - especially if you're touching rapidly changing areas of the
> code.
> 
> Now, that time spent may very well be worth the benefits (I suspect it
> would be), but it does have a significant impact on the way we work and
> how much time is spent on hacking vs process (rebasing/reviewing/etc).

Keep in mind that way too often we have to 'fix things later' with the
current model. Stuff that *should* have been caught on review and would
have wasted a lot less time.
catching a bug later sometimes takes a *lot* more time than a review.

I have had very good success in this regard with other projects. It
doesn't mean bugs do not happen, but the quality is certainly superior,
which means less debugging down the road and wasting a lot of peoples
time catching heisenbugs or other bugs that are very hard to reproduce.

Every heisenbug we can spare is a lot of time that can go into reviews
and have even better code quality and shared knowledge.

Remember, unmaintained/unmaintainable code is dead(ish) code anyway.

Simo.

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