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

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to