Re: [PATCH 1/2] test-lib: mark function variables as local

[Tomi Ollila <tomi.ollila@iki.fi>]

On Wed, May 06 2020, Daniel Kahn Gillmor wrote:

> Several functions in test/test-lib.sh used variable names that are
> also used outside of those functions (e.g. $output and $expected are
> used in many of the test scripts), but they are not expected to
> communicate via those variables.
>
> We mark those variables "local" within test-lib.sh so that they do not
> get clobbered when used outside test-lib.


Good stuff

robustness comment IMO:

There is slight difference when writing

    local foo=`false`

and

    local foo; foo=`false`


former does not "fail"; latter does,


Although there is (currently!) no difference in our test code
(we don't have `set -e` there, IMO the former serves as a bad
example for anyone looking the code. 

(same applies to export foo=`bar`, readonly foo=`bar` and so
on, for anyone curious...)

IMO better declare all local variables in one line separately,
e.g.

    local output expected

and then either

    output=$1
    expected=$2
or
    output=$1 expected=$2

( FYI: exection latter in shell differs in a way one could do
  output=$expected expected=$output ) (IIRC, did not test >;)

(add double quotes around $1 and $2 if you desire =D)


well, when doing change just add the `local` line, smaller diff :)


Tomi

>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> ---
>  test/test-lib.sh | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 5c8eab7c..e8feab3b 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -109,7 +109,6 @@ unset ALTERNATE_EDITOR
>  
>  add_gnupg_home ()
>  {
> -    local output
>      [ -e "${GNUPGHOME}/gpg.conf" ] && return
>      _gnupg_exit () { gpgconf --kill all 2>/dev/null || true; }
>      at_exit_function _gnupg_exit
> @@ -427,7 +426,7 @@ emacs_fcc_message ()
>  # number of messages.
>  add_email_corpus ()
>  {
> -    corpus=${1:-default}
> +    local corpus=${1:-default}
>