[PATCH v3 (for maint)] git-completion: fix zsh support

[PATCH v3 (for maint)] git-completion: fix zsh support

[Felipe Contreras]

It turns out 'words' is a special variable used by zsh completion, sort
of like 'COMP_WORDS' in bash. This was not isolated correctly in zsh's
bash completion, so by trying to set it as 'local' in git's completion,
unexpected results occur; assignations are not propagated to upper
levels in the call stack.

This is now fixed in the latest master branch of zsh[1] by simply
defining 'words' as hidden (typeset -h), which removes the special
meaning inside the emulated bash function. It probably won't be released
until version 4.3.12.

In the meantime, we can workaround the issue by doing the same; defining
words as hidden (typeset -h) as soon as possible.

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

[1] http://zsh.git.sourceforge.net/git/gitweb.cgi?p=zsh/zsh;a=commitdiff;h=e880604f029088f32fb1ecc39213d720ae526aaa

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

This patch is meant for the maintenance branch, so Szeder's patches are not
required.

v2: fix _gitk() too as Szeder suggested.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 840ae38..763f145 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 bashinit'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 bashinit'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.1.g638e6

Re: [PATCH v3 (for maint)] git-completion: fix zsh support

[Jonathan Nieder]

Felipe Contreras wrote:

> It turns out 'words' is a special variable used by zsh completion, sort
> of like 'COMP_WORDS' in bash.

I was not aware that COMP_WORDS was special (rather than just
prepopulated).  Is it?

> This was not isolated correctly in zsh's
> bash completion, so by trying to set it as 'local' in git's completion,
> unexpected results occur; assignations are not propagated to upper
> levels in the call stack.

Does the call stack grow up or down?  I suspect this means:

	zsh's bash completion emulation layer does not sufficiently
	insulate us from that reality.  In particular, the variable
	keeps the "special" attribute (even after a declaration "local
	words"), so assignments within a function are undone whenever
	the function returns.

	In particular, until 3bee6a473 (completion: don't declare
	'local words' to make zsh happy, 2011-04-28), the "words" array
	would be cleared in _git by declaring "local words" and its new
	value would never be propagated from _get_comp_words_by_ref so
	it remained empty and the completion script could not tell that
	there were existing subcommand names on the command line (so
	"git log m<TAB>" would complete subcommand names).

	And even after 3bee6a473 we do not have the ability to modify
	words.  (... explanation of impact of the change goes here ...)

I am not a great writer so that is probably more verbose than needed.
So it might be better for me to describe the goals of a commit message:

 1) the text should be specific about what the commit fixes, so
    someone reading it later (e.g., after bisecting) does not come
    around and accidentally break it
 
 2) in particular, the text should be specific about the observable
    symptoms, so it can be easier to check if a later change has
    broken it.

> This is now fixed in the latest master branch of zsh[1] by simply
> defining 'words' as hidden (typeset -h), which removes the special
> meaning inside the emulated bash function. It probably won't be released
> until version 4.3.12.
>
> In the meantime, we can workaround the issue by doing the same; defining
> words as hidden (typeset -h) as soon as possible.

It might make sense to reverse the order of these: first explain the
fix in the context of the problem being solved, and then add a note
mentioning that the fix will not be needed for long and that the
method is the same as what zsh is planning to use.

Meanwhile this doesn't address the risk that functions called by the
completion script will use $words.  Outside the context of the commit
message I think you've said something about that (e.g., that the zsh
developers prefer this fix --- a reference would be nice so we could
steal their rationale).  Maybe the best thing to say would be "that is
a risk, but let's wait and see", to give future readers more
confidence that that was considered but it is ok to fix it if it comes
up?

> --- 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 bashinit's bug that leaves 'words' as a
> +		# special variable in versions < 4.3.12
> +		typeset -h words

I don't think the comment clarifies much.  What is the intended
message to the reader?  For example if it is "don't remove this line
unless you use zsh 4.3.12 or greater", I'd say something like

		# bashcompinit versions after 4.3.12 already hide the
		# special "words" variable already.  We do it
		# again ourselves to support older zsh versions.

