threads / patch / 60209

patchcompletion(switch/checkout): treat --track and -t the same

Subject: [PATCH] completion(switch/checkout): treat --track and -t the same

## tl;dr

6 messages between Sep 8, 2023 and Sep 11, 2023. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Johannes Schindelin via GitGitGadget· Sep 8, 2023, 12:28 UTC · lore
From: Johannes Schindelin <johannes.schindelin@gmx.de>

When `git switch --track ` is to be completed, only remote refs are eligible because that is what the `--track` option targets.

And when the short-hand `-t` is used instead, the same _should_ happen. Let's make it so.

Note that the bug exists both in the completions of `switch` and `completion`, even if it manifests in slightly different ways: While the completion of `git switch -t ` will not even look at remote refs, the completion of `git checkout -t ` will look at both remote _and_ local refs. Both should look only at remote refs.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Fix the completion of git switch/git checkout: Treat --track and -t the
    same
    
    Just something that was nagging me for one year too many.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1584%2Fdscho%2Fcomplete-switch-t-properly-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1584/dscho/complete-switch-t-properly-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1584
 contrib/completion/git-completion.bash |  4 ++--
 t/t9902-completion.sh                  | 12 ++++++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)
Show changes to 2 files +12 −4

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 133ec92bfae..745dc901fbe 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1607,7 +1607,7 @@ _git_checkout ()
 
 		if [ -n "$(__git_find_on_cmdline "-b -B -d --detach --orphan")" ]; then
 			__git_complete_refs --mode="refs"
-		elif [ -n "$(__git_find_on_cmdline "--track")" ]; then
+		elif [ -n "$(__git_find_on_cmdline "-t --track")" ]; then
 			__git_complete_refs --mode="remote-heads"
 		else
 			__git_complete_refs $dwim_opt --mode="refs"
@@ -2514,7 +2514,7 @@ _git_switch ()
 
 		if [ -n "$(__git_find_on_cmdline "-c -C -d --detach")" ]; then
 			__git_complete_refs --mode="refs"
-		elif [ -n "$(__git_find_on_cmdline "--track")" ]; then
+		elif [ -n "$(__git_find_on_cmdline "-t --track")" ]; then
 			__git_complete_refs --mode="remote-heads"
 		else
 			__git_complete_refs $dwim_opt --mode="heads"
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 8835e16e811..df8bc44c285 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1622,14 +1622,22 @@ test_expect_success 'git checkout - with -d, complete only references' '
 '
 
 test_expect_success 'git switch - with --track, complete only remote branches' '
-	test_completion "git switch --track " <<-\EOF
+:	test_completion "git switch --track " <<-\EOF &&
+	other/branch-in-other Z
+	other/main-in-other Z
+	EOF
+	test_completion "git switch -t " <<-\EOF
 	other/branch-in-other Z
 	other/main-in-other Z
 	EOF
 '
 
 test_expect_success 'git checkout - with --track, complete only remote branches' '
-	test_completion "git checkout --track " <<-\EOF
+	test_completion "git checkout --track " <<-\EOF &&
+	other/branch-in-other Z
+	other/main-in-other Z
+	EOF
+	test_completion "git checkout -t " <<-\EOF
 	other/branch-in-other Z
 	other/main-in-other Z
 	EOF

base-commit: 94e83dcf5b5faaa22e32729305f8fd7090bfdfed

-- 
gitgitgadget
Junio C Hamano· Sep 8, 2023, 15:51 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH] completion(switch/checkout): treat --track and -t the same

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When `git switch --track ` is to be completed, only remote refs are
> eligible because that is what the `--track` option targets.
OK.  Presumably that is the same for "checkout --track".
> And when the short-hand `-t` is used instead, the same _should_ happen.

That sounds sensible. The code change for _git_checkout() and _git_switch() is surprisingly simple ;-) "-t" was not handled any specially at all and fell through to "refs" complation.

> Let's make it so.
Sounds good.
Show 5 quoted lines
> Note that the bug exists both in the completions of `switch` and
> `completion`, even if it manifests in slightly different ways: While
> the completion of `git switch -t ` will not even look at remote refs,
> the completion of `git checkout -t ` will look at both remote _and_
> local refs. Both should look only at remote refs.
Correct.
Show 22 quoted lines
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 133ec92bfae..745dc901fbe 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1607,7 +1607,7 @@ _git_checkout ()
>  
>  		if [ -n "$(__git_find_on_cmdline "-b -B -d --detach --orphan")" ]; then
>  			__git_complete_refs --mode="refs"
> -		elif [ -n "$(__git_find_on_cmdline "--track")" ]; then
> +		elif [ -n "$(__git_find_on_cmdline "-t --track")" ]; then
>  			__git_complete_refs --mode="remote-heads"
>  		else
>  			__git_complete_refs $dwim_opt --mode="refs"
> @@ -2514,7 +2514,7 @@ _git_switch ()
>  
>  		if [ -n "$(__git_find_on_cmdline "-c -C -d --detach")" ]; then
>  			__git_complete_refs --mode="refs"
> -		elif [ -n "$(__git_find_on_cmdline "--track")" ]; then
> +		elif [ -n "$(__git_find_on_cmdline "-t --track")" ]; then
>  			__git_complete_refs --mode="remote-heads"
>  		else
>  			__git_complete_refs $dwim_opt --mode="heads"

