Re: [PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh
Junio C Hamano writes: > Rick van Hattem 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. > >> # Clear the variables caching builtins' options when (re-)sourcing >> # the completion script. >> -if [[ -n ${ZSH_VERSION-} ]]; then >> +if [[ -n ${ZSH_NAME-} ]]; then > > ... > ZSH_VERSION='' . "$script" > ... > > 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)? Ah, I was totally mis-reading the script (and partly was confused by your use of "unset"). ZSH_VERSION is reset to an empty string, which breaks the check later done in the bash completion script.
Re: [PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh
Hi, Rick van Hattem wrote: > On 4 June 2018 at 05:40, Junio C Hamano wrote: >> 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 > > 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. >> >> The proposed fix is quite simple and shouldn't break any backwards >> compatibility. > > Hopefully that clears a little bit of the confusion. Thanks. That helps a little, but it's missing a crucial piece: may we forge your sign-off? See Documentation/SubmittingPatches section [[sign-off]] ("Certify your work") for more details about what this means. Thanks, Jonathan
Re: [PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh
On 6 June 2018 at 13:41, SZEDER Gábor wrote: > >> On 4 June 2018 at 05:40, Junio C Hamano wrote: >> Rick van Hattem 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
Re: [PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh
> On 4 June 2018 at 05:40, Junio C Hamano wrote: > Rick van Hattem 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 e
Re: [PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh
On 4 June 2018 at 05:40, Junio C Hamano wrote: Rick van Hattem 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 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. > > 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. > 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" ... 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 ... ~rick On 4 June 2018 at 05:40, Junio C Hamano wrote: > Rick van Hattem 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. > >> # 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. > > I think what the proposed log message refers to as "unsets" is this > part of the script: > > ... > zstyle -s ":completion:*:*:git:*" script script > if [ -z "$script" ]; then > local -a locations > local e > locations=( > $(dirname > ${funcsourcetrace[1]%:*})/git-completion.bash > ... > ) > for e in $locations; do > test -f $e && script="$e" && break > done > fi > ZSH_VERSION='' . "$script" > ... > > 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, wh
Re: [PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh
Rick van Hattem 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. > # 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. I think what the proposed log message refers to as "unsets" is this part of the script: ... zstyle -s ":completion:*:*:git:*" script script if [ -z "$script" ]; then local -a locations local e locations=( $(dirname ${funcsourcetrace[1]%:*})/git-completion.bash ... ) for e in $locations; do test -f $e && script="$e" && break done fi ZSH_VERSION='' . "$script" ... 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...
[PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh
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. --- contrib/completion/git-completion.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 12814e9bbf6be..7e2b9ad454930 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -348,7 +348,7 @@ __gitcomp () # Clear the variables caching builtins' options when (re-)sourcing # the completion script. -if [[ -n ${ZSH_VERSION-} ]]; then +if [[ -n ${ZSH_NAME-} ]]; 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_) -- https://github.com/git/git/pull/500