Re: [PATCH v2 1/3] git-completion.bash: pass $__git_subcommand_idx from __git_main()

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Mar 27, 2021, 18:35 UTC
Message-ID: <20210327183554.GD2271@szeder.dev>
In-Reply-To: <e4aa3e8cd7f64512ce0d72906f4d15f1f0dc0a60.1616574955.git.liu.denton@gmail.com>

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:

Show 5 quoted lines

> 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?

Show 7 quoted lines

> 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@gmail.com>
> ---
>  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.

Show 70 quoted lines

> 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]}.

Show 14 quoted lines

> @@ -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.

Show 6 quoted lines

>  		esac
>  		((c++))
>  	done
> -- 
> 2.31.0.rc2.261.g7f71774620
>