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

From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Taylor Blau" <me@ttaylorr.com>
Subject: Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD
Date: Tue, 30 Nov 2021 23:44:35 +0100	[thread overview]
Message-ID: <20211130224435.GA1991@szeder.dev> (raw)
In-Reply-To: <YaaS4Idhdyo2wZ9q@coredump.intra.peff.net>

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.