Re: [PATCH] completion: correct zsh detection when run from git-completion.zsh (Re: [PATCH v2] completion: reduce overhead of clearing cached --options)
On 11 June 2018 at 20:20, Jonathan Nieder wrote:
> SZEDER Gábor wrote:
>
>> Being in RC phase, I'm all for aiming for a minimal solution.
>> However, I don't think that the better fix would be erm.. any "less
>> minimal":
>
> Thanks again. May we have your sign-off?
>
> contrib/completion/git-completion.bash | 5 -
> contrib/completion/git-completion.zsh | 2 +-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 12814e9bbf..f4a2e6774b 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3223,7 +3223,10 @@ __gitk_main ()
> __git_complete_revlist
> }
>
> -if [[ -n ${ZSH_VERSION-} ]]; then
> +if [[ -n ${ZSH_VERSION-} ]] &&
> + # Don't define these functions when sourced from 'git-completion.zsh',
> + # it has its own implementations.
> + [[ -z ${GIT_SOURCING_ZSH_COMPLETION-} ]]; then
> echo "WARNING: this script is deprecated, please see
> git-completion.zsh" 1>&2
>
> autoload -U +X compinit && compinit
> diff --git a/contrib/completion/git-completion.zsh
> b/contrib/completion/git-completion.zsh
> index 53cb0f934f..049d6b80f6 100644
> --- a/contrib/completion/git-completion.zsh
> +++ b/contrib/completion/git-completion.zsh
> @@ -39,7 +39,7 @@ if [ -z "$script" ]; then
> test -f $e && script="$e" && break
> done
> fi
> -ZSH_VERSION='' . "$script"
> +GIT_SOURCING_ZSH_COMPLETION=y . "$script"
>
> __gitcomp ()
> {
> --
> 2.18.0.rc1.242.g61856ae69a
The change looks good to me :)
Re: [PATCH v2] completion: reduce overhead of clearing cached --options
The zsh script expects the bash completion script to be available so
that might be the issue here.
To reproduce this is what I do. First, a sparse checkout:
# mkdir ~/git
# cd ~/git
# git init
# git remote add origin g...@github.com:git/git.git
# git config core.sparseCheckout true
# mkdir .git/info
# echo contrib/completion >> .git/info/sparse-checkout
# git pull origin master --depth 1
After that I simply link the zsh script to _git:
# cd git/contrib/completion
# ln git-completion.zsh _git
And add the following to a new .zshrc file:
autoload -U compinit; compinit
autoload -U bashcompinit; bashcompinit
fpath=(~/git/contrib/completion $fpath)
And that appears to be enough to reproduce:
# git
git/contrib/completion/git-completion.bash:354: read-only variable: QISUFFIX
zsh:12: command not found: ___main
zsh:15: _default: function definition file not found
_dispatch:70: bad math expression: operand expected at `/usr/bin/g...'
zsh: segmentation fault zsh
~rick
On 7 June 2018 at 07:48, Jonathan Nieder wrote:
> Hi,
>
> SZEDER Gábor wrote:
>
>> Other Bash versions, notably 4.4.19 on macOS via homebrew (i.e. a
>> newer version on the same platform) and 3.2.25 on CentOS (i.e. a
>> slightly earlier version, though on a different platform) are not
>> affected. ZSH in macOS (the versions shipped by default or installed
>> via homebrew) or on other platforms isn't affected either.
> [...]
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -282,7 +282,11 @@ __gitcomp ()
>>
>> # Clear the variables caching builtins' options when (re-)sourcing
>> # the completion script.
>> -unset $(set |sed -ne
>> 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null
>> +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
>
> As discussed at [1], this fails catastrophically when sourced by
> git-completion.zsh, which explicitly clears ZSH_VERSION. By
> catastrophically, I mean that Rick van Hattem and Dave Borowitz report
> that it segfaults.
>
> I can't reproduce it since I am having trouble following the
> instructions at the top of git-completion.zsh to get the recommended
> zsh completion script loading mechanism working at all. (I've
> succeeded in using git's bash completion on zsh before, but that was
> many years ago.) First attempt:
>
> 1. rm -fr ~/.zsh ~/.zshrc
> # you can move them out of the way instead for a less destructive
> # reproduction recipe
> 2. zsh
> 3. hit "q"
> 4. zstyle ':completion:*:*:git:*' script \
> $(pwd)/contrib/completion/git-completion.zsh
> 5. git checkout
>
> Expected result: segfault
>
> Actual result: succeeds, using zsh's standard completion script (not
> git's).
>
> I also tried
>
> 1. rm -fr ~/.zsh ~/.zshrc
> # you can move them out of the way instead for a less destructive
> # reproduction recipe
> 2. zsh
> 3. hit "2"
> 4. mkdir ~/.zsh; cp contrib/completion/git-completion.zsh ~/.zsh/_git
> 5. echo 'fpath=(~/.zsh $fpath)' >>~/.zshrc
> 6. ^D; zsh
> 7. git checkout
>
> and a similar process with fpath earlier in the zshrc file. Whatever
> I try, I end up with one of two results: either it uses zsh's standard
> completion, or it spews a stream of errors about missing functions
> like compgen. What am I doing wrong?
>
> Related question: what would it take to add a zsh completion sanity
> check to t/t9902-completion.sh?
>
> When running with "set -x", here is what Dave Borowitz gets before the
> segfault. I don't have a stacktrace because, as mentioned above, I
> haven't been able to reproduce it.
>
> str=git
> [[ -n git ]]
> service=git
> i=
> _compskip=default
> eval ''
> ret=0
> [[ default = *patterns* ]]
> [[ default = all ]]
> str=/usr/bin/git
> [[ -n /usr/bin/git ]]
> service=_dispatch:70: bad math expression: operand expected at
> `/usr/bin/g...'
>
> zsh: segmentation fault zsh
>
> Here's a minimal fix, untested. As a followup, as Gábor suggests at [2],
> it would presumably make sense to stop overriding ZSH_VERSION, using
> this GIT_ scoped variable everywhere instead.
>
> Thoughts?
>
> Reported-by: Rick van Hattem
> Reported-by: Dave Borowitz
> Signed-off-by: Jonathan Nieder
>
> [1]
> https://public-inbox.org/git/01020163c683e753-04629405-15f8-4a30-9dc3-e4e3f2a5aa26-000...@eu-west-1.amazonses.com/
> [
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
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&qu
[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
