On 03/14/2018 12:53 AM, Luke Shumaker wrote:
Part of it is to have a common style. Trying to rectify two codebases that diverged 7 years ago is rough. When trying to come up with clean diffs, having to guess "did the other one quote this variable?" makes it harder. If you can say "always quote (except for the LHS of [[ ]])" or something, that makes it a bit easier.
I'm not sure that "specifically for the sole sake of diffs against our fork" is a valid justification on its own for modifying a coding style.
backup_package_variables() { - for var in ${splitpkg_overrides[@]}; do + for var in "${splitpkg_overrides[@]}"; do indirect="${var}_backup" - eval "${indirect}=(\${$var[@]})" + eval "${indirect}=(\"\${$var[@]}\")" done }
restore_package_variables() { - for var in ${splitpkg_overrides[@]}; do + for var in "${splitpkg_overrides[@]}"; do indirect="${var}_backup" if [ -n "${!indirect}" ]; then - eval "${var}=(\${$indirect[@]})" + eval "${var}=(\"\${$indirect[@]}\")" else - unset ${var} + unset "${var}" fi done
This is too much escaping and metaprogramming, there are better ways of backing up a variable to begin with. :/
We do it in makepkg, I will have us do it here as well. Advantage: using declare -p means the shell auto-escapes things where needed.
I haven't been keeping my thumb on makepkg git, but the eval lines as I wrote them exactly match the eval lines in makepkg 5.0.2's version of {backup,restore}_package_variables (makepkg's versions don't quote the for loops, or the unset command).
Hmm, I was thinking of: eval "$restoretrap" eval "$restoreset" eval "$restoreshopt" eval "$restore_envvars" and similar. Maybe I should fix the backups as well, but that is a slightly more complicated case there.
- if ! ${CLEANUP_DRYRUN}; then + if ! "${CLEANUP_DRYRUN}"; then
This is a variable being run as a command, so if there were to be spaces in it we'd end up trying to run a command with a space in it. Arguably we should not be running this as a command (even though they are set to true/false which is a shell builtin blah blah blah) but since we are it would be illogical to indicate that if there are spaces they should be interpreted as string literals in an executable filename.
For the true/false idiom, quoting it is just a style rule. I figure accepting the true/false idiom doesn't imply allowing the boolean variable to have any value. Having the quotes would help catch the variable being erroneously set to a different value.
So would doing a bash test.
At some point, I'd like to have `make lint` run shellcheck over dbscripts. That's a long way off, both because of a whole bunch of changes needed in dbscripts to make it come back clean, and a few features needed in shellcheck to avoid having to drop entirely too many shellcheck directives in to the dbscripts source.
Anyway, I know linters should be taken with a grain of salt, but when there's something simple like this, that you know just about any linter would complain about... why not?
That would imply one of my long-term goals is being able to run a linter. If I did, this rule would be the first thing I disabled -- it is far, far too prone to both false positives and false negatives. -- Eli Schwartz Bug Wrangler and Trusted User