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. Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]> Reviewed-by: SZEDER Gábor <[email protected]> Test-code-by: SZEDER Gábor <[email protected]> --- On Fri, Apr 21, 2017 at 2:28 PM, SZEDER Gábor <[email protected]> wrote: > > On Tue, Apr 18, 2017 at 3:31 PM, Ævar Arnfjörð Bjarmason <[email protected]> > 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 <[email protected]> >> --- >> 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
