fix: quote boolean command names such as if "$boolean_variable"; then

[akinomyoga]

I have created this branch after #891 (comment) but left it for about one month. Now there was a related discussion in #910 (comment), so I decided to submit it here.

edit: This branch also includes other fixes: 7b6fb48, e731fc4, 7971f50

[akinomyoga]

I decided to separate the change $split && return to "$split" && return to another PR #918 for easier review.

[scop]

Ouch. I had written this last week, but forgot to click "Review changes", so the comment was left in pending state. Sorry! Anyway, here goes.

---

I can't say I'm too enthusiastic about this at least in cases where we are in complete control of the variable's contents.

I can see how this would be necessary to make things work in some crazy $IFS cases, but I believe the codebase is riddled with things that would break with them (this PR's just scratching the surface), and I'm not convinced we should try and support everything one might want to throw at it. I suppose there was a PR somewhere (which I've likely failed to follow up with) that would ensure a desired environment at start of a completion -- that would sound much preferable over this approach to me.

To me, reading if "$something"; then raises the question "was this supposed to be if [[ "$something" ]]; then" (even though the quotes within the square brackets are superfluous) and thus makes the code heavier to read. Also I'm quite convinced that it will take me a long time until I learn to take this into account, and there's no tool to fix it up or warn me. And even if I did learn it, I can't say I'd like it.

I'm leaning towards that we either don't do anything about this at all, or switch to comparing variable values instead of invoking boolean commands they contain, similarly as we did e.g. for the _known_hosts_real $ipv4 and $ipv6 cases below before this change. I think I'd prefer the string set might as a "set" marker better than 1 or true to clearly differentiate from the boolean commands and arithmetic context, but that's a bit besides the point, but in case we're modifying these anyway...

[akinomyoga]

I suppose there was a PR somewhere (which I've likely failed to follow up with) that would ensure a desired environment at start of a completion -- that would sound much preferable over this approach to me.

PR #739 and Issue #786 are related. PR #739 contains unrelated long discussions, but the current situation is summarized in the following two comments:

• feat: implement BASH_COMPLETION_FINALIZE_HOOKS #739 (comment)
• Aborting a completion attempt leaves set -o noglob in effect #786 (comment)

To me, reading if "$something"; then raises the question "was this supposed to be if [[ "$something" ]]; then" (even though the quotes within the square brackets are superfluous) and thus makes the code heavier to read.

I actually prefer [[ $something ]] with something being an empty/non-empty variable. The reason that I decided to switch to this direction was item 2 in the review comment #891 (review). Might I have misread the comment? Anyway, if you would like to switch to [[ $something ]], I will adjust them.

[scop]

I actually prefer [[ $something ]] with something being an empty/non-empty variable. The reason that I decided to switch to this direction was item 2 in the review comment #891 (review). Might I have misread the comment?

You've got it right. My opinion is still that if $something; then reads cleaner than if [[ $something ]]; then. But per reasoning here, we can't have the former if we want to protect against funky $IFS. And I think if "$something"; then with the quotes is so weird that I prefer if [[ $something ]]; then over that.

Anyway, if you would like to switch to [[ $something ]], I will adjust them.

Yes, please. I'd also suggest using literal set as the value when the variable is set, instead of true, 1 or the like.

[akinomyoga]

Yes, please. I'd also suggest using literal set as the value when the variable is set, instead of true, 1 or the like.

I reworked them so decided to submit them as another PR #931.

[akinomyoga]

• Switched to an opposite approach fix: use if [[ $flag ]] for IFS-safe boolean tests #931.
• Some other changes are added to fix: quote $split of $split && return for custom IFS #918.
• bf959e5 and b56c28a are queued in my local branch. I'll resubmit them someday.