Code review required for commits - formal Team vote.

Fri Oct 12 23:01:13 MDT 2012

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..
I'll try to repeat the reasons that made me vote -1.
But before let's rewind on autobuild that is so often compared:
* on Sept 30 2010 tridge proposed that everybody start to try it out 
because according to him it works well (first traces starts on the 25th 
of September)
* he proposed to try for one week and then propose to make mandatory 
(nobody voiced against it), by that time some tools and documentation 
was already present
* on Oct 3 2010 tridge propose to make it optional one more week then 
mandatory but with no enforcement and then one more week and enforce it
* on Oct 13 tridge to propose to make it mandatory but not enforced but 
already a lot of person at that time were using it as de-facto standard 
and even if they had a free-way and we had autobuild errors due to 
flakey tests nobody used it
* on Feb 03 2011 metze activated the enforcement

What I don't like in current proposal:

* 0 tools are ready, 0 documentation proposal on how this will be organized
* 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
* no voluntary based period (like ok everybody tries to ask for a review 
for all the patches)
* 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)
* 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

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.

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.

Cheers.
Matthieu.

>
> Cheers - Michael
>
> On 2012-10-12 at 10:48 -0700, Jeremy Allison wrote:
>> Ok, I'd like to bring this to a conclusion before I have to go
>> on my European trip next week :-).
>>
>> Sorry for asking for another vote after people have already
>> +1 and -1'ed, but there have been so many discussions about
>> what exactly the details of the policy should be that I think
>> it might be useful to re-iterate exactly what we're voting on.
>>
>> Just to be clear - the formal proposal, originally from
>> Ira (but slightly tweaked by me after the discussions)
>> is here:
>>
>> --------------------------------------------------------
>> No team member commits their own code.  All code will be "signed off"
>> by two team members, as a team member you may sign off your own code.
>> The "non-author" team member will be responsible for pushing the code.
>> If there are two they can agree among themselves.
>>
>> Tools for code review to be decided amongst reviewers, but any
>> patch posted to samba-technical is an explicit request for review.
>>
>> Release branches work as they do today, though the + should be treated
>> as a sign-off in bugzilla. All release branch bug commits must contain
>> their bug number in them, so we can track back what happened.
>> --------------------------------------------------------
>>
>> Given this policy (no author push, no free pass) as an initial
>> start I think we can move to a vote.
>>
>> Yes, there will be things to sort out and discuss as we go
>> along, but we're all reasonable (-ish :-) people and can work
>> out the minor details to make it work as we go along.
>>
>> So I know many people have voted back and forth on various
>> versions, but I'd like to have a formal Team vote (anyone
>> on the Team with a samba.org address) to make a decision on
>> this. No reply means abstention.
>>
>> Let's wait until the end of next week to allow everyone
>> who might be travelling or not reading email to respond
>> and think carefully about it, and tally the votes on
>> Friday 19th Oct (I'll be travelling on that date so
>> someone else will have probably to do the count, volunteers
>> welcome :-). If it passes let's target implementation on
>> Monday 22nd Oct.
>>
>> I'm voting +1 (in case you wondered).
>>
>> Cheers,
>>
>>          Jeremy.

-- 
Matthieu Patou
Samba Team
http://samba.org