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