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

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

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.

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.