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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Hannes,

On Mon, 30 Apr 2018, Johannes Sixt wrote:

> 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