[PATCH 0/1] completion: complete dir-type option args to am, format_patch

[PATCH 0/1] completion: complete dir-type option args to am, format_patch

[Britton Leo Kerin]

I think are a few other places this could be done as well but I wanted to
start with just a couple to show the idea.

Britton Leo Kerin (1):
  completion: dir-type optargs for am, format-patch

 contrib/completion/git-completion.bash | 38 ++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)


base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
--
2.43.0

[PATCH v2 0/1] completion: complete dir-type option args to am, format_patch

[Britton Leo Kerin]

This revision adds missing double quotes to improve the completion
situation for paths with spaces in them, and adds some comments.

Britton Leo Kerin (1):
  completion: dir-type optargs for am, format-patch

 contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Range-diff against v1:
1:  2d788b0b18 ! 1:  5161304a92 completion: dir-type optargs for am, format-patch
    @@ contrib/completion/git-completion.bash: __git_count_arguments ()
      	printf "%d" $c
      }

    -+
     +# Complete actual dir (not pathspec), respecting any -C options.
     +#
     +# Usage: __git_complete_refs [<option>]...
    @@ contrib/completion/git-completion.bash: __git_count_arguments ()
     +		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
    ++	[ -d "$context_dir" ] || return 1
     +
    -+	COMPREPLY=$(cd $context_dir 2>/dev/null && compgen -d -- "$cur_")
    ++	COMPREPLY=$(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_")
     +}
    -+
     +
      __git_whitespacelist="nowarn warn error error-all fix"
      __git_patchformat="mbox stgit stgit-series hg mboxrd"
--
2.43.0

[PATCH v2 1/1] completion: dir-type optargs for am, format-patch

[Britton Leo Kerin]

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_")
+}
+
 __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
+		;;
 	--thread=*)
 		__gitcomp "
 			deep shallow
-- 
2.43.0

Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch

[Rubén Justo]

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.

Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch

[Britton Kerin]

On Sat, Feb 3, 2024 at 6:13 AM Rubén Justo <rjusto@gmail.com> wrote:
>
> 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.

Thanks for all these suggestions.  Considering them and working on it
some more I end up with this function:


__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

        compopt -o noquote

        local IFS=$'\n'
        local unescaped_candidates=($(cd "$context_dir" 2>/dev/null &&
compgen -d -S / -- "$cur_"))
        for ii in "${!unescaped_candidates[@]}"; do
                COMPREPLY[$ii]=$(printf "%q" "${unescaped_candidates[$ii]}")
        done
}

This one works for all weird characters that I've tried in bash 5.2 at
least, and in frameworks that do their own escaping also (e.g.
ble.sh).  Since your advice so far was so good I thought I'd ask if
there is anything obvious to you that is still wrong here?

If not I guess what's left is special code to make it work better with
old versions of bash.  I'm a little sceptical that this is worth it
since bash 5 is already 5 years old and it's only completion code
we're talking about  but I guess it could be done.

Britton

Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch

[Rubén Justo]

On Mon, Feb 12, 2024 at 13:52:53 -0900, Britton Kerin wrote:

> __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
> 
>         compopt -o noquote
> 
>         local IFS=$'\n'
>         local unescaped_candidates=($(cd "$context_dir" 2>/dev/null &&
> compgen -d -S / -- "$cur_"))
>         for ii in "${!unescaped_candidates[@]}"; do
>                 COMPREPLY[$ii]=$(printf "%q" "${unescaped_candidates[$ii]}")
>         done
> }
> 
> This one works for all weird characters that I've tried in bash 5.2 at
> least, and in frameworks that do their own escaping also (e.g.
> ble.sh).  Since your advice so far was so good I thought I'd ask if
> there is anything obvious to you that is still wrong here?

> If not I guess what's left is special code to make it work better with
> old versions of bash.  I'm a little sceptical that this is worth it
> since bash 5 is already 5 years old and it's only completion code
> we're talking about  but I guess it could be done.

I don't think you need to dig too much into old Bash versions.  If it
works with a recent one, it's a good start.

Have you considered adding some tests to t/t9902-completion.sh?

It is desirable to see some tests at least for __git_complete_dir.
Perhaps it would also help you to polish the function.

Sorry for the late response.  I just found your message while reviewing
the topics in the 'What's cooking'.