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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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