On Tue, Nov 30 2021, SZEDER Gábor wrote: > 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: >> >> > 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. 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 <patch-1.1-9f735bd0d49-20211129T200950Z-avarab@xxxxxxxxx> 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?
