On Tue, May 10, 2011 at 12:13 AM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html