Re: [PATCH 07/12] t: prepare `test_when_finished ()`/`test_atexit()` for `set -e`

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> Both `test_when_finished ()` and `test_atexit ()` build up a chain of
> cleanup commands by prepending each new command to the existing cleanup
> string. To preserve the exit code of the test body across cleanup
> execution, we append the following logic:
>
>     } && (exit "$eval_ret"); eval_ret=$?; ...
>
> The intent of this is to run the cleanup block and then unconditionally
> restore `eval_ret`. The original behaviour of this is is:
>
>    +------------------+---------+------------------------------------+
>    |test body         │ cleanup │ old behaviour                      │
>    +------------------+---------+------------------------------------+
>    │pass (eval_ret=0) | pass    │ && taken -> (exit 0) -> eval_ret=0 |
>    +------------------+---------+------------------------------------+
>    │pass (eval_ret=0) | fail    │ && not taken -> eval_ret=$?        |
>    +------------------+---------+------------------------------------+
>    │fail (eval_ret=1) | pass    │ && taken -> (exit 1) -> eval_ret=1 |
>    +------------------+---------+------------------------------------+
>    │fail (eval_ret=1) | fail    | && not taken -> eval_ret=$?        |
>    +------------------+---------+------------------------------------+
>
> This logic will start to fail once we enable `set -e`. When `$eval_ret`
> is non-zero, the subshell we create will fail, and with `set -e` we'll
> thus bail out without evaluating the logic after the semicolon.
>
> Fix this issue by instead using `|| eval_ret=\$?; ...`. Besides being
> a bit simpler, it also retains the original behaviour:
>
>    +------------------+---------+------------------------------------+
>    |test body         │ cleanup │ old behaviour                      │
>    +------------------+---------+------------------------------------+
>    │pass (eval_ret=0) | pass    │ || not taken -> eval_ret unchanged |
>    +------------------+---------+------------------------------------+
>    │pass (eval_ret=0) | fail    │ || taken -> eval_ret=$?            |
>    +------------------+---------+------------------------------------+
>    │fail (eval_ret=1) | pass    │ || not taken -> eval_ret unchanged |
>    +------------------+---------+------------------------------------+
>    │fail (eval_ret=1) | fail    | || taken -> eval_ret=$?            |
>    +------------------+---------+------------------------------------+
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/test-lib-functions.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index f8bc77619b..0eac676109 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1516,7 +1516,7 @@ test_when_finished () {
>  	test "${BASH_SUBSHELL-0}" = 0 ||
>  	BUG "test_when_finished does nothing in a subshell"
>  	test_cleanup="{ $*
> -		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
> +		} || eval_ret=\$?; $test_cleanup"

Hmph, it seems this "&& (exit $eval_ret)" pattern has been with us
forever since it was introduced at 3bf78867 (test-lib: Let tests
specify commands to be run at end of test, 2010-05-02), and survived
a slight modification to work around issues on FreeBSD done in
b6b0afdc (test-lib: some shells do not let $? propagate into an
eval, 2010-05-06).  When a major part of test-lib.sh was split into
test-lib-functions.s at 12a29b1a (Move the user-facing test library
to test-lib-functions.sh, 2012-02-17), this part was copied intact.

Does this interact well with the glitch b6b0afdc (test-lib: some
shells do not let $? propagate into an eval, 2010-05-06) tried to
work around, by the way?

Thanks.

>  }
>  
>  # This function can be used to schedule some commands to be run
> @@ -1544,7 +1544,7 @@ test_atexit () {
>  	test "${BASH_SUBSHELL-0}" = 0 ||
>  	BUG "test_atexit does nothing in a subshell"
>  	test_atexit_cleanup="{ $*
> -		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
> +		} || eval_ret=\$?; $test_atexit_cleanup"
>  }
>  
>  # Deprecated wrapper for "git init", use "git init" directly instead