completion: expand "push --delete <remote> <ref>" for refs on that <remote>
PATCH 1/1 completion: expand "push --delete <remote> <ref>" for refs on that <remote>
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.
--- contrib/completion/git-completion.bash | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
contrib/completion/git-completion.bash +5 -2
@@ -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_"
Signature · 1 line
2.11.0
Quoted 10 lines · Æ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.
Quoted 7 lines
I looked over
t9902-completion.shbut 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 <[email protected]>---
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.
Quoted 35 lines
contrib/completion/git-completion.bash | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)contrib/completion/git-completion.bash +5
-2@@ -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_"
Quoted 2 lines · Ævar Arnfjörð Bjarmason
Change the completion of "push --delete <remote> <ref>" to complete refs on that <remote>, not all refs.
Good.
Quoted 2 lines
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.
Quoted 9 lines
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.shbut 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 :)
Quoted 23 lines
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 <[email protected]>--- contrib/completion/git-completion.bash | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)contrib/completion/git-completion.bash +1
-1@@ -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
contrib/completion/git-completion.bash __git_complete_remote_or_refspec () · line 712
Quoted 4 lines
contrib/completion/git-completion.bash +1
-0@@ -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.
Quoted 16 lines
contrib/completion/git-completion.bash +3
-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_"Signature · 1 line
2.11.0
PATCH v2 1/1 completion: expand "push --delete <remote> <ref>" for refs on that <remote>
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 <[email protected]> with minor setup modifications on my part.
---
Quoted 115 lines · SZEDER Gábor
Quoted 2 lines · Ævar Arnfjörð Bjarmason
Change the completion of "push --delete <remote> <ref>" to complete refs on that <remote>, not all refs.
Good.
Quoted 2 lines
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.
Quoted 9 lines
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.shbut 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:
(quoted diff) +13
-0@@ -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:
(quoted diff) +4
-0@@ -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 :)
Quoted 23 lines
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 <[email protected]>--- contrib/completion/git-completion.bash | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)contrib/completion/git-completion.bash +1
-1@@ -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++)) ficontrib/completion/git-completion.bash __git_complete_remote_or_refspec () · line 712
Quoted 4 lines
contrib/completion/git-completion.bash +1
-0@@ -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 ;;
Quoted 10 lines
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".
Quoted 8 lines
Quoted 8 lines
contrib/completion/git-completion.bash +0
-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
Quoted 8 lines
Quoted 8 lines
contrib/completion/git-completion.bash +3
-1+ 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(+)
contrib/completion/git-completion.bash +1 -0
@@ -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 ;;
t/t9902-completion.sh +34 -0
@@ -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
Signature · 1 line
2.11.0