From: Rubén Justo <rjusto@gmail.com>
Date: Sat, 03 Feb 2024 15:13:36 GMT
Subject: Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch
Message-ID: <40c3a824-a961-490b-94d4-4eb23c8f713d@gmail.com>
In-Reply-To: <d37781c3-6af2-409b-95a8-660a9b92d20b@smtp-relay.sendinblue.com>

On 08-ene-2024 15:53:03, Britton Leo Kerin wrote:
> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 185b47d802..2b2b6c9738 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1356,6 +1356,29 @@ __git_count_arguments ()
>  	printf "%d" $c
>  }
>  
> +# Complete actual dir (not pathspec), respecting any -C options.
> +#
> +# Usage: __git_complete_refs [<option>]...
> +# --cur=<word>: The current dir to be completed.  Defaults to the current word.
> +__git_complete_dir ()
> +{
> +	local cur_="$cur"
> +
> +	while test $# != 0; do
> +		case "$1" in
> +		--cur=*)	cur_="${1##--cur=}" ;;
> +		*)		return 1 ;;
> +		esac
> +		shift
> +	done
> +
> +        # This rev-parse invocation amounts to a pwd which respects -C options
> +	local context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)
> +	[ -d "$context_dir" ] || return 1
> +
> +	COMPREPLY=$(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_")

This assignment is problematic.

First, COMPREPLY is expected to be an array.  Maybe a simple change can
do the trick:

-	COMPREPLY=$(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_")
+	COMPREPLY=( $(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_") )

But, what happens with directories that have SP's in its name?  We're
giving wrong options:

    $ mkdir one
    $ mkdir "one more dir"
    $ git am --directory=o<TAB><TAB>
    dir   more  one

Setting IFS can help us:

+       local IFS=$'\n'

Now we're returning correct options:

    $ mkdir one
    $ mkdir "one more dir"
    $ git am --directory=o<TAB><TAB>
    one       one more dir

Here, the user might be expecting directory names with a trailing '/',
as Bash do.  Again, a simple trick:

-	COMPREPLY=( $(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_") )
+	COMPREPLY=( $(cd "$context_dir" 2>/dev/null && compgen -d -S / -- "$cur_") )

Now looks better, IMO:

    $ git am --directory=o<TAB><TAB>
    one/      one more dir/

But, after all of this, we're going to provoke a problematic completion due
to the SP:

    $ mkdir "another one"
    $ git am --directory=anot<TAB><TAB>
    ...
    $ git am --directory=another one/

We should complete to:

    $ git am --directory=another\ one/

Here we need a less simple trick:

+       # use a hack to enable file mode in bash < 4
+       # compopt -o filenames +o nospace 2>/dev/null ||
+       compgen -f /non-existing-dir/ >/dev/null ||
+       true

Some commits you may find interesting:
fea16b47b6 (git-completion.bash: add support for path completion, 2013-01-11)
3ffa4df4b2 (completion: add hack to enable file mode in bash < 4, 2013-04-27)

Well, so far, so good?  I'm afraid, not:  What happens with other
special characters like quotes '"'?

I suggest you take a look at how we are already doing all of
considerations for other commands, like git-add.

> +}
> +
>  __git_whitespacelist="nowarn warn error error-all fix"
>  __git_patchformat="mbox stgit stgit-series hg mboxrd"
>  __git_showcurrentpatch="diff raw"
> @@ -1374,6 +1397,10 @@ _git_am ()
>  		__gitcomp "$__git_whitespacelist" "" "${cur##--whitespace=}"
>  		return
>  		;;
> +	--directory=*)
> +		__git_complete_dir --cur="${cur##--directory=}"
> +		return
> +		;;
>  	--patch-format=*)
>  		__gitcomp "$__git_patchformat" "" "${cur##--patch-format=}"
>  		return
> @@ -1867,7 +1894,17 @@ __git_format_patch_extra_options="
>  
>  _git_format_patch ()
>  {
> +	case "$prev,$cur" in
> +	-o,*)
> +		__git_complete_dir
> +		return
> +		;;
> +	esac
>  	case "$cur" in
> +	--output-directory=*)
> +		__git_complete_dir --cur="${cur##--output-directory=}"
> +		return
> +		;;

Adding completion for these options, and possibly opening the way for
others, is a nice improvement.

Thank you for working on this.