* [PATCH for maint branch] git-completion: fix zsh support
@ 2011-05-05 14:24 Felipe Contreras
2011-05-05 17:34 ` Junio C Hamano
2011-05-05 18:59 ` SZEDER Gábor
0 siblings, 2 replies; 14+ messages in thread
From: Felipe Contreras @ 2011-05-05 14:24 UTC (permalink / raw)
To: git; +Cc: SZEDER Gábor, Felipe Contreras
It turns out 'words' is a special variable used by zsh completion.
There's probably a bug in zsh's bashcompinit:
http://article.gmane.org/gmane.comp.shells.zsh.devel/22546
But in the meantime we can workaround it this way.
Currently zsh is completely broken after commit da48616 (bash: get
--pretty=m<tab> completion to work with bash v4), which introduced
_get_comp_words_by_ref() that comes from debian's bash_completion
scripts, and relies on the 'words' variable to behave like any normal
variable.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
contrib/completion/git-completion.bash | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
This patch is meant for the maintenance branch, so Szeder's patches are not
needed.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 840ae38..8d5eae7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2710,6 +2710,9 @@ _git ()
if [[ -n ${ZSH_VERSION-} ]]; then
emulate -L bash
setopt KSH_TYPESET
+
+ # 'words' has special meaning in zsh; override that
+ typeset -h words
fi
local cur words cword
--
1.7.5.1.1.g638e6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH for maint branch] git-completion: fix zsh support
2011-05-05 14:24 [PATCH for maint branch] git-completion: fix zsh support Felipe Contreras
@ 2011-05-05 17:34 ` Junio C Hamano
2011-05-05 19:29 ` Felipe Contreras
2011-05-05 18:59 ` SZEDER Gábor
1 sibling, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2011-05-05 17:34 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor
Felipe Contreras <felipe.contreras@gmail.com> writes:
> It turns out 'words' is a special variable used by zsh completion.
>
> There's probably a bug in zsh's bashcompinit:
> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546
>
> But in the meantime we can workaround it this way.
>
> Currently zsh is completely broken after commit da48616 (bash: get
> --pretty=m<tab> completion to work with bash v4), which introduced
> _get_comp_words_by_ref() that comes from debian's bash_completion
> scripts, and relies on the 'words' variable to behave like any normal
> variable.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> contrib/completion/git-completion.bash | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> This patch is meant for the maintenance branch, so Szeder's patches are not
> needed.
Do you mean sg/completion-upadates that is in 'next' need to be reverted
and replaced with this one? If that is the case I would love to have an
Acked-by in this patch.
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 840ae38..8d5eae7 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2710,6 +2710,9 @@ _git ()
> if [[ -n ${ZSH_VERSION-} ]]; then
> emulate -L bash
> setopt KSH_TYPESET
> +
> + # 'words' has special meaning in zsh; override that
> + typeset -h words
> fi
>
> local cur words cword
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for maint branch] git-completion: fix zsh support
2011-05-05 14:24 [PATCH for maint branch] git-completion: fix zsh support Felipe Contreras
2011-05-05 17:34 ` Junio C Hamano
@ 2011-05-05 18:59 ` SZEDER Gábor
2011-05-05 19:52 ` [PATCH for maint] " Felipe Contreras
1 sibling, 1 reply; 14+ messages in thread
From: SZEDER Gábor @ 2011-05-05 18:59 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Junio C Hamano
Hi,
On Thu, May 05, 2011 at 05:24:18PM +0300, Felipe Contreras wrote:
> It turns out 'words' is a special variable used by zsh completion.
>
> There's probably a bug in zsh's bashcompinit:
> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546
>
> But in the meantime we can workaround it this way.
I would prefer some details about this "workaround it this way" in the
commit message (i.e. that you used "typedef -h", what it does in zsh,
and why it fixes the issue; my zsh-fu is, well, not particularly
large, so I have no idea), so when someone later runs 'git log --
contrib/completion' then he will easier understand what's going on
without the need to look at the patch.
> Currently zsh is completely broken after commit da48616 (bash: get
> --pretty=m<tab> completion to work with bash v4), which introduced
> _get_comp_words_by_ref() that comes from debian's bash_completion
> scripts, and relies on the 'words' variable to behave like any normal
> variable.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> contrib/completion/git-completion.bash | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> This patch is meant for the maintenance branch, so Szeder's patches are not
> needed.
My first two patches are cleanups, they definitely worth
keeping, even when this patch alone would fix the zsh issues.
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 840ae38..8d5eae7 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2710,6 +2710,9 @@ _git ()
> if [[ -n ${ZSH_VERSION-} ]]; then
> emulate -L bash
> setopt KSH_TYPESET
> +
> + # 'words' has special meaning in zsh; override that
> + typeset -h words
You have to do the same in _gitk(), too. Although _gitk() itself
doesn't use $words, it invokes __git_has_doubledash(), which does,
hence it's still broken.
> fi
>
> local cur words cword
> --
> 1.7.5.1.1.g638e6
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for maint branch] git-completion: fix zsh support
2011-05-05 17:34 ` Junio C Hamano
@ 2011-05-05 19:29 ` Felipe Contreras
0 siblings, 0 replies; 14+ messages in thread
From: Felipe Contreras @ 2011-05-05 19:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, SZEDER Gábor
On Thu, May 5, 2011 at 8:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> It turns out 'words' is a special variable used by zsh completion.
>>
>> There's probably a bug in zsh's bashcompinit:
>> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546
>>
>> But in the meantime we can workaround it this way.
>>
>> Currently zsh is completely broken after commit da48616 (bash: get
>> --pretty=m<tab> completion to work with bash v4), which introduced
>> _get_comp_words_by_ref() that comes from debian's bash_completion
>> scripts, and relies on the 'words' variable to behave like any normal
>> variable.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>> contrib/completion/git-completion.bash | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> This patch is meant for the maintenance branch, so Szeder's patches are not
>> needed.
>
> Do you mean sg/completion-upadates that is in 'next' need to be reverted
> and replaced with this one? If that is the case I would love to have an
> Acked-by in this patch.
Are those patches going to be applied on the 'maint' branch? The first
two are general cleanups, the third one is actually fixing stuff, but
it depends on the previous ones. My approach doesn't require any other
patches so it can be applied directly into maint. It can also be
applied on top of the two cleanup patches from Szeder.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH for maint] git-completion: fix zsh support
2011-05-05 18:59 ` SZEDER Gábor
@ 2011-05-05 19:52 ` Felipe Contreras
2011-05-05 23:25 ` Jonathan Nieder
2011-05-06 1:28 ` Jonathan Nieder
0 siblings, 2 replies; 14+ messages in thread
From: Felipe Contreras @ 2011-05-05 19:52 UTC (permalink / raw)
To: git; +Cc: SZEDER Gábor, Felipe Contreras
It turns out 'words' is a special variable used by zsh completion.
There's probably a bug in zsh's bashcompinit:
http://article.gmane.org/gmane.comp.shells.zsh.devel/22546
But in the meantime we can workaround it by using 'typedef -h', which
gets rid of any special meaning.
Currently zsh is completely broken after commit da48616 (bash: get
--pretty=m<tab> completion to work with bash v4), which introduced
_get_comp_words_by_ref() that comes from debian's bash_completion
scripts, and relies on the 'words' variable to behave like any normal
variable.
Comments-by: SZEDER Gábor <szeder@ira.uka.de>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
contrib/completion/git-completion.bash | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
This patch is meant for the maintenance branch, so Szeder's patches are not
required as dependency (althought they are good by themselves).
v2: fix _gitk() too as Szeder suggested.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 840ae38..74c0b4d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2710,6 +2710,9 @@ _git ()
if [[ -n ${ZSH_VERSION-} ]]; then
emulate -L bash
setopt KSH_TYPESET
+
+ # 'words' has special meaning in zsh; override that
+ typeset -h words
fi
local cur words cword
@@ -2761,6 +2764,9 @@ _gitk ()
if [[ -n ${ZSH_VERSION-} ]]; then
emulate -L bash
setopt KSH_TYPESET
+
+ # 'words' has special meaning in zsh; override that
+ typeset -h words
fi
__git_has_doubledash && return
--
1.7.5.1.1.g638e6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH for maint] git-completion: fix zsh support
2011-05-05 19:52 ` [PATCH for maint] " Felipe Contreras
@ 2011-05-05 23:25 ` Jonathan Nieder
2011-05-06 4:51 ` Junio C Hamano
2011-05-06 9:27 ` Felipe Contreras
2011-05-06 1:28 ` Jonathan Nieder
1 sibling, 2 replies; 14+ messages in thread
From: Jonathan Nieder @ 2011-05-05 23:25 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor
Hi,
Felipe Contreras wrote:
> It turns out 'words' is a special variable used by zsh completion.
>
> There's probably a bug in zsh's bashcompinit:
> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546
>
> But in the meantime we can workaround it by using 'typedef -h', which
> gets rid of any special meaning.
As I mentioned before (sorry to come in late; I assume you forgot to
cc the previous participants in the discussion?), I do not think this
is a good fix.
The point here is that 'words' is a special variable used by zsh
completion, and we are using facilities from zsh completion. So
if you set 'typeset -h', then the zsh completion functions will
use _our_ copy of "words".
Now in practice our copy of words matches zsh's anyway, so nothing
goes wrong. But that could easily change in the future.
> Currently zsh is completely broken after commit da48616 (bash: get
> --pretty=m<tab> completion to work with bash v4), which introduced
> _get_comp_words_by_ref() that comes from debian's bash_completion
> scripts
The bash_completion project does not originate in Debian fwiw; it
was originally from Ian Macdonald iirc and available from
http://www.caliban.org/bash/index.shtml
and then it was abandoned. The bash-completion project on alioth is
not Debian-specific, either.
Maybe simplest would be to use Szeder's fix + make the zsh version of
_get_comp_words_by_ref not overwrite "words" at all?
Hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for maint] git-completion: fix zsh support
2011-05-05 19:52 ` [PATCH for maint] " Felipe Contreras
2011-05-05 23:25 ` Jonathan Nieder
@ 2011-05-06 1:28 ` Jonathan Nieder
1 sibling, 0 replies; 14+ messages in thread
From: Jonathan Nieder @ 2011-05-06 1:28 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor
Hi,
Felipe Contreras wrote:
> Currently zsh is completely broken after commit da48616 (bash: get
> --pretty=m<tab> completion to work with bash v4)
I forgot to say: thanks to you and Gábor for working on this! I've
been grumpy lately but that is not your fault. I am very happy to
see someone using the zsh support and keeping an eye on bugs and code
cleanliness.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for maint] git-completion: fix zsh support
2011-05-05 23:25 ` Jonathan Nieder
@ 2011-05-06 4:51 ` Junio C Hamano
2011-05-06 5:27 ` Jonathan Nieder
2011-05-06 9:27 ` Felipe Contreras
1 sibling, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2011-05-06 4:51 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Felipe Contreras, git, SZEDER Gábor
Jonathan Nieder <jrnieder@gmail.com> writes:
> Maybe simplest would be to use Szeder's fix + make the zsh version of
> _get_comp_words_by_ref not overwrite "words" at all?
I do not use zsh myself, but it appears to me that these three-patch
series can graduate and if real zsh users find problems after using it
they can be fixed independenty in-tree.
Would that risk too many patch ping-pong among zsh users on 'master'?
The "don't declare 'local words' in zsh" patch seems to be the right
work-around for the peculiar semantics of "words" array, at least to me.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for maint] git-completion: fix zsh support
2011-05-06 4:51 ` Junio C Hamano
@ 2011-05-06 5:27 ` Jonathan Nieder
2011-05-06 9:34 ` Felipe Contreras
0 siblings, 1 reply; 14+ messages in thread
From: Jonathan Nieder @ 2011-05-06 5:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Felipe Contreras, git, SZEDER Gábor
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Maybe simplest would be to use Szeder's fix + make the zsh version of
>> _get_comp_words_by_ref not overwrite "words" at all?
>
> I do not use zsh myself, but it appears to me that these three-patch
> series can graduate and if real zsh users find problems after using it
> they can be fixed independenty in-tree.
>
> Would that risk too many patch ping-pong among zsh users on 'master'?
> The "don't declare 'local words' in zsh" patch seems to be the right
> work-around for the peculiar semantics of "words" array, at least to me.
Gábor's patches already work. I don't think they will cause breakage
or patch ping-pong.
I was trying to imagine Felipe's objection and all I could think of
was that it is not so appealing that _get_comp_words_by_ref is not
actually writing to "words". For example, the following on top of
sg/completion-updates (= 3bee6a4) will print a greeting and the words
being completed when you press tab, rather than <foo> <bar> <baz>:
-- >8 --
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -469,7 +469,8 @@ _get_comp_words_by_ref ()
prev=${COMP_WORDS[COMP_CWORD-1]}
;;
words)
- words=("${COMP_WORDS[@]}")
+ words=(foo bar baz)
+ echo >&2 Greetings
;;
cword)
cword=$COMP_CWORD
@@ -2614,6 +2615,9 @@ _git ()
local cur cword prev
_get_comp_words_by_ref -n =: cur words cword prev
+
+ printf >&2 "WORDS: <%s>\n" "${words[@]}"
+
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
-- 8< --
In practice it works great since "words" already has the right
content, but maybe the "typeset -h" suggestion was motivated by a
desire to have something easier to explain.
I don't think that's a very strong reason to prevent the fix from
graduating, though I suppose I would be happy to see something like
the following on top at some point.
-- 8< --
Subject: completion: do not pretend to assign to special variable $words on zsh
The special variable $words already has essentially the same meaning
as bash's COMP_WORDS on zsh. While assigning one to the other appears
to work okay, as a no-op, that is actually an illusion --- the value
of $words can be changed in the _get_comp_words_by_ref function where
it is assigned, but after that function returns, $words is back to
normal.
Guard against future breakage by adding a comment mentioning this and
removing the redundant assignment.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
contrib/completion/git-completion.bash | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 10c1b83..8dfd97f 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -469,7 +469,10 @@ _get_comp_words_by_ref ()
prev=${COMP_WORDS[COMP_CWORD-1]}
;;
words)
- words=("${COMP_WORDS[@]}")
+ # Nothing to do --- zsh's $words already contains
+ # the list of words being completed (and if we
+ # try to assign to it, the value wouldn't propagate
+ # from this function anyway).
;;
cword)
cword=$COMP_CWORD
--
1.7.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH for maint] git-completion: fix zsh support
2011-05-05 23:25 ` Jonathan Nieder
2011-05-06 4:51 ` Junio C Hamano
@ 2011-05-06 9:27 ` Felipe Contreras
2011-05-06 9:59 ` Jonathan Nieder
1 sibling, 1 reply; 14+ messages in thread
From: Felipe Contreras @ 2011-05-06 9:27 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor
[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]
On Fri, May 6, 2011 at 2:25 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> It turns out 'words' is a special variable used by zsh completion.
>>
>> There's probably a bug in zsh's bashcompinit:
>> http://article.gmane.org/gmane.comp.shells.zsh.devel/22546
>>
>> But in the meantime we can workaround it by using 'typedef -h', which
>> gets rid of any special meaning.
>
> As I mentioned before (sorry to come in late; I assume you forgot to
> cc the previous participants in the discussion?), I do not think this
> is a good fix.
>
> The point here is that 'words' is a special variable used by zsh
> completion, and we are using facilities from zsh completion. So
> if you set 'typeset -h', then the zsh completion functions will
> use _our_ copy of "words".
No, the scope remains local.
>From the manual:
---
Hide: only useful for special parameters (those marked `<S>' in the
table in zshparam(1)), and for local parameters with the same name as
a special parameter, though harmless for others. A special parameter
with this attribute will not retain its special effect when made
local. Thus after `typeset -h PATH', a function containing `typeset
PATH' will create an ordinary local parameter without the usual
behaviour of PATH. Alterna‐ tively, the local parameter may itself be
given this attribute; hence inside a function `typeset -h PATH'
creates an ordinary local parameter and the special PATH parameter is
not altered in any way. It is also possible to create a local
parameter using `typeset +h special', where the local copy of special
will retain its special properties regardless of having the -h
attribute. Global special parameters loaded from shell modules (cur‐
rently those in zsh/mapfile and zsh/parameter) are automatically given
the -h attribute to avoid name clashes.
---
> Now in practice our copy of words matches zsh's anyway, so nothing
> goes wrong. But that could easily change in the future.
It doesn't matter if we change the value of 'words'. See the attached
test. The result is:
complete: words=foo (before)
foo: cur=foo words=blah cwords=1
foo bar: cur=foo words=blah cwords=1
complete: words=foo (after)
See? No change.
In fact, if you follow the link I posted, that's precisely the fix the
zsh guys were pushing for. And the it is already merged:
http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa
I also discussed the workaround with zsh guys:
http://article.gmane.org/gmane.comp.shells.zsh.devel/22557
>> Currently zsh is completely broken after commit da48616 (bash: get
>> --pretty=m<tab> completion to work with bash v4), which introduced
>> _get_comp_words_by_ref() that comes from debian's bash_completion
>> scripts
>
> The bash_completion project does not originate in Debian fwiw; it
> was originally from Ian Macdonald iirc and available from
>
> http://www.caliban.org/bash/index.shtml
>
> and then it was abandoned. The bash-completion project on alioth is
> not Debian-specific, either.
Well, it's hosted on debian.org, and I haven't seen it used anywhere
else. I just don't know how else to identify that project.
> Maybe simplest would be to use Szeder's fix + make the zsh version of
> _get_comp_words_by_ref not overwrite "words" at all?
I think the simplest fix is the one I'm proposing, which zsh guys agree with.
Cheers.
--
Felipe Contreras
[-- Attachment #2: zsh_words_2 --]
[-- Type: application/octet-stream, Size: 717 bytes --]
#!zsh
set_vars ()
{
cur=${COMP_WORDS[COMP_CWORD]}
# words=("${COMP_WORDS[@]}")
words="blah"
cwords=$COMP_CWORD
}
_foo_bar ()
{
local cur words cwords
set_vars
echo "foo bar: cur=${cur} words=${words} cwords=${cwords}" >> /tmp/comp_test.txt
}
_foo ()
{
typeset -h words
local cur words cwords
set_vars
echo "foo: cur=${cur} words=${words} cwords=${cwords}" >> /tmp/comp_test.txt
_foo_bar
}
bash_complete ()
{
(( COMP_CWORD = CURRENT - 1))
COMP_WORDS=( $words )
COMPREPLY=()
echo "complete: words=${words} (before)" >> /tmp/comp_test.txt
_foo "${words[0]}" "${words[CURRENT-1]}" "${words[CURRENT-2]}"
echo "complete: words=${words} (after)" >> /tmp/comp_test.txt
}
compdef bash_complete foo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for maint] git-completion: fix zsh support
2011-05-06 5:27 ` Jonathan Nieder
@ 2011-05-06 9:34 ` Felipe Contreras
0 siblings, 0 replies; 14+ messages in thread
From: Felipe Contreras @ 2011-05-06 9:34 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, SZEDER Gábor
On Fri, May 6, 2011 at 8:27 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> Maybe simplest would be to use Szeder's fix + make the zsh version of
>>> _get_comp_words_by_ref not overwrite "words" at all?
>>
>> I do not use zsh myself, but it appears to me that these three-patch
>> series can graduate and if real zsh users find problems after using it
>> they can be fixed independenty in-tree.
>>
>> Would that risk too many patch ping-pong among zsh users on 'master'?
>> The "don't declare 'local words' in zsh" patch seems to be the right
>> work-around for the peculiar semantics of "words" array, at least to me.
>
> Gábor's patches already work. I don't think they will cause breakage
> or patch ping-pong.
>
> I was trying to imagine Felipe's objection and all I could think of
> was that it is not so appealing that _get_comp_words_by_ref is not
> actually writing to "words". For example, the following on top of
> sg/completion-updates (= 3bee6a4) will print a greeting and the words
> being completed when you press tab, rather than <foo> <bar> <baz>:
[...]
> In practice it works great since "words" already has the right
> content, but maybe the "typeset -h" suggestion was motivated by a
> desire to have something easier to explain.
>
> I don't think that's a very strong reason to prevent the fix from
> graduating, though I suppose I would be happy to see something like
> the following on top at some point.
>
> -- 8< --
> Subject: completion: do not pretend to assign to special variable $words on zsh
>
> The special variable $words already has essentially the same meaning
> as bash's COMP_WORDS on zsh. While assigning one to the other appears
> to work okay, as a no-op, that is actually an illusion --- the value
> of $words can be changed in the _get_comp_words_by_ref function where
> it is assigned, but after that function returns, $words is back to
> normal.
>
> Guard against future breakage by adding a comment mentioning this and
> removing the redundant assignment.
I don't really object to Gábor's patches. The third one actually fixes
the issue.
The thing is that it's a *workaround*, and I prefer to keep
workarounds to the minimal. So I prefer my patch that introduces only
2 new lines and doesn't change the semantics of the rest of the code.
Moreover, it's already fixed in zsh's master, so at some point we
might want to remove it, and it's easier to remove 2 non-intrusive
lines.
But anyway, my end goal is to get this into the 'maint' branch. It's
up to Junio how to get the fix there (if at all), but I think it's
easier to just apply my patch there.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for maint] git-completion: fix zsh support
2011-05-06 9:27 ` Felipe Contreras
@ 2011-05-06 9:59 ` Jonathan Nieder
2011-05-06 10:06 ` Jonathan Nieder
2011-05-09 13:51 ` Felipe Contreras
0 siblings, 2 replies; 14+ messages in thread
From: Jonathan Nieder @ 2011-05-06 9:59 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor
Felipe Contreras wrote:
> No, the scope remains local.
Is this local in the sense of typeset used in ksh functions declared
as "function f" or local in the sense of typeset used in ksh functions
declared as "f ()"? If the latter, I think you misunderstood me --- I
never meant to suggest otherwise.
> In fact, if you follow the link I posted, that's precisely the fix the
> zsh guys were pushing for. And the it is already merged:
> http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa
Now this changes things (since it amounts to a guarantee that the
bash completion emulation has already extracted all it needs from
$words before calling a completion function). What happens if someone
calls "compgen -F" after words is hidden?
> Well, it's hosted on debian.org, and I haven't seen it used anywhere
> else. I just don't know how else to identify that project.
Here you go: http://pkgs.fedoraproject.org/gitweb/?p=bash-completion.git;a=tree
It's called bash-completion or bash_completion. It really has
basically nothing to do with Debian; they just are using Debian
infrastructure. But hey, call it what you want. I don't like
arguing.
> I think the simplest fix is the one I'm proposing
I'm happy with it as long as the zsh people are committed to making
sure it continues to work (which it sounds like they are, luckily).
I suppose it is intended to obsolete the third patch from Gábor's
series? Could you provide an explanation for the commit log,
something to the effect that
- the words array has special meaning
- that produces such-and-such puzzling symptoms
- zsh 4.3.12 (?) will fix it by using "typeset -h" to hide it when
running completion functions designed for bash
- we can make the same fix to work correctly with earlier versions of
zsh, and that's what this patch does
?
Sorry I have been so dense --- hopefully I understand it roughly
now.
Regards,
Jonathan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for maint] git-completion: fix zsh support
2011-05-06 9:59 ` Jonathan Nieder
@ 2011-05-06 10:06 ` Jonathan Nieder
2011-05-09 13:51 ` Felipe Contreras
1 sibling, 0 replies; 14+ messages in thread
From: Jonathan Nieder @ 2011-05-06 10:06 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, SZEDER Gábor
Jonathan Nieder wrote:
> What happens if someone
> calls "compgen -F" after words is hidden?
For avoidance of doubt: here "after words is hidden" means "from a
scope in which words is hidden".
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for maint] git-completion: fix zsh support
2011-05-06 9:59 ` Jonathan Nieder
2011-05-06 10:06 ` Jonathan Nieder
@ 2011-05-09 13:51 ` Felipe Contreras
1 sibling, 0 replies; 14+ messages in thread
From: Felipe Contreras @ 2011-05-09 13:51 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, SZEDER Gábor
On Fri, May 6, 2011 at 12:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> No, the scope remains local.
>
> Is this local in the sense of typeset used in ksh functions declared
> as "function f" or local in the sense of typeset used in ksh functions
> declared as "f ()"? If the latter, I think you misunderstood me --- I
> never meant to suggest otherwise.
I'm sorry, I don't know what you mean.
By "local" I mean that all the layers bellow in the call stack will
use a separate "words" variable than the layers above.
>> In fact, if you follow the link I posted, that's precisely the fix the
>> zsh guys were pushing for. And the it is already merged:
>> http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa
>
> Now this changes things (since it amounts to a guarantee that the
> bash completion emulation has already extracted all it needs from
> $words before calling a completion function). What happens if someone
> calls "compgen -F" after words is hidden?
I fail to see an example of that. Why don't you try?
>> Well, it's hosted on debian.org, and I haven't seen it used anywhere
>> else. I just don't know how else to identify that project.
>
> Here you go: http://pkgs.fedoraproject.org/gitweb/?p=bash-completion.git;a=tree
That's just packaging:
URL: http://bash-completion.alioth.debian.org/
http://pkgs.fedoraproject.org/gitweb/?p=bash-completion.git;a=blob;f=bash-completion.spec;h=855462dd092a50959d9576f6bc02c01332dd63a2;hb=HEAD#l12
>> I think the simplest fix is the one I'm proposing
>
> I'm happy with it as long as the zsh people are committed to making
> sure it continues to work (which it sounds like they are, luckily).
> I suppose it is intended to obsolete the third patch from Gábor's
> series? Could you provide an explanation for the commit log,
> something to the effect that
>
> - the words array has special meaning
> - that produces such-and-such puzzling symptoms
> - zsh 4.3.12 (?) will fix it by using "typeset -h" to hide it when
> running completion functions designed for bash
> - we can make the same fix to work correctly with earlier versions of
> zsh, and that's what this patch does
>
> ?
I thought that's what I explained more or less. I will add the missing
point and the new developments on the commit message and resend.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-05-09 13:52 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-05-05 14:24 [PATCH for maint branch] git-completion: fix zsh support Felipe Contreras
2011-05-05 17:34 ` Junio C Hamano
2011-05-05 19:29 ` Felipe Contreras
2011-05-05 18:59 ` SZEDER Gábor
2011-05-05 19:52 ` [PATCH for maint] " Felipe Contreras
2011-05-05 23:25 ` Jonathan Nieder
2011-05-06 4:51 ` Junio C Hamano
2011-05-06 5:27 ` Jonathan Nieder
2011-05-06 9:34 ` Felipe Contreras
2011-05-06 9:27 ` Felipe Contreras
2011-05-06 9:59 ` Jonathan Nieder
2011-05-06 10:06 ` Jonathan Nieder
2011-05-09 13:51 ` Felipe Contreras
2011-05-06 1:28 ` Jonathan Nieder
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).