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

[Jonathan Nieder <jrnieder@gmail.com>]

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