Code review required for commits - Discuss.

[simo]

On Thu, 2012-10-11 at 13:43 -0700, 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.

Nitpick:
We should avoid saying 'sign-off' as that has a special meaning in other
communities (kernel) and would confuse people.

In the kernel community people add the 'Acked-by:' flag to ack patches.

> > >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.

Not necessary, again in the kernel community people reply to emails
with:
Acked-by: Foo Bar <foo.bar at bar.foo>

And the committer just amends the commit with the line.

If we want to keep the actual acks in the commit then Karolin could add
them (as well as something like: Bug-Id: 1234) when she commits to the
release branch.

> > >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.

We could add a specific tag, either just the number or the whole url for
convenience:

Bug-Id: https://bugzilla.samba....1234

> > 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.

While this is true I think it is fundamental that patches are not just
thrown on the mailing list, they should CC the specific developer a
review is called from if there is any urgency to commit them.

> > 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.
> 
> > 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.

It is, but we should not need to, we are all adults and can respect a
few rules, right ? If someone abuses we'll decide how to enforce.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>