Issue 5504092: Update lilygit.tcl (Issue 2092) (Closed)
|
Created:
13 years, 3 months ago by Carl Modified:
13 years, 1 month ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionUpdate lilygit.tcl (Issue 2092)
Makes lilygit.tcl respect the environment variable $LILYPOND_GIT.
If $LILYPOND_GIT is unset, default of $HOME/lilypond-git will be used.
Also does all working on dev/local_working branch, instead of master
Adds a Push Patch button to push patch to staging.
Patch Set 1 #Patch Set 2 : Make push button optional, add support for branches #Patch Set 3 : Base working branch on master #
Total comments: 1
Patch Set 4 : Added window to display git-log and confirm push #Patch Set 5 : Rebase to current staging #
Total comments: 1
Patch Set 6 : Fix problem with bad staging creation #Patch Set 7 : Make lily-git robust to missing branches #Patch Set 8 : Proper treatment of staging #Patch Set 9 : Rebase to current master #
Total comments: 12
Patch Set 10 : Respond to Janek's comments #
Total comments: 1
MessagesTotal messages: 22
Here's a stab at revising lily-git.tcl to facilitate patch pushing by git novices. Please review. Thanks, Carl
Sign in to reply to this message.
Thanks for working on this! Unfortunately I'm between cities at the moment; I'll try to comment more tomorrow evening. First thought: I'm a bit leery of adding a "push to staging", since: 1. that clutters up the interface. Sure, it's just one more button, but OTOH that's 25% more buttons. :) 2. it might confuse new contributors, who aren't allowed to push to staging. 3. anybody who's stayed around long enough to get git push ability shouldn't need it. (particularly once I get around to working on my cg patch again)
Sign in to reply to this message.
On 2011/12/29 06:45:53, Graham Percival wrote: > First thought: I'm a bit leery of adding a "push to staging", since: > 1. that clutters up the interface. Sure, it's just one more button, but OTOH > that's 25% more buttons. :) It would seem to me that this button need only appear when you have told the configuration (is there one?) that you are a developer.
Sign in to reply to this message.
On 2011/12/29 08:31:38, dak wrote: > > It would seem to me that this button need only appear when you have told the > configuration (is there one?) Not for lily-git.tcl specifically, but there _is_ something it checks wen you run it; for instance if you use it for the very first time it knows you have not done (effectively) git config --global user.name/user.email and prompts you to fill in those two entries by popping up a small GUI to fill in. So for push access lilygit could check for the 'ssh://' line in .git/config as, according to the CG, you have to set up for ssh with git manually anyway. So I am assuming that without this line in the .git/config you couldn't push even if you were an experienced dev. Then maybe the button could be greyed out (like they all are except 'update' the very first time you run it). Just a thought. James
Sign in to reply to this message.
2011/12/29 <pkx166h@gmail.com>: > On 2011/12/29 08:31:38, dak wrote: >> It would seem to me that this button need only appear when you have >> told the configuration (is there one?) > > Not for lily-git.tcl specifically, but there _is_ something it checks > wen you run it; for instance if you use it for the very first time it > knows you have not done (effectively) > > git config --global user.name/user.email > > and prompts you to fill in those two entries by popping up a small GUI > to fill in. So for push access lilygit could check for the 'ssh://' line > in .git/config as, according to the CG, you have to set up for ssh with > git manually anyway. So I am assuming that without this line in the > .git/config you couldn't push even if you were an experienced dev. Then > maybe the button could be greyed out (like they all are except 'update' > the very first time you run it). +1
Sign in to reply to this message.
Graham, can you ping me when changes to lily-git.tcl are finalized and I'll get new version into lilydev-remix image. Thanks, Jon 2011/12/29 Janek Warchoł <janek.lilypond@gmail.com>: > 2011/12/29 <pkx166h@gmail.com>: >> On 2011/12/29 08:31:38, dak wrote: >>> It would seem to me that this button need only appear when you have >>> told the configuration (is there one?) >> >> Not for lily-git.tcl specifically, but there _is_ something it checks >> wen you run it; for instance if you use it for the very first time it >> knows you have not done (effectively) >> >> git config --global user.name/user.email >> >> and prompts you to fill in those two entries by popping up a small GUI >> to fill in. So for push access lilygit could check for the 'ssh://' line >> in .git/config as, according to the CG, you have to set up for ssh with >> git manually anyway. So I am assuming that without this line in the >> .git/config you couldn't push even if you were an experienced dev. Then >> maybe the button could be greyed out (like they all are except 'update' >> the very first time you run it). > > +1 > > _______________________________________________ > lilypond-devel mailing list > lilypond-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/lilypond-devel -- Jonathan Kulp http://jonathankulp.org
Sign in to reply to this message.
I've uploaded a new patch set. The Push button is disabled by default; an experienced user can enable it with a simple edit to the script. I couldn't find a good value that I believed would work for automatic detection, since my push-enabled configuration is different from that listed in the CG. I've also added support for an environment variable specifying the branch. This will allow lily-git users to work with more than one patch simultaneously by doing LILYPOND_BRANCH=dev/fix-my-issue lilypond-git.tcl and they will work on that branch. If the branch doesn't exist it will be created off of staging.
Sign in to reply to this message.
On Thu, Dec 29, 2011 at 05:21:34PM +0000, Carl.D.Sorensen@gmail.com wrote: > The Push button is disabled by default; an experienced user can enable > it with a simple edit to the script. By "disabled", do you mean "commented out" ? If new contributors see a greyed-out button, they'll get confused and ask silly questions. The UI should be designed to avoid silly questions[1]. [1] actually, in this case it would be a sensible question rather than silly -- so IMO it's an even better idea to change the UI if it gives misleading suggestions. > I've also added support for an environment variable specifying the > branch. This will allow lily-git users to work with more than one patch > simultaneously by doing > > LILYPOND_BRANCH=dev/fix-my-issue lilypond-git.tcl hmm. I'll come back to this later. > and they will work on that branch. If the branch doesn't exist it > will be created off of staging. This is definitely not right; please branch off of master. We have absolutely no guarantee that staging will compile the binary, let alone the documentation. Cheers, - Graham
Sign in to reply to this message.
On 2011/12/29 18:19:26, Graham Percival wrote: > On Thu, Dec 29, 2011 at 05:21:34PM +0000, mailto:Carl.D.Sorensen@gmail.com wrote: > > The Push button is disabled by default; an experienced user can enable > > it with a simple edit to the script. > > By "disabled", do you mean "commented out" ? If new contributors > see a greyed-out button, they'll get confused and ask silly > questions. The UI should be designed to avoid silly questions[1]. > On 2011/12/29 18:19:26, Graham Percival wrote: > On Thu, Dec 29, 2011 at 05:21:34PM +0000, mailto:Carl.D.Sorensen@gmail.com wrote: > > The Push button is disabled by default; an experienced user can enable > > it with a simple edit to the script. > > By "disabled", do you mean "commented out" ? If new contributors > see a greyed-out button, they'll get confused and ask silly > questions. The UI should be designed to avoid silly questions[1]. By disabled, I mean it doesn't appear. It isn't commented out, but it's not put into the user interface unless push_access is set to 1. > > I've also added support for an environment variable specifying the > > branch. This will allow lily-git users to work with more than one patch > > simultaneously by doing > > > > LILYPOND_BRANCH=dev/fix-my-issue lilypond-git.tcl > > hmm. I'll come back to this later. If this is not used, the branch defaults to dev/local_working. The environment variable is not needed and can easily be ignored by novices. > > This is definitely not right; please branch off of master. We > have absolutely no guarantee that staging will compile the binary, > let alone the documentation. I guess that is true. In my work, I always branch off staging, because that is where I will have to push it. So I can see that the working branch should be off master, and then when pushing we should rebase off staging. i will fix that. Thanks for the review. Carl
Sign in to reply to this message.
LGTM apart from one detail http://codereview.appspot.com/5504092/diff/5001/scripts/auxiliar/lily-git.tcl File scripts/auxiliar/lily-git.tcl (right): http://codereview.appspot.com/5504092/diff/5001/scripts/auxiliar/lily-git.tcl... scripts/auxiliar/lily-git.tcl:295: git push origin HEAD:$pushHead I'm still concerned about this type of automatic pushing. The revised CG material on branches http://codereview.appspot.com/5484043/ makes a bit deal about always checking gitk -- just for 5 seconds -- before pushing, and I think that's a good step. Patchy will not question any ridiculous git history that arises due to any kind of weird series of commands in git.
Sign in to reply to this message.
On 2011/12/30 20:57:02, Graham Percival wrote: > Patchy will not question any > ridiculous git history that arises due to any kind of weird series of commands > in git. Maybe it would make sense if Patchy refused fast forwarding over a history involving a merge _from_ staging. I think that merges should just be from master; a merge from staging implies a merging pull, and I can't think of a situation where we would want to see those in the history of master.
Sign in to reply to this message.
On 2011/12/30 20:57:02, Graham Percival wrote: > LGTM apart from one detail > > > http://codereview.appspot.com/5504092/diff/5001/scripts/auxiliar/lily-git.tcl... > scripts/auxiliar/lily-git.tcl:295: git push origin HEAD:$pushHead > I'm still concerned about this type of automatic pushing. The revised CG > material on branches > http://codereview.appspot.com/5484043/ > makes a bit deal about always checking gitk -- just for 5 seconds -- before > pushing, and I think that's a good step. Patchy will not question any I think I can achieve that automatically by looking at the commit log between staging and working. If there are no merge commits in that log, we are good to push IIUC. I ought to be able to check for that automatically and only do the push if there are no problems. I'll try to cook a revised patch in a bit. Thanks for the review. Carl
Sign in to reply to this message.
On Sat, Dec 31, 2011 at 12:10:14AM +0000, Carl.D.Sorensen@gmail.com wrote: > On 2011/12/30 20:57:02, Graham Percival wrote: > >I'm still concerned about this type of automatic pushing. The revised > CG > >material on branches > > http://codereview.appspot.com/5484043/ > >makes a bit deal about always checking gitk -- just for 5 seconds -- > before > >pushing, and I think that's a good step. Patchy will not question any > > I think I can achieve that automatically by looking at the commit log > between staging and working. If there are no merge commits in that log, > we are good to push IIUC. Hmm. Could I interest you in adding some checks to Patchy? I'd rather have that complexity in Patchy rather than lily-git.tcl -- that way, it doesn't matter if anything weird happens on a command-line or within lily-git; we're still protected. Cheers, - Graham
Sign in to reply to this message.
The problem is not merges. The problem is unintentional merges. merges _from_ staging (or origin/staging) are usually a mistake.
Sign in to reply to this message.
http://codereview.appspot.com/5504092/diff/8001/scripts/auxiliar/lily-git.tcl File scripts/auxiliar/lily-git.tcl (right): http://codereview.appspot.com/5504092/diff/8001/scripts/auxiliar/lily-git.tcl... scripts/auxiliar/lily-git.tcl:13: set push_access 1 This must be set to 0.
Sign in to reply to this message.
After commenting out the "git push" line, and also disconnecting my network cable, then going to "push patch", I get this: fatal: ambiguous argument 'staging..dev/local_working': unknown revision or path not in the working tree. Use '--' to separate paths from revisions Look, could we just treat this as three separate stages? 1. update lily-git.tcl to use $LILYPOND_GIT and use dev/local-staging. 2. get the git instructions for the command-line nailed down (issue 2100). 3. if it's still relevant, work on adding push ability to lily-git.tcl. Having a really good set of instructions in step 2 will make #3 easier -- and there's no point delaying #1 for another few weeks when AFAIK that part is perfect.
Sign in to reply to this message.
On 2012/01/14 09:31:39, Graham Percival wrote: > Look, could we just treat this as three separate stages? > 1. update lily-git.tcl to use $LILYPOND_GIT and use dev/local-staging. > 2. get the git instructions for the command-line nailed down (issue 2100). > 3. if it's still relevant, work on adding push ability to lily-git.tcl. We could, but I don't think we need to. Your problem was that your repository was missing staging, and I didn't have it being created automatically. It is now created properly and automatically. So 1 and 3 are done with this patch (and 3 can be easily disabled by leaving push_access at 0). I will get 2 done today. Thanks, Carl
Sign in to reply to this message.
New version of lilygit.tcl that: 1. Treats staging properly -- i.e. does not create a local staging branch 2. Does rebase before pushing patch on a detached head (as now described in cg patch) 3. Has an environment variable setting push access, so no editing of the file is necessary (which makes for easy updates) Please review
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Some questions and concerns. thanks, Janek http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tcl File scripts/auxiliar/lily-git.tcl (right): http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:222: proc update_lilypond_norebase {} { this isn't used anywhere, we keep it just in case? http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:252: if {$workingBranch != ""} { Do i understand correctly that we check workingBranch and switch to working on it if it's not empty? So, if we don't want to work on workingBranch, we set it to null and lily-git will remain on OriginHead (i.e. master)? http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:253: add_working_branch Won't this reset our workingBranch to be a copy of master every time we update repository? Wait, i see: it's done only when creating new repository? http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:270: git rebase origin/$originHead shouldn't we update local master too? If we don't do this, how can we not run into trouble in line 309? http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:272: git merge origin/$originHead Shouldn't we checkout workingBranch first (after checking if it exists)? http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:309: git rebase $originHead isn't there a risk that master is less up-to-date than working-branch? (see comment on line 270) http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:560: if [catch {set workingBranch $env(LILYPOND_BRANCH)}] { does this mean "it there's no external variable to control workingBranch"?
Sign in to reply to this message.
Nice review. I will make changes in response to your comments. Thanks, Carl http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tcl File scripts/auxiliar/lily-git.tcl (right): http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:222: proc update_lilypond_norebase {} { On 2012/01/21 21:12:58, janek wrote: > this isn't used anywhere, we keep it just in case? Looks like it can go. Thanks for the catch. http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:252: if {$workingBranch != ""} { On 2012/01/21 21:12:58, janek wrote: > Do i understand correctly that we check workingBranch and switch to working on > it if it's not empty? So, if we don't want to work on workingBranch, we set it > to null and lily-git will remain on OriginHead (i.e. master)? No. workingBranch should never be null. If it is, we'll get lots of git errors. So if workingBranch is null, we don't do any checking out. No. $workingBranch should never be null. If you try to ch http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:253: add_working_branch On 2012/01/21 21:12:58, janek wrote: > Won't this reset our workingBranch to be a copy of master every time we update > repository? > Wait, i see: it's done only when creating new repository? Precisely. http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:270: git rebase origin/$originHead On 2012/01/21 21:12:58, janek wrote: > shouldn't we update local master too? > If we don't do this, how can we not run into trouble in line 309? Ahh. I see what you mean. Before we were only rebasing master to origin/master. Then we changed to rebasing workingBranch to origin/master. We should be rebasing master to origin/master, and then workingBranch to master. Thanks for the catch. http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:560: if [catch {set workingBranch $env(LILYPOND_BRANCH)}] { On 2012/01/21 21:12:58, janek wrote: > does this mean "it there's no external variable to control workingBranch"? Yes. Strictly speaking it means "If there is an error in setting the value of the local variable workingBranch to the environment variable LILYPOND_BRANCH".
Sign in to reply to this message.
LGTM http://codereview.appspot.com/5504092/diff/20001/scripts/auxiliar/lily-git.tcl File scripts/auxiliar/lily-git.tcl (right): http://codereview.appspot.com/5504092/diff/20001/scripts/auxiliar/lily-git.tc... scripts/auxiliar/lily-git.tcl:248: git remote add -t $originHead \ This should change to git clone, but that can happen in a later edit. Let's get this first change pushed.
Sign in to reply to this message.
|