Hope that helps,
Jonathan

Re: [PATCH v3 (for maint)] git-completion: fix zsh support

[Felipe Contreras]

On Tue, May 10, 2011 at 12:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> It turns out 'words' is a special variable used by zsh completion, sort
>> of like 'COMP_WORDS' in bash.
>
> I was not aware that COMP_WORDS was special (rather than just
> prepopulated).  Is it?

I didn't mean to suggest it was special, only what 'words' was used
for. I'll remove that section as it doesn't really matter.

>> This was not isolated correctly in zsh's
>> bash completion, so by trying to set it as 'local' in git's completion,
>> unexpected results occur; assignations are not propagated to upper
>> levels in the call stack.
>
> Does the call stack grow up or down?  I suspect this means:

I'll change that to "outer levels".

>        zsh's bash completion emulation layer does not sufficiently
>        insulate us from that reality.  In particular, the variable
>        keeps the "special" attribute (even after a declaration "local
>        words"), so assignments within a function are undone whenever
>        the function returns.

That explains less.

>        In particular, until 3bee6a473 (completion: don't declare
>        'local words' to make zsh happy, 2011-04-28), the "words" array
>        would be cleared in _git by declaring "local words" and its new
>        value would never be propagated from _get_comp_words_by_ref so
>        it remained empty and the completion script could not tell that
>        there were existing subcommand names on the command line (so
>        "git log m<TAB>" would complete subcommand names).

I don't see the point in explaining in excruciating detail all the
series of steps in which an unset variable causes problems; the
variable doesn't get set, thus one can assume there are problems. And
the problem is already explained to be that completion is completely
broken.

>        And even after 3bee6a473 we do not have the ability to modify
>        words.  (... explanation of impact of the change goes here ...)
>
> I am not a great writer so that is probably more verbose than needed.
> So it might be better for me to describe the goals of a commit message:
>
>  1) the text should be specific about what the commit fixes, so
>    someone reading it later (e.g., after bisecting) does not come
>    around and accidentally break it

See the title: fix zsh support

>  2) in particular, the text should be specific about the observable
>    symptoms, so it can be easier to check if a later change has
>    broken it.

From the title: zsh completion doesn't work at all. Which is repeated again:
---
Right now zsh is completely broken...
---

If zsh completion is completely broken, chance are it has to do with
this. And I did explain the most obvious test; check if the value of
'words' doesn't get propagated to the upper layers in the call stack.

>> This is now fixed in the latest master branch of zsh[1] by simply
>> defining 'words' as hidden (typeset -h), which removes the special
>> meaning inside the emulated bash function. It probably won't be released
>> until version 4.3.12.
>>
>> In the meantime, we can workaround the issue by doing the same; defining
>> words as hidden (typeset -h) as soon as possible.
>
> It might make sense to reverse the order of these: first explain the
> fix in the context of the problem being solved, and then add a note
> mentioning that the fix will not be needed for long and that the
> method is the same as what zsh is planning to use.

That's what I did initially, but everyone doubted my solution. Now I
want to start making sure people see it's a good solution.

> Meanwhile this doesn't address the risk that functions called by the
> completion script will use $words.

It does.

> Outside the context of the commit
> message I think you've said something about that (e.g., that the zsh
> developers prefer this fix --- a reference would be nice so we could
> steal their rationale).

I did. If you don't like their commit message, talk to them.

> Maybe the best thing to say would be "that is
> a risk, but let's wait and see", to give future readers more
> confidence that that was considered but it is ok to fix it if it comes
> up?

I don't see any risk.

>> --- 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 bashinit's bug that leaves 'words' as a
>> +             # special variable in versions < 4.3.12
>> +             typeset -h words
>
> I don't think the comment clarifies much.  What is the intended
> message to the reader?  For example if it is "don't remove this line
> unless you use zsh 4.3.12 or greater", I'd say something like
>
>                # bashcompinit versions after 4.3.12 already hide the
>                # special "words" variable already.  We do it
>                # again ourselves to support older zsh versions.

I think it's perfectly clear. Specially if you compare it against the
two lines above that have no explanation at all. Double standards
much?

