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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxx/?
