Re: [PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh
Show 13 quoted lines
> 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.
Show 14 quoted lines
> 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.
Show 41 quoted lines
> > 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.
Show 14 quoted lines
> > 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.
Show 8 quoted lines
> 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-} ]]; thenLooking 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.
Rick van Hattem
