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