-- 
Felipe Contreras

[PATCH v4 (for maint)] git-completion: fix zsh support

[Felipe Contreras]

It turns out 'words' is a special variable used by zsh completion. This
was not isolated correctly in zsh's bash completion emulation, so by
trying to set it as 'local' in git's completion, unexpected results
occur; assignations are not propagated to outer levels in the call
stack.

This is now fixed in the latest master branch of zsh[1] by simply
defining 'words' as hidden (typeset -h), which removes the special
meaning inside the emulated bash function. It probably won't be released
until version 4.3.12.

In the meantime, we can workaround the issue by doing the same; defining
words as hidden (typeset -h) as soon as possible.

Right now 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 bash-completion[2] scripts, and
relies on the 'words' variable to behave like any normal variable.

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

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

This patch is meant for the maintenance branch, so Szeder's patches are not
required.

v2: fix _gitk() too as Szeder suggested.

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

v4: more commit message improvements

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 840ae38..763f145 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 bashinit'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 bashinit'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.1.g638e6

Re: [PATCH v4 (for maint)] git-completion: fix zsh support

[Jonathan Nieder]

Felipe Contreras wrote:

> v4: more commit message improvements

Thanks.  You seem to have ignored most of my review; I'll try to find
time to clarify the commit message if the consensus is that this
approach is a good idea.

An alternative possibility if we need this fixed in the v1.7.5.x
series (do we?) would be cherry-picking the fix from
sg/completion-updates on top of maint.

To clarify the trade-offs:

 - in terms of lines of code, the fix itself in sg/completion-updates
   and this fix are about the same size.  But the sg/completion-updates
   version relies on a code cleanup.

 - the fix in sg/completion-updates is less likely to be broken by
   future changes in the bashcompinit library.

 - this fix is conceptually simpler.  In a way, the fix in
   sg/completion-updates only works by accident.

Re: [PATCH v4 (for maint)] git-completion: fix zsh support

[Felipe Contreras]

On Tue, May 10, 2011 at 1:53 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> v4: more commit message improvements
>
> Thanks.  You seem to have ignored most of my review;

I did not ignore it, I replied to your comments. A review doesn't
meant that I should do what you say.

> I'll try to find
> time to clarify the commit message if the consensus is that this
> approach is a good idea.
>
> An alternative possibility if we need this fixed in the v1.7.5.x
> series (do we?) would be cherry-picking the fix from
> sg/completion-updates on top of maint.

Which cannot be done; you need the rest of the cleanups. Otherwise you
would have to make many many intrusive changes.

> To clarify the trade-offs:
>
>  - in terms of lines of code, the fix itself in sg/completion-updates
>   and this fix are about the same size.  But the sg/completion-updates
>   version relies on a code cleanup.

It's not:

fc: 1 files changed, 2 insertions(+), 0 deletions(-)
sg: 1 files changed, 8 insertions(+), 0 deletions(-)

That is if I remove the comments I added; Gabor's version doesn't have any.

>  - the fix in sg/completion-updates is less likely to be broken by
>   future changes in the bashcompinit library.

How exactly?

>  - this fix is conceptually simpler.  In a way, the fix in
>   sg/completion-updates only works by accident.

You are missing other advantages:

- thix fix is less intrusive
- this fix would be easier to remove in the future (kind of comes from
the previous one)
- this fix has been acknowledge by zsh developers
- this fix can be applies both on 'maint' and on top of Gabor's
reorganization without changes

I really don't understand how adding two lines of code that make
something from totally broken to working properly can be so difficult.

-- 
Felipe Contreras

Re: [PATCH v4 (for maint)] git-completion: fix zsh support

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> An alternative possibility if we need this fixed in the v1.7.5.x
> series (do we?) would be cherry-picking the fix from
> sg/completion-updates on top of maint.

As contrib/ material, I personally don't think we would care strongly
enough to bother.

> To clarify the trade-offs:
>
>  - in terms of lines of code, the fix itself in sg/completion-updates
>    and this fix are about the same size.  But the sg/completion-updates
>    version relies on a code cleanup.
>
>  - the fix in sg/completion-updates is less likely to be broken by
>    future changes in the bashcompinit library.
>
>  - this fix is conceptually simpler.  In a way, the fix in
>    sg/completion-updates only works by accident.

