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

[Felipe Contreras <felipe.contreras@gmail.com>]

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