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

[Christian Couder <christian.couder@gmail.com>]

On Wed, Dec 9, 2020 at 2:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > 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.

Another solution could be to define a command_not_found_handle
function as bourne shells should call that.

By the way it's not the first time we get such an issue, see:

https://lore.kernel.org/git/CAP8UFD15+p+xKwJ=B9WVsrc+2TvLHKmu78SBCLUFZVSYoTtbbg@mail.gmail.com/