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

[Rick van Hattem <wolph@wol.ph>]

 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