Re: [PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

From: Junio C Hamano <gitster@pobox.com>
Date: May 2, 2018, 03:40 UTC
Message-ID: <xmqqvac6wwrw.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CACsJy8A7K6-W=H_08JcJgtziz3aQ4B1WgOcsoMSMuSvEQDW8=A@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 17 quoted lines

> On Mon, Apr 30, 2018 at 12:17 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> t1406 specifically verifies that certain code paths fail with a BUG: ...
>> message.
>>
>> In the upcoming commit, we will convert that message to be generated via
>> BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
>> regular exit code.
>
> On the other hand, SIGABRT on linux creates core dumps. And on some
> setup (like mine) core dumps may be redirected to some central place
> via /proc/sys/kernel/core_pattern. I think systemd does it too but I
> didn't check.
>
> This moving to SIGABRT when we know it _will_ happen when running the
> test suite will accumulate core dumps over time and not cleaned up by
> the test suite. Maybe keeping die("BUG: here is a good compromise.

I do not think it is. At regular runtime, we _do_ want Git to dump core if it triggers BUG() condition, whose point is to mark conditions that should never happen.

As discussed in this thread, tests that use t/helper/ executables that try to trickle BUG() codepath to ensure that these "should never happen" conditions are caught do need to deal with it. If dumping core is undesirable, tweaking BUG() implementation so that it becomes die("BUG: ...") *ONLY* when the caller knows what it is doing (e.g. running t/helper/ commands) is probably a good idea. Perhaps GIT_TEST_OPTS can gain one feature "--bug-no-abort" and set an environment variable so that implementation of BUG() can notice, or something.

When we are testing normal parts of Git outside t/helper/, we never want to hit BUG(). Aborting and dumping core when that happens is an desirable outcome. From that point of view, the idea in 1/6 of this patch series to annotate test_must_fail and say "we know this one is going to hit BUG()" is a sound one. The implementation in 1/6 to treat SIGABRT as an acceptable failure needs to be replaced to instead use the above mechanism you would use to tell BUG() not to abort but die with message to arrange that to happen before running the git command (most likely something from t/helper/) under test_must_fail ok=sigabrt; and then those who regularly break their Git being tested (read: us devs) and hit BUG() could instead set the environment variable (i.e. internal implementation detail) manually in their environment to turn these BUG()s into die("BUG:...)s while testing their early mistakes if they do not want core (of course, you could just do "ulimit -c", and that may be simpler solution of your "testing Git contaminates central core depot" issue).