[PATCH 0/9] gitk: improve keyboard support

[PATCH 0/9] gitk: improve keyboard support

[Jens Lidestrom via GitGitGadget]

It is often convenient to use the keyboard to navigate the gitk GUI and
there are keyboard shortcut bindings for many operations such as searching
and scrolling. There is however no keyboard binding for the most common
operations on branches and commits: Check out, reset, cherry-pick, create
and delete branches.

This PR adds keyboard bindings for these 5 commands. It also adjusts some
GUI focus defaults to simplify keyboard navigation.

Some refactoring of the command implementation has been necessary.
Originally the commands was using the mouse context menu to get info about
the head and commit to act on. When using keyboard binds this information
isn't available so instead the row that is selected in the GUI is used. By
adding procedures for doing this the PR lays the groundwork for more similar
keyboard binds in the future.

I'm including Paul Mackerras because he seems to be the maintainer of gitk.
Can you review, Paul?

Jens Lidestrom (9):
  gitk: add procedures to get commit info from selected row
  gitk: use term "current branch" in gui
  gitk: add keyboard bind for reset
  gitk: show branch name in reset dialog
  gitk: add keyboard bind for checkout
  gitk: add keyboard bind for create and remove branch
  gitk: add keyboard bind to cherry-pick
  gitk: focus ok button in reset dialog
  gitk: default select reset hard in dialog

 gitk-git/gitk | 132 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 96 insertions(+), 36 deletions(-)


base-commit: 94486b6763c29144c60932829a65fec0597e17b3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1551
-- 
gitgitgadget

[PATCH 1/9] gitk: add procedures to get commit info from selected row

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

These procedures are useful for implementing keyboard commands. Keyboard
commands don't have access to commits that are selected by the mouse and
hence must get info from the selected row.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index df3ba2ea99b..a533ff9002e 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -1333,6 +1333,24 @@ proc commitonrow {row} {
     return $id
 }
 
