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@xxxxxxxxx> with minor setup
modifications on my part.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
Reviewed-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
Test-code-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
---
On Fri, Apr 21, 2017 at 2:28 PM, SZEDER Gábor <szeder.dev@xxxxxxxxx> wrote:
>
> On Tue, Apr 18, 2017 at 3:31 PM, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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