[PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote>

[PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote>

[Ævar Arnfjörð Bjarmason]

Change the completion of "push --delete <remote> <ref>" to complete
refs on that <remote>, not all refs. Before this e.g. cloning git.git
and doing "git push --delete origin p<TAB>" will complete nothing,
whereas origin/p<TAB> will uselessly complete origin/pu.

Now p<TAB> will complete as "pu". The completion of giving --delete
later, e.g. "git push origin --delete p<TAB>" remains unchanged, this
is a bug, but is a general existing limitation of the bash completion,
and not how git-push is documented, so I'm not fixing that case.

I looked over t9902-completion.sh but couldn't quickly find out how to
add a test for this, but all the existing tests pass, and all my
manual testing of "git push --delete <remote> ..." does the right
thing now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/completion/git-completion.bash | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1150164d5c..2e5b3ed776 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -701,7 +701,7 @@ __git_complete_revlist ()
 __git_complete_remote_or_refspec ()
 {
 	local cur_="$cur" cmd="${words[1]}"
-	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0 delete=0
 	if [ "$cmd" = "remote" ]; then
 		((c++))
 	fi
@@ -709,6 +709,7 @@ __git_complete_remote_or_refspec ()
 		i="${words[c]}"
 		case "$i" in
 		--mirror) [ "$cmd" = "push" ] && no_complete_refspec=1 ;;
+		--delete) delete=1 ;;
 		--all)
 			case "$cmd" in
 			push) no_complete_refspec=1 ;;
@@ -761,7 +762,9 @@ __git_complete_remote_or_refspec ()
 		fi
 		;;
 	push)
-		if [ $lhs = 1 ]; then
+		if [ $delete = 1 ]; then
+			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
+		elif [ $lhs = 1 ]; then
 			__git_complete_refs --pfx="$pfx" --cur="$cur_"
 		else
 			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
-- 
2.11.0

Re: [PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote>

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the completion of "push --delete <remote> <ref>" to complete
> refs on that <remote>, not all refs. Before this e.g. cloning git.git
> and doing "git push --delete origin p<TAB>" will complete nothing,
> whereas origin/p<TAB> will uselessly complete origin/pu.
>
> Now p<TAB> will complete as "pu". The completion of giving --delete
> later, e.g. "git push origin --delete p<TAB>" remains unchanged, this
> is a bug, but is a general existing limitation of the bash completion,
> and not how git-push is documented, so I'm not fixing that case.
>
> I looked over t9902-completion.sh but couldn't quickly find out how to
> add a test for this, but all the existing tests pass, and all my
> manual testing of "git push --delete <remote> ..." does the right
> thing now.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---

This looks like a sensible thing to want to add.  Perhaps somebody
more familiar with the completion tests can help with t/ and also
give us general comments.  Until then, let me queue it as is on
'pu'.

Thanks.

>  contrib/completion/git-completion.bash | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1150164d5c..2e5b3ed776 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -701,7 +701,7 @@ __git_complete_revlist ()
>  __git_complete_remote_or_refspec ()
>  {
>  	local cur_="$cur" cmd="${words[1]}"
> -	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
> +	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0 delete=0
>  	if [ "$cmd" = "remote" ]; then
>  		((c++))
>  	fi
> @@ -709,6 +709,7 @@ __git_complete_remote_or_refspec ()
>  		i="${words[c]}"
>  		case "$i" in
>  		--mirror) [ "$cmd" = "push" ] && no_complete_refspec=1 ;;
> +		--delete) delete=1 ;;
>  		--all)
>  			case "$cmd" in
>  			push) no_complete_refspec=1 ;;
> @@ -761,7 +762,9 @@ __git_complete_remote_or_refspec ()
>  		fi
>  		;;
>  	push)
> -		if [ $lhs = 1 ]; then
> +		if [ $delete = 1 ]; then
> +			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
> +		elif [ $lhs = 1 ]; then
>  			__git_complete_refs --pfx="$pfx" --cur="$cur_"
>  		else
>  			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"

