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

Hi,

[I'm travelling, so I don't have the means to actually try out this patch, and it might take a while a to reply to any follow ups.]

On Apr 10, 2014 12:03 AM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes: 

quoted

Some commands need the first word to determine the actual action that is 
being executed, however, the command is wrong when we use an alias, for 
example 'alias.p=push', if we try to complete 'git p origin ', the 
result would be wrong because __git_complete_remote_or_refspec() doesn't 
know where it come from. 

So let's override words[1], so the alias 'p' is override by the actual 
command, 'push'. 

Reported-by: Aymeric Beaumet <redacted> 
Signed-off-by: Felipe Contreras <redacted> 
--- 

Does "some commands" above refer to anything that uses 
__git_complete_remote_or_refspec, or is the set of commands larger 
than that? 

I am wondering if it is safer to introduce a new "local" variable 
that is set by the caller of __git_complete_remote_or_refspec and 
inspected by __git_complete_remote_or_refspec (instead of words[1]) 
to communiate the real name of the git subcommand being completed, 
without touching words[] in place. 

That way, we wouldn't have to worry about all the other references 
of words[c], words[i], words[CURRENT] etc. in the script seeing the 
word that the end-user did not type and did not actually appear on 
the command line. 

But perhaps we muck with the contents of words[] in a similar way in 
many different places in the existing completion code often enough 
that such an attempt not to touch the words[] array does not buy us 
much safety anyway.  I didn't check (and that is why I am asking 
with "I am wondering...").

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

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.

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.

Best,
Gábor

Thanks, will queue. 

[Ram and Szeder CC'ed as they appear in shortlog for the past 12 
months]. 

quoted

 contrib/completion/git-completion.bash | 1 + 
 contrib/completion/git-completion.zsh  | 1 + 
 2 files changed, 2 insertions(+) 

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash 
index 9525343..893ae5d 100644 
--- a/contrib/completion/git-completion.bash 
+++ b/contrib/completion/git-completion.bash 

@@ -2547,6 +2547,7 @@ __git_main () 
 
 local expansion=$(__git_aliased_command "$command") 
 if [ -n "$expansion" ]; then 
+ words[1]=$expansion 
 completion_func="_git_${expansion//-/_}" 
 declare -f $completion_func >/dev/null && $completion_func 
 fi 

diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh 
index 6b77968..9f6f0fa 100644 
--- a/contrib/completion/git-completion.zsh 
+++ b/contrib/completion/git-completion.zsh 

@@ -104,6 +104,7 @@ __git_zsh_bash_func () 
 
 local expansion=$(__git_aliased_command "$command") 
 if [ -n "$expansion" ]; then 
+ words[1]=$expansion 
 completion_func="_git_${expansion//-/_}" 
 declare -f $completion_func >/dev/null && $completion_func 
 fi