Hmm, zsh does not want to see "word" getting localized by the user because
it has a special semantics associated with it.  Szeder avoids localizing
it.  Felipe sidesteps the issue by stripping the funny special semantics
from the variable.  I guess both have a similar degree of conceptual
simplicity.

One big thing going for this patch is that this is the blessed solution
zsh folks themselves like to use, no? The repeated mention "zsh bashinit
bug" in the code seems to suggest that it is the case.

I do not mind reverting sg/completion-updates from 'next' (please remind
me that I need to resurrect your "private shopt shim" separately if we go
this route) and applying a cleaned-up version of this one.

Thanks.

Re: [PATCH v4 (for maint)] git-completion: fix zsh support

[Jonathan Nieder]

Felipe Contreras wrote:
> On Tue, May 10, 2011 at 1:53 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>  - the fix in sg/completion-updates is less likely to be broken by
>>   future changes in the bashcompinit library.
>
> How exactly?

Because there remains the possibility that functions from bashcompinit
will make use of the $words variable.  I have said this about three
times.  It is not very likely, assuming the zsh developers want to keep
supporting that fix (and I think they should), but the chance is there.

>>  - this fix is conceptually simpler.  In a way, the fix in
>>   sg/completion-updates only works by accident.
>
> You are missing other advantages:

Sorry, I should have prefaced the above with "in my opinion".  And to
be clear, I am not saying this fix should not be applied; I am just
explaining the trade-offs as I understand them.

The reason I asked for another opinion is that I find it hard to be
objective in this case, because of another consideration I didn't
mention: each moment I have been spending on this is an exercise in
frustration.

I even _like_ your fix (just like I _like_ Gábor's cleanup and see
no reason to contemplate the possibility of backing it out).  So it's
a shame.

Re: [PATCH v4 (for maint)] git-completion: fix zsh support

[Jonathan Nieder]

Junio C Hamano wrote:

> One big thing going for this patch is that this is the blessed solution
> zsh folks themselves like to use, no? The repeated mention "zsh bashinit
> bug" in the code seems to suggest that it is the case.
>
> I do not mind reverting sg/completion-updates from 'next' (please remind
> me that I need to resurrect your "private shopt shim" separately if we go
> this route) and applying a cleaned-up version of this one.

Thanks, will spend some time later today to do so.

> Thanks.

Sorry for all the noise.  I wish I knew a way to work on patches like
this one without causing so much animosity.

Re: [PATCH v4 (for maint)] git-completion: fix zsh support

[Felipe Contreras]

On Tue, May 10, 2011 at 2:28 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Tue, May 10, 2011 at 1:53 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>>  - the fix in sg/completion-updates is less likely to be broken by
>>>   future changes in the bashcompinit library.
>>
>> How exactly?
>
> Because there remains the possibility that functions from bashcompinit
> will make use of the $words variable.  I have said this about three
> times.  It is not very likely, assuming the zsh developers want to keep
> supporting that fix (and I think they should), but the chance is there.

And I even wrote a test to show you that's not the case:
http://article.gmane.org/gmane.comp.version-control.git/172963

Now, can you modify my test to explain how *exactly* zsh folks can
screw my patch up?

>>>  - this fix is conceptually simpler.  In a way, the fix in
>>>   sg/completion-updates only works by accident.
>>
>> You are missing other advantages:
>
> Sorry, I should have prefaced the above with "in my opinion".  And to
> be clear, I am not saying this fix should not be applied; I am just
> explaining the trade-offs as I understand them.
>
> The reason I asked for another opinion is that I find it hard to be
> objective in this case, because of another consideration I didn't
> mention: each moment I have been spending on this is an exercise in
> frustration.

Well, don't :) Just ask yourself this question: is the patch good
enough? If not, send your own version.

Personally I think the latest version of the patch clearly explains
what it is doing, and why. It even has comments on the code, which the
other alternative doesn't.

-- 
Felipe Contreras

