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

[Junio C Hamano <gitster@pobox.com>]

Duy Nguyen <pclouds@gmail.com> writes:

> 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).