Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 16, 2021, 13:04 UTC
Message-ID: <211216.864k78bsjs.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqczlx93kg.fsf@gitster.g>

[Replying to the thread-at-large]

On Tue, Dec 14 2021, Jeff King wrote:

Show 30 quoted lines

> On Mon, Dec 13, 2021 at 10:51:14AM -0800, Junio C Hamano wrote:
>
>> > I don't see any argument pertinent to BASH_XTRACEFD in general in that
>> > email, of in favor of its removal in particular, and there are no
>> > valid arguments for its removal in earlier emails in this thread
>> > either.
>> 
>> If I am reading Ævar right, the argument is "dash would not be fixed
>> with BASH_XTRACEFD, so there needs another way that would work there,
>> and if the approach happens to work also for bash, then there is no
>> reason to use BASH_XTRACEFD", I think.
>> 
>> Now, if the way Ævar came up with to help shells with "-x" not to
>> contaminate their standard error stream that our test scripts want
>> to inspect is worse to write, understand, and maintain, compared to
>> the way we have been writing our tests that inspect their standard
>> errors, without having to worry about "-x" output thanks to the use
>> of BASH_XTRACEFD, it may make a regression to developer
>> productivity, but I am not sure if that is the case.
>
> I think the method for handling this in the test scripts _is_ worse to
> write, understand, and maintain. The problem to me is less that it's
> ugly to workaround (which as you note in this case is not great, but not
> _too_ bad), but that it's a subtle friction point that may jump up and
> bite any test-writer who does something like:
>
>   (foo && bar) 2>stderr
>
> So my view had always been that BASH_XTRACEFD is the good
> solution[...]

Yes, I agree that's much better than what you need to do when not under bash and when you can't benefit from BASH_XTRACEFD.

But unless we're talking about requiring bash and/or not supporting -x at all (which seems to be overkill, seeing as only one test scipt wasn't compatible with it) this discussion seems a bit like talking about how some code is nicer in C17 than C99 or C89.

Sure, it is. But if you're also supporting the latter two it's usually not worth it to maintain both with an ifdef.

Similarly, we can't really get any real benefits from BASH_XTRACEFD as long as we're going to support "-x" with other shells.

Literally the only part of the test suite where we hard-depended on it is the code adjusted in my 1/2 here. I do think if we could rely on the pre-image it would be nicer, but not so nice that I don't want "-x" working under "dash".

And in any case carrying a "BASH_XTRACEFD" seems to be just a dead end for that reason.

On Wed, Dec 15 2021, Junio C Hamano wrote:

Show 18 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> On Wed, Dec 15, 2021 at 09:05:15AM -0800, Junio C Hamano wrote:
>>
>>> Still.  I wonder if keeping BASH_XTRACEFD helps developers, when
>>> they need to diagnose a new breakage?  If their new test fails only
>>> in the "dash -x" run but not "bash -x" at the CI, for example?
>>
>> I have done that, but usually only after realizing that "./t1234-foo.sh"
>> passes and "./t1234-foo.sh -x" does not. So I think just tweaking use of
>> "-x" would be the main tool.
>
> Ah, that's true.  You only need to compare "sh -x test.sh" with "sh
> test.sh" with any value of "sh", especially after BASH_XTRACEFD is
> removed, demoting "bash" to the same state as "dash" wrt "-x".
>
> OK, I am more OK with the removal of BASH_XTRACEFD support than
> before ;-)

Yes, I've run into those cases and I don't think BASH_XTRACEFD really helps at all. Yes you'll get a test failure because trace output got into something we're invoking "test_cmp" on, or whatever.

But that's never a subtle failure, and the fix for it brings us back to the question of whether we're going to support non-bash, or take the more drastic step of marking the whole test script as bash-only under "-x".

I think the answers to those are always going to be "yes" and "no", and thus we're not getting any benefit from "BASH_XTRACEFD".