git-gui: fix deleting item from all_remotes variable
PATCH 1/2 git-gui: fix deleting item from all_remotes variable
lsearch and lreplace both take the variable content as argument and not just their name.
--- lib/remote.tcl | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
lib/remote.tcl +2 -2
@@ -264,8 +264,8 @@ proc remove_remote {name} { unset repo_config(remote.$name.push) } - set i [lsearch -exact all_remotes $name] - lreplace all_remotes $i $i + set i [lsearch -exact $all_remotes $name] + set all_remotes [lreplace $all_remotes $i $i] set remote_m .mbar.remote delete_from_menu $remote_m.fetch $name
Signature · 1 line
1.7.4.34.gd2cb1
Quoted 21 lines · Heiko Voigt
lsearch and lreplace both take the variable content as argument and not just their name.
Signed-off-by Heiko Voigt <[email protected]>---
lib/remote.tcl | 4 ++--1 files changed, 2 insertions(+), 2 deletions(-)lib/remote.tcl +2
-2@@ -264,8 +264,8 @@ proc remove_remote {name} { unset repo_config(remote.$name.push) } - set i [lsearch -exact all_remotes $name] - lreplace all_remotes $i $i + set i [lsearch -exact $all_remotes $name] + set all_remotes [lreplace $all_remotes $i $i]
Quoted 6 lines
set
remote_m.mbar.remote
delete_from_menu $remote_m.fetch $name
--
1.7.4.34.gd2cb1
This fix is good and clearly resolves a bug in the tcl code -- however, what does it actually fix in the application? It looks like removing a remote works anyway even though this variable is not being updated.
Pat Thoyts
Hi Pat,
Quoted 4 lines · Pat Thoyts
This fix is good and clearly resolves a bug in the tcl code -- however, what does it actually fix in the application? It looks like removing a remote works anyway even though this variable is not being updated.
I do not know the other implications but I needed this fix for a patch I wrote. I did not send it because it is quite long and I wanted to wait until my other patches are ok so you do not have to review too much. But since you asked I will reply with the two patches to this email.
Cheers Heiko
PATCH 1/2 git-gui: refactor remote submenu creation into subroutine
--- lib/remote.tcl | 40 ++++++++++++++++++++++++---------------- 1 files changed, 24 insertions(+), 16 deletions(-)
lib/remote.tcl +24 -16
@@ -157,22 +157,7 @@ proc add_fetch_entry {r} { } if {$enable} { - if {![winfo exists $fetch_m]} { - menu $remove_m - $remote_m insert 0 cascade \ - -label [mc "Remove Remote"] \ - -menu $remove_m - - menu $prune_m - $remote_m insert 0 cascade \ - -label [mc "Prune from"] \ - -menu $prune_m - - menu $fetch_m - $remote_m insert 0 cascade \ - -label [mc "Fetch from"] \ - -menu $fetch_m - } + make_sure_remote_submenues_exist $remote_m $fetch_m add command \ -label $r \
@@ -222,6 +207,29 @@ proc add_push_entry {r} { } } +proc make_sure_remote_submenues_exist {remote_m} { + set fetch_m $remote_m.fetch + set prune_m $remote_m.prune + set remove_m $remote_m.remove + + if {![winfo exists $fetch_m]} { + menu $remove_m + $remote_m insert 0 cascade \ + -label [mc "Remove Remote"] \ + -menu $remove_m + + menu $prune_m + $remote_m insert 0 cascade \ + -label [mc "Prune from"] \ + -menu $prune_m + + menu $fetch_m + $remote_m insert 0 cascade \ + -label [mc "Fetch from"] \ + -menu $fetch_m + } +} + proc populate_remotes_menu {} { global all_remotes
Signature · 1 line
1.7.4.rc3.4.g155c4
RFC PATCH 2/2 git-gui: teach fetch/prune menu to do it for all remotes
In reply to Heiko VoigtThe commandline fetch already has this option for some time. Since this was not available at the time git gui was written lets implement it now.
---
It just came to my mind that I probably should implement a version check of the commandline to ensure that this option is available. Thats why I tagged only this patch with RFC.
Cheers Heiko
lib/remote.tcl | 45 +++++++++++++++++++++++++++++++++++++++++++++ lib/transport.tcl | 29 +++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 0 deletions(-)
lib/remote.tcl +45 -0
@@ -230,6 +230,45 @@ proc make_sure_remote_submenues_exist {remote_m} { } } +proc update_all_remotes_menu_entry {} { + global all_remotes + + set have_remote 0 + foreach r $all_remotes { + set have_remote 1 + } + + set remote_m .mbar.remote + set fetch_m $remote_m.fetch + set prune_m $remote_m.prune + if {$have_remote} { + make_sure_remote_submenues_exist $remote_m + if {[$fetch_m entrycget 0 -label] ne "All"} { + + $fetch_m insert 0 separator + $fetch_m insert 0 command \ + -label "All" \ + -command fetch_from_all + + $prune_m insert 0 separator + $prune_m insert 0 command \ + -label "All" \ + -command prune_from_all + } + } else { + if {[winfo exists $fetch_m]} { + if {[$fetch_m type end] eq "separator"} { + + delete_from_menu $fetch_m 0 + delete_from_menu $fetch_m 0 + + delete_from_menu $prune_m 0 + delete_from_menu $prune_m 0 + } + } + } +} + proc populate_remotes_menu {} { global all_remotes
@@ -237,6 +276,8 @@ proc populate_remotes_menu {} { add_fetch_entry $r add_push_entry $r } + + update_all_remotes_menu_entry } proc add_single_remote {name location} {
@@ -252,6 +293,8 @@ proc add_single_remote {name location} { add_fetch_entry $name add_push_entry $name + + update_all_remotes_menu_entry } proc delete_from_menu {menu name} {
@@ -281,4 +324,6 @@ proc remove_remote {name} { delete_from_menu $remote_m.remove $name # Not all remotes are in the push menu catch { delete_from_menu $remote_m.push $name } + + update_all_remotes_menu_entry }
lib/transport.tcl +29 -0
@@ -20,6 +20,35 @@ proc prune_from {remote} { console::exec $w [list git remote prune $remote] } +proc fetch_from_all {} { + set w [console::new \ + [mc "fetch all remotes"] \ + [mc "Fetching new changes from all remotes"]] + + set cmd [list git fetch --all] + if {[is_config_true gui.pruneduringfetch]} { + lappend cmd --prune + } + + console::exec $w $cmd +} + +proc prune_from_all {} { + global all_remotes + + set w [console::new \ + [mc "remote prune all remotes"] \ + [mc "Pruning tracking branches deleted from all remotes"]] + + set cmd [list git remote prune] + + foreach r $all_remotes { + lappend cmd $r + } + + console::exec $w $cmd +} + proc push_to {remote} { set w [console::new \ [mc "push %s" $remote] \
Signature · 1 line
1.7.4.rc3.4.g155c4
PATCH 1/1 git-gui: Include version check and test for tearoff menu entry
The --all option for git fetch was added in v1.6.6 so ensure we have a usable version before adding the menu items.
Sometimes people use tearoff menus and these offset the entry indices by one.
---
Quoted 5 lines · Heiko Voigt
It just came to my mind that I probably should implement a version check of the commandline to ensure that this option is available. Thats why I tagged only this patch with RFC.
Cheers Heiko
The posted patch seems fine except that an error is reported if tearoff menus are present. So this patch accommodates tearoff's. I looked up when the --all option was added (1.6.6) and skip adding the menu entry if we have an older version.
Seems to do the right thing.
lib/remote.tcl | 22 +++++++++++++--------- 1 files changed, 13 insertions(+), 9 deletions(-)
lib/remote.tcl +13 -9
@@ -233,6 +233,8 @@ proc make_sure_remote_submenues_exist {remote_m} { proc update_all_remotes_menu_entry {} { global all_remotes + if {[git-version < 1.6.6]} { return } + set have_remote 0 foreach r $all_remotes { set have_remote 1
@@ -243,27 +245,29 @@ proc update_all_remotes_menu_entry {} { set prune_m $remote_m.prune if {$have_remote} { make_sure_remote_submenues_exist $remote_m - if {[$fetch_m entrycget 0 -label] ne "All"} { + set index [expr {[$fetch_m type 0] eq "tearoff" ? 1 : 0}] + if {[$fetch_m entrycget $index -label] ne "All"} { - $fetch_m insert 0 separator - $fetch_m insert 0 command \ + $fetch_m insert $index separator + $fetch_m insert $index command \ -label "All" \ -command fetch_from_all - $prune_m insert 0 separator - $prune_m insert 0 command \ + $prune_m insert $index separator + $prune_m insert $index command \ -label "All" \ -command prune_from_all } } else { if {[winfo exists $fetch_m]} { + set index [expr {[$fetch_m type 0] eq "tearoff" ? 1 : 0}] if {[$fetch_m type end] eq "separator"} { - delete_from_menu $fetch_m 0 - delete_from_menu $fetch_m 0 + delete_from_menu $fetch_m $index + delete_from_menu $fetch_m $index - delete_from_menu $prune_m 0 - delete_from_menu $prune_m 0 + delete_from_menu $prune_m $index + delete_from_menu $prune_m $index } } }
Signature · 1 line
1.7.4.47.gb308bf
Re: [PATCH] git-gui: Include version check and test for tearoff menu entry
Hi,
Quoted 12 lines · Pat Thoyts
The --all option for git fetch was added in v1.6.6 so ensure we have a usable version before adding the menu items.
Sometimes people use tearoff menus and these offset the entry indices by one.Signed-off-by Pat Thoyts <[email protected]>---
Quoted 4 lines · Heiko Voigt
It just came to my mind that I probably should implement a version check of the commandline to ensure that this option is available. Thats why I tagged only this patch with RFC.
Quoted 8 lines
Quoted 1 line
Cheers Heiko
The posted patch seems fine except that an error is reported if tearoff menus are present. So this patch accommodates tearoff's. I looked up when the --all option was added (1.6.6) and skip adding the menu entry if we have an older version.
Seems to do the right thing.
Works and looks good to me as well. Did not know about tearoff menues how do you get those?
Cheers Heiko
P.S.: I discovered a whitespace issue in line 258 which came from my patch. Could you correct that?
Re: [PATCH] git-gui: Include version check and test for tearoff menu entry
Quoted 24 lines · Heiko Voigt
Hi,
Quoted 12 lines · Pat Thoyts
The --all option for git fetch was added in v1.6.6 so ensure we have a usable version before adding the menu items.
Sometimes people use tearoff menus and these offset the entry indices by one.Signed-off-by Pat Thoyts <[email protected]>---
Quoted 4 lines · Heiko Voigt
It just came to my mind that I probably should implement a version check of the commandline to ensure that this option is available. Thats why I tagged only this patch with RFC.
Quoted 8 lines
Quoted 1 line
Cheers Heiko
The posted patch seems fine except that an error is reported if tearoff menus are present. So this patch accommodates tearoff's. I looked up when the --all option was added (1.6.6) and skip adding the menu entry if we have an older version.
Seems to do the right thing.
Quoted 7 lines
Works and looks good to me as well. Did not know about tearoff menues how do you get those?
Cheers Heiko
P.S.: I discovered a whitespace issue in line 258 which came from my patch. Could you correct that?
Sure - squashed in.
The tearoff's appear by default on unix but are disabled on windows as they are not normal gui features on that platform. Search for *Menu.tearOff 0 in git-gui.sh. Unix users can disable these using the .Xresources file adding *Menu.tearOff: 0
Signature · 2 lines
Pat Thoyts http://www.patthoyts.tk/ PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
Re: Re: [PATCH] git-gui: Include version check and test for tearoff menu entry
Quoted 4 lines · Pat Thoyts
The tearoff's appear by default on unix but are disabled on windows as they are not normal gui features on that platform. Search for *Menu.tearOff 0 in
git-gui.sh. Unix users can disable these using the .Xresources file adding *Menu.tearOff: 0
Thanks for the enlightenment. They are visible on my linux box. I probably did never see them because I did not know they were there.
Cheers Heiko
Re: [RFC PATCH 2/2] git-gui: teach fetch/prune menu to do it for all remotes
In reply to Heiko VoigtAm 13.02.2011 14:57, schrieb Heiko Voigt:
Quoted 2 lines
The commandline fetch already has this option for some time. Since this was not available at the time git gui was written lets implement it now.
I really like this feature, I wanted to have that for quite some time!
After testing it, I noticed two minor things:
1) It would be nice if the new menu entry would only appear when there is more than one remote to fetch from.
2) I would rather like to see it at the *end* of the submenu, not at the beginning. Being used to always click on the first menu entry only to learn that the remote that used to be there got with something else is kind of surprising ;-)
What do others think?
PATCH 1/2 git-gui: fetch/prune all entry only for more than one entry
In case there is only one remote a fetch/prune all entry is redundant.
---
Quoted 2 lines · Jens Lehmann
1) It would be nice if the new menu entry would only appear when there is more than one remote to fetch from.
How about this? Disclaimer: Only superficially tested on OSX.
lib/remote.tcl | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
lib/remote.tcl +2 -2
@@ -237,13 +237,13 @@ proc update_all_remotes_menu_entry {} { set have_remote 0 foreach r $all_remotes { - set have_remote 1 + incr have_remote } set remote_m .mbar.remote set fetch_m $remote_m.fetch set prune_m $remote_m.prune - if {$have_remote} { + if {$have_remote > 1} { make_sure_remote_submenues_exist $remote_m set index [expr {[$fetch_m type 0] eq "tearoff" ? 1 : 0}] if {[$fetch_m entrycget $index -label] ne "All"} {
Signature · 1 line
1.7.4.1.30.gd0a3
Re: [PATCH 1/2] git-gui: fetch/prune all entry only for more than one entry
Quoted 25 lines · Heiko Voigt
In case there is only one remote a fetch/prune all entry is redundant.
Signed-off-by Heiko Voigt <[email protected]>---
Quoted 2 lines · Jens Lehmann
1) It would be nice if the new menu entry would only appear when there is more than one remote to fetch from.
How about this? Disclaimer: Only superficially tested on OSX.
lib/remote.tcl| 4 ++--1 files changed, 2 insertions(+), 2 deletions(-)lib/remote.tcl +1
-1@@ -237,13 +237,13 @@ proc update_all_remotes_menu_entry {} { set have_remote 0 foreach r $all_remotes { - set have_remote 1 + incr have_remote
lib/remote.tcl proc update_all_remotes_menu_entry {} { · line 249
Quoted 10 lines
lib/remote.tcl +1
-1} set remote_m .mbar.remote set fetch_m $remote_m.fetch set prune_m $remote_m.prune - if {$have_remote} { + if {$have_remote > 1} { make_sure_remote_submenues_exist $remote_m set index [expr {[$fetch_m type 0] eq "tearoff" ? 1 : 0}] if {[$fetch_m entrycget $index -label] ne "All"} {
This is fine - applied and checked it on Windows.
I'll add a Suggested-by from Jens as this was a response to his request.
Signature · 2 lines
Pat Thoyts http://www.patthoyts.tk/ PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
PATCH 2/2 git-gui: fetch/prune all entry appears last
In reply to Jens LehmannThe user might have got used to the order the remotes appeared previously. Lets add the all entry last so the all entry does not confuse previous users.
---
Quoted 4 lines · Jens Lehmann
2) I would rather like to see it at the *end* of the submenu, not at the beginning. Being used to always click on the first menu entry only to learn that the remote that used to be there got with something else is kind of surprising ;-)
And this? Disclaimer: Also only superficially tested on OSX.
lib/remote.tcl | 22 ++++++++++------------ 1 files changed, 10 insertions(+), 12 deletions(-)
lib/remote.tcl +10 -12
@@ -245,29 +245,27 @@ proc update_all_remotes_menu_entry {} { set prune_m $remote_m.prune if {$have_remote > 1} { make_sure_remote_submenues_exist $remote_m - set index [expr {[$fetch_m type 0] eq "tearoff" ? 1 : 0}] - if {[$fetch_m entrycget $index -label] ne "All"} { + if {[$fetch_m entrycget end -label] ne "All"} { - $fetch_m insert $index separator - $fetch_m insert $index command \ + $fetch_m insert end separator + $fetch_m insert end command \ -label "All" \ -command fetch_from_all - $prune_m insert $index separator - $prune_m insert $index command \ + $prune_m insert end separator + $prune_m insert end command \ -label "All" \ -command prune_from_all } } else { if {[winfo exists $fetch_m]} { - set index [expr {[$fetch_m type 0] eq "tearoff" ? 1 : 0}] - if {[$fetch_m type end] eq "separator"} { + if {[$fetch_m entrycget end -label] eq "All"} { - delete_from_menu $fetch_m $index - delete_from_menu $fetch_m $index + delete_from_menu $fetch_m end + delete_from_menu $fetch_m end - delete_from_menu $prune_m $index - delete_from_menu $prune_m $index + delete_from_menu $prune_m end + delete_from_menu $prune_m end } } }
Signature · 1 line
1.7.4.1.30.gd0a3
Re: [PATCH 2/2] git-gui: fetch/prune all entry appears last
Am 22.02.2011 20:30, schrieb Heiko Voigt:
Quoted 5 lines
The user might have got used to the order the remotes appeared previously. Lets add the all entry last so the all entry does not confuse previous users.
Signed-off-by Heiko Voigt <[email protected]>
I tested both patches under Linux, looks great now.
Re: [PATCH 2/2] git-gui: fetch/prune all entry appears last
In reply to Heiko VoigtQuoted 52 lines · Heiko Voigt
The user might have got used to the order the remotes appeared previously. Lets add the all entry last so the all entry does not confuse previous users.
Signed-off-by Heiko Voigt <[email protected]>---
Quoted 4 lines · Jens Lehmann
2) I would rather like to see it at the *end* of the submenu, not at the beginning. Being used to always click on the first menu entry only to learn that the remote that used to be there got with something else is kind of surprising ;-)
And this? Disclaimer: Also only superficially tested on OSX.
lib/remote.tcl| 22 ++++++++++------------1 files changed, 10 insertions(+), 12 deletions(-)lib/remote.tcl +6
-10@@ -245,29 +245,27 @@ proc update_all_remotes_menu_entry {} { set prune_m $remote_m.prune if {$have_remote > 1} { make_sure_remote_submenues_exist $remote_m - set index [expr {[$fetch_m type 0] eq "tearoff" ? 1 : 0}] - if {[$fetch_m entrycget $index -label] ne "All"} { + if {[$fetch_m entrycget end -label] ne "All"} { - $fetch_m insert $index separator - $fetch_m insert $index command \ + $fetch_m insert end separator + $fetch_m insert end command \ -label "All" \ -command fetch_from_all - $prune_m insert $index separator - $prune_m insert $index command \ + $prune_m insert end separator + $prune_m insert end command \ -label "All" \ -command prune_from_all } } else { if {[winfo exists $fetch_m]} { - set index [expr {[$fetch_m type 0] eq "tearoff" ? 1 : 0}] - if {[$fetch_m type end] eq "separator"} { + if {[$fetch_m entrycget end -label] eq "All"} { - delete_from_menu $fetch_m $index - delete_from_menu $fetch_m $index
lib/remote.tcl proc update_all_remotes_menu_entry {} { · line 271
Quoted 10 lines
lib/remote.tcl +4
-2+ delete_from_menu $fetch_m end + delete_from_menu $fetch_m end - delete_from_menu $prune_m $index - delete_from_menu $prune_m $index + delete_from_menu $prune_m end + delete_from_menu $prune_m end } } }
This is fine as well. Tested it on windows. Applied to master.
Signature · 2 lines
Pat Thoyts http://www.patthoyts.tk/ PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
Hi Pat,
Quoted 17 lines · Pat Thoyts
Quoted 16 lines · Heiko Voigt
lsearch and lreplace both take the variable content as argument and not just their name.
Signed-off-by Heiko Voigt <[email protected]>---
lib/remote.tcl | 4 ++--1 files changed, 2 insertions(+), 2 deletions(-)lib/remote.tcl +0
-0@@ -264,8 +264,8 @@ proc remove_remote {name} { unset repo_config(remote.$name.push) }
Quoted 4 lines
lib/remote.tcl proc remove_remote {name} { · line 268
Quoted 4 lines
lib/remote.tcl +2
-2- set i [lsearch -exact all_remotes $name] - lreplace all_remotes $i $i + set i [lsearch -exact $all_remotes $name] + set all_remotes [lreplace $all_remotes $i $i]
If you were going to please wait with applying it. I just found another location where this variable is changed in a wrong manner. I will update the patch accordingly.
Cheers Heiko
Hi Pat,
Quoted 3 lines · Heiko Voigt
If you were going to please wait with applying it. I just found another location where this variable is changed in a wrong manner. I will update the patch accordingly.
Please forget this comment. I mistakenly found an lappend call with the same usage pattern, but for lappend this is obviously correct.
Cheers Heiko