Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 3, 2022, 23:05 UTC
Message-ID: <220604.8635glxs3g.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqczfpe9ua.fsf@gitster.g>

On Fri, Jun 03 2022, Junio C Hamano wrote:

Show 21 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> However, in doing so we've been fooling ourselves when it comes to
>>> what trace2 events we log. The events tested for in
>>> 0a9dde4a04c (usage: trace2 BUG() invocations, 2021-02-05) are not the
>>> real ones, but those that we emit only from the "test-tool".
>>
>> I can fully agree with the above reasoning, i.e. let's test what we
>> do use in production, instead of something nobody uses for real, if
>> we were adding a test for BUG() in vacuum, but why did we have to
>> "fake" it in the first place?
>> ...
>> Are we sure that the reason no longer applies?  How do we know?  We
>> would want to explain that to future developers in the proposed log
>> message, I would think.
>
> We can flip it the other way around.  
>
> I do not think I ever saw anybody asked anybody on this list who got
> a BUG() message to use the coredump to do something useful.  Don't
> modern distros ship with "ulimit -c 0" these days?

I think that part of it was just a side-effect of SIGABRT.

> It might be possible that a better direction is to introduce
> GIT_ABORT_ON_BUG environment or core.abortOnBUG configuration that
> chooses between abort() and exit(99), or something like that, and
> then we switch to use the latter by default over time?

I think the reason for abort() was what's covered in raise(3), i.e. it's a one-stop-shop to getting "stop it" behavior both under threading and non-threading, which as e.g. exit(3) discusses wouldn't be thread-safe with it.

But perhaps that was all premature worrying, we're mostly running non-threaded, and to the extent that we ever BUG() running into exit(3) threading issues is probably the least of our worries.

So perhaps we should drop abort() entirely, I don't know. These proposed patches tried not to do that, but just to log things when we did so.