Il giorno mar 22 mar 2022 alle ore 09:51 Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> ha scritto:
>
First of all, thanks for the review.
>
> 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 :)
>
It was not a quantitative assessment. I just wanted to point out that
the macros stdlib.h EXIT_SUCCESS and EXIT_FAILURE already exist in the
git code.
> 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.
No. It is defined be 0
https://pubs.opengroup.org/onlinepubs/009604599/basedefs/stdlib.h.html
>
> 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.
>
EXIT_FAILURE it is not defined what precise value it has by the standard C.
However linux, aix, solaris and windows define it as "1". Only Z / OS
calls it 8 but I'm sure git doesn't care about it.
Z/OS
https://www.ibm.com/docs/en/zos/2.1.0?topic=functions-exit-end-program
SOLARIS
https://gitlab.anu.edu.au/mu/x-lcc/blob/24be447de544ed06d490ca0b2304a6531362156a/include/sparc/solaris/stdlib.h
AIX
https://www.rpi.edu/dept/acm/packages/egcs/1.1.2/rs_aix42/lib/gcc-lib/powerpc-ibm-aix4.3.1.0/egcs-2.91.66/include/stdlib.h
WINDOWS
https://docs.microsoft.com/it-it/cpp/c-runtime-library/exit-success-exit-failure?view=msvc-170
> 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?
See the previous answer
>
> We also document for some of these programs that on failure we'll return
> 1 specifically, not whatever EXIT_FAILURE is.
>
See the previous answer.
EXIT_FAILURE is always 1 on all popular platforms that git has been
ported to. So you don't even have to change the documentation.
> 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.
My patch was just meant to introduce some standardization into git
using the posix/c standard. No more, No less. As other major projects
do, I didn't invent anything.
SYSTEMD COCCI
https://github.com/systemd/systemd/blob/main/coccinelle/exit-0.cocci
Lxc cocci
https://github.com/lxc/lxc/blob/master/coccinelle/exit.cocci
>
> 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.
In fact, these exit codes are more like shell-specific return codes to
indicate the "type" of the error.
I repeat that it was not the purpose of this patch to fix any problems
that may exist with exit codes. Certainly not using coccinelle. But I
agree it's a job to do. But not in this patch.
>
> 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.
Z / OS is a false problem. git on z / os runs in a linux partition
https://medium.com/theropod/git-on-z-os-f9234cd2a89a#:~:text=On%20z%2FOS%2C%20Git%20plays,added%20feature
% 20of% 20codepage% 20translation.
However, calling pedantic a solution widely used in other projects and
provided by standards (EXIT_SUCCESS and EXIT_FAILURE are reported by
the c/posix standard for generic success / error codes) does not seem
to me an appropriate term. But YMMV .
Thanks
>
> 1. https://www.ibm.com/docs/en/zos/2.1.0?topic=functions-exit-end-program
