On Tue, Jul 26, 2011 at 12:05:31PM +0200, David Kastrup wrote: > "m...@apollinemike.com" <m...@apollinemike.com> 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 _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
