Fix subcommand/choices completion in bash

[brentyi]

1. The choices list was formerly not being expanded correctly.
2. Removing local IFS=$'\\n' fixed some issues as well. Wasn't totally following why this was needed.

[casperdcl]

Thanks! Sorry for the delay.

Fixes #86 (subparser completion).
Unfortunately this also re-breaks #81 <- #74 (/paths/with\ spaces completion).
Potentially ok as a trade-off for now. WDYT @pared @shcheklein @efiop @0x2b3bfa0?

[codecov-commenter]

Codecov Report

Base: 74.46% // Head: 74.46% // No change to project coverage 👍

Coverage data is based on head (9bfda44) compared to base (13de419).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #106   +/-   ##
=======================================
  Coverage   74.46%   74.46%           
=======================================
  Files           2        2           
  Lines          47       47           
=======================================
  Hits           35       35           
  Misses         12       12           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[casperdcl]

/tag v1.5.6 f8203d9

[brentyi]

Thanks @casperdcl!!

[NiklasReisser]

Ah, unsetting IFS in between did the trick! Thanks for fixing this!

[casperdcl]

Yep was unreasonably hard to figure out. Can't tell what's worse about shells - the language itself, or the (lack of) good documentation. 🤺😅

[atnak]

@casperdcl Re your comment on SO:

you can also unset IFS as soon as it's served its purpose. That way subsequent commands can be simplified

No it's required by the compgen -W line for the same reason I wrote on the answer.

Instead of going over them again, I'll just give you examples showing you 1. why it's not the culprit, 2. why it's required. After that, I'll point out 3. where the bug in this code is.

1. Why it's not the culprit:

$ CHOICES=(bla blue)
$ printf "[%s]\n" "${CHOICES[@]}"
[bla]
[blue]
$ compgen -W "${CHOICES[*]}" bl
bla
blue
$ IFS=$'\n'
$ compgen -W "${CHOICES[*]}" bl
bla
blue
$ unset IFS

👆 Having IFS set doesn't change the outcome.

2. Why it's required:

$ CHOICES=("big bla" "big blue")
$ printf "[%s]\n" "${CHOICES[@]}"
[big bla]
[big blue]
$ compgen -W "${CHOICES[*]}" "big bl" # Broken without IFS setting
$ IFS=$'\n'
$ compgen -W "${CHOICES[*]}" "big bl"
big bla
big blue
$ unset IFS

👆 Because IFS wasn't set before the first compgen -W, compgen treated the candate list as big, bla, blue instead of big bla and big blue.

3. Where the bug in this code is

It's because current_action_choices isn't an array. The original *_choices variable is probably an array but the following line converts it to a scalar:

local current_action_choices_var="${current_action}_choices[@]"
current_action_choices="${!current_action_choices_var}"

Instead you need to do:

 local current_action_choices_var="${current_action}_choices[@]"
-current_action_choices="${!current_action_choices_var}"
+current_action_choices=("${!current_action_choices_var}")

Here's a reproduction:

$ FOO=(a b "c d")
printf "[%s]\n" "${FOO[@]}"
[a]
[b]
[c d]
$ BAR=FOO[@]
$ echo "$BAR"
FOO[@]
$ printf "[%s]\n" "${!BAR}"
[a]
[b]
[c d]
$ BROKEN_ARRAY="${!BAR}"
$ printf "[%s]\n" "${BROKEN_ARRAY[@]}"
[a b c d]
$ PROPER_ARRAY=("${!BAR}")
$ printf "[%s]\n" "${PROPER_ARRAY[@]}"
[a]
[b]
[c d]

(e.g. no need to printf ;%q\n)

On a final note, you also wrote 👆 in your comment but printf %q has nothing to do with IFS.

It's there to change COMPREPLY to big\ bla and big\ blue instead of big bla and big blue:

$ printf "%q\n" "big bla" "big blue"
big\ bla
big\ blue

Whether it's required or not depends entirely on the command and whether big and blue should be treated as a single argument or two separate arguments. e.g. If "big blue" was a name of a file and auto-completion did cat big blue instead of cat big\ blue it wouldn't be very helpful.