Re: [PATCH v3 (for maint)] git-completion: fix zsh support
- From
Felipe Contreras <felipe.contreras@gmail.com>
- Date
- May 9, 2011, 22:08 UTC
- Message-ID
- <BANLkTikkhryMa69DSx4EAYjw+aar4icKcQ@mail.gmail.com>
- In-Reply-To
- <20110509211316.GB22908@elie>
On Tue, May 10, 2011 at 12:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 7 quoted lines
> 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.
Show 6 quoted lines
>> 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".
Show 5 quoted lines
> 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.
Show 7 quoted lines
> 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.
Show 9 quoted lines
> 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.
Show 12 quoted lines
>> 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.
Show 18 quoted lines
>> --- 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