Re: [PATCH v4 3/3] test-lib.sh: remove "BASH_XTRACEFD"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 22, 2022, 15:14 UTC
Message-ID: <220222.86h78q7wc4.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220221231121.GB1658@szeder.dev>

On Tue, Feb 22 2022, SZEDER Gábor wrote:

Show 39 quoted lines

> On Mon, Dec 13, 2021 at 02:38:36AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Stop setting "BASH_XTRACEFD=4" to direct "-x" output to file
>> descriptor 4 under bash.
>> 
>> When it was added in d88785e424a (test-lib: set BASH_XTRACEFD
>> automatically, 2016-05-11) it was needed as a workaround for various
>> tests that didn't pass cleanly under "-x".
>> 
>> Most of those were later fixed in 71e472dc43 (Merge branch
>> 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the
>> final remaining and removed the "test_untraceable" facility.
>
> Those commits made the test suite pass with '-x' without BASH_XTRACEFD
> only when all went well, but during development that's often not the
> case.  So let's not forget about c5c39f4e34 (test-lib: fix interrupt
> handling with 'dash' and '--verbose-log -x', 2019-03-13), before which
> dash was not really suitable to run tests involving daemon processes
> with '-x' during development.  If dash were to announce redirections
> in its '-x' trace, like many not as quite as popular shells do, then
> the workaround in that commit wouldn't work at all.
>
> In general, between POSIX leaving a lot of things explicitly
> unspecified, or, worse, silently unspecified, shells not conforming to
> POSIX, being buggy, and/or implementing their own extensions, I am
> actually quite surprised that it works as well as it does with so many
> shell.  At least as far as we know it, and I wouldn't at all be
> surprised if there were unknown issues lurking in some corner cases
> and/or with some more exotic shells.
>
>> We could retain "BASH_XTRACEFD=4" anyway, but doing so is bad because:
>> 
>>  1) Since we're caring about "-x" passing in CI under "dash" on Ubuntu
>>     using "BASH_XTRACEFD=4" will amount to hiding an error we'll run
>>     into eventually. Tests will pass locally with "bash", but fail in
>>     CI with "dash" (or under other non-"bash" shells).
>
> This is not "bad", this is exactly what we use CI for.  This is the
> smae case as when the test suite passes on a developer's Linux box,
> but breaks on OSX or Windows in CI.

Yes we could definitely live with it. My aim here was to tighten up the cycle of testing, submitting/pushing a patch, having CI fail because that test was on bash only etc.

Show 21 quoted lines

> Furthermore, while I fully agree that keeping the whole test suite
> passing with '-x' without BASH_XTRACEFD is desirable, I do think it's
> a bad idea to forbid developers from using it while hacking away to
> scratch their itches.  I for one sometimes deliberately use various
> bashisms in my tests, including 'test_cmp'-ing the stderr of loops and
> functions, because they make writing tests then and there easier, when
> at that point I'd rather focus my attention on getting the C changes
> right, and clean them up eventually when I deem the changes worthy for
> submission.
>
>
> Overall I consider this patch as a cleanup solely for cleanup's sake,
> without any benefits at all.
>
> I'm kind of low on time myself as well, at least to argue about this
> any further.  Therefore, as the one who did the vast majority of work
> to make '-x' work even without BASH_XTRACEFD, I leave here my firm:
>
>   Not-Acked-By: SZEDER Gábor <szeder.dev@gmail.com>
>
> to any patch that attempts to remove support for BASH_XTRACEFD.

Sure, if you feel that strongly about it I'm fine with dropping this version and submitting a re-roll without the test-lib.sh changes.

I just genuinely didn't grok why you wanted to keep it before.

But I get it now, thanks.

I think it's fair to say that it's for the exact same reason I'd like to get rid of it, you just think of it as a feature, not a bug :)

I.e. the "landmine" of getting something working under bash, only to find out later that it fails on other shells.

But everyone's workflow is different. It sounds as though you sometimes start hacking tests to use bash-specific features that you later distill down to POSIX-compatibility.

Which I don't think I've ever done, since doing so is going to require a rewrite anyway I might as well struggle to get it POSIX-ly working in the first place.

But I can see how that's useful, and I don't want to take that away from you.

Do you feel equally strongly that this mode of operation must be the default under bash, or that it would be OK to have it be opt-in under a knob like GIT_TEST_BASHISMS=true, or something like that?

The reason I ran into this in the first place was because I saw such unexplained behavior changes with bash v.s. non-bash, only to arrive at this part of test-lib.sh (which I'm sure I'd read N times before, but had forgotten about).

I don't want to make your local workflow harder, but I'd think it would be fair to say that the average git.git developer won't be as keenly aware of shell differences as you, and would be better served by having this subtlety be opt-in.

I.e. it's surely more likely that someone is to write POSIX-compatible test code for ML submission, than intentionally using a bash-only feature for prototyping, which will start breaking once it's exposed to dash et al in CI.

Or, if you feel strongly about that too I wouldn't mind much if "GIT_TEST_BASHISMS=true" was the default, as long as I can set it to "GIT_TEST_BASHISMS=false" and /other/ bash-specific behavior without further opting it in to optional bash-only behavior that's off by default.

Assuming you'd reply "no" to not wanting such a knob *and* having it by off by default, would that be a "yes" to having the knob and needing to turn it off?

Thanks!