Bug in completion for git merge

Bug in completion for git merge

[Thomas Becker]

There seems to be a bug in the completion for git merge; the completion function uses the following statement to both declare and initialize an array, which from what I can tell is not allowed:

local -a git_commit_opts=(--all --not HEAD --not)

I traced the addition of this to the following patch:

http://www.zsh.org/mla/workers//2015/msg02947.html

The code as written results in the following when invoking tab completion on git merge:

arda:~/.dotfiles $ git merge
_git-merge:3: number expected
_git-merge:3: number expected
_git-merge:3: number expected

Re: Bug in completion for git merge

[Bart Schaefer]

On Feb 15, 10:41am, Thomas Becker wrote:
}
} There seems to be a bug in the completion for git merge; the
} completion function uses the following statement to both declare and
} initialize an array, which from what I can tell is not allowed:
}
} local -a git_commit_opts=(--all --not HEAD --not)

This syntax is correct in zsh 5.1 and later.  Completion functions are
generally not backward-portable to older versions of the shell.

Re: Bug in completion for git merge

[Thomas Becker]

Ahh sorry about that.  That explains some of the other weirdness I’ve been seeing. I’m trying to upgrade my completion to something that knows about newer git options than what is provided in 5.0.5. Is there any way to do that? Thanks.

> On Feb 15, 2016, at 11:58 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> On Feb 15, 10:41am, Thomas Becker wrote:
> }
> } There seems to be a bug in the completion for git merge; the
> } completion function uses the following statement to both declare and
> } initialize an array, which from what I can tell is not allowed:
> }
> } local -a git_commit_opts=(--all --not HEAD --not)
> 
> This syntax is correct in zsh 5.1 and later.  Completion functions are
> generally not backward-portable to older versions of the shell.

Re: Bug in completion for git merge

[Eric Cook]

On 02/15/2016 12:20 PM, Thomas Becker wrote:
> Ahh sorry about that.  That explains some of the other weirdness I’ve been seeing. I’m trying to upgrade my completion to something that knows about newer git options than what is provided in 5.0.5. Is there any way to do that? Thanks.
> 
By patching the completer to use portable syntax, portable in terms of zsh.
The following patch applied to master should work in pre 5.1.

Considering that this is the only line in _git that expects the reserved word behavior,
it may be a good idea to commit it for consistency sake. But bart has a point.

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index b7eaf2e..fb10fa6 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -1129,9 +1129,9 @@ _git-log () {

 (( $+functions[_git-merge] )) ||
 _git-merge () {
-  local -a merge_options
+  local -a merge_options git_commit_opts
   __git_setup_merge_options
-  local -a git_commit_opts=(--all --not HEAD --not)
+  git_commit_opts=(--all --not HEAD --not)

   _arguments -w -S -s \
     $merge_options \

Re: Bug in completion for git merge

[Daniel Shahaf]

Thomas Becker wrote on Mon, Feb 15, 2016 at 12:20:33 -0500:
> Ahh sorry about that.  That explains some of the other weirdness I’ve
> been seeing. I’m trying to upgrade my completion to something that
> knows about newer git options than what is provided in 5.0.5. Is there
> any way to do that? Thanks.

Install the latest binary, not just the latest $fpath.

    git clone git://git.code.sf.net/p/zsh/code zsh
    cd zsh
    git checkout zsh-5.2
    Util/preconfig && ./configure && make all check install

Re: Bug in completion for git merge

[Daniel Shahaf]

Eric Cook wrote on Mon, Feb 15, 2016 at 22:02:24 -0500:
> On 02/15/2016 12:20 PM, Thomas Becker wrote:
> > Ahh sorry about that.  That explains some of the other weirdness I’ve been seeing. I’m trying to upgrade my completion to something that knows about newer git options than what is provided in 5.0.5. Is there any way to do that? Thanks.
> > 
> By patching the completer to use portable syntax, portable in terms of zsh.
> The following patch applied to master should work in pre 5.1.
> 
> Considering that this is the only line in _git that expects the reserved word behavior,
> it may be a good idea to commit it for consistency sake. But bart has a point.

I added the backwards-incompatible line.  I believe the decision to
use a backwards-incompatible syntax was a conscious one: either because
the issue had been discussed on-list, or because there had been precedent.

I don't have a strong opinion regarding the specific patch.  It's
obviously correct.

However, what should we do with the other instances of the
backwards-incompatible syntax that are currently in Completion/ and
Functions/?

If we want to make $fpath backwards-compatible, we'd need to change all
of them and stop adding new instances of backwards-incompatible
syntaxes... which effectively means we'd stop eating our own dogfood.

Perhaps a 'backwards-compatible $fpath' should live on a separate git
branch, so users of master keep dogfooding.

Cheers,

Daniel

> diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
> index b7eaf2e..fb10fa6 100644
> --- a/Completion/Unix/Command/_git
> +++ b/Completion/Unix/Command/_git
> @@ -1129,9 +1129,9 @@ _git-log () {
> 
>  (( $+functions[_git-merge] )) ||
>  _git-merge () {
> -  local -a merge_options
> +  local -a merge_options git_commit_opts
>    __git_setup_merge_options
> -  local -a git_commit_opts=(--all --not HEAD --not)
> +  git_commit_opts=(--all --not HEAD --not)
> 
>    _arguments -w -S -s \
>      $merge_options \
> 

Re: Bug in completion for git merge

[Eric Cook]

On 02/16/2016 05:49 AM, Daniel Shahaf wrote:

> I don't have a strong opinion regarding the specific patch.  It's
> obviously correct.
> 
> However, what should we do with the other instances of the
> backwards-incompatible syntax that are currently in Completion/ and
> Functions/?
> 
> If we want to make $fpath backwards-compatible, we'd need to change all
> of them and stop adding new instances of backwards-incompatible
> syntaxes... which effectively means we'd stop eating our own dogfood.

I wasn't trying to imply that fpath should be backwards compatible indefinitely
or that using completers from master with older shells is the smartest of ideas*.


Just that in this particular instance, since the completer has already long used
typeset in the old way; that reverting this one would make typeset's usage consistent
throughout this sole function.

* Even i've picked individual completers from master that i knew fixed bugs like
  users/20219, discarding them once i have a newer release on that machine.