From: Pratyush Yadav <me@yadavpratyush.com>
To: Bert Wesarg <bert.wesarg@googlemail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton
Date: Thu, 12 Sep 2019 01:45:00 +0530 [thread overview]
Message-ID: <20190911201500.6utotzdrwsp6qoe3@yadavpratyush.com> (raw)
In-Reply-To: <ab1f68cc8552e405c9d04622be1e728ab81bda17.1567713659.git.bert.wesarg@googlemail.com>
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.
--
Regards,
Pratyush Yadav
next prev parent reply other threads:[~2019-09-11 20:15 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-05 20:09 [PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton Bert Wesarg
2019-09-05 20:09 ` [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu Bert Wesarg
2019-09-11 20:55 ` Pratyush Yadav
2019-09-12 6:05 ` Birger Skogeng Pedersen
2019-09-12 16:29 ` Pratyush Yadav
2019-09-12 18:41 ` [PATCH v3] git-gui: add hotkey to toggle "Amend Last Commit" Birger Skogeng Pedersen
2019-09-13 14:37 ` Pratyush Yadav
2019-09-13 21:11 ` [PATCH v4] " Birger Skogeng Pedersen
2019-09-13 21:32 ` Birger Skogeng Pedersen
2019-09-13 22:11 ` Pratyush Yadav
2019-09-14 9:07 ` Birger Skogeng Pedersen
2019-09-14 9:18 ` [PATCH v5] " Birger Skogeng Pedersen
2019-09-14 17:48 ` Pratyush Yadav
2019-09-16 12:23 ` Birger Skogeng Pedersen
2019-09-12 21:34 ` [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu Marc Branchaud
2019-09-12 22:23 ` Philip Oakley
2019-09-13 7:50 ` Birger Skogeng Pedersen
2019-09-13 13:55 ` Marc Branchaud
2019-09-13 17:47 ` Pratyush Yadav
2019-09-05 20:33 ` [PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton Pratyush Yadav
2019-09-11 20:15 ` Pratyush Yadav [this message]
2019-09-12 19:35 ` Bert Wesarg
2019-09-13 17:14 ` Pratyush Yadav
2019-09-12 19:39 ` Bert Wesarg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190911201500.6utotzdrwsp6qoe3@yadavpratyush.com \
--to=me@yadavpratyush.com \
--cc=bert.wesarg@googlemail.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).