Re: changes in chord names formatting (1503, 1572) (issue 4981052)

[Adam Spiers]

On Fri, Nov 04, 2011 at 11:55:42AM +0000, Carl Sorensen wrote:
> On 11/4/11 5:35 AM, "Adam Spiers" <address@hidden> wrote:
> >On Thu, Nov 03, 2011 at 06:28:55PM +0000, Carl Sorensen wrote:
> >> I can't find the suggestion to replace tabs with spaces in this review
> >> string, so I can't comment on the suggestions.
> >
> >http://codereview.appspot.com/4981052/#msg3
> 
> OK, I don't know how I missed that before.
> Had I seen that review, I'd have mentioned that .scm indentation is not
> yet a settled issue on the GOP.  I'm sorry I didn't catch it earlier.

No problem :-)

> >> Our policy of testing each commit to ensure that it doesn't break
> >> build prevents multiple patches per issue.
> >
> >I don't understand why this policy would prevent multiple patches per
> >issue, but you probably shouldn't waste time explaining it to me :)
> 
> Because the finest granularity we have in the review process is a Rietveld
> issue.  So we only check each Rietveld issue to verify that it doesn't
> break build.
> 
> It would be possible to have a series of three commits in a Rietveld
> issue, with the first two commits breaking the build, and the third fixing
> the problem.  When this set of three is checked on Rietveld, we get a
> clean make, so the push is approved.  If I then push to master as a set of
> three commits, two of them break build and therefore mess up git-bisect.

Sure, understood - but I still don't see why each commit within a
single issue couldn't be checked accumulatively, rather than just
applying all three together and only then doing the check.  Of course
it's more work, but arguably still less work (and less noise) than
creating an issue per commit.

> In the case of your chords patch, I looked over each commit carefully and
> I'm quite certain that if the build status of the final commit is
> good,

Sadly there is one regression :-(  I've just posted a fix to:

  http://code.google.com/p/lilypond/issues/detail?id=1503

> P.S. Your patch is currently up for review, but with a different issue
> number since I'm now the owner
> 
> http://codereview.appspot.com/5320074/

Yep, I got the email alerts for that.  Thanks for your work on this!