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

["SZEDER Gábor" <szeder.dev@gmail.com>]

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.

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.