Code review required for commits - formal Team vote.

[Matthieu Patou]

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..
> 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.
Are my remark not worth commenting on ?


Matthieu.