diff mbox series

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

Message ID pull.1584.git.1694176123471.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series completion(switch/checkout): treat --track and -t the same | expand

Commit Message

Johannes Schindelin Sept. 8, 2023, 12:28 p.m. UTC 
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(-)


base-commit: 94e83dcf5b5faaa22e32729305f8fd7090bfdfed

Comments

Junio C Hamano Sept. 8, 2023, 3:51 p.m. UTC | #1 
"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.

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

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

> 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'.

>  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 Sept. 8, 2023, 4:14 p.m. UTC | #2 
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).

>> +	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'.
Junio C Hamano Sept. 8, 2023, 4:25 p.m. UTC | #3 
Todd Zullinger <tmz@pobox.com> writes:

> 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 Sept. 10, 2023, 8:22 a.m. UTC | #4 
Hi Todd,

On Fri, 8 Sep 2023, Todd Zullinger wrote:

> 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

>
> >> +	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 Sept. 11, 2023, 6:09 a.m. UTC | #5 
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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.
diff mbox series

Patch

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