Re: [PATCH 5/5] completion: fix completion of certain aliases
Junio C Hamano wrote: Gábor Szeder sze...@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. 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 oriTAB, 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 oriTAB, 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 oriTAB 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. Which is the far more common use-case, and the problem I've seen people report multiple times in multiple media. 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,... 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 Since I already fixed the problem (all 3) years ago[1], you can take a look at the patch and see which commands which use a hard-coded index value might have real issues. My guess is that it's more than just remote-or-refspec, but I don't have the incentive to look deeper into this (the issue is solved for me). (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. I already sent two patches, one that solves all the problems, and one that solves the most important one. I would gladly revisit my old patch and see which of those changes are really necessary and solve a real issue, *if* I knew the resulting patch wouldn't get the same road-blockers as the old one did; namely being held to higher standards than the current code. I say it's more important to fix the real issues real people have than hold on to arbitrary standards which might force the bug to remain present for years just because some variable was not documented in the original patch (just like all other variables in the script)... But that's just me. [1] http://thread.gmane.org/gmane.comp.version-control.git/197226 -- Felipe Contreras-- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] completion: fix completion of certain aliases
Gábor Szeder sze...@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. 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 oriTAB, 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 oriTAB, 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 oriTAB 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,... 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 gits...@pobox.com wrote:
Felipe Contreras felipe.contre...@gmail.com writes:
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 aymeric.beau...@gmail.com
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
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 oriTAB, 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
oriTAB, 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 oriTAB 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].
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
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
[PATCH 5/5] completion: fix completion of certain aliases
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 aymeric.beau...@gmail.com
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
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
--
1.9.1+fc1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] completion: fix completion of certain aliases
Felipe Contreras felipe.contre...@gmail.com writes:
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 aymeric.beau...@gmail.com
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
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...).
Thanks, will queue.
[Ram and Szeder CC'ed as they appear in shortlog for the past 12
months].
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] completion: fix completion of certain aliases
Junio C Hamano wrote: Felipe Contreras felipe.contre...@gmail.com writes: 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 aymeric.beau...@gmail.com Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- Does some commands above refer to anything that uses __git_complete_remote_or_refspec, or is the set of commands larger than that? For this particular issue, yes, the former. 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...). The 'words' array is already messed up and not used correctly, so I wouldn't worry too much about this patch messing it more (I don't see how that can be). For example: % git --git-dir=$PWD/.git fetch ortab -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
