Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 8, 2020, 22:17 UTC
Message-ID: <xmqqh7ow54eb.fsf@gitster.c.googlers.com>
In-Reply-To: <xmqqsg8g559i.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

> Josh Steadmon <steadmon@google.com> writes:
>
>> Caught a typo here, sending this as a squash patch since it's already in
>> next:
>
> The breakage and the fix looks obvious to me, but...
>
> How did CI allow 'next' to pass with such a typo, I wonder?
> Moreover, my pre-push tests of all the integration branches
> I didn't notice this to fail, but I cannot see how it could
> have been succeeded.  Puzzled...

That is because of this:

    $ (sh t7900-maintenance.sh 2>&1; echo $?) | tail -5
    ok 25 - register preserves existing strategy
    t7900-maintenance.sh: line 444: test_execpt_success: command not found
    # passed all 25 test(s)
    1..25
    0

The story is the same with prove.

    $ prove t7900-maintenance-sh
    t7900-maintenance.sh .. 24/? t7900-maintenance.sh: line 444: test_execpt_success: command not found
    t7900-maintenance.sh .. ok
    All tests successful.
    Files=1, Tests=25,  2 wallclock secs ( 0.02 usr  0.01 sys +  0.97 cusr  0.97 csys =  1.97 CPU)
    Result: PASS

Since this typo appeared immediately before test_done, we _could_ improve test_done to pay attention to $? when it starts (and in a similar fashion, we _could_ also check $? at the beginning of the test_expect_* for the previous step), but I do not think that is a good approach that would scale well. There are legitimate reasons we have to write things other than test_expect_* at the top level of the script (e.g. test helper function may have to be defined to be shared amongst the test pieces in the same script).

I wonder if it is a good direction to go to run the tests with the "set -e" option on, and accept its peculiarities.