Re: review process not working

[address@hidden]

On Jul 26, 2011, at 12:05 PM, David Kastrup wrote:

> "address@hidden" <address@hidden> writes:
> 
>> On Jul 26, 2011, at 11:22 AM, Jan Nieuwenhuizen wrote:
>> 
>>> David Kastrup writes:
>>> 
>>>> The overall code makes obvious that this has been created by a
>>>> comparative novice to the programming languages and data structures of
>>>> Lilypond.  He has been doing his best.
>>> 
>>> I think this patch should be reverted, moved to Rietveld, and worked
>>> on.
> 
> Possibly.
> 
>> The reason for my pow (2.0,...) is because the push broke make and my
>> fix passed regtests.
> 
> My point was that a more experienced person patched something up
> afterwards, and the patch was exclusively for passing a regtest, _kept_
> the problematic real arithmetic and other stuff in the surroundings.
> 
> And no: the regtest _still_ gives out warnings for unknown glyphs.  If
> people ignore warnings in regtests, we will be forced to turn warnings
> into errors.
> 

This is my fault.  I don't know why I missed it these warnings in the 
side-by-side comparison, but I won't let this slip again.

>> Perhaps this would be a good case study.
> 
> The more interesting question is just how common the circumstances are
> under which this happens.
> 
> 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 completely agree.  This is a condition I impose on myself, and I would not at 
all mind it to be policy (at least for newer developers).

>> 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.
> 
>> Did it pass regtests?
> 
> Sure, but with quite suspicious warnings relevant to the test case.
> 
>> Is there any chance that a patch could make it through full review and
>> still not build on all Unix-based platforms?
> 
> Of course.
> 
>> Normally, before making any change I post a patch for review, but as
>> this seemed fairly urgent, I sent an e-mail to devel and "fixed" the
>> issue so that the current master would build.
> 
> Your patch was an improvement in some sense, but it also was a missed
> opportunity for improvement, masking a larger problem.
> 

I see what you mean - I had not been operating under the assumption that the 
original was problematic, but was rather in "oh-crap" mode with regards to 
current master.

Cheers,
MS