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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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, 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/?