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

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

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