{"thread":{"id":"52115","subject":"[PATCH 0/1] git-gui: remove unused global declarations","startedAt":"2019-10-25T01:33:02Z","lastAt":"2019-10-29T21:32:09Z","messageCount":6,"participants":["Pratyush Yadav","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"384821","messageId":"20191025013255.7367-1-me@yadavpratyush.com","threadId":"52115","inReplyTo":null,"subject":"[PATCH 0/1] git-gui: remove unused global declarations","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-25T01:32:54Z","receivedAt":"2019-10-25T01:33:02Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"A lot of places have unused global variables declared. Remove them.\n\nIt started as a couple of quick fixes and ended up in me writing an ugly\nhacky script to catch all the instances of unused global declarations.\nLot more than I expected.\n\nThe script can be found at [0]. The script at [0] will only catch the\nunused globals in 'proc's. But git-gui also has 'method's and\n'constructor's. Just change '^proc' to '^constructor' or '^method'.\n\nI manually checked each instance before removing just to be sure I'm not\ndoing something wrong. Still, a bit of testing would be appreciated.\nJust apply this patch and go on about your business as usual. There\n_should_ be no change in the behaviour. I tested some basic functions\nlike commit, push, etc, and they work fine for me.\n\n[0] https://gist.github.com/prati0100/0f3ef903ad1658e76ea0b95f001b4865\n\nPratyush Yadav (1):\n  git-gui: remove unused global declarations\n\n git-gui.sh                | 12 ++++--------\n lib/about.tcl             |  2 +-\n lib/blame.tcl             |  3 +--\n lib/branch_checkout.tcl   |  2 +-\n lib/branch_create.tcl     |  1 -\n lib/branch_delete.tcl     |  2 +-\n lib/browser.tcl           |  2 +-\n lib/checkout_op.tcl       |  4 +---\n lib/choose_font.tcl       |  2 +-\n lib/choose_repository.tcl |  6 +++---\n lib/class.tcl             |  1 -\n lib/commit.tcl            | 10 +++-------\n lib/console.tcl           |  2 +-\n lib/database.tcl          |  2 +-\n lib/diff.tcl              | 21 +++++++--------------\n lib/error.tcl             |  2 +-\n lib/index.tcl             |  9 ++++-----\n lib/line.tcl              |  2 +-\n lib/merge.tcl             |  5 ++---\n lib/mergetool.tcl         | 10 +++++-----\n lib/remote_add.tcl        |  5 ++---\n lib/search.tcl            |  4 ++--\n lib/sshkey.tcl            |  2 +-\n lib/tools_dlg.tcl         |  6 +++---\n 24 files changed, 47 insertions(+), 70 deletions(-)\n\n--\n2.21.0\n\n"},{"id":"384822","messageId":"20191025013255.7367-2-me@yadavpratyush.com","threadId":"52115","inReplyTo":"20191025013255.7367-1-me@yadavpratyush.com","subject":"[PATCH 1/1] git-gui: remove unused global declarations","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-25T01:32:55Z","receivedAt":"2019-10-25T01:33:03Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"A lot of places have unused global variables declared. Remove them.\n\nThe instances were found using a script, but I checked each change\nmanually just to be sure. And in the unlikely case something _does_ go\nwrong, it would just probably be caught as an \"undeclared variable\"\nerror. FWIW, quick testing shows the commonly used features like commit,\npush, branch work fine.\n\nWhile cleaning up, I also moved the remaining declarations around to\navoid a bunch of short lines with only one or two declarations each.\n\nSigned-off-by: Pratyush Yadav <me@yadavpratyush.com>\n---\n git-gui.sh                | 12 ++++--------\n lib/about.tcl             |  2 +-\n lib/blame.tcl             |  3 +--\n lib/branch_checkout.tcl   |  2 +-\n lib/branch_create.tcl     |  1 -\n lib/branch_delete.tcl     |  2 +-\n lib/browser.tcl           |  2 +-\n lib/checkout_op.tcl       |  4 +---\n lib/choose_font.tcl       |  2 +-\n lib/choose_repository.tcl |  6 +++---\n lib/class.tcl             |  1 -\n lib/commit.tcl            | 10 +++-------\n lib/console.tcl           |  2 +-\n lib/database.tcl          |  2 +-\n lib/diff.tcl              | 21 +++++++--------------\n lib/error.tcl             |  2 +-\n lib/index.tcl             |  9 ++++-----\n lib/line.tcl              |  2 +-\n lib/merge.tcl             |  5 ++---\n lib/mergetool.tcl         | 10 +++++-----\n lib/remote_add.tcl        |  5 ++---\n lib/search.tcl            |  4 ++--\n lib/sshkey.tcl            |  2 +-\n lib/tools_dlg.tcl         |  6 +++---\n 24 files changed, 47 insertions(+), 70 deletions(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex 0d21f56..c5d7af6 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -501,7 +501,6 @@ proc is_shellscript {filename} {\n # contain a command with arguments. On windows we must check for shell\n # scripts specifically otherwise just call the filter command.\n proc open_cmd_pipe {cmd path} {\n-\tglobal env\n \tif {![file executable [shellpath]]} {\n \t\tset exe [auto_execok [lindex $cmd 0]]\n \t\tif {[is_shellscript [lindex $exe 0]]} {\n@@ -1396,7 +1395,6 @@ proc unlock_index {} {\n ## status\n \n proc repository_state {ctvar hdvar mhvar} {\n-\tglobal current_branch\n \tupvar $ctvar ct $hdvar hd $mhvar mh\n \n \tset mh [list]\n@@ -1450,8 +1448,7 @@ proc force_amend {} {\n }\n \n proc rescan {after {honor_trustmtime 1}} {\n-\tglobal HEAD PARENT MERGE_HEAD commit_type\n-\tglobal ui_index ui_workdir ui_comm\n+\tglobal HEAD PARENT MERGE_HEAD commit_type ui_comm\n \tglobal rescan_active file_states\n \tglobal repo_config\n \n@@ -1741,7 +1738,6 @@ proc read_ls_others {fd after} {\n \n proc rescan_done {fd buf after} {\n \tglobal rescan_active current_diff_path\n-\tglobal file_states repo_config\n \tupvar $buf to_clear\n \n \tif {![eof $fd]} return\n@@ -2370,7 +2366,7 @@ proc do_commit {} {\n }\n \n proc next_diff {{after {}}} {\n-\tglobal next_diff_p next_diff_w next_diff_i\n+\tglobal next_diff_p next_diff_w\n \tshow_diff $next_diff_p $next_diff_w {} {} $after\n }\n \n@@ -2411,7 +2407,7 @@ proc find_file_from {flist idx delta path mmask} {\n \n proc find_next_diff {w path {lno {}} {mmask {}}} {\n \tglobal next_diff_p next_diff_w next_diff_i\n-\tglobal file_lists ui_index ui_workdir\n+\tglobal file_lists ui_index\n \n \tset flist $file_lists($w)\n \tif {$lno eq {}} {\n@@ -2495,7 +2491,7 @@ proc force_first_diff {after} {\n }\n \n proc toggle_or_diff {mode w args} {\n-\tglobal file_states file_lists current_diff_path ui_index ui_workdir\n+\tglobal file_states file_lists ui_index ui_workdir\n \tglobal last_clicked selected_paths file_lists_last_clicked\n \n \tif {$mode eq \"click\"} {\ndiff --git a/lib/about.tcl b/lib/about.tcl\nindex cfa50fc..d519890 100644\n--- a/lib/about.tcl\n+++ b/lib/about.tcl\n@@ -4,7 +4,7 @@\n proc do_about {} {\n \tglobal appvers copyright oguilib\n \tglobal tcl_patchLevel tk_patchLevel\n-\tglobal ui_comm_spell NS use_ttk\n+\tglobal ui_comm_spell NS\n \n \tset w .about_dialog\n \tDialog $w\ndiff --git a/lib/blame.tcl b/lib/blame.tcl\nindex a1aeb8b..919affb 100644\n--- a/lib/blame.tcl\n+++ b/lib/blame.tcl\n@@ -62,7 +62,7 @@ field tooltip_timer     {} ; # Current timer event for our tooltip\n field tooltip_commit    {} ; # Commit(s) in tooltip\n \n constructor new {i_commit i_path i_jump} {\n-\tglobal cursor_ptr M1B M1T have_tk85 use_ttk NS\n+\tglobal cursor_ptr have_tk85 NS\n \tvariable active_color\n \tvariable group_colors\n \n@@ -921,7 +921,6 @@ method _load_new_commit {new_commit new_path jump} {\n }\n \n method _showcommit {cur_w lno} {\n-\tglobal repo_config\n \tvariable active_color\n \n \tif {$highlight_commit ne {}} {\ndiff --git a/lib/branch_checkout.tcl b/lib/branch_checkout.tcl\nindex d06037d..f5d2a0a 100644\n--- a/lib/branch_checkout.tcl\n+++ b/lib/branch_checkout.tcl\n@@ -10,7 +10,7 @@ field opt_fetch     1; # refetch tracking branch if used?\n field opt_detach    0; # force a detached head case?\n \n constructor dialog {} {\n-\tglobal use_ttk NS\n+\tglobal NS\n \tmake_dialog top w\n \twm withdraw $w\n \twm title $top [mc \"%s (%s): Checkout Branch\" [appname] [reponame]]\ndiff --git a/lib/branch_create.tcl b/lib/branch_create.tcl\nindex ba367d5..446248d 100644\n--- a/lib/branch_create.tcl\n+++ b/lib/branch_create.tcl\n@@ -115,7 +115,6 @@ constructor dialog {} {\n \n method _create {} {\n \tglobal repo_config\n-\tglobal M1B\n \n \tset spec [$w_rev get_tracking_branch]\n \tswitch -- $name_type {\ndiff --git a/lib/branch_delete.tcl b/lib/branch_delete.tcl\nindex a505163..2bf52db 100644\n--- a/lib/branch_delete.tcl\n+++ b/lib/branch_delete.tcl\n@@ -9,7 +9,7 @@ field w_check         ; # revision picker for merge test\n field w_delete        ; # delete button\n \n constructor dialog {} {\n-\tglobal current_branch use_ttk NS\n+\tglobal current_branch NS\n \n \tmake_dialog top w\n \twm withdraw $w\ndiff --git a/lib/browser.tcl b/lib/browser.tcl\nindex a982983..ff7e772 100644\n--- a/lib/browser.tcl\n+++ b/lib/browser.tcl\n@@ -269,7 +269,7 @@ field w              ; # widget path\n field w_rev          ; # mega-widget to pick the initial revision\n \n constructor dialog {} {\n-\tglobal use_ttk NS\n+\tglobal NS\n \tmake_dialog top w\n \twm withdraw $top\n \twm title $top [mc \"%s (%s): Browse Branch Files\" [appname] [reponame]]\ndiff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl\nindex a522829..0fabc7d 100644\n--- a/lib/checkout_op.tcl\n+++ b/lib/checkout_op.tcl\n@@ -389,9 +389,7 @@ $err\n }\n \n method _after_readtree {} {\n-\tglobal commit_type HEAD MERGE_HEAD PARENT\n-\tglobal current_branch is_detached\n-\tglobal ui_comm\n+\tglobal HEAD current_branch is_detached\n \n \tset name [_name $this]\n \tset log \"checkout: moving\"\ndiff --git a/lib/choose_font.tcl b/lib/choose_font.tcl\nindex ebe50bd..bd2fe91 100644\n--- a/lib/choose_font.tcl\n+++ b/lib/choose_font.tcl\n@@ -17,7 +17,7 @@ variable all_families [list]  ; # All fonts known to Tk\n \n constructor pick {path title a_family a_size} {\n \tvariable all_families\n-\tglobal use_ttk NS\n+\tglobal NS\n \n \tset v_family $a_family\n \tset v_size $a_size\ndiff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl\nindex 80f5a59..4450b6f 100644\n--- a/lib/choose_repository.tcl\n+++ b/lib/choose_repository.tcl\n@@ -23,7 +23,7 @@ field readtree_err        ; # Error output from read-tree (if any)\n field sorted_recent       ; # recent repositories (sorted)\n \n constructor pick {} {\n-\tglobal M1T M1B use_ttk NS\n+\tglobal M1T M1B NS\n \n \tif {[set maxrecent [get_config gui.maxrecentrepo]] eq {}} {\n \t\tset maxrecent 10\n@@ -403,7 +403,7 @@ proc _objdir {path} {\n ## Create New Repository\n \n method _do_new {} {\n-\tglobal use_ttk NS\n+\tglobal NS\n \t$w_next conf \\\n \t\t-state disabled \\\n \t\t-command [cb _do_new2] \\\n@@ -487,7 +487,7 @@ proc _new_ok {p} {\n ## Clone Existing Repository\n \n method _do_clone {} {\n-\tglobal use_ttk NS\n+\tglobal NS\n \t$w_next conf \\\n \t\t-state disabled \\\n \t\t-command [cb _do_clone2] \\\ndiff --git a/lib/class.tcl b/lib/class.tcl\nindex f08506f..0b1e671 100644\n--- a/lib/class.tcl\n+++ b/lib/class.tcl\n@@ -136,7 +136,6 @@ proc delete_this {{t {}}} {\n \n proc make_dialog {t w args} {\n \tupvar $t top $w pfx this this\n-\tglobal use_ttk\n \tuplevel [linsert $args 0 make_toplevel $t $w]\n \tcatch {wm attributes $top -type dialog}\n \tpave_toplevel $pfx\ndiff --git a/lib/commit.tcl b/lib/commit.tcl\nindex b516aa2..55ee24a 100644\n--- a/lib/commit.tcl\n+++ b/lib/commit.tcl\n@@ -3,7 +3,6 @@\n \n proc load_last_commit {} {\n \tglobal HEAD PARENT MERGE_HEAD commit_type ui_comm commit_author\n-\tglobal repo_config\n \n \tif {[llength $PARENT] == 0} {\n \t\terror_popup [mc \"There is nothing to amend.\n@@ -142,7 +141,7 @@ proc setup_commit_encoding {msg_wt {quiet 0}} {\n }\n \n proc commit_tree {} {\n-\tglobal HEAD commit_type file_states ui_comm repo_config\n+\tglobal HEAD commit_type file_states ui_comm\n \tglobal pch_error\n \n \tif {[committer_ident] eq {}} return\n@@ -269,7 +268,7 @@ proc commit_prehook_wait {fd_ph curHEAD msg_p} {\n }\n \n proc commit_commitmsg {curHEAD msg_p} {\n-\tglobal is_detached repo_config\n+\tglobal is_detached\n \tglobal pch_error\n \n \tif {$is_detached\n@@ -332,11 +331,8 @@ proc commit_writetree {curHEAD msg_p} {\n \n proc commit_committree {fd_wt curHEAD msg_p} {\n \tglobal HEAD PARENT MERGE_HEAD commit_type commit_author\n-\tglobal current_branch\n \tglobal ui_comm commit_type_is_amend\n-\tglobal file_states selected_paths rescan_active\n-\tglobal repo_config\n-\tglobal env\n+\tglobal file_states selected_paths\n \n \tgets $fd_wt tree_id\n \tif {[catch {close $fd_wt} err]} {\ndiff --git a/lib/console.tcl b/lib/console.tcl\nindex 1f3248f..f9f2231 100644\n--- a/lib/console.tcl\n+++ b/lib/console.tcl\n@@ -27,7 +27,7 @@ constructor embed {path title} {\n }\n \n method _init {} {\n-\tglobal M1B use_ttk NS\n+\tglobal M1B NS\n \n \tif {$is_toplevel} {\n \t\tmake_dialog top w -autodelete 0\ndiff --git a/lib/database.tcl b/lib/database.tcl\nindex 8578308..cb01ead 100644\n--- a/lib/database.tcl\n+++ b/lib/database.tcl\n@@ -2,7 +2,7 @@\n # Copyright (C) 2006, 2007 Shawn Pearce\n \n proc do_stats {} {\n-\tglobal use_ttk NS\n+\tglobal NS\n \tset fd [git_read count-objects -v]\n \twhile {[gets $fd line] > 0} {\n \t\tif {[regexp {^([^:]+): (\\d+)$} $line _ name value]} {\ndiff --git a/lib/diff.tcl b/lib/diff.tcl\nindex 871ad48..6f66936 100644\n--- a/lib/diff.tcl\n+++ b/lib/diff.tcl\n@@ -63,7 +63,7 @@ proc force_diff_encoding {enc} {\n }\n \n proc handle_empty_diff {} {\n-\tglobal current_diff_path file_states file_lists\n+\tglobal current_diff_path file_states\n \tglobal diff_empty_count\n \n \tset path $current_diff_path\n@@ -88,10 +88,8 @@ A rescan will be automatically started to find other files which may have the sa\n }\n \n proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {\n-\tglobal file_states file_lists\n-\tglobal is_3way_diff is_conflict_diff diff_active repo_config\n-\tglobal ui_diff ui_index ui_workdir\n-\tglobal current_diff_path current_diff_side current_diff_header\n+\tglobal file_states file_lists is_conflict_diff diff_active\n+\tglobal current_diff_path current_diff_side\n \tglobal current_diff_queue\n \n \tif {$diff_active || ![lock_index read]} return\n@@ -133,8 +131,7 @@ proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {\n }\n \n proc show_unmerged_diff {cont_info} {\n-\tglobal current_diff_path current_diff_side\n-\tglobal merge_stages ui_diff is_conflict_diff\n+\tglobal current_diff_path merge_stages is_conflict_diff\n \tglobal current_diff_queue\n \n \tif {$merge_stages(2) eq {}} {\n@@ -179,10 +176,7 @@ proc advance_diff_queue {cont_info} {\n }\n \n proc show_other_diff {path w m cont_info} {\n-\tglobal file_states file_lists\n-\tglobal is_3way_diff diff_active repo_config\n-\tglobal ui_diff ui_index ui_workdir\n-\tglobal current_diff_path current_diff_side current_diff_header\n+\tglobal diff_active ui_diff\n \n \t# - Git won't give us the diff, there's nothing to compare to!\n \t#\n@@ -271,9 +265,8 @@ proc show_other_diff {path w m cont_info} {\n }\n \n proc start_show_diff {cont_info {add_opts {}}} {\n-\tglobal file_states file_lists\n+\tglobal file_states ui_index ui_workdir\n \tglobal is_3way_diff is_submodule_diff diff_active repo_config\n-\tglobal ui_diff ui_index ui_workdir\n \tglobal current_diff_path current_diff_side current_diff_header\n \n \tset path $current_diff_path\n@@ -879,7 +872,7 @@ proc apply_or_revert_range_or_line {x y revert} {\n # stack/deque for simplicity, so multiple undos are not possible. Maybe this\n # can be added if the need for something like this is felt in the future.\n proc undo_last_revert {} {\n-\tglobal last_revert current_diff_path current_diff_header\n+\tglobal last_revert\n \tglobal last_revert_enc\n \n \tif {$last_revert eq {}} return\ndiff --git a/lib/error.tcl b/lib/error.tcl\nindex 8968a57..990c5cd 100644\n--- a/lib/error.tcl\n+++ b/lib/error.tcl\n@@ -71,7 +71,7 @@ proc ask_popup {msg} {\n }\n \n proc hook_failed_popup {hook msg {is_fatal 1}} {\n-\tglobal use_ttk NS\n+\tglobal NS\n \tset w .hookfail\n \tDialog $w\n \twm withdraw $w\ndiff --git a/lib/index.tcl b/lib/index.tcl\nindex e07b7a3..416aeb6 100644\n--- a/lib/index.tcl\n+++ b/lib/index.tcl\n@@ -8,7 +8,7 @@ proc _delete_indexlock {} {\n }\n \n proc _close_updateindex {fd after} {\n-\tglobal use_ttk NS\n+\tglobal NS\n \tfconfigure $fd -blocking 1\n \tif {[catch {close $fd} err]} {\n \t\tset w .indexfried\n@@ -87,7 +87,7 @@ proc update_indexinfo {msg pathList after} {\n \n proc write_update_indexinfo {fd pathList totalCnt batch after} {\n \tglobal update_index_cp\n-\tglobal file_states current_diff_path\n+\tglobal file_states\n \n \tif {$update_index_cp >= $totalCnt} {\n \t\t_close_updateindex $fd $after\n@@ -153,7 +153,7 @@ proc update_index {msg pathList after} {\n \n proc write_update_index {fd pathList totalCnt batch after} {\n \tglobal update_index_cp\n-\tglobal file_states current_diff_path\n+\tglobal file_states\n \n \tif {$update_index_cp >= $totalCnt} {\n \t\t_close_updateindex $fd $after\n@@ -229,8 +229,7 @@ proc checkout_index {msg pathList after} {\n }\n \n proc write_checkout_index {fd pathList totalCnt batch after} {\n-\tglobal update_index_cp\n-\tglobal file_states current_diff_path\n+\tglobal update_index_cp file_states\n \n \tif {$update_index_cp >= $totalCnt} {\n \t\t_close_updateindex $fd $after\ndiff --git a/lib/line.tcl b/lib/line.tcl\nindex a026de9..5c80fec 100644\n--- a/lib/line.tcl\n+++ b/lib/line.tcl\n@@ -9,7 +9,7 @@ field ctext\n field linenum   {}\n \n constructor new {i_w i_text args} {\n-\tglobal use_ttk NS\n+\tglobal NS\n \tset w      $i_w\n \tset ctext  $i_text\n \ndiff --git a/lib/merge.tcl b/lib/merge.tcl\nindex 9f253db..6967eca 100644\n--- a/lib/merge.tcl\n+++ b/lib/merge.tcl\n@@ -144,8 +144,7 @@ method _finish {cons ok} {\n }\n \n constructor dialog {} {\n-\tglobal current_branch\n-\tglobal M1B use_ttk NS\n+\tglobal current_branch M1B NS\n \n \tif {![_can_merge $this]} {\n \t\tdelete_this\n@@ -212,7 +211,7 @@ method _cancel {} {\n namespace eval merge {\n \n proc reset_hard {} {\n-\tglobal HEAD commit_type file_states\n+\tglobal HEAD commit_type\n \n \tif {[string match amend* $commit_type]} {\n \t\tinfo_popup [mc \"Cannot abort while amending.\ndiff --git a/lib/mergetool.tcl b/lib/mergetool.tcl\nindex 120bc40..44a019b 100644\n--- a/lib/mergetool.tcl\n+++ b/lib/mergetool.tcl\n@@ -52,7 +52,7 @@ proc do_merge_stage_workdir {path} {\n }\n \n proc merge_add_resolution {path} {\n-\tglobal current_diff_path ui_workdir\n+\tglobal ui_workdir\n \n \tset after [next_diff_after_action $ui_workdir $path {} {^_?U}]\n \n@@ -110,7 +110,7 @@ proc read_merge_stages {fd cont} {\n \t\tset fcols [split $p \"\\t\"]\n \t\tset cols  [split [lindex $fcols 0] \" \"]\n \t\tset stage [lindex $cols 2]\n-\t\t\n+\n \t\tset merge_stages($stage) [lrange $cols 0 1]\n \t}\n \n@@ -304,7 +304,7 @@ proc merge_tool_get_stages {target stages} {\n }\n \n proc merge_tool_start {cmdline target backup stages} {\n-\tglobal merge_stages mtool_target mtool_tmpfiles mtool_fd mtool_mtime\n+\tglobal mtool_target mtool_tmpfiles mtool_fd mtool_mtime\n \n \tif {[info exists mtool_fd]} {\n \t\tif {[ask_popup [mc \"Merge tool is already running, terminate it?\"]] eq {yes}} {\n@@ -358,7 +358,7 @@ proc merge_tool_start {cmdline target backup stages} {\n }\n \n proc read_mtool_output {fd} {\n-\tglobal mtool_fd mtool_tmpfiles\n+\tglobal mtool_fd\n \n \tread $fd\n \tif {[eof $fd]} {\n@@ -370,7 +370,7 @@ proc read_mtool_output {fd} {\n }\n \n proc merge_tool_finish {fd} {\n-\tglobal mtool_tmpfiles mtool_target mtool_mtime\n+\tglobal mtool_tmpfiles mtool_target\n \n \tset backup [lindex $mtool_tmpfiles end]\n \tset failed 0\ndiff --git a/lib/remote_add.tcl b/lib/remote_add.tcl\nindex 480a6b3..56c193f 100644\n--- a/lib/remote_add.tcl\n+++ b/lib/remote_add.tcl\n@@ -13,7 +13,7 @@ field location     {}; # location of the remote the user has chosen\n field opt_action fetch; # action to do after registering the remote locally\n \n constructor dialog {} {\n-\tglobal repo_config use_ttk NS\n+\tglobal NS\n \n \tmake_dialog top w\n \twm withdraw $top\n@@ -88,8 +88,7 @@ constructor dialog {} {\n }\n \n method _add {} {\n-\tglobal repo_config env\n-\tglobal M1B\n+\tglobal env\n \n \tif {$name eq {}} {\n \t\ttk_messageBox \\\ndiff --git a/lib/search.tcl b/lib/search.tcl\nindex ef1e555..d04b9c3 100644\n--- a/lib/search.tcl\n+++ b/lib/search.tcl\n@@ -21,7 +21,7 @@ field smarktop\n field smarkbot\n \n constructor new {i_w i_text args} {\n-\tglobal use_ttk NS\n+\tglobal NS\n \tset w      $i_w\n \tset ctext  $i_text\n \n@@ -68,7 +68,7 @@ constructor new {i_w i_text args} {\n \tbind $w.ent <Shift-Return> [cb find_prev]\n \tbind $w.ent <Key-Up>   [cb _prev_search]\n \tbind $w.ent <Key-Down> [cb _next_search]\n-\t\n+\n \tbind $w <Destroy> [list delete_this $this]\n \treturn $this\n }\ndiff --git a/lib/sshkey.tcl b/lib/sshkey.tcl\nindex 589ff8f..94de6aa 100644\n--- a/lib/sshkey.tcl\n+++ b/lib/sshkey.tcl\n@@ -18,7 +18,7 @@ proc find_ssh_key {} {\n }\n \n proc do_ssh_key {} {\n-\tglobal sshkey_title have_tk85 sshkey_fd use_ttk NS\n+\tglobal sshkey_title have_tk85 use_ttk NS\n \n \tset w .sshkey_dialog\n \tif {[winfo exists $w]} {\ndiff --git a/lib/tools_dlg.tcl b/lib/tools_dlg.tcl\nindex c05413c..0e373c6 100644\n--- a/lib/tools_dlg.tcl\n+++ b/lib/tools_dlg.tcl\n@@ -16,7 +16,7 @@ field ask_branch    0; # ask for a revision\n field ask_args      0; # ask for additional args\n \n constructor dialog {} {\n-\tglobal repo_config use_ttk NS\n+\tglobal NS\n \n \tmake_dialog top w\n \twm title $top [mc \"%s (%s): Add Tool\" [appname] [reponame]]\n@@ -179,7 +179,7 @@ field w              ; # widget path\n field w_names        ; # name list\n \n constructor dialog {} {\n-\tglobal repo_config global_config system_config use_ttk NS\n+\tglobal global_config system_config NS\n \n \tload_config 1\n \n@@ -272,7 +272,7 @@ field is_ok         0; # ok to start\n field argstr       {}; # arguments\n \n constructor dialog {fullname} {\n-\tglobal M1B use_ttk NS\n+\tglobal M1B NS\n \n \tset title [get_config \"guitool.$fullname.title\"]\n \tif {$title eq {}} {\n-- \n2.21.0\n\n"},{"id":"384841","messageId":"xmqq1rv1eaz1.fsf@gitster-ct.c.googlers.com","threadId":"52115","inReplyTo":"20191025013255.7367-2-me@yadavpratyush.com","subject":"Re: [PATCH 1/1] git-gui: remove unused global declarations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-25T03:54:26Z","receivedAt":"2019-10-25T03:54:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pratyush Yadav <me@yadavpratyush.com> writes:\n\n>  proc next_diff {{after {}}} {\n> -\tglobal next_diff_p next_diff_w next_diff_i\n> +\tglobal next_diff_p next_diff_w\n>  \tshow_diff $next_diff_p $next_diff_w {} {} $after\n>  }\n\nNot in particular about next_diff_i, but seeing a hunk like this\nmakes me wonder if you want to go the other way around.  If a future\nfix needs to (re)introduce the use of next_diff_i global variable in\nthis proc (it seems that there are two procs that declare the\nvariable as global, one of which is this one, and the other one\nassigns to it), the code change must resurrect this declaration;\notherwise the code would only confuse itself by potentially having\ntwo variables (one global, one local) with the same name, no?\n\nFor next_diff_i in particular, I think the right solution would be\nto remove both global decl and the assignment, as the assignment is\nmade to otherwise unused variable.  But the primary point in such a\nchange is not \"remove unused global decl\"; it is \"remove unused\nvariable\".\n\n"},{"id":"384990","messageId":"nycvar.QRO.7.76.6.1910281412300.46@tvgsbejvaqbjf.bet","threadId":"52115","inReplyTo":"20191025013255.7367-1-me@yadavpratyush.com","subject":"Re: [PATCH 0/1] git-gui: remove unused global declarations","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-28T13:15:51Z","receivedAt":"2019-10-28T13:16:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Pratyush,\n\nOn Fri, 25 Oct 2019, Pratyush Yadav wrote:\n\n> A lot of places have unused global variables declared. Remove them.\n>\n> It started as a couple of quick fixes and ended up in me writing an ugly\n> hacky script to catch all the instances of unused global declarations.\n> Lot more than I expected.\n>\n> The script can be found at [0].\n\nWouldn't it make more sense to integrate that script into a Makefile\ntarget `check`, rather than hiding it in a gist (that might become\nunavailable if GitHub goes away, as some of you seem to fear)?\n\n> The script at [0] will only catch the unused globals in 'proc's. But\n> git-gui also has 'method's and 'constructor's. Just change '^proc' to\n> '^constructor' or '^method'.\n\nWhy not use `grep -E -n -e '^(proc|method)'`?\n\n> I manually checked each instance before removing just to be sure I'm not\n> doing something wrong. Still, a bit of testing would be appreciated.\n> Just apply this patch and go on about your business as usual. There\n> _should_ be no change in the behaviour. I tested some basic functions\n> like commit, push, etc, and they work fine for me.\n\nI would, if I used Git GUI regularly ;-)\n\nCiao,\nDscho\n\n>\n> [0] https://gist.github.com/prati0100/0f3ef903ad1658e76ea0b95f001b4865\n>\n> Pratyush Yadav (1):\n>   git-gui: remove unused global declarations\n>\n>  git-gui.sh                | 12 ++++--------\n>  lib/about.tcl             |  2 +-\n>  lib/blame.tcl             |  3 +--\n>  lib/branch_checkout.tcl   |  2 +-\n>  lib/branch_create.tcl     |  1 -\n>  lib/branch_delete.tcl     |  2 +-\n>  lib/browser.tcl           |  2 +-\n>  lib/checkout_op.tcl       |  4 +---\n>  lib/choose_font.tcl       |  2 +-\n>  lib/choose_repository.tcl |  6 +++---\n>  lib/class.tcl             |  1 -\n>  lib/commit.tcl            | 10 +++-------\n>  lib/console.tcl           |  2 +-\n>  lib/database.tcl          |  2 +-\n>  lib/diff.tcl              | 21 +++++++--------------\n>  lib/error.tcl             |  2 +-\n>  lib/index.tcl             |  9 ++++-----\n>  lib/line.tcl              |  2 +-\n>  lib/merge.tcl             |  5 ++---\n>  lib/mergetool.tcl         | 10 +++++-----\n>  lib/remote_add.tcl        |  5 ++---\n>  lib/search.tcl            |  4 ++--\n>  lib/sshkey.tcl            |  2 +-\n>  lib/tools_dlg.tcl         |  6 +++---\n>  24 files changed, 47 insertions(+), 70 deletions(-)\n>\n> --\n> 2.21.0\n>\n>\n"},{"id":"385095","messageId":"20191029190934.mr73g3ohwtgmndoo@yadavpratyush.com","threadId":"52115","inReplyTo":"nycvar.QRO.7.76.6.1910281412300.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/1] git-gui: remove unused global declarations","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-29T19:09:34Z","receivedAt":"2019-10-29T19:09:39Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 28/10/19 02:15PM, Johannes Schindelin wrote:\n> Hi Pratyush,\n> \n> On Fri, 25 Oct 2019, Pratyush Yadav wrote:\n> \n> > A lot of places have unused global variables declared. Remove them.\n> >\n> > It started as a couple of quick fixes and ended up in me writing an ugly\n> > hacky script to catch all the instances of unused global declarations.\n> > Lot more than I expected.\n> >\n> > The script can be found at [0].\n> \n> Wouldn't it make more sense to integrate that script into a Makefile\n> target `check`, rather than hiding it in a gist (that might become\n> unavailable if GitHub goes away, as some of you seem to fear)?\n\nThe idea of the script was just a quick hack to list as many instances \nas possible, and then I could manually verify them. It turned out to be \npretty accurate. I linked it here so other people can know what I used \nto find these, and can suggest some cases I missed, if any. That's why I \nlinked to the script in the cover letter, and not in the commit message.\n\nIt would probably be a better idea to use a linter/static analyzer. This \nwould provide much better coverage than a script written for one-off \nusage.\n\nI looked up some linters. There doesn't seem to be a lot of options, and \nmost of them are not very good. One option I found is [0], but it gives \nmore useless warnings than useful ones. Tcl being a really difficult \nlanguage to lint doesn't help. So unless someone knows a good Tcl \nlinter, maybe the best compromise would be to clean up the script and \nadd it to our build process. But I'd really prefer a \"proper\" linter.\n \n> > The script at [0] will only catch the unused globals in 'proc's. But\n> > git-gui also has 'method's and 'constructor's. Just change '^proc' to\n> > '^constructor' or '^method'.\n> \n> Why not use `grep -E -n -e '^(proc|method)'`?\n\nNo particular reason. I just wrote it for 'proc', and then realized I \nmight as well check 'method' and 'constructor'. Like I said, the aim of \nthe script was use-and-throw.\n \n> > I manually checked each instance before removing just to be sure I'm not\n> > doing something wrong. Still, a bit of testing would be appreciated.\n> > Just apply this patch and go on about your business as usual. There\n> > _should_ be no change in the behaviour. I tested some basic functions\n> > like commit, push, etc, and they work fine for me.\n> \n> I would, if I used Git GUI regularly ;-)\n\n[0] https://github.com/Xilinx/XilinxTclStore/blob/master/support/linter/tcl_lint.tcl\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"385117","messageId":"20191029213202.67mmtd3lo324bhmx@yadavpratyush.com","threadId":"52115","inReplyTo":"xmqq1rv1eaz1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] git-gui: remove unused global declarations","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-10-29T21:32:03Z","receivedAt":"2019-10-29T21:32:09Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 25/10/19 12:54PM, Junio C Hamano wrote:\n> Pratyush Yadav <me@yadavpratyush.com> writes:\n> \n> >  proc next_diff {{after {}}} {\n> > -\tglobal next_diff_p next_diff_w next_diff_i\n> > +\tglobal next_diff_p next_diff_w\n> >  \tshow_diff $next_diff_p $next_diff_w {} {} $after\n> >  }\n> \n> Not in particular about next_diff_i, but seeing a hunk like this\n> makes me wonder if you want to go the other way around.  If a future\n> fix needs to (re)introduce the use of next_diff_i global variable in\n> this proc (it seems that there are two procs that declare the\n> variable as global, one of which is this one, and the other one\n> assigns to it), the code change must resurrect this declaration;\n> otherwise the code would only confuse itself by potentially having\n> two variables (one global, one local) with the same name, no?\n\nI'm not sure what you mean by this. Do you mean we should keep \nnext_diff_i global, or do you mean we should instead convert it to a \nlocal variable?\n\nOr is it related to similar sounding variable names (next_diff_i, \nnext_diff_w, next_diff_p), which appear in multiple functions together?\n \n> For next_diff_i in particular, I think the right solution would be\n> to remove both global decl and the assignment, as the assignment is\n> made to otherwise unused variable.  But the primary point in such a\n> change is not \"remove unused global decl\"; it is \"remove unused\n> variable\".\n\nThanks for spotting it! Will fix.\n\n-- \nRegards,\nPratyush Yadav\n"}]}