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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 7, 2012, 00:20 UTC
Message-ID: <CAMP44s3q-X7W5qeu6LZzu13C6SfhEZy9i4AfJ9Sszcou52MsGw@mail.gmail.com>
In-Reply-To: <20120506233235.GN2164@goldbirke>

Hi,

On Mon, May 7, 2012 at 1:32 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Show 25 quoted lines

> 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.

Sure.

Show 5 quoted lines

>> 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

Perhaps not, but why are you arguing for making users' life more difficult? Even if it's just a little.

In fact, it could be even simpler:

_GIT_complete gf fetch

Show 9 quoted lines

>> > 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?

It does work... on my branch.

If you have another example that works, feel free to suggest it, but I'm going to remove that message, along with _GIT_complete (and replace it with __git_complete and a note that it's not public API), because I'm tired of trying to make users' life easier; I just want the patch in.

Show 6 quoted lines

>> 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.

${words[1]} is part of $words. And BTW, git bundle also fails similarly.

And then, even if you fetch the command properly, __git_complete_remote_or_refspec will still fail because it would assume the remote is 'git'.

Also BTW, git fetch is already broken anyway:

 git --no-pager fetch <TAB>

So don't blame my patch :)

Show 14 quoted lines

>> > 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.

That's not enough to make 'git fetch' work, try it.

Show 12 quoted lines

>> >> +     __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.

Yeah, but that's still what bash does, and I saw no reason to change it.

Show 5 quoted lines

>> 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.

Sure, but it was just 4 more characters that didn't hurt anybody. Your version makes passing those arguments more difficult, so I see no need to try to implement that.

Cheers.

-- 
Felipe Contreras