git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 1/3] git-completion.bash: pass $__git_subcommand_idx from __git_main()
Date: Sat, 27 Mar 2021 19:35:54 +0100	[thread overview]
Message-ID: <20210327183554.GD2271@szeder.dev> (raw)
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:

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


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

  reply	other threads:[~2021-03-27 18:37 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-16  0:54 [PATCH 0/3] git-completion.bash: improvements to _git_stash() Denton Liu
2021-03-16  0:54 ` [PATCH 1/3] git-completion.bash: extract from else in _git_stash() Denton Liu
2021-03-16  0:54 ` [PATCH 2/3] git-completion.bash: fix `git <args>... stash branch` bug Denton Liu
2021-03-16  0:54 ` [PATCH 3/3] git-completion.bash: use __gitcomp_builtin() in _git_stash() Denton Liu
2021-03-18  9:46 ` [RESEND PATCH 0/3] git-completion.bash: improvements to _git_stash() Denton Liu
2021-03-18  9:46   ` [RESEND PATCH 1/3] git-completion.bash: extract from else in _git_stash() Denton Liu
2021-03-18  9:46   ` [RESEND PATCH 2/3] git-completion.bash: fix `git <args>... stash branch` bug Denton Liu
2021-03-18 20:30     ` Junio C Hamano
2021-03-19  8:05       ` Denton Liu
2021-03-19 15:53         ` Junio C Hamano
2021-03-18  9:46   ` [RESEND PATCH 3/3] git-completion.bash: use __gitcomp_builtin() in _git_stash() Denton Liu
2021-03-18 21:58   ` [RESEND PATCH 0/3] git-completion.bash: improvements to _git_stash() Junio C Hamano
2021-03-19  8:09     ` Denton Liu
2021-03-19 15:57       ` Junio C Hamano
2021-03-24  8:36 ` [PATCH v2 " Denton Liu
2021-03-24  8:36   ` [PATCH v2 1/3] git-completion.bash: pass $__git_subcommand_idx from __git_main() Denton Liu
2021-03-27 18:35     ` SZEDER Gábor [this message]
2021-03-28 10:31     ` SZEDER Gábor
2021-03-24  8:36   ` [PATCH v2 2/3] git-completion.bash: extract from else in _git_stash() Denton Liu
2021-03-28 10:30     ` SZEDER Gábor
2021-03-24  8:36   ` [PATCH v2 3/3] git-completion.bash: use __gitcomp_builtin() " Denton Liu
2021-03-28 11:04     ` SZEDER Gábor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210327183554.GD2271@szeder.dev \
    --to=szeder.dev@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=liu.denton@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).