Code review required for commits - formal Team vote.

[Michael Adam]

Hi Jeremy,

On 2012-10-16 at 01:38 -0700, Jeremy Allison wrote:
> On Mon, Oct 15, 2012 at 01:05:31AM +0200, Michael Adam wrote:
> > Yes, the proposal has had a very unfortunate start, indeed.
> > Let me rephrase what my understanding and view of the proposal is:
> >
> 
> ..... (lots of stuff deleted :-).

Hehe, I can add a lot of stuff again. ;)

Watch out there is a new suggestion further down!

> > 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.
> 
> So as far as I can tell this is the same as the original
> proposal (lot more words though :-) with the addition of
> a review period after 3-4 months to evaluate how things
> are going.

Good. Then at least I have not coarsely misunderstood you! ;-)
I think the discussion has shown that it was not absolutely
clear that the initial proposal was one for a trial period.

> Michael, if you want to make an official change to the proposal
> to add the (let's say) 4 month evaluation period, then we probaby
> need to reset the vote count (sorry, but everyone needs to be very
> clear on *exactly* what we're all voting for IMHO) and reset the time
> period for another week after you make this proposal.
> 
> If you want to officially make this proposal I'm happy to
> withdraw mine in favour of this.

I am not sure that we are still in the stage of official proposals
to be voted on. I think we are somewhat back at discussing things.

If we are proposing a trial period for mandatory code review,
then we need to have some form of consensus. It can only work
if the team commits to seriously trying it. I don't think
a simple majority vote can lead us to a meaningful trial period,
since this would assume good will and motivation to work in this
mode from virtually all of the team.

The idea was brought up and quickly a vote was called,
but the discussion has shown that there is (imho) not enough
consensus right now to reasonably do it.

I originally thought the matter was rather simple, but it
has turned out to have created a big wave. Talking to various people
also off the mailing list thread, I see that there are various
concerns about the switching to mandatory code review and non
author push even with a trial period. We should take these
concerns seriously!

So, since we are discussing anyways, and the discussion takes
a lot longer and gets a lot more heated than I had anticipated,
I think we should take some momentum out of it and not force
things.

SUGGESTION:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1. I suggest that we defer a team vote on a commited trial period
   or even a policy change for some time, at least a couple of weeks
   until we have thought and discussed things through thoroughly
   and without hastening, maybe until after 4.0 is released.

2. In the meanwhile, we encourage team members to do reviews
   more often on a voluntary basis, which means both asking for
   review before pushing and giving review for others.

3. We also encourage to have the reviewer push.

4. We can also try any mechanisms/tools on a voluntary basis.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This allows us to concentrate on shipping 4.0 which is the most
important goal currently! The worst we can do now is delay or
spoil the 4.0 effort by working ourselves into the ground over
heated policy discussions.

And it allows us on a completely voluntary basis to gain more
experience and confidence in the reviewing business and possibly
get an idea of the load and mechanisms.

This will certainly move towards mandatory reviews much slower
than the hard trial period, but it is more respectful for the
folks that have serious concerns. I think that we will still make
progress, based on the following observations:

- I think we can safely assume that we all want to sustain and
  improve the quality of our commits and would consider changing
  our mode of operation on the master branch if suitable.

- I got from mails and discussions that nobody actually
  thinks that doing reviews is a bad thing!

The concerns we should address are these:

- How will we provide the ressources to review 6000 patches / year?
  (or will it imply a drop in productivity?)

- Do we need a tracking tool for reviews so patches don't get lost?
  (this is roughly a.k.a how do I get a reviewer for my patch?)

- Can we find *two* team reviewers even for the most exotic areas
  in code ore do we need to establish more flexible rules?

- and probably more I forgot to mention, please fill this up...

I hope that even a voluntary period (as has been suggested by
others) will lead us to some insight about theses issues in the
next couple of weeks and that we can move forward with voting
about something more concrete and with a better feeling attached
to it after 4.0 is out.

Cheers - Michael


PS: Jeremy, I apologize for back-pedalling here. I still think
we should do it, but not by all means. This is the only way
I see how we can continue in a reasonable and respectful way.

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