git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Elia Pinto <gitter.spiros@gmail.com>
Date
Mar 22, 2022, 17:47 UTC
Message-ID
<CA+EOSBmKue=z98NnCPENN3uc6NzXXJ7kBBJB8cJ0k_WRozX1sg@mail.gmail.com>
In-Reply-To
<220322.86r16unzer.gmgdl@evledraar.gmail.com>

Il giorno mar 22 mar 2022 alle ore 09:51 Ævar Arnfjörð Bjarmason <avarab@gmail.com> ha scritto:

>
First of all, thanks for the review.
Show 9 quoted lines
>
> 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.

Show 15 quoted lines
> 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

Show 9 quoted lines
>
> 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

Show 7 quoted lines
>
> 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.

Show 48 quoted lines
>
> 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
Previous: Ævar Arnfjörð BjarmasonNext: Junio C Hamano
Message 47 of 48 in “use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit status”
  1. 00/41 use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  2. 07/41 help.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  3. 13/41 rebase.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  4. 34/41 setup.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  5. 05/41 commit.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  6. 02/41 branch.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  7. 40/41 upload-pack.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  8. 32/41 remote-curl.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  9. 27/41 git.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  10. 26/41 daemon.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  11. 41/41 exit.cocci: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  12. 01/41 archive.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  13. 24/41 git-credential-osxkeychain.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  14. 20/41 tag.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  15. 09/41 mailsplit.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  16. 38/41 test-submodule-config.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  17. 11/41 merge.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  18. 17/41 shortlog.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  19. 12/41 pull.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  20. 37/41 test-reach.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  21. 33/41 run-command.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  22. 16/41 rm.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  23. 31/41 path.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  24. 04/41 blame.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  25. 15/41 rev-parse.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  26. 29/41 http-backend.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  27. 18/41 show-branch.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  28. 03/41 am.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  29. 36/41 test-json-writer.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  30. 28/41 help.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  31. 39/41 test-submodule-nested-repo-config.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  32. 06/41 credential-cache--daemon.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  33. 14/41 remote-ext.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  34. 10/41 merge-index.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  35. 25/41 git-credential-wincred.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  36. 22/41 update-index.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  37. 08/41 init-db.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  38. 23/41 obstack.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  39. 21/41 unpack-objects.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  40. 30/41 parse-options.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  41. 19/41 stash.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  42. 35/41 shell.c: use the stdlib EXIT_SUCCESS or EXIT_FAILURE exit statusElia Pinto, Mar 21, 2022
  43. Bagas SanjayaMar 22, 2022
  44. Elia PintoMar 22, 2022
  45. Bagas SanjayaMar 22, 2022
  46. Ævar Arnfjörð BjarmasonMar 22, 2022
  47. Elia PintoMar 22, 2022
  48. Junio C HamanoMar 23, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.