Code review required for commits - formal Team vote.

[simo]

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.

Simo.

Simo.



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