The fallback behaviours are different, which was adequately described in the proposed log message. As "switch" does not want to auto-detach upon receiving a ref that is not a local branch, while "checkout" does, the difference is justifiable.

Show 18 quoted lines
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 8835e16e811..df8bc44c285 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1622,14 +1622,22 @@ test_expect_success 'git checkout - with -d, complete only references' '
>  '
>  
>  test_expect_success 'git switch - with --track, complete only remote branches' '
> -	test_completion "git switch --track " <<-\EOF
> +:	test_completion "git switch --track " <<-\EOF &&
> +	other/branch-in-other Z
> +	other/main-in-other Z
> +	EOF
> +	test_completion "git switch -t " <<-\EOF
>  	other/branch-in-other Z
>  	other/main-in-other Z
>  	EOF
>  '
So, this demonstrates that '-t' behaves the same way as '--track'.
Show 10 quoted lines
>  test_expect_success 'git checkout - with --track, complete only remote branches' '
> -	test_completion "git checkout --track " <<-\EOF
> +	test_completion "git checkout --track " <<-\EOF &&
> +	other/branch-in-other Z
> +	other/main-in-other Z
> +	EOF
> +	test_completion "git checkout -t " <<-\EOF
>  	other/branch-in-other Z
>  	other/main-in-other Z
>  	EOF

This is, too. If we had a test for "-t" without the code fix, we would have seen local branches in its output, but now we can see the candidates are limited to the remote ones.

Good.
Will queue.  Thanks.
Todd Zullinger· Sep 8, 2023, 16:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] completion(switch/checkout): treat --track and -t the same

Junio C Hamano wrote:
Show 12 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 8835e16e811..df8bc44c285 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -1622,14 +1622,22 @@ test_expect_success 'git checkout - with -d, complete only references' '
>>  '
>>  
>>  test_expect_success 'git switch - with --track, complete only remote branches' '
>> -	test_completion "git switch --track " <<-\EOF
>> +:	test_completion "git switch --track " <<-\EOF &&

Is this new leading ":" intended? It looks out of place (though perhaps I just don't unerstand the context well enough).

Show 10 quoted lines
>> +	other/branch-in-other Z
>> +	other/main-in-other Z
>> +	EOF
>> +	test_completion "git switch -t " <<-\EOF
>>  	other/branch-in-other Z
>>  	other/main-in-other Z
>>  	EOF
>>  '
> 
> So, this demonstrates that '-t' behaves the same way as '--track'.
-- 
Todd
Junio C Hamano· Sep 8, 2023, 16:25 UTC · re: Todd Zullinger · lore

Re: [PATCH] completion(switch/checkout): treat --track and -t the same

Todd Zullinger <tmz@pobox.com> writes:
Show 17 quoted lines
> Junio C Hamano wrote:
>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>>> index 8835e16e811..df8bc44c285 100755
>>> --- a/t/t9902-completion.sh
>>> +++ b/t/t9902-completion.sh
>>> @@ -1622,14 +1622,22 @@ test_expect_success 'git checkout - with -d, complete only references' '
>>>  '
>>>  
>>>  test_expect_success 'git switch - with --track, complete only remote branches' '
>>> -	test_completion "git switch --track " <<-\EOF
>>> +:	test_completion "git switch --track " <<-\EOF &&
>
> Is this new leading ":" intended?  It looks out of place
> (though perhaps I just don't unerstand the context well
> enough).
Good eyes.  It makes it an expensive no-op ;-)
Johannes Schindelin· Sep 10, 2023, 08:22 UTC · re: Todd Zullinger · lore

Re: [PATCH] completion(switch/checkout): treat --track and -t the same

Hi Todd,
On Fri, 8 Sep 2023, Todd Zullinger wrote:
Show 17 quoted lines
> Junio C Hamano wrote:
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> >> index 8835e16e811..df8bc44c285 100755
> >> --- a/t/t9902-completion.sh
> >> +++ b/t/t9902-completion.sh
> >> @@ -1622,14 +1622,22 @@ test_expect_success 'git checkout - with -d, complete only references' '
> >>  '
> >>
> >>  test_expect_success 'git switch - with --track, complete only remote branches' '
> >> -	test_completion "git switch --track " <<-\EOF
> >> +:	test_completion "git switch --track " <<-\EOF &&
>
> Is this new leading ":" intended?  It looks out of place
> (though perhaps I just don't unerstand the context well
> enough).

Thanks for catching this. It is a debugging left-over, when I wanted to make sure that the `-t` validation I added would run immediately.

I see that Junio helpfully dropped it before merging down to `next`, so I will refrain from sending a v2.

Ciao, Johannes

Show 15 quoted lines
>
> >> +	other/branch-in-other Z
> >> +	other/main-in-other Z
> >> +	EOF
> >> +	test_completion "git switch -t " <<-\EOF
> >>  	other/branch-in-other Z
> >>  	other/main-in-other Z
> >>  	EOF
> >>  '
> >
> > So, this demonstrates that '-t' behaves the same way as '--track'.
>
> --
> Todd
>
Junio C Hamano· Sep 11, 2023, 06:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH] completion(switch/checkout): treat --track and -t the same

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
> Thanks for catching this. It is a debugging left-over, when I wanted to
> make sure that the `-t` validation I added would run immediately.
>
> I see that Junio helpfully dropped it before merging down to `next`, so I
> will refrain from sending a v2.
Yup.  Thanks, all.

← back to recent threads