Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

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

On Sun, Dec 12, 2021 at 06:06:31PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Dec 12 2021, SZEDER Gábor wrote:
> 
> > On Fri, Dec 10, 2021 at 11:07:55AM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> In the preceding commit the use of "test_untraceable=UnfortunatelyYes"
> >> was removed from "t1510-repo-setup.sh" in favor of more narrow
> >> redirections of the output of specific commands (and not entire
> >> sub-shells or functions).
> >> 
> >> This is in line with the fixes in the series that introduced the
> >> "test_untraceable" facility. See 571e472dc43 (Merge branch
> >> 'sg/test-x', 2018-03-14) for the series as a whole, and
> >> e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a
> >> subshell, 2018-02-24) for a commit that's in line with the changes in
> >> the preceding commit.
> >> 
> >> We've thus solved the TODO item noted when "test_untraceable" was
> >> added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark
> >> as untraceable with '-x', 2018-02-24).
> >> 
> >> So let's remove the feature entirely. Not only is it currently unused,
> >> but it actively encourages an anti-pattern in our tests. We should be
> >> testing the output of specific commands, not entire subshells or
> >> functions.
> >> 
> >> That the "-x" output had to be disabled as a result is only one
> >> symptom, but even under bash those tests will be harder to debug as
> >> the subsequent check of the redirected file will be far removed from
> >> the command that emitted the output.
> >> 
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >>  t/README      |  3 ---
> >>  t/test-lib.sh | 66 ++++++---------------------------------------------
> >>  2 files changed, 7 insertions(+), 62 deletions(-)
> >> 
> >> diff --git a/t/README b/t/README
> >> index 29f72354bf1..3d30bbff34a 100644
> >> --- a/t/README
> >> +++ b/t/README
> >> @@ -86,9 +86,6 @@ appropriately before running "make". Short options can be bundled, i.e.
> >>  -x::
> >>  	Turn on shell tracing (i.e., `set -x`) during the tests
> >>  	themselves. Implies `--verbose`.
> >> -	Ignored in test scripts that set the variable 'test_untraceable'
> >> -	to a non-empty value, unless it's run with a Bash version
> >> -	supporting BASH_XTRACEFD, i.e. v4.1 or later.
> >>  
> >>  -d::
> >>  --debug::
> >> diff --git a/t/test-lib.sh b/t/test-lib.sh
> >> index 57efcc5e97a..b008716917b 100644
> >> --- a/t/test-lib.sh
> >> +++ b/t/test-lib.sh
> >> @@ -381,29 +381,6 @@ then
> >>  	exit
> >>  fi
> >>  
> >> -if test -n "$trace" && test -n "$test_untraceable"
> >> -then
> >> -	# '-x' tracing requested, but this test script can't be reliably
> >> -	# traced, unless it is run with a Bash version supporting
> >> -	# BASH_XTRACEFD (introduced in Bash v4.1).
> >> -	#
> >> -	# Perform this version check _after_ the test script was
> >> -	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
> >> -	# '--verbose-log', so the right shell is checked and the
> >> -	# warning is issued only once.
> >> -	if test -n "$BASH_VERSION" && eval '
> >> -	     test ${BASH_VERSINFO[0]} -gt 4 || {
> >> -	       test ${BASH_VERSINFO[0]} -eq 4 &&
> >> -	       test ${BASH_VERSINFO[1]} -ge 1
> >> -	     }
> >> -	   '
> >> -	then
> >> -		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
> >> -	else
> >> -		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
> >> -		trace=
> >> -	fi
> >> -fi
> >>  if test -n "$trace" && test -z "$verbose_log"
> >>  then
> >>  	verbose=t
> >> @@ -650,19 +627,6 @@ else
> >>  	exec 4>/dev/null 3>/dev/null
> >>  fi
> >>  
> >> -# Send any "-x" output directly to stderr to avoid polluting tests
> >> -# which capture stderr. We can do this unconditionally since it
> >> -# has no effect if tracing isn't turned on.
> >> -#
> >> -# Note that this sets up the trace fd as soon as we assign the variable, so it
> >> -# must come after the creation of descriptor 4 above. Likewise, we must never
> >> -# unset this, as it has the side effect of closing descriptor 4, which we
> >> -# use to show verbose tests to the user.
> >> -#
> >> -# Note also that we don't need or want to export it. The tracing is local to
> >> -# this shell, and we would not want to influence any shells we exec.
> >> -BASH_XTRACEFD=4
> >
> > Please do not remove BASH_XTRACEFD.  And especially not like this,
> > without even mentioning it in the commit message.
> 
> I can re-roll with an amended commit message that explicitly mentions
> it

It should rather be a separate patch...

>, but that doesn't address your "please do not remove",
> 
> Do you see reason to keep it at the end-state fo this series? E.g. a
> counter-argument to
> https://lore.kernel.org/git/211210.86pmq4daxm.gmgdl@evledraar.gmail.com/?

I don't see any argument pertinent to BASH_XTRACEFD in general in that
email, of in favor of its removal in particular, and there are no
valid arguments for its removal in earlier emails in this thread
either.