On Tue, Nov 30 2021, Jeff King wrote: > On Mon, Nov 29, 2021 at 09:13:23PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> 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.