Re: [PATCH v3 (for maint)] git-completion: fix zsh support

[Jonathan Nieder]

Hi,

Sorry I missed this message before.

Felipe Contreras wrote:
> On Tue, May 10, 2011 at 12:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>        zsh's bash completion emulation layer does not sufficiently
>>        insulate us from that reality.  In particular, the variable
>>        keeps the "special" attribute (even after a declaration "local
>>        words"), so assignments within a function are undone whenever
>>        the function returns.
>
> That explains less.

I believe you.  Could you give a hint of what it misses, so that it
can be fixed?

What I was trying to say is that (in the previous paragraph) it turns
out 'words' is a special variable used by zsh completion and (in the
quoted text)

 - we would prefer to use that variable for something else

 - zsh's bashcompinit library can and should help us to do so

 - it would do so by using typeset -h to hide the variable, an
   important effect of which is to clear the attribute called
   "special"

 - even declaring "local words" does not clear that attribute

 - the pertinent effect here is that (in the words of the zshcompwid
   manpage) as long as parameters like words remain special, "except
   for compstate, 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".

>>        In particular, until 3bee6a473 (completion: don't declare
>>        'local words' to make zsh happy, 2011-04-28), the "words" array
>>        would be cleared in _git by declaring "local words" and its new
>>        value would never be propagated from _get_comp_words_by_ref so
>>        it remained empty and the completion script could not tell that
>>        there were existing subcommand names on the command line (so
>>        "git log m<TAB>" would complete subcommand names).
>
> I don't see the point in explaining in excruciating detail all the
> series of steps in which an unset variable causes problems; the
> variable doesn't get set, thus one can assume there are problems.

Am I daft?  I guess so.  I really do have sympathy for the person who
runs into this code, and wanting to check while making some change
with unrelated purpose that it is still fixed, fires up zsh and runs

	git a<TAB>

.  Does it complete?  Yep, check, moving on.  Oops.

There's some flamebait in the rest of your responses, so I'm snipping
the rest of my explanations.  If you have sincere questions about my
feedback in the future, I'd be glad to answer them.

[PATCH v5 0/2] git-completion: fix zsh support

[Jonathan Nieder]

Hi again,

As promised, here are two patches.  No doubt I have done all sorts
of horrible things to ruin them.  Sorry about that; thoughts,
improvements, bug reports welcome.

Felipe Contreras (1):
  completion: suppress zsh's special 'words' variable

Jonathan Nieder (1):
  completion: move private shopt shim for zsh to __git_ namespace

 contrib/completion/git-completion.bash |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

[PATCH 1/2] completion: suppress zsh's special 'words' variable

[Jonathan Nieder]

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

After git's tab completion script gained zsh support in
v1.7.4-rc0~169^2 (completion: make compatible with zsh, 2010-09-06)
it was broken moments later.  More precisely, the completion does not
notice when it has seen a subcommand name, so all words complete as
options to the git wrapper or subcommand names.  For example, typing
"git log origi<TAB>" gives no completions because there are no "git
origi..." commands.

The cause: it turns out 'words' is one of the special parameters used
by the zsh completion system, used to hold the words from the command
it is completing.  As a result (in the words of 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.

Each function in git's completion script using the 'words' array

 - declares "local words", causing the array to be cleared (but not
   resetting the special attribute);

 - calls "_get_comp_words_by_ref -n := words" to fill it with a
   modified version of COMP_WORDS with ':' and '=' no longer treated
   as word separators (see v1.7.4-rc0~11^2~2, 2010-12-02).  Within
   _get_comp_words_by_ref all is well, and when the function returns,
   words is reset to its former value;

 - examines $words and finds it empty.

Fix it by suppressing the special 'words' variable with typeset -h
so it can be used as an ordinary array.  The only risk is that the
completion script might call a function that wants to inspect the
'words' variable, expecting the zsh-specific meaning; luckily the next
version of zsh's bashcompinit (e880604f, 29140: hide the "words"
special variable so that it may be used as an ordinary variable by
bash completions, 2011-05-04) will also use 'typeset -h words' when
calling completion functions so

 - soon this fix will be redundant :)
 - anyone else using the bashcompinit library is risking the same
   problem, so presumably other functions from that library are
   carefully written to only look at $COMP_WORDS and not $words.

