| Message ID | 20191017173501.3198-4-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion: improve completion for 'git worktree' | expand |
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
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.
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?
On Fri, Oct 18, 2019 at 05:01:42PM -0400, Eric Sunshine wrote: > 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? It's not necessarily an outright programming mistake, and that error could be triggered by ordinary users as well. Let's suppose that a user has a custom 'git-foo' command in $PATH with a custom '_git_foo' completion function in '~/.my-git-completions', which the helper function '__git_bar --option' from our completion script. Let's also suppose that the user sources this completion function from '~/.bashrc', but otherwise uses the system-wide git completion script, and that $HOME is shared across multiple computers. In this (arguably somewhat convoluted) scenario it might happen that on a not quote up-to-date computer the system-wide git completion script already has the '__git_bar' helper function, but it doesn't yet support '--option'. If '__git_bar' then prints an error to stderr, then the command line will get badly messed up, and the user will have to ctrl-C and start over. However, if '__git_bar' silently ignores the unknown option, then the worst that can happen is that completion doesn't work, and e.g. it falls back to Bash's filename completion or offers something nonsensical. In either case, after a brief "Huh?!" moment the user can correct it by hitting backspace a couple of times and then enter the rest of the command by hand.
On Fri, Oct 18, 2019 at 04:37:28PM +0200, SZEDER Gábor wrote: > 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. Well, there is a better option for debugging: adding 'echo >>~/LOG "<msg>"' to the completion script and running 'tail -f ~/LOG' in a separate terminal window is so much more convenient than screwing up the command line with those messages to stderr every time. I just wonder why it took me about a dozen years to figure this one out... ;)
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
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(-)