Code review required for commits - Discuss.

[Matthieu Patou]

On 10/11/2012 01:43 PM, Jeremy Allison wrote:
> On Thu, Oct 11, 2012 at 01:33:43PM -0700, Matthieu Patou wrote:
>
>> To my french mind the wording seems a bit confusing, are you saying that:
>> 1) every author could (should?) add their own sign-off
>> 2) in total 2 sign-off are need, which means at least 1 non-author
>> review (but can be two if the author decide not to sign-off it's
>> patch)
> Yep.
>
>>> Release branches work as they do today, though the + should be treated
>>> as a sign-off in bugzilla.
>> Why-not but should it be automatic ?
> I don't think we need any change to what we're currently
> doing in release branches - this is just writing up what
> we already have. The reason it can't be automatic I think
> is that a patch gets posted to bugzilla, other people +1
> it then Karolin commits. For a + to add a signoff people
> would have to re-upload the patch with their signoff added,
> and I don't think we need that extra step.
>
>>> All bug commits should now contain their
>>> bug number in them, so we can track back what happened.
>> Agreed.
> We've mostly been doing that anyway.
>
>> In general I'm mostly ok with this proposal but we have to aware
>> that will increase the workload on some of us.
> Yes, this is true.
I mean I don't care too much because I don't think I'll receive a lot of 
reviews but maybe should I ?
>
>> Also I want to have precision on how to handle the failure in
>> autobuild and I'd like to see a kind of watchdog so that patches
>> can't wait more that xx days for review.
> Hopefully peer pressure will act as a watchdog here, plus
> pressure from the patch author.
I still prefer a clear rule that will put some social pressure on the 
reviewer and also give a kind of grant to the author of when this will 
land in master.

>> Finally as we did so far we can try this rule on a voluntary basis
>> (it was the case for autobuild too) and see how it flies for 1 or 2
>> months.
> I'm not sure this is enforcible by automation (git experts please chime in
> here :-) so it'll be by Team convention anyway.
I don't think it should be very difficult to check that the autobuild 
user has a signoff in each commit and that autobuild user != author
Still in order to let people get familiar with this we can start on a 
voluntary basis rather than "now it has to be like that" (even if we 
don't have tools to enforce it).

Matthieu.

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