| Message ID | d37781c3-6af2-409b-95a8-660a9b92d20b@smtp-relay.sendinblue.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] completion: dir-type optargs for am, format-patch | expand |
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.
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
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'.
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
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com> --- contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)