lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: review process not working


From: David Kastrup
Subject: Re: review process not working
Date: Tue, 26 Jul 2011 12:05:31 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux)

"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.

> 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?

> 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 just recently converted "(format string ..." calls into "(format port
string ..." calls, and did not just brainlessly use #f for a port, but
partly rearranged things in a manner that obliterated unnecessary
display calls, replaced map calls with discarded results with for-each,
fixed obviously broken code and so on.  Basically, while being there
anyway, I cleaned up a bit as well.

It's obviously far from being as effective as a full review, but every
little bit helps.

-- 
David Kastrup



reply via email to

[Prev in Thread] Current Thread [Next in Thread]