Code review required for commits - formal Team vote.

[Michael Adam]

Hi Andrew,

On 2012-10-13 at 19:25 +1100, Andrew Bartlett wrote:
> On Sat, 2012-10-13 at 09:33 +0200, Kai Blin wrote:
> 
> > Basically I'm asking that we prove that the code review process works
> > before adopting it as a mandatory policy. You're asking us to adopt
> > it, and then possibly need to prove it doesn't work. I'm afraid it's
> > going to be hard to prove, because I'm sure the process will work for
> > some people.
> 
> I agree, and what I fear is that those of us likely to have the most
> trouble with it will simply (by the tone of the argument so far) be
> labelled not as experienced contributors with a valid point but
> dissenters who didn't really try the process hard enough.  
> 
> Without agreed metrics at the start, there will be no way to measure
> success or failure, or the relative benefit to the cost. 
> 
> Therefore I fear the 'proof' will be that others found it so terribly
> successful and so the problem can't be with the policy, but with us.
> Rather than tweak the policy to deal with our legitimate concerns (and
> therefore create something we all can live with), we will be sidelined.
> 
> Perhaps that's not the intent, but then we need to very quickly deal
> with the tone of the advocacy for this proposal.

As Jeremy wrote in a different mail, the intention was to try
mandatory review for the samba team now, and not to put it into
a formal policy rule at this time.

> To put it another way, consider if this was the proposal:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> A number of members of the team have for some time used a [author !=
> commiter|100% of changes|all substantive changes|voluntary] review
> modal.  We would like to encourage more of the team to take this on, so
> we wanted to see if others would like to join us.  
> 
> It will be a trial in a sense, to show that this really does improve the
> quality of the code we all write, and to show others on the team the
> practical process.  If you join us, you have the commitment that one of
> us will review and push your patches in a timely manner (typically 3
> business days).  Naturally, we expect you will also do some review in
> turn.  
> 
> None of this impacts on your rights or status on the team, nor your
> responsibility to ask the right person for review of complex or
> controversial changes.  
> 
> All patches should be sent via samba-technical in at least the first
> instance. 
> 
> If this doesn't work out for you, then there are no hard feelings, and
> you can continue to work they way you have always done.  However, we
> hope you might be willing to indicate why, so we could perhaps craft a
> better process that we all can work with.  
> 
> There will not be any social pressure to conform to this as a de-facto
> rule.  However, if at some point in the future (ie after a number of
> months) we all do this as a matter of course and our metric of number of
> reverts or post-commit re-edits shows a helpful reduction, then we may
> come back for a vote to make this some kind of rule. 

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> If that was how this was put, I imagine this would have a very different
> reaction.

Yes, the proposal has had a very unfortunate start, indeed.
Let me rephrase what my understanding and view of the proposal is:

* A couple of us have worked with frequent review and partly
  non-author push since some time and made very good experiences.

* Furthermore other projects use this successfully, and maintain a
  good level of patch quality with it.

* Therefore I would like to propose that we try out mandatory
  code review for the samba master branch.

* I would futhermore popose to require (for the trial period)
  the pusher of a patch to be different from the author, since
  pushing as part of reviewing might force the reviewer to review
  more thoroughly (doing the push seems to demonstrate a stronger
  commitment/involvement).

* I believe that such a trial can only work if the team as a
  whole commits to testing mandatory review for a certain period
  of time, after which it can be decided whether we want to move
  forward with rooting mandatory review into a formal policy.


Here are some details for a proposal:

1. Two team members should review patches.

   - If the author is a team member, then the author counts as
     reviewer, so one additional review is sufficient.

   - If the patch is created by pair-programming of two team
     members, the two authors are also counted as reviewers.

2. The review should be indicated in the commit message by a
   tag like "Signed-off-by: Name <email>".

   But this can be discussed: A different tags that has been
   proposed: "Acked-by: ...". I'd also suggest "Reviewed-by: ..."

3. The author should not push his/her own patch, so usually the
   reviewer should push after positively reviewing the patch.

   - If the patch is done by pair-programming (of team members),
     i.e. carries the "Pair-programmed-with: ..." tag,
     any of the authors may push.

4. At this time I do explicitly not propose any specific mechanism
   for reviewing. The important thing is the result in the
   commits and pushes!

   A couple of possibilities:

   - The patch can be posted to the samba-technical mailing list
     asking for review.

   - If developed in collaboration, the patch can also be
     pushed directly by the reviewer / co-author.
     (Indeed, review can be quite an interactive process.)

   - If anybody wants to try a special tool for organizing
     reviewing, that is of course also possible.

5. Committing to doing (trying) mandatory code review also
   means that we commit to doing reviews more reliably and
   timely that possibly up to now as a part of our regular
   development work.

I guess this is not complete yet, but the most important points
(for me) are covered.

Christian's concerns about special areas of code like
super-special VFS modules are not covered yet here, but our trial
period will tell whether we need special rules for such areas.

I suggest that we try this for 3-4 months. After that timeframe
we should evaluate the outcome. If all goes well, we would have
elaborated a process and formal requirement that we could put
into a policy. If we meet impossible obstacles before that
instead, we need to revise and re-discuss earlier of course.
But I think we need to give it at least a couple of weeks' time
to make a meaningful statement.

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/20121015/0e2b183f/attachment.pgp>