From: Britton Kerin <britton.kerin@gmail.com>
Date: Tue, 13 Feb 2024 00:52:53 GMT
Subject: Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch
Message-ID: <CAC4O8c9z4s4fFU6_h6ZRBnDhZyiTp3XR8j0DrARj+1SauLbQEQ@mail.gmail.com>
In-Reply-To: <40c3a824-a961-490b-94d4-4eb23c8f713d@gmail.com>

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