From: Felipe Contreras Date: Mon, 09 May 2011 22:08:17 GMT Subject: Re: [PATCH v3 (for maint)] git-completion: fix zsh support Message-ID: In-Reply-To: <20110509211316.GB22908@elie> On Tue, May 10, 2011 at 12:13 AM, Jonathan Nieder 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" 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