Re: [PATCH v5 1/3] completion: add new __gitcompadd helper

["SZEDER Gábor" <szeder@ira.uka.de>]

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 <szeder@ira.uka.de> 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.