Re: [PATCH v3] completion: add new _GIT_complete helper

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

Hi,


On Sun, May 06, 2012 at 10:37:06PM +0200, Felipe Contreras wrote:
> On Sun, May 6, 2012 at 1:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:
> >> This simplifies the completions, and makes it easier to define aliases:
> >>
> >>  _GIT_complete gf git_fetch
> >
> > So, 'gf' is an alias for 'git fetch', for which the user would like to
> > use the completion for 'git fetch', right?  But that completion
> > function is caled _git_fetch(), so the underscore prefix is missing
> > here.
> 
> No, it's not missing:
> 
>        local name="${2-$1}"
>        eval "$(typeset -f __git_func_wrap | sed -e "s/__git_func/_$name/")"
>        complete -o bashdefault -o default -o nospace -F _${name}_wrap
> $1 2>/dev/null \
>                || complete -o default -o nospace -F _${name}_wrap $1
> 
> See how we add '_' before $name?

Indeed, the '_' is added before $name no less than three times.  How
could I have missed that?! ;)  It would be better to do it once and be
done with it.

> There's not point in burdening the
> user with adding a prefix that will _always_ be there anyway.

I don't think it's that much of a burden.  The function is called
_git_fetch, use that as second argument

> > Besides, this example won't work, because the completion for 'git
> > fetch' uses __git_complete_remote_or_refspec(), which in turn relies
> > on finding out the name of the git command from the word on the
> > command line, and it won't be able to do that from 'gf'.
> 
> That's irrelevant, it's an example; 

It's relevant; if you give an example, then at least that example
should work properly, don't you think?

> replace with another command that
> doesn't use 'words', and it would work.

That it doesn't work has nothing to do with $words.  The problem is that
__git_complete_remote_or_refspec() expects to find the git command in
${words[1]}, but in case of an alias it can't.

> > I remember we discussed this in an earlier round, and you even
> > suggested a possible fix (passing the command name as argument to
> > __git_complete_remote_or_refspec()).  I think that's the right thing
> > to do here.
> 
> Yeah, but I suggested that in order to avoid the eval and the typeset
> that I require for future future patches, but it turns out it's still
> needed anyway, so my suggestion is to have a 'cmd' variable that
> stores the command; __git_func_wrap would take the responsibility of
> doing that.

Well, now I suggest to do that to fix
__git_complete_remote_or_refspec(), because that seems to be the
easiest, cleanest, and fastest solution.

> >> +     __git_func "$@"
> >
> > What is this "$@" for and why?  None of the _git_<cmd>() functions
> > take any arguments, nor does _git() and _gitk(), and AFAICT Bash won't
> > pass any either.
> 
> bash's complete passes 3 arguments.

Oh, indeed; the first argument is the command name, the second is the
current word, and the third is the previous word.  All these are
available in our completion functions as ${words[0]}, $cur, and $prev,
respectively.

> They might not be used, but it
> doesn't hurt to pass them either.

They _are_ not used, so passing them has no benefit either.  I would
rather stick to using $cur and $prev than $2 and $3.


Best,
Gábor