Code review required for commits - Discuss.

[Jelmer Vernooij]

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

Cheers,

Jelmer