Re: [PATCH 00/41] use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit status

On Mon, Mar 21 2022, Elia Pinto wrote:

On existing use: That's quite the overstatement :)

We use EXIT_{SUCCESS,FAILURE} only in:

 * contrib/credential/ code.
 * sh-i18n--envsubst.c
 * EXIT_FAILURE in one stray test helper

So out of "real git" that users see only sh-i18n--envsubst.c will ever run by default, and the reason it uses these is because it's as-is imported GNU code.

I'd think if anything we'd be better off doing this the other way around, and always hardcoding either 0 or 1.

I'm not aware of any platform where EXIT_SUCCESS is non-zero, although that's probably left open by the C standard.

For EXIT_FAILURE there *are* platforms where it's non-1, but I don't know if we're ported to any of those, e.g. on z/OS it's[1]:

    The argument status can have a value from 0 to 255 inclusive or be
    one of the macros EXIT_SUCCESS or EXIT_FAILURE. The value of
    EXIT_SUCCESS is defined in stdlib.h as 0; the value of EXIT_FAILURE
    is 8.

Now, I don't know z/OS at all, but e.g. if a shellscripts calls a C program there would $? be 1 if we hardcode 1, but 8 on EXIT_FAILURE?

We also document for some of these programs that on failure we'll return 1 specifically, not whatever EXIT_FAILURE is.

These patches also miss cases where we'll set 0 or 1 in a variable, and then exit(ret). See e.g. builtin/rm.c. You just changed the hardcoded exit(1), but missed where we'll return a hardcoded 0 or 1 via a variable.

And then there's changing exit(-1) to exit(1). That's existing non-portable use that we really should fix. But I know that you missed a lot there, since I instrumented git.c recently to intercept those for testing (it came up in some thread). We have a lot more than you spotted (and some will error if mapped to 1 IIRC). Most of those also want to exit 128, not 1.

Anyway:

All in all I think we should just double down on the hardcoding instead, but we should fix the exit(-1) cases, and that's best done with some new GIT_TEST_ASSERT_NO_UNPORTABLE_EXIT testing or whatever.

A lot of these codepaths are also paths we should fix, but not because we exit(N) with a hardcoded N, but because we invoke exit(N) there at all. See 338abb0f045 (builtins + test helpers: use return instead of exit() in cmd_*, 2021-06-08) for how some of those should be changed.

I think we'd be much better off with something like this in git-compat-util.h:

    #ifndef BYPASS_EXIT_SANITY
    #ifdef EXIT_SUCCESS
    #if EXIT_SUCCESS != 0
    #error "git assumes EXIT_SUCCESS is 0, not whatever yours is, please report this. Build with -DBYPASS_EXIT_SANITY to continue building at your own risk"
    #endif
    #endif
    #ifdef EXIT_FAILURE
    #if EXIT_FAILURE != 0
    #error "git assumes EXIT_FAILRE is 1, not whatever yours is, please report this. Build with -DBYPASS_EXIT_SANITY to continue building at your own risk"
    #endif
    #endif
    #endif

Or *if* we're going to pursue this a twist on that (I really don't think this is worthwhile, just saying) where we'd re-define EXIT_SUCCESS and EXIT_FAILURE to some sentinel values like 123 and 124.

Then run our entire test suite and roundtrip-assert that at least we ourselves handled that properly. I.e. whenever run_command() runs and we check for success we check 123, not 0, and a "normal failure" is 124, not 1.

I know we'll get a *lot of* failures if we do that, so I'm not arguing that we *should*, just that it's rather easy for you to test that and see the resulting test suite dumpster fire.

So I don't see how a *partial conversion* is really getting us anywhere, even if we take the pedantic C portability view of things.

All we'd have accomplished is a false sense of portability on most OS's, as these will be 0 and 1 anyway. And on any stray odd OS's like z/OS we'll just need to deal with e.g. both 1 and 8 for EXIT_FAILURE, since we *will* miss a lot of cases.

1. https://www.ibm.com/docs/en/zos/2.1.0?topic=functions-exit-end-program