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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Apr 13, 2026 at 10:23:37AM -0700, Junio C Hamano wrote:
> 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.

Ah, that's a very good question. I'll spin up a FreeBSD VM to give this
a test, thanks!

Patrick