Re: [PATCH v3] contrib/completion: fix zsh completion regression from 59d85a2a05

On Tue, Jun 1, 2021 at 12:15 PM Felipe Contreras
[off-list ref] wrote:

David Aguilar wrote:

quoted

A recent change to make git-completion.bash use $__git_cmd_idx
Add "git" to the "words" array in _git_zsh_main to guarantee
that "git" is at least always in the completion list.

Hm, no. The current code already guarantees "git" is always at the start
of the completion list. In [1] I suggested to add git *if* $words is
used instead of $orig_words.

If you add "git" to $orig_words you end up with something like
"git git mv", so the __git_cmd_idx is definitely not 1.

You should probably try to test yourself:

  words=( git ${words[@]} )
  echo "$words" >> /tmp/words-log.txt

The problem is that zsh's _arguments eats all the words it finds, so for
example if you type:

  git mv --force <tab>

$words will be 'mv --force'.

It's better to use $words because in case there's arguments beforehand,
like:

  git --git-dir=/tmp/test/.git mv --force

$words will be 'mv --force', so we can get the proper index by just
adding 'git' beforehand.

But it doesn't work for arguments not in _arguments, like:

  git --foo mv --force

Which returns:

  --foo mv --force

And unfortunately upstream's version of the wrapper doesn't understand
many arguments, like -c, or -C. git-completion does have all of them

It's better to just leave the code as it is and just fix the regression
by adding __git_cmd_idx=1.

quoted

Helped-by: Felipe Contreras [off-list ref]

I mean I kind of wrote 2 of the 3 lines you sent, can I get a
Suggested-by?

The v4 I just sent is now basically the same as v2 modulo the
Suggested-by: trailer update.

quoted

--- a/contrib/completion/git-completion.zsh
+++ b/contrib/completion/git-completion.zsh

@@ -251,7 +251,7 @@ __git_zsh_main ()
              done
              ;;
      (arg)
-             local command="${words[1]}" __git_dir
+             local command="${words[1]}" __git_dir __git_cmd_idx=1

This is needed.

quoted

              if (( $+opt_args[--bare] )); then
                      __git_dir='.'

@@ -261,7 +261,7 @@ __git_zsh_main ()

              (( $+opt_args[--help] )) && command='help'

-             words=( ${orig_words[@]} )
+             words=( git ${orig_words[@]} )

This is wrong. The current code is fine.

Cheers.

[1] https://lore.kernel.org/git/60b3c2d7557bd_be762089a@natae.notmuch/ (local)

Thanks for the detailed explanation.

Just so I'm understanding this correctly.. if this was instead..

    words=( git ${words[@]} )

(instead of orig_words like I mistakenly included in v3) would that be
an improvement, no-op or would it be worse? It sounds like additional
changes are needed to make it properly support options between "git"
and the sub-command name, hence the patch is fine as-is in v4,
correct?

Hopefully in the future it can be extended to cover eg. "git -c
foo.bar -C some-dir <sub-command>" as well. Thanks for your patience.

cheers,
-- 
David