Re: [PATCH 5/5] completion: fix completion of certain aliases

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2014, 20:20 UTC
Message-ID: <xmqqmwfnoe7m.fsf@gitster.dls.corp.google.com>
In-Reply-To: <E1WZEWT-0002R7-1d@iramx2.ira.uni-karlsruhe.de>

Gábor Szeder <szeder@ira.uka.de> writes:

> words[] is just fine, we never modify it after it is filled by
> _get_comp_words_by_ref() at the very beginning.

Hmph. I would have understood if the latter were "we never look at it (to decide what to do)". "we never modify it" does not sound like an enough justification behind "words[] is just fine"---maybe I am not reading you correctly.

Show 21 quoted lines

> The root of the problem is that the expected position of the name
> of the git command in __git_complete_remote_or_refspec() is
> hardcoded as words[1], but that is not the case when:
>
>   1) it's an alias, as in Felipe's example: git p ori<TAB>,
>   because while the index is ok, the content is not.
>
>   2) in presence of options of the main git command: git -c
>   foo=bar push ori<TAB>, because the index is off.
>
>   3) the command is a shell alias for which the user explicitly
>   set the completion function with __git_complete() (at his own
>   risk): alias gp="git push"; __git_complete gp _git_push; gp
>   ori<TAB> Neither the index nor the content are ok.
>
> Fixing the hard-coded indexing would only solve 2) but not 1) and
> 3), as it obviously couldn't turn the git or shell alias into a
> git command on its own.
>
> Felipe's patch only deals with 1), as it only kicks in in case of
> a git alias.

Yeah, do completions for commands (not just for the ones that use remote-or-refspec Felipe's patch addresses) have trouble with the latter two in general? If that is the case,...

Show 5 quoted lines

> Communicating the name of the git command to
> __git_complete_remote_or_refspec() by its callers via a new
> variable as suggested by Junio, or perhaps by an additional
> parameter to the function is IMHO the right thing to do, because,
> unless I'm missing something, it would make all three cases work.

... while the above analysis may be correct, taking Felipe's patch to address only (1) and leaving a solution to the more general words[1] problem for other patches on top might not be too bad an approach.

Unless

 (A) remote-or-refspec thing is the primary offender, and other
     commands do not suffer from the words[1] problem, in which case
     I tend to agree that an additional parameter would be the way
     to go (there are only a few callers of the function); or

 (B) even if words[1] problem is more widespread, such a more
     general solution to all three issues can be coded cleanly and
     quickly, without having to have Felipe's patch as a stop-gap
     measure.

that is.

I'll keep Felipe's patch in the meantime to 'pu'.

Thanks.