[1/2] git-gui: convert new/amend commit radiobutton to checketton
diff mbox series

Message ID ab1f68cc8552e405c9d04622be1e728ab81bda17.1567713659.git.bert.wesarg@googlemail.com
State New
Headers show
Series
  • [1/2] git-gui: convert new/amend commit radiobutton to checketton
Related show

Commit Message

Bert Wesarg Sept. 5, 2019, 8:09 p.m. UTC
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(-)

Comments

Pratyush Yadav Sept. 5, 2019, 8:33 p.m. UTC | #1
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]
Pratyush Yadav Sept. 11, 2019, 8:15 p.m. UTC | #2
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.
Bert Wesarg Sept. 12, 2019, 7:35 p.m. UTC | #3
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
Bert Wesarg Sept. 12, 2019, 7:39 p.m. UTC | #4
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
Pratyush Yadav Sept. 13, 2019, 5:14 p.m. UTC | #5
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.

Patch
diff mbox series

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
 		}
 	}
 }