Code review required for commits - formal Team vote.

[simo]

On Wed, 2012-10-17 at 09:07 -0400, Ira Cooper wrote:
> On Wed, Oct 17, 2012 at 9:00 AM, simo <idra at samba.org> wrote:
> > On Wed, 2012-10-17 at 08:53 -0400, Ira Cooper wrote:
> >> On Wed, Oct 17, 2012 at 8:43 AM, simo <idra at samba.org> wrote:
> >> > On Wed, 2012-10-17 at 05:18 -0700, Jeremy Allison wrote:
> >> >> On Wed, Oct 17, 2012 at 07:17:48AM -0400, Ira Cooper wrote:
> >> >> > On Wed, Oct 17, 2012 at 3:07 AM, Jeremy Allison <jra at samba.org> wrote:
> >> >> > > On Wed, Oct 17, 2012 at 01:00:58AM +0200, Michael Adam wrote:
> >> >> >
> >> >> > >> - Do we need a tracking tool for reviews so patches don't get lost?
> >> >> > >>   (this is roughly a.k.a how do I get a reviewer for my patch?)
> >> >> > >
> >> >> > > Right now I'm using my inbox. What would you suggest as
> >> >> > > a different tool ? At work we use gerrit, but Simo has
> >> >> > > some valid (Java cough, cough :-) reasons to avoid that :-).
> >> >> >
> >> >> > Why is Java a valid reason?
> >> >>
> >> >> I'll let Simo answer if he wants, but basically he objects
> >> >> to them re-writing a git server rather than re-using the
> >> >> existing one. Java is just the punchline to the joke :-).
> >> >
> >> > I mostly object in gerrit taking over the master tree, I'd be fine using
> >> > it if it were a side tool that could push *and* pull from master if/when
> >> > we push patches there.
> >> > Also as much as I like neat UIs I strongly prefer a tool that let's you
> >> > do everything from the command line. Autobuild does that, but afaik
> >> > gerrit requires WebUI interaction, to review/push patches.
> >> > I really do not like that, I want the option of doing reviews offline
> >> > and commit stuff in my 'reviewd' branch to be pushed once I get online.
> >>
> >> It has a cli.
> >>
> >> https://review.openstack.org/Documentation/cmd-index.html
> >
> > ... and I learn Gerrit has it's own ssh server too ??
> >
> > that's really bad, does it have it's own kernel to install too ? :-)
> >
> >> I believe you can also just pull anything up for review... So you
> >> should be able to avoid the web ui.
> >>
> >> I personally believe the requirements we have can be met by some
> >> python scripts that just keep track of what e-mail threads are active
> >> on SN, and possibly output a webpage :)
> >>
> >> Note: I don't care if it is Gerrit, or just a set of python scripts on
> >> sn that say "this e-mail topic is active... this one is being reviewed
> >> by x, this one is closed."
> >>
> >> Nobody says this has to be complex.  It as complex as we want to make
> >> it, I explicitly stuck to giving requirements for a reason.  How to
> >> implement those requirements in a non-odious way is another topic. :)
> >
> > Yup, we already have autobuild that does all the automatic and
> > regression testing we need. And we just need to keep track of changes.
> > I believe a simple git tree would work neatly as long as the owner
> > maintains it (ie removes changesets that are not relevant anymore)
> 
> Ah, I'd rather the reviewer do that ;)

Yeah that works too, but then the branch needs to be in a repo on which
every team member can write by default. I am fine with creating a new
'review' git repo with a branch per person.
This way people can push stuff in the branch under their name and
reviewers can pull and clean up if they push, or even propose changes
back if so we decide to interact.

The problem is that we will probably need multiple branches per person
as you do not want to pile unrelated patchsets on the same branch, so it
came become complex pretty quickly.

Maybe we should have a patchset tracker of some sort.

Simo.

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