Code review required for commits - Discuss.

[Andrew Bartlett]

On Fri, 2012-10-12 at 10:22 -0700, Jeremy Allison wrote:
> On Fri, Oct 12, 2012 at 01:41:01AM -0700, Matthieu Patou wrote:
> > On 10/12/2012 01:20 AM, Volker Lendecke wrote:
> > >On Fri, Oct 12, 2012 at 10:16:21AM +0200, Kai Blin wrote:
> > >>-----BEGIN PGP SIGNED MESSAGE-----
> > >>Hash: SHA1
> > >>
> > >>On 2012-10-12 09:38, Michael Adam wrote:
> > >>
> > >>Hi,
> > >>
> > >>>Ok, the main point is this:
> > >>>
> > >>>Change "review" from a matter of interest to a required step in
> > >>>our software development.
> > >>>
> > >>>Hence, I am inclinend to change my proposal to:
> > >>>
> > >>>* we should _not_ allow a free push pass after any number of days
> > >>>at all.
> > >>Ok, as the one who works on an area that sees little interest at all
> > >>until things break, how do I make sure I get my code in?
> > >Just be a PITA for the reviewers. Send mails, call them,
> > >start to use other means like hop on a train to Goettingen
> > >with a LART tool :-)
> > Of course I do understand the joke but in every joke there is a part
> > of truth, so what about people in .au or .us ? Should I knock on the
> > door of Jeremy to get a review ?
> 
> You do know where I live :-).
> 
> > I don't think it's being fair to the developer to ask him to nag
> > reviewers in order to get a review. I do think that within 3 days a
> > reviewer should pop up to say: I want to review this and then maybe
> > the reviewer gets a bit more time to do the review but I still do
> > think (also) that it should be limited so that we keep on the
> > dynamic and review never get stuck for weeks as it has been the case
> > in the past for some voluntary reviews.
> 
> Look at the things that have been stuck for weeks (and I'm
> mainly thinking of poor ddis's async RPC rewrite).
> 
> They're mainly *big* changes that require lots of work to
> review.

As this hasn't been pushed to the tree prematurely, what is broken about
our current process?

> But you know what, they're the things that *should* be
> reviewed more thoroughly.
> 
> I haven't seen the case where a simple patch didn't get
> a prompt ack.

As I said, to my recollection, almost none of 'simple patch'
integration-related changes I've posted to this list in the last year
have had an ack, prompt or otherwise.

Andrew Bartlett
-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org