Nit: I don't like the word "pass" in the subject line, because
you don't actually "pass" that variable as a parameter, but simly set
it, and it will be visible in all called functions, because that's how
shell variables work.
On Wed, Mar 24, 2021 at 01:36:27AM -0700, Denton Liu wrote:
> Many completion functions perform hardcoded comparisons with $cword.
> This fails in the case where the main git command is given arguments
> (e.g. `git -C . bundle<TAB>` would fail to complete its subcommands).
It's not just the hardcoded comparison with $cword, but the hardcoded
indices into the $words array that causes these problems:
> Even _git_worktree(), which uses __git_find_on_cmdline(), could still
> fail. With something like `git -C add worktree move<TAB>`, the
> subcommand would be incorrectly identified as "add" instead of "move".
>
> Assign $__git_subcommand_idx in __git_main(), where the git subcommand
In 'git -C add worktree move this there' we invoke the 'worktree'
command's 'move' subcommand. Therefore, this variable should be
called $__git_command_idx. Or perhaps $__git_cmd_idx, to spare some
keystrokes without sacrificing readability?
> is actually found and the corresponding completion function is called.
> Use this variable to replace hardcoded comparisons with $cword.
>
> Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> ---
> contrib/completion/git-completion.bash | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
This patch leaves a couple of accesses to $words and $cword unchanged,
though they still suffer from the same issues and should be changed,
e.g.:
__git_complete_remote_or_refspec() assumes that ${words[1]} is the
command and starts looking for options starting at index 2, so e.g.
'git fetch <TAB>' lists configured remotes, but 'git -C . fetch <TAB>'
doesn't.
_git_branch() is curious, because, just like the "main" 'git' command,
'git branch' has '-c' and '-C' options, and as a result 'git branch
o<TAB>' lists branches from 'origin', but 'git -c foo.bar=baz -C .
branch o<TAB>' doesn't.
It's debatable whether __git_find_on_cmdline() and its friends should
be changed. If we only look at the function's name, then no, because
it implicitly implies that it searches through the whole command line.
However, if we look at how we actually use it, then we'll find that we
only use it to check for the presence of subcommands or certain
options of a command or subcommand. This means that we only want to
search the words following the command, but since it starts its scan
at ${words[1]}, it leads to that issue with 'git worktree' that you
described in the commit message, but it affects all other commands
with subcommands as well.
I haven't looked closely at the other cases, but I'm inclinened to
think that all _git_cmd() functions and any helper functions invoked
by them should only concern themselves with words after the git
command.
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 7dc6cd8eb8..a2f1b5e916 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1474,12 +1474,12 @@ _git_branch ()
>
> _git_bundle ()
> {
> - local cmd="${words[2]}"
> + local cmd="${words[__git_subcommand_idx+1]}"
> case "$cword" in
> - 2)
> + $((__git_subcommand_idx+1)))
> __gitcomp "create list-heads verify unbundle"
> ;;
> - 3)
> + $((__git_subcommand_idx+2)))
> # looking for a file
> ;;
> *)
> @@ -1894,7 +1894,7 @@ _git_grep ()
> esac
>
> case "$cword,$prev" in
> - 2,*|*,-*)
> + $((__git_subcommand_idx+1)),*|*,-*)
> __git_complete_symbol && return
> ;;
> esac
> @@ -3058,7 +3058,7 @@ _git_stash ()
> branch,--*)
> ;;
> branch,*)
> - if [ $cword -eq 3 ]; then
> + if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
> __git_complete_refs
> else
> __gitcomp_nl "$(__git stash list \
> @@ -3277,11 +3277,9 @@ __git_complete_worktree_paths ()
> _git_worktree ()
> {
> local subcommands="add list lock move prune remove unlock"
> - local subcommand subcommand_idx
> + local subcommand
>
> - subcommand="$(__git_find_on_cmdline --show-idx "$subcommands")"
> - subcommand_idx="${subcommand% *}"
> - subcommand="${subcommand#* }"
> + subcommand="$(__git_find_on_cmdline "$subcommands")"
>
> case "$subcommand,$cur" in
> ,*)
> @@ -3306,7 +3304,7 @@ _git_worktree ()
> # be either the 'add' subcommand, the unstuck
> # argument of an option (e.g. branch for -b|-B), or
> # the path for the new worktree.
> - if [ $cword -eq $((subcommand_idx+1)) ]; then
> + if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
> # Right after the 'add' subcommand: have to
> # complete the path, so fall back to Bash
> # filename completion.
> @@ -3330,7 +3328,7 @@ _git_worktree ()
> __git_complete_worktree_paths
> ;;
> move,*)
> - if [ $cword -eq $((subcommand_idx+1)) ]; then
> + if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
> # The first parameter must be an existing working
> # tree to be moved.
> __git_complete_worktree_paths
I don't like these changes to _git_worktree(), because they implicitly
assume that 'git worktree' doesn't have any --options, and it would
then start to misbehave if we added one.
And these changes wouldn't be necessary if __git_find_on_cmdline()
started its search at $__git_cmd_idx instead of at ${words[1]}.
> @@ -3398,6 +3396,7 @@ __git_main ()
> {
> local i c=1 command __git_dir __git_repo_path
> local __git_C_args C_args_count=0
> + local __git_subcommand_idx
>
> while [ $c -lt $cword ]; do
> i="${words[c]}"
> @@ -3412,7 +3411,7 @@ __git_main ()
> __git_C_args[C_args_count++]="${words[c]}"
> ;;
> -*) ;;
> - *) command="$i"; break ;;
> + *) command="$i"; __git_subcommand_idx="$c"; break ;;
See what variable is $i assigned to? $command, not $subcommand.
> esac
> ((c++))
> done
> --
> 2.31.0.rc2.261.g7f71774620
>
