Re: [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage

["SZEDER Gábor" <szeder.dev@gmail.com>]

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@gmail.com>
> > ---
> >  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