patchgit-completion: fix zsh support

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.

Felipe Contreras <felipe.contreras@gmail.com> writes:

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.

On Thu, May 5, 2011 at 8:34 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

Hi,

On Thu, May 05, 2011 at 05:24:18PM +0300, Felipe Contreras wrote:

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.

My first two patches are cleanups, they definitely worth keeping, even when this patch alone would fix the zsh issues.

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.

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.

Hi,

Felipe Contreras wrote:

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.

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

Jonathan Nieder <jrnieder@gmail.com> writes:

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.

Junio C Hamano wrote:

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 --

On Fri, May 6, 2011 at 8:27 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

[...]

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.

On Fri, May 6, 2011 at 2:25 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

No, the scope remains local.

---
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.
---

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

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.

I think the simplest fix is the one I'm proposing, which zsh guys agree with.

Cheers.

Felipe Contreras wrote:

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.

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?

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'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

Jonathan Nieder wrote:

For avoidance of doubt: here "after words is hidden" means "from a scope in which words is hidden".

On Fri, May 6, 2011 at 12:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

I fail to see an example of that. Why don't you try?

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 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.

Hi,

Felipe Contreras wrote:

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.