Code review required for commits - formal Team vote.

[Andrew Bartlett]

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.

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.

Thanks,

Andrew Bartlett
-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org