On Mon, Mar 21 2022, Elia Pinto wrote:
> EXIT_SUCCESS or EXIT_FAILURE are already used in some functions in git but
> not everywhere. Also in branch.c there is a returns exit(-1), ie 255, when
> exit(1) might be more appropriate.
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
