From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:01
Hi,
Here's a bit of reorganition. I'm introducing a new __gitcompadd helper that is
useful to wrapp all changes to COMPREPLY. 2nd and 3rd patches show how it's
useful.
The zsh wrapper is now very very simple, but I haven't received much feedback
yet. I hope it will get in at some point in time.
Felipe Contreras (3):
completion: add new __gitcompadd helper
tests: use __gitcompadd to simplify completion tests
completion: add new zsh completion
contrib/completion/git-completion.bash | 65 ++++++++++++++++++----------------
contrib/completion/git-completion.zsh | 48 +++++++++++++++++++++++++
t/t9902-completion.sh | 29 +++++----------
3 files changed, 91 insertions(+), 51 deletions(-)
create mode 100644 contrib/completion/git-completion.zsh
--
1.7.12.1
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:01
It seems there's always issues with zsh's bash completion emulation.
I've tried to fix as many as I could and most of the fixes are already
in the latest version of zsh, but still, there are issues.
There is no point in going through all that pain; the emulation is easy
to achieve, and this patch works better than zsh's emulation.
Signed-off-by: Felipe Contreras <redacted>
---
v5:
* Even more simplification by using __gitcompadd
v4:
* Simplification updates for the latest bash completion
v3:
* Simplification
* Avoid COMPREPLY; call compadd directly
* Fix _get_comp_words_by_ref
contrib/completion/git-completion.zsh | 48 +++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
create mode 100644 contrib/completion/git-completion.zsh
@@ -0,0 +1,48 @@+#compdef git gitk++# zsh completion wrapper for git+#+# You need git's bash completion script installed somewhere, by default on the+# same directory as this script.+#+# If your script is on ~/.git-completion.sh instead, you can configure it on+# your ~/.zshrc:+#+# zstyle ':completion:*:*:git:*' script ~/.git-completion.sh+#+# The recommended way to install this script is to copy to+# '~/.zsh/completion/_git', and then add the following to your ~/.zshrc file:+#+# fpath=(~/.zsh/completion $fpath)++complete()+{+# do nothing+return0+}++zstyle-s":completion:*:*:git:*"scriptscript+test-z"$script"&&script="$(dirname${funcsourcetrace[1]%:*})"/git-completion.bash+ZSH_VERSION=''."$script"++__gitcompadd()+{+compadd-Q-S"$4"-P"$2"-p"${(M)cur#*[=:]}"--${=1}&&_ret=0+}++_git()+{+local_ret=1+(){+emulate-Lksh+localcurcwordprev+cur=${words[CURRENT-1]}+prev=${words[CURRENT-2]}+letcword=CURRENT-1+__${service}_main+}+let_ret&&_default-S''&&_ret=0+return_ret+}++_git
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:01
The idea is to never touch the COMPREPLY variable directly.
This allows other completion systems override __gitcompadd, and do
something different instead.
Also, this allows the simplifcation of the completino tests (separate
patch).
There should be no functional changes.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/completion/git-completion.bash | 65 ++++++++++++++++++----------------
1 file changed, 34 insertions(+), 31 deletions(-)
@@ -225,6 +225,11 @@ _get_comp_words_by_ref ()fifi+__gitcompadd()+{+COMPREPLY=($(compgen-W"$1"-P"$2"-S"$4"--"$3"))+}+# Generates completion reply with compgen, appending a space to possible# completion words, if necessary.# It accepts 1 to 4 arguments:
From: SZEDER Gábor <hidden> Date: 2016-06-15 22:55:03
On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote:
quoted hunk
The idea is to never touch the COMPREPLY variable directly.
This allows other completion systems override __gitcompadd, and do
something different instead.
Also, this allows the simplifcation of the completino tests (separate
patch).
There should be no functional changes.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/completion/git-completion.bash | 65 ++++++++++++++++++----------------
1 file changed, 34 insertions(+), 31 deletions(-)
@@ -225,6 +225,11 @@ _get_comp_words_by_ref ()fifi+__gitcompadd()+{+COMPREPLY=($(compgen-W"$1"-P"$2"-S"$4"--"$3"))+}+# Generates completion reply with compgen, appending a space to possible# completion words, if necessary.# It accepts 1 to 4 arguments:
I feel hesitant about this change. One of the ways I'm exploring to
fix the issues with shell metacharacters and expansion in compgen is
to actually replace compgen. We already iterate over all possible
completion words in __gitcomp_1(), so it doesn't make much of a
difference to do the filtering for the current word while we are at
it. However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
...)), and tha basic idea of never touching COMPREPLY directly make
this basically impossible.
quoted hunk
__git_heads ()
@@ -486,7 +489,7 @@ __git_complete_remote_or_refspec () case "$cmd" in push) no_complete_refspec=1 ;; fetch)- COMPREPLY=()+ __gitcompadd return ;; *) ;;
@@ -502,7 +505,7 @@ __git_complete_remote_or_refspec () return fi if [ $no_complete_refspec = 1 ]; then- COMPREPLY=()+ __gitcompadd return fi [ "$remote" = "." ] && remote=
These changes effectively run compgen in a subshell to generate an
empty completion reply. While it doesn't really matter on Linux,
it'll add another half a tenth of a second delay in those cases on my
Windows machine. At least it should be conditional, i.e. $(compgen
...) shouldn't be executed when there are no possible completion
words.
However, I think those COMPREPLY=() assignments are pointless anyway.
COMPREPLY is always empty when completion functions are invoked, so
there is no need to explicitly set it to an empty array when we don't
provide any words for completion. Their only use is basically to
explicitly tell us humans that in those cases we don't offer any words
for completion. But we don't do that consistently: there are several
places without offering words for completion and without COMPREPLY=(),
e.g. the '__git_has_doubledash && return' pattern.
Perhaps it would be time to get rid of these COMPREPLY=() assignments?
Please don't. Running compgen is a fundamental part of the completion
script, therefore tests must run it as it is in the completion script
and not some copy of it.
quoted hunk
run_completion ()
{
- local -a COMPREPLY _words
+ local -a _words
local _cword
_words=( $1 )
(( _cword = ${#_words[@]} - 1 ))
- __git_wrap__git_main && print_comp
+ __git_wrap__git_main
}
test_completion ()
@@ -70,12 +69,10 @@ test_expect_success '__gitcomp - trailing space - options' ' --reset-author Z EOF (- local -a COMPREPLY &&
I'm not sure what I was thinking when I wrote this, but using the
local keyword while not within a function but in a subshell doesn't
seem to be that clever ;) Maybe just a copy-paste from the local
variable declarations of run-completion().
And here I should have used print_comp().
All these can be cleaned up without overriding __gitcompadd() and
potentialy compromising correctness. Will send a patch in a minute.
From: SZEDER Gábor <hidden> Date: 2016-06-15 22:55:03
Clean up two issues in the tests I added in 74a8c849 (tests: add tests
for the __gitcomp() completion helper function, 2012-04-17):
- The COMPREPLY array is created using 'local -a' while in a
subshell. However, the local keyword should only be used in a
shell function, and a variable created in a subshell is by
definition local to that subshell. Use 'declare -a' instead.
- The contents of the COMPREPLY array is written through an IFS
fiddling + echo + redirection combo, although there is the
print_comp() helper function for exactly this purpose.
Signed-off-by: SZEDER Gábor <redacted>
---
t/t9902-completion.sh | 27 ++++++++++-----------------
1 file changed, 10 insertions(+), 17 deletions(-)
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:03
On Wed, Oct 17, 2012 at 7:54 PM, SZEDER Gábor [off-list ref] wrote:
Clean up two issues in the tests I added in 74a8c849 (tests: add tests
for the __gitcomp() completion helper function, 2012-04-17):
- The COMPREPLY array is created using 'local -a' while in a
subshell. However, the local keyword should only be used in a
shell function, and a variable created in a subshell is by
definition local to that subshell. Use 'declare -a' instead.
- The contents of the COMPREPLY array is written through an IFS
fiddling + echo + redirection combo, although there is the
print_comp() helper function for exactly this purpose.
Makes sense. But this code seems awfully similar, a helper function might help.
--
Felipe Contreras
Please don't. Running compgen is a fundamental part of the completion
script, therefore tests must run it as it is in the completion script
and not some copy of it.
All right. I added this patch as an after though to help sell the idea
of __gitcompadd. Either way I'm not to worried about overriding it,
we are not really exercising any code that could catch issues with
calling compgen; we probably need specialized tests for that. In fact
I amended the quote you are quoting above as it's totally different
from the proposed __gitcompadd, but it still works nonetheless.
--
Felipe Contreras
@@ -225,6 +225,11 @@ _get_comp_words_by_ref ()fifi+__gitcompadd()+{+COMPREPLY=($(compgen-W"$1"-P"$2"-S"$4"--"$3"))+}+# Generates completion reply with compgen, appending a space to possible# completion words, if necessary.# It accepts 1 to 4 arguments:
I feel hesitant about this change. One of the ways I'm exploring to
fix the issues with shell metacharacters and expansion in compgen is
to actually replace compgen. We already iterate over all possible
completion words in __gitcomp_1(), so it doesn't make much of a
difference to do the filtering for the current word while we are at
it. However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
...)), and tha basic idea of never touching COMPREPLY directly make
this basically impossible.
How is it impossible? You can still replace compgen, all you have to
do is modify __gitcompadd and replace that code with whatever custom
code you want. You can change the arguments and everything. The only
limitation is that it should be the only place where COMPREPLY is
modified, and all is good. Well, it doesn't have to be only _one_
place, but the less functions that do this, the better.
quoted
__git_heads ()
@@ -486,7 +489,7 @@ __git_complete_remote_or_refspec () case "$cmd" in push) no_complete_refspec=1 ;; fetch)- COMPREPLY=()+ __gitcompadd return ;; *) ;;
@@ -502,7 +505,7 @@ __git_complete_remote_or_refspec () return fi if [ $no_complete_refspec = 1 ]; then- COMPREPLY=()+ __gitcompadd return fi [ "$remote" = "." ] && remote=
These changes effectively run compgen in a subshell to generate an
empty completion reply. While it doesn't really matter on Linux,
it'll add another half a tenth of a second delay in those cases on my
Windows machine. At least it should be conditional, i.e. $(compgen
...) shouldn't be executed when there are no possible completion
words.
However, I think those COMPREPLY=() assignments are pointless anyway.
COMPREPLY is always empty when completion functions are invoked, so
there is no need to explicitly set it to an empty array when we don't
provide any words for completion. Their only use is basically to
explicitly tell us humans that in those cases we don't offer any words
for completion. But we don't do that consistently: there are several
places without offering words for completion and without COMPREPLY=(),
e.g. the '__git_has_doubledash && return' pattern.
Perhaps it would be time to get rid of these COMPREPLY=() assignments?
I'm all for it, I never understood what was the purpose of that. I
believe zsh could benefit from this information to decide whether to
run the default completion (e.g. files) or not, but as you said, if
it's not used consistently for bash, there's no point in trying.
Cheers.
--
Felipe Contreras
@@ -225,6 +225,11 @@ _get_comp_words_by_ref ()fifi+__gitcompadd()+{+COMPREPLY=($(compgen-W"$1"-P"$2"-S"$4"--"$3"))+}+# Generates completion reply with compgen, appending a space to possible# completion words, if necessary.# It accepts 1 to 4 arguments:
I feel hesitant about this change. One of the ways I'm exploring to
fix the issues with shell metacharacters and expansion in compgen is
to actually replace compgen. We already iterate over all possible
completion words in __gitcomp_1(), so it doesn't make much of a
difference to do the filtering for the current word while we are at
it. However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
...)), and tha basic idea of never touching COMPREPLY directly make
this basically impossible.
How is it impossible? You can still replace compgen, all you have to
do is modify __gitcompadd and replace that code with whatever custom
code you want. You can change the arguments and everything. The only
limitation is that it should be the only place where COMPREPLY is
modified, and all is good. Well, it doesn't have to be only _one_
place, but the less functions that do this, the better.
That's exactly the problem: there isn't, there can't be one single
"whatever custom code I want".
The compgen() in __gitcomp() will be replaced by an enhanced version
of the loop in __gitcomp_1(), while in __gitcomp_nl() it will be
replaced by a little awk scriptlet. And then there is the oddball
$(git ls-tree |sed magic) in __git_complete_revlist_file(), where
possible completion words are filenames possibly containing newlines,
therefore requiring yet another approach.