This fixes a regression introduced by v1.7.4-rc0~11^2~2 (2010-12-02).

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b81f444..da586e5 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2608,6 +2608,7 @@ _git ()
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
 		setopt KSH_TYPESET
+		typeset -h words
 	fi
 
 	local cur words cword prev
@@ -2659,6 +2660,7 @@ _gitk ()
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
 		setopt KSH_TYPESET
+		typeset -h words
 	fi
 
 	local cur words cword prev
-- 
1.7.5.1

[PATCH 2/2] completion: move private shopt shim for zsh to __git_ namespace

[Jonathan Nieder]

Most zsh users probably probably do not expect a custom shopt function
to enter their environment just because they ran "source
~/.git-completion.sh".

Such namespace pollution makes development of other scripts confusing
(because it makes the bash-specific shopt utility seem to be available
in zsh) and makes git's tab completion script brittle (since any other
shell snippet implementing some other subset of shopt will break it).
Rename the shopt shim to the more innocuous __git_shopt to be a good
citizen (with two underscores to avoid confusion with completion rules
for a hypothetical "git shopt" command).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: SZEDER Gábor <szeder@ira.uka.de>
---
Thanks again.

 contrib/completion/git-completion.bash |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index da586e5..a236234 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -628,12 +628,12 @@ __git_refs_remotes ()
 __git_remotes ()
 {
 	local i ngoff IFS=$'\n' d="$(__gitdir)"
-	shopt -q nullglob || ngoff=1
-	shopt -s nullglob
+	__git_shopt -q nullglob || ngoff=1
+	__git_shopt -s nullglob
 	for i in "$d/remotes"/*; do
 		echo ${i#$d/remotes/}
 	done
-	[ "$ngoff" ] && shopt -u nullglob
+	[ "$ngoff" ] && __git_shopt -u nullglob
 	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
 		i="${i#remote.}"
 		echo "${i/.url*/}"
@@ -2701,7 +2701,7 @@ complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
 fi
 
 if [[ -n ${ZSH_VERSION-} ]]; then
