Re: [PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh

["SZEDER Gábor" <szeder.dev@gmail.com>]

> On 4 June 2018 at 05:40, Junio C Hamano <gitster@pobox.com> wrote:
> Rick van Hattem <wolph@wol.ph> writes:
> 
> > > The `git-completion.zsh` unsets the `$ZSH_VERSION` which makes this check moot. The result (at least for me) is that zsh segfaults because of all the variables it's unsetting.
> > > ---
> >
> > Overlong line, lack of sign-off.
> 
> Apologies for the long lines, I wrote the message on Github where this
> message is properly formatted, apparently the submitgit script can be
> considered broken as it truncates the message when converting to email.
> 
> The original message can be found here: https://github.com/git/git/pull/500

That link points to the pull request.  The important thing is the
actual commit message, which can be found here:

  https://github.com/git/git/pull/500/commits/b740bc3fedf419c7ee12364279cad84e1f2f7bb7

So submitgit neither truncated the commit message nor changed its
formatting.

> Below is the original message with proper formatting:
> 
> > A recent change (94408dc) broke zsh for me (actually segfault zsh when
> > trying to use git completion)
> >
> > The reason is that the `git-completion.zsh` sets the `ZSH_VERSION`
> > variable to an empty string:
> >     ...
> >     ZSH_VERSION='' . "$script"
> >     ...
> >
> > I'm not sure if @szeder or @gitster used a different zsh version for
> > testing commit 94408dc but it segfaults zsh 5.5.1
> > (x86_64-apple-darwin15.6.0) on my OS X 10.11.6 machine.

I used "zsh 5.1.1 (x86_64-ubuntu-linux-gnu)", the one shipped in this
LTS of a Debian derivative's derivative, for superficial testing:
started zsh, dot-sourced 'git-completion.bash' (yes, .bash), it
appeared to be doing what I thought it should be doing, great, done.

I don't test 'git-completion.zsh': merely sourcing it doesn't seem to
work at all for me, I still get ZSH's git completion.

> > The proposed fix is quite simple and shouldn't break any backwards
> > compatibility.
> 
> Hopefully that clears a little bit of the confusion.
> 
> > >  # Clear the variables caching builtins' options when (re-)sourcing
> > >  # the completion script.
> > > -if [[ -n ${ZSH_VERSION-} ]]; then
> > > +if [[ -n ${ZSH_NAME-} ]]; then
> >
> > I am not a zsh user, and I do not know how reliable $ZSH_NAME can be
> > taken as an indication that we are running zsh and have already
> > found a usable git-completion-.bash script.
> 
> >From what I gathered this variable has been available since 1995. But
> I'm not ZSH expert...
> 
> You can search for ZSH_NAME in the 3.0 changelog:
> http://zsh.sourceforge.net/Etc/changelog-3.0.html
> 
> > I think what the proposed log message refers to as "unsets" is this
> > part of the script:
> 
> As mentioned above, I was referring to commit 94408dc which changed the
> behaviour of the bash completion script.
> 
> Specifically:
> 
>     ...
>     if [[ -n ${ZSH_VERSION-} ]]; then
>         unset $(set |sed -ne
> 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p')
> 2>/dev/null
>     else
>         unset $(compgen -v __gitcomp_builtin_)
>     fi
>     ...
> 
> Because the ZSH script unsets the ZSH_VERSION variable (which is needed
> because the bash script checks for that later in the script) it defaults
> to the bash behaviour resulting in a segfault.

I think this segfault issue should definitely be addressed in ZSH.  No
matter what foolish or downright wrong thing a script does, the shell
should not segfault.

> > If your ZSH_VERSION is empty, doesn't it indicate that the script
> > did not find a usable git-completion.bash script (to which it
> > outsources the bulk of the completion work)?  I do agree segfaulting
> > is not a friendly way to tell you that your setup is lacking to make
> > it work, but I have a feeling that what you are seeing is an
> > indication of a bigger problem, which will be sweeped under the rug
> > with this patch but without getting fixed...
> 
> The git-completion.zsh script purposefully unsets the ZSH_VERSION
> before including the git-completion.bash script like this:
> 
>     ...
>     ZSH_VERSION='' . "$script"
>     ...

Oh, I was not aware of this.  It does feel a bit hackish, doesn't it.

> The reason for that is (presumably) the check that's used within the
> git-completion.bash script to warn ZSH users:
> 
>     ...
>     if [[ -n ${ZSH_VERSION-} ]]; then
>     echo "WARNING: this script is deprecated, please see
> git-completion.zsh" 1>&2
>     ...

And, perhaps more importantly, to not load a bunch of shell functions
that follow that warning.

> >>  # Clear the variables caching builtins' options when (re-)sourcing
> >>  # the completion script.
> >> -if [[ -n ${ZSH_VERSION-} ]]; then
> >> +if [[ -n ${ZSH_NAME-} ]]; then

Looking at $ZSH_VERSION is our standard check both in the completion
and prompt scripts.  Changing only one of those checks to look at
$ZSH_NAME instead brings inconcistency and confusion.

I think it would be better to eliminate that "let's pretend it's not
ZSH" hack and make 'git-completion.zsh' more explicit by sourcing
'git-completion.bash' something like this:

  DOT_SOURCING_FROM_GIT_COMPLETION_ZSH=PleaseSkipDeprecatedFunctions . "$script"

(with a more sensible variable name, of course :), and
'git-completion.bash' should additionally check this variable as well.