Re: review process not working

[Graham Percival]

On Tue, Jul 26, 2011 at 12:05:31PM +0200, David Kastrup wrote:
> "address@hidden" <address@hidden> writes:
> 
> > Perhaps this would be a good case study.
> 
> The more interesting question is just how common the circumstances are
> under which this happens.
...
> > Did 104f80daf1dab11ef5b598006e3d4be8dfbe1926 go through full review?
> > Was it verified with a full build?
> 
> Very likely.  This was not "gold star" quality which tends to work on
> first compilation attempt.

This patch looks absolutely excellent to me.

  104f80daf1dab11ef5b598006e3d4be8dfbe1926
fixed:
  http://code.google.com/p/lilypond/issues/detail?id=1655
first patch set May 17 or so:
  http://codereview.appspot.com/4543055/
next patch set May 26 or so:
  http://codereview.appspot.com/4536068/
this second patchset had a total of:
  - 6 drafts
  - 9 comments not from the author
  - a LGTM from Neil
  - countdowns on
  June 03
    http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00036.html
  June 06
    http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00104.html
  June 14
    http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00283.html

Speaking from an administrative perspective, this patch looked
*excellent* to push.  I'd say that only about 5% of pushed patches
have received as much scrutiny as this one; if the bar is even
higher, then we'd be looking at pushing 4-5 patches per month.


Make no mistake -- I think it would be great if highly skilled
people spent more time reviewing patches.  Some problems still
slip through the cracks, and if somebody is willing to spend more
time working to avoid such problems, then great!

But we will always have *some* problems.  The goal should be to
minimize or mitigate those problems, not to require so much
scrutiny that problems are impossible.

> Perhaps a minimal measure of sanity would be if a patch countdown
> without code review was only started when the author of the patch says
> "I feel reasonably confident that this not just works, but is good".
> 
> In git, there is the "formal" sanctification of "Signed-off-by".
> Perhaps we should not start a patch countdown on any patch that has not
> been signed off by anybody?

I speak against this, at least for now.  This is a question of
balance between support for new contributors (i.e. mentors, of
which we have far fewer than I would like), amount of available
reviewers (which is smaller than we would like), and the moral of
contributors.


> > Did it pass regtests?
> 
> Sure, but with quite suspicious warnings relevant to the test case.

This is a different problem.  Am I correct in reading that it
"passed" the regtests but added some warnings?

IMO, that should not be considered to be "passing" the regtests.
Of course, at the moment we have some "harmless" warnings in the
regtests; it would be great if we could remove all those
"harmless" warnings and be stricter about warnings.  I would
certainly like to enable warning-as-error for the regtests:
http://code.google.com/p/lilypond/issues/detail?id=814

Cheers,
- Graham