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

["SZEDER Gábor" <szeder.dev@gmail.com>]

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