Re: [PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote>

[SZEDER Gábor]

On Tue, Apr 18, 2017 at 3:31 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Change the completion of "push --delete <remote> <ref>" to complete
> refs on that <remote>, not all refs.

Good.

> Before this e.g. cloning git.git
> and doing "git push --delete origin p<TAB>" will complete nothing,

Well, it will complete all local branches starting with 'p', but
perhaps you don't happen to have any.

> whereas origin/p<TAB> will uselessly complete origin/pu.
> 
> Now p<TAB> will complete as "pu". The completion of giving --delete
> later, e.g. "git push origin --delete p<TAB>" remains unchanged, this
> is a bug, but is a general existing limitation of the bash completion,
> and not how git-push is documented, so I'm not fixing that case.
> 
> I looked over t9902-completion.sh but couldn't quickly find out how to
> add a test for this,

Yeah, this helper function has to look at the whole command line to do
its thing, and we don't have other unit test-like tests doing
something like that.

One option would be something like this:

@@ -1162,6 +1162,19 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '
 	test_cmp expected out
 '
 
+test_expect_success '__git_complete_remote_or_refspec - push -d' '
+	sed -e "s/Z$//" >expected <<-EOF &&
+	master-in-other Z
+	EOF
+	(
+		words=(git push -d other ma) &&
+		cword=${#words[@]} cur=${words[cword-1]} &&
+		__git_complete_remote_or_refspec &&
+		print_comp
+	) &&
+	test_cmp expected out
+'
+
 test_expect_success 'teardown after ref completion' '
 	git branch -d matching-branch &&
 	git tag -d matching-tag &&

This is chatty, no question about that, but it only excercises
__git_complete_remote_or_refspec() (and __git_refs() behind its back),
and thus fits right in there with other refs completion tests.


The other option would be something like this:

@@ -1348,6 +1361,10 @@ test_expect_success 'complete tree filename with metacharacters' '
 	EOF
 '
 
+test_expect_success 'complete remote refs for git push -d' '
+	test_completion "git push -d other ma" "master-in-other "
+'
+
 test_expect_success 'send-email' '
 	test_completion "git send-email --cov" "--cover-letter " &&
 	test_completion "git send-email ma" "master "

While this is much more compact, it does excercise the whole shebang,
therefore it has to go to the integration tests.  However, at that
point in the test script there aren't any remote refs in the
repository (and, consequently this test will fail as it is), so you
would need to add a few, which in turn would most likely require
adjustments in other tests.

I'm partial to the former, even if it's more lines of code, because if
it were to fail, then it already narrowed down the scope where we'd
need to look for the cause of the failure.

Take your pick :)

> but all the existing tests pass, and all my
> manual testing of "git push --delete <remote> ..." does the right
> thing now.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1150164d5c..2e5b3ed776 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -701,7 +701,7 @@ __git_complete_revlist ()
>  __git_complete_remote_or_refspec ()
>  {
>  	local cur_="$cur" cmd="${words[1]}"
> -	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
> +	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0 delete=0
>  	if [ "$cmd" = "remote" ]; then
>  		((c++))
>  	fi
> @@ -709,6 +709,7 @@ __git_complete_remote_or_refspec ()
>  		i="${words[c]}"
>  		case "$i" in
>  		--mirror) [ "$cmd" = "push" ] && no_complete_refspec=1 ;;
> +		--delete) delete=1 ;;

I noticed the two identical __git_complete_refs() calls in the hunk
below.  How about:

  -d|--delete) [ "$cmd" = "push" ] && lhs=0 ;;

First, it recognizes the short option, too.
Second, with 'push -d' any ref is interpreted as the right hand side
of a refspec whose left hand side is empty (i.e. '-d pu' means ':pu').
That 'lhs=0' tells the rest of the function to complete the right hand
side of a refspec, i.e. in case of 'push' to list remote refs, which
is exactly what you want.  And you won't need the extra if branch in
the hunk below, or the new local variable.
In this case, however, we should check that the command is 'push' as
well, just in case the other commands whose completion is driven by
this helper function get these options in the future.

>  		--all)
>  			case "$cmd" in
>  			push) no_complete_refspec=1 ;;
> @@ -761,7 +762,9 @@ __git_complete_remote_or_refspec ()
>  		fi
>  		;;
>  	push)
> -		if [ $lhs = 1 ]; then
> +		if [ $delete = 1 ]; then
> +			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
> +		elif [ $lhs = 1 ]; then
>  			__git_complete_refs --pfx="$pfx" --cur="$cur_"
>  		else
>  			__git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
> -- 
> 2.11.0

