git-completion: fix zsh support

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

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

index 840ae38..763f145 100755
@@ -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

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

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.

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?

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

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.

I'll change that to "outer levels".

That explains less.

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.

See the title: fix zsh support

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.

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.

It does.

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

I don't see any risk.

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

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/

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

index 840ae38..763f145 100755
@@ -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

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.

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

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.

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

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

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

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

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.

How exactly?

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

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.

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.

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?

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

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

From: Felipe Contreras <[email protected]>

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

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

index b81f444..da586e5 100755
@@ -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

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 <[email protected]>
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]

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

index da586e5..b80830e 100755
@@ -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

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.

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

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.

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

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

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.

The 'word' variable is not "suppressed"; it's replaced locally with an ordinary 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.

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

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

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

Write a test that shows the problem.

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

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

Also, mention that you wrote the changelog:

[rewrote changelog]

Felipe Contreras

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

---
Thanks again.

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

index da586e5..a236234 100755
@@ -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

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

Hi,

Sorry I missed this message before.

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

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.

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.

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