Re: [PATCH v5] completion: add new __git_complete helper

On Mon, May 14, 2012 at 7:43 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

This simplifies the completions, and would make it easier to define
aliases in the future.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/completion/git-completion.bash |   70 +++++++++++++++-----------------
 t/t9902-completion.sh                  |    2 +-
 2 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9f56ec7..d60bb8a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -2603,21 +2603,6 @@ _git ()

 {
      local i c=1 command __git_dir

-     if [[ -n ${ZSH_VERSION-} ]]; then
-             emulate -L bash
-             setopt KSH_TYPESET
-
-             # workaround zsh's bug that leaves 'words' as a special
-             # variable in versions < 4.3.12
-             typeset -h words
-
-             # workaround zsh's bug that quotes spaces in the COMPREPLY
-             # array if IFS doesn't contain spaces.
-             typeset -h IFS
-     fi
-
-     local cur words cword prev
-     _get_comp_words_by_ref -n =: cur words cword prev
      while [ $c -lt $cword ]; do
              i="${words[c]}"
              case "$i" in

@@ -2667,22 +2652,6 @@ _git ()

 _gitk ()
 {
-     if [[ -n ${ZSH_VERSION-} ]]; then
-             emulate -L bash
-             setopt KSH_TYPESET
-
-             # workaround zsh's bug that leaves 'words' as a special
-             # variable in versions < 4.3.12
-             typeset -h words
-
-             # workaround zsh's bug that quotes spaces in the COMPREPLY
-             # array if IFS doesn't contain spaces.
-             typeset -h IFS
-     fi
-
-     local cur words cword prev
-     _get_comp_words_by_ref -n =: cur words cword prev
-

@@ -2703,16 +2672,43 @@ _gitk ()

      __git_complete_revlist
 }

-complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \
-     || complete -o default -o nospace -F _git git
-complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \
-     || complete -o default -o nospace -F _gitk gitk

Nice code reduction by moving the duplicated code into the new helpers ;-)

quoted

+__git_func_wrap ()
+{
+     if [[ -n ${ZSH_VERSION-} ]]; then
+             emulate -L bash
+             setopt KSH_TYPESET
+
+             # workaround zsh's bug that leaves 'words' as a special
+             # variable in versions < 4.3.12
+             typeset -h words
+
+             # workaround zsh's bug that quotes spaces in the COMPREPLY
+             # array if IFS doesn't contain spaces.
+             typeset -h IFS
+     fi
+     local cur words cword prev
+     _get_comp_words_by_ref -n =: cur words cword prev
+     $1
+}
+
+# Setup completion for certain functions defined above by setting common
+# variables and workarounds.
+# This is NOT a public function; use at your own risk.
+__git_complete ()
+{
+     local wrapper="__git_wrap${2}"
+     eval "$wrapper () { __git_func_wrap $2 ; }"
+     complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \
+             || complete -o default -o nospace -F $wrapper $1
+}
+
+__git_complete git _git
+__git_complete gitk _gitk

It makes my stomach queasy whenever I see $var not in double quotes that
forces me to guess (and trace to verify if the codepath is what I really
care about) if any value with $IFS in it could be used there, so even when
they are known to be safe, I'd prefer to see either explicit quotes or
comment that says what are expected in $1 and $2.

Which ones? These?

quoted

+     $1
+     complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \
+             || complete -o default -o nospace -F $wrapper $1

So you want:

  "$1"

  complete -o bashdefault -o default -o nospace -F "$wrapper" "$1" 2>/dev/null \
    || complete -o default -o nospace -F "$wrapper" "$1"

I wouldn't object to the arguments of complete, but the function call
seems totally unintuitive to me like that.

-- 
Felipe Contreras