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

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

[Rick van Hattem]

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.
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 12814e9bbf6be..7e2b9ad454930 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -348,7 +348,7 @@ __gitcomp ()
 
 # Clear the variables caching builtins' options when (re-)sourcing
 # the completion script.
-if [[ -n ${ZSH_VERSION-} ]]; then
+if [[ -n ${ZSH_NAME-} ]]; 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_)

--
https://github.com/git/git/pull/500

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

[Junio C Hamano]

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.

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

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

[Rick van Hattem]

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

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

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

[SZEDER Gábor]

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

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

[Rick van Hattem]

 On 6 June 2018 at 13:41, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
>> 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

Ah, now I see the problem. That was unintentional, I created this pull
request through the Github interface where wrapping is auto enabled
which masked the issue for me.

That's what I get for trying to use a webinterface instead of doing
this commandline... mea culpa.


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

I agree, segfaulting is unacceptable behaviour.

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

Yes, it definitely does feel hackish but since this has been the case
for a long time I worry about breaking backwards compatibility with
peoples shell configs by changing the behaviour now.

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

I agree, that would be a better solution.

For the time being I would opt for either reverting 94408dc or
implementing this commit though.

~rick

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

[Jonathan Nieder]

Hi,

Rick van Hattem wrote:
> On 4 June 2018 at 05:40, Junio C Hamano <gitster@pobox.com> wrote:

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

Thanks.  That helps a little, but it's missing a crucial piece: may we
forge your sign-off?  See Documentation/SubmittingPatches section
[[sign-off]] ("Certify your work") for more details about what this
means.

Thanks,
Jonathan

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

Ah, I was totally mis-reading the script (and partly was confused by
your use of "unset").  ZSH_VERSION is reset to an empty string,
which breaks the check later done in the bash completion script.