Re: [PATCH v2] completion: reduce overhead of clearing cached --options

[Rick van Hattem <wolph@wol.ph>]

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 git@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<tab>
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 <jrnieder@gmail.com> 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 <TAB>
>
> 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 <TAB>
>
> 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 <wolph@wol.ph>
> Reported-by: Dave Borowitz <dborowitz@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>
> [1] https://public-inbox.org/git/01020163c683e753-04629405-15f8-4a30-9dc3-e4e3f2a5aa26-000000@eu-west-1.amazonses.com/
> [2] https://public-inbox.org/git/20180606114147.7753-1-szeder.dev@gmail.com/
>
> diff --git i/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash
> index 12814e9bbf..e4bcc435ea 100644
> --- i/contrib/completion/git-completion.bash
> +++ w/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_VERSION-} || -n ${GIT_SOURCING_ZSH_COMPLETION-} ]]; 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_)
> diff --git i/contrib/completion/git-completion.zsh w/contrib/completion/git-completion.zsh
> index 53cb0f934f..c7be9fd4dc 100644
> --- i/contrib/completion/git-completion.zsh
> +++ w/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=1 ZSH_VERSION='' . "$script"
>
>  __gitcomp ()
>  {