First patch: Issue 1275 (name+email for lily-git.tcl)

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

First patch: Issue 1275 (name+email for lily-git.tcl)

Owen Tuz
Hi -
  Having talked about C++ and Scheme, I went and picked one in tcl/tk instead. The original issue is here: http://code.google.com/p/lilypond/issues/detail?id=1275

It's a small patch which opens the user's global .gitconfig file and checks it for the words "name" and "email". If either one is not found, it opens a popup window asking for them and submits the response using the "git config" command.

It's fairly trivial but I'm assuming that the common practice for Frogs is to post it to the mailing list first? It all seems to work and I'm fairly sure the tcl part is fine but if someone who has some experience with tk could have a look at the code for the popup box in particular just to check it's not a horrible mess, that'd be great.

Thanks,
 Owen



0001-Checks-for-git-user-name-email.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Graham Percival
On Mon, Nov 15, 2010 at 6:30 PM, Owen Tuz <[hidden email]> wrote:
> Hi -
>   Having talked about C++ and Scheme, I went and picked one in tcl/tk
> instead. The original issue is here:
> http://code.google.com/p/lilypond/issues/detail?id=1275

I could kiss you.  I've been hoping for months -- actually, literally
years plural -- that somebody would look through the tracker, pick an
existing "frog" problem, and send in a patch without needing to
discuss it for 5 hours.  You have just made me very, very happy.  :)
(and if you haven't noticed already, that's not an easy thing to do!)

> It's a small patch which opens the user's global .gitconfig file and checks
> it for the words "name" and "email". If either one is not found, it opens a
> popup window asking for them and submits the response using the "git config"
> command.

I haven't tested it since I need to go to orchestra right now, and I
haven't looked at the other part of lily-git.tcl, but does your patch
follow the same indentation pattern?  Normally we have two spaces for
each {, or four spaces, or something like that.
If lily-git.tcl has such a pattern, please follow it.  If not, then
just ignore this paragraph.

> It's fairly trivial but I'm assuming that the common practice for Frogs is
> to post it to the mailing list first?

Yes, thank you.  A developer (possibly me) will push the patch for you
once it's been tested and accepted.

Cheers,
- Graham

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

J_ames
In reply to this post by Owen Tuz
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Owen Tuz
In reply to this post by Graham Percival

I could kiss you.  I've been hoping for months -- actually, literally
years plural -- that somebody would look through the tracker, pick an
existing "frog" problem, and send in a patch without needing to
discuss it for 5 hours.  You have just made me very, very happy.  :)
(and if you haven't noticed already, that's not an easy thing to do!)

I haven't tested it since I need to go to orchestra right now, and I
haven't looked at the other part of lily-git.tcl, but does your patch
follow the same indentation pattern?  Normally we have two spaces for
each {, or four spaces, or something like that.
If lily-git.tcl has such a pattern, please follow it.  If not, then
just ignore this paragraph.
 
Cheers,
- Graham

Hooray!

James

PS. I'll leave the all the kissing to Graham :)


Well, always nice to hear :)

