Code review required for commits - Discuss.

[Michael Adam]

Hi Andrew,

On 2012-10-12 at 15:55 +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.

I don't think it will create a lot of disruption:
As the author, you just keep working with your local branch
containing your patches on top until they hit upstream.
It is not that you are stalled between submitting a patch for
review and the time it reaches the official tree.

You are right that we should carefully think about whether we
should really allow free pushing if no review is available.
Probably we should rather not! Rather wait for some more time.
Someone will be available for review for sure, even if it takes
a week or 10 days under bad circumstances. Maybe no free pass
rule would be better for a start.

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

I don't buy that argument: If you seriously review patchsets (at
least more complicated ones), then you have to apply them to a
local working tree anyways and build it and test it and closely
look at the code.

The act of pushing it to autobuild then is not time consuming at
all. If autobuild fails due to the patches, they go back to the
author. If autobuild fails due to a unrelated flakey test, you
just push again. No big deal actually.

One should run manual autobuilds before submitting the patch
for review. This will basically remove the first failure case.

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

Yes. This can happen again and will happen again with mandatory
review and non-author push of course! (Since neither review nor
autobuild are complete...) This example just triggered the
discussion again.

And I don't see it as a crisis at all. It is simply a time to
review our push process, whether we can move to a better mode.

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

The point of discussion is the proposal to establish the _requirement_
for _everyone_ to post all (not just the substantial) patches for
review and not push on ones own.

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

Mandatory review will increase the code quality, I think,
reduce bugs (uncaught by autobuild) and reduce design flaws
(or improve design, to put it more positively) before code hits
master.

I guess if review is taken seriously and done carefully, then
many patchsets will take more than one roundtrip before acked.
If you happen to do pair-programming on a patchset, then review
is more or less automatic, of course.


Think about it this way:

Up to now, you desperately asked for review for many if your
patchsets. If no review was done after quite some time you
pushed frustratedly. With the new proposed rule, others would
be _required_ to do review. I.e. you have a much stronger
position in requesting review.

I think that this puts two new challenges to the developers
(as I have briefly mentioned in a different mail already):

1) the author needs to excercise her silence.

2) the others need to excercise their discipline in doing
   reviews carefully and in a timely manner.

   This is the much bigger change.
   The move would establish review as a much more important
   and integral part of our software development.

This won't work perfectly from day 1 one, but should improve.


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/fc82c521/attachment.pgp>