[PATCH v2] completion: expand "push --delete <remote> <ref>" for refs on that <remote>

[Ævar Arnfjörð Bjarmason]

Change the completion of "push --delete <remote> <ref>" to complete
refs on that <remote>, not all refs.

Before this cloning git.git and doing "git push --delete origin
p<TAB>" will complete nothing, since a fresh clone of git.git will
have no "pu" branch, whereas origin/p<TAB> will uselessly complete
origin/pu, but fully qualified references aren't accepted by
"--delete".

Now p<TAB> will complete as "pu". The completion of giving --delete
later, e.g. "git push origin --delete p<TAB>" remains unchanged, this
is a bug, but is a general existing limitation of the bash completion,
and not how git-push is documented, so I'm not fixing that case, but
adding a failing TODO test for it.

The testing code was supplied by SZEDER Gábor in
<20170421122832.24617-1-szeder.dev@gmail.com> with minor setup
modifications on my part.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Reviewed-by: SZEDER Gábor <szeder.dev@gmail.com>
Test-code-by: SZEDER Gábor <szeder.dev@gmail.com>
---

On Fri, Apr 21, 2017 at 2:28 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Tue, Apr 18, 2017 at 3:31 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> Change the completion of "push --delete <remote> <ref>" to complete
>> refs on that <remote>, not all refs.
>
> Good.
>
>> Before this e.g. cloning git.git
>> and doing "git push --delete origin p<TAB>" will complete nothing,
>
> Well, it will complete all local branches starting with 'p', but
> perhaps you don't happen to have any.
>
>> whereas origin/p<TAB> will uselessly complete origin/pu.
>>
>> Now p<TAB> will complete as "pu". The completion of giving --delete
>> later, e.g. "git push origin --delete p<TAB>" remains unchanged, this
>> is a bug, but is a general existing limitation of the bash completion,
>> and not how git-push is documented, so I'm not fixing that case.
>>
>> I looked over t9902-completion.sh but couldn't quickly find out how to
>> add a test for this,
>
> Yeah, this helper function has to look at the whole command line to do
> its thing, and we don't have other unit test-like tests doing
> something like that.
>
> One option would be something like this:
>
> @@ -1162,6 +1162,19 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '
>         test_cmp expected out
>  '
>
> +test_expect_success '__git_complete_remote_or_refspec - push -d' '
> +       sed -e "s/Z$//" >expected <<-EOF &&
> +       master-in-other Z
> +       EOF
> +       (
> +               words=(git push -d other ma) &&
> +               cword=${#words[@]} cur=${words[cword-1]} &&
> +               __git_complete_remote_or_refspec &&
> +               print_comp
> +       ) &&
> +       test_cmp expected out
> +'
> +
>  test_expect_success 'teardown after ref completion' '
>         git branch -d matching-branch &&
>         git tag -d matching-tag &&
>
> This is chatty, no question about that, but it only excercises
> __git_complete_remote_or_refspec() (and __git_refs() behind its back),
> and thus fits right in there with other refs completion tests.
>
>
> The other option would be something like this:
>
> @@ -1348,6 +1361,10 @@ test_expect_success 'complete tree filename with metacharacters' '
>         EOF
>  '
>
> +test_expect_success 'complete remote refs for git push -d' '
> +       test_completion "git push -d other ma" "master-in-other "
> +'
> +
>  test_expect_success 'send-email' '
>         test_completion "git send-email --cov" "--cover-letter " &&
>         test_completion "git send-email ma" "master "
>
> While this is much more compact, it does excercise the whole shebang,
> therefore it has to go to the integration tests.  However, at that
> point in the test script there aren't any remote refs in the
> repository (and, consequently this test will fail as it is), so you
> would need to add a few, which in turn would most likely require
> adjustments in other tests.
>
> I'm partial to the former, even if it's more lines of code, because if
> it were to fail, then it already narrowed down the scope where we'd
> need to look for the cause of the failure.
>
> Take your pick :)
>
>> but all the existing tests pass, and all my
>> manual testing of "git push --delete <remote> ..." does the right
>> thing now.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  contrib/completion/git-completion.bash | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 1150164d5c..2e5b3ed776 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -701,7 +701,7 @@ __git_complete_revlist ()
>>  __git_complete_remote_or_refspec ()
>>  {
>>       local cur_="$cur" cmd="${words[1]}"
>> -     local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>> +     local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0 delete=0
>>       if [ "$cmd" = "remote" ]; then
>>               ((c++))
>>       fi
>> @@ -709,6 +709,7 @@ __git_complete_remote_or_refspec ()
>>               i="${words[c]}"
>>               case "$i" in
>>               --mirror) [ "$cmd" = "push" ] && no_complete_refspec=1 ;;
>> +             --delete) delete=1 ;;
>
> I noticed the two identical __git_complete_refs() calls in the hunk
> below.  How about:
>
>   -d|--delete) [ "$cmd" = "push" ] && lhs=0 ;;
>
> First, it recognizes the short option, too.
> Second, with 'push -d' any ref is interpreted as the right hand side
> of a refspec whose left hand side is empty (i.e. '-d pu' means ':pu').
> That 'lhs=0' tells the rest of the function to complete the right hand
> side of a refspec, i.e. in case of 'push' to list remote refs, which
> is exactly what you want.  And you won't need the extra if branch in
> the hunk below, or the new local variable.
> In this case, however, we should check that the command is 'push' as
> well, just in case the other commands whose completion is driven by
> this helper function get these options in the future.

