Re: [PATCH v3 3/7] completion: add new __gitcompadd helper
Felipe Contreras writes: > On Fri, Apr 12, 2013 at 12:55 PM, Junio C Hamano wrote: >> Felipe Contreras writes: > >>> } >>> >>> # Generates completion reply with compgen from newline-separated possible >>> @@ -1820,7 +1823,7 @@ _git_config () >>> local remote="${prev#remote.}" >>> remote="${remote%.fetch}" >>> if [ -z "$cur" ]; then >>> - COMPREPLY=("refs/heads/") >>> + __gitcompadd "refs/heads/" >> >> I am not sure about this one, though. >> >> Other callers took pains to protet against triggering unset variable >> references by using ${1-} instead of ${1}. Shouldn't this caller be >> passing three empty strings? > > Perhaps, or perhaps we were being too careful before: 'compgen -W foo' > is the same as 'compgen -W foo -S "" -P "" -- ""'. Yes, they are the same (otherwise this patch would not be valid), but that is not what i was wondering about. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/7] completion: add new __gitcompadd helper
On Fri, Apr 12, 2013 at 12:55 PM, Junio C Hamano wrote: > Felipe Contreras writes: >> } >> >> # Generates completion reply with compgen from newline-separated possible >> @@ -1820,7 +1823,7 @@ _git_config () >> local remote="${prev#remote.}" >> remote="${remote%.fetch}" >> if [ -z "$cur" ]; then >> - COMPREPLY=("refs/heads/") >> + __gitcompadd "refs/heads/" > > I am not sure about this one, though. > > Other callers took pains to protet against triggering unset variable > references by using ${1-} instead of ${1}. Shouldn't this caller be > passing three empty strings? Perhaps, or perhaps we were being too careful before: 'compgen -W foo' is the same as 'compgen -W foo -S "" -P "" -- ""'. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/7] completion: add new __gitcompadd helper
Felipe Contreras writes: > The idea is to never touch the COMPREPLY variable directly. > > This allows other completion systems (i.e. zsh) to override > __gitcompadd, and do something different instead. > > Also, this allows further optimizations down the line. > > There should be no functional changes. > > Signed-off-by: Felipe Contreras > --- > contrib/completion/git-completion.bash | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/contrib/completion/git-completion.bash > b/contrib/completion/git-completion.bash > index 2c87fd8..90b54ab 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -195,6 +195,11 @@ _get_comp_words_by_ref () > } > fi > > +__gitcompadd () > +{ > + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) Making a mental note that this takes prefix ($2), suffix ($4) and the actual word ($3) are given in addition to the list of expansions ($1)... > +} > + > # Generates completion reply with compgen, appending a space to possible > # completion words, if necessary. > # It accepts 1 to 4 arguments: > @@ -211,9 +216,7 @@ __gitcomp () > ;; > *) > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" \ > - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ > - -- "$cur_")) > + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" "" This did not use to use suffix, but we pass an empty string as $4, so it is an equivalent rewrite. > ;; > esac > } > @@ -230,7 +233,7 @@ __gitcomp () > __gitcomp_nl () > { > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }" This also is a straight-forward rewrite. > } > > # Generates completion reply with compgen from newline-separated possible > @@ -1820,7 +1823,7 @@ _git_config () > local remote="${prev#remote.}" > remote="${remote%.fetch}" > if [ -z "$cur" ]; then > - COMPREPLY=("refs/heads/") > + __gitcompadd "refs/heads/" I am not sure about this one, though. Other callers took pains to protet against triggering unset variable references by using ${1-} instead of ${1}. Shouldn't this caller be passing three empty strings? > return > fi > __gitcomp_nl "$(__git_refs_remotes "$remote")" -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/7] completion: add new __gitcompadd helper
The idea is to never touch the COMPREPLY variable directly. This allows other completion systems (i.e. zsh) to override __gitcompadd, and do something different instead. Also, this allows further optimizations down the line. There should be no functional changes. Signed-off-by: Felipe Contreras --- contrib/completion/git-completion.bash | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 2c87fd8..90b54ab 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -195,6 +195,11 @@ _get_comp_words_by_ref () } 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: @@ -211,9 +216,7 @@ __gitcomp () ;; *) local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" \ - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ - -- "$cur_")) + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" "" ;; esac } @@ -230,7 +233,7 @@ __gitcomp () __gitcomp_nl () { local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }" } # Generates completion reply with compgen from newline-separated possible @@ -1820,7 +1823,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")" -- 1.8.2.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html