Code review required for commits - Discuss.

[Volker Lendecke]

On Fri, Oct 12, 2012 at 03:55:06PM +1100, Andrew Bartlett wrote:
> On Thu, 2012-10-11 at 15:48 -0400, Ira Cooper wrote:
> 
> > Actual formal suggestion:
> > 
> > No team member commits their own code.  
> 
> I really think this is a bad idea, and a good way to create a large
> backlog for those of us who are fortunate enough to live in places that
> are (allegedly!) currently enjoying a warm spring afternoon.
> 
> On a serious note, and taking note of your proposal to ensure that
> changes can go in if no review is available, I think this specific rule,
> applied unwaveringly across the tree creates significant disruption and
> work without clear added value.

The added value I see is better code. If it only leads to
more people getting a better understanding of more parts of
the code, I would do it just for that. If it slows us down,
so be it. Yes, we have more developers in my timezone than
in yours, but you can also benefit from it. You can send a
patch out and right behind you you have Germany waking up.
There's whole organizations around this "follow the sun"
principle.

You might argue that it slows down our development. For the
individual patch this is probably true, but overall I am
really convinced that it will even speed up development, if
just for the reason that more developers know more code and
we reduce the risk of bad patches lingering in the tree for
long. The risk is not zero, but it will be reduced.

> That is, given the vagaries of autobuild, it is not reasonable for the
> reviewer and author to have to go back and forth, potentially across
> multiple time zones and days, while getting a patch into the tree.  It
> also dramatically increases the workload on the reviewer, who has not
> only to inspect the patches, but also to find the time in their day for
> what can be a multi-hour challenge to get a series of changes past our
> testsuite.

If it does not pass make test, it goes back to the author.
We have to fix the flaky tests, so that it will be easier to
detect real failures from the flaky ones. I think peer
pressure will lead to people being careful to run private
autobuilds before sending something out.

> I do appreciate review on my code, and you will have noticed that most
> of my changes go past this list for review, particularly if they impact
> on the source3 code in any substantive way.  Indeed, it is ironic that
> the patch that seems to have sparked this crisis was positively reviewed
> by the appropriate maintainer. 
> 
> If there are others interested in the work I'm doing (and we should have
> those in at least the integration work), I'm happy to continue posting
> the substantive patches for comment.  
> 
> Finally, on this proposal, I would like to have it clearly stated what
> exactly (beyond that Google and others have the resources to do this)
> the exact proposal is expected to achieve, and consider if this change
> would really have addressed whatever failure is alleged to have
> occurred. 

The main reason for me to like and +1 the "you can't push
your own stuff, reviewer must push" is a purely
psychological one. If I have to push the code that I review,
this puts significantly more pressure on me to do more
careful review. Just +1ing a patch is way too easy to force
me to do proper review.

With best regards,

Volker Lendecke

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de