From: David Aguilar <hidden> Date: 2021-06-01 16:53:00
A recent change to make git-completion.bash use $__git_cmd_idx
in more places broke a number of completions on zsh because it
modified __git_main but did not update __git_zsh_main.
Notably, completions for "add", "branch", "mv" and "push" were
broken as a result of this change.
In addition to the undefined variable usage, "git mv <tab>" also
prints the following error:
__git_count_arguments:7: bad math expression:
operand expected at `"1"'
_git_mv:[:7: unknown condition: -gt
Remove the quotes around $__git_cmd_idx in __git_count_arguments
and set __git_cmd_idx=1 early in __git_zsh_main to fix the
regressions from 59d85a2a05.
Add "git" to the "words" array in _git_zsh_main to guarantee
that "git" is at least always in the completion list. It may not
be completely correct because __git_cmd_idx is not always 1,
but it is enough to fix the regression.
This was tested on zsh 5.7.1 (x86_64-apple-darwin19.0).
Helped-by: Felipe Contreras [off-list ref]
Signed-off-by: David Aguilar <redacted>
---
contrib/completion/git-completion.bash | 2 +-
contrib/completion/git-completion.zsh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
From: Felipe Contreras <hidden> Date: 2021-06-01 19:15:06
David Aguilar wrote:
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.
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?
From: David Aguilar <hidden> Date: 2021-06-01 21:00:34
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.
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
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 would be an improvement, but it's orthogonal to the regression you
are trying to fix.
I would just fix the regression for v2.32, and then afterwards try to do
the improvement.
I have a testing framework for the zsh completion in my git-completion
project, so I would be much more confident about this change if all the
tests pass. Alas I have not yet merged any v2.32.0-rc* so it's not
straightforward to run the tests now.
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?
I mean there's git options, and git command options. I don't know how
many changes are needed to make all the interactions work correctly, but
I wouldn't have confidence in any of them so close to a release,
especially considering git.git doesn't have any zsh tests.
So yes, v4 is fine.
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.
That will work correctly on git-completion once I merge your v4 patch
(and v2.32).
--
Felipe Contreras