The indentation doesn't seem to be completely standard throughout, but it's there in places so I've changed it on my machine. I'll wait until I hear back from someone about the rest of the code before I send that out (and if you don't mind, can we hold off on the kissing until then too?)

Thanks to both of you,
 Owen

Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Graham Percival
On Mon, Nov 15, 2010 at 07:28:45PM +0000, Owen Tuz wrote:
>    The indentation doesn't seem to be completely standard throughout,

Ick, you're right.  That's unfortunate.  :(

>    it's there in places so I've changed it on my machine.

Thanks!  Don't worry about indentation.  I'll auto-indent the file
once your patch is accepted.  If you hadn't done it already, I'd
tell you not to bother, but if it's there, no harm done.


> I'll wait until I hear back from someone about the rest of the
> code before I send that out

Hmm.  If .gitconfig doesn't exist, then line 66 causes
lily-git.tcl to bail.  I don't know if tcl does exceptions, or a
"try to open, and tell me if you failed" function, or what, but
I'm certain there's a way to handle this case.

>    (and if you don't mind, can we hold off on the kissing until then too?)

Hey, I said that I _could_ kiss you, not that I _would_ kiss you.
Some people say never to kiss on a first date, anyway.

Cheers,
- Graham

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Owen Tuz


Hmm.  If .gitconfig doesn't exist, then line 66 causes
lily-git.tcl to bail.  I don't know if tcl does exceptions, or a
"try to open, and tell me if you failed" function, or what, but
I'm certain there's a way to handle this case.

Cheers,
- Graham


Right, attached is a revised version (of the whole patch, not a patch to my first patch). Nothing special - since the script seems to be failing securely enough on its own I thought it would be safe to add code to check only for this specific problem and exit with a popup box telling the user exactly what was wrong.

Thanks,
 Owen

0001-Checks-for-git-user-name-email.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Graham Percival
On Mon, Nov 15, 2010 at 11:37:14PM +0000, Owen Tuz wrote:
>    Right, attached is a revised version (of the whole patch, not a patch to
>    my first patch).

This looks like a patch to be added after the previous patch was
(theoretically) applied.  We generally prefer to have a clear
patch for the entire thing.

> Nothing special - since the script seems to be failing
>    securely enough on its own I thought it would be safe to add code to check
>    only for this specific problem and exit with a popup box telling the user
>    exactly what was wrong.

.gitconfig won't be created automatically -- installing git just
puts it in a central location.  There's absolutely nothing wrong
with an installation (or a user :) if they have no .gitconfig
before running lily-git.tcl.

Take a look at the "file <option> <name>" command, and check the
result of that before trying to open the file.

Cheers,
- Graham

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Graham Percival
On Mon, Nov 15, 2010 at 11:44:08PM +0000, Graham Percival wrote:
> On Mon, Nov 15, 2010 at 11:37:14PM +0000, Owen Tuz wrote:
> >    Right, attached is a revised version (of the whole patch, not a patch to
> >    my first patch).
>
> This looks like a patch to be added after the previous patch was
> (theoretically) applied.  We generally prefer to have a clear
> patch for the entire thing.

BTW, if you're using lily-git.tcl yourself (which I heartily
recommend), then this is exactly what command 2b. is for.

Cheers,
- Graham

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Owen Tuz
In reply to this post by Graham Percival

This looks like a patch to be added after the previous patch was
(theoretically) applied.  We generally prefer to have a clear
patch for the entire thing.

.gitconfig won't be created automatically -- installing git just
puts it in a central location.  There's absolutely nothing wrong
with an installation (or a user :) if they have no .gitconfig
before running lily-git.tcl.

Take a look at the "file <option> <name>" command, and check the
result of that before trying to open the file.

Cheers,
- Graham

Ah, sorry about that. I tried to avoid exactly that but I'm still getting to know git.

I've kept the "file exists" check in there but it now controls the flag following the "open" command.

If .gitconfig isn't there, the flag a+ opens the file for reading and writing, and will create it if it doesn't exist. If .gitconfig exists, though, it starts access from the end of the file and the script won't find anything in it - hence the need to use a different flag.

That's the last one from me for tonight, thanks for your time checking all this for me! Sorry you still didn't manage to get away without the discussion. Maybe next time.

Cheers (and goodnight!),
Owen

0001-Checks-for-git-user-details.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Graham Percival
On Tue, Nov 16, 2010 at 01:25:19AM +0000, Owen Tuz wrote:
>    Ah, sorry about that. I tried to avoid exactly that but I'm still getting
>    to know git.

No problem.  The latest patch is good in that respect.

>    If .gitconfig isn't there, the flag a+ opens the file for reading and
>    writing, and will create it if it doesn't exist. If .gitconfig exists,
>    though, it starts access from the end of the file and the script won't
>    find anything in it - hence the need to use a different flag.

Ok.  However, now I have a problem with ttk::frame
----
Error in startup script: invalid command name "ttk::frame"
    while executing
"ttk::frame .b.c -padding "5 5 24 24""
    (procedure "requestuserdata" line 6)
    invoked from within
"requestuserdata"
    invoked from within
"if {![regexp "name" $usercheck] || ![regexp "email" $usercheck]}
then {
requestuserdata
tkwait window .b
    }"
    (file "scripts/auxiliar/lily-git.tcl" line 71)
----

I don't know much about tk, so I can't really judge which
librar(ies) I'm missing... but the goal of lily-git.tcl is maximum
compatibility (and ease of use), so any problem is a problem.  I'm
running a stock ubuntu 10.04, so we should expect a lot of
potential contributors to have the same setup as me.

The unpatched lily-git.tcl works on my system... I have no idea
what's going on here.  I suspect that you know more about tcl/tk
than me... any guesses?


>    That's the last one from me for tonight, thanks for your time checking all
>    this for me! Sorry you still didn't manage to get away without the
>    discussion. Maybe next time.

Well, you still produced a patch without needing discussion.  It's
expected that patches require discussion -- I was mainly happy
that you downloaded the source, found a problem, made a serious
attempt at fixing the problem, and produced a patch on your own.

Cheers,
- Graham

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

RE: First patch: Issue 1275 (name+email for lily-git.tcl)

Owen Tuz
Forgot to copy the frogs list in, sorry:

Ok.  However, now I have a problem with ttk::frame
----
Error in startup script: invalid command name "ttk::frame"
   while executing
"ttk::frame .b.c -padding "5 5 24 24""
   (procedure "requestuserdata" line 6)
   invoked from within
"requestuserdata"
   invoked from within
"if {![regexp "name" $usercheck] || ![regexp "email" $usercheck]}
then {
requestuserdata
tkwait window .b
   }"
   (file "scripts/auxiliar/lily-git.tcl" line 71)
----

I don't know much about tk, so I can't really judge which
librar(ies) I'm missing... but the goal of lily-git.tcl is maximum
compatibility (and ease of use), so any problem is a problem.  I'm
running a stock ubuntu 10.04, so we should expect a lot of
potential contributors to have the same setup as me.

The unpatched lily-git.tcl works on my system... I have no idea
what's going on here.  I suspect that you know more about tcl/tk
than me... any guesses?
 
Cheers,
- Graham

What I know about tcl/tk has basically been picked up from the wiki and man pages over the last couple of days... that said, it looks to me like the problem is backwards compatibility between tk 8.4 and 8.5, the latter of which was only released on September 8. I didn't realise but it seems the ttk::[widget] set is a new set of themed widgets introduced in 8.5.

I couldn't find much on tk 8.4 (which is odd), but I think this should do it. Would you be able to test the revised version for me using your 8.4 install?

Thanks,
 Owen




0001-Checks-for-git-user-details.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Graham Percival
On Wed, Nov 17, 2010 at 01:36:39AM +0000, Owen Tuz wrote:
>    I couldn't find much on tk 8.4 (which is odd), but I think this should do
>    it. Would you be able to test the revised version for me using your 8.4
>    install?

Yep, works well over here!  I'll upload it to Reitveld later
today.  :)

Cheers,
- Graham

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: First patch: Issue 1275 (name+email for lily-git.tcl)

Graham Percival
In reply to this post by Owen Tuz
Thanks, this has been pushed.

Cheers,
- Graham


On Wed, Nov 17, 2010 at 1:36 AM, Owen Tuz <[hidden email]> wrote:

> Forgot to copy the frogs list in, sorry:
>
>> Ok.  However, now I have a problem with ttk::frame
>> ----
>> Error in startup script: invalid command name "ttk::frame"
>>    while executing
>> "ttk::frame .b.c -padding "5 5 24 24""
>>    (procedure "requestuserdata" line 6)
>>    invoked from within
>> "requestuserdata"
>>    invoked from within
>> "if {![regexp "name" $usercheck] || ![regexp "email" $usercheck]}
>> then {
>> requestuserdata
>> tkwait window .b
>>    }"
>>    (file "scripts/auxiliar/lily-git.tcl" line 71)
>> ----
>>
>> I don't know much about tk, so I can't really judge which
>> librar(ies) I'm missing... but the goal of lily-git.tcl is maximum
>> compatibility (and ease of use), so any problem is a problem.  I'm
>> running a stock ubuntu 10.04, so we should expect a lot of
>> potential contributors to have the same setup as me.
>>
>> The unpatched lily-git.tcl works on my system... I have no idea
>> what's going on here.  I suspect that you know more about tcl/tk
>> than me... any guesses?
>
>
>>
>> Cheers,
>> - Graham
>
> What I know about tcl/tk has basically been picked up from the wiki and man
> pages over the last couple of days... that said, it looks to me like the
> problem is backwards compatibility between tk 8.4 and 8.5, the latter of
> which was only released on September 8. I didn't realise but it seems the
> ttk::[widget] set is a new set of themed widgets introduced in 8.5.
>
> I couldn't find much on tk 8.4 (which is odd), but I think this should do
> it. Would you be able to test the revised version for me using your 8.4
> install?
>
> Thanks,
>  Owen
>
>
>
>

---
----
Join the Frogs!