Re: [PATCH] test: aggregate-results.sh: consistent style. zero forks.

[Daniel Kahn Gillmor <dkg@fifthhorseman.net>]

[-- Attachment #1: Type: text/plain, Size: 1590 bytes --]

On Tue 2019-06-04 22:46:24 +0300, Tomi Ollila wrote:
> - all variables in $((...)) without leading $
> - all comparisons use -gt, -eq or -ne
> - no -a nor -o inside [ ... ] expressions
> - all indentation levels using one tab
>
> Dropped unnecessary empty string check when reading results files.
>
> Replaced pluralize() which was executed in subshell with
> pluralize_s(). pluralize_s sets $s to 's' or '' based on value of
> $1. Calls to pluralize_s are done in context of current shell, so
> no forks to subshells executed.

I'm fine with all these changes, but:

> -if [ "$fixed" = "0" ] && [ "$failed" = "0" ]; then
> +if [ $fixed -eq 0 ] && [ $failed -eq 0 ]; then

I think we've set $fixed and $failed above to a non-empty string value,
so this is technically correct.  But the shell programmer nit-picker in
me gets nervous seeing any variable used inside a test without proper
wrapping, and i'm going to have trouble adopting $foo instead of "$foo"
in other shell scripts.  it seems to require a lot of global reasoning
about the state of a given variable to use it without quotes safely, and
it introduces some subtle requirements (like, no unsetting these
variables and no setting them to the empty string).

So anyway, i don't see the harm in using "$foo" instead in this case,
and it seems like it actually reduces the cognitive burden of
maintiaining the code.  i'm happy to listen to any compelling story
yo've got for why this is an important change, but i'd prefer to avoid
it.  (i don't at all mind unwrapping "0" to 0 though)

thanks for doing this cleanup!

     --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]