Decide and document quoting rule

[akinomyoga]

Hmm, I suppose this means we should be quoting all arguments containing variables to any commands or functions we invoke? This PR does not address them all throughout the codebase, but we can take care of them in followup PR's.

I'm starting to feel that we should write up some rules about all this stuff in our docs somewhere, perhaps styleguide.md. And it would be great if there was some tooling that would help by flagging or fixing undesired uses. I wonder if shellcheck or shfmt would accept some of that stuff.

Originally posted by @scop in #931 (comment)

[scop]

As we're going quite deep in our support for weird (considering interactive shell) $IFS already anyway, I suppose quoting these arguments containing variables fits in that theme. Likely needs to be done for other things as well, for example return values.

I'm not sure how to best write this up rather than to note that quoting needs to be done wherever word splitting can occur (making no assumptions about $IFS). Also not sure how long will it take until I start to actually remember to write things that way 😛

(On the other hand, would be nice to remove unnecessary quoting where splitting does not occur, for example in assignments and within [[ ]].)

[scop]

Re that unnecessary quoting, filed mvdan/sh#996

[scop]

I guess I'd be fine with starting to quote all arguments, return values or the RHS of comparisons no matter if they contain variables or not. That'd make it easier to remember, and would colorize consistently in editors. What do you think?

[akinomyoga]

I agree with it. Actually, I personally prefer quoting them independently of IFS, noglob, etc. in the context. Context-independent constructs are easier to read because we do not need to check the contexts in reading.

[scop]

Cool. If you feel like it and can think of a good wording, feel free to take a look at filing a style guide PR. I'll get to it eventually myself, too, no pressure.

I think we should follow whatever our tooling (mostly shfmt I guess) does with regards to this, and only apply our "rules" on top of what it does, and not fight the tools.

[akinomyoga]

Thank you. I think I can try it, but I have questions.

• $* and ${arr[*]} need to be quoted as "$*" and "${arr[*]}" even in the contexts where the quoting is usually unnecessary because unquoted $* and ${arr[*]} is still affected by word splitting in bash <= 4.2:

  $ bash-4.2 -c 'a=(1 2 3);IFS=x;v=${a[*]};echo "[$v]"'
  [1 2 3]
  $ bash-4.3 -c 'a=(1 2 3);IFS=x;v=${a[*]};echo "[$v]"'
  [1x2x3]

  However, shfmt requires us to unquote it in a conditional command.

  shfmt
  -    [[ "$*" == abc ]]
  +    [[ $* == abc ]]

• Do we specify the detailed types of the quoting? Maybe we do not need to be too specific.
  • For example, when $ or ` is not included in the string, do we use single quotes 'foo bar', double quotes "foo bar", or even foo\ bar? By the way, we are using var="" in the codebase for the assignments of an empty string.
  • When control characters are included in the string (such as tabs and newlines), do we use '...' with raw control characters or $'...' with escape sequences such as \t?
  • How about when ', ", $, \, etc. are included in the string?

---

Edit

quoting of here strings and shellcheck

• Also, shellcheck seems to require us to quote here strings. With

      cat <<<$var

  shellcheck produces the following error:

  shellcheck...............................................................Failed
  - hook id: shellcheck
  - exit code: 1
  
  bash_completion:309:12: note: Double quote to prevent globbing and word splitting. [SC2086]
  

  The message says double quotes are needed to prevent word splitting and pathname expansions, but word splitting and pathname expansions do not happen in this context even without the quoting.~~

It turned out that bash <= 4.3 has a bug: koalaman/shellcheck#1009 (comment), so we should also quote here strings as far as we support bash 4.3.

[scop]

Re shfmt, I wonder if that's something we should report as a bug against shfmt. Also, I suppose there's no way to tell shfmt to leave a line alone (e.g. something like # nofmt).

Re quote character, I suppose we could write out a preference in the style guide. I've accustomed to using the double quotes by default, excluding sometimes if a constant string needs to embed them.

Re control characters, I'm accustomed to using $'...' -- unsure what the alternative would look like.

Re ', ", $, \, I think it depends and makes sense to go with something that does not require excessive backslash escaping or unnecessary splitting/concatenation using different quotes.