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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 1, 2018, 11:04 UTC
Message-ID: <nycvar.QRO.7.76.6.1805011257400.79@tvgsbejvaqbjf.bet>
In-Reply-To: <7087f0b9-1362-f8ca-315d-96d27b91b26b@kdbg.org>

Hi Hannes,

On Mon, 30 Apr 2018, Johannes Sixt wrote:

Show 18 quoted lines

> Am 30.04.2018 um 00:17 schrieb Johannes Schindelin:
> > 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.
> >
> > [...]
> >
> >   test_expect_success 'create-reflog() not allowed' '
> > -	test_must_fail $RUN create-reflog HEAD 1
> > +	test_must_fail ok=sigabrt $RUN create-reflog HEAD 1
> >   '
> 
> I can't quite follow the rationale for this change. A 'BUG' error exit must
> never be reached, otherwise it is a bug in the program by definition. It
> cannot be OK that SIGABRT is a valid result from Git.

I thought so at first, too. However, what these test cases run is not Git itself. Instead, they run a t/helper/ command *specifically* designed to hit the BUG code path, probably to ensure that bugs in future code will actually not be silently ignored, but do exit with an error.

> If SIGABRT occurs as a result of BUG(), and we know that this happens for
> certain cases, it means we have an unfixed bug.

Not in this case: The code in question is in https://github.com/git/git/blob/v2.17.0/t/helper/test-ref-store.c#L190-L201 and it is called in a way that fails to have the required flags for the operation. This would normally indicate a bug, but in this case, that is exactly what the regression test tries to trigger: we *want* such a bug to cause a failure.

I'll do my best to clarify that in the commit message.

Ciao, Dscho