-	shopt () {
+	__git_shopt () {
 		local option
 		if [ $# -ne 2 ]; then
 			echo "USAGE: $0 (-q|-s|-u) <option>" >&2
@@ -2724,4 +2724,8 @@ if [[ -n ${ZSH_VERSION-} ]]; then
 			return 1
 		esac
 	}
+else
+	__git_shopt () {
+		shopt "$@"
+	}
 fi
-- 
1.7.5.1

Re: [PATCH 1/2] completion: suppress zsh's special 'words' variable

[Jonathan Nieder]

Jonathan Nieder wrote:

>  contrib/completion/git-completion.bash |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

I forgot to list changes since v4:

 - new commit message
 - removed comment I considered unclear --- probably the following
   should be squashed in, though, to prevent someone from seeing the
   seemingly redundant "typeset -h words" and removing it, not
   realizing it is needed for compatibility with old zsh versions.

-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Subject: completion: add a comment to explain what "typeset -h words" is for

In current zsh master, bashcompinit hides the 'words' special variable
already, so our own "typeset -h words" in _git and _gitk might seem
redundant.  Add a comment to explain that it is there for
compatibility with old zsh versions.

[jn: based on the original comment by Felipe Contreras]

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 contrib/completion/git-completion.bash |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index da586e5..b80830e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2608,6 +2608,10 @@ _git ()
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
 		setopt KSH_TYPESET
+
+		# Suppress the "words" special variable from zsh.
+		# Only zsh versions <= 4.3.11 need this --- in later
+		# versions, bashcompinit takes care of it.
 		typeset -h words
 	fi
 
@@ -2660,6 +2664,10 @@ _gitk ()
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
 		setopt KSH_TYPESET
+
+		# Suppress the "words" special variable from zsh.
+		# Only zsh versions <= 4.3.11 need this --- in later
+		# versions, bashcompinit takes care of it.
 		typeset -h words
 	fi
 
-- 
1.7.5.1

Re: [PATCH v3 (for maint)] git-completion: fix zsh support

[Felipe Contreras]

On Tue, May 10, 2011 at 5:04 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Tue, May 10, 2011 at 12:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>>        zsh's bash completion emulation layer does not sufficiently
>>>        insulate us from that reality.  In particular, the variable
>>>        keeps the "special" attribute (even after a declaration "local
>>>        words"), so assignments within a function are undone whenever
>>>        the function returns.
>>
>> That explains less.
>
> I believe you.  Could you give a hint of what it misses, so that it
> can be fixed?

This is what I explained:
---
so by trying to set it as 'local' in git's completion, unexpected
results occur; assignations are not propagated to outer levels in the
call stack.
---

My text explains why Gabor's patch works: it doesn't set 'words' as
local. Your text doesn't explain that; it generalizes that 'words'
always behave that way, which is not the case. Your text also suggests
that the assignments are always undone, which is not the case, as I
explained; it only happens to the outer levels in the call stack.

>>>        In particular, until 3bee6a473 (completion: don't declare
>>>        'local words' to make zsh happy, 2011-04-28), the "words" array
>>>        would be cleared in _git by declaring "local words" and its new
>>>        value would never be propagated from _get_comp_words_by_ref so
>>>        it remained empty and the completion script could not tell that
>>>        there were existing subcommand names on the command line (so
>>>        "git log m<TAB>" would complete subcommand names).
>>
>> I don't see the point in explaining in excruciating detail all the
>> series of steps in which an unset variable causes problems; the
>> variable doesn't get set, thus one can assume there are problems.
>
> Am I daft?  I guess so.  I really do have sympathy for the person who
> runs into this code, and wanting to check while making some change
> with unrelated purpose that it is still fixed, fires up zsh and runs
>
>        git a<TAB>
>
> .  Does it complete?  Yep, check, moving on.  Oops.

I didn't notice that. That can be easily explained with one line:

---
the completion script could not tell that there were existing
subcommand names on the command line (so "git log m<TAB>" would
complete subcommand names).
---

-- 
Felipe Contreras

Re: [PATCH v5 0/2] git-completion: fix zsh support

[Felipe Contreras]

On Tue, May 10, 2011 at 5:55 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> As promised, here are two patches.  No doubt I have done all sorts
> of horrible things to ruin them.  Sorry about that; thoughts,
> improvements, bug reports welcome.
>
> Felipe Contreras (1):
>  completion: suppress zsh's special 'words' variable
>
> Jonathan Nieder (1):
>  completion: move private shopt shim for zsh to __git_ namespace

These two patches are not related. I don't see why they are on the
same patch set. I thought the idea was to have two patch sets:

* Important fix:
completion: suppress zsh's special 'words' variable

* Cleanups and improvements:
completion: don't modify the $cur variable in completion functions
completion: remove unnecessary _get_comp_words_by_ref() invocations
completion: move private shopt shim for zsh to __git_ namespace

-- 
Felipe Contreras

Re: [PATCH 1/2] completion: suppress zsh's special 'words' variable

[Felipe Contreras]

On Tue, May 10, 2011 at 5:59 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>

The summary:
"completion: suppress zsh's special 'words' variable"

Does not explain what is the effect to the end-user. If some months
for now somebody starts looking for the fix to this issue, the summary
would not help, and the commit would be easily missed.

> After git's tab completion script gained zsh support in
> v1.7.4-rc0~169^2 (completion: make compatible with zsh, 2010-09-06)
> it was broken moments later.

You are missing a comma there. And to me "after foo, it got" sounds
more natural.

> More precisely, the completion does not
> notice when it has seen a subcommand name, so all words complete as
> options to the git wrapper or subcommand names.  For example, typing
> "git log origi<TAB>" gives no completions because there are no "git
> origi..." commands.
>
> The cause: it turns out 'words' is one of the special parameters used
> by the zsh completion system, used to hold the words from the command
> it is completing.  As a result (in the words of 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.

Interesting. I hadn't seen this behavior documented before.

> Each function in git's completion script using the 'words' array
>
>  - declares "local words", causing the array to be cleared (but not
>   resetting the special attribute);
>
>  - calls "_get_comp_words_by_ref -n := words" to fill it with a
>   modified version of COMP_WORDS with ':' and '=' no longer treated
>   as word separators (see v1.7.4-rc0~11^2~2, 2010-12-02).  Within
>   _get_comp_words_by_ref all is well, and when the function returns,
>   words is reset to its former value;
>
>  - examines $words and finds it empty.

I don't see the point of explaining these 3 points. This is explaining
how the fix was found, which is rarely useful. If you really want to
explain each step from the fix to it's effect, I say that should be at
the end of the commit message.

> Fix it by suppressing the special 'words' variable with typeset -h
> so it can be used as an ordinary array.

The 'word' variable is not "suppressed"; it's replaced locally with an
ordinary variable.

> The only risk is that the
> completion script might call a function that wants to inspect the
> 'words' variable,

There are no such functions. AFAIK the only function used by bash
completion is 'compgen' and in no part of it does the zsh emulation
make use of the 'words' special variable.

> expecting the zsh-specific meaning;

> luckily the next
> version of zsh's bashcompinit (e880604f, 29140: hide the "words"
> special variable so that it may be used as an ordinary variable by
> bash completions, 2011-05-04) will also use 'typeset -h words' when
> calling completion functions so

This looks like it should be a separate paragraph, specially since you
are referring to this text next:

>  - soon this fix will be redundant :)

This is important, and can be explained with one word in the summary:
workaround.

>  - anyone else using the bashcompinit library is risking the same
>   problem, so presumably other functions from that library are
>   carefully written to only look at $COMP_WORDS and not $words.

There is no problem, you have always assumed so, you are the only one.

Write a test that shows the problem.

> This fixes a regression introduced by v1.7.4-rc0~11^2~2 (2010-12-02).

This is the important part, you should start with that.

> Reported-by: Stefan Haller <lists@haller-berlin.de>
> Improved-by: SZEDER Gábor <szeder@ira.uka.de>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

I have not s-o-b'ed this.

Also, mention that you wrote the changelog:

[rewrote changelog]
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

-- 
Felipe Contreras

Re: [PATCH 1/2] completion: suppress zsh's special 'words' variable

[Felipe Contreras]

On Tue, May 10, 2011 at 6:17 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>
>>  contrib/completion/git-completion.bash |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> I forgot to list changes since v4:
>
>  - new commit message
>  - removed comment I considered unclear --- probably the following
>   should be squashed in, though, to prevent someone from seeing the
>   seemingly redundant "typeset -h words" and removing it, not
>   realizing it is needed for compatibility with old zsh versions.
>
> -- >8 --
> From: Felipe Contreras <felipe.contreras@gmail.com>
> Subject: completion: add a comment to explain what "typeset -h words" is for
>
> In current zsh master, bashcompinit hides the 'words' special variable
> already, so our own "typeset -h words" in _git and _gitk might seem
> redundant.  Add a comment to explain that it is there for
> compatibility with old zsh versions.
>
> [jn: based on the original comment by Felipe Contreras]
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  contrib/completion/git-completion.bash |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index da586e5..b80830e 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2608,6 +2608,10 @@ _git ()
>        if [[ -n ${ZSH_VERSION-} ]]; then
>                emulate -L bash
>                setopt KSH_TYPESET
> +
> +               # Suppress the "words" special variable from zsh.

That is not accurate. This is hiding the special meaning of the
'words' variable, only locally. "Suppress" denotes that the special
meaning of 'words' is gone forever, even on outer layers of the call
stack.

> +               # Only zsh versions <= 4.3.11 need this --- in later
> +               # versions, bashcompinit takes care of it.

That can be explained in shorter form:

# Workaround for versions < 4.3.12

The word workaround denotes that there's a bug, and by limiting where
the workaround is needed, it's obvious where proper fix is.

All this can be explained in this sentence:

# workaround zsh's bug which leaves 'words' as a special variable in
versions < 4.3.12

What is the need to change that?

-- 
Felipe Contreras