[PATCH v5 (for maint)] git-completion: fix regression in zsh support

[PATCH v5 (for maint)] git-completion: fix regression in zsh support

[Felipe Contreras]

Right now zsh is quite broken; the completion doesn't notice when
there's a subcommand. For example: "git log origi<TAB>" gives no
completions because it would try to find a "git origi..." command. The
issue would be gone in zsh 4.3.12, but for now we can workaround it by
porting the same fix.

The problem started after commit v1.7.4-rc0~11^2~2 (bash: get
--pretty=m<tab> completion to work with bash v4), which introduced
_get_comp_words_by_ref() that comes from bash-completion[1] scripts, and
relies on the 'words' variable. However, it turns out 'words' is a
special variable used by zsh completion. From zshcompwid(1):

  [...] the parameters are reset on each function exit (including nested
  function calls from within the completion widget) to the values they
  had when the function was entered.

As a result, subcommand words are lost.

This is now fixed in the latest master branch of zsh[2] by simply
defining 'words' as hidden (typeset -h), which removes the special
meaning inside the emulated bash function. So let's do the same.

Jonathan Nieder helped on the commit message.

[1] http://bash-completion.alioth.debian.org/
[2] http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa

Reported-by: Stefan Haller <lists@haller-berlin.de>
Comments-by: SZEDER Gábor <szeder@ira.uka.de>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

v2: fix _gitk() too as Szeder suggested.

v3: improve commit message as Jonathan Nieder suggested.
    Also, improve comments.

v4: more commit message improvements

v5: even more commit message improvements from Jonathan's version

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 840ae38..a7d20df 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2710,6 +2710,10 @@ _git ()
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
 		setopt KSH_TYPESET
+
+		# workaround zsh's bug that leaves 'words' as a special
+		# variable in versions < 4.3.12
+		typeset -h words
 	fi
 
 	local cur words cword
@@ -2761,6 +2765,10 @@ _gitk ()
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
 		setopt KSH_TYPESET
+
+		# workaround zsh's bug that leaves 'words' as a special
+		# variable in versions < 4.3.12
+		typeset -h words
 	fi
 
 	__git_has_doubledash && return
-- 
1.7.5.1

Re: [PATCH v5 (for maint)] git-completion: fix regression in zsh support

[Junio C Hamano]

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

> Right now zsh is quite broken;...

Yuck--sounds like zsh as a whole is broken, but certainly you didn't mean
that ;-) Could you also clarify "Right now" a bit?

Is "The git-completion script in contrib/ in 1.7.5 is broken for versions
of zsh before 4.3.12" a good rewrite of the above?

No need for resend if that is the only change required.

Re: [PATCH v5 (for maint)] git-completion: fix regression in zsh support

[Felipe Contreras]

On Tue, May 10, 2011 at 8:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Right now zsh is quite broken;...
>
> Yuck--sounds like zsh as a whole is broken, but certainly you didn't mean
> that ;-)

Right: s/zsh/zsh support/

> Could you also clarify "Right now" a bit?
>
> Is "The git-completion script in contrib/ in 1.7.5 is broken for versions
> of zsh before 4.3.12" a good rewrite of the above?

Well, "right now" all versions of zsh are before 4.3.12. I also don't
see the point mentioning of 1.7.5. Maybe "The git-completion script in
contrib/ is quite broken".

> of zsh before 4.3.12"
> No need for resend if that is the only change required.

All right. Hopefully there won't be other changes.

-- 
Felipe Contreras

Re: [PATCH v5 (for maint)] git-completion: fix regression in zsh support

[Junio C Hamano]

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

> Well, "right now" all versions of zsh are before 4.3.12.

I was trying to help people who read the log in 12 months.  They can
certainly see the date of the commit, but then they have to correlate that
date with the then-current version of zsh if you do not mention it.

> I also don't
> see the point mentioning of 1.7.5. Maybe "The git-completion script in
> contrib/ is quite broken".

That is Ok by me, if you mean this zsh breakages is not something our
recent change has exposed.  Mentioning 1.7.5 only helps the users if
git-completion.bash in 1.7.4 did not have an issue even on the current zsh
and 1.7.5 had an issue. I thought you may know which without me having to
dig that information out.

Re: [PATCH v5 (for maint)] git-completion: fix regression in zsh support

[Jonathan Nieder]

Junio C Hamano wrote:

> Mentioning 1.7.5 only helps the users if
> git-completion.bash in 1.7.4 did not have an issue even on the current zsh
> and 1.7.5 had an issue. I thought you may know which without me having to
> dig that information out.

The zsh support was introduced in 1.7.4-rc0[1] and the breakage was introduced
in 1.7.4-rc0[2], for what it's worth.

[1] v1.7.4-rc0~169^2 (completion: make compatible with zsh, 2010-09-06)
[2] v1.7.4-rc0~11^2~2 (bash: get --pretty=m<tab> completion to work
with bash v4, 2010-12-02)

Re: [PATCH v5 (for maint)] git-completion: fix regression in zsh support

[Jonathan Nieder]

Felipe Contreras wrote:

> Right now zsh is quite broken; the completion doesn't notice when
> there's a subcommand. For example: "git log origi<TAB>" gives no
> completions because it would try to find a "git origi..." command. The
> issue would be gone in zsh 4.3.12, but for now we can workaround it by
> porting the same fix.
>
> The problem started after commit v1.7.4-rc0~11^2~2 (bash: get
> --pretty=m<tab> completion to work with bash v4), which introduced
> _get_comp_words_by_ref() that comes from bash-completion[1] scripts, and
> relies on the 'words' variable. However, it turns out 'words' is a

The _get_comp_words_by_ref function does not rely on but writes to the
'words' variable.  Aside from that, this is a nicer summary.

> special variable used by zsh completion. From zshcompwid(1):
[...]
> [2] http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa

I doubt this URL will last forever.  Why not just say commit
e880604f029088f32fb1ecc, or '"29140: hide the "words" special variable
so that it may be used as an ordinary variable by bash completions",
2011-05-04' which conveniently includes a mailing list reference?

Thanks for the fix and your patience with me.  To answer the sincere
questions I could find among your replies to v4:

 - I included the "completion: move private shopt shim for zsh to
   __git_ namespace" patch in the series as a reminder to Junio,
   since he had requested one.

 - "in no part of [compgen] does the zsh emulation make use of the
   'words' special variable" is just plain false.  compgen -F uses the
   'words' variable, and we are lucky to have not needed to use it
   yet.

 - I changed the comment because when I read the original, putting
   myself in the shoes of someone who didn't know the context, I found
   it confusing.  "work around bug in old versions of zsh" just leaves
   me wondering "What bug?  Is it a bug in the behavior of 'local',
   and are we working around it by using an alternate spelling?"  

   That said, I suppose the main point of a comment is to say
   "something important happens here", and a person can use 

	git log -S"workaround zsh's bug" -- contrib/completion/git-completion.bash

   to find out what exactly it is, so no harm done.