Thanks a lot. I changed all of this as you suggested & integrated your
testing code, now also with a TODO test for "git push origin
--delete".

>>               --all)
>>                       case "$cmd" in
>>                       push) no_complete_refspec=1 ;;
>> @@ -761,7 +762,9 @@ __git_complete_remote_or_refspec ()
>>               fi
>>               ;;
>>       push)
>> -             if [ $lhs = 1 ]; then
>> +             if [ $delete = 1 ]; then
>> +                     __git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
>> +             elif [ $lhs = 1 ]; then
>>                       __git_complete_refs --pfx="$pfx" --cur="$cur_"
>>               else
>>                       __git_complete_refs --remote="$remote" --pfx="$pfx" --cur="$cur_"
>> --
>> 2.11.0

 contrib/completion/git-completion.bash |  1 +
 t/t9902-completion.sh                  | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1150164d5c..b617019075 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -709,6 +709,7 @@ __git_complete_remote_or_refspec ()
 		i="${words[c]}"
 		case "$i" in
 		--mirror) [ "$cmd" = "push" ] && no_complete_refspec=1 ;;
+		-d|--delete) [ "$cmd" = "push" ] && lhs=0 ;;
 		--all)
 			case "$cmd" in
 			push) no_complete_refspec=1 ;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5ed28135be..2cb999ecfa 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1457,4 +1457,38 @@ test_expect_failure 'complete with tilde expansion' '
 	test_completion "git add ~/tmp/" "~/tmp/file"
 '
 
+test_expect_success 'setup other remote for remote reference completion' '
+	git remote add other otherrepo &&
+	git fetch other
+'
+
+for flag in -d --delete
+do
+	test_expect_success "__git_complete_remote_or_refspec - push $flag other" '
+		sed -e "s/Z$//" >expected <<-EOF &&
+		master-in-other Z
+		EOF
+		(
+			words=(git push '$flag' other ma) &&
+			cword=${#words[@]} cur=${words[cword-1]} &&
+			__git_complete_remote_or_refspec &&
+			print_comp
+		) &&
+		test_cmp expected out
+	'
+
+	test_expect_failure "__git_complete_remote_or_refspec - push other $flag" '
+		sed -e "s/Z$//" >expected <<-EOF &&
+		master-in-other Z
+		EOF
+		(
+			words=(git push other '$flag' ma) &&
+			cword=${#words[@]} cur=${words[cword-1]} &&
+			__git_complete_remote_or_refspec &&
+			print_comp
+		) &&
+		test_cmp expected out
+	'
+done
+
 test_done
-- 
2.11.0