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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 1, 2021, 14:06 UTC
Message-ID: <211201.868rx4beer.gmgdl@evledraar.gmail.com>
In-Reply-To: <20211130224435.GA1991@szeder.dev>

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

Show 47 quoted lines

> 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@gmail.com> 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?