| Message ID | ab1f68cc8552e405c9d04622be1e728ab81bda17.1567713659.git.bert.wesarg@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] git-gui: convert new/amend commit radiobutton to checketton | expand |
Hi Bert, I have some _very_ busy few days coming up, so I won't be able to devote much time to this right now. I will take a look at the series and your other patches by the coming Wednesday. Thanks. [snip]
Typo in the subject. s/checketton/checkbutton/ On 05/09/19 10:09PM, Bert Wesarg wrote: > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> > --- > git-gui.sh | 36 +++++++++--------------------------- > lib/checkout_op.tcl | 6 +++--- > lib/commit.tcl | 4 ++-- > lib/index.tcl | 8 ++++---- > 4 files changed, 18 insertions(+), 36 deletions(-) > > diff --git a/git-gui.sh b/git-gui.sh > index 5bc21b8..80a07d5 100755 > --- a/git-gui.sh > +++ b/git-gui.sh > @@ -1341,6 +1341,7 @@ set HEAD {} > set PARENT {} > set MERGE_HEAD [list] > set commit_type {} > +set commit_type_is_amend 0 > set empty_tree {} > set current_branch {} > set is_detached 0 > @@ -1348,7 +1349,6 @@ set current_diff_path {} > set is_3way_diff 0 > set is_submodule_diff 0 > set is_conflict_diff 0 > -set selected_commit_type new > set diff_empty_count 0 > > set nullid "0000000000000000000000000000000000000000" > @@ -1435,7 +1435,7 @@ proc PARENT {} { > } > > proc force_amend {} { > - global selected_commit_type > + global commit_type_is_amend > global HEAD PARENT MERGE_HEAD commit_type > > repository_state newType newHEAD newMERGE_HEAD > @@ -1444,7 +1444,7 @@ proc force_amend {} { > set MERGE_HEAD $newMERGE_HEAD > set commit_type $newType > > - set selected_commit_type amend > + set commit_type_is_amend 1 Why do we need a separate variable for this? Why not just check commit_type to know whether it is an amend or not? My guess after reading the patch is that we need to associate a variable with the checkbutton to decide its state, and since there are multiple types of amend that commit_type can be (amend, amend-initial, amend-merge), it is not easy to use it directly. Am I guessing correctly? > do_select_commit_type > } > > @@ -2828,19 +2828,10 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} { > menu .mbar.commit > > if {![is_enabled nocommit]} { > - .mbar.commit add radiobutton \ > - -label [mc "New Commit"] \ > - -command do_select_commit_type \ > - -variable selected_commit_type \ > - -value new > - lappend disable_on_lock \ > - [list .mbar.commit entryconf [.mbar.commit index last] -state] > - > - .mbar.commit add radiobutton \ > + .mbar.commit add checkbutton \ > -label [mc "Amend Last Commit"] \ > - -command do_select_commit_type \ > - -variable selected_commit_type \ > - -value amend > + -variable commit_type_is_amend \ > + -command do_select_commit_type > lappend disable_on_lock \ > [list .mbar.commit entryconf [.mbar.commit index last] -state] > > @@ -3313,18 +3304,10 @@ set ui_comm .vpane.lower.commarea.buffer.frame.t > set ui_coml .vpane.lower.commarea.buffer.header.l > > if {![is_enabled nocommit]} { > - ${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \ > - -text [mc "New Commit"] \ > - -command do_select_commit_type \ > - -variable selected_commit_type \ > - -value new > - lappend disable_on_lock \ > - [list .vpane.lower.commarea.buffer.header.new conf -state] > - ${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \ > + ${NS}::checkbutton .vpane.lower.commarea.buffer.header.amend \ > -text [mc "Amend Last Commit"] \ > - -command do_select_commit_type \ > - -variable selected_commit_type \ > - -value amend > + -variable commit_type_is_amend \ > + -command do_select_commit_type > lappend disable_on_lock \ > [list .vpane.lower.commarea.buffer.header.amend conf -state] > } > @@ -3349,7 +3332,6 @@ pack $ui_coml -side left -fill x > > if {![is_enabled nocommit]} { > pack .vpane.lower.commarea.buffer.header.amend -side right > - pack .vpane.lower.commarea.buffer.header.new -side right > } > > textframe .vpane.lower.commarea.buffer.frame > diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl > index 9e7412c..a522829 100644 > --- a/lib/checkout_op.tcl > +++ b/lib/checkout_op.tcl > @@ -389,7 +389,7 @@ $err > } > > method _after_readtree {} { > - global selected_commit_type commit_type HEAD MERGE_HEAD PARENT > + global commit_type HEAD MERGE_HEAD PARENT This cleans up an unused variable declaration. Nice. > global current_branch is_detached > global ui_comm > > @@ -490,12 +490,12 @@ method _update_repo_state {} { > # amend mode our file lists are accurate and we can avoid > # the rescan. > # > - global selected_commit_type commit_type HEAD MERGE_HEAD PARENT > + global commit_type_is_amend commit_type HEAD MERGE_HEAD PARENT > global ui_comm > > unlock_index > set name [_name $this] > - set selected_commit_type new > + set commit_type_is_amend 0 > if {[string match amend* $commit_type]} { > $ui_comm delete 0.0 end > $ui_comm edit reset > diff --git a/lib/commit.tcl b/lib/commit.tcl > index 83620b7..384f18f 100644 > --- a/lib/commit.tcl > +++ b/lib/commit.tcl > @@ -327,7 +327,7 @@ proc commit_writetree {curHEAD msg_p} { > proc commit_committree {fd_wt curHEAD msg_p} { > global HEAD PARENT MERGE_HEAD commit_type commit_author > global current_branch > - global ui_comm selected_commit_type > + global ui_comm commit_type_is_amend > global file_states selected_paths rescan_active > global repo_config > global env > @@ -461,8 +461,8 @@ A rescan will be automatically started now. > > # -- Update in memory status > # > - set selected_commit_type new > set commit_type normal > + set commit_type_is_amend 0 > set HEAD $cmt_id > set PARENT $cmt_id > set MERGE_HEAD [list] > diff --git a/lib/index.tcl b/lib/index.tcl > index b588db1..e07b7a3 100644 > --- a/lib/index.tcl > +++ b/lib/index.tcl > @@ -466,19 +466,19 @@ proc do_revert_selection {} { > } > > proc do_select_commit_type {} { > - global commit_type selected_commit_type > + global commit_type commit_type_is_amend > > - if {$selected_commit_type eq {new} > + if {$commit_type_is_amend == 0 > && [string match amend* $commit_type]} { > create_new_commit > - } elseif {$selected_commit_type eq {amend} > + } elseif {$commit_type_is_amend == 1 > && ![string match amend* $commit_type]} { Not exactly related to your change, but shouldn't these "string match amend*" in the two ifs be assertions instead of checks? If $commit_type_is_amend == 0, then $commit_type should _always_ be amend*, and if $commit_type_is_amend == 1, then $commit_type should _never_ be amend*. I don't see assertions being used anywhere, but I suppose we should look into them in the future. It would be great if you can start using something like that here, but I'm fine with keeping this like it is right now too. > load_last_commit > > # The amend request was rejected... > # > if {![string match amend* $commit_type]} { > - set selected_commit_type new > + set commit_type_is_amend 0 > } > } > } I tested it on my setup and it works fine. Thanks.
On Wed, Sep 11, 2019 at 10:15 PM Pratyush Yadav <me@yadavpratyush.com> wrote: > > Typo in the subject. s/checketton/checkbutton/ > > On 05/09/19 10:09PM, Bert Wesarg wrote: > > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> > > --- > > git-gui.sh | 36 +++++++++--------------------------- > > lib/checkout_op.tcl | 6 +++--- > > lib/commit.tcl | 4 ++-- > > lib/index.tcl | 8 ++++---- > > 4 files changed, 18 insertions(+), 36 deletions(-) > > > > diff --git a/git-gui.sh b/git-gui.sh > > index 5bc21b8..80a07d5 100755 > > --- a/git-gui.sh > > +++ b/git-gui.sh > > @@ -1341,6 +1341,7 @@ set HEAD {} > > set PARENT {} > > set MERGE_HEAD [list] > > set commit_type {} > > +set commit_type_is_amend 0 > > set empty_tree {} > > set current_branch {} > > set is_detached 0 > > @@ -1348,7 +1349,6 @@ set current_diff_path {} > > set is_3way_diff 0 > > set is_submodule_diff 0 > > set is_conflict_diff 0 > > -set selected_commit_type new > > set diff_empty_count 0 > > > > set nullid "0000000000000000000000000000000000000000" > > @@ -1435,7 +1435,7 @@ proc PARENT {} { > > } > > > > proc force_amend {} { > > - global selected_commit_type > > + global commit_type_is_amend > > global HEAD PARENT MERGE_HEAD commit_type > > > > repository_state newType newHEAD newMERGE_HEAD > > @@ -1444,7 +1444,7 @@ proc force_amend {} { > > set MERGE_HEAD $newMERGE_HEAD > > set commit_type $newType > > > > - set selected_commit_type amend > > + set commit_type_is_amend 1 > > Why do we need a separate variable for this? Why not just check > commit_type to know whether it is an amend or not? My guess after > reading the patch is that we need to associate a variable with the > checkbutton to decide its state, and since there are multiple types of > amend that commit_type can be (amend, amend-initial, amend-merge), it is > not easy to use it directly. Am I guessing correctly? > > > do_select_commit_type > > } > > > > @@ -2828,19 +2828,10 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} { > > menu .mbar.commit > > > > if {![is_enabled nocommit]} { > > - .mbar.commit add radiobutton \ > > - -label [mc "New Commit"] \ > > - -command do_select_commit_type \ > > - -variable selected_commit_type \ > > - -value new > > - lappend disable_on_lock \ > > - [list .mbar.commit entryconf [.mbar.commit index last] -state] > > - > > - .mbar.commit add radiobutton \ > > + .mbar.commit add checkbutton \ > > -label [mc "Amend Last Commit"] \ > > - -command do_select_commit_type \ > > - -variable selected_commit_type \ > > - -value amend > > + -variable commit_type_is_amend \ > > + -command do_select_commit_type > > lappend disable_on_lock \ > > [list .mbar.commit entryconf [.mbar.commit index last] -state] > > > > @@ -3313,18 +3304,10 @@ set ui_comm .vpane.lower.commarea.buffer.frame.t > > set ui_coml .vpane.lower.commarea.buffer.header.l > > > > if {![is_enabled nocommit]} { > > - ${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \ > > - -text [mc "New Commit"] \ > > - -command do_select_commit_type \ > > - -variable selected_commit_type \ > > - -value new > > - lappend disable_on_lock \ > > - [list .vpane.lower.commarea.buffer.header.new conf -state] > > - ${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \ > > + ${NS}::checkbutton .vpane.lower.commarea.buffer.header.amend \ > > -text [mc "Amend Last Commit"] \ > > - -command do_select_commit_type \ > > - -variable selected_commit_type \ > > - -value amend > > + -variable commit_type_is_amend \ > > + -command do_select_commit_type > > lappend disable_on_lock \ > > [list .vpane.lower.commarea.buffer.header.amend conf -state] > > } > > @@ -3349,7 +3332,6 @@ pack $ui_coml -side left -fill x > > > > if {![is_enabled nocommit]} { > > pack .vpane.lower.commarea.buffer.header.amend -side right > > - pack .vpane.lower.commarea.buffer.header.new -side right > > } > > > > textframe .vpane.lower.commarea.buffer.frame > > diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl > > index 9e7412c..a522829 100644 > > --- a/lib/checkout_op.tcl > > +++ b/lib/checkout_op.tcl > > @@ -389,7 +389,7 @@ $err > > } > > > > method _after_readtree {} { > > - global selected_commit_type commit_type HEAD MERGE_HEAD PARENT > > + global commit_type HEAD MERGE_HEAD PARENT > > This cleans up an unused variable declaration. Nice. > > > global current_branch is_detached > > global ui_comm > > > > @@ -490,12 +490,12 @@ method _update_repo_state {} { > > # amend mode our file lists are accurate and we can avoid > > # the rescan. > > # > > - global selected_commit_type commit_type HEAD MERGE_HEAD PARENT > > + global commit_type_is_amend commit_type HEAD MERGE_HEAD PARENT > > global ui_comm > > > > unlock_index > > set name [_name $this] > > - set selected_commit_type new > > + set commit_type_is_amend 0 > > if {[string match amend* $commit_type]} { > > $ui_comm delete 0.0 end > > $ui_comm edit reset > > diff --git a/lib/commit.tcl b/lib/commit.tcl > > index 83620b7..384f18f 100644 > > --- a/lib/commit.tcl > > +++ b/lib/commit.tcl > > @@ -327,7 +327,7 @@ proc commit_writetree {curHEAD msg_p} { > > proc commit_committree {fd_wt curHEAD msg_p} { > > global HEAD PARENT MERGE_HEAD commit_type commit_author > > global current_branch > > - global ui_comm selected_commit_type > > + global ui_comm commit_type_is_amend > > global file_states selected_paths rescan_active > > global repo_config > > global env > > @@ -461,8 +461,8 @@ A rescan will be automatically started now. > > > > # -- Update in memory status > > # > > - set selected_commit_type new > > set commit_type normal > > + set commit_type_is_amend 0 > > set HEAD $cmt_id > > set PARENT $cmt_id > > set MERGE_HEAD [list] > > diff --git a/lib/index.tcl b/lib/index.tcl > > index b588db1..e07b7a3 100644 > > --- a/lib/index.tcl > > +++ b/lib/index.tcl > > @@ -466,19 +466,19 @@ proc do_revert_selection {} { > > } > > > > proc do_select_commit_type {} { > > - global commit_type selected_commit_type > > + global commit_type commit_type_is_amend > > > > - if {$selected_commit_type eq {new} > > + if {$commit_type_is_amend == 0 > > && [string match amend* $commit_type]} { > > create_new_commit > > - } elseif {$selected_commit_type eq {amend} > > + } elseif {$commit_type_is_amend == 1 > > && ![string match amend* $commit_type]} { > > Not exactly related to your change, but shouldn't these "string match > amend*" in the two ifs be assertions instead of checks? If > $commit_type_is_amend == 0, then $commit_type should _always_ be amend*, > and if $commit_type_is_amend == 1, then $commit_type should _never_ be > amend*. > AFAIU this now is, that the former 'selected_commit_type' was also used as a request for the commit type, you set it to the desired one, than call do_select_commit_type, it will than check if a state change actually happen, and if it was to request an amend, which may also fail, it goes back to !amend. Thus its not an assert. > I don't see assertions being used anywhere, but I suppose we should look > into them in the future. It would be great if you can start using > something like that here, but I'm fine with keeping this like it is > right now too. > > > load_last_commit > > > > # The amend request was rejected... > > # > > if {![string match amend* $commit_type]} { > > - set selected_commit_type new > > + set commit_type_is_amend 0 > > } > > } > > } > > I tested it on my setup and it works fine. Thanks. > > -- > Regards, > Pratyush Yadav
On Wed, Sep 11, 2019 at 10:15 PM Pratyush Yadav <me@yadavpratyush.com> wrote: > > Typo in the subject. s/checketton/checkbutton/\ Will re-roll and drop the actual keybinding patch, so that Birger can resend his part, Bert > > On 05/09/19 10:09PM, Bert Wesarg wrote: > > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> > > --- > > git-gui.sh | 36 +++++++++--------------------------- > > lib/checkout_op.tcl | 6 +++--- > > lib/commit.tcl | 4 ++-- > > lib/index.tcl | 8 ++++---- > > 4 files changed, 18 insertions(+), 36 deletions(-) > > > > diff --git a/git-gui.sh b/git-gui.sh > > index 5bc21b8..80a07d5 100755 > > --- a/git-gui.sh > > +++ b/git-gui.sh > > @@ -1341,6 +1341,7 @@ set HEAD {} > > set PARENT {} > > set MERGE_HEAD [list] > > set commit_type {} > > +set commit_type_is_amend 0 > > set empty_tree {} > > set current_branch {} > > set is_detached 0 > > @@ -1348,7 +1349,6 @@ set current_diff_path {} > > set is_3way_diff 0 > > set is_submodule_diff 0 > > set is_conflict_diff 0 > > -set selected_commit_type new > > set diff_empty_count 0 > > > > set nullid "0000000000000000000000000000000000000000" > > @@ -1435,7 +1435,7 @@ proc PARENT {} { > > } > > > > proc force_amend {} { > > - global selected_commit_type > > + global commit_type_is_amend > > global HEAD PARENT MERGE_HEAD commit_type > > > > repository_state newType newHEAD newMERGE_HEAD > > @@ -1444,7 +1444,7 @@ proc force_amend {} { > > set MERGE_HEAD $newMERGE_HEAD > > set commit_type $newType > > > > - set selected_commit_type amend > > + set commit_type_is_amend 1 > > Why do we need a separate variable for this? Why not just check > commit_type to know whether it is an amend or not? My guess after > reading the patch is that we need to associate a variable with the > checkbutton to decide its state, and since there are multiple types of > amend that commit_type can be (amend, amend-initial, amend-merge), it is > not easy to use it directly. Am I guessing correctly? > > > do_select_commit_type > > } > > > > @@ -2828,19 +2828,10 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} { > > menu .mbar.commit > > > > if {![is_enabled nocommit]} { > > - .mbar.commit add radiobutton \ > > - -label [mc "New Commit"] \ > > - -command do_select_commit_type \ > > - -variable selected_commit_type \ > > - -value new > > - lappend disable_on_lock \ > > - [list .mbar.commit entryconf [.mbar.commit index last] -state] > > - > > - .mbar.commit add radiobutton \ > > + .mbar.commit add checkbutton \ > > -label [mc "Amend Last Commit"] \ > > - -command do_select_commit_type \ > > - -variable selected_commit_type \ > > - -value amend > > + -variable commit_type_is_amend \ > > + -command do_select_commit_type > > lappend disable_on_lock \ > > [list .mbar.commit entryconf [.mbar.commit index last] -state] > > > > @@ -3313,18 +3304,10 @@ set ui_comm .vpane.lower.commarea.buffer.frame.t > > set ui_coml .vpane.lower.commarea.buffer.header.l > > > > if {![is_enabled nocommit]} { > > - ${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \ > > - -text [mc "New Commit"] \ > > - -command do_select_commit_type \ > > - -variable selected_commit_type \ > > - -value new > > - lappend disable_on_lock \ > > - [list .vpane.lower.commarea.buffer.header.new conf -state] > > - ${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \ > > + ${NS}::checkbutton .vpane.lower.commarea.buffer.header.amend \ > > -text [mc "Amend Last Commit"] \ > > - -command do_select_commit_type \ > > - -variable selected_commit_type \ > > - -value amend > > + -variable commit_type_is_amend \ > > + -command do_select_commit_type > > lappend disable_on_lock \ > > [list .vpane.lower.commarea.buffer.header.amend conf -state] > > } > > @@ -3349,7 +3332,6 @@ pack $ui_coml -side left -fill x > > > > if {![is_enabled nocommit]} { > > pack .vpane.lower.commarea.buffer.header.amend -side right > > - pack .vpane.lower.commarea.buffer.header.new -side right > > } > > > > textframe .vpane.lower.commarea.buffer.frame > > diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl > > index 9e7412c..a522829 100644 > > --- a/lib/checkout_op.tcl > > +++ b/lib/checkout_op.tcl > > @@ -389,7 +389,7 @@ $err > > } > > > > method _after_readtree {} { > > - global selected_commit_type commit_type HEAD MERGE_HEAD PARENT > > + global commit_type HEAD MERGE_HEAD PARENT > > This cleans up an unused variable declaration. Nice. > > > global current_branch is_detached > > global ui_comm > > > > @@ -490,12 +490,12 @@ method _update_repo_state {} { > > # amend mode our file lists are accurate and we can avoid > > # the rescan. > > # > > - global selected_commit_type commit_type HEAD MERGE_HEAD PARENT > > + global commit_type_is_amend commit_type HEAD MERGE_HEAD PARENT > > global ui_comm > > > > unlock_index > > set name [_name $this] > > - set selected_commit_type new > > + set commit_type_is_amend 0 > > if {[string match amend* $commit_type]} { > > $ui_comm delete 0.0 end > > $ui_comm edit reset > > diff --git a/lib/commit.tcl b/lib/commit.tcl > > index 83620b7..384f18f 100644 > > --- a/lib/commit.tcl > > +++ b/lib/commit.tcl > > @@ -327,7 +327,7 @@ proc commit_writetree {curHEAD msg_p} { > > proc commit_committree {fd_wt curHEAD msg_p} { > > global HEAD PARENT MERGE_HEAD commit_type commit_author > > global current_branch > > - global ui_comm selected_commit_type > > + global ui_comm commit_type_is_amend > > global file_states selected_paths rescan_active > > global repo_config > > global env > > @@ -461,8 +461,8 @@ A rescan will be automatically started now. > > > > # -- Update in memory status > > # > > - set selected_commit_type new > > set commit_type normal > > + set commit_type_is_amend 0 > > set HEAD $cmt_id > > set PARENT $cmt_id > > set MERGE_HEAD [list] > > diff --git a/lib/index.tcl b/lib/index.tcl > > index b588db1..e07b7a3 100644 > > --- a/lib/index.tcl > > +++ b/lib/index.tcl > > @@ -466,19 +466,19 @@ proc do_revert_selection {} { > > } > > > > proc do_select_commit_type {} { > > - global commit_type selected_commit_type > > + global commit_type commit_type_is_amend > > > > - if {$selected_commit_type eq {new} > > + if {$commit_type_is_amend == 0 > > && [string match amend* $commit_type]} { > > create_new_commit > > - } elseif {$selected_commit_type eq {amend} > > + } elseif {$commit_type_is_amend == 1 > > && ![string match amend* $commit_type]} { > > Not exactly related to your change, but shouldn't these "string match > amend*" in the two ifs be assertions instead of checks? If > $commit_type_is_amend == 0, then $commit_type should _always_ be amend*, > and if $commit_type_is_amend == 1, then $commit_type should _never_ be > amend*. > > I don't see assertions being used anywhere, but I suppose we should look > into them in the future. It would be great if you can start using > something like that here, but I'm fine with keeping this like it is > right now too. > > > load_last_commit > > > > # The amend request was rejected... > > # > > if {![string match amend* $commit_type]} { > > - set selected_commit_type new > > + set commit_type_is_amend 0 > > } > > } > > } > > I tested it on my setup and it works fine. Thanks. > > -- > Regards, > Pratyush Yadav
On 12/09/19 09:35PM, Bert Wesarg wrote: > On Wed, Sep 11, 2019 at 10:15 PM Pratyush Yadav <me@yadavpratyush.com> wrote: > > > > Typo in the subject. s/checketton/checkbutton/ > > > > On 05/09/19 10:09PM, Bert Wesarg wrote: > > > diff --git a/lib/index.tcl b/lib/index.tcl > > > index b588db1..e07b7a3 100644 > > > --- a/lib/index.tcl > > > +++ b/lib/index.tcl > > > @@ -466,19 +466,19 @@ proc do_revert_selection {} { > > > } > > > > > > proc do_select_commit_type {} { > > > - global commit_type selected_commit_type > > > + global commit_type commit_type_is_amend > > > > > > - if {$selected_commit_type eq {new} > > > + if {$commit_type_is_amend == 0 > > > && [string match amend* $commit_type]} { > > > create_new_commit > > > - } elseif {$selected_commit_type eq {amend} > > > + } elseif {$commit_type_is_amend == 1 > > > && ![string match amend* $commit_type]} { > > > > Not exactly related to your change, but shouldn't these "string match > > amend*" in the two ifs be assertions instead of checks? If > > $commit_type_is_amend == 0, then $commit_type should _always_ be amend*, > > and if $commit_type_is_amend == 1, then $commit_type should _never_ be > > amend*. > > > > AFAIU this now is, that the former 'selected_commit_type' was also > used as a request for the commit type, you set it to the desired one, > than call do_select_commit_type, it will than check if a state change > actually happen, and if it was to request an amend, which may also > fail, it goes back to !amend. > > Thus its not an assert. Thanks for explaining. While I'm not the biggest fan of this design, let's just keep it this way for now.
diff --git a/git-gui.sh b/git-gui.sh index 5bc21b8..80a07d5 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -1341,6 +1341,7 @@ set HEAD {} set PARENT {} set MERGE_HEAD [list] set commit_type {} +set commit_type_is_amend 0 set empty_tree {} set current_branch {} set is_detached 0 @@ -1348,7 +1349,6 @@ set current_diff_path {} set is_3way_diff 0 set is_submodule_diff 0 set is_conflict_diff 0 -set selected_commit_type new set diff_empty_count 0 set nullid "0000000000000000000000000000000000000000" @@ -1435,7 +1435,7 @@ proc PARENT {} { } proc force_amend {} { - global selected_commit_type + global commit_type_is_amend global HEAD PARENT MERGE_HEAD commit_type repository_state newType newHEAD newMERGE_HEAD @@ -1444,7 +1444,7 @@ proc force_amend {} { set MERGE_HEAD $newMERGE_HEAD set commit_type $newType - set selected_commit_type amend + set commit_type_is_amend 1 do_select_commit_type } @@ -2828,19 +2828,10 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} { menu .mbar.commit if {![is_enabled nocommit]} { - .mbar.commit add radiobutton \ - -label [mc "New Commit"] \ - -command do_select_commit_type \ - -variable selected_commit_type \ - -value new - lappend disable_on_lock \ - [list .mbar.commit entryconf [.mbar.commit index last] -state] - - .mbar.commit add radiobutton \ + .mbar.commit add checkbutton \ -label [mc "Amend Last Commit"] \ - -command do_select_commit_type \ - -variable selected_commit_type \ - -value amend + -variable commit_type_is_amend \ + -command do_select_commit_type lappend disable_on_lock \ [list .mbar.commit entryconf [.mbar.commit index last] -state] @@ -3313,18 +3304,10 @@ set ui_comm .vpane.lower.commarea.buffer.frame.t set ui_coml .vpane.lower.commarea.buffer.header.l if {![is_enabled nocommit]} { - ${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \ - -text [mc "New Commit"] \ - -command do_select_commit_type \ - -variable selected_commit_type \ - -value new - lappend disable_on_lock \ - [list .vpane.lower.commarea.buffer.header.new conf -state] - ${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \ + ${NS}::checkbutton .vpane.lower.commarea.buffer.header.amend \ -text [mc "Amend Last Commit"] \ - -command do_select_commit_type \ - -variable selected_commit_type \ - -value amend + -variable commit_type_is_amend \ + -command do_select_commit_type lappend disable_on_lock \ [list .vpane.lower.commarea.buffer.header.amend conf -state] } @@ -3349,7 +3332,6 @@ pack $ui_coml -side left -fill x if {![is_enabled nocommit]} { pack .vpane.lower.commarea.buffer.header.amend -side right - pack .vpane.lower.commarea.buffer.header.new -side right } textframe .vpane.lower.commarea.buffer.frame diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl index 9e7412c..a522829 100644 --- a/lib/checkout_op.tcl +++ b/lib/checkout_op.tcl @@ -389,7 +389,7 @@ $err } method _after_readtree {} { - global selected_commit_type commit_type HEAD MERGE_HEAD PARENT + global commit_type HEAD MERGE_HEAD PARENT global current_branch is_detached global ui_comm @@ -490,12 +490,12 @@ method _update_repo_state {} { # amend mode our file lists are accurate and we can avoid # the rescan. # - global selected_commit_type commit_type HEAD MERGE_HEAD PARENT + global commit_type_is_amend commit_type HEAD MERGE_HEAD PARENT global ui_comm unlock_index set name [_name $this] - set selected_commit_type new + set commit_type_is_amend 0 if {[string match amend* $commit_type]} { $ui_comm delete 0.0 end $ui_comm edit reset diff --git a/lib/commit.tcl b/lib/commit.tcl index 83620b7..384f18f 100644 --- a/lib/commit.tcl +++ b/lib/commit.tcl @@ -327,7 +327,7 @@ proc commit_writetree {curHEAD msg_p} { proc commit_committree {fd_wt curHEAD msg_p} { global HEAD PARENT MERGE_HEAD commit_type commit_author global current_branch - global ui_comm selected_commit_type + global ui_comm commit_type_is_amend global file_states selected_paths rescan_active global repo_config global env @@ -461,8 +461,8 @@ A rescan will be automatically started now. # -- Update in memory status # - set selected_commit_type new set commit_type normal + set commit_type_is_amend 0 set HEAD $cmt_id set PARENT $cmt_id set MERGE_HEAD [list] diff --git a/lib/index.tcl b/lib/index.tcl index b588db1..e07b7a3 100644 --- a/lib/index.tcl +++ b/lib/index.tcl @@ -466,19 +466,19 @@ proc do_revert_selection {} { } proc do_select_commit_type {} { - global commit_type selected_commit_type + global commit_type commit_type_is_amend - if {$selected_commit_type eq {new} + if {$commit_type_is_amend == 0 && [string match amend* $commit_type]} { create_new_commit - } elseif {$selected_commit_type eq {amend} + } elseif {$commit_type_is_amend == 1 && ![string match amend* $commit_type]} { load_last_commit # The amend request was rejected... # if {![string match amend* $commit_type]} { - set selected_commit_type new + set commit_type_is_amend 0 } } }
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> --- git-gui.sh | 36 +++++++++--------------------------- lib/checkout_op.tcl | 6 +++--- lib/commit.tcl | 4 ++-- lib/index.tcl | 8 ++++---- 4 files changed, 18 insertions(+), 36 deletions(-)