Re: Doc: NR - 2.10.2 - Arabic Music - improved example (issue 572600048 by address@hidden)

[David Kastrup]

address@hidden writes:

> Hello,
>
> On 29/01/2020 22:47, address@hidden wrote:
>> https://codereview.appspot.com/572600048/diff/548660043/Documentation/snippets/new/non-traditional-key-signatures.ly
>> File Documentation/snippets/new/non-traditional-key-signatures.ly
>> (right):
>>
>> https://codereview.appspot.com/572600048/diff/548660043/Documentation/snippets/new/non-traditional-key-signatures.ly#newcode5
>> Documentation/snippets/new/non-traditional-key-signatures.ly:5:
>> version-specific, non-traditional, world-music"
>> Breaking this line breaks makelsr.py .  How did this get through?
>>
>> I see that it was silently fixed in
>>
>> commit c511518a4e7669ac7263f8a28565debefe25a358
>> Author: James Lowe <address@hidden>
>> Date:   Mon Apr 15 21:03:57 2019 +0100
>>
>>      makelsr run for previous commit
>>           Changes made based on commit
>>      Doc: NR - 2.10.2 - Arabic Music - improved example
>>      03f78e3e7bda039ea08744d3736bfa4a39dea2a4
>>
>> diff --git
>> a/Documentation/snippets/new/non-traditional-key-signatures.ly
>> b/Documentation/snippets/new/non-traditional-key-signatures.ly
>> index f5967009bd..f2a16ca841 100644
>> --- a/Documentation/snippets/new/non-traditional-key-signatures.ly
>> +++ b/Documentation/snippets/new/non-traditional-key-signatures.ly
>> @@ -1,8 +1,7 @@
>>   \version "2.19.21"
>>     \header {
>> -  lsrtags = "contemporary-notation, pitches, staff-notation,
>> -  version-specific, non-traditional, world-music"
>> +  lsrtags = "contemporary-notation, pitches, staff-notation,
>> version-specific, non-traditional, world-music"
>>
>> which is a really, really, really bad idea since obviously makelsr runs
>> are not cherry-picked, so this fix should have been made in a separate
>> commit.  Took me a few hours (after all, make doc will take about half
>> an hour on my computer).  Please folks, don't pack corrective commits
>> into the commits running scripts.
>>
>> https://codereview.appspot.com/572600048/
>>
> I don't understand what I did wrong (apologies by the way).
>
> I often get asked to add snippets so the workflow is to test the patch
> with the snippet + makelsr (obviously else make doc fails) but when I 
> come to push, the makelsr 'run' is always in a separate commit.
>
> I thought that was the preferred way.

And that is perfectly fine and ok.  The problem is that this commit did
_not_ just run makelsr.py (as it announces to be doing) but _also_ fixed
a bad header line in
Documentation/snippets/new/non-traditional-key-signatures.ly .  This fix
should have been a separate commit.  The reason is that I never
cherry-pick a "Run makelsr" commit into another branch since its changes
would not match the changes in the branch makelsr was originally run
(due to different state of snippets) but rather rerun makelsr myself.

But that means that any fixes not made in a _separate_ commit are not
cherry-picked either.

> If you look for 'makelsr' as a commit string you can see I've been
> doing this 'makelsr for previous commit xxx' as a separate always.

Sure, and that's perfectly fine.  The problem here was that you added
some change that was required to make makelsr work, into the _same_
commit.  A commit named "Run makelsr" should _only_ run makelsr, not add
any manual prerequisites to doing so successfully.  If the tree does not
permit running makelsr successfully, of course the tree does need to get
fixed first.  But that needs to be committed in an upfront commit,
before committing _only_ the makelsr action as one commit.

It actually turned out that the actual problem blocking a successful doc
build was yet a different one and I misdiagnosed it.  So I was really
lucky to be on the wrong track and be able to find and fix this problem
in the stable branch which otherwise would have kept under the radar and
caused problems later.

-- 
David Kastrup