Re: [PATCH 6/7] test-lib: make --verbose output valid TAP

From: Chris Torek <chris.torek@gmail.com>
Date: Mar 10, 2021, 07:42 UTC
Message-ID: <CAPx1GvcDvCykfOJBBPq1vx19dx2zT4cvs=ZPGBtzzw87ZDagzQ@mail.gmail.com>
In-Reply-To: <20210309160219.13779-7-avarab@gmail.com>

There is a lot of historical stuff I can't really comment on here, so I'll just point out this one tiny bit of shell oddity:

On Tue, Mar 9, 2021 at 8:04 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> Make the --verbose output be valid TAP, making it machine-readable for
> TAP parsers again.

[snippage]

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index aa7068b06b6..0070d05234b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh

[snippage]

Show 24 quoted lines

> @@ -364,9 +366,22 @@ then
>         (
>                 GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1
>                 echo $? >"$TEST_RESULTS_BASE.exit"
> -       ) | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
> +       ) | "$GIT_BUILD_DIR"/t/helper/test-tool tee \
> +               --tap --prefix="GIT_TEST_TEE_STARTED " \
> +               --escape-stdout ${HARNESS_ACTIVE+--escape-file} \
> +               "$GIT_TEST_TEE_OUTPUT_FILE"
>         test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
>         exit
> +elif test -n "$verbose" -a -n "$HARNESS_ACTIVE"
> +then
> +       ret=
> +       (
> +               GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1
> +               ret=$?
> +       ) | "$GIT_BUILD_DIR"/t/helper/test-tool tee \
> +               --tap --prefix="GIT_TEST_TEE_STARTED " \
> +               --escape-stdout
> +       exit $ret
>  fi
>
>  if test -n "$trace" && test -n "$test_untraceable"

[snippage]

In the block beginning with:

    ret=

we have a subshell:

    ( ... ) | "$GIT_BUILD_DIR"/t/helper/test-tool tee ...

In the subshell itself, we set `ret=$?`. But this is inside a subshell, which then exits, so the setting of `ret` will get lost.

Did you perhaps want `{ ...; }` instead here? Unfortunately the pipe means that the whole left side may run in a subshell anyway, so even that doesn't fix the problem. We need a temp file, a la the code above that dumps $? into "$TEST_RESULTS_BASE.exit".

Chris