[arch-projects] [dbscripts] [PATCH 1/8] Fix quoting around variables, especially arrays.

[Eli Schwartz]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/arch-projects/attachments/20180314/04d8e98c/attachment-0001.asc>