# [PATCH v5 0/3] completion: refactor and zsh wrapper 14 messages from 2012-10-14 to 2012-10-30. Participants: Felipe Contreras, Matthieu Moy, SZEDER Gábor. Thread: https://gitlist.dev/t/31814 ## Felipe Contreras, 2012-10-14 15:52 Subject: [PATCH v5 0/3] completion: refactor and zsh wrapper Message-ID: <1350229971-9343-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1350229971-9343-1-git-send-email-felipe.contreras%40gmail.com ``` Hi, Here's a bit of reorganition. I'm introducing a new __gitcompadd helper that is useful to wrapp all changes to COMPREPLY. 2nd and 3rd patches show how it's useful. The zsh wrapper is now very very simple, but I haven't received much feedback yet. I hope it will get in at some point in time. Felipe Contreras (3): completion: add new __gitcompadd helper tests: use __gitcompadd to simplify completion tests completion: add new zsh completion contrib/completion/git-completion.bash | 65 ++++++++++++++++++---------------- contrib/completion/git-completion.zsh | 48 +++++++++++++++++++++++++ t/t9902-completion.sh | 29 +++++---------- 3 files changed, 91 insertions(+), 51 deletions(-) create mode 100644 contrib/completion/git-completion.zsh -- 1.7.12.1 ``` ## Felipe Contreras, 2012-10-14 15:52 Subject: [PATCH v5 1/3] completion: add new __gitcompadd helper Message-ID: <1350229971-9343-2-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1350229971-9343-2-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1350229971-9343-1-git-send-email-felipe.contreras@gmail.com> ``` The idea is to never touch the COMPREPLY variable directly. This allows other completion systems override __gitcompadd, and do something different instead. Also, this allows the simplifcation of the completino tests (separate patch). There should be no functional changes. Signed-off-by: Felipe Contreras --- contrib/completion/git-completion.bash | 65 ++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 31 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index d743e56..01325de 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -225,6 +225,11 @@ _get_comp_words_by_ref () fi fi +__gitcompadd () +{ + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) +} + # Generates completion reply with compgen, appending a space to possible # completion words, if necessary. # It accepts 1 to 4 arguments: @@ -238,13 +243,11 @@ __gitcomp () case "$cur_" in --*=) - COMPREPLY=() + __gitcompadd ;; *) local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" \ - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ - -- "$cur_")) + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" "" ;; esac } @@ -261,7 +264,7 @@ __gitcomp () __gitcomp_nl () { local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }" } __git_heads () @@ -486,7 +489,7 @@ __git_complete_remote_or_refspec () case "$cmd" in push) no_complete_refspec=1 ;; fetch) - COMPREPLY=() + __gitcompadd return ;; *) ;; @@ -502,7 +505,7 @@ __git_complete_remote_or_refspec () return fi if [ $no_complete_refspec = 1 ]; then - COMPREPLY=() + __gitcompadd return fi [ "$remote" = "." ] && remote= @@ -776,7 +779,7 @@ _git_am () " return esac - COMPREPLY=() + __gitcompadd } _git_apply () @@ -796,7 +799,7 @@ _git_apply () " return esac - COMPREPLY=() + __gitcompadd } _git_add () @@ -811,7 +814,7 @@ _git_add () " return esac - COMPREPLY=() + __gitcompadd } _git_archive () @@ -856,7 +859,7 @@ _git_bisect () __gitcomp_nl "$(__git_refs)" ;; *) - COMPREPLY=() + __gitcompadd ;; esac } @@ -965,7 +968,7 @@ _git_clean () return ;; esac - COMPREPLY=() + __gitcompadd } _git_clone () @@ -989,7 +992,7 @@ _git_clone () return ;; esac - COMPREPLY=() + __gitcompadd } _git_commit () @@ -1023,7 +1026,7 @@ _git_commit () " return esac - COMPREPLY=() + __gitcompadd } _git_describe () @@ -1154,7 +1157,7 @@ _git_fsck () return ;; esac - COMPREPLY=() + __gitcompadd } _git_gc () @@ -1165,7 +1168,7 @@ _git_gc () return ;; esac - COMPREPLY=() + __gitcompadd } _git_gitk () @@ -1242,7 +1245,7 @@ _git_init () return ;; esac - COMPREPLY=() + __gitcompadd } _git_ls_files () @@ -1261,7 +1264,7 @@ _git_ls_files () return ;; esac - COMPREPLY=() + __gitcompadd } _git_ls_remote () @@ -1377,7 +1380,7 @@ _git_mergetool () return ;; esac - COMPREPLY=() + __gitcompadd } _git_merge_base () @@ -1393,7 +1396,7 @@ _git_mv () return ;; esac - COMPREPLY=() + __gitcompadd } _git_name_rev () @@ -1563,7 +1566,7 @@ _git_send_email () return ;; esac - COMPREPLY=() + __gitcompadd } _git_stage () @@ -1616,7 +1619,7 @@ _git_config () local remote="${prev#remote.}" remote="${remote%.fetch}" if [ -z "$cur" ]; then - COMPREPLY=("refs/heads/") + __gitcompadd "refs/heads/" return fi __gitcomp_nl "$(__git_refs_remotes "$remote")" @@ -1676,7 +1679,7 @@ _git_config () return ;; *.*) - COMPREPLY=() + __gitcompadd return ;; esac @@ -2056,7 +2059,7 @@ _git_remote () __gitcomp "$c" ;; *) - COMPREPLY=() + __gitcompadd ;; esac } @@ -2100,7 +2103,7 @@ _git_rm () return ;; esac - COMPREPLY=() + __gitcompadd } _git_shortlog () @@ -2170,7 +2173,7 @@ _git_stash () if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then __gitcomp "$subcommands" else - COMPREPLY=() + __gitcompadd fi ;; esac @@ -2183,14 +2186,14 @@ _git_stash () __gitcomp "--index --quiet" ;; show,--*|drop,--*|branch,--*) - COMPREPLY=() + __gitcompadd ;; show,*|apply,*|drop,*|pop,*|branch,*) __gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \ | sed -n -e 's/:.*//p')" ;; *) - COMPREPLY=() + __gitcompadd ;; esac fi @@ -2307,7 +2310,7 @@ _git_svn () __gitcomp "--revision= --parent" ;; *) - COMPREPLY=() + __gitcompadd ;; esac fi @@ -2332,13 +2335,13 @@ _git_tag () case "$prev" in -m|-F) - COMPREPLY=() + __gitcompadd ;; -*|tag) if [ $f = 1 ]; then __gitcomp_nl "$(__git_tags)" else - COMPREPLY=() + __gitcompadd fi ;; *) -- 1.7.12.1 ``` ## Felipe Contreras, 2012-10-14 15:52 Subject: [PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests Message-ID: <1350229971-9343-3-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1350229971-9343-3-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1350229971-9343-1-git-send-email-felipe.contreras@gmail.com> ``` Signed-off-by: Felipe Contreras --- t/t9902-completion.sh | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 92d7eb4..49c6eb4 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -39,19 +39,18 @@ _get_comp_words_by_ref () done } -print_comp () +__gitcompadd () { - local IFS=$'\n' - echo "${COMPREPLY[*]}" > out + compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}" > out } run_completion () { - local -a COMPREPLY _words + local -a _words local _cword _words=( $1 ) (( _cword = ${#_words[@]} - 1 )) - __git_wrap__git_main && print_comp + __git_wrap__git_main } test_completion () @@ -70,12 +69,10 @@ test_expect_success '__gitcomp - trailing space - options' ' --reset-author Z EOF ( - local -a COMPREPLY && cur="--re" && __gitcomp "--dry-run --reuse-message= --reedit-message= --reset-author" && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + IFS="$newline" ) && test_cmp expected out ' @@ -88,12 +85,10 @@ test_expect_success '__gitcomp - trailing space - config keys' ' browser.Z EOF ( - local -a COMPREPLY && cur="br" && __gitcomp "branch. branch.autosetupmerge branch.autosetuprebase browser." && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + IFS="$newline" ) && test_cmp expected out ' @@ -104,12 +99,10 @@ test_expect_success '__gitcomp - option parameter' ' resolve Z EOF ( - local -a COMPREPLY && cur="--strategy=re" && __gitcomp "octopus ours recursive resolve subtree " "" "re" && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + IFS="$newline" ) && test_cmp expected out ' @@ -120,12 +113,10 @@ test_expect_success '__gitcomp - prefix' ' branch.maint.mergeoptions Z EOF ( - local -a COMPREPLY && cur="branch.me" && __gitcomp "remote merge mergeoptions rebase " "branch.maint." "me" && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + IFS="$newline" ) && test_cmp expected out ' @@ -136,12 +127,10 @@ test_expect_success '__gitcomp - suffix' ' branch.maint.Z EOF ( - local -a COMPREPLY && cur="branch.me" && __gitcomp "master maint next pu " "branch." "ma" "." && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + IFS="$newline" ) && test_cmp expected out ' -- 1.7.12.1 ``` ## Felipe Contreras, 2012-10-14 15:52 Subject: [PATCH v5 3/3] completion: add new zsh completion Message-ID: <1350229971-9343-4-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1350229971-9343-4-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1350229971-9343-1-git-send-email-felipe.contreras@gmail.com> ``` It seems there's always issues with zsh's bash completion emulation. I've tried to fix as many as I could and most of the fixes are already in the latest version of zsh, but still, there are issues. There is no point in going through all that pain; the emulation is easy to achieve, and this patch works better than zsh's emulation. Signed-off-by: Felipe Contreras --- v5: * Even more simplification by using __gitcompadd v4: * Simplification updates for the latest bash completion v3: * Simplification * Avoid COMPREPLY; call compadd directly * Fix _get_comp_words_by_ref contrib/completion/git-completion.zsh | 48 +++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 contrib/completion/git-completion.zsh diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh new file mode 100644 index 0000000..dbb5261 --- /dev/null +++ b/contrib/completion/git-completion.zsh @@ -0,0 +1,48 @@ +#compdef git gitk + +# zsh completion wrapper for git +# +# You need git's bash completion script installed somewhere, by default on the +# same directory as this script. +# +# If your script is on ~/.git-completion.sh instead, you can configure it on +# your ~/.zshrc: +# +# zstyle ':completion:*:*:git:*' script ~/.git-completion.sh +# +# The recommended way to install this script is to copy to +# '~/.zsh/completion/_git', and then add the following to your ~/.zshrc file: +# +# fpath=(~/.zsh/completion $fpath) + +complete () +{ + # do nothing + return 0 +} + +zstyle -s ":completion:*:*:git:*" script script +test -z "$script" && script="$(dirname ${funcsourcetrace[1]%:*})"/git-completion.bash +ZSH_VERSION='' . "$script" + +__gitcompadd () +{ + compadd -Q -S "$4" -P "$2" -p "${(M)cur#*[=:]}" -- ${=1} && _ret=0 +} + +_git () +{ + local _ret=1 + () { + emulate -L ksh + local cur cword prev + cur=${words[CURRENT-1]} + prev=${words[CURRENT-2]} + let cword=CURRENT-1 + __${service}_main + } + let _ret && _default -S '' && _ret=0 + return _ret +} + +_git -- 1.7.12.1 ``` ## Felipe Contreras, 2012-10-15 06:38 Subject: Re: [PATCH v5 3/3] completion: add new zsh completion Message-ID: URL: https://gitlist.dev/e/CAMP44s1XG43c0MDCa2mSA%3DR5YMF%3DjNSGVdv%3DUrHKsHmrmRCXMw%40mail.gmail.com In-Reply-To: <1350229971-9343-4-git-send-email-felipe.contreras@gmail.com> ``` On Sun, Oct 14, 2012 at 5:52 PM, Felipe Contreras wrote: > +__gitcompadd () > +{ > + compadd -Q -S "$4" -P "$2" -p "${(M)cur#*[=:]}" -- ${=1} && _ret=0 I just found a bug, should be: compadd -Q -S "$4" -P "${(M)cur#*[=:]}" -p "$2" -- ${=1} && _ret=0 -- Felipe Contreras ``` ## Matthieu Moy, 2012-10-15 16:45 Subject: Re: [PATCH v5 0/3] completion: refactor and zsh wrapper Message-ID: URL: https://gitlist.dev/e/vpqr4oz65wr.fsf%40grenoble-inp.fr In-Reply-To: <1350229971-9343-1-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > Hi, > > Here's a bit of reorganition. I'm introducing a new __gitcompadd helper that is > useful to wrapp all changes to COMPREPLY. 2nd and 3rd patches show how it's > useful. > > The zsh wrapper is now very very simple, but I haven't received much feedback > yet. I hope it will get in at some point in time. I didn't review the code, but I've installed your patch series, and it seems to work well. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Felipe Contreras, 2012-10-16 00:24 Subject: Re: [PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests Message-ID: URL: https://gitlist.dev/e/CAMP44s2ej4oz4E4tYk7hNjBXe-pvK-v2kQDuz9pg42stBtL61w%40mail.gmail.com In-Reply-To: <1350229971-9343-3-git-send-email-felipe.contreras@gmail.com> ``` On Sun, Oct 14, 2012 at 5:52 PM, Felipe Contreras wrote: > -print_comp () > +__gitcompadd () > { > - local IFS=$'\n' > - echo "${COMPREPLY[*]}" > out > + compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}" > out Rather: compgen -W "$1" -P "$2" -S "$4" -- "$3" > out -- Felipe Contreras ``` ## SZEDER Gábor, 2012-10-17 17:28 Subject: Re: [PATCH v5 1/3] completion: add new __gitcompadd helper Message-ID: <20121017172808.GD2156@goldbirke> URL: https://gitlist.dev/e/20121017172808.GD2156%40goldbirke In-Reply-To: <1350229971-9343-2-git-send-email-felipe.contreras@gmail.com> ``` On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote: > The idea is to never touch the COMPREPLY variable directly. > > This allows other completion systems override __gitcompadd, and do > something different instead. > > Also, this allows the simplifcation of the completino tests (separate > patch). > > There should be no functional changes. > > Signed-off-by: Felipe Contreras > --- > contrib/completion/git-completion.bash | 65 ++++++++++++++++++---------------- > 1 file changed, 34 insertions(+), 31 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index d743e56..01325de 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -225,6 +225,11 @@ _get_comp_words_by_ref () > fi > fi > > +__gitcompadd () > +{ > + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) > +} > + > # Generates completion reply with compgen, appending a space to possible > # completion words, if necessary. > # It accepts 1 to 4 arguments: > @@ -238,13 +243,11 @@ __gitcomp () > > case "$cur_" in > --*=) > - COMPREPLY=() > + __gitcompadd > ;; > *) > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" \ > - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ > - -- "$cur_")) > + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" "" > ;; > esac > } > @@ -261,7 +264,7 @@ __gitcomp () > __gitcomp_nl () > { > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }" > } I feel hesitant about this change. One of the ways I'm exploring to fix the issues with shell metacharacters and expansion in compgen is to actually replace compgen. We already iterate over all possible completion words in __gitcomp_1(), so it doesn't make much of a difference to do the filtering for the current word while we are at it. However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen ...)), and tha basic idea of never touching COMPREPLY directly make this basically impossible. > __git_heads () > @@ -486,7 +489,7 @@ __git_complete_remote_or_refspec () > case "$cmd" in > push) no_complete_refspec=1 ;; > fetch) > - COMPREPLY=() > + __gitcompadd > return > ;; > *) ;; > @@ -502,7 +505,7 @@ __git_complete_remote_or_refspec () > return > fi > if [ $no_complete_refspec = 1 ]; then > - COMPREPLY=() > + __gitcompadd > return > fi > [ "$remote" = "." ] && remote= > @@ -776,7 +779,7 @@ _git_am () > " > return > esac > - COMPREPLY=() > + __gitcompadd These changes effectively run compgen in a subshell to generate an empty completion reply. While it doesn't really matter on Linux, it'll add another half a tenth of a second delay in those cases on my Windows machine. At least it should be conditional, i.e. $(compgen ...) shouldn't be executed when there are no possible completion words. However, I think those COMPREPLY=() assignments are pointless anyway. COMPREPLY is always empty when completion functions are invoked, so there is no need to explicitly set it to an empty array when we don't provide any words for completion. Their only use is basically to explicitly tell us humans that in those cases we don't offer any words for completion. But we don't do that consistently: there are several places without offering words for completion and without COMPREPLY=(), e.g. the '__git_has_doubledash && return' pattern. Perhaps it would be time to get rid of these COMPREPLY=() assignments? > } > > _git_apply () > @@ -796,7 +799,7 @@ _git_apply () > " > return > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_add () > @@ -811,7 +814,7 @@ _git_add () > " > return > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_archive () > @@ -856,7 +859,7 @@ _git_bisect () > __gitcomp_nl "$(__git_refs)" > ;; > *) > - COMPREPLY=() > + __gitcompadd > ;; > esac > } > @@ -965,7 +968,7 @@ _git_clean () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_clone () > @@ -989,7 +992,7 @@ _git_clone () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_commit () > @@ -1023,7 +1026,7 @@ _git_commit () > " > return > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_describe () > @@ -1154,7 +1157,7 @@ _git_fsck () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_gc () > @@ -1165,7 +1168,7 @@ _git_gc () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_gitk () > @@ -1242,7 +1245,7 @@ _git_init () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_ls_files () > @@ -1261,7 +1264,7 @@ _git_ls_files () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_ls_remote () > @@ -1377,7 +1380,7 @@ _git_mergetool () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_merge_base () > @@ -1393,7 +1396,7 @@ _git_mv () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_name_rev () > @@ -1563,7 +1566,7 @@ _git_send_email () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_stage () > @@ -1616,7 +1619,7 @@ _git_config () > local remote="${prev#remote.}" > remote="${remote%.fetch}" > if [ -z "$cur" ]; then > - COMPREPLY=("refs/heads/") > + __gitcompadd "refs/heads/" > return > fi > __gitcomp_nl "$(__git_refs_remotes "$remote")" > @@ -1676,7 +1679,7 @@ _git_config () > return > ;; > *.*) > - COMPREPLY=() > + __gitcompadd > return > ;; > esac > @@ -2056,7 +2059,7 @@ _git_remote () > __gitcomp "$c" > ;; > *) > - COMPREPLY=() > + __gitcompadd > ;; > esac > } > @@ -2100,7 +2103,7 @@ _git_rm () > return > ;; > esac > - COMPREPLY=() > + __gitcompadd > } > > _git_shortlog () > @@ -2170,7 +2173,7 @@ _git_stash () > if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then > __gitcomp "$subcommands" > else > - COMPREPLY=() > + __gitcompadd > fi > ;; > esac > @@ -2183,14 +2186,14 @@ _git_stash () > __gitcomp "--index --quiet" > ;; > show,--*|drop,--*|branch,--*) > - COMPREPLY=() > + __gitcompadd > ;; > show,*|apply,*|drop,*|pop,*|branch,*) > __gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \ > | sed -n -e 's/:.*//p')" > ;; > *) > - COMPREPLY=() > + __gitcompadd > ;; > esac > fi > @@ -2307,7 +2310,7 @@ _git_svn () > __gitcomp "--revision= --parent" > ;; > *) > - COMPREPLY=() > + __gitcompadd > ;; > esac > fi > @@ -2332,13 +2335,13 @@ _git_tag () > > case "$prev" in > -m|-F) > - COMPREPLY=() > + __gitcompadd > ;; > -*|tag) > if [ $f = 1 ]; then > __gitcomp_nl "$(__git_tags)" > else > - COMPREPLY=() > + __gitcompadd > fi > ;; > *) > -- > 1.7.12.1 > ``` ## SZEDER Gábor, 2012-10-17 17:50 Subject: Re: [PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests Message-ID: <20121017175040.GE2156@goldbirke> URL: https://gitlist.dev/e/20121017175040.GE2156%40goldbirke In-Reply-To: <1350229971-9343-3-git-send-email-felipe.contreras@gmail.com> ``` On Sun, Oct 14, 2012 at 05:52:50PM +0200, Felipe Contreras wrote: > Signed-off-by: Felipe Contreras > --- > t/t9902-completion.sh | 29 +++++++++-------------------- > 1 file changed, 9 insertions(+), 20 deletions(-) > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index 92d7eb4..49c6eb4 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -39,19 +39,18 @@ _get_comp_words_by_ref () > done > } > > -print_comp () > +__gitcompadd () > { > - local IFS=$'\n' > - echo "${COMPREPLY[*]}" > out > + compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}" > out > } Please don't. Running compgen is a fundamental part of the completion script, therefore tests must run it as it is in the completion script and not some copy of it. > run_completion () > { > - local -a COMPREPLY _words > + local -a _words > local _cword > _words=( $1 ) > (( _cword = ${#_words[@]} - 1 )) > - __git_wrap__git_main && print_comp > + __git_wrap__git_main > } > > test_completion () > @@ -70,12 +69,10 @@ test_expect_success '__gitcomp - trailing space - options' ' > --reset-author Z > EOF > ( > - local -a COMPREPLY && I'm not sure what I was thinking when I wrote this, but using the local keyword while not within a function but in a subshell doesn't seem to be that clever ;) Maybe just a copy-paste from the local variable declarations of run-completion(). > cur="--re" && > __gitcomp "--dry-run --reuse-message= --reedit-message= > --reset-author" && > - IFS="$newline" && > - echo "${COMPREPLY[*]}" > out And here I should have used print_comp(). All these can be cleaned up without overriding __gitcompadd() and potentialy compromising correctness. Will send a patch in a minute. > + IFS="$newline" This was only necessary for echoing the array. > ) && > test_cmp expected out > ' > @@ -88,12 +85,10 @@ test_expect_success '__gitcomp - trailing space - config keys' ' > browser.Z > EOF > ( > - local -a COMPREPLY && > cur="br" && > __gitcomp "branch. branch.autosetupmerge > branch.autosetuprebase browser." && > - IFS="$newline" && > - echo "${COMPREPLY[*]}" > out > + IFS="$newline" > ) && > test_cmp expected out > ' > @@ -104,12 +99,10 @@ test_expect_success '__gitcomp - option parameter' ' > resolve Z > EOF > ( > - local -a COMPREPLY && > cur="--strategy=re" && > __gitcomp "octopus ours recursive resolve subtree > " "" "re" && > - IFS="$newline" && > - echo "${COMPREPLY[*]}" > out > + IFS="$newline" > ) && > test_cmp expected out > ' > @@ -120,12 +113,10 @@ test_expect_success '__gitcomp - prefix' ' > branch.maint.mergeoptions Z > EOF > ( > - local -a COMPREPLY && > cur="branch.me" && > __gitcomp "remote merge mergeoptions rebase > " "branch.maint." "me" && > - IFS="$newline" && > - echo "${COMPREPLY[*]}" > out > + IFS="$newline" > ) && > test_cmp expected out > ' > @@ -136,12 +127,10 @@ test_expect_success '__gitcomp - suffix' ' > branch.maint.Z > EOF > ( > - local -a COMPREPLY && > cur="branch.me" && > __gitcomp "master maint next pu > " "branch." "ma" "." && > - IFS="$newline" && > - echo "${COMPREPLY[*]}" > out > + IFS="$newline" > ) && > test_cmp expected out > ' > -- > 1.7.12.1 > ``` ## SZEDER Gábor, 2012-10-17 17:54 Subject: [PATCH] completion: clean up __gitcomp() tests Message-ID: <20121017175428.GF2156@goldbirke> URL: https://gitlist.dev/e/20121017175428.GF2156%40goldbirke In-Reply-To: <20121017175040.GE2156@goldbirke> ``` Clean up two issues in the tests I added in 74a8c849 (tests: add tests for the __gitcomp() completion helper function, 2012-04-17): - The COMPREPLY array is created using 'local -a' while in a subshell. However, the local keyword should only be used in a shell function, and a variable created in a subshell is by definition local to that subshell. Use 'declare -a' instead. - The contents of the COMPREPLY array is written through an IFS fiddling + echo + redirection combo, although there is the print_comp() helper function for exactly this purpose. Signed-off-by: SZEDER Gábor --- t/t9902-completion.sh | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index cbd0fb66..cc375ed0 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -70,8 +70,6 @@ test_completion_long () test_completion "$1" } -newline=$'\n' - test_expect_success '__gitcomp - trailing space - options' ' sed -e "s/Z$//" >expected <<-\EOF && --reuse-message=Z @@ -79,12 +77,11 @@ test_expect_success '__gitcomp - trailing space - options' ' --reset-author Z EOF ( - local -a COMPREPLY && + declare -a COMPREPLY && cur="--re" && __gitcomp "--dry-run --reuse-message= --reedit-message= --reset-author" && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + print_comp ) && test_cmp expected out ' @@ -97,12 +94,11 @@ test_expect_success '__gitcomp - trailing space - config keys' ' browser.Z EOF ( - local -a COMPREPLY && + declare -a COMPREPLY && cur="br" && __gitcomp "branch. branch.autosetupmerge branch.autosetuprebase browser." && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + print_comp ) && test_cmp expected out ' @@ -113,12 +109,11 @@ test_expect_success '__gitcomp - option parameter' ' resolve Z EOF ( - local -a COMPREPLY && + declare -a COMPREPLY && cur="--strategy=re" && __gitcomp "octopus ours recursive resolve subtree " "" "re" && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + print_comp ) && test_cmp expected out ' @@ -129,12 +124,11 @@ test_expect_success '__gitcomp - prefix' ' branch.maint.mergeoptions Z EOF ( - local -a COMPREPLY && + declare -a COMPREPLY && cur="branch.me" && __gitcomp "remote merge mergeoptions rebase " "branch.maint." "me" && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + print_comp ) && test_cmp expected out ' @@ -145,12 +139,11 @@ test_expect_success '__gitcomp - suffix' ' branch.maint.Z EOF ( - local -a COMPREPLY && + declare -a COMPREPLY && cur="branch.me" && __gitcomp "master maint next pu " "branch." "ma" "." && - IFS="$newline" && - echo "${COMPREPLY[*]}" > out + print_comp ) && test_cmp expected out ' -- 1.8.0.rc0.83.gc8e1777 ``` ## Felipe Contreras, 2012-10-17 18:21 Subject: Re: [PATCH] completion: clean up __gitcomp() tests Message-ID: URL: https://gitlist.dev/e/CAMP44s2Daj-yvTqKc-fMiqvA64z_odZy_dhGvtB7rM--LbuQRQ%40mail.gmail.com In-Reply-To: <20121017175428.GF2156@goldbirke> ``` On Wed, Oct 17, 2012 at 7:54 PM, SZEDER Gábor wrote: > Clean up two issues in the tests I added in 74a8c849 (tests: add tests > for the __gitcomp() completion helper function, 2012-04-17): > > - The COMPREPLY array is created using 'local -a' while in a > subshell. However, the local keyword should only be used in a > shell function, and a variable created in a subshell is by > definition local to that subshell. Use 'declare -a' instead. > > - The contents of the COMPREPLY array is written through an IFS > fiddling + echo + redirection combo, although there is the > print_comp() helper function for exactly this purpose. Makes sense. But this code seems awfully similar, a helper function might help. -- Felipe Contreras ``` ## Felipe Contreras, 2012-10-17 18:26 Subject: Re: [PATCH v5 2/3] tests: use __gitcompadd to simplify completion tests Message-ID: URL: https://gitlist.dev/e/CAMP44s1L_0FS4jrdb4aFxgPENrO1ySMXhCqJt3QiODhbuTPFQA%40mail.gmail.com In-Reply-To: <20121017175040.GE2156@goldbirke> ``` On Wed, Oct 17, 2012 at 7:50 PM, SZEDER Gábor wrote: > On Sun, Oct 14, 2012 at 05:52:50PM +0200, Felipe Contreras wrote: >> Signed-off-by: Felipe Contreras >> --- >> t/t9902-completion.sh | 29 +++++++++-------------------- >> 1 file changed, 9 insertions(+), 20 deletions(-) >> >> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >> index 92d7eb4..49c6eb4 100755 >> --- a/t/t9902-completion.sh >> +++ b/t/t9902-completion.sh >> @@ -39,19 +39,18 @@ _get_comp_words_by_ref () >> done >> } >> >> -print_comp () >> +__gitcompadd () >> { >> - local IFS=$'\n' >> - echo "${COMPREPLY[*]}" > out >> + compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}" > out >> } > > Please don't. Running compgen is a fundamental part of the completion > script, therefore tests must run it as it is in the completion script > and not some copy of it. All right. I added this patch as an after though to help sell the idea of __gitcompadd. Either way I'm not to worried about overriding it, we are not really exercising any code that could catch issues with calling compgen; we probably need specialized tests for that. In fact I amended the quote you are quoting above as it's totally different from the proposed __gitcompadd, but it still works nonetheless. -- Felipe Contreras ``` ## Felipe Contreras, 2012-10-22 00:41 Subject: Re: [PATCH v5 1/3] completion: add new __gitcompadd helper Message-ID: URL: https://gitlist.dev/e/CAMP44s3guAhFe1ACrAzC9T05fCMAejU6Wn%2BGtYfSpmfJ8EX-xg%40mail.gmail.com In-Reply-To: <20121017172808.GD2156@goldbirke> ``` On Wed, Oct 17, 2012 at 7:28 PM, SZEDER Gábor wrote: > On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote: >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash >> index d743e56..01325de 100644 >> --- a/contrib/completion/git-completion.bash >> +++ b/contrib/completion/git-completion.bash >> @@ -225,6 +225,11 @@ _get_comp_words_by_ref () >> fi >> fi >> >> +__gitcompadd () >> +{ >> + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) >> +} >> + >> # Generates completion reply with compgen, appending a space to possible >> # completion words, if necessary. >> # It accepts 1 to 4 arguments: >> @@ -238,13 +243,11 @@ __gitcomp () >> >> case "$cur_" in >> --*=) >> - COMPREPLY=() >> + __gitcompadd >> ;; >> *) >> local IFS=$'\n' >> - COMPREPLY=($(compgen -P "${2-}" \ >> - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ >> - -- "$cur_")) >> + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" "" >> ;; >> esac >> } >> @@ -261,7 +264,7 @@ __gitcomp () >> __gitcomp_nl () >> { >> local IFS=$'\n' >> - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) >> + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }" >> } > > I feel hesitant about this change. One of the ways I'm exploring to > fix the issues with shell metacharacters and expansion in compgen is > to actually replace compgen. We already iterate over all possible > completion words in __gitcomp_1(), so it doesn't make much of a > difference to do the filtering for the current word while we are at > it. However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen > ...)), and tha basic idea of never touching COMPREPLY directly make > this basically impossible. How is it impossible? You can still replace compgen, all you have to do is modify __gitcompadd and replace that code with whatever custom code you want. You can change the arguments and everything. The only limitation is that it should be the only place where COMPREPLY is modified, and all is good. Well, it doesn't have to be only _one_ place, but the less functions that do this, the better. >> __git_heads () >> @@ -486,7 +489,7 @@ __git_complete_remote_or_refspec () >> case "$cmd" in >> push) no_complete_refspec=1 ;; >> fetch) >> - COMPREPLY=() >> + __gitcompadd >> return >> ;; >> *) ;; >> @@ -502,7 +505,7 @@ __git_complete_remote_or_refspec () >> return >> fi >> if [ $no_complete_refspec = 1 ]; then >> - COMPREPLY=() >> + __gitcompadd >> return >> fi >> [ "$remote" = "." ] && remote= >> @@ -776,7 +779,7 @@ _git_am () >> " >> return >> esac >> - COMPREPLY=() >> + __gitcompadd > > These changes effectively run compgen in a subshell to generate an > empty completion reply. While it doesn't really matter on Linux, > it'll add another half a tenth of a second delay in those cases on my > Windows machine. At least it should be conditional, i.e. $(compgen > ...) shouldn't be executed when there are no possible completion > words. > > However, I think those COMPREPLY=() assignments are pointless anyway. > COMPREPLY is always empty when completion functions are invoked, so > there is no need to explicitly set it to an empty array when we don't > provide any words for completion. Their only use is basically to > explicitly tell us humans that in those cases we don't offer any words > for completion. But we don't do that consistently: there are several > places without offering words for completion and without COMPREPLY=(), > e.g. the '__git_has_doubledash && return' pattern. > > Perhaps it would be time to get rid of these COMPREPLY=() assignments? I'm all for it, I never understood what was the purpose of that. I believe zsh could benefit from this information to decide whether to run the default completion (e.g. files) or not, but as you said, if it's not used consistently for bash, there's no point in trying. Cheers. -- Felipe Contreras ``` ## SZEDER Gábor, 2012-10-30 23:18 Subject: Re: [PATCH v5 1/3] completion: add new __gitcompadd helper Message-ID: <20121030231832.GB15709@goldbirke> URL: https://gitlist.dev/e/20121030231832.GB15709%40goldbirke In-Reply-To: ``` On Mon, Oct 22, 2012 at 02:41:21AM +0200, Felipe Contreras wrote: > On Wed, Oct 17, 2012 at 7:28 PM, SZEDER Gábor wrote: > > On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote: > > >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > >> index d743e56..01325de 100644 > >> --- a/contrib/completion/git-completion.bash > >> +++ b/contrib/completion/git-completion.bash > >> @@ -225,6 +225,11 @@ _get_comp_words_by_ref () > >> fi > >> fi > >> > >> +__gitcompadd () > >> +{ > >> + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) > >> +} > >> + > >> # Generates completion reply with compgen, appending a space to possible > >> # completion words, if necessary. > >> # It accepts 1 to 4 arguments: > >> @@ -238,13 +243,11 @@ __gitcomp () > >> > >> case "$cur_" in > >> --*=) > >> - COMPREPLY=() > >> + __gitcompadd > >> ;; > >> *) > >> local IFS=$'\n' > >> - COMPREPLY=($(compgen -P "${2-}" \ > >> - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ > >> - -- "$cur_")) > >> + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" "" > >> ;; > >> esac > >> } > >> @@ -261,7 +264,7 @@ __gitcomp () > >> __gitcomp_nl () > >> { > >> local IFS=$'\n' > >> - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > >> + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }" > >> } > > > > I feel hesitant about this change. One of the ways I'm exploring to > > fix the issues with shell metacharacters and expansion in compgen is > > to actually replace compgen. We already iterate over all possible > > completion words in __gitcomp_1(), so it doesn't make much of a > > difference to do the filtering for the current word while we are at > > it. However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen > > ...)), and tha basic idea of never touching COMPREPLY directly make > > this basically impossible. > > How is it impossible? You can still replace compgen, all you have to > do is modify __gitcompadd and replace that code with whatever custom > code you want. You can change the arguments and everything. The only > limitation is that it should be the only place where COMPREPLY is > modified, and all is good. Well, it doesn't have to be only _one_ > place, but the less functions that do this, the better. That's exactly the problem: there isn't, there can't be one single "whatever custom code I want". The compgen() in __gitcomp() will be replaced by an enhanced version of the loop in __gitcomp_1(), while in __gitcomp_nl() it will be replaced by a little awk scriptlet. And then there is the oddball $(git ls-tree |sed magic) in __git_complete_revlist_file(), where possible completion words are filenames possibly containing newlines, therefore requiring yet another approach. ```