Re: [arch-projects] [dbscripts] [PATCH 3/8] Export TMPDIR, and use mktemp -t instead of making it part of the template

[Eli Schwartz via arch-projects]

On 03/13/2018 09:52 PM, Luke Shumaker wrote:
> From: Luke Shumaker 


> diff --git a/test/cases/db-update.bats b/test/cases/db-update.bats
> index e7e4489..2e44b91 100644
> --- a/test/cases/db-update.bats
> +++ b/test/cases/db-update.bats
> @@ -222,7 +222,7 @@ load ../lib/common
>  
>  @test "package has to be aregular file" {
>   local p
> - local target=$(mktemp -d)
> + local target=$(mktemp -dt)
>   local arches=('i686' 'x86_64')
>  
>   releasePackage extra 'pkg-simple-a'
> diff --git a/test/lib/common.bash b/test/lib/common.bash
> index 568a541..45e4800 100644
> --- a/test/lib/common.bash
> +++ b/test/lib/common.bash
> @@ -83,7 +83,7 @@ setup() {
>   local a
>   PKGEXT=".pkg.tar.xz"
>  
> - TMP="$(mktemp -d)"
> + TMP="$(mktemp -dt)"
>  
>   export DBSCRIPTS_CONFIG=${TMP}/config.local
>   cat < "${DBSCRIPTS_CONFIG}"
> 

These two have no TEMPLATE given anyways, so this change is extraneous.

-- 
Eli Schwartz
Bug Wrangler and Trusted User



signature.asc
Description: OpenPGP digital signature

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

[Eli Schwartz via arch-projects]

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



signature.asc
Description: OpenPGP digital signature