Code review required for commits - Discuss.

Ira Cooper ira at samba.org
Thu Oct 11 22:26:22 MDT 2012


On Thu, Oct 11, 2012 at 11:52 PM, Matthieu Patou <mat at matws.net> wrote:
> On 10/11/2012 06:58 PM, simo wrote:
>>
>> On Thu, 2012-10-11 at 18:23 -0400, Scott Lovenberg wrote:
>>>
>>> On Thu, Oct 11, 2012 at 4:47 PM, Ira Cooper <ira at samba.org> wrote:
>>>>
>>>> On Thu, Oct 11, 2012 at 4:33 PM, Matthieu Patou <mat at samba.org> wrote:
>>>>>
>>>>> On 10/11/2012 12:48 PM, Ira Cooper wrote:
>>>>>>
>>>>>> Actual formal suggestion:
>>>>>>
>>>>>> 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. ;)
>>>>>
>>>>> 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)
>>>>
>>>> That is correct.
>>>>
>>>> Time to time we all write things that need some more review, and we
>>>> know it.  Not signing them off is a way to say "Hey, I want a second
>>>> reviewer, because I know this is hairy."  I actually have some code
>>>> sitting around that may trigger that, because of what it plays with
>>>> :).
>>>>
>>>> Also it covers all "non-team" code, they all go through 2 sign-offs.
>>>>
>>>> -Ira
>>>
>>>
>>> Ira, we discussed this a bit in the IRC channel today, but I'm
>>> curious.  How does this work for a non-member that has code in Samba?
>>> Can I maintain the parts of code that I've written without two team
>>> members signing off?  It's really not a big deal, but we might as well
>>> get these things ironed out while there is discussion going on.
>>
>> As far as I am concerned 1 signoff (author) and 1 ack should be
>> sufficient, independently of who is the author.
>> However it would be really desirable that who acks is the maintainer.
>
> Why not at the opposite encouraging review by someone not necessarily
> familiar with area of the code ? so that we tend to encourage involvement in
> parts where we usually don't work ?
>
> As for sensitive part people concerned about the patches (ie. maintainer(s))
> could always pop-up on the list and tell that they want to do a review as
> well and do it within 7 days.

Matthieu,

You got it 1/2 right, I agree people should do what they can to expand
as they can.  But the whole "maintainers" system is the 100% anthesis
of that.

No kings, no popes.  If you are respected, people will defer to you
naturally.  Order should come from the chaos naturally, not a file.

"MAINTAINERS" should come out as a cultural convention.  People defer
to those more knowledgable.

As far as the time duration.  I stand by 3 days.  If you can't get to
it in 3 days.. you aren't getting to it.

-Ira


More information about the samba-technical mailing list