Patch for Issue #830

classic Classic list List threaded Threaded
41 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Patch for Issue #830

Marc Hohl
Hi all,

I have renamed the feta-*.mf files accordingly (see issue #830).
I did

make clean
make all

and didn't get any errors. Ok to apply?

Greetings

Marc

0001-Issue-830-renaming-mf-files.patch.gz (146K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Carl Sorensen



On 12/19/09 1:07 PM, "Marc Hohl" <[hidden email]> wrote:

> Hi all,
>
> I have renamed the feta-*.mf files accordingly (see issue #830).
> I did
>
> make clean
> make all
>
> and didn't get any errors. Ok to apply?

I will check this later today.

I think I'd like to recommend that in the future, instead of emailing
patches (with the associated problems with line endings), we just have Frogs
post them on Rietveld.

If, as a Frog, you don't want to expose your patch to the whole -devel
community, you can post your patch on Rietveld and only announce it to
Frogs.

Thanks,

Carl


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Neil Puttock
2009/12/20 Carl Sorensen <[hidden email]>:

> I think I'd like to recommend that in the future, instead of emailing
> patches (with the associated problems with line endings), we just have Frogs
> post them on Rietveld.

I'd also recommend at least checking a snippet runs properly (if not
doing a regression test check); an error-free run of make all
certainly isn't enough for such an invasive patch.

warning: time signature symbol `C44' not found; reverting to numbered style

Regards,
Neil

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Marc Hohl
In reply to this post by Carl Sorensen
Carl Sorensen schrieb:

>
> On 12/19/09 1:07 PM, "Marc Hohl" <[hidden email]> wrote:
>
>  
>> Hi all,
>>
>> I have renamed the feta-*.mf files accordingly (see issue #830).
>> I did
>>
>> make clean
>> make all
>>
>> and didn't get any errors. Ok to apply?
>>    
>
> I will check this later today.
>
> I think I'd like to recommend that in the future, instead of emailing
> patches (with the associated problems with line endings), we just have Frogs
> post them on Rietveld.
>
> If, as a Frog, you don't want to expose your patch to the whole -devel
> community, you can post your patch on Rietveld and only announce it to
> Frogs.
>  
I *did* send it to -devel, too, but the mail was rejected due to its size.
The frog list seems to be less strict...

Marc
> Thanks,
>
> Carl
>
>
>  


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Marc Hohl
In reply to this post by Neil Puttock
Neil Puttock schrieb:

> 2009/12/20 Carl Sorensen <[hidden email]>:
>
>  
>> I think I'd like to recommend that in the future, instead of emailing
>> patches (with the associated problems with line endings), we just have Frogs
>> post them on Rietveld.
>>    
>
> I'd also recommend at least checking a snippet runs properly (if not
> doing a regression test check); an error-free run of make all
> certainly isn't enough for such an invasive patch.
>
> warning: time signature symbol `C44' not found; reverting to numbered style
>  
Oops. I checked the metafont runs for possible breaks or typos
in the included files, but as it seems, I have overlooked something.
I'll try to find the error and do more tests before posting the
corrected patch.

Thanks

Marc

> Regards,
> Neil
>
>  


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Mark Polesky
Marc Hohl wrote:
> I have renamed the feta-*.mf files accordingly (see issue
> #830).  I did
>
> make clean
> make all
>
> and didn't get any errors. Ok to apply?

I don't see any problems looking at the patch, but that
doesn't mean there aren't any.  I'll let Neil or Carl weigh
in here.

One recommendation: every time you modify a source file, run
a macro or something to trim the trailing whitespaces
(applying this patch triggered some git warnings).  In this
case, it's clear that you didn't *add* them (they were in
the original files), but even so, patches will apply more
cleanly if you make sure to remove them.

What text editor are you using?

- Mark

**********

$ git apply 0001-Issue-830-renaming-mf-files.patch
[...]-files.patch:144: trailing whitespace.
%
[...]-files.patch:469: trailing whitespace.
        labels (5, 6, 7, 8);
[...]-files.patch:1080: trailing whitespace.
        set_char_box (0, 1.6 staff_space#,
[...]-files.patch:4256: trailing whitespace.

[...]-files.patch:4270: trailing whitespace.

warning: squelched 37 whitespace errors
warning: 42 lines add whitespace errors.


     

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Marc Hohl
Mark Polesky schrieb:

> Marc Hohl wrote:
>  
>> I have renamed the feta-*.mf files accordingly (see issue
>> #830).  I did
>>
>> make clean
>> make all
>>
>> and didn't get any errors. Ok to apply?
>>    
>
> I don't see any problems looking at the patch, but that
> doesn't mean there aren't any.  I'll let Neil or Carl weigh
> in here.
>  
I think I found the error, but my computer is doing 'make all' at the
moment,
so I have to wait for some tests.
> One recommendation: every time you modify a source file, run
> a macro or something to trim the trailing whitespaces
> (applying this patch triggered some git warnings).  In this
> case, it's clear that you didn't *add* them (they were in
> the original files), but even so, patches will apply more
> cleanly if you make sure to remove them.
>
> What text editor are you using?
>  
I use kate. I think there are some options about handling whitespaces,
I'll have a closer look at the preferences.

Thanks for the hint.

Marc

> - Mark
>
> **********
>
> $ git apply 0001-Issue-830-renaming-mf-files.patch
> [...]-files.patch:144: trailing whitespace.
> %
> [...]-files.patch:469: trailing whitespace.
>         labels (5, 6, 7, 8);
> [...]-files.patch:1080: trailing whitespace.
>         set_char_box (0, 1.6 staff_space#,
> [...]-files.patch:4256: trailing whitespace.
>
> [...]-files.patch:4270: trailing whitespace.
>
> warning: squelched 37 whitespace errors
> warning: 42 lines add whitespace errors.
>
>
>      
>
>  


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

RE: Patch for Issue #830

J_ames
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Graham Percival
There's some git command that removes whitespaces at the end of
lines.  There's also a tracker issue to find a command (ideally
with python, maybe?) that removes those whitespaces.  Either it's
a hard issue to solve, or nobody noticed it in the tracker.

Cheers,
- Graham


On Sun, Dec 20, 2009 at 08:57:32PM +0000, James Lowe wrote:

>    > One recommendation: every time you modify a source file, run
>    > a macro or something to trim the trailing whitespaces
>    As someone who wouldn't know what to run and who does get this
>    occasionally in doc patches I push, can someone give me some advice so I
>    don't irritate people with any future patches?'
>    
>    My edits are going to be mainly (only) on the help system (Learning Manual
>    etc) than writing code for functions, but any guidance would be helpful to
>    pre-squelch this stuff. I see that the lilycontrib.tcl does some - but I
>    think that is more a function of git when it makes the patch than anything
>    else.
>    
>    regards
>    
>    James
>
>      ----------------------------------------------------------------------
>
>    From: Marc Hohl
>    Sent: Sun 20/12/2009 20:53
>    To: Mark Polesky
>    Cc: Neil Puttock; Carl Sorensen; Lily-Devel List; [hidden email]
>    Subject: Re: [frogs] Patch for Issue #830
>
>  Mark Polesky schrieb:
>  > Marc Hohl wrote:
>  >  
>  >> I have renamed the feta-*.mf files accordingly (see issue
>  >> #830).  I did
>  >>
>  >> make clean
>  >> make all
>  >>
>  >> and didn't get any errors. Ok to apply?
>  >>    
>  >
>  > I don't see any problems looking at the patch, but that
>  > doesn't mean there aren't any.  I'll let Neil or Carl weigh
>  > in here.
>  >  
>  I think I found the error, but my computer is doing 'make all' at the
>  moment,
>  so I have to wait for some tests.
>  > One recommendation: every time you modify a source file, run
>  > a macro or something to trim the trailing whitespaces
>  > (applying this patch triggered some git warnings).  In this
>  > case, it's clear that you didn't *add* them (they were in
>  > the original files), but even so, patches will apply more
>  > cleanly if you make sure to remove them.
>  >
>  > What text editor are you using?
>  >  
>  I use kate. I think there are some options about handling whitespaces,
>  I'll have a closer look at the preferences.
>
>  Thanks for the hint.
>
>  Marc
>  > - Mark
>  >
>  > **********
>  >
>  > $ git apply 0001-Issue-830-renaming-mf-files.patch
>  > [...]-files.patch:144: trailing whitespace.
>  > %
>  > [...]-files.patch:469: trailing whitespace.
>  >         labels (5, 6, 7, 8);
>  > [...]-files.patch:1080: trailing whitespace.
>  >         set_char_box (0, 1.6 staff_space#,
>  > [...]-files.patch:4256: trailing whitespace.
>  >
>  > [...]-files.patch:4270: trailing whitespace.
>  >
>  > warning: squelched 37 whitespace errors
>  > warning: 42 lines add whitespace errors.
>  >
>  >
>  >      
>  >
>  >  
>
>
>  ---
>  ----
>  Join the Frogs!

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Corrected patch for Issue #830

Marc Hohl
In reply to this post by Marc Hohl
Marc Hohl schrieb:

> Mark Polesky schrieb:
>> Marc Hohl wrote:
>>  
>>> I have renamed the feta-*.mf files accordingly (see issue
>>> #830).  I did
>>>
>>> make clean
>>> make all
>>>
>>> and didn't get any errors. Ok to apply?
>>>    
>>
>> I don't see any problems looking at the patch, but that
>> doesn't mean there aren't any.  I'll let Neil or Carl weigh
>> in here.
>>  
> I think I found the error, but my computer is doing 'make all' at the
> moment,
> so I have to wait for some tests.
The error is gone; patch attached.

>> One recommendation: every time you modify a source file, run
>> a macro or something to trim the trailing whitespaces
>> (applying this patch triggered some git warnings).  In this
>> case, it's clear that you didn't *add* them (they were in
>> the original files), but even so, patches will apply more
>> cleanly if you make sure to remove them.
>>
>> What text editor are you using?
>>  
> I use kate. I think there are some options about handling whitespaces,
> I'll have a closer look at the preferences.
Hmm - I can switch the option "remove trailing whitespaces" on, but kate
doesn't seem
to recognize it properly - or I am doing something wrong here.
I corrected a few whitespaces manually, but as the are, well,
whitespaces, they are
not easily spotted ;-)

Yes, some python script would be great ...

Marc

>
> Thanks for the hint.
>
> Marc
>> - Mark
>>
>> **********
>>
>> $ git apply 0001-Issue-830-renaming-mf-files.patch
>> [...]-files.patch:144: trailing whitespace.
>> %
>> [...]-files.patch:469: trailing whitespace.
>>         labels (5, 6, 7, 8);
>> [...]-files.patch:1080: trailing whitespace.
>>         set_char_box (0, 1.6 staff_space#,
>> [...]-files.patch:4256: trailing whitespace.
>>
>> [...]-files.patch:4270: trailing whitespace.
>>
>> warning: squelched 37 whitespace errors
>> warning: 42 lines add whitespace errors.
>>
>>
>>      
>>  
>
>
> ---
> ----
> Join the Frogs!
>
>


Issue-830-renaming-mf-files.patch.gz (146K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Marc Hohl
In reply to this post by Graham Percival
Graham Percival schrieb:
> There's some git command that removes whitespaces at the end of
> lines.  There's also a tracker issue to find a command (ideally
> with python, maybe?) that removes those whitespaces.  Either it's
> a hard issue to solve, or nobody noticed it in the tracker.
>  
I found something useful here:
http://code.google.com/p/pymc/issues/detail?id=230

The program is simple but seems to work:

---
#!/usr/bin/env python
import sys

for fname in sys.argv[1:]:
    fd = open(fname,mode='U') # open in universal newline mode
    lines = []
    for line in fd.readlines():
        lines.append( line.rstrip() )
    fd.close()

    fd = open(fname,mode='w')
    fd.seek(0)
    for line in lines:
        fd.write(line+'\n')
    fd.close()
---

Marc

> Cheers,
> - Graham
>
>
> On Sun, Dec 20, 2009 at 08:57:32PM +0000, James Lowe wrote:
>  
>>    > One recommendation: every time you modify a source file, run
>>    > a macro or something to trim the trailing whitespaces
>>    As someone who wouldn't know what to run and who does get this
>>    occasionally in doc patches I push, can someone give me some advice so I
>>    don't irritate people with any future patches?'
>>    
>>    My edits are going to be mainly (only) on the help system (Learning Manual
>>    etc) than writing code for functions, but any guidance would be helpful to
>>    pre-squelch this stuff. I see that the lilycontrib.tcl does some - but I
>>    think that is more a function of git when it makes the patch than anything
>>    else.
>>    
>>    regards
>>    
>>    James
>>
>>      ----------------------------------------------------------------------
>>
>>    From: Marc Hohl
>>    Sent: Sun 20/12/2009 20:53
>>    To: Mark Polesky
>>    Cc: Neil Puttock; Carl Sorensen; Lily-Devel List; [hidden email]
>>    Subject: Re: [frogs] Patch for Issue #830
>>
>>  Mark Polesky schrieb:
>>  > Marc Hohl wrote:
>>  >  
>>  >> I have renamed the feta-*.mf files accordingly (see issue
>>  >> #830).  I did
>>  >>
>>  >> make clean
>>  >> make all
>>  >>
>>  >> and didn't get any errors. Ok to apply?
>>  >>    
>>  >
>>  > I don't see any problems looking at the patch, but that
>>  > doesn't mean there aren't any.  I'll let Neil or Carl weigh
>>  > in here.
>>  >  
>>  I think I found the error, but my computer is doing 'make all' at the
>>  moment,
>>  so I have to wait for some tests.
>>  > One recommendation: every time you modify a source file, run
>>  > a macro or something to trim the trailing whitespaces
>>  > (applying this patch triggered some git warnings).  In this
>>  > case, it's clear that you didn't *add* them (they were in
>>  > the original files), but even so, patches will apply more
>>  > cleanly if you make sure to remove them.
>>  >
>>  > What text editor are you using?
>>  >  
>>  I use kate. I think there are some options about handling whitespaces,
>>  I'll have a closer look at the preferences.
>>
>>  Thanks for the hint.
>>
>>  Marc
>>  > - Mark
>>  >
>>  > **********
>>  >
>>  > $ git apply 0001-Issue-830-renaming-mf-files.patch
>>  > [...]-files.patch:144: trailing whitespace.
>>  > %
>>  > [...]-files.patch:469: trailing whitespace.
>>  >         labels (5, 6, 7, 8);
>>  > [...]-files.patch:1080: trailing whitespace.
>>  >         set_char_box (0, 1.6 staff_space#,
>>  > [...]-files.patch:4256: trailing whitespace.
>>  >
>>  > [...]-files.patch:4270: trailing whitespace.
>>  >
>>  > warning: squelched 37 whitespace errors
>>  > warning: 42 lines add whitespace errors.
>>  >
>>  >
>>  >      
>>  >
>>  >  
>>
>>
>>  ---
>>  ----
>>  Join the Frogs!
>>    
>
> ---
> ----
> Join the Frogs!
>
>
>  


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Corrected patch for Issue #830

Neil Puttock
In reply to this post by Marc Hohl
2009/12/20 Marc Hohl <[hidden email]>:

> The error is gone; patch attached.

Nope, still there.

You can't change the names given in the macros
fet_begingroup/fet_endgroup, since they are part of the glyph-names;
for example, you've changed "timesig" to "timesignatures", which means
the glyph lookup for 2/2 and 4/4 breaks (it's looking for
"timesig.C22"/"timesig.C44").  The same problem occurs with pedals.

Cheers,
Neil

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Trevor D-2
In reply to this post by Graham Percival

Graham Percival wrote Sunday, December 20, 2009 9:01 PM


> There's some git command that removes whitespaces at the end of
> lines.

Yes, it's git rebase.  That has a --whitespace=fix option.
So if your mods are in branch mymods, say, just enter

git rebase --whitespace=fix master mymods

You'll normally want to rebase before you create a patch
anyway, so this is a handy way of removing whitespace.

Trevor



---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Graham Percival
On Sun, Dec 20, 2009 at 11:05:27PM -0000, Trevor Daniels wrote:
>
> git rebase --whitespace=fix master mymods

Can that be written as a universal preference?  and added to CG
1.1?  (I think it's that section; it might have moved)

Cheers,
- Graham

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Trevor D-2

Graham Percival wrote Sunday, December 20, 2009 11:13 PM


> On Sun, Dec 20, 2009 at 11:05:27PM -0000, Trevor Daniels wrote:
>>
>> git rebase --whitespace=fix master mymods
>
> Can that be written as a universal preference?  and added to CG
> 1.1?  (I think it's that section; it might have moved)
 
This is only useful for people who are comfortable
with git branches, and they are not mentioned until
1.4 - Advanced git stuff.  

Developers are certainly familiar with branches, as they
make code development so much easier and are essential
for uploading code to Reitveld.  Contributors generating
patches for documentation are probably not familiar
with git branches, so this will not help them.

Actually, I don't think rebase is mentioned in the CG.
I can't see it on a quick trawl.  It should be, as a
branch should be rebased before a patch is generated
from it if master has been updated since the branch
was created, else inadvertent reverts might be included.

I'll have a closer look tomorrow to see what should be
done.

Trevor


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Mark Polesky
Trevor Daniels wrote:
> Actually, I don't think rebase is mentioned in the CG.  I
> can't see it on a quick trawl.  It should be, as a branch
> should be rebased before a patch is generated from it if
> master has been updated since the branch was created, else
> inadvertent reverts might be included.
>
> I'll have a closer look tomorrow to see what should be
> done.

Trevor, you should know that I might be restructuring the CG
quite a bit in the coming days...

http://lists.gnu.org/archive/html/lilypond-devel/2009-12/msg00487.html

- Mark

p.s. why are all these replies coming in twice?


     

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Corrected patch for Issue #830

Marc Hohl
In reply to this post by Neil Puttock
Neil Puttock schrieb:
> 2009/12/20 Marc Hohl <[hidden email]>:
>
>  
>> The error is gone; patch attached.
>>    
>
> Nope, still there.
>  
Stupid me - I forgot to git add the changed files before
git  commit --amend, so the corrections worked fine, but only locally.
Here it is again.

Marc

> You can't change the names given in the macros
> fet_begingroup/fet_endgroup, since they are part of the glyph-names;
> for example, you've changed "timesig" to "timesignatures", which means
> the glyph lookup for 2/2 and 4/4 breaks (it's looking for
> "timesig.C22"/"timesig.C44").  The same problem occurs with pedals.
>
> Cheers,
> Neil
>
>  


Issue-830-renaming-mf-files.patch.gz (146K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Trevor D-2
In reply to this post by Mark Polesky

Mark Polesky wrote Monday, December 21, 2009 1:42 AM


> Trevor Daniels wrote:
>> Actually, I don't think rebase is mentioned in the CG.  I
>> can't see it on a quick trawl.  It should be, as a branch
>> should be rebased before a patch is generated from it if
>> master has been updated since the branch was created, else
>> inadvertent reverts might be included.
>>
>> I'll have a closer look tomorrow to see what should be
>> done.
>
> Trevor, you should know that I might be restructuring the CG
> quite a bit in the coming days...
>
> http://lists.gnu.org/archive/html/lilypond-devel/2009-12/msg00487.html

No problem; git handles multiple changes.  However
you should always update master and rebase your
branch before pushing it - that way, if there are any
conflicts, you will see them and can deal with them
before attempting to push.

> p.s. why are all these replies coming in twice?

Because they are sent to two lists and you are
subscribed to both.

Trevor



---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Corrected patch for Issue #830

Carl Sorensen
In reply to this post by Marc Hohl



On 12/20/09 11:58 PM, "Marc Hohl" <[hidden email]> wrote:

> Neil Puttock schrieb:
>> 2009/12/20 Marc Hohl <[hidden email]>:
>>
>>  
>>> The error is gone; patch attached.
>>>    
>>
>> Nope, still there.
>>  
> Stupid me - I forgot to git add the changed files before
> git  commit --amend, so the corrections worked fine, but only locally.
> Here it is again.

Marc, just do

git commit -a --amend

and you'll get it all done in one command.

HTH,

Carl



---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: Patch for Issue #830

Graham Percival
In reply to this post by Trevor D-2
On Mon, Dec 21, 2009 at 12:56:51AM -0000, Trevor Daniels wrote:

>
> Graham Percival wrote Sunday, December 20, 2009 11:13 PM
>
>>> git rebase --whitespace=fix master mymods
>>
>> Can that be written as a universal preference?  and added to CG
>> 1.1?  (I think it's that section; it might have moved)
>
> Developers are certainly familiar with branches, as they
> make code development so much easier and are essential
> for uploading code to Reitveld.

I beg your pardon, but *I'm* not familiar with branches, and I
apply more patches from other people than anybody else.  If git
can be configured to automatically fix whitespaces, then I'll
copy&paste the command and then stop pushing commits that add
whitespace errors.

I'd rather keep that command in the CG somewhere -- I agree that
it doesn't need to be CG 1.1, but maybe a "configuration for
advanced commands" or whatever -- because I periodically switch
computers, and if I just find the command now, I won't have it "on
hand" when I set up another computer.

Cheers,
- Graham

---
----
Join the Frogs!

123