On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote:
> On Wed, Dec 01, 2021 at 09:11:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Amend the tests checking whether stderr is empty added in
> > 4868b2ea17b (Subject: setup: officially support --work-tree without
> > --git-dir, 2011-01-19) work portably on all POSIX shells, instead
> > suppressing the trace output with "test_untraceable" on shells that
> > aren't bash.
> >
> > The tests that used the "try_repo" helper wanted to check whether git
> > commands invoked therein would emit anything on stderr. To do this
> > they invoked the function and redirected the stderr to a "message"
> > file.
> >
> > In 58275069288 (t1510-repo-setup: mark as untraceable with '-x',
> > 2018-02-24) these were made to use "test_untraceable" introduced in
> > 5fc98e79fc0 (t: add means to disable '-x' tracing for individual test
> > scripts, 2018-02-24).
> >
> > It is better to have the "try_repo" function itself start with a
> > "test_when_finished 'rm stderr'", and then redirect the stderr output
> > from git commands it invokes via its helpers to a "stderr" file.
> >
> > This means that if we have a failure it'll be closer to the source of
> > the problem, and most importantly isn't incompatible with "-x" on
> > shells that aren't "bash".
> >
> > We also need to split those tests that had two "try_repo" invocations
> > into different tests, which'll further help to narrow down any
> > potential failures. This wasn't strictly necessary (an artifact of the
> > use of "test_when_finished"), but the pattern enforces better test
> > hygiene.
> >
> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> > ---
> > t/t1510-repo-setup.sh | 83 +++++++++++++++++++++----------------------
> > 1 file changed, 40 insertions(+), 43 deletions(-)
> >
> > diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
> > index 591505a39c0..f1748ac4a19 100755
> > --- a/t/t1510-repo-setup.sh
> > +++ b/t/t1510-repo-setup.sh
> > @@ -40,9 +40,6 @@ A few rules for repo setup:
> > prefix is NULL.
> > "
> >
> > -# This test heavily relies on the standard error of nested function calls.
> > -test_untraceable=UnfortunatelyYes
> > -
> > TEST_PASSES_SANITIZE_LEAK=true
> > . ./test-lib.sh
> >
> > @@ -62,7 +59,7 @@ test_repo () {
> > export GIT_WORK_TREE
> > fi &&
> > rm -f trace &&
> > - GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null &&
> > + GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr &&
>
> I suspect that it's lines like this that make Peff argue for
> BASH_XTRACEFD :)
>
> While this is not a compound command, it does contain a command
> substitution, and the trace generated when executing the command in
> that command substitution goes to the command's stderr, and then,
> because of the redirection, to the 'stderr' file.
>
> I find it suspicious that this doesn't trigger a failure in a
> 'test_must_be_empty stderr' later on.
Ah, that's because this hunk is executed in a subshell that starts
with 'cd "$1"', creating an extra 'stderr' file in a subdirectory:
$ ./t1510-repo-setup.sh -r 1 -d
[...]
$ find trash\ directory.t1510-repo-setup/ |grep stderr
trash directory.t1510-repo-setup/0/stderr
trash directory.t1510-repo-setup/0/sub/stderr
Changing that redirection to '2>>"$TRASH_DIRECTORY"/stderr' makes a
bunch of tests fail with:
+ test_must_be_empty stderr
'stderr' is not empty, it contains:
+ pwd
error: last command exited with $?=1
+ rm stderr
not ok 1 - #0: nonbare repo, no explicit configuration
