* [PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote> @ 2017-04-18 13:31 Ævar Arnfjörð Bjarmason 2017-04-19 3:45 ` Junio C Hamano 2017-04-21 12:28 ` SZEDER Gábor 0 siblings, 2 replies; 4+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-18 13:31 UTC (permalink / raw) To: git Cc: Junio C Hamano, SZEDER Gábor, Jay Soffian, Æ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. 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. 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 ;; --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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote> 2017-04-18 13:31 [PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote> Ævar Arnfjörð Bjarmason @ 2017-04-19 3:45 ` Junio C Hamano 2017-04-21 12:28 ` SZEDER Gábor 1 sibling, 0 replies; 4+ messages in thread From: Junio C Hamano @ 2017-04-19 3:45 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, SZEDER Gábor, Jay Soffian Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- 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. > 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 ;; > --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_" ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote> 2017-04-18 13:31 [PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote> Ævar Arnfjörð Bjarmason 2017-04-19 3:45 ` Junio C Hamano @ 2017-04-21 12:28 ` SZEDER Gábor 2017-04-22 17:55 ` [PATCH v2] " Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 4+ messages in thread From: SZEDER Gábor @ 2017-04-21 12:28 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: SZEDER Gábor, Junio C Hamano, Jay Soffian, git 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] completion: expand "push --delete <remote> <ref>" for refs on that <remote> 2017-04-21 12:28 ` SZEDER Gábor @ 2017-04-22 17:55 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 4+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-22 17:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, SZEDER Gábor, Jay Soffian, Ævar Arnfjörð Bjarmason 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-22 17:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-18 13:31 [PATCH] completion: expand "push --delete <remote> <ref>" for refs on that <remote> Ævar Arnfjörð Bjarmason 2017-04-19 3:45 ` Junio C Hamano 2017-04-21 12:28 ` SZEDER Gábor 2017-04-22 17:55 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).