Code review required for commits - formal Team vote.

[Ira Cooper]

On Sat, Oct 13, 2012 at 12:05 PM, simo <idra at samba.org> wrote:
> On Sat, 2012-10-13 at 11:34 -0400, Ira Cooper wrote:
>> On Sat, Oct 13, 2012 at 7:28 AM, Christian Ambach <ambi at samba.org> wrote:
>> > On 10/12/2012 07:48 PM, Jeremy Allison wrote:
>> >
>> >> --------------------------------------------------------
>> >> 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.
>> >> --------------------------------------------------------
>> >
>> >
>> > In principle, I would vote +1 for this proposal.
>> >
>> > But I can also understand some of the reservations that others have,
>> > especially if it comes to patches to areas of the code that are not under
>> > regular development near to being un-maintained at all.
>> > It will be difficult to find a second reviewer for this.
>> >
>> > Other areas might be special VFS modules that are made for certain client
>> > applications (like catia, mediaharmony) or uncommon platforms (e.g.
>> > OpenAFS).
>> >
>> > So I'll put my vote on hold until the concerns have been resolved.
>>
>> Well put sir.
>>
>> I agree with your views, and share your concerns, and vote "I haven't
>> voted yet."
>>
>> My concern follows, and may help some of yours and others:
>>
>> I'm also concerned that patches will fall through the cracks.
>>
>> There must be a way that we can know what reviews need to be done
>> easily, and monitor the situation.  I don't know how to do this, but I
>> think we should.  Just so we actually make sure the obscure patches
>> get reviewed.
>>
>> I know at work, I have a website I goto, and it lists my reviews I
>> have to get to.  That's handy, but it is not the requirement.  A
>> command tool, a radio station, samba branded carrier pigeons, anything
>> will work.  It merely needs to exist.  (I believe this echoes some of
>> Matthieu's concerns.)  Given that I think this requirement can be
>> solved about 18,000 ways.  It is not a "show stopper."  But it is a
>> "release blocker" shall we say.
>
> Ok this could be easily done by having a 'review' git branch for each
> team member where patches are pushed.
> Should be easy then to fetch trees in the morning and see what's
> pending, unless people leave the branches unmaintained.
>
> The other option could be bugzilla, but that is a bit more heavy handed.

I 100% left how to do it off intentionally, it is polarizing.  I
merely want agreement that we WILL do it.  How can be voted later.
(Given that I suspect someone will just jump on the sword short term.)

Interestingly, you didn't mention 2 approaches I thought of:

1. Use a review tool.  (Gerrit, ReviewBoard, whatever... I'm not that picky.)
2. Have a simple set of scripts on sn-devel that know the topics of
the outstanding e-mail to be reviewed.  (I'm sure you can see that set
of scripts isn't that hard to write for us if we want it.)

The big thing I've seen so far is: A central repository for the
information, and a common way to query it.

But if you can show me it can be done w/o that.  All the better!

The big thing: If we can't measure it.  It doesn't exist.

And that goes for stale patches, and other such things.  Also I
suspect many of us would run multiple review branches, etc at times.
I don't expect people to have 1 review at a time up.  Most will be 0,
some may be 5.

-Ira