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

On Tue, Nov 30 2021, 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. 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.

I'd be fine with a hard dependency on bash, but that seems unlikely to
happen, and if that's not the case having these hybrid modes seems like
the worst of both worlds.

We already hard depend on -x working without BASH_XTRACEFD for all tests
except this one test don't we? I.e our CI runs with --verbose-log -x,
and t1510-repo-setup.sh was the only test that was turning it off, and
some of that runs with a /bin/sh of "dash".

Now when that test fails we'll get nice -x output like for every other
test, and AFAICT nothing else is changed by this patch, except the
obvious change of us not having to support these two modes anymore.