stdenv-setup: Add quotes that don't do anything for consistency. #27614
Conversation
Ericson2314
force-pushed
the
stdenv-quotes
branch
from
a80427c
to
29e9f2a
Compare
| if [ -d "$dir" ]; then | ||
| eval export ${varName}=${!varName}${!varName:+$delimiter}${dir} | ||
| export "${varName}=${!varName}${!varName:+$delimiter}${dir}" |
There was a problem hiding this comment.
Oh. I had this branch sitting around for a while before my current tail-between-legs conservationism :D. But, spare me this one eval removal if stdenv builds :D:D?
| local fd | ||
| local magic | ||
| exec {fd}< "$fn" | ||
| read -r -n 4 -u $fd magic | ||
| read -r -n 4 -u "$fd" magic |
There was a problem hiding this comment.
I don't even know what that exec {fd}< "$fn" means. Is fd guaranteed to have no spaces here?
There was a problem hiding this comment.
It's a file descriptor, so yes. From the bash manpage:
Each redirection that may be preceded by a file descriptor number may instead be preceded by a word of the form {varname}. In this case, for each redirection operator except >&- and <&-, the shell will allocate a file descriptor greater than or equal to 10 and assign it to varname.
| local fd | ||
| local magic | ||
| if ! [ -x /bin/sh ]; then return 0; fi | ||
| exec {fd}< "$fn" | ||
| read -r -n 2 -u $fd magic | ||
| read -r -n 2 -u "$fd" magic |
There was a problem hiding this comment.
Even before my general PR, my basic is philosophy with bash is
"$asdf"is a string substitution"${asdf[@]}"is an array of strings substitution$asdfis only for intentional re-splitting, and must be paired with a comment explaining why the re-splitting is happening.
This emulates a decent language where the first two have the same syntax, and the 3rd would be a combination of that syntax with some explicit coercion / splitting syntax.
In other words, don't forgo quoting because of a lack of deliminator in the definition. Even if that is obviously true it is still tempting fate, and per my bullets above is misleading because there is no intention of a word-splitting.
The logic was made pure for the normal libSystem, but this change never made it to the bootstrap tools. Deduplication the logic as the comment suggests would have prevented this, but here's a stop-gap until we do so.
Ericson2314
force-pushed
the
stdenv-quotes
branch
from
29e9f2a
to
34c0ba4
Compare
|
Built on both repos. I can now commence with the master -> staging merge :). |
Motivation for this change
@vcunat and others rightly point out that it's easier to quote always than learn Bash's idiosyncrasies enough to know when it doesn't make a difference.
This reverts commit 2743078, which removes quotes that don't do anything, and then goes further adding even more quotes.
Things done
Please check what applies. Note that these are not hard requirements but mereley serve as information for reviewers.
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)