On 6 June 2018 at 13:41, SZEDER Gábor <[email protected]> wrote:
>
>> On 4 June 2018 at 05:40, Junio C Hamano <[email protected]> wrote:
>> Rick van Hattem <[email protected]> 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
