[3/6] completion: return the index of found word from __git_find_on_cmdline()
diff mbox series

Message ID 20191017173501.3198-4-szeder.dev@gmail.com
State New
Headers show
Series
  • completion: improve completion for 'git worktree'
Related show

Commit Message

SZEDER Gábor Oct. 17, 2019, 5:34 p.m. UTC
When using the __git_find_on_cmdline() helper function so far we've
only been interested in which one of a set of words appear on the
command line.  To complete options for some of 'git worktree's
subcommands in the following patches we'll need not only that, but the
index of that word on the command line as well.

Extend __git_find_on_cmdline() to optionally show the index of the
found word on the command line (IOW in the $words array) when the
'--show-idx' option is given.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 20 +++++++++++++++---
 t/t9902-completion.sh                  | 29 ++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

Comments

Eric Sunshine Oct. 17, 2019, 5:52 p.m. UTC | #1
On Thu, Oct 17, 2019 at 1:35 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> When using the __git_find_on_cmdline() helper function so far we've
> only been interested in which one of a set of words appear on the
> command line.  To complete options for some of 'git worktree's
> subcommands in the following patches we'll need not only that, but the
> index of that word on the command line as well.
>
> Extend __git_find_on_cmdline() to optionally show the index of the
> found word on the command line (IOW in the $words array) when the
> '--show-idx' option is given.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> @@ -1069,18 +1069,32 @@ __git_aliased_command ()
>  # Check whether one of the given words is present on the command line,
>  # and print the first word found.
> +#
> +# Usage: __git_find_on_cmdline [<option>]... "<wordlist>"
> +# --show-idx: Optionally show the index of the found word in the $words array.
>  __git_find_on_cmdline ()
>  {
> -       local word c=1
> +       local word c=1 show_idx
> +
> +       while test $# -gt 1; do
> +               case "$1" in
> +               --show-idx)     show_idx=y ;;
> +               *)              return 1 ;;

Should this emit an error message to aid a person debugging a test
which fails on a call to __git_find_on_cmdline()? For instance:

    echo "unrecognized option/argument: $1" >&2
    return 1
    ;;

or something...

> +               esac
> +               shift
> +       done
>         local wordlist="$1"
>
>         while [ $c -lt $cword ]; do
>                 for word in $wordlist; do
>                         if [ "$word" = "${words[c]}" ]; then
> -                               echo "$word"
> +                               if [ -n "$show_idx" ]; then
> +                                       echo "$c $word"
> +                               else
> +                                       echo "$word"
> +                               fi
>                                 return
>                         fi
>                 done
SZEDER Gábor Oct. 18, 2019, 2:37 p.m. UTC | #2
On Thu, Oct 17, 2019 at 01:52:27PM -0400, Eric Sunshine wrote:
> >  __git_find_on_cmdline ()
> >  {
> > -       local word c=1
> > +       local word c=1 show_idx
> > +
> > +       while test $# -gt 1; do
> > +               case "$1" in
> > +               --show-idx)     show_idx=y ;;
> > +               *)              return 1 ;;
> 
> Should this emit an error message to aid a person debugging a test
> which fails on a call to __git_find_on_cmdline()? For instance:
> 
>     echo "unrecognized option/argument: $1" >&2
>     return 1
>     ;;
> 
> or something...

When debugging the completion script I frequently resort to 'echo >&2
"<msg>"', for lack of better options.  However, I intentionally did
not add an error message like that here, or in any similar option
parsing loops before, because due to a bug it might spew such a
message to standard error during regular completion (i.e. not during
debugging).

And printing anything to standard error during completion is
inherently bad: it disrupts the command line, can't be deleted (you
hit backspace, and in the terminal it looks as if the error message
was deleted, but in reality it's the command you've already entered
that gets deleted), and the user is eventually fored to Ctrl-C and
start over most of the time.  Well, at least I always end up hitting
Ctrl-C and start over.  Remaining silent about the unrecognized option
is in my opinion better, because then the completion script usually
does nothing, and Bash falls back to filename completion.  Yeah,
that's not ideal, but at least the user can easily correct it and
finish entering the command.
Eric Sunshine Oct. 18, 2019, 9:01 p.m. UTC | #3
On Fri, Oct 18, 2019 at 10:37 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Thu, Oct 17, 2019 at 01:52:27PM -0400, Eric Sunshine wrote:
> > > +               case "$1" in
> > > +               --show-idx)     show_idx=y ;;
> > > +               *)              return 1 ;;
> >
> > Should this emit an error message to aid a person debugging a test
> > which fails on a call to __git_find_on_cmdline()? [...]
>
> And printing anything to standard error during completion is
> inherently bad: it disrupts the command line, can't be deleted [...]
> Remaining silent about the unrecognized option
> is in my opinion better, because then the completion script usually
> does nothing, and Bash falls back to filename completion.  Yeah,
> that's not ideal, but at least the user can easily correct it and
> finish entering the command.

I had tunnel-vision and was thinking about this only in the context of
the tests. However, while I agree that spewing errors during
completion is not ideal, aren't there two different classes of errors
to consider? Some errors can crop up via normal usage of Git commands
in Real World situations; those errors should be suppressed since they
are expected and can be tolerated. However, the second class of error
(such as passing a bogus option to this internal function) is an
outright programming mistake by a maintainer of the completion script
itself, and it would be helpful to let the programmer know as early as
possible about the mistake.

Or, are there backward-compatibility or other concerns which would
make emitting error messages undesirable even for outright programmer
mistakes?

Patch
diff mbox series

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2384f91e78..55a2d3e174 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1069,18 +1069,32 @@  __git_aliased_command ()
 	done
 }
 
-# __git_find_on_cmdline requires 1 argument
 # Check whether one of the given words is present on the command line,
 # and print the first word found.
+#
+# Usage: __git_find_on_cmdline [<option>]... "<wordlist>"
+# --show-idx: Optionally show the index of the found word in the $words array.
 __git_find_on_cmdline ()
 {
-	local word c=1
+	local word c=1 show_idx
+
+	while test $# -gt 1; do
+		case "$1" in
+		--show-idx)	show_idx=y ;;
+		*)		return 1 ;;
+		esac
+		shift
+	done
 	local wordlist="$1"
 
 	while [ $c -lt $cword ]; do
 		for word in $wordlist; do
 			if [ "$word" = "${words[c]}" ]; then
-				echo "$word"
+				if [ -n "$show_idx" ]; then
+					echo "$c $word"
+				else
+					echo "$word"
+				fi
 				return
 			fi
 		done
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 847ce601d2..3e3299819a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1392,6 +1392,35 @@  test_expect_success '__git_find_on_cmdline - no match' '
 	test_must_be_empty actual
 '
 
+test_expect_success '__git_find_on_cmdline - single match with index' '
+	echo "3 list" >expect &&
+	(
+		words=(git command --opt list) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - multiple matches with index' '
+	echo "4 remove" >expect &&
+	(
+		words=(git command -o --opt remove list add) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success '__git_find_on_cmdline - no match with index' '
+	(
+		words=(git command --opt branch) &&
+		cword=${#words[@]} &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_must_be_empty actual
+'
+
 test_expect_success '__git_get_config_variables' '
 	cat >expect <<-EOF &&
 	name-1