Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

On Tue, Nov 30 2021, SZEDER Gábor wrote:

On Tue, Nov 30, 2021 at 04:08:48PM -0500, Jeff King wrote:

quoted

On Mon, Nov 29, 2021 at 09:13:23PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

Once that's been removed we can dig deeper and see that we only have
"BASH_XTRACEFD" due to an earlier attempt to work around the same
issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,
2016-05-11) and the 90c8a1db9d6 (test-lib: silence "-x" cleanup under
bash, 2017-12-08) follow-up.

I.e. we had redirection in "test_eval_" to get more relevant trace
output under bash, which in turn was only needed because
BASH_XTRACEFD=1 was set, which in turn was trying to work around test
failures under "set -x".

It's better if our test suite works the same way on all shells, rather
than getting a passing run under "bash", only to have it fail with
"-x" under say "dash". As the deleted code shows this is much simpler
to implement across our supported POSIX shells.

I'm mildly negative on dropping BASH_XTRACEFD.

I agree, using BASH_XTRACEFD is the most robust (and least hacky) way
to get reliable trace from our test scripts, and we should definitely
keep it.

quoted

IMHO it is not worth the
maintenance headache to have to remain vigilant against any shell
function's stderr being examined, when there is single-line solution
that fixes everything. Yes, the cost of using bash is high on some
platforms, but "-x" is an optional feature (though I am sympathetic to
people who are _surprised_ when "-x" breaks things, because it really is
a subtle thing, and knowing "you should try using bash" is not
immediately obvious).

Some folks (like Gábor) disagree and have done the work so far to make
sure that we pass even with "-x". But it feels like this is committing
the whole project to that maintenance. I dunno.

It's not that I disagree but rather it's in our best interest to keep
'-x' working with non-Bash shells as well, because:

  - We run our tests with '-x' in CI, because we want to get as much
    information out of test failures as possible.

  - We run our tests with dash in the Ubuntu jobs not only because
    that's the default /bin/sh, but more importantly because we want
    to avoid bashisms in our test suite.

I don't really mind keeping BASH_XTRACEFD if it's doing something
useful, but I feel like I'm missing something here. Is it really doing
something useful?

AFAICT the ony case where it mattered was t1510-repo-setup.sh, which
with my upthread
[ref] now works with
-x, at the trivial cost of skipping a small bi of the test with -x.

I suppose we could move this BASH_XTRACEFD to tht file in particular if
anyone feel strongly about the trivial loss of tracing that entails. I
figured just skipping it under "-x" and adding a "say" to that effect
was a better trade-off.

For the rest of the test suite BASH_XTRACEFD effectively didn't matter,
since all our tests had to work under --verbose-log -x anyway under
dash.

Am I just wrong about this line of thinking, or is it purely that you
two would like to keep BASH_XTRACEFD in case some hypothetical future
caller wants to make use of "test_untraceable=UnfortunatelyYes" again?