Re: [PATCH v3 2/3] sparse-checkout: custom tab completion

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jan 15, 2022, 09:57 UTC
Message-ID: <20220115095725.GA1738@szeder.dev>
In-Reply-To: <256e5f034c6072b6e3621adfa96c5c6319752fae.1641841193.git.gitgitgadget@gmail.com>

On Mon, Jan 10, 2022 at 06:59:51PM +0000, Lessley Dennington via GitGitGadget wrote:

> Subject: Re: [PATCH v3 2/3] sparse-checkout: custom tab completion

None of these patches touch sparse-checkout, but only the completion script and its tests. Therefore "completion:" would be a better matching area prefix.

Show 5 quoted lines

> Fix custom tab completion for sparse-checkout command. This will ensure:
> 
> 1. The full list of subcommands is provided when users enter git
> sparse-checkout <TAB>.
> 2. The --help option is tab-completable.

This is inconsistent with the rest of the completion script, because it doesn't list '--help' for any commands or subcommand (with the sole exception of 'git --<TAB>').

> 3. Subcommand options are tab-completable.
> 4. A list of directories (but not files) is provided when users enter git
> sparse-checkout add <TAB> or git sparse-checkout set <TAB>.

Why limit completion only to directories? Both of those subcommands accept files, and I think 'git sparse-checkout set README.md' is a perfectly reasonable command.

Show 55 quoted lines

> It is
> important to note that this will apply for both cone mode and non-cone
> mode (even though non-cone mode matches on patterns rather than
> directories).
> 
> Failing tests that were added in the previous commit to verify these
> scenarios are now passing with these updates.
> 
> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 38 ++++++++++++++++++--------
>  t/t9902-completion.sh                  |  8 +++---
>  2 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c82ccaebcc7..f478883f51c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2986,24 +2986,38 @@ _git_show_branch ()
>  	__git_complete_revlist
>  }
>  
> +__git_sparse_checkout_subcommand_opts="--cone --no-cone --sparse-index --no-sparse-index"
> +
>  _git_sparse_checkout ()
>  {
> -	local subcommands="list init set disable"
> +	local subcommands="list init set disable add reapply"
>  	local subcommand="$(__git_find_on_cmdline "$subcommands")"
> +
>  	if [ -z "$subcommand" ]; then
> -		__gitcomp "$subcommands"
> -		return
> +		case "$cur" in
> +			--*)
> +				__gitcomp "--help"
> +				;;
> +			*)
> +				__gitcomp "$subcommands"
> +				;;
> +		esac
>  	fi
>  
> -	case "$subcommand,$cur" in
> -	init,--*)
> -		__gitcomp "--cone"
> -		;;
> -	set,--*)
> -		__gitcomp "--stdin"
> -		;;
> -	*)
> -		;;
> +	case "$subcommand" in
> +		set)
> +			__gitcomp "$__git_sparse_checkout_subcommand_opts --stdin"

Oh, a hard-coded list of --options is so old school :)

All subcommands of 'git sparse-checkout' use parse-options (even those that don't have any --options at the moment), so their options can be completed programmatically, and then we wouldn't have to worry about updating the list of options in the completion script ever again. It would also make several tests added in the previous patch unnecessary.

You could use the completion function of 'git notes' for inspiration on how to do that.

> +			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"

This will have troubles with unusual characters in pathnames:

  - If a pathname contains a space (or any other IFS character), then
    the shell will split that into multiple words.

  - If a pathname contains a special character, e.g. double quote or
    backslash, then 'git ls-tree' will quote that path.  Furthermore,
    by default it will quote pathnames containing e.g. UTF-8
    characters as well.

  - The shell has its own special characteers that have to be
    quoted/escaped on the command line to strip them from their
    special meaning, that quoting/escaping will interfere with listing
    only paths matching the current word to be completed.

You should use the __git_complete_index_file() helper function instead, which takes care of most of this.

Furthermore, these two subsequent __gitcomp() calls will offer both options and paths for 'git sparse-checkout set <TAB>', which is inconsistent with the rest of the completion script. Usually it lists --options only after e.g. 'git rm --<TAB>' or 'git log --<TAB>', but after 'git rm <TAB>' and 'git log <TAB>' it lists only files and refs, respectively.

> +			;;
> +		add)
> +			__gitcomp "--stdin"
> +			__gitcomp "$(git ls-tree -d -r HEAD --name-only)"

Likewise.

Show 9 quoted lines

> +			;;
> +		init|reapply)
> +			__gitcomp "$__git_sparse_checkout_subcommand_opts"
> +			;;
> +		*)
> +			;;
>  	esac
>  }
>