npm/global_config_entry.pp: improve shell quoting #436
Conversation
| $onlyif_command = $facts['os']['family'] ? { | ||
| 'Windows' => "${cmd_exe_path} /C FOR /F %G IN ('${npm_path} config get globalconfig') DO IF EXIST %G (FINDSTR /B /C:\"${$config_setting}\" %G) ELSE (EXIT 1)", | ||
| default => "test -f $(${npm_path} config get globalconfig) && grep -qe \"^${$config_setting}\" $(${npm_path} config get globalconfig)", | ||
| default => "test -f \"$(${npm_path} config get globalconfig)\" && grep -qe '^${$config_setting}' \"$(${npm_path} config get globalconfig)\"", |
There was a problem hiding this comment.
i don't understand the logic of your choices: when did you pick single quotes and when did you pick double quotes, and, why?
There was a problem hiding this comment.
@igalic when we don't need the shell to do any expansion or interpolation, we can use single quotes, which also saves us some escaping of double quotes. Command substitutions should usually be quoted to prevent word splitting and filename expansion on the results. Probably this doesn't matter for the expected values here, but might as well. Also, here
we already had proper quoting of the command substitution, so I wanted to make the other ones consistent.
Shell programming is the worst 😉
I wonder if this should be using inifile instead, but I guess this works. 🤷♂️
There was a problem hiding this comment.
actually, using inifile would make this infinitely more readable, and easier to transport from one OS to another, and simplify our testing, greatly, as the platform abstraction would already be handled by inifile itself
There was a problem hiding this comment.
so, what do we do?
who has time for this?
For safety and readability.
kenyon
force-pushed
the
shell-quoting-improvements
branch
from
b999f28 to
2eb623f
Compare
|
The preferred way to do this these days is to use arrays instead of strings, so I'll close this. |
2 participants
Improve shell quoting for safety and readability. Noticed these while reviewing #435 but didn't get to this before that was merged.