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