Code review required for commits - formal Team vote.

[Michael Adam]

Salut Matthieu,

On 2012-10-14 at 16:21 -0700, Matthieu Patou wrote:
> Micheal,
> 
> On 10/12/2012 10:01 PM, Matthieu Patou wrote:
> >On 10/12/2012 04:13 PM, Michael Adam wrote:
> >>my vote: +1
> >>
> >>question:
> >>what kind of vote is this intended to be?
> >>unanimous? majority? ...
> >>
> >>comments:
> >>* I don't yet understand the reasons for "-1"s
> >>* it makes no sense to require review but add a free pass after a
> >>   timeout
> >>* Productivity need not drop when not pushing ones own
> >>   patches, because one can work on a private branch until
> >>   patches hit master.
> >>
> >>More later..
 
> Are my remark not worth commenting on ?

I am sorry if you got that impression!
Of course the are.

Let me comment inline below:

> >What I don't like in current proposal:
> >
> >* no planning for the trial (is it a trial ?) no proposed milestones 
> >to say ok we try this for xx hours/days/months then we review that and 
> >then we move to this

In my latest version of a proposal I clearly proposed a trial
period (no enforcement) for 3-4 months.
If desired, we can pin it down to a concrete date, e.g. Feburary 1,
but this only makes sens after we agree to do it at all.

> >* 0 tools are ready, 0 documentation proposal on how this will be 
> >organized

See my mail from last night:

- I explicitly don't want to prescribe or require specific tools
  now, but stay flexible in that respect. The result is the important thing
  to discuss now, namely that commit messages end up in master
  only if they bear a reviewed-tag by >= 2 team members.

- During the trial period, everyone can freely experiment with tools.

> >* no voluntary based period (like ok everybody tries to ask for a 
> >review for all the patches)

As detailed before, many of us have done review on most of their
patches, and so now for a trial period for mandatory reviews,
I think this can only work if the team as a whole commits to trying it.

> >* desire to make it mandatory when some people raise concerns and no 
> >formal process for problem resolution is proposed (going to Gottingen 
> >or any samba team's lair is not a valid solution)

Of course not. :-)

I'd like to try it at first. Just try how well it works out.
I think the teams commitment to trying it would especially
mean that the team members commit to doing reviews and not
let anyone "starve" by not reviewing their patches.

Of course, the concern is valid that this might happen if
someone (e.g. Andrew or Metze) is producing a very big number
of patches. This is why we have to try whether it can work out.

If it turns out to work too badly, then we need to reconsider.
I guess we can tell after a couple of weeks.

> >* no demonstration on this working on a regular basis, I'm not seeing 
> >you requesting reviews for your code, maybe you are doing it in 
> >private but not in public

Many if not most of my patches have been reviewed lately,
many of them have been reviewed by Metze, bearing his signed-off
tag.

I als don't propose to _require_ review to be asked for
explicitly. If done in tight collaboration, then it is perfectly
ok if the patches end up in master with 2 review tags.

I'd just like to not over-formalize but apply some common sense
for the initial trial period. If it turns out we need a more
formal process, we can discuss and establish one later on.

> >I'd like to highlight that with autobuild we had a long period of free 
> >pass but nobody ever exercised it after mid October even though we 
> >still had some trouble with autobuild and given the emails I'm sure it 
> >has hitched some of us. So I don't see a problem of having at the 
> >beginning a free pass and also the objective to make review mandatory 
> >just the opposite by having this option we might be more encline to 
> >play by the rules as we know that there is still a way out (even if we 
> >never exercise it) then once we ironed all the problem and we have 
> >proved ourselves that no kai, andrew or myself will not have to beg 20 
> >times to get a review then why not to make it mandatory with no free 
> >pass but not from the very beginning. If at the beginning due to this 
> >free pass we change our code from having 20% of non public 
> >review(estimation)  and 5% of public review (estimation) to 50% of 
> >public review I think it worth it.

Yep. When we start a trial, there will effectively (technically) be a
free pass, but the idea would be to try not to use it.

> >The fact that I say -1 right now didn't prevent supporters to add some 
> >tools to ease the process, to add some documentation, to see solutions 
> >for problems like autobuild fails because of a flaky build (I do think 
> >that in case of the reviewer push the patch it's the reviewer job to 
> >check if it's due to a flaky tests or maybe because of the patch and 
> >repush it if needed) and envision a process where area with less 
> >tractions get still reviewed a timely manner.
> >
> >On IRC Jeremy said that we should all understand that doing review is 
> >now part of our "job" as a team member and that review should be done 
> >before coding, well I can agree but still kai, mathias and myself 
> >(mostly) are doing it for the fun and if it's not fun anymore because 
> >we have to chase reviews or because we spend 90% of our time doing 
> >reviews then we will most probably contribute much less to samba.
> >Note that I'm not saying that we should get exemption but that we 
> >should search for solution to be sure that nobody get bored and in the 
> >same time the quality improve.

Yeah. I don't know how well it work out.

Also, of course those who can contribute less to samba should not
be expected to spend the little amount of time they have doing
reviews if instead they'd rather be writing code.
On the other hand, doing reviews, might even be a welcome means
for some team members to get more deeply into the code (in some
areas / again / ...).

I think your concerns (and Andrew's and Kai's) are very valid.
I'd appreciate if we would give it a real try, though.

The alternative of just trying to do some reviews could
work out, too, but the effort might as well die out, and on the
other hand a commitment to try mandatory reviews might create
a stronger sense of the need to do reviews (for those who can
afford the time).

So, I don't have full answers or solutions to all your
questions/comments/concerns, but I am positive we could work
it out.

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