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

[Carl Sorensen]

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:
>> On 11/3/11 10:50 AM, "Adam Spiers" <address@hidden>
>>wrote:
>> >I originally avoided any pure-whitespace commits, but at the
>> >review of my initial patches, I was told to replace tabs with spaces
>> >in the files I was modifying:
>> >
>> >  http://codereview.appspot.com/4981052/
>> 
>> 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.

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

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,
the build status of all previous commits will be good.  So I'm comfortable
with pushing your chord patches as a set of commits.  But in general, I
don't think that's a good idea.

Thanks,
Carl

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/