Web lists-archives.com

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




On 4 June 2018 at 05:40, Junio C Hamano <gitster@xxxxxxxxx> wrote:
Rick van Hattem <wolph@xxxxxx> 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

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.
>
> 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.

> 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"
    ...

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
    ...


~rick

On 4 June 2018 at 05:40, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Rick van Hattem <wolph@xxxxxx> 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.
>
>>  # 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.
>
> I think what the proposed log message refers to as "unsets" is this
> part of the script:
>
>         ...
>         zstyle -s ":completion:*:*:git:*" script script
>         if [ -z "$script" ]; then
>                 local -a locations
>                 local e
>                 locations=(
>                         $(dirname ${funcsourcetrace[1]%:*})/git-completion.bash
>                         ...
>                         )
>                 for e in $locations; do
>                         test -f $e && script="$e" && break
>                 done
>         fi
>         ZSH_VERSION='' . "$script"
>         ...
>
> 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...