Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Bagas Sanjaya <bagasdotme@gmail.com>,
	Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
Subject: Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
Date: Thu, 26 May 2022 09:59:20 +0200	[thread overview]
Message-ID: <220526.86fskw7mbv.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqh75d2aif.fsf@gitster.g>

On Wed, May 25 2022, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +/* usage.c: if bug() is called we should have a BUG_if_bug() afterwards */
>>> +extern int bug_called_must_BUG;
>>
>> I am not sure about the name, ...
>
> I finally figured out why I found this name so disturbing.  This is
> written from the viewpoint of somebody who is trying to catch a
> programmer's error of calling bug() without calling BUG_if_bug();
> it is not named to help the users of API to understand it better.

I named it like that to indicate a "lesser bug", I think BUG_if_bug()
came later, because ...

> I wonder if it makes sense to make the call to BUG_if_bug() totally
> optional.  The rule becomes slightly different:
>
>  * You can call bug() zero or more times.  It issues a fatal error
>    message, and internally remembers the fact that we detected a
>    programming error that invalidates the execution of this program,
>    without immediately terminating the program.
>
>  * When you exit() from the program, the runtime consults that "did
>    we call bug()?" record and makes the program exit with known exit
>    code (we could arrange it to abort() just like BUG, but I'd
>    prefer controlled crash).
>
>  * But it is inconvenient to always keep going, after you may have
>    called one or more bug(), to the normal program completion.  So
>    there is a helper exit_if_called_bug(), which is an equivalent to
>    checking the "did we call bug()?" record and calling exit().
>
> By making BUG_if_bug() optional, we can lose a handful of lines from
> the test helper, because it makes it a non-issue to exit the program
> without calling it.

I don't think we should do it like that and keep it a BUG() not to call
BUG_if_bug() when we hit exit(), because e.g. in the case of
parse-options.c once we have the a bad "struct options" we don't want to
continue, as we might segfault, or have other bad behavior etc. So we'd
like to BUG() out as soon as possible.

That's how we use BUG() itself, i.e. we think the program execution is
bad and we immediately abort(), the new bug() makes a small concession
that we may be OK for a little while (e.g. while looping over the
options), but would like to BUG() out immediately after that.