+# Get commits ID of the row that is selected in the GUI
+proc selected_line_id {} {
+    global selectedline
+    return [commitonrow $selectedline]
+}
+
+# Gets the first branch name of the row that is selected in the GUI, or the
+# empty string if there is no branches on that commit.
+proc selected_line_head {} {
+    global idheads
+    set id [selected_line_id]
+    if {[info exists idheads($id)]} {
+        return [lindex $idheads($id) 0]
+    } else {
+        return ""
+    }
+}
+
 proc closevarcs {v} {
     global varctok varccommits varcid parents children
     global cmitlisted commitidx vtokmod curview numcommits
-- 
gitgitgadget

[PATCH 2/9] gitk: use term "current branch" in gui

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

This is the term that is used in the official documentation. Instead of
"HEAD branch" which in not standard.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index a533ff9002e..574a80fbcc2 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2693,7 +2693,7 @@ proc makewindow {} {
         {mc "Write commit to file" command writecommit}
         {mc "Create new branch" command mkbranch}
         {mc "Cherry-pick this commit" command cherrypick}
-        {mc "Reset HEAD branch to here" command resethead}
+        {mc "Reset current branch to here" command resethead}
         {mc "Mark this commit" command markhere}
         {mc "Return to mark" command gotomark}
         {mc "Find descendant of this and mark" command find_common_desc}
-- 
gitgitgadget

[PATCH 3/9] gitk: add keyboard bind for reset

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 574a80fbcc2..99c6fb6a848 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2675,6 +2675,7 @@ proc makewindow {} {
     bind $ctext $ctxbut {pop_diff_menu %W %X %Y %x %y}
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
+    bind . <$M1B-t> {resethead [selected_line_id]}
     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }
@@ -2693,7 +2694,7 @@ proc makewindow {} {
         {mc "Write commit to file" command writecommit}
         {mc "Create new branch" command mkbranch}
         {mc "Cherry-pick this commit" command cherrypick}
-        {mc "Reset current branch to here" command resethead}
+        {mc "Reset current branch to here" command {resethead $rowmenuid}}
         {mc "Mark this commit" command markhere}
         {mc "Return to mark" command gotomark}
         {mc "Find descendant of this and mark" command find_common_desc}
@@ -3166,6 +3167,7 @@ proc keys {} {
 [mc "<%s-KP->	Decrease font size" $M1T]
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
+[mc "<%s-T>		Reset current branch to selected commit" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2
@@ -9859,8 +9861,13 @@ proc revert {} {
     notbusy revert
 }
 
-proc resethead {} {
-    global mainhead rowmenuid confirm_ok resettype NS
+proc resethead {reset_target_id} {
+    global headids mainhead confirm_ok resettype NS
+
+    if {! [info exists headids($mainhead)]} {
+        error_popup [mc "Cannot reset a detached head"]
+        return
+    }
 
     set confirm_ok 0
     set w ".confirmreset"
@@ -9868,7 +9875,7 @@ proc resethead {} {
     make_transient $w .
     wm title $w [mc "Confirm reset"]
     ${NS}::label $w.m -text \
-        [mc "Reset branch %s to %s?" $mainhead [string range $rowmenuid 0 7]]
+        [mc "Reset branch %s to %s?" $mainhead [string range $reset_target_id 0 7]]
     pack $w.m -side top -fill x -padx 20 -pady 20
     ${NS}::labelframe $w.f -text [mc "Reset type:"]
     set resettype mixed
@@ -9891,13 +9898,13 @@ proc resethead {} {
     tkwait window $w
     if {!$confirm_ok} return
     if {[catch {set fd [open \
-            [list | git reset --$resettype $rowmenuid 2>@1] r]} err]} {
+            [list | git reset --$resettype $reset_target_id 2>@1] r]} err]} {
         error_popup $err
     } else {
         dohidelocalchanges
         filerun $fd [list readresetstat $fd]
         nowbusy reset [mc "Resetting"]
-        selbyid $rowmenuid
+        selbyid $reset_target_id
     }
 }
 
-- 
gitgitgadget

[PATCH 4/9] gitk: show branch name in reset dialog

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 99c6fb6a848..bfe912983f4 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -1351,6 +1351,21 @@ proc selected_line_head {} {
     }
 }
 
+# If any branch is associated with the argument ID then return that. Otherwise
+# return the ID itself. Useful for displaying the best name of a commit.
+proc commit_name {id should_shorten_id} {
+    global idheads idtags
+    if {[info exists idheads($id)]} {
+        return [lindex $idheads($id) 0]
+    } elseif {[info exists idtags($id)]} {
+        return [lindex $idtags($id) 0]
+    } elseif {$should_shorten_id} {
+        return [string range $id 0 7]
+    } else {
+        return $id
+    }
+}
+
 proc closevarcs {v} {
     global varctok varccommits varcid parents children
     global cmitlisted commitidx vtokmod curview numcommits
@@ -9875,7 +9890,7 @@ proc resethead {reset_target_id} {
     make_transient $w .
     wm title $w [mc "Confirm reset"]
     ${NS}::label $w.m -text \
-        [mc "Reset branch %s to %s?" $mainhead [string range $reset_target_id 0 7]]
+        [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
     pack $w.m -side top -fill x -padx 20 -pady 20
     ${NS}::labelframe $w.f -text [mc "Reset type:"]
     set resettype mixed
-- 
gitgitgadget

[PATCH 5/9] gitk: add keyboard bind for checkout

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

This also introduces the ability to check out detatched heads. This
shouldn't result any problems, because gitk already works with
detatched heads if they are checked out using the terminal.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index bfe912983f4..596977abe89 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2691,6 +2691,7 @@ proc makewindow {} {
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
     bind . <$M1B-t> {resethead [selected_line_id]}
+    bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }
@@ -2707,7 +2708,7 @@ proc makewindow {} {
         {mc "Create tag" command mktag}
         {mc "Copy commit reference" command copyreference}
         {mc "Write commit to file" command writecommit}
-        {mc "Create new branch" command mkbranch}
+        {mc "Create new branch" command {mkbranch $rowmenuid}}
         {mc "Cherry-pick this commit" command cherrypick}
         {mc "Reset current branch to here" command {resethead $rowmenuid}}
         {mc "Mark this commit" command markhere}
@@ -2732,7 +2733,7 @@ proc makewindow {} {
 
     set headctxmenu .headctxmenu
     makemenu $headctxmenu {
-        {mc "Check out this branch" command cobranch}
+        {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
         {mc "Rename this branch" command mvbranch}
         {mc "Remove this branch" command rmbranch}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
@@ -3183,6 +3184,7 @@ proc keys {} {
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
+[mc "<%s-O>		Check out selected commit" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2
@@ -9978,25 +9980,26 @@ proc headmenu {x y id head} {
     tk_popup $headctxmenu $x $y
 }
 
-proc cobranch {} {
-    global headmenuid headmenuhead headids
+proc checkout {newhead newheadid} {
+    global headids
     global showlocalchanges
 
     # check the tree is clean first??
-    set newhead $headmenuhead
+
+    # The ref is either the head, if it exists, or the ID
+    set newheadref [expr {$newhead ne "" ? $newhead : $newheadid}]
+
     set command [list | git checkout]
     if {[string match "remotes/*" $newhead]} {
         set remote $newhead
         set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
-        # The following check is redundant - the menu option should
-        # be disabled to begin with...
         if {[info exists headids($newhead)]} {
             error_popup [mc "A local branch named %s exists already" $newhead]
             return
         }
         lappend command -b $newhead --track $remote
     } else {
-        lappend command $newhead
+        lappend command $newheadref
     }
     lappend command 2>@1
     nowbusy checkout [mc "Checking out"]
@@ -10011,11 +10014,11 @@ proc cobranch {} {
             dodiffindex
         }
     } else {
-        filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
+        filerun $fd [list readcheckoutstat $fd $newhead $newheadref $newheadid]
     }
 }
 
-proc readcheckoutstat {fd newhead newheadid} {
+proc readcheckoutstat {fd newhead newheadref newheadid} {
     global mainhead mainheadid headids idheads showlocalchanges progresscoords
     global viewmainheadid curview
 
@@ -10034,12 +10037,13 @@ proc readcheckoutstat {fd newhead newheadid} {
         return
     }
     set oldmainid $mainheadid
-    if {! [info exists headids($newhead)]} {
+
+    if {$newhead ne "" && ! [info exists headids($newhead)]} {
         set headids($newhead) $newheadid
         lappend idheads($newheadid) $newhead
         addedhead $newheadid $newhead
     }
-    set mainhead $newhead
+    set mainhead $newheadref
     set mainheadid $newheadid
     set viewmainheadid($curview) $newheadid
     redrawtags $oldmainid
-- 
gitgitgadget

[PATCH 6/9] gitk: add keyboard bind for create and remove branch

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 596977abe89..0d83a72a424 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2692,6 +2692,8 @@ proc makewindow {} {
     bind $ctext <<Selection>> rehighlight_search_results
     bind . <$M1B-t> {resethead [selected_line_id]}
     bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
+    bind . <$M1B-m> {rmbranch [selected_line_head] [selected_line_id] 1}
+    bind . <$M1B-b> {mkbranch [selected_line_id]}
     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }
@@ -2735,7 +2737,7 @@ proc makewindow {} {
     makemenu $headctxmenu {
         {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
         {mc "Rename this branch" command mvbranch}
-        {mc "Remove this branch" command rmbranch}
+        {mc "Remove this branch" command {rmbranch $headmenuhead $headmenuid 0}}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
     }
     $headctxmenu configure -tearoff 0
@@ -3185,6 +3187,8 @@ proc keys {} {
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
 [mc "<%s-O>		Check out selected commit" $M1T]
+[mc "<%s-C>		Create branch on selected commit" $M1T]
+[mc "<%s-M>		Remove selected branch" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2
@@ -9576,13 +9580,13 @@ proc wrcomcan {} {
     unset wrcomtop
 }
 
-proc mkbranch {} {
-    global NS rowmenuid
+proc mkbranch {id} {
+    global NS
 
     set top .branchdialog
 
     set val(name) ""
-    set val(id) $rowmenuid
+    set val(id) $id
     set val(command) [list mkbrgo $top]
 
     set ui(title) [mc "Create branch"]
@@ -10054,13 +10058,14 @@ proc readcheckoutstat {fd newhead newheadref newheadid} {
     }
 }
 
-proc rmbranch {} {
-    global headmenuid headmenuhead mainhead
+proc rmbranch {head id shouldComfirm} {
+    global mainhead
     global idheads
-
-    set head $headmenuhead
-    set id $headmenuid
     # this check shouldn't be needed any more...
+    if {$head eq ""} {
+        error_popup [mc "Cannot delete a detached head"]
+        return
+    }
     if {$head eq $mainhead} {
         error_popup [mc "Cannot delete the currently checked-out branch"]
         return
@@ -10070,6 +10075,8 @@ proc rmbranch {} {
         # the stuff on this branch isn't on any other branch
         if {![confirm_popup [mc "The commits on branch %s aren't on any other\
                         branch.\nReally delete branch %s?" $head $head]]} return
+    } elseif {$shouldComfirm} {
+        if {![confirm_popup [mc "Really delete branch %s?" $head]]} return
     }
     nowbusy rmbranch
     update
-- 
gitgitgadget

[PATCH 7/9] gitk: add keyboard bind to cherry-pick

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 0d83a72a424..5b01d1902a5 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2690,6 +2690,7 @@ proc makewindow {} {
     bind $ctext $ctxbut {pop_diff_menu %W %X %Y %x %y}
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
+    bind . <$M1B-p> {cherrypick [selected_line_id]}
     bind . <$M1B-t> {resethead [selected_line_id]}
     bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
     bind . <$M1B-m> {rmbranch [selected_line_head] [selected_line_id] 1}
@@ -2711,7 +2712,7 @@ proc makewindow {} {
         {mc "Copy commit reference" command copyreference}
         {mc "Write commit to file" command writecommit}
         {mc "Create new branch" command {mkbranch $rowmenuid}}
-        {mc "Cherry-pick this commit" command cherrypick}
+        {mc "Cherry-pick this commit" command {cherrypick $rowmenuid}}
         {mc "Reset current branch to here" command {resethead $rowmenuid}}
         {mc "Mark this commit" command markhere}
         {mc "Return to mark" command gotomark}
@@ -3186,6 +3187,7 @@ proc keys {} {
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
+[mc "<%s-P>		Cherry-pick selected commit to current branch" $M1T]
 [mc "<%s-O>		Check out selected commit" $M1T]
 [mc "<%s-C>		Create branch on selected commit" $M1T]
 [mc "<%s-M>		Remove selected branch" $M1T]
@@ -9758,24 +9760,29 @@ proc exec_citool {tool_args {baseid {}}} {
     array set env $save_env
 }
 
-proc cherrypick {} {
-    global rowmenuid curview
+proc cherrypick {id} {
+    global curview headids
     global mainhead mainheadid
     global gitdir
 
+    if {! [info exists headids($mainhead)]} {
+        error_popup [mc "Cannot cherry-pick to a detached head"]
+        return
+    }
+
     set oldhead [exec git rev-parse HEAD]
-    set dheads [descheads $rowmenuid]
+    set dheads [descheads $id]
     if {$dheads ne {} && [lsearch -exact $dheads $oldhead] >= 0} {
         set ok [confirm_popup [mc "Commit %s is already\
                 included in branch %s -- really re-apply it?" \
-                                   [string range $rowmenuid 0 7] $mainhead]]
+                                   [string range $id 0 7] $mainhead]]
         if {!$ok} return
     }
     nowbusy cherrypick [mc "Cherry-picking"]
     update
     # Unfortunately git-cherry-pick writes stuff to stderr even when
     # no error occurs, and exec takes that as an indication of error...
-    if {[catch {exec sh -c "git cherry-pick -r $rowmenuid 2>&1"} err]} {
+    if {[catch {exec sh -c "git cherry-pick -r $id 2>&1"} err]} {
         notbusy cherrypick
         if {[regexp -line \
                  {Entry '(.*)' (would be overwritten by merge|not uptodate)} \
@@ -9791,7 +9798,7 @@ proc cherrypick {} {
                         resolve it?"]]} {
                 # Force citool to read MERGE_MSG
                 file delete [file join $gitdir "GITGUI_MSG"]
-                exec_citool {} $rowmenuid
+                exec_citool {} $id
             }
         } else {
             error_popup $err
-- 
gitgitgadget

[PATCH 8/9] gitk: focus ok button in reset dialog

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

This is more convenient for users, especially when using keyboard
commands.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 5b01d1902a5..9d93053e360 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -9922,7 +9922,7 @@ proc resethead {reset_target_id} {
     ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
     bind $w <Key-Escape> [list destroy $w]
     pack $w.cancel -side right -fill x -padx 20 -pady 20
-    bind $w <Visibility> "grab $w; focus $w"
+    bind $w <Visibility> "grab $w; focus $w.ok"
     tkwait window $w
     if {!$confirm_ok} return
     if {[catch {set fd [open \
-- 
gitgitgadget

[PATCH 9/9] gitk: default select reset hard in dialog

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Reset hard is dangerous but also the most common reset type, and not
having it pre-selected in the dialog is annoying to users.

It is also less dangerous in the GUI where there is a confirmation
dialog. Also, dangling commits remain in the GUI and can be recovered.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 9d93053e360..5b0a0ea46be 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {
         [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
     pack $w.m -side top -fill x -padx 20 -pady 20
     ${NS}::labelframe $w.f -text [mc "Reset type:"]
-    set resettype mixed
+    # Reset hard is dangerous but also the most common reset type, and not
+    # having it pre-selected in the dialog is annoying to users.
+    set resettype hard
     ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
         -text [mc "Soft: Leave working tree and index untouched"]
     grid $w.f.soft -sticky w
-- 
gitgitgadget

Re: [PATCH 9/9] gitk: default select reset hard in dialog

[Johannes Sixt]

Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
> From: Jens Lidestrom <jens@lidestrom.se>
> 
> Reset hard is dangerous but also the most common reset type, and not
> having it pre-selected in the dialog is annoying to users.

I agree that the operation of the Reset dialog is clumsy before this
series. However, this patch together with the previous patch turns it
into a foot gun. It becomes far too easy to destroy uncommitted work.

I would prefer to keep the default at "mixed" mode, set the focus on the
radio button to make it easy to switch to "hard" mode by hitting the
Down arrow key, and then make it so that Enter triggers the OK button.

> It is also less dangerous in the GUI where there is a confirmation
> dialog. Also, dangling commits remain in the GUI and can be recovered.

The problem with "hard" mode are not the commits. The real danger is
that it blows away uncommitted changes. Besides of that, I do not
consider this UI a confirmation dialog.

-- Hannes

> 
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
>  gitk-git/gitk | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 9d93053e360..5b0a0ea46be 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {
>          [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
>      pack $w.m -side top -fill x -padx 20 -pady 20
>      ${NS}::labelframe $w.f -text [mc "Reset type:"]
> -    set resettype mixed
> +    # Reset hard is dangerous but also the most common reset type, and not
> +    # having it pre-selected in the dialog is annoying to users.
> +    set resettype hard
>      ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
>          -text [mc "Soft: Leave working tree and index untouched"]
>      grid $w.f.soft -sticky w

Re: [PATCH 6/9] gitk: add keyboard bind for create and remove branch

[Johannes Sixt]

Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
> From: Jens Lidestrom <jens@lidestrom.se>
> 
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
>  gitk-git/gitk | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 596977abe89..0d83a72a424 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -2692,6 +2692,8 @@ proc makewindow {} {
>      bind $ctext <<Selection>> rehighlight_search_results
>      bind . <$M1B-t> {resethead [selected_line_id]}
>      bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
> +    bind . <$M1B-m> {rmbranch [selected_line_head] [selected_line_id] 1}
> +    bind . <$M1B-b> {mkbranch [selected_line_id]}

"b" vs...

>      for {set i 1} {$i < 10} {incr i} {
>          bind . <$M1B-Key-$i> [list go_to_parent $i]
>      }
> @@ -2735,7 +2737,7 @@ proc makewindow {} {
>      makemenu $headctxmenu {
>          {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
>          {mc "Rename this branch" command mvbranch}
> -        {mc "Remove this branch" command rmbranch}
> +        {mc "Remove this branch" command {rmbranch $headmenuhead $headmenuid 0}}
>          {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
>      }
>      $headctxmenu configure -tearoff 0
> @@ -3185,6 +3187,8 @@ proc keys {} {
>  [mc "<F5>		Update"]
>  [mc "<%s-T>		Reset current branch to selected commit" $M1T]
>  [mc "<%s-O>		Check out selected commit" $M1T]
> +[mc "<%s-C>		Create branch on selected commit" $M1T]

... "C"? Which one is it?

> +[mc "<%s-M>		Remove selected branch" $M1T]
>  " \
>              -justify left -bg $bgcolor -border 2 -relief groove
>      pack $w.m -side top -fill both -padx 2 -pady 2
> @@ -9576,13 +9580,13 @@ proc wrcomcan {} {
>      unset wrcomtop
>  }
>  
> -proc mkbranch {} {
> -    global NS rowmenuid
> +proc mkbranch {id} {
> +    global NS
>  
>      set top .branchdialog
>  
>      set val(name) ""
> -    set val(id) $rowmenuid
> +    set val(id) $id
>      set val(command) [list mkbrgo $top]
>  
>      set ui(title) [mc "Create branch"]
> @@ -10054,13 +10058,14 @@ proc readcheckoutstat {fd newhead newheadref newheadid} {
>      }
>  }
>  
> -proc rmbranch {} {
> -    global headmenuid headmenuhead mainhead
> +proc rmbranch {head id shouldComfirm} {
> +    global mainhead
>      global idheads
> -
> -    set head $headmenuhead
> -    set id $headmenuid
>      # this check shouldn't be needed any more...
> +    if {$head eq ""} {
> +        error_popup [mc "Cannot delete a detached head"]
> +        return
> +    }
>      if {$head eq $mainhead} {
>          error_popup [mc "Cannot delete the currently checked-out branch"]
>          return
> @@ -10070,6 +10075,8 @@ proc rmbranch {} {
>          # the stuff on this branch isn't on any other branch
>          if {![confirm_popup [mc "The commits on branch %s aren't on any other\
>                          branch.\nReally delete branch %s?" $head $head]]} return
> +    } elseif {$shouldComfirm} {
> +        if {![confirm_popup [mc "Really delete branch %s?" $head]]} return
>      }
>      nowbusy rmbranch
>      update

The key binding to remove a branch does not make sense to me. It does
happen that I have more than one branch on a commit, but there is no way
to select which one to remove via the keyboard. I have to use the
context menu. This needs more thought IMHO. At a minimum, separate it
out into its own commit.

-- Hannes

Re: [PATCH 0/9] gitk: improve keyboard support

[Johannes Sixt]

Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
> It is often convenient to use the keyboard to navigate the gitk GUI and
> there are keyboard shortcut bindings for many operations such as searching
> and scrolling. There is however no keyboard binding for the most common
> operations on branches and commits: Check out, reset, cherry-pick, create
> and delete branches.
> 
> This PR adds keyboard bindings for these 5 commands. It also adjusts some
> GUI focus defaults to simplify keyboard navigation.
> 
> Some refactoring of the command implementation has been necessary.
> Originally the commands was using the mouse context menu to get info about
> the head and commit to act on. When using keyboard binds this information
> isn't available so instead the row that is selected in the GUI is used. By
> adding procedures for doing this the PR lays the groundwork for more similar
> keyboard binds in the future.

I like it when an application can be navigated with the keyboard. These
changes are very much appreciated.

I've left some comments on individual commits. The important one is that
I think it makes the Reset dialog way too easy to destroy uncommitted work.

Please note that gitk-git directory is in its own repository that is
only subtree-merged into the Git repository. You should generate patches
against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it
would be for Paul to integrate patches that were generated by gitgitgadget).

-- Hannes

> 
> I'm including Paul Mackerras because he seems to be the maintainer of gitk.
> Can you review, Paul?
> 
> Jens Lidestrom (9):
>   gitk: add procedures to get commit info from selected row
>   gitk: use term "current branch" in gui
>   gitk: add keyboard bind for reset
>   gitk: show branch name in reset dialog
>   gitk: add keyboard bind for checkout
>   gitk: add keyboard bind for create and remove branch
>   gitk: add keyboard bind to cherry-pick
>   gitk: focus ok button in reset dialog
>   gitk: default select reset hard in dialog
> 
>  gitk-git/gitk | 132 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 96 insertions(+), 36 deletions(-)
> 
> 
> base-commit: 94486b6763c29144c60932829a65fec0597e17b3
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1551

Re: [PATCH 0/9] gitk: improve keyboard support

[Jens Lideström]

Thanks for your comments, Hannes!

> Please note that gitk-git directory is in its own repository that is
> only subtree-merged into the Git repository. You should generate 
> patches
> against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult 
> it
> would be for Paul to integrate patches that were generated by 
> gitgitgadget).

I'll try to figure out how to do that.

I looked briefly at Pauls repo but got the impression that it was out of 
date. I'll have a second look.

/Jens

On 2023-06-28 08:09, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> It is often convenient to use the keyboard to navigate the gitk GUI 
>> and
>> there are keyboard shortcut bindings for many operations such as 
>> searching
>> and scrolling. There is however no keyboard binding for the most 
>> common
>> operations on branches and commits: Check out, reset, cherry-pick, 
>> create
>> and delete branches.
>> 
>> This PR adds keyboard bindings for these 5 commands. It also adjusts 
>> some
>> GUI focus defaults to simplify keyboard navigation.
>> 
>> Some refactoring of the command implementation has been necessary.
>> Originally the commands was using the mouse context menu to get info 
>> about
>> the head and commit to act on. When using keyboard binds this 
>> information
>> isn't available so instead the row that is selected in the GUI is 
>> used. By
>> adding procedures for doing this the PR lays the groundwork for more 
>> similar
>> keyboard binds in the future.
> 
> I like it when an application can be navigated with the keyboard. These
> changes are very much appreciated.
> 
> I've left some comments on individual commits. The important one is 
> that
> I think it makes the Reset dialog way too easy to destroy uncommitted 
> work.
> 
> Please note that gitk-git directory is in its own repository that is
> only subtree-merged into the Git repository. You should generate 
> patches
> against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult 
> it
> would be for Paul to integrate patches that were generated by 
> gitgitgadget).
> 
> -- Hannes
> 
>> 
>> I'm including Paul Mackerras because he seems to be the maintainer of 
>> gitk.
>> Can you review, Paul?
>> 
>> Jens Lidestrom (9):
>>   gitk: add procedures to get commit info from selected row
>>   gitk: use term "current branch" in gui
>>   gitk: add keyboard bind for reset
>>   gitk: show branch name in reset dialog
>>   gitk: add keyboard bind for checkout
>>   gitk: add keyboard bind for create and remove branch
>>   gitk: add keyboard bind to cherry-pick
>>   gitk: focus ok button in reset dialog
>>   gitk: default select reset hard in dialog
>> 
>>  gitk-git/gitk | 132 
>> ++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 96 insertions(+), 36 deletions(-)
>> 
>> 
>> base-commit: 94486b6763c29144c60932829a65fec0597e17b3
>> Published-As: 
>> https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
>> pr-1551/jensli/keyboard-for-gitk-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1551

Re: [PATCH 6/9] gitk: add keyboard bind for create and remove branch

[Jens Lideström]

>> +[mc "<%s-C>		Create branch on selected commit" $M1T]
> 
> ... "C"? Which one is it?

"C" is a mistake. Good catch, thanks!

I choose Ctrl-B to avoid a conflict with Ctrl-C for copying text.

> The key binding to remove a branch does not make sense to me. It does
> happen that I have more than one branch on a commit, but there is no 
> way
> to select which one to remove via the keyboard. I have to use the
> context menu. This needs more thought IMHO.

My intention is to always remove the first branch head that is displayed 
for a single commit in the GUI. This caters to the common use case, with 
only one branch for a single commit. If there are multiple branch heads 
on a commit and the users don't want to remove the first one then they 
need to use the mouse context menu to choose which one to delete.

I could change the implementation to display a dialog that lets the user 
choose in case of multiple branch heads.

In that case, should I do that as part of this PR, or as a follow up? I 
would prefer to finish this one first.

> At a minimum, separate it out into its own commit.

I'll do so.

/Jens

On 2023-06-28 07:59, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> From: Jens Lidestrom <jens@lidestrom.se>
>> 
>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>> ---
>>  gitk-git/gitk | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>> 
>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>> index 596977abe89..0d83a72a424 100755
>> --- a/gitk-git/gitk
>> +++ b/gitk-git/gitk
>> @@ -2692,6 +2692,8 @@ proc makewindow {} {
>>      bind $ctext <<Selection>> rehighlight_search_results
>>      bind . <$M1B-t> {resethead [selected_line_id]}
>>      bind . <$M1B-o> {checkout [selected_line_head] 
>> [selected_line_id]}
>> +    bind . <$M1B-m> {rmbranch [selected_line_head] [selected_line_id] 
>> 1}
>> +    bind . <$M1B-b> {mkbranch [selected_line_id]}
> 
> "b" vs...
> 
>>      for {set i 1} {$i < 10} {incr i} {
>>          bind . <$M1B-Key-$i> [list go_to_parent $i]
>>      }
>> @@ -2735,7 +2737,7 @@ proc makewindow {} {
>>      makemenu $headctxmenu {
>>          {mc "Check out this branch" command {checkout $headmenuhead 
>> $headmenuid}}
>>          {mc "Rename this branch" command mvbranch}
>> -        {mc "Remove this branch" command rmbranch}
>> +        {mc "Remove this branch" command {rmbranch $headmenuhead 
>> $headmenuid 0}}
>>          {mc "Copy branch name" command {clipboard clear; clipboard 
>> append $headmenuhead}}
>>      }
>>      $headctxmenu configure -tearoff 0
>> @@ -3185,6 +3187,8 @@ proc keys {} {
>>  [mc "<F5>		Update"]
>>  [mc "<%s-T>		Reset current branch to selected commit" $M1T]
>>  [mc "<%s-O>		Check out selected commit" $M1T]
>> +[mc "<%s-C>		Create branch on selected commit" $M1T]
> 
> ... "C"? Which one is it?
> 
>> +[mc "<%s-M>		Remove selected branch" $M1T]
>>  " \
>>              -justify left -bg $bgcolor -border 2 -relief groove
>>      pack $w.m -side top -fill both -padx 2 -pady 2
>> @@ -9576,13 +9580,13 @@ proc wrcomcan {} {
>>      unset wrcomtop
>>  }
>> 
>> -proc mkbranch {} {
>> -    global NS rowmenuid
>> +proc mkbranch {id} {
>> +    global NS
>> 
>>      set top .branchdialog
>> 
>>      set val(name) ""
>> -    set val(id) $rowmenuid
>> +    set val(id) $id
>>      set val(command) [list mkbrgo $top]
>> 
>>      set ui(title) [mc "Create branch"]
>> @@ -10054,13 +10058,14 @@ proc readcheckoutstat {fd newhead newheadref 
>> newheadid} {
>>      }
>>  }
>> 
>> -proc rmbranch {} {
>> -    global headmenuid headmenuhead mainhead
>> +proc rmbranch {head id shouldComfirm} {
>> +    global mainhead
>>      global idheads
>> -
>> -    set head $headmenuhead
>> -    set id $headmenuid
>>      # this check shouldn't be needed any more...
>> +    if {$head eq ""} {
>> +        error_popup [mc "Cannot delete a detached head"]
>> +        return
>> +    }
>>      if {$head eq $mainhead} {
>>          error_popup [mc "Cannot delete the currently checked-out 
>> branch"]
>>          return
>> @@ -10070,6 +10075,8 @@ proc rmbranch {} {
>>          # the stuff on this branch isn't on any other branch
>>          if {![confirm_popup [mc "The commits on branch %s aren't on 
>> any other\
>>                          branch.\nReally delete branch %s?" $head 
>> $head]]} return
>> +    } elseif {$shouldComfirm} {
>> +        if {![confirm_popup [mc "Really delete branch %s?" $head]]} 
>> return
>>      }
>>      nowbusy rmbranch
>>      update
> 
> The key binding to remove a branch does not make sense to me. It does
> happen that I have more than one branch on a commit, but there is no 
> way
> to select which one to remove via the keyboard. I have to use the
> context menu. This needs more thought IMHO. At a minimum, separate it
> out into its own commit.
> 
> -- Hannes

Re: [PATCH 9/9] gitk: default select reset hard in dialog

[Jens Lideström]

> I would prefer to keep the default at "mixed" mode, set the focus on 
> the
> radio button to make it easy to switch to "hard" mode by hitting the
> Down arrow key, and then make it so that Enter triggers the OK button.

That indeed sounds better! Safer but still convenient. I'll change the 
solution to this.

I noticed that some dialogues have a key bind to close with success 
using Enter.

/Jens


On 2023-06-28 07:46, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> From: Jens Lidestrom <jens@lidestrom.se>
>> 
>> Reset hard is dangerous but also the most common reset type, and not
>> having it pre-selected in the dialog is annoying to users.
> 
> I agree that the operation of the Reset dialog is clumsy before this
> series. However, this patch together with the previous patch turns it
> into a foot gun. It becomes far too easy to destroy uncommitted work.
> 
> I would prefer to keep the default at "mixed" mode, set the focus on 
> the
> radio button to make it easy to switch to "hard" mode by hitting the
> Down arrow key, and then make it so that Enter triggers the OK button.
> 
>> It is also less dangerous in the GUI where there is a confirmation
>> dialog. Also, dangling commits remain in the GUI and can be recovered.
> 
> The problem with "hard" mode are not the commits. The real danger is
> that it blows away uncommitted changes. Besides of that, I do not
> consider this UI a confirmation dialog.
> 
> -- Hannes
> 
>> 
>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>> ---
>>  gitk-git/gitk | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>> index 9d93053e360..5b0a0ea46be 100755
>> --- a/gitk-git/gitk
>> +++ b/gitk-git/gitk
>> @@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {
>>          [mc "Reset branch %s to %s?" $mainhead [commit_name 
>> $reset_target_id 1]]
>>      pack $w.m -side top -fill x -padx 20 -pady 20
>>      ${NS}::labelframe $w.f -text [mc "Reset type:"]
>> -    set resettype mixed
>> +    # Reset hard is dangerous but also the most common reset type, 
>> and not
>> +    # having it pre-selected in the dialog is annoying to users.
>> +    set resettype hard
>>      ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
>>          -text [mc "Soft: Leave working tree and index untouched"]
>>      grid $w.f.soft -sticky w

Re: [PATCH 0/9] gitk: improve keyboard support

[Jens Lideström]

@Hannes: I choose key combinations with Ctrl+<another-key>.

Another possibility is to use Ctrl+Shift+<another-key>. That is more complicated to press, but it creates a nice distinction between two categories of commands:

Searching and navigation command (existing): Ctrl+<another-key>

Branch modification commands (new): Ctrl+Shift+<another-key>

Do you have any opinion on this? Only Ctrl, or Ctrl+Shift for the new commands?

/Jens



On 2023-06-28 08:09, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> It is often convenient to use the keyboard to navigate the gitk GUI and
>> there are keyboard shortcut bindings for many operations such as searching
>> and scrolling. There is however no keyboard binding for the most common
>> operations on branches and commits: Check out, reset, cherry-pick, create
>> and delete branches.
>>
>> This PR adds keyboard bindings for these 5 commands. It also adjusts some
>> GUI focus defaults to simplify keyboard navigation.
>>
>> Some refactoring of the command implementation has been necessary.
>> Originally the commands was using the mouse context menu to get info about
>> the head and commit to act on. When using keyboard binds this information
>> isn't available so instead the row that is selected in the GUI is used. By
>> adding procedures for doing this the PR lays the groundwork for more similar
>> keyboard binds in the future.
> I like it when an application can be navigated with the keyboard. These
> changes are very much appreciated.
>
> I've left some comments on individual commits. The important one is that
> I think it makes the Reset dialog way too easy to destroy uncommitted work.
>
> Please note that gitk-git directory is in its own repository that is
> only subtree-merged into the Git repository. You should generate patches
> against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it
> would be for Paul to integrate patches that were generated by gitgitgadget).
>
> -- Hannes
>
>> I'm including Paul Mackerras because he seems to be the maintainer of gitk.
>> Can you review, Paul?
>>
>> Jens Lidestrom (9):
>>    gitk: add procedures to get commit info from selected row
>>    gitk: use term "current branch" in gui
>>    gitk: add keyboard bind for reset
>>    gitk: show branch name in reset dialog
>>    gitk: add keyboard bind for checkout
>>    gitk: add keyboard bind for create and remove branch
>>    gitk: add keyboard bind to cherry-pick
>>    gitk: focus ok button in reset dialog
>>    gitk: default select reset hard in dialog
>>
>>   gitk-git/gitk | 132 ++++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 96 insertions(+), 36 deletions(-)
>>
>>
>> base-commit: 94486b6763c29144c60932829a65fec0597e17b3
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1551

Re: [PATCH 6/9] gitk: add keyboard bind for create and remove branch

[Johannes Sixt]

Am 28.06.23 um 09:12 schrieb Jens Lideström:
> My intention is to always remove the first branch head that is displayed
> for a single commit in the GUI. This caters to the common use case, with
> only one branch for a single commit. If there are multiple branch heads
> on a commit and the users don't want to remove the first one then they
> need to use the mouse context menu to choose which one to delete.
> 
> I could change the implementation to display a dialog that lets the user
> choose in case of multiple branch heads.

IMO a selection dialog is the second-best solution. The best one is to
make the branch labels selectable somehow via keyboard navigation. I am
not a fan of the here implement behavior, because it is only a
half-solution.

Also, the order of branch labels on a line is not 100% deterministic:
when you create a branch, it goes last, but when you refresh the view,
branch labels are sorted alphabetically (I think). This means you can't
create a branch and delete it right away if there is already a branch on
the commit.

> In that case, should I do that as part of this PR, or as a follow up? I
> would prefer to finish this one first.

My preference would be to not implement this behavior until it is clear
what it should be.

-- Hannes

Re: [PATCH 0/9] gitk: improve keyboard support

[Johannes Sixt]

Am 28.06.23 um 19:32 schrieb Jens Lideström:
> @Hannes: I choose key combinations with Ctrl+<another-key>.
> 
> Another possibility is to use Ctrl+Shift+<another-key>. That is more
> complicated to press, but it creates a nice distinction between two
> categories of commands:
> 
> Searching and navigation command (existing): Ctrl+<another-key>
> 
> Branch modification commands (new): Ctrl+Shift+<another-key>
> 
> Do you have any opinion on this? Only Ctrl, or Ctrl+Shift for the new
> commands?

I have no strong opinion, with a mild preference for the versions
without Shift, just because they are more ergonomic.

-- Hannes

Re: [PATCH 6/9] gitk: add keyboard bind for create and remove branch

[Jens Lideström]

> IMO a selection dialog is the second-best solution. The best one is to
> make the branch labels selectable somehow via keyboard navigation. I am
> not a fan of the here implement behavior, because it is only a
> half-solution.

I added a branch selection dialog to the remove command, triggered when there is more than one branch on the selected commit. It works fine and is convenient to use. However, the implementation is more complicated and the patch is much larger.

Since the patch is larger I felt that it was warranted to clarify some variable names.

The checkout command also has got a branch selection dialog.

Create and remove commands have been put in separate commits.

/Jens


On 2023-06-28 22:30, Johannes Sixt wrote:
> Am 28.06.23 um 09:12 schrieb Jens Lideström:
>> My intention is to always remove the first branch head that is displayed
>> for a single commit in the GUI. This caters to the common use case, with
>> only one branch for a single commit. If there are multiple branch heads
>> on a commit and the users don't want to remove the first one then they
>> need to use the mouse context menu to choose which one to delete.
>>
>> I could change the implementation to display a dialog that lets the user
>> choose in case of multiple branch heads.
> 
> IMO a selection dialog is the second-best solution. The best one is to
> make the branch labels selectable somehow via keyboard navigation. I am
> not a fan of the here implement behavior, because it is only a
> half-solution.
> 
> Also, the order of branch labels on a line is not 100% deterministic:
> when you create a branch, it goes last, but when you refresh the view,
> branch labels are sorted alphabetically (I think). This means you can't
> create a branch and delete it right away if there is already a branch on
> the commit.
> 
>> In that case, should I do that as part of this PR, or as a follow up? I
>> would prefer to finish this one first.
> 
> My preference would be to not implement this behavior until it is clear
> what it should be.
> 
> -- Hannes
> 

Re: [PATCH 9/9] gitk: default select reset hard in dialog

[Jens Lideström]

I updated the implementation to use "mixed" as the default, but allow switching selected item with up and down keys, and accept with enter. It works fine! :)

/Jens

On 2023-06-28 09:16, Jens Lideström wrote:
>> I would prefer to keep the default at "mixed" mode, set the focus on the
>> radio button to make it easy to switch to "hard" mode by hitting the
>> Down arrow key, and then make it so that Enter triggers the OK button.
> 
> That indeed sounds better! Safer but still convenient. I'll change the solution
> to this.
> 
> I noticed that some dialogues have a key bind to close with success using Enter.
> 
> /Jens
> 
> 
> On 2023-06-28 07:46, Johannes Sixt wrote:
>> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>>> From: Jens Lidestrom <jens@lidestrom.se>
>>>
>>> Reset hard is dangerous but also the most common reset type, and not
>>> having it pre-selected in the dialog is annoying to users.
>>
>> I agree that the operation of the Reset dialog is clumsy before this
>> series. However, this patch together with the previous patch turns it
>> into a foot gun. It becomes far too easy to destroy uncommitted work.
>>
>> I would prefer to keep the default at "mixed" mode, set the focus on the
>> radio button to make it easy to switch to "hard" mode by hitting the
>> Down arrow key, and then make it so that Enter triggers the OK button.
>>
>>> It is also less dangerous in the GUI where there is a confirmation
>>> dialog. Also, dangling commits remain in the GUI and can be recovered.
>>
>> The problem with "hard" mode are not the commits. The real danger is
>> that it blows away uncommitted changes. Besides of that, I do not
>> consider this UI a confirmation dialog.
>>
>> -- Hannes
>>
>>>
>>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>>> ---
>>>  gitk-git/gitk | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>>> index 9d93053e360..5b0a0ea46be 100755
>>> --- a/gitk-git/gitk
>>> +++ b/gitk-git/gitk
>>> @@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {
>>>          [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id
>>> 1]]
>>>      pack $w.m -side top -fill x -padx 20 -pady 20
>>>      ${NS}::labelframe $w.f -text [mc "Reset type:"]
>>> -    set resettype mixed
>>> +    # Reset hard is dangerous but also the most common reset type, and not
>>> +    # having it pre-selected in the dialog is annoying to users.
>>> +    set resettype hard
>>>      ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
>>>          -text [mc "Soft: Leave working tree and index untouched"]
>>>      grid $w.f.soft -sticky w

Re: [PATCH 5/9] gitk: add keyboard bind for checkout

[Jens Lideström]

I added a branch selection dialog to be be able to check out any branch on a selected commit, in the same way as for the remove branch command.

/Jens

On 2023-06-27 16:41, Jens Lidestrom via GitGitGadget wrote:
> From: Jens Lidestrom <jens@lidestrom.se>
> 
> This also introduces the ability to check out detatched heads. This
> shouldn't result any problems, because gitk already works with
> detatched heads if they are checked out using the terminal.
> 
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
>  gitk-git/gitk | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index bfe912983f4..596977abe89 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -2691,6 +2691,7 @@ proc makewindow {} {
>      bind $ctext <Button-1> {focus %W}
>      bind $ctext <<Selection>> rehighlight_search_results
>      bind . <$M1B-t> {resethead [selected_line_id]}
> +    bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
>      for {set i 1} {$i < 10} {incr i} {
>          bind . <$M1B-Key-$i> [list go_to_parent $i]
>      }
> @@ -2707,7 +2708,7 @@ proc makewindow {} {
>          {mc "Create tag" command mktag}
>          {mc "Copy commit reference" command copyreference}
>          {mc "Write commit to file" command writecommit}
> -        {mc "Create new branch" command mkbranch}
> +        {mc "Create new branch" command {mkbranch $rowmenuid}}
>          {mc "Cherry-pick this commit" command cherrypick}
>          {mc "Reset current branch to here" command {resethead $rowmenuid}}
>          {mc "Mark this commit" command markhere}
> @@ -2732,7 +2733,7 @@ proc makewindow {} {
>  
>      set headctxmenu .headctxmenu
>      makemenu $headctxmenu {
> -        {mc "Check out this branch" command cobranch}
> +        {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
>          {mc "Rename this branch" command mvbranch}
>          {mc "Remove this branch" command rmbranch}
>          {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
> @@ -3183,6 +3184,7 @@ proc keys {} {
>  [mc "<%s-minus>	Decrease font size" $M1T]
>  [mc "<F5>		Update"]
>  [mc "<%s-T>		Reset current branch to selected commit" $M1T]
> +[mc "<%s-O>		Check out selected commit" $M1T]
>  " \
>              -justify left -bg $bgcolor -border 2 -relief groove
>      pack $w.m -side top -fill both -padx 2 -pady 2
> @@ -9978,25 +9980,26 @@ proc headmenu {x y id head} {
>      tk_popup $headctxmenu $x $y
>  }
>  
> -proc cobranch {} {
> -    global headmenuid headmenuhead headids
> +proc checkout {newhead newheadid} {
> +    global headids
>      global showlocalchanges
>  
>      # check the tree is clean first??
> -    set newhead $headmenuhead
> +
> +    # The ref is either the head, if it exists, or the ID
> +    set newheadref [expr {$newhead ne "" ? $newhead : $newheadid}]
> +
>      set command [list | git checkout]
>      if {[string match "remotes/*" $newhead]} {
>          set remote $newhead
>          set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
> -        # The following check is redundant - the menu option should
> -        # be disabled to begin with...
>          if {[info exists headids($newhead)]} {
>              error_popup [mc "A local branch named %s exists already" $newhead]
>              return
>          }
>          lappend command -b $newhead --track $remote
>      } else {
> -        lappend command $newhead
> +        lappend command $newheadref
>      }
>      lappend command 2>@1
>      nowbusy checkout [mc "Checking out"]
> @@ -10011,11 +10014,11 @@ proc cobranch {} {
>              dodiffindex
>          }
>      } else {
> -        filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
> +        filerun $fd [list readcheckoutstat $fd $newhead $newheadref $newheadid]
>      }
>  }
>  
> -proc readcheckoutstat {fd newhead newheadid} {
> +proc readcheckoutstat {fd newhead newheadref newheadid} {
>      global mainhead mainheadid headids idheads showlocalchanges progresscoords
>      global viewmainheadid curview
>  
> @@ -10034,12 +10037,13 @@ proc readcheckoutstat {fd newhead newheadid} {
>          return
>      }
>      set oldmainid $mainheadid
> -    if {! [info exists headids($newhead)]} {
> +
> +    if {$newhead ne "" && ! [info exists headids($newhead)]} {
>          set headids($newhead) $newheadid
>          lappend idheads($newheadid) $newhead
>          addedhead $newheadid $newhead
>      }
> -    set mainhead $newhead
> +    set mainhead $newheadref
>      set mainheadid $newheadid
>      set viewmainheadid($curview) $newheadid
>      redrawtags $oldmainid

Re: [PATCH 0/9] gitk: improve keyboard support

[Jens Lideström]

I have updated the PR after suggestions from Hannes. Mainly these changes have been made:

* The reset command dialog uses "mixed" as the default, but is more convenient to navigate with the keyboard.
* Remove and checkout branch commands now have branch selection dialogs if there is more than one branch head on the selected commit.
* Remove and checkout branch command patches handles a few more cases regarding remote branches and detached heads that I didn't think about originally. This has made them larger.
* I have split one commit, added another and moved some functionality around. Because of this the original patch number are no longer in sync with GitHub. How should I handle that?

On 2023-06-28 08:09, Johannes Sixt wrote:
> Please note that gitk-git directory is in its own repository that is
> only subtree-merged into the Git repository. You should generate patches
> against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it
> would be for Paul to integrate patches that were generated by gitgitgadget).

@Paul Mackerras: Paul, can you have a look at this? Can you accept a PR through GitGitGadget if I rebase it onto master for git.ozlabs.org/~paulus/gitk? Or do you have some other preferred way to receive patches?

Best regards,
Jens Lideström

On 2023-06-28 08:09, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> It is often convenient to use the keyboard to navigate the gitk GUI and
>> there are keyboard shortcut bindings for many operations such as searching
>> and scrolling. There is however no keyboard binding for the most common
>> operations on branches and commits: Check out, reset, cherry-pick, create
>> and delete branches.
>>
>> This PR adds keyboard bindings for these 5 commands. It also adjusts some
>> GUI focus defaults to simplify keyboard navigation.
>>
>> Some refactoring of the command implementation has been necessary.
>> Originally the commands was using the mouse context menu to get info about
>> the head and commit to act on. When using keyboard binds this information
>> isn't available so instead the row that is selected in the GUI is used. By
>> adding procedures for doing this the PR lays the groundwork for more similar
>> keyboard binds in the future.
> 
> I like it when an application can be navigated with the keyboard. These
> changes are very much appreciated.
> 
> I've left some comments on individual commits. The important one is that
> I think it makes the Reset dialog way too easy to destroy uncommitted work.
> 
> Please note that gitk-git directory is in its own repository that is
> only subtree-merged into the Git repository. You should generate patches
> against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it
> would be for Paul to integrate patches that were generated by gitgitgadget).
> 
> -- Hannes
> 
>>
>> I'm including Paul Mackerras because he seems to be the maintainer of gitk.
>> Can you review, Paul?
>>
>> Jens Lidestrom (9):
>>   gitk: add procedures to get commit info from selected row
>>   gitk: use term "current branch" in gui
>>   gitk: add keyboard bind for reset
>>   gitk: show branch name in reset dialog
>>   gitk: add keyboard bind for checkout
>>   gitk: add keyboard bind for create and remove branch
>>   gitk: add keyboard bind to cherry-pick
>>   gitk: focus ok button in reset dialog
>>   gitk: default select reset hard in dialog
>>
>>  gitk-git/gitk | 132 ++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 96 insertions(+), 36 deletions(-)
>>
>>
>> base-commit: 94486b6763c29144c60932829a65fec0597e17b3
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1551
> 

[PATCH v2 00/10] gitk: improve keyboard support

[Jens Lidestrom via GitGitGadget]

It is often convenient to use the keyboard to navigate the gitk GUI and
there are keyboard shortcut bindings for many operations such as searching
and scrolling. There is however no keyboard binding for the most common
operations on branches and commits: Check out, reset, cherry-pick, create
and delete branches.

This PR adds keyboard bindings for these 5 commands. It also adjusts some
GUI focus defaults to simplify keyboard navigation.

Some refactoring of the command implementation has been necessary.
Originally the commands was using the mouse context menu to get info about
the head and commit to act on. When using keyboard binds this information
isn't available so instead the row that is selected in the GUI is used. By
adding procedures for doing this the PR lays the groundwork for more similar
keyboard binds in the future.

I'm including Paul Mackerras because he seems to be the maintainer of gitk.
Can you review, Paul?

----------------------------------------------------------------------------

I have updated the PR after suggestions from Hannes. Mainly these changes
have been made:

 * The reset command dialog uses "mixed" as the default, but is more
   convenient to navigate with the keyboard.
 * Remove and checkout branch commands now have branch selection dialogs if
   there is more than one branch head on the selected commit.
 * Remove and checkout branch command patches handles a few more cases
   regarding remote branches and detached heads that I didn't think about
   originally. This has made them larger.
 * I have split one commit, added another and moved some functionality
   around. Because of this the original patch number are no longer in sync
   with GitHub. How should I handle that?

Jens Lidestrom (10):
  gitk: add procedures to get commit info from selected row
  gitk: use term "current branch" in gui
  gitk: add keyboard bind for reset command
  gitk: show branch name in reset dialog
  gitk: add keyboard bind for checkout command
  gitk: add keyboard bind for remove branch command
  gitk: add keyboard bind for cherry-pick command
  gitk: add keyboard bind for create branch command
  gitk: improve keyboard convenience in reset dialog
  gitk: allow checkout of remote branch

 gitk-git/gitk | 343 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 278 insertions(+), 65 deletions(-)


base-commit: 94486b6763c29144c60932829a65fec0597e17b3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1551

Range-diff vs v1:

  1:  cbbc1462f58 !  1:  063b5652c0e gitk: add procedures to get commit info from selected row
     @@ gitk-git/gitk: proc commitonrow {row} {
      +    return [commitonrow $selectedline]
      +}
      +
     -+# Gets the first branch name of the row that is selected in the GUI, or the
     -+# empty string if there is no branches on that commit.
     -+proc selected_line_head {} {
     ++# Gets all branche names on the commit that is selected in the GUI, or the
     ++# empty list if there is no branches on that commit.
     ++proc selected_line_heads {} {
      +    global idheads
      +    set id [selected_line_id]
      +    if {[info exists idheads($id)]} {
     -+        return [lindex $idheads($id) 0]
     ++        return $idheads($id)
      +    } else {
     -+        return ""
     ++        return {}
      +    }
      +}
      +
  2:  863fa3ee311 =  2:  f4ec77c1586 gitk: use term "current branch" in gui
  3:  2ba695bf94f !  3:  ca042ded0bb gitk: add keyboard bind for reset
     @@ Metadata
      Author: Jens Lidestrom <jens@lidestrom.se>
      
       ## Commit message ##
     -    gitk: add keyboard bind for reset
     +    gitk: add keyboard bind for reset command
      
          Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
      
  4:  8594e2cc68a !  4:  bd261e702f6 gitk: show branch name in reset dialog
     @@ Commit message
          Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
      
       ## gitk-git/gitk ##
     -@@ gitk-git/gitk: proc selected_line_head {} {
     +@@ gitk-git/gitk: proc selected_line_heads {} {
           }
       }
       
  5:  53e5a2e40ab !  5:  aaca07db597 gitk: add keyboard bind for checkout
     @@ Metadata
      Author: Jens Lidestrom <jens@lidestrom.se>
      
       ## Commit message ##
     -    gitk: add keyboard bind for checkout
     +    gitk: add keyboard bind for checkout command
      
          This also introduces the ability to check out detatched heads. This
          shouldn't result any problems, because gitk already works with
     @@ gitk-git/gitk: proc makewindow {} {
           bind $ctext <Button-1> {focus %W}
           bind $ctext <<Selection>> rehighlight_search_results
           bind . <$M1B-t> {resethead [selected_line_id]}
     -+    bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
     ++    bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
           for {set i 1} {$i < 10} {incr i} {
               bind . <$M1B-Key-$i> [list go_to_parent $i]
           }
      @@ gitk-git/gitk: proc makewindow {} {
     -         {mc "Create tag" command mktag}
     -         {mc "Copy commit reference" command copyreference}
     -         {mc "Write commit to file" command writecommit}
     --        {mc "Create new branch" command mkbranch}
     -+        {mc "Create new branch" command {mkbranch $rowmenuid}}
     -         {mc "Cherry-pick this commit" command cherrypick}
     -         {mc "Reset current branch to here" command {resethead $rowmenuid}}
     -         {mc "Mark this commit" command markhere}
     -@@ gitk-git/gitk: proc makewindow {} {
       
           set headctxmenu .headctxmenu
           makemenu $headctxmenu {
      -        {mc "Check out this branch" command cobranch}
     -+        {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
     ++        {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
               {mc "Rename this branch" command mvbranch}
               {mc "Remove this branch" command rmbranch}
               {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
     @@ gitk-git/gitk: proc headmenu {x y id head} {
       
      -proc cobranch {} {
      -    global headmenuid headmenuhead headids
     -+proc checkout {newhead newheadid} {
     -+    global headids
     ++proc checkout {heads_on_commit id_to_checkout} {
     ++    global headids mainhead
           global showlocalchanges
     ++    global sel_ix confirm_ok NS
       
           # check the tree is clean first??
      -    set newhead $headmenuhead
      +
     -+    # The ref is either the head, if it exists, or the ID
     -+    set newheadref [expr {$newhead ne "" ? $newhead : $newheadid}]
     ++    # Filter out remote branches if local branch is also present
     ++    foreach remote_ix [lsearch -all $heads_on_commit "remotes/*"] {
     ++        set remote_head [lindex $heads_on_commit $remote_ix]
     ++        set local_head [string range $remote_head [expr [string last / $remote_head] + 1] end]
     ++        if {$local_head in $heads_on_commit} {
     ++            set heads_on_commit [lreplace $heads_on_commit $remote_ix $remote_ix]
     ++        }
     ++    }
     ++
     ++    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
     ++        # Only the currently active branch
     ++        return
     ++    }
     ++
     ++    # Filter out mainhead
     ++    set mainhead_ix [lsearch $heads_on_commit $mainhead]
     ++    if {$mainhead_ix != -1} {
     ++        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
     ++    }
     ++    set nr_heads_on_commit [llength $heads_on_commit]
     ++
     ++    # The number of heads on the commit determines how to select what to checkout
     ++    if {$nr_heads_on_commit == 0} {
     ++        set head_to_checkout ""
     ++        set ref_to_checkout $id_to_checkout
     ++    } elseif {$nr_heads_on_commit == 1} {
     ++        set head_to_checkout [lindex $heads_on_commit 0]
     ++        set ref_to_checkout $head_to_checkout
     ++    } else {
     ++        # Branch selection dialog
     ++
     ++        set confirm_ok 0
     ++
     ++        set w ".selectbranch"
     ++        ttk_toplevel $w
     ++        make_transient $w .
     ++        wm title $w [mc "Check out branch"]
     ++        ${NS}::label $w.m -text [mc "Check out which branch?"]
     ++        pack $w.m -side top -fill x -padx 20 -pady 20
     ++        ${NS}::frame $w.f
     ++
     ++        set sel_ix 0
     ++        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
     ++            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
     ++                -text [lindex $heads_on_commit $i]
     ++            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
     ++            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
     ++            grid $w.f.id_$i -sticky w -padx 20
     ++        }
      +
     ++        pack $w.f -side top -fill x -padx 4
     ++        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
     ++        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
     ++        pack $w.ok -side left -fill x -padx 20 -pady 20
     ++        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
     ++        bind $w <Key-Escape> [list destroy $w]
     ++        pack $w.cancel -side right -fill x -padx 20 -pady 20
     ++        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
     ++
     ++        tkwait window $w
     ++
     ++        if {!$confirm_ok} return
     ++
     ++        set head_to_checkout [lindex $heads_on_commit $sel_ix]
     ++        set ref_to_checkout $head_to_checkout
     ++    }
     ++
     ++    # Handle remote branches
           set command [list | git checkout]
     -     if {[string match "remotes/*" $newhead]} {
     -         set remote $newhead
     -         set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
     +-    if {[string match "remotes/*" $newhead]} {
     +-        set remote $newhead
     +-        set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
      -        # The following check is redundant - the menu option should
      -        # be disabled to begin with...
     -         if {[info exists headids($newhead)]} {
     -             error_popup [mc "A local branch named %s exists already" $newhead]
     +-        if {[info exists headids($newhead)]} {
     +-            error_popup [mc "A local branch named %s exists already" $newhead]
     ++    if {[string match "remotes/*" $head_to_checkout]} {
     ++        set remote $head_to_checkout
     ++        set head_to_checkout [string range $head_to_checkout [expr [string last / $head_to_checkout] + 1] end]
     ++        set ref_to_checkout $head_to_checkout
     ++        if {[info exists headids($head_to_checkout)]} {
     ++            error_popup [mc "A local branch named %s exists already" $head_to_checkout]
                   return
               }
     -         lappend command -b $newhead --track $remote
     +-        lappend command -b $newhead --track $remote
     ++        lappend command -b $head_to_checkout --track $remote
           } else {
      -        lappend command $newhead
     -+        lappend command $newheadref
     ++        lappend command $ref_to_checkout
           }
           lappend command 2>@1
           nowbusy checkout [mc "Checking out"]
     @@ gitk-git/gitk: proc cobranch {} {
               }
           } else {
      -        filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
     -+        filerun $fd [list readcheckoutstat $fd $newhead $newheadref $newheadid]
     ++        filerun $fd [list readcheckoutstat $fd $head_to_checkout $ref_to_checkout $id_to_checkout]
           }
       }
       
      -proc readcheckoutstat {fd newhead newheadid} {
     -+proc readcheckoutstat {fd newhead newheadref newheadid} {
     ++proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
           global mainhead mainheadid headids idheads showlocalchanges progresscoords
           global viewmainheadid curview
       
      @@ gitk-git/gitk: proc readcheckoutstat {fd newhead newheadid} {
     +         error_popup $err
               return
           }
     -     set oldmainid $mainheadid
     +-    set oldmainid $mainheadid
      -    if {! [info exists headids($newhead)]} {
     +-        set headids($newhead) $newheadid
     +-        lappend idheads($newheadid) $newhead
     +-        addedhead $newheadid $newhead
     +-    }
     +-    set mainhead $newhead
     +-    set mainheadid $newheadid
     +-    set viewmainheadid($curview) $newheadid
     +-    redrawtags $oldmainid
     +-    redrawtags $newheadid
     +-    selbyid $newheadid
     ++    set old_main_id $mainheadid
      +
     -+    if {$newhead ne "" && ! [info exists headids($newhead)]} {
     -         set headids($newhead) $newheadid
     -         lappend idheads($newheadid) $newhead
     -         addedhead $newheadid $newhead
     ++    if {$head_to_checkout ne "" && ! [info exists headids($head_to_checkout)]} {
     ++        set headids($head_to_checkout) $id_to_checkout
     ++        lappend idheads($id_to_checkout) $head_to_checkout
     ++        addedhead $id_to_checkout $head_to_checkout
     ++    }
     ++    set mainhead $ref_to_checkout
     ++    set mainheadid $id_to_checkout
     ++    set viewmainheadid($curview) $id_to_checkout
     ++    redrawtags $old_main_id
     ++    redrawtags $id_to_checkout
     ++    selbyid $id_to_checkout
     +     if {$showlocalchanges} {
     +         dodiffindex
           }
     --    set mainhead $newhead
     -+    set mainhead $newheadref
     -     set mainheadid $newheadid
     -     set viewmainheadid($curview) $newheadid
     -     redrawtags $oldmainid
  6:  661f098d882 !  6:  ad6617a7bad gitk: add keyboard bind for create and remove branch
     @@ Metadata
      Author: Jens Lidestrom <jens@lidestrom.se>
      
       ## Commit message ##
     -    gitk: add keyboard bind for create and remove branch
     +    gitk: add keyboard bind for remove branch command
      
          Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
      
     @@ gitk-git/gitk
      @@ gitk-git/gitk: proc makewindow {} {
           bind $ctext <<Selection>> rehighlight_search_results
           bind . <$M1B-t> {resethead [selected_line_id]}
     -     bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
     -+    bind . <$M1B-m> {rmbranch [selected_line_head] [selected_line_id] 1}
     +     bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
     ++    bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}
      +    bind . <$M1B-b> {mkbranch [selected_line_id]}
           for {set i 1} {$i < 10} {incr i} {
               bind . <$M1B-Key-$i> [list go_to_parent $i]
           }
      @@ gitk-git/gitk: proc makewindow {} {
           makemenu $headctxmenu {
     -         {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
     +         {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
               {mc "Rename this branch" command mvbranch}
      -        {mc "Remove this branch" command rmbranch}
     -+        {mc "Remove this branch" command {rmbranch $headmenuhead $headmenuid 0}}
     ++        {mc "Remove this branch" command {rmbranch [list $headmenuhead] $headmenuid 0}}
               {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
           }
           $headctxmenu configure -tearoff 0
     @@ gitk-git/gitk: proc keys {} {
       [mc "<F5>		Update"]
       [mc "<%s-T>		Reset current branch to selected commit" $M1T]
       [mc "<%s-O>		Check out selected commit" $M1T]
     -+[mc "<%s-C>		Create branch on selected commit" $M1T]
     ++[mc "<%s-B>		Create branch on selected commit" $M1T]
      +[mc "<%s-M>		Remove selected branch" $M1T]
       " \
                   -justify left -bg $bgcolor -border 2 -relief groove
           pack $w.m -side top -fill both -padx 2 -pady 2
     -@@ gitk-git/gitk: proc wrcomcan {} {
     -     unset wrcomtop
     - }
     - 
     --proc mkbranch {} {
     --    global NS rowmenuid
     -+proc mkbranch {id} {
     -+    global NS
     - 
     -     set top .branchdialog
     - 
     -     set val(name) ""
     --    set val(id) $rowmenuid
     -+    set val(id) $id
     -     set val(command) [list mkbrgo $top]
     - 
     -     set ui(title) [mc "Create branch"]
     -@@ gitk-git/gitk: proc readcheckoutstat {fd newhead newheadref newheadid} {
     +@@ gitk-git/gitk: proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
           }
       }
       
      -proc rmbranch {} {
      -    global headmenuid headmenuhead mainhead
     -+proc rmbranch {head id shouldComfirm} {
     ++proc rmbranch {heads_on_commit id shouldComfirm} {
      +    global mainhead
           global idheads
     --
     ++    global confirm_ok sel_ix NS
     + 
      -    set head $headmenuhead
      -    set id $headmenuid
     -     # this check shouldn't be needed any more...
     -+    if {$head eq ""} {
     +-    # this check shouldn't be needed any more...
     +-    if {$head eq $mainhead} {
     ++    if {[llength $heads_on_commit] == 0} {
      +        error_popup [mc "Cannot delete a detached head"]
      +        return
      +    }
     -     if {$head eq $mainhead} {
     ++
     ++    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
               error_popup [mc "Cannot delete the currently checked-out branch"]
               return
     -@@ gitk-git/gitk: proc rmbranch {} {
     -         # the stuff on this branch isn't on any other branch
     -         if {![confirm_popup [mc "The commits on branch %s aren't on any other\
     -                         branch.\nReally delete branch %s?" $head $head]]} return
     -+    } elseif {$shouldComfirm} {
     -+        if {![confirm_popup [mc "Really delete branch %s?" $head]]} return
           }
     +-    set dheads [descheads $id]
     +-    if {[llength $dheads] == 1 && $idheads($dheads) eq $head} {
     +-        # the stuff on this branch isn't on any other branch
     +-        if {![confirm_popup [mc "The commits on branch %s aren't on any other\
     +-                        branch.\nReally delete branch %s?" $head $head]]} return
     ++
     ++    # Filter out mainhead
     ++    set mainhead_ix [lsearch $heads_on_commit $mainhead]
     ++    if {$mainhead_ix != -1} {
     ++        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
     ++    }
     ++
     ++    # Filter out remote branches
     ++    foreach head_ix [lsearch -all $heads_on_commit "remotes/*"] {
     ++        set heads_on_commit [lreplace $heads_on_commit $head_ix $head_ix]
     ++    }
     ++
     ++    if {[llength $heads_on_commit] == 0} {
     ++        # Probably only current branch and its remote branch on commit
     ++        error_popup [mc "Cannot delete branch"]
     ++        return
     +     }
     ++
     ++    set nr_heads_on_commit [llength $heads_on_commit]
     ++    set first_head [lindex $heads_on_commit 0]
     ++
     ++    if {$nr_heads_on_commit == 1} {
     ++        # Only a single head, simple comfirm dialogs
     ++
     ++        set head_to_remove $first_head
     ++        set dheads [descheads $id]
     ++
     ++        if {[llength $dheads] == 1 && $idheads($dheads) eq $head_to_remove} {
     ++            # the stuff on this branch isn't on any other branch
     ++            if {![confirm_popup [mc "The commits on branch %s aren't on any other\
     ++                            branch.\nReally delete branch %s?" $head_to_remove $head_to_remove]]} return
     ++        } elseif {$shouldComfirm} {
     ++            if {![confirm_popup [mc "Really delete branch %s?" $head_to_remove]]} return
     ++        }
     ++    } else {
     ++        # Select branch dialog
     ++
     ++        set confirm_ok 0
     ++
     ++        set w ".selectbranch"
     ++        ttk_toplevel $w
     ++        make_transient $w .
     ++        wm title $w [mc "Delete branch"]
     ++        ${NS}::label $w.m -text [mc "Which branch to delete?"]
     ++        pack $w.m -side top -fill x -padx 20 -pady 20
     ++        ${NS}::frame $w.f
     ++
     ++        set sel_ix 0
     ++        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
     ++            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
     ++                -text [lindex $heads_on_commit $i]
     ++            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
     ++            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
     ++            grid $w.f.id_$i -sticky w -padx 20
     ++        }
     ++
     ++        pack $w.f -side top -fill x -padx 4
     ++        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
     ++        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
     ++        pack $w.ok -side left -fill x -padx 20 -pady 20
     ++        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
     ++        bind $w <Key-Escape> [list destroy $w]
     ++        pack $w.cancel -side right -fill x -padx 20 -pady 20
     ++        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
     ++
     ++        tkwait window $w
     ++        if {!$confirm_ok} return
     ++
     ++        set head_to_remove [lindex $heads_on_commit $sel_ix]
     ++    }
     ++
     ++    # Perform the command
     ++
           nowbusy rmbranch
           update
     +-    if {[catch {exec git branch -D $head} err]} {
     ++    if {[catch {exec git branch -D $head_to_remove} err]} {
     +         notbusy rmbranch
     +         error_popup $err
     +         return
     +     }
     +-    removehead $id $head
     +-    removedhead $id $head
     ++    removehead $id $head_to_remove
     ++    removedhead $id $head_to_remove
     +     redrawtags $id
     +     notbusy rmbranch
     +     dispneartags 0
  7:  46cbbaa6fe2 !  7:  54afa8fe9e8 gitk: add keyboard bind to cherry-pick
     @@ Metadata
      Author: Jens Lidestrom <jens@lidestrom.se>
      
       ## Commit message ##
     -    gitk: add keyboard bind to cherry-pick
     +    gitk: add keyboard bind for cherry-pick command
      
          Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
      
     @@ gitk-git/gitk: proc makewindow {} {
           bind $ctext <<Selection>> rehighlight_search_results
      +    bind . <$M1B-p> {cherrypick [selected_line_id]}
           bind . <$M1B-t> {resethead [selected_line_id]}
     -     bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
     -     bind . <$M1B-m> {rmbranch [selected_line_head] [selected_line_id] 1}
     +     bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
     +     bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}
      @@ gitk-git/gitk: proc makewindow {} {
     +         {mc "Create tag" command mktag}
               {mc "Copy commit reference" command copyreference}
               {mc "Write commit to file" command writecommit}
     -         {mc "Create new branch" command {mkbranch $rowmenuid}}
     +-        {mc "Create new branch" command mkbranch}
      -        {mc "Cherry-pick this commit" command cherrypick}
     ++        {mc "Create new branch" command {mkbranch $rowmenuid}}
      +        {mc "Cherry-pick this commit" command {cherrypick $rowmenuid}}
               {mc "Reset current branch to here" command {resethead $rowmenuid}}
               {mc "Mark this commit" command markhere}
     @@ gitk-git/gitk: proc keys {} {
       [mc "<%s-T>		Reset current branch to selected commit" $M1T]
      +[mc "<%s-P>		Cherry-pick selected commit to current branch" $M1T]
       [mc "<%s-O>		Check out selected commit" $M1T]
     - [mc "<%s-C>		Create branch on selected commit" $M1T]
     + [mc "<%s-B>		Create branch on selected commit" $M1T]
       [mc "<%s-M>		Remove selected branch" $M1T]
      @@ gitk-git/gitk: proc exec_citool {tool_args {baseid {}}} {
           array set env $save_env
  -:  ----------- >  8:  bd498f5b326 gitk: add keyboard bind for create branch command
  8:  49cdbff270f !  9:  a37a677036d gitk: focus ok button in reset dialog
     @@ Metadata
      Author: Jens Lidestrom <jens@lidestrom.se>
      
       ## Commit message ##
     -    gitk: focus ok button in reset dialog
     +    gitk: improve keyboard convenience in reset dialog
      
     -    This is more convenient for users, especially when using keyboard
     -    commands.
     +    Make it more convenient to use the reset dialog using keyboard.
     +
     +    * Set focus to the combo box.
     +    * Accept with Return key.
     +    * Auto-select combo items when navigating in menu with up/down keys.
      
          Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
      
       ## gitk-git/gitk ##
      @@ gitk-git/gitk: proc resethead {reset_target_id} {
     +     ttk_toplevel $w
     +     make_transient $w .
     +     wm title $w [mc "Confirm reset"]
     ++
     +     ${NS}::label $w.m -text \
     +         [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
     +     pack $w.m -side top -fill x -padx 20 -pady 20
     +     ${NS}::labelframe $w.f -text [mc "Reset type:"]
     ++
     +     set resettype mixed
     ++
     +     ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
     +         -text [mc "Soft: Leave working tree and index untouched"]
     ++    bind $w.f.soft <Key-Up> "set resettype hard"
     ++    bind $w.f.soft <Key-Down> "set resettype mixed"
     +     grid $w.f.soft -sticky w
     ++
     +     ${NS}::radiobutton $w.f.mixed -value mixed -variable resettype \
     +         -text [mc "Mixed: Leave working tree untouched, reset index"]
     ++    bind $w.f.mixed <Key-Up> "set resettype soft"
     ++    bind $w.f.mixed <Key-Down> "set resettype hard"
     +     grid $w.f.mixed -sticky w
     ++
     +     ${NS}::radiobutton $w.f.hard -value hard -variable resettype \
     +         -text [mc "Hard: Reset working tree and index\n(discard ALL local changes)"]
     ++    bind $w.f.hard <Key-Up> "set resettype mixed"
     ++    bind $w.f.hard <Key-Down> "set resettype soft"
     +     grid $w.f.hard -sticky w
     +     pack $w.f -side top -fill x -padx 4
     ++
     +     ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
     ++    bind $w <Key-Return> "set confirm_ok 1; destroy $w"
     +     pack $w.ok -side left -fill x -padx 20 -pady 20
           ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
           bind $w <Key-Escape> [list destroy $w]
           pack $w.cancel -side right -fill x -padx 20 -pady 20
      -    bind $w <Visibility> "grab $w; focus $w"
     -+    bind $w <Visibility> "grab $w; focus $w.ok"
     ++    bind $w <Visibility> "grab $w; focus $w.f.mixed"
           tkwait window $w
           if {!$confirm_ok} return
           if {[catch {set fd [open \
  9:  97857c3509f <  -:  ----------- gitk: default select reset hard in dialog
  -:  ----------- > 10:  3f0438fc027 gitk: allow checkout of remote branch

-- 
gitgitgadget

[PATCH v2 01/10] gitk: add procedures to get commit info from selected row

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

These procedures are useful for implementing keyboard commands. Keyboard
commands don't have access to commits that are selected by the mouse and
hence must get info from the selected row.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index df3ba2ea99b..6d12e04ca64 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -1333,6 +1333,24 @@ proc commitonrow {row} {
     return $id
 }
 
+# Get commits ID of the row that is selected in the GUI
+proc selected_line_id {} {
+    global selectedline
+    return [commitonrow $selectedline]
+}
+
+# Gets all branche names on the commit that is selected in the GUI, or the
+# empty list if there is no branches on that commit.
+proc selected_line_heads {} {
+    global idheads
+    set id [selected_line_id]
+    if {[info exists idheads($id)]} {
+        return $idheads($id)
+    } else {
+        return {}
+    }
+}
+
 proc closevarcs {v} {
     global varctok varccommits varcid parents children
     global cmitlisted commitidx vtokmod curview numcommits
-- 
gitgitgadget

[PATCH v2 02/10] gitk: use term "current branch" in gui

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

This is the term that is used in the official documentation. Instead of
"HEAD branch" which in not standard.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 6d12e04ca64..fab21d8cfbc 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2693,7 +2693,7 @@ proc makewindow {} {
         {mc "Write commit to file" command writecommit}
         {mc "Create new branch" command mkbranch}
         {mc "Cherry-pick this commit" command cherrypick}
-        {mc "Reset HEAD branch to here" command resethead}
+        {mc "Reset current branch to here" command resethead}
         {mc "Mark this commit" command markhere}
         {mc "Return to mark" command gotomark}
         {mc "Find descendant of this and mark" command find_common_desc}
-- 
gitgitgadget

[PATCH v2 03/10] gitk: add keyboard bind for reset command

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index fab21d8cfbc..3d4bfa5f1d8 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2675,6 +2675,7 @@ proc makewindow {} {
     bind $ctext $ctxbut {pop_diff_menu %W %X %Y %x %y}
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
+    bind . <$M1B-t> {resethead [selected_line_id]}
     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }
@@ -2693,7 +2694,7 @@ proc makewindow {} {
         {mc "Write commit to file" command writecommit}
         {mc "Create new branch" command mkbranch}
         {mc "Cherry-pick this commit" command cherrypick}
-        {mc "Reset current branch to here" command resethead}
+        {mc "Reset current branch to here" command {resethead $rowmenuid}}
         {mc "Mark this commit" command markhere}
         {mc "Return to mark" command gotomark}
         {mc "Find descendant of this and mark" command find_common_desc}
@@ -3166,6 +3167,7 @@ proc keys {} {
 [mc "<%s-KP->	Decrease font size" $M1T]
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
+[mc "<%s-T>		Reset current branch to selected commit" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2
@@ -9859,8 +9861,13 @@ proc revert {} {
     notbusy revert
 }
 
-proc resethead {} {
-    global mainhead rowmenuid confirm_ok resettype NS
+proc resethead {reset_target_id} {
+    global headids mainhead confirm_ok resettype NS
+
+    if {! [info exists headids($mainhead)]} {
+        error_popup [mc "Cannot reset a detached head"]
+        return
+    }
 
     set confirm_ok 0
     set w ".confirmreset"
@@ -9868,7 +9875,7 @@ proc resethead {} {
     make_transient $w .
     wm title $w [mc "Confirm reset"]
     ${NS}::label $w.m -text \
-        [mc "Reset branch %s to %s?" $mainhead [string range $rowmenuid 0 7]]
+        [mc "Reset branch %s to %s?" $mainhead [string range $reset_target_id 0 7]]
     pack $w.m -side top -fill x -padx 20 -pady 20
     ${NS}::labelframe $w.f -text [mc "Reset type:"]
     set resettype mixed
@@ -9891,13 +9898,13 @@ proc resethead {} {
     tkwait window $w
     if {!$confirm_ok} return
     if {[catch {set fd [open \
-            [list | git reset --$resettype $rowmenuid 2>@1] r]} err]} {
+            [list | git reset --$resettype $reset_target_id 2>@1] r]} err]} {
         error_popup $err
     } else {
         dohidelocalchanges
         filerun $fd [list readresetstat $fd]
         nowbusy reset [mc "Resetting"]
-        selbyid $rowmenuid
+        selbyid $reset_target_id
     }
 }
 
-- 
gitgitgadget

[PATCH v2 04/10] gitk: show branch name in reset dialog

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 3d4bfa5f1d8..642cd7f652a 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -1351,6 +1351,21 @@ proc selected_line_heads {} {
     }
 }
 
+# If any branch is associated with the argument ID then return that. Otherwise
+# return the ID itself. Useful for displaying the best name of a commit.
+proc commit_name {id should_shorten_id} {
+    global idheads idtags
+    if {[info exists idheads($id)]} {
+        return [lindex $idheads($id) 0]
+    } elseif {[info exists idtags($id)]} {
+        return [lindex $idtags($id) 0]
+    } elseif {$should_shorten_id} {
+        return [string range $id 0 7]
+    } else {
+        return $id
+    }
+}
+
 proc closevarcs {v} {
     global varctok varccommits varcid parents children
     global cmitlisted commitidx vtokmod curview numcommits
@@ -9875,7 +9890,7 @@ proc resethead {reset_target_id} {
     make_transient $w .
     wm title $w [mc "Confirm reset"]
     ${NS}::label $w.m -text \
-        [mc "Reset branch %s to %s?" $mainhead [string range $reset_target_id 0 7]]
+        [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
     pack $w.m -side top -fill x -padx 20 -pady 20
     ${NS}::labelframe $w.f -text [mc "Reset type:"]
     set resettype mixed
-- 
gitgitgadget

[PATCH v2 05/10] gitk: add keyboard bind for checkout command

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

This also introduces the ability to check out detatched heads. This
shouldn't result any problems, because gitk already works with
detatched heads if they are checked out using the terminal.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 125 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 98 insertions(+), 27 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 642cd7f652a..8364033ad58 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2691,6 +2691,7 @@ proc makewindow {} {
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
     bind . <$M1B-t> {resethead [selected_line_id]}
+    bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }
@@ -2732,7 +2733,7 @@ proc makewindow {} {
 
     set headctxmenu .headctxmenu
     makemenu $headctxmenu {
-        {mc "Check out this branch" command cobranch}
+        {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
         {mc "Rename this branch" command mvbranch}
         {mc "Remove this branch" command rmbranch}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
@@ -3183,6 +3184,7 @@ proc keys {} {
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
+[mc "<%s-O>		Check out selected commit" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2
@@ -9978,25 +9980,93 @@ proc headmenu {x y id head} {
     tk_popup $headctxmenu $x $y
 }
 
-proc cobranch {} {
-    global headmenuid headmenuhead headids
+proc checkout {heads_on_commit id_to_checkout} {
+    global headids mainhead
     global showlocalchanges
+    global sel_ix confirm_ok NS
 
     # check the tree is clean first??
-    set newhead $headmenuhead
+
+    # Filter out remote branches if local branch is also present
+    foreach remote_ix [lsearch -all $heads_on_commit "remotes/*"] {
+        set remote_head [lindex $heads_on_commit $remote_ix]
+        set local_head [string range $remote_head [expr [string last / $remote_head] + 1] end]
+        if {$local_head in $heads_on_commit} {
+            set heads_on_commit [lreplace $heads_on_commit $remote_ix $remote_ix]
+        }
+    }
+
+    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
+        # Only the currently active branch
+        return
+    }
+
+    # Filter out mainhead
+    set mainhead_ix [lsearch $heads_on_commit $mainhead]
+    if {$mainhead_ix != -1} {
+        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
+    }
+    set nr_heads_on_commit [llength $heads_on_commit]
+
+    # The number of heads on the commit determines how to select what to checkout
+    if {$nr_heads_on_commit == 0} {
+        set head_to_checkout ""
+        set ref_to_checkout $id_to_checkout
+    } elseif {$nr_heads_on_commit == 1} {
+        set head_to_checkout [lindex $heads_on_commit 0]
+        set ref_to_checkout $head_to_checkout
+    } else {
+        # Branch selection dialog
+
+        set confirm_ok 0
+
+        set w ".selectbranch"
+        ttk_toplevel $w
+        make_transient $w .
+        wm title $w [mc "Check out branch"]
+        ${NS}::label $w.m -text [mc "Check out which branch?"]
+        pack $w.m -side top -fill x -padx 20 -pady 20
+        ${NS}::frame $w.f
+
+        set sel_ix 0
+        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
+            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
+                -text [lindex $heads_on_commit $i]
+            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
+            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
+            grid $w.f.id_$i -sticky w -padx 20
+        }
+
+        pack $w.f -side top -fill x -padx 4
+        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
+        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
+        pack $w.ok -side left -fill x -padx 20 -pady 20
+        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
+        bind $w <Key-Escape> [list destroy $w]
+        pack $w.cancel -side right -fill x -padx 20 -pady 20
+        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
+
+        tkwait window $w
+
+        if {!$confirm_ok} return
+
+        set head_to_checkout [lindex $heads_on_commit $sel_ix]
+        set ref_to_checkout $head_to_checkout
+    }
+
+    # Handle remote branches
     set command [list | git checkout]
-    if {[string match "remotes/*" $newhead]} {
-        set remote $newhead
-        set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
-        # The following check is redundant - the menu option should
-        # be disabled to begin with...
-        if {[info exists headids($newhead)]} {
-            error_popup [mc "A local branch named %s exists already" $newhead]
+    if {[string match "remotes/*" $head_to_checkout]} {
+        set remote $head_to_checkout
+        set head_to_checkout [string range $head_to_checkout [expr [string last / $head_to_checkout] + 1] end]
+        set ref_to_checkout $head_to_checkout
+        if {[info exists headids($head_to_checkout)]} {
+            error_popup [mc "A local branch named %s exists already" $head_to_checkout]
             return
         }
-        lappend command -b $newhead --track $remote
+        lappend command -b $head_to_checkout --track $remote
     } else {
-        lappend command $newhead
+        lappend command $ref_to_checkout
     }
     lappend command 2>@1
     nowbusy checkout [mc "Checking out"]
@@ -10011,11 +10081,11 @@ proc cobranch {} {
             dodiffindex
         }
     } else {
-        filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
+        filerun $fd [list readcheckoutstat $fd $head_to_checkout $ref_to_checkout $id_to_checkout]
     }
 }
 
-proc readcheckoutstat {fd newhead newheadid} {
+proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
     global mainhead mainheadid headids idheads showlocalchanges progresscoords
     global viewmainheadid curview
 
@@ -10033,18 +10103,19 @@ proc readcheckoutstat {fd newhead newheadid} {
         error_popup $err
         return
     }
-    set oldmainid $mainheadid
-    if {! [info exists headids($newhead)]} {
-        set headids($newhead) $newheadid
-        lappend idheads($newheadid) $newhead
-        addedhead $newheadid $newhead
-    }
-    set mainhead $newhead
-    set mainheadid $newheadid
-    set viewmainheadid($curview) $newheadid
-    redrawtags $oldmainid
-    redrawtags $newheadid
-    selbyid $newheadid
+    set old_main_id $mainheadid
+
+    if {$head_to_checkout ne "" && ! [info exists headids($head_to_checkout)]} {
+        set headids($head_to_checkout) $id_to_checkout
+        lappend idheads($id_to_checkout) $head_to_checkout
+        addedhead $id_to_checkout $head_to_checkout
+    }
+    set mainhead $ref_to_checkout
+    set mainheadid $id_to_checkout
+    set viewmainheadid($curview) $id_to_checkout
+    redrawtags $old_main_id
+    redrawtags $id_to_checkout
+    selbyid $id_to_checkout
     if {$showlocalchanges} {
         dodiffindex
     }
-- 
gitgitgadget

[PATCH v2 06/10] gitk: add keyboard bind for remove branch command

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 104 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 89 insertions(+), 15 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 8364033ad58..65ca11becca 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2692,6 +2692,8 @@ proc makewindow {} {
     bind $ctext <<Selection>> rehighlight_search_results
     bind . <$M1B-t> {resethead [selected_line_id]}
     bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
+    bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}
+    bind . <$M1B-b> {mkbranch [selected_line_id]}
     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }
@@ -2735,7 +2737,7 @@ proc makewindow {} {
     makemenu $headctxmenu {
         {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
         {mc "Rename this branch" command mvbranch}
-        {mc "Remove this branch" command rmbranch}
+        {mc "Remove this branch" command {rmbranch [list $headmenuhead] $headmenuid 0}}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
     }
     $headctxmenu configure -tearoff 0
@@ -3185,6 +3187,8 @@ proc keys {} {
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
 [mc "<%s-O>		Check out selected commit" $M1T]
+[mc "<%s-B>		Create branch on selected commit" $M1T]
+[mc "<%s-M>		Remove selected branch" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2
@@ -10121,32 +10125,102 @@ proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
     }
 }
 
-proc rmbranch {} {
-    global headmenuid headmenuhead mainhead
+proc rmbranch {heads_on_commit id shouldComfirm} {
+    global mainhead
     global idheads
+    global confirm_ok sel_ix NS
 
-    set head $headmenuhead
-    set id $headmenuid
-    # this check shouldn't be needed any more...
-    if {$head eq $mainhead} {
+    if {[llength $heads_on_commit] == 0} {
+        error_popup [mc "Cannot delete a detached head"]
+        return
+    }
+
+    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
         error_popup [mc "Cannot delete the currently checked-out branch"]
         return
     }
-    set dheads [descheads $id]
-    if {[llength $dheads] == 1 && $idheads($dheads) eq $head} {
-        # the stuff on this branch isn't on any other branch
-        if {![confirm_popup [mc "The commits on branch %s aren't on any other\
-                        branch.\nReally delete branch %s?" $head $head]]} return
+
+    # Filter out mainhead
+    set mainhead_ix [lsearch $heads_on_commit $mainhead]
+    if {$mainhead_ix != -1} {
+        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
+    }
+
+    # Filter out remote branches
+    foreach head_ix [lsearch -all $heads_on_commit "remotes/*"] {
+        set heads_on_commit [lreplace $heads_on_commit $head_ix $head_ix]
+    }
+
+    if {[llength $heads_on_commit] == 0} {
+        # Probably only current branch and its remote branch on commit
+        error_popup [mc "Cannot delete branch"]
+        return
     }
+
+    set nr_heads_on_commit [llength $heads_on_commit]
+    set first_head [lindex $heads_on_commit 0]
+
+    if {$nr_heads_on_commit == 1} {
+        # Only a single head, simple comfirm dialogs
+
+        set head_to_remove $first_head
+        set dheads [descheads $id]
+
+        if {[llength $dheads] == 1 && $idheads($dheads) eq $head_to_remove} {
+            # the stuff on this branch isn't on any other branch
+            if {![confirm_popup [mc "The commits on branch %s aren't on any other\
+                            branch.\nReally delete branch %s?" $head_to_remove $head_to_remove]]} return
+        } elseif {$shouldComfirm} {
+            if {![confirm_popup [mc "Really delete branch %s?" $head_to_remove]]} return
+        }
+    } else {
+        # Select branch dialog
+
+        set confirm_ok 0
+
+        set w ".selectbranch"
+        ttk_toplevel $w
+        make_transient $w .
+        wm title $w [mc "Delete branch"]
+        ${NS}::label $w.m -text [mc "Which branch to delete?"]
+        pack $w.m -side top -fill x -padx 20 -pady 20
+        ${NS}::frame $w.f
+
+        set sel_ix 0
+        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
+            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
+                -text [lindex $heads_on_commit $i]
+            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
+            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
+            grid $w.f.id_$i -sticky w -padx 20
+        }
+
+        pack $w.f -side top -fill x -padx 4
+        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
+        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
+        pack $w.ok -side left -fill x -padx 20 -pady 20
+        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
+        bind $w <Key-Escape> [list destroy $w]
+        pack $w.cancel -side right -fill x -padx 20 -pady 20
+        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
+
+        tkwait window $w
+        if {!$confirm_ok} return
+
+        set head_to_remove [lindex $heads_on_commit $sel_ix]
+    }
+
+    # Perform the command
+
     nowbusy rmbranch
     update
-    if {[catch {exec git branch -D $head} err]} {
+    if {[catch {exec git branch -D $head_to_remove} err]} {
         notbusy rmbranch
         error_popup $err
         return
     }
-    removehead $id $head
-    removedhead $id $head
+    removehead $id $head_to_remove
+    removedhead $id $head_to_remove
     redrawtags $id
     notbusy rmbranch
     dispneartags 0
-- 
gitgitgadget

[PATCH v2 07/10] gitk: add keyboard bind for cherry-pick command

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 65ca11becca..351b88f10c0 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2690,6 +2690,7 @@ proc makewindow {} {
     bind $ctext $ctxbut {pop_diff_menu %W %X %Y %x %y}
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
+    bind . <$M1B-p> {cherrypick [selected_line_id]}
     bind . <$M1B-t> {resethead [selected_line_id]}
     bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
     bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}
@@ -2710,8 +2711,8 @@ proc makewindow {} {
         {mc "Create tag" command mktag}
         {mc "Copy commit reference" command copyreference}
         {mc "Write commit to file" command writecommit}
-        {mc "Create new branch" command mkbranch}
-        {mc "Cherry-pick this commit" command cherrypick}
+        {mc "Create new branch" command {mkbranch $rowmenuid}}
+        {mc "Cherry-pick this commit" command {cherrypick $rowmenuid}}
         {mc "Reset current branch to here" command {resethead $rowmenuid}}
         {mc "Mark this commit" command markhere}
         {mc "Return to mark" command gotomark}
@@ -3186,6 +3187,7 @@ proc keys {} {
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
+[mc "<%s-P>		Cherry-pick selected commit to current branch" $M1T]
 [mc "<%s-O>		Check out selected commit" $M1T]
 [mc "<%s-B>		Create branch on selected commit" $M1T]
 [mc "<%s-M>		Remove selected branch" $M1T]
@@ -9758,24 +9760,29 @@ proc exec_citool {tool_args {baseid {}}} {
     array set env $save_env
 }
 
-proc cherrypick {} {
-    global rowmenuid curview
+proc cherrypick {id} {
+    global curview headids
     global mainhead mainheadid
     global gitdir
 
+    if {! [info exists headids($mainhead)]} {
+        error_popup [mc "Cannot cherry-pick to a detached head"]
+        return
+    }
+
     set oldhead [exec git rev-parse HEAD]
-    set dheads [descheads $rowmenuid]
+    set dheads [descheads $id]
     if {$dheads ne {} && [lsearch -exact $dheads $oldhead] >= 0} {
         set ok [confirm_popup [mc "Commit %s is already\
                 included in branch %s -- really re-apply it?" \
-                                   [string range $rowmenuid 0 7] $mainhead]]
+                                   [string range $id 0 7] $mainhead]]
         if {!$ok} return
     }
     nowbusy cherrypick [mc "Cherry-picking"]
     update
     # Unfortunately git-cherry-pick writes stuff to stderr even when
     # no error occurs, and exec takes that as an indication of error...
-    if {[catch {exec sh -c "git cherry-pick -r $rowmenuid 2>&1"} err]} {
+    if {[catch {exec sh -c "git cherry-pick -r $id 2>&1"} err]} {
         notbusy cherrypick
         if {[regexp -line \
                  {Entry '(.*)' (would be overwritten by merge|not uptodate)} \
@@ -9791,7 +9798,7 @@ proc cherrypick {} {
                         resolve it?"]]} {
                 # Force citool to read MERGE_MSG
                 file delete [file join $gitdir "GITGUI_MSG"]
-                exec_citool {} $rowmenuid
+                exec_citool {} $id
             }
         } else {
             error_popup $err
-- 
gitgitgadget

[PATCH v2 08/10] gitk: add keyboard bind for create branch command

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 351b88f10c0..f559e279b7a 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -9582,13 +9582,13 @@ proc wrcomcan {} {
     unset wrcomtop
 }
 
-proc mkbranch {} {
-    global NS rowmenuid
+proc mkbranch {id} {
+    global NS
 
     set top .branchdialog
 
     set val(name) ""
-    set val(id) $rowmenuid
+    set val(id) $id
     set val(command) [list mkbrgo $top]
 
     set ui(title) [mc "Create branch"]
-- 
gitgitgadget

[PATCH v2 09/10] gitk: improve keyboard convenience in reset dialog

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

Make it more convenient to use the reset dialog using keyboard.

* Set focus to the combo box.
* Accept with Return key.
* Auto-select combo items when navigating in menu with up/down keys.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index f559e279b7a..fafff2b1a5b 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -9902,27 +9902,40 @@ proc resethead {reset_target_id} {
     ttk_toplevel $w
     make_transient $w .
     wm title $w [mc "Confirm reset"]
+
     ${NS}::label $w.m -text \
         [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
     pack $w.m -side top -fill x -padx 20 -pady 20
     ${NS}::labelframe $w.f -text [mc "Reset type:"]
+
     set resettype mixed
+
     ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
         -text [mc "Soft: Leave working tree and index untouched"]
+    bind $w.f.soft <Key-Up> "set resettype hard"
+    bind $w.f.soft <Key-Down> "set resettype mixed"
     grid $w.f.soft -sticky w
+
     ${NS}::radiobutton $w.f.mixed -value mixed -variable resettype \
         -text [mc "Mixed: Leave working tree untouched, reset index"]
+    bind $w.f.mixed <Key-Up> "set resettype soft"
+    bind $w.f.mixed <Key-Down> "set resettype hard"
     grid $w.f.mixed -sticky w
+
     ${NS}::radiobutton $w.f.hard -value hard -variable resettype \
         -text [mc "Hard: Reset working tree and index\n(discard ALL local changes)"]
+    bind $w.f.hard <Key-Up> "set resettype mixed"
+    bind $w.f.hard <Key-Down> "set resettype soft"
     grid $w.f.hard -sticky w
     pack $w.f -side top -fill x -padx 4
+
     ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
+    bind $w <Key-Return> "set confirm_ok 1; destroy $w"
     pack $w.ok -side left -fill x -padx 20 -pady 20
     ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
     bind $w <Key-Escape> [list destroy $w]
     pack $w.cancel -side right -fill x -padx 20 -pady 20
-    bind $w <Visibility> "grab $w; focus $w"
+    bind $w <Visibility> "grab $w; focus $w.f.mixed"
     tkwait window $w
     if {!$confirm_ok} return
     if {[catch {set fd [open \
-- 
gitgitgadget

[PATCH v2 10/10] gitk: allow checkout of remote branch

[Jens Lidestrom via GitGitGadget]

From: Jens Lidestrom <jens@lidestrom.se>

This is a consequence of allowing checkout of detached heads (through
keyboard shortcut). It doesn't make sense to disallow checkout of
commits on remote branches if all other commits can be checked out.

Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
---
 gitk-git/gitk | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index fafff2b1a5b..99f93371482 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -9989,10 +9989,6 @@ proc headmenu {x y id head} {
     set headmenuhead $head
     array set state {0 normal 1 normal 2 normal}
     if {[string match "remotes/*" $head]} {
-        set localhead [string range $head [expr [string last / $head] + 1] end]
-        if {[info exists headids($localhead)]} {
-            set state(0) disabled
-        }
         array set state {1 disabled 2 disabled}
     }
     if {$head eq $mainhead} {
@@ -10078,17 +10074,29 @@ proc checkout {heads_on_commit id_to_checkout} {
         set ref_to_checkout $head_to_checkout
     }
 
-    # Handle remote branches
     set command [list | git checkout]
     if {[string match "remotes/*" $head_to_checkout]} {
-        set remote $head_to_checkout
+        # Handle remote branches
+        set remote_head $head_to_checkout
         set head_to_checkout [string range $head_to_checkout [expr [string last / $head_to_checkout] + 1] end]
-        set ref_to_checkout $head_to_checkout
         if {[info exists headids($head_to_checkout)]} {
-            error_popup [mc "A local branch named %s exists already" $head_to_checkout]
-            return
+            # The local branch exists
+            set id_to_checkout $headids($remote_head)
+            if {$headids($head_to_checkout) eq $id_to_checkout} {
+                # Remote and local are at the same commit
+                set ref_to_checkout $head_to_checkout
+            } else {
+                # Check out the detached head of the remote branch
+                set head_to_checkout ""
+                set ref_to_checkout $id_to_checkout
+            }
+            lappend command $ref_to_checkout
+        } else {
+            # Create local branch and checkout
+            set ref_to_checkout $head_to_checkout
+            puts $ref_to_checkout
+            lappend command -b $ref_to_checkout --track $remote_head
         }
-        lappend command -b $head_to_checkout --track $remote
     } else {
         lappend command $ref_to_checkout
     }
-- 
gitgitgadget

Re: [PATCH v2 05/10] gitk: add keyboard bind for checkout command

[Johannes Sixt]

Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:
> From: Jens Lidestrom <jens@lidestrom.se>
> 
> This also introduces the ability to check out detatched heads. This
> shouldn't result any problems, because gitk already works with
> detatched heads if they are checked out using the terminal.

I find it irritating that it is possible to check out a detached head
with Ctrl-O, but not via the context menu.

Playing around a bit with this feature, it may be a bit too easy to
check out a different commit with a simple key press and without
confirmation. At a minimum, there should be a confirmation when a
transition from a branch to a detached head happens, because otherwise
inexperienced users end up in inconvenient territories.

I propose that you do not include the ability to check out a detached
head in this commit, but make it a new feature once this series has settled.

> 
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
>  gitk-git/gitk | 125 +++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 98 insertions(+), 27 deletions(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 642cd7f652a..8364033ad58 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -2691,6 +2691,7 @@ proc makewindow {} {
>      bind $ctext <Button-1> {focus %W}
>      bind $ctext <<Selection>> rehighlight_search_results
>      bind . <$M1B-t> {resethead [selected_line_id]}
> +    bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
>      for {set i 1} {$i < 10} {incr i} {
>          bind . <$M1B-Key-$i> [list go_to_parent $i]
>      }
> @@ -2732,7 +2733,7 @@ proc makewindow {} {
>  
>      set headctxmenu .headctxmenu
>      makemenu $headctxmenu {
> -        {mc "Check out this branch" command cobranch}
> +        {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
>          {mc "Rename this branch" command mvbranch}
>          {mc "Remove this branch" command rmbranch}
>          {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
> @@ -3183,6 +3184,7 @@ proc keys {} {
>  [mc "<%s-minus>	Decrease font size" $M1T]
>  [mc "<F5>		Update"]
>  [mc "<%s-T>		Reset current branch to selected commit" $M1T]
> +[mc "<%s-O>		Check out selected commit" $M1T]
>  " \
>              -justify left -bg $bgcolor -border 2 -relief groove
>      pack $w.m -side top -fill both -padx 2 -pady 2
> @@ -9978,25 +9980,93 @@ proc headmenu {x y id head} {
>      tk_popup $headctxmenu $x $y
>  }
>  
> -proc cobranch {} {
> -    global headmenuid headmenuhead headids
> +proc checkout {heads_on_commit id_to_checkout} {
> +    global headids mainhead
>      global showlocalchanges
> +    global sel_ix confirm_ok NS
>  
>      # check the tree is clean first??
> -    set newhead $headmenuhead
> +
> +    # Filter out remote branches if local branch is also present
> +    foreach remote_ix [lsearch -all $heads_on_commit "remotes/*"] {
> +        set remote_head [lindex $heads_on_commit $remote_ix]
> +        set local_head [string range $remote_head [expr [string last / $remote_head] + 1] end]
> +        if {$local_head in $heads_on_commit} {
> +            set heads_on_commit [lreplace $heads_on_commit $remote_ix $remote_ix]
> +        }
> +    }
> +
> +    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
> +        # Only the currently active branch
> +        return
> +    }
> +
> +    # Filter out mainhead
> +    set mainhead_ix [lsearch $heads_on_commit $mainhead]
> +    if {$mainhead_ix != -1} {
> +        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
> +    }
> +    set nr_heads_on_commit [llength $heads_on_commit]
> +
> +    # The number of heads on the commit determines how to select what to checkout
> +    if {$nr_heads_on_commit == 0} {
> +        set head_to_checkout ""
> +        set ref_to_checkout $id_to_checkout
> +    } elseif {$nr_heads_on_commit == 1} {
> +        set head_to_checkout [lindex $heads_on_commit 0]
> +        set ref_to_checkout $head_to_checkout
> +    } else {
> +        # Branch selection dialog
> +
> +        set confirm_ok 0
> +
> +        set w ".selectbranch"
> +        ttk_toplevel $w
> +        make_transient $w .
> +        wm title $w [mc "Check out branch"]

There is already a branch selection dialog (hit F2 to bring it up).
Could you please have a look whether it can be reused here? Because...

> +        ${NS}::label $w.m -text [mc "Check out which branch?"]
> +        pack $w.m -side top -fill x -padx 20 -pady 20
> +        ${NS}::frame $w.f
> +
> +        set sel_ix 0
> +        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
> +            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
> +                -text [lindex $heads_on_commit $i]

... presenting a list of items in the form of radio buttons is a very
non-standard user-interface, IMO. I would have expected a list widget in
which an entry is highlighted. Also, if the list is long, a list would
have a scroll bar, but a radio button group is limited to the available
screen height.

> +            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
> +            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
> +            grid $w.f.id_$i -sticky w -padx 20
> +        }
> +
> +        pack $w.f -side top -fill x -padx 4
> +        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
> +        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
> +        pack $w.ok -side left -fill x -padx 20 -pady 20
> +        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
> +        bind $w <Key-Escape> [list destroy $w]
> +        pack $w.cancel -side right -fill x -padx 20 -pady 20
> +        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
> +
> +        tkwait window $w
> +
> +        if {!$confirm_ok} return
> +
> +        set head_to_checkout [lindex $heads_on_commit $sel_ix]
> +        set ref_to_checkout $head_to_checkout
> +    }
> +
> +    # Handle remote branches
>      set command [list | git checkout]
> -    if {[string match "remotes/*" $newhead]} {
> -        set remote $newhead
> -        set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
> -        # The following check is redundant - the menu option should
> -        # be disabled to begin with...
> -        if {[info exists headids($newhead)]} {
> -            error_popup [mc "A local branch named %s exists already" $newhead]
> +    if {[string match "remotes/*" $head_to_checkout]} {
> +        set remote $head_to_checkout
> +        set head_to_checkout [string range $head_to_checkout [expr [string last / $head_to_checkout] + 1] end]
> +        set ref_to_checkout $head_to_checkout
> +        if {[info exists headids($head_to_checkout)]} {
> +            error_popup [mc "A local branch named %s exists already" $head_to_checkout]
>              return
>          }
> -        lappend command -b $newhead --track $remote
> +        lappend command -b $head_to_checkout --track $remote
>      } else {
> -        lappend command $newhead
> +        lappend command $ref_to_checkout
>      }
>      lappend command 2>@1
>      nowbusy checkout [mc "Checking out"]
> @@ -10011,11 +10081,11 @@ proc cobranch {} {
>              dodiffindex
>          }
>      } else {
> -        filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
> +        filerun $fd [list readcheckoutstat $fd $head_to_checkout $ref_to_checkout $id_to_checkout]
>      }
>  }
>  
> -proc readcheckoutstat {fd newhead newheadid} {
> +proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
>      global mainhead mainheadid headids idheads showlocalchanges progresscoords
>      global viewmainheadid curview
>  
> @@ -10033,18 +10103,19 @@ proc readcheckoutstat {fd newhead newheadid} {
>          error_popup $err
>          return
>      }
> -    set oldmainid $mainheadid
> -    if {! [info exists headids($newhead)]} {
> -        set headids($newhead) $newheadid
> -        lappend idheads($newheadid) $newhead
> -        addedhead $newheadid $newhead
> -    }
> -    set mainhead $newhead
> -    set mainheadid $newheadid
> -    set viewmainheadid($curview) $newheadid
> -    redrawtags $oldmainid
> -    redrawtags $newheadid
> -    selbyid $newheadid
> +    set old_main_id $mainheadid
> +
> +    if {$head_to_checkout ne "" && ! [info exists headids($head_to_checkout)]} {
> +        set headids($head_to_checkout) $id_to_checkout
> +        lappend idheads($id_to_checkout) $head_to_checkout
> +        addedhead $id_to_checkout $head_to_checkout
> +    }
> +    set mainhead $ref_to_checkout
> +    set mainheadid $id_to_checkout
> +    set viewmainheadid($curview) $id_to_checkout
> +    redrawtags $old_main_id
> +    redrawtags $id_to_checkout
> +    selbyid $id_to_checkout
>      if {$showlocalchanges} {
>          dodiffindex
>      }

Re: [PATCH v2 09/10] gitk: improve keyboard convenience in reset dialog

[Johannes Sixt]

Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:
> From: Jens Lidestrom <jens@lidestrom.se>
> 
> Make it more convenient to use the reset dialog using keyboard.
> 
> * Set focus to the combo box.
> * Accept with Return key.
> * Auto-select combo items when navigating in menu with up/down keys.

I like it!

> 
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
>  gitk-git/gitk | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index f559e279b7a..fafff2b1a5b 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -9902,27 +9902,40 @@ proc resethead {reset_target_id} {
>      ttk_toplevel $w
>      make_transient $w .
>      wm title $w [mc "Confirm reset"]
> +
>      ${NS}::label $w.m -text \
>          [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
>      pack $w.m -side top -fill x -padx 20 -pady 20
>      ${NS}::labelframe $w.f -text [mc "Reset type:"]
> +
>      set resettype mixed
> +
>      ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
>          -text [mc "Soft: Leave working tree and index untouched"]
> +    bind $w.f.soft <Key-Up> "set resettype hard"
> +    bind $w.f.soft <Key-Down> "set resettype mixed"
>      grid $w.f.soft -sticky w
> +
>      ${NS}::radiobutton $w.f.mixed -value mixed -variable resettype \
>          -text [mc "Mixed: Leave working tree untouched, reset index"]
> +    bind $w.f.mixed <Key-Up> "set resettype soft"
> +    bind $w.f.mixed <Key-Down> "set resettype hard"
>      grid $w.f.mixed -sticky w
> +
>      ${NS}::radiobutton $w.f.hard -value hard -variable resettype \
>          -text [mc "Hard: Reset working tree and index\n(discard ALL local changes)"]
> +    bind $w.f.hard <Key-Up> "set resettype mixed"
> +    bind $w.f.hard <Key-Down> "set resettype soft"
>      grid $w.f.hard -sticky w
>      pack $w.f -side top -fill x -padx 4
> +
>      ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
> +    bind $w <Key-Return> "set confirm_ok 1; destroy $w"
>      pack $w.ok -side left -fill x -padx 20 -pady 20
>      ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
>      bind $w <Key-Escape> [list destroy $w]
>      pack $w.cancel -side right -fill x -padx 20 -pady 20
> -    bind $w <Visibility> "grab $w; focus $w"
> +    bind $w <Visibility> "grab $w; focus $w.f.mixed"
>      tkwait window $w
>      if {!$confirm_ok} return
>      if {[catch {set fd [open \

Re: [PATCH v2 06/10] gitk: add keyboard bind for remove branch command

[Johannes Sixt]

Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:
> From: Jens Lidestrom <jens@lidestrom.se>
> 
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
>  gitk-git/gitk | 104 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 89 insertions(+), 15 deletions(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 8364033ad58..65ca11becca 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -2692,6 +2692,8 @@ proc makewindow {} {
>      bind $ctext <<Selection>> rehighlight_search_results
>      bind . <$M1B-t> {resethead [selected_line_id]}
>      bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
> +    bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}
> +    bind . <$M1B-b> {mkbranch [selected_line_id]}

This second line should not be in this commit, should it?

>      for {set i 1} {$i < 10} {incr i} {
>          bind . <$M1B-Key-$i> [list go_to_parent $i]
>      }
> @@ -2735,7 +2737,7 @@ proc makewindow {} {
>      makemenu $headctxmenu {
>          {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
>          {mc "Rename this branch" command mvbranch}
> -        {mc "Remove this branch" command rmbranch}
> +        {mc "Remove this branch" command {rmbranch [list $headmenuhead] $headmenuid 0}}
>          {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
>      }
>      $headctxmenu configure -tearoff 0
> @@ -3185,6 +3187,8 @@ proc keys {} {
>  [mc "<F5>		Update"]
>  [mc "<%s-T>		Reset current branch to selected commit" $M1T]
>  [mc "<%s-O>		Check out selected commit" $M1T]
> +[mc "<%s-B>		Create branch on selected commit" $M1T]
> +[mc "<%s-M>		Remove selected branch" $M1T]

Same here.

>  " \
>              -justify left -bg $bgcolor -border 2 -relief groove
>      pack $w.m -side top -fill both -padx 2 -pady 2
> @@ -10121,32 +10125,102 @@ proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
>      }
>  }
>  
> -proc rmbranch {} {
> -    global headmenuid headmenuhead mainhead
> +proc rmbranch {heads_on_commit id shouldComfirm} {
> +    global mainhead
>      global idheads
> +    global confirm_ok sel_ix NS
>  
> -    set head $headmenuhead
> -    set id $headmenuid
> -    # this check shouldn't be needed any more...
> -    if {$head eq $mainhead} {
> +    if {[llength $heads_on_commit] == 0} {
> +        error_popup [mc "Cannot delete a detached head"]
> +        return
> +    }
> +
> +    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
>          error_popup [mc "Cannot delete the currently checked-out branch"]
>          return
>      }
> -    set dheads [descheads $id]
> -    if {[llength $dheads] == 1 && $idheads($dheads) eq $head} {
> -        # the stuff on this branch isn't on any other branch
> -        if {![confirm_popup [mc "The commits on branch %s aren't on any other\
> -                        branch.\nReally delete branch %s?" $head $head]]} return
> +
> +    # Filter out mainhead
> +    set mainhead_ix [lsearch $heads_on_commit $mainhead]
> +    if {$mainhead_ix != -1} {
> +        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
> +    }
> +
> +    # Filter out remote branches
> +    foreach head_ix [lsearch -all $heads_on_commit "remotes/*"] {
> +        set heads_on_commit [lreplace $heads_on_commit $head_ix $head_ix]
> +    }
> +
> +    if {[llength $heads_on_commit] == 0} {
> +        # Probably only current branch and its remote branch on commit
> +        error_popup [mc "Cannot delete branch"]
> +        return
>      }
> +
> +    set nr_heads_on_commit [llength $heads_on_commit]
> +    set first_head [lindex $heads_on_commit 0]
> +
> +    if {$nr_heads_on_commit == 1} {
> +        # Only a single head, simple comfirm dialogs
> +
> +        set head_to_remove $first_head
> +        set dheads [descheads $id]
> +
> +        if {[llength $dheads] == 1 && $idheads($dheads) eq $head_to_remove} {
> +            # the stuff on this branch isn't on any other branch
> +            if {![confirm_popup [mc "The commits on branch %s aren't on any other\
> +                            branch.\nReally delete branch %s?" $head_to_remove $head_to_remove]]} return
> +        } elseif {$shouldComfirm} {
> +            if {![confirm_popup [mc "Really delete branch %s?" $head_to_remove]]} return
> +        }
> +    } else {
> +        # Select branch dialog
> +
> +        set confirm_ok 0
> +
> +        set w ".selectbranch"
> +        ttk_toplevel $w
> +        make_transient $w .
> +        wm title $w [mc "Delete branch"]
> +        ${NS}::label $w.m -text [mc "Which branch to delete?"]
> +        pack $w.m -side top -fill x -padx 20 -pady 20
> +        ${NS}::frame $w.f
> +
> +        set sel_ix 0
> +        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
> +            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
> +                -text [lindex $heads_on_commit $i]

The same comment as in the earlier commit about the checkout command
applies here: I don't think that radio buttons are the correct UI for a
branch selection.

In general, such duplication of code should not happen. The dialog only
differs in the title and help text and should be factored into a helper
function.

> +            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
> +            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
> +            grid $w.f.id_$i -sticky w -padx 20
> +        }
> +
> +        pack $w.f -side top -fill x -padx 4
> +        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
> +        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
> +        pack $w.ok -side left -fill x -padx 20 -pady 20
> +        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
> +        bind $w <Key-Escape> [list destroy $w]
> +        pack $w.cancel -side right -fill x -padx 20 -pady 20
> +        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
> +
> +        tkwait window $w
> +        if {!$confirm_ok} return
> +
> +        set head_to_remove [lindex $heads_on_commit $sel_ix]
> +    }
> +
> +    # Perform the command
> +
>      nowbusy rmbranch
>      update
> -    if {[catch {exec git branch -D $head} err]} {
> +    if {[catch {exec git branch -D $head_to_remove} err]} {
>          notbusy rmbranch
>          error_popup $err
>          return
>      }
> -    removehead $id $head
> -    removedhead $id $head
> +    removehead $id $head_to_remove
> +    removedhead $id $head_to_remove
>      redrawtags $id
>      notbusy rmbranch
>      dispneartags 0

Re: [PATCH v2 08/10] gitk: add keyboard bind for create branch command

[Johannes Sixt]

Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:
> From: Jens Lidestrom <jens@lidestrom.se>
> 
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
>  gitk-git/gitk | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 351b88f10c0..f559e279b7a 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -9582,13 +9582,13 @@ proc wrcomcan {} {
>      unset wrcomtop
>  }
>  
> -proc mkbranch {} {
> -    global NS rowmenuid
> +proc mkbranch {id} {
> +    global NS
>  
>      set top .branchdialog
>  
>      set val(name) ""
> -    set val(id) $rowmenuid
> +    set val(id) $id
>      set val(command) [list mkbrgo $top]
>  
>      set ui(title) [mc "Create branch"]

Clearly, the lines from the 'remove branch' commit belong here.

Re: [PATCH v2 07/10] gitk: add keyboard bind for cherry-pick command

[Johannes Sixt]

Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:
> From: Jens Lidestrom <jens@lidestrom.se>
> 
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
>  gitk-git/gitk | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 65ca11becca..351b88f10c0 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -2690,6 +2690,7 @@ proc makewindow {} {
>      bind $ctext $ctxbut {pop_diff_menu %W %X %Y %x %y}
>      bind $ctext <Button-1> {focus %W}
>      bind $ctext <<Selection>> rehighlight_search_results
> +    bind . <$M1B-p> {cherrypick [selected_line_id]}
>      bind . <$M1B-t> {resethead [selected_line_id]}
>      bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
>      bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}
> @@ -2710,8 +2711,8 @@ proc makewindow {} {
>          {mc "Create tag" command mktag}
>          {mc "Copy commit reference" command copyreference}
>          {mc "Write commit to file" command writecommit}
> -        {mc "Create new branch" command mkbranch}
> -        {mc "Cherry-pick this commit" command cherrypick}
> +        {mc "Create new branch" command {mkbranch $rowmenuid}}
> +        {mc "Cherry-pick this commit" command {cherrypick $rowmenuid}}

The change regarding Create new branch is not related to this commit's
topic and should be elsewhere.

>          {mc "Reset current branch to here" command {resethead $rowmenuid}}
>          {mc "Mark this commit" command markhere}
>          {mc "Return to mark" command gotomark}
> @@ -3186,6 +3187,7 @@ proc keys {} {
>  [mc "<%s-minus>	Decrease font size" $M1T]
>  [mc "<F5>		Update"]
>  [mc "<%s-T>		Reset current branch to selected commit" $M1T]
> +[mc "<%s-P>		Cherry-pick selected commit to current branch" $M1T]
>  [mc "<%s-O>		Check out selected commit" $M1T]
>  [mc "<%s-B>		Create branch on selected commit" $M1T]
>  [mc "<%s-M>		Remove selected branch" $M1T]
> @@ -9758,24 +9760,29 @@ proc exec_citool {tool_args {baseid {}}} {
>      array set env $save_env
>  }
>  
> -proc cherrypick {} {
> -    global rowmenuid curview
> +proc cherrypick {id} {
> +    global curview headids
>      global mainhead mainheadid
>      global gitdir
>  
> +    if {! [info exists headids($mainhead)]} {
> +        error_popup [mc "Cannot cherry-pick to a detached head"]
> +        return
> +    }

Why is it necessary to forbid this now? It was not forbidden before.

> +
>      set oldhead [exec git rev-parse HEAD]
> -    set dheads [descheads $rowmenuid]
> +    set dheads [descheads $id]
>      if {$dheads ne {} && [lsearch -exact $dheads $oldhead] >= 0} {
>          set ok [confirm_popup [mc "Commit %s is already\
>                  included in branch %s -- really re-apply it?" \
> -                                   [string range $rowmenuid 0 7] $mainhead]]
> +                                   [string range $id 0 7] $mainhead]]
>          if {!$ok} return
>      }
>      nowbusy cherrypick [mc "Cherry-picking"]
>      update
>      # Unfortunately git-cherry-pick writes stuff to stderr even when
>      # no error occurs, and exec takes that as an indication of error...
> -    if {[catch {exec sh -c "git cherry-pick -r $rowmenuid 2>&1"} err]} {
> +    if {[catch {exec sh -c "git cherry-pick -r $id 2>&1"} err]} {
>          notbusy cherrypick
>          if {[regexp -line \
>                   {Entry '(.*)' (would be overwritten by merge|not uptodate)} \
> @@ -9791,7 +9798,7 @@ proc cherrypick {} {
>                          resolve it?"]]} {
>                  # Force citool to read MERGE_MSG
>                  file delete [file join $gitdir "GITGUI_MSG"]
> -                exec_citool {} $rowmenuid
> +                exec_citool {} $id
>              }
>          } else {
>              error_popup $err

Re: [PATCH v2 05/10] gitk: add keyboard bind for checkout command

[Jens Lideström]

> There is already a branch selection dialog (hit F2 to bring it up).
> Could you please have a look whether it can be reused here? Because...

Ah, I didn't consider that! I'll investigate using something similar.

> At a minimum, there should be a confirmation when a
> transition from a branch to a detached head happens

That sounds like a good idea to me too, will fix.

> I find it irritating that it is possible to check out a detached head
> with Ctrl-O, but not via the context menu.
> 
> ...
>
> I propose that you do not include the ability to check out a detached
> head in this commit, but make it a new feature once this series has settled.

I agree that the asymmetry between the keyboard bind and the context menu is somewhat unfortunate. But I also think that there is a pretty good justification it, and because of this I'd like to keep the feature in this PR.

The row context menu has a bit too many entries already, to the point where it is a bit hard to use. We really only what to put the most important or commonly used commands here. This is an inherit limitation for GUI menus: the screen real estate is limited. So there is a valid reason not to include the Checkout command.

For keyboard binds however these is no such screen real-estate issue. On the contrary: For keyboard binds it is an artificial asymmetry without any technical justification to NOT be able to checkout the commit of a selected row.

Apart from this it will unfortunately be hard for me to take the time to create a separate PR.

Do you accept this motivation for having the "checkout detached head" feature stay in this PR?

> I find it irritating that it is possible to check out a detached head
> with Ctrl-O, but not via the context menu.
> 
> Playing around a bit with this feature, it may be a bit too easy to
> check out a different commit with a simple key press and without
> confirmation. At a minimum, there should be a confirmation when a
> transition from a branch to a detached head happens, because otherwise
> inexperienced users end up in inconvenient territories.
> 
> I propose that you do not include the ability to check out a detached
> head in this commit, but make it a new feature once this series has settled.


On 2023-07-05 19:29, Johannes Sixt wrote:
> Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:
>> From: Jens Lidestrom <jens@lidestrom.se>
>>
>> This also introduces the ability to check out detatched heads. This
>> shouldn't result any problems, because gitk already works with
>> detatched heads if they are checked out using the terminal.
> 
> I find it irritating that it is possible to check out a detached head
> with Ctrl-O, but not via the context menu.
> 
> Playing around a bit with this feature, it may be a bit too easy to
> check out a different commit with a simple key press and without
> confirmation. At a minimum, there should be a confirmation when a
> transition from a branch to a detached head happens, because otherwise
> inexperienced users end up in inconvenient territories.
> 
> I propose that you do not include the ability to check out a detached
> head in this commit, but make it a new feature once this series has settled.
> 
>>
>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>> ---
>>  gitk-git/gitk | 125 +++++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 98 insertions(+), 27 deletions(-)
>>
>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>> index 642cd7f652a..8364033ad58 100755
>> --- a/gitk-git/gitk
>> +++ b/gitk-git/gitk
>> @@ -2691,6 +2691,7 @@ proc makewindow {} {
>>      bind $ctext <Button-1> {focus %W}
>>      bind $ctext <<Selection>> rehighlight_search_results
>>      bind . <$M1B-t> {resethead [selected_line_id]}
>> +    bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
>>      for {set i 1} {$i < 10} {incr i} {
>>          bind . <$M1B-Key-$i> [list go_to_parent $i]
>>      }
>> @@ -2732,7 +2733,7 @@ proc makewindow {} {
>>  
>>      set headctxmenu .headctxmenu
>>      makemenu $headctxmenu {
>> -        {mc "Check out this branch" command cobranch}
>> +        {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
>>          {mc "Rename this branch" command mvbranch}
>>          {mc "Remove this branch" command rmbranch}
>>          {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
>> @@ -3183,6 +3184,7 @@ proc keys {} {
>>  [mc "<%s-minus>	Decrease font size" $M1T]
>>  [mc "<F5>		Update"]
>>  [mc "<%s-T>		Reset current branch to selected commit" $M1T]
>> +[mc "<%s-O>		Check out selected commit" $M1T]
>>  " \
>>              -justify left -bg $bgcolor -border 2 -relief groove
>>      pack $w.m -side top -fill both -padx 2 -pady 2
>> @@ -9978,25 +9980,93 @@ proc headmenu {x y id head} {
>>      tk_popup $headctxmenu $x $y
>>  }
>>  
>> -proc cobranch {} {
>> -    global headmenuid headmenuhead headids
>> +proc checkout {heads_on_commit id_to_checkout} {
>> +    global headids mainhead
>>      global showlocalchanges
>> +    global sel_ix confirm_ok NS
>>  
>>      # check the tree is clean first??
>> -    set newhead $headmenuhead
>> +
>> +    # Filter out remote branches if local branch is also present
>> +    foreach remote_ix [lsearch -all $heads_on_commit "remotes/*"] {
>> +        set remote_head [lindex $heads_on_commit $remote_ix]
>> +        set local_head [string range $remote_head [expr [string last / $remote_head] + 1] end]
>> +        if {$local_head in $heads_on_commit} {
>> +            set heads_on_commit [lreplace $heads_on_commit $remote_ix $remote_ix]
>> +        }
>> +    }
>> +
>> +    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
>> +        # Only the currently active branch
>> +        return
>> +    }
>> +
>> +    # Filter out mainhead
>> +    set mainhead_ix [lsearch $heads_on_commit $mainhead]
>> +    if {$mainhead_ix != -1} {
>> +        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
>> +    }
>> +    set nr_heads_on_commit [llength $heads_on_commit]
>> +
>> +    # The number of heads on the commit determines how to select what to checkout
>> +    if {$nr_heads_on_commit == 0} {
>> +        set head_to_checkout ""
>> +        set ref_to_checkout $id_to_checkout
>> +    } elseif {$nr_heads_on_commit == 1} {
>> +        set head_to_checkout [lindex $heads_on_commit 0]
>> +        set ref_to_checkout $head_to_checkout
>> +    } else {
>> +        # Branch selection dialog
>> +
>> +        set confirm_ok 0
>> +
>> +        set w ".selectbranch"
>> +        ttk_toplevel $w
>> +        make_transient $w .
>> +        wm title $w [mc "Check out branch"]
> 
> There is already a branch selection dialog (hit F2 to bring it up).
> Could you please have a look whether it can be reused here? Because...
> 
>> +        ${NS}::label $w.m -text [mc "Check out which branch?"]
>> +        pack $w.m -side top -fill x -padx 20 -pady 20
>> +        ${NS}::frame $w.f
>> +
>> +        set sel_ix 0
>> +        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
>> +            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
>> +                -text [lindex $heads_on_commit $i]
> 
> ... presenting a list of items in the form of radio buttons is a very
> non-standard user-interface, IMO. I would have expected a list widget in
> which an entry is highlighted. Also, if the list is long, a list would
> have a scroll bar, but a radio button group is limited to the available
> screen height.
> 
>> +            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
>> +            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
>> +            grid $w.f.id_$i -sticky w -padx 20
>> +        }
>> +
>> +        pack $w.f -side top -fill x -padx 4
>> +        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
>> +        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
>> +        pack $w.ok -side left -fill x -padx 20 -pady 20
>> +        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
>> +        bind $w <Key-Escape> [list destroy $w]
>> +        pack $w.cancel -side right -fill x -padx 20 -pady 20
>> +        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
>> +
>> +        tkwait window $w
>> +
>> +        if {!$confirm_ok} return
>> +
>> +        set head_to_checkout [lindex $heads_on_commit $sel_ix]
>> +        set ref_to_checkout $head_to_checkout
>> +    }
>> +
>> +    # Handle remote branches
>>      set command [list | git checkout]
>> -    if {[string match "remotes/*" $newhead]} {
>> -        set remote $newhead
>> -        set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
>> -        # The following check is redundant - the menu option should
>> -        # be disabled to begin with...
>> -        if {[info exists headids($newhead)]} {
>> -            error_popup [mc "A local branch named %s exists already" $newhead]
>> +    if {[string match "remotes/*" $head_to_checkout]} {
>> +        set remote $head_to_checkout
>> +        set head_to_checkout [string range $head_to_checkout [expr [string last / $head_to_checkout] + 1] end]
>> +        set ref_to_checkout $head_to_checkout
>> +        if {[info exists headids($head_to_checkout)]} {
>> +            error_popup [mc "A local branch named %s exists already" $head_to_checkout]
>>              return
>>          }
>> -        lappend command -b $newhead --track $remote
>> +        lappend command -b $head_to_checkout --track $remote
>>      } else {
>> -        lappend command $newhead
>> +        lappend command $ref_to_checkout
>>      }
>>      lappend command 2>@1
>>      nowbusy checkout [mc "Checking out"]
>> @@ -10011,11 +10081,11 @@ proc cobranch {} {
>>              dodiffindex
>>          }
>>      } else {
>> -        filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
>> +        filerun $fd [list readcheckoutstat $fd $head_to_checkout $ref_to_checkout $id_to_checkout]
>>      }
>>  }
>>  
>> -proc readcheckoutstat {fd newhead newheadid} {
>> +proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
>>      global mainhead mainheadid headids idheads showlocalchanges progresscoords
>>      global viewmainheadid curview
>>  
>> @@ -10033,18 +10103,19 @@ proc readcheckoutstat {fd newhead newheadid} {
>>          error_popup $err
>>          return
>>      }
>> -    set oldmainid $mainheadid
>> -    if {! [info exists headids($newhead)]} {
>> -        set headids($newhead) $newheadid
>> -        lappend idheads($newheadid) $newhead
>> -        addedhead $newheadid $newhead
>> -    }
>> -    set mainhead $newhead
>> -    set mainheadid $newheadid
>> -    set viewmainheadid($curview) $newheadid
>> -    redrawtags $oldmainid
>> -    redrawtags $newheadid
>> -    selbyid $newheadid
>> +    set old_main_id $mainheadid
>> +
>> +    if {$head_to_checkout ne "" && ! [info exists headids($head_to_checkout)]} {
>> +        set headids($head_to_checkout) $id_to_checkout
>> +        lappend idheads($id_to_checkout) $head_to_checkout
>> +        addedhead $id_to_checkout $head_to_checkout
>> +    }
>> +    set mainhead $ref_to_checkout
>> +    set mainheadid $id_to_checkout
>> +    set viewmainheadid($curview) $id_to_checkout
>> +    redrawtags $old_main_id
>> +    redrawtags $id_to_checkout
>> +    selbyid $id_to_checkout
>>      if {$showlocalchanges} {
>>          dodiffindex
>>      }
> 

Re: [PATCH v2 07/10] gitk: add keyboard bind for cherry-pick command

[Jens Lideström]

>> +        {mc "Create new branch" command {mkbranch $rowmenuid}}
>> +        {mc "Cherry-pick this commit" command {cherrypick $rowmenuid}}
> 
> The change regarding Create new branch is not related to this commit's
> topic and should be elsewhere.

Sorry, sloppy mistake, will fix, thanks.

>> +    if {! [info exists headids($mainhead)]} {
>> +        error_popup [mc "Cannot cherry-pick to a detached head"]
>> +        return
>> +    }
> 
> Why is it necessary to forbid this now? It was not forbidden before.

Oh, I had no idea you can actually cherry-pick to a detached head! When a added this check I though I fixed an existing bug, which only manifested if the user had checked out a detached head using the terminal and then used cherry-pick in gitk. I now see that it is not an error.

I have mixed feelings about allowing this, but it's probably best to not invent artificial limitations in gitk, so I'll remove the check.

/Jens

On 2023-07-05 22:07, Johannes Sixt wrote:
> Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:
>> From: Jens Lidestrom <jens@lidestrom.se>
>>
>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>> ---
>>  gitk-git/gitk | 23 +++++++++++++++--------
>>  1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>> index 65ca11becca..351b88f10c0 100755
>> --- a/gitk-git/gitk
>> +++ b/gitk-git/gitk
>> @@ -2690,6 +2690,7 @@ proc makewindow {} {
>>      bind $ctext $ctxbut {pop_diff_menu %W %X %Y %x %y}
>>      bind $ctext <Button-1> {focus %W}
>>      bind $ctext <<Selection>> rehighlight_search_results
>> +    bind . <$M1B-p> {cherrypick [selected_line_id]}
>>      bind . <$M1B-t> {resethead [selected_line_id]}
>>      bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
>>      bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}
>> @@ -2710,8 +2711,8 @@ proc makewindow {} {
>>          {mc "Create tag" command mktag}
>>          {mc "Copy commit reference" command copyreference}
>>          {mc "Write commit to file" command writecommit}
>> -        {mc "Create new branch" command mkbranch}
>> -        {mc "Cherry-pick this commit" command cherrypick}
>> +        {mc "Create new branch" command {mkbranch $rowmenuid}}
>> +        {mc "Cherry-pick this commit" command {cherrypick $rowmenuid}}
> 
> The change regarding Create new branch is not related to this commit's
> topic and should be elsewhere.
> 
>>          {mc "Reset current branch to here" command {resethead $rowmenuid}}
>>          {mc "Mark this commit" command markhere}
>>          {mc "Return to mark" command gotomark}
>> @@ -3186,6 +3187,7 @@ proc keys {} {
>>  [mc "<%s-minus>	Decrease font size" $M1T]
>>  [mc "<F5>		Update"]
>>  [mc "<%s-T>		Reset current branch to selected commit" $M1T]
>> +[mc "<%s-P>		Cherry-pick selected commit to current branch" $M1T]
>>  [mc "<%s-O>		Check out selected commit" $M1T]
>>  [mc "<%s-B>		Create branch on selected commit" $M1T]
>>  [mc "<%s-M>		Remove selected branch" $M1T]
>> @@ -9758,24 +9760,29 @@ proc exec_citool {tool_args {baseid {}}} {
>>      array set env $save_env
>>  }
>>  
>> -proc cherrypick {} {
>> -    global rowmenuid curview
>> +proc cherrypick {id} {
>> +    global curview headids
>>      global mainhead mainheadid
>>      global gitdir
>>  
>> +    if {! [info exists headids($mainhead)]} {
>> +        error_popup [mc "Cannot cherry-pick to a detached head"]
>> +        return
>> +    }
> 
> Why is it necessary to forbid this now? It was not forbidden before.
> 
>> +
>>      set oldhead [exec git rev-parse HEAD]
>> -    set dheads [descheads $rowmenuid]
>> +    set dheads [descheads $id]
>>      if {$dheads ne {} && [lsearch -exact $dheads $oldhead] >= 0} {
>>          set ok [confirm_popup [mc "Commit %s is already\
>>                  included in branch %s -- really re-apply it?" \
>> -                                   [string range $rowmenuid 0 7] $mainhead]]
>> +                                   [string range $id 0 7] $mainhead]]
>>          if {!$ok} return
>>      }
>>      nowbusy cherrypick [mc "Cherry-picking"]
>>      update
>>      # Unfortunately git-cherry-pick writes stuff to stderr even when
>>      # no error occurs, and exec takes that as an indication of error...
>> -    if {[catch {exec sh -c "git cherry-pick -r $rowmenuid 2>&1"} err]} {
>> +    if {[catch {exec sh -c "git cherry-pick -r $id 2>&1"} err]} {
>>          notbusy cherrypick
>>          if {[regexp -line \
>>                   {Entry '(.*)' (would be overwritten by merge|not uptodate)} \
>> @@ -9791,7 +9798,7 @@ proc cherrypick {} {
>>                          resolve it?"]]} {
>>                  # Force citool to read MERGE_MSG
>>                  file delete [file join $gitdir "GITGUI_MSG"]
>> -                exec_citool {} $rowmenuid
>> +                exec_citool {} $id
>>              }
>>          } else {
>>              error_popup $err
> 

Re: [PATCH v2 06/10] gitk: add keyboard bind for remove branch command

[Jens Lideström]

> This second line should not be in this commit, should it?

Damn, those adjacent lines where commands are listed always conflict when I reorder commits. I have fixed them several times and I seem to have forgot to fix them after the last reorder.

> The same comment as in the earlier commit about the checkout command
> applies here: I don't think that radio buttons are the correct UI for a
> branch selection.

I discuss this in the tread about the checkout command.

> In general, such duplication of code should not happen. The dialog only
> differs in the title and help text and should be factored into a helper
> function.

I will extract a procedure for this and use in the implementation of both the commands, possibly something similar to the List references dialog.

/Jens

On 2023-07-05 22:00, Johannes Sixt wrote:
> Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:
>> From: Jens Lidestrom <jens@lidestrom.se>
>>
>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>> ---
>>  gitk-git/gitk | 104 ++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 89 insertions(+), 15 deletions(-)
>>
>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>> index 8364033ad58..65ca11becca 100755
>> --- a/gitk-git/gitk
>> +++ b/gitk-git/gitk
>> @@ -2692,6 +2692,8 @@ proc makewindow {} {
>>      bind $ctext <<Selection>> rehighlight_search_results
>>      bind . <$M1B-t> {resethead [selected_line_id]}
>>      bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
>> +    bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}
>> +    bind . <$M1B-b> {mkbranch [selected_line_id]}
> 
> This second line should not be in this commit, should it?
> 
>>      for {set i 1} {$i < 10} {incr i} {
>>          bind . <$M1B-Key-$i> [list go_to_parent $i]
>>      }
>> @@ -2735,7 +2737,7 @@ proc makewindow {} {
>>      makemenu $headctxmenu {
>>          {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
>>          {mc "Rename this branch" command mvbranch}
>> -        {mc "Remove this branch" command rmbranch}
>> +        {mc "Remove this branch" command {rmbranch [list $headmenuhead] $headmenuid 0}}
>>          {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
>>      }
>>      $headctxmenu configure -tearoff 0
>> @@ -3185,6 +3187,8 @@ proc keys {} {
>>  [mc "<F5>		Update"]
>>  [mc "<%s-T>		Reset current branch to selected commit" $M1T]
>>  [mc "<%s-O>		Check out selected commit" $M1T]
>> +[mc "<%s-B>		Create branch on selected commit" $M1T]
>> +[mc "<%s-M>		Remove selected branch" $M1T]
> 
> Same here.
> 
>>  " \
>>              -justify left -bg $bgcolor -border 2 -relief groove
>>      pack $w.m -side top -fill both -padx 2 -pady 2
>> @@ -10121,32 +10125,102 @@ proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
>>      }
>>  }
>>  
>> -proc rmbranch {} {
>> -    global headmenuid headmenuhead mainhead
>> +proc rmbranch {heads_on_commit id shouldComfirm} {
>> +    global mainhead
>>      global idheads
>> +    global confirm_ok sel_ix NS
>>  
>> -    set head $headmenuhead
>> -    set id $headmenuid
>> -    # this check shouldn't be needed any more...
>> -    if {$head eq $mainhead} {
>> +    if {[llength $heads_on_commit] == 0} {
>> +        error_popup [mc "Cannot delete a detached head"]
>> +        return
>> +    }
>> +
>> +    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
>>          error_popup [mc "Cannot delete the currently checked-out branch"]
>>          return
>>      }
>> -    set dheads [descheads $id]
>> -    if {[llength $dheads] == 1 && $idheads($dheads) eq $head} {
>> -        # the stuff on this branch isn't on any other branch
>> -        if {![confirm_popup [mc "The commits on branch %s aren't on any other\
>> -                        branch.\nReally delete branch %s?" $head $head]]} return
>> +
>> +    # Filter out mainhead
>> +    set mainhead_ix [lsearch $heads_on_commit $mainhead]
>> +    if {$mainhead_ix != -1} {
>> +        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
>> +    }
>> +
>> +    # Filter out remote branches
>> +    foreach head_ix [lsearch -all $heads_on_commit "remotes/*"] {
>> +        set heads_on_commit [lreplace $heads_on_commit $head_ix $head_ix]
>> +    }
>> +
>> +    if {[llength $heads_on_commit] == 0} {
>> +        # Probably only current branch and its remote branch on commit
>> +        error_popup [mc "Cannot delete branch"]
>> +        return
>>      }
>> +
>> +    set nr_heads_on_commit [llength $heads_on_commit]
>> +    set first_head [lindex $heads_on_commit 0]
>> +
>> +    if {$nr_heads_on_commit == 1} {
>> +        # Only a single head, simple comfirm dialogs
>> +
>> +        set head_to_remove $first_head
>> +        set dheads [descheads $id]
>> +
>> +        if {[llength $dheads] == 1 && $idheads($dheads) eq $head_to_remove} {
>> +            # the stuff on this branch isn't on any other branch
>> +            if {![confirm_popup [mc "The commits on branch %s aren't on any other\
>> +                            branch.\nReally delete branch %s?" $head_to_remove $head_to_remove]]} return
>> +        } elseif {$shouldComfirm} {
>> +            if {![confirm_popup [mc "Really delete branch %s?" $head_to_remove]]} return
>> +        }
>> +    } else {
>> +        # Select branch dialog
>> +
>> +        set confirm_ok 0
>> +
>> +        set w ".selectbranch"
>> +        ttk_toplevel $w
>> +        make_transient $w .
>> +        wm title $w [mc "Delete branch"]
>> +        ${NS}::label $w.m -text [mc "Which branch to delete?"]
>> +        pack $w.m -side top -fill x -padx 20 -pady 20
>> +        ${NS}::frame $w.f
>> +
>> +        set sel_ix 0
>> +        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
>> +            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
>> +                -text [lindex $heads_on_commit $i]
> 
> The same comment as in the earlier commit about the checkout command
> applies here: I don't think that radio buttons are the correct UI for a
> branch selection.
> 
> In general, such duplication of code should not happen. The dialog only
> differs in the title and help text and should be factored into a helper
> function.
> 
>> +            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
>> +            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
>> +            grid $w.f.id_$i -sticky w -padx 20
>> +        }
>> +
>> +        pack $w.f -side top -fill x -padx 4
>> +        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
>> +        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
>> +        pack $w.ok -side left -fill x -padx 20 -pady 20
>> +        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
>> +        bind $w <Key-Escape> [list destroy $w]
>> +        pack $w.cancel -side right -fill x -padx 20 -pady 20
>> +        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
>> +
>> +        tkwait window $w
>> +        if {!$confirm_ok} return
>> +
>> +        set head_to_remove [lindex $heads_on_commit $sel_ix]
>> +    }
>> +
>> +    # Perform the command
>> +
>>      nowbusy rmbranch
>>      update
>> -    if {[catch {exec git branch -D $head} err]} {
>> +    if {[catch {exec git branch -D $head_to_remove} err]} {
>>          notbusy rmbranch
>>          error_popup $err
>>          return
>>      }
>> -    removehead $id $head
>> -    removedhead $id $head
>> +    removehead $id $head_to_remove
>> +    removedhead $id $head_to_remove
>>      redrawtags $id
>>      notbusy rmbranch
>>      dispneartags 0
>