Re: [PATCH v3 5/5] branch.c: replace questionable exit() codes

On Mon, Dec 13 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

The latter should really be "exit(1)", not 128. We should reserve that
for die().

Is it because install_branch_config_multiple_remotes() gives enough
information to the user that the caller exits without its own
message?  In other words, are messages given by the callee to the
users are morally equivalent to what the caller would call die()
with, if the callee were silent?  If so, 128 is perfectly fine.  If
not, 1 or anything positive that is not 128 may be more appropriate.

We don't really document this outside of this tidbit:
    
    Documentation/technical/api-error-handling.txt-- `die` is for fatal application errors.  It prints a message to
    Documentation/technical/api-error-handling.txt:  the user and exits with status 128.
    Documentation/technical/api-error-handling.txt-
    Documentation/technical/api-error-handling.txt-- `usage` is for errors in command line usage.  After printing its
    Documentation/technical/api-error-handling.txt-  message, it exits with status 129.  (See also `usage_with_options`
    Documentation/technical/api-error-handling.txt-  in the link:api-parse-options.html[parse-options API].)

But while that doesn't say that you can *only* use 128 for die, and I
wouldn't consider the existing code that calls exit(128) in dire need of
a change, most of the builtins simply return with 1 for generic errors,
and reserve 128 for die()..

So for any new code it makes sense to follow that convention.

Either case, -1 is a definite no-no.

I've got a local WIP patch to fix those that I can polish up, if you're
interested. It's the result of adding the below & running the test suite
against it:

diff --git a/git.c b/git.c
index 60c2784be45..d6bdb3571df 100644
--- a/git.c
+++ b/git.c

@@ -419,6 +419,7 @@ static int handle_alias(int *argcp, const char ***argv)
 static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 {
 	int status, help;
+	int posix_status;
 	struct stat st;
 	const char *prefix;
 

@@ -459,6 +460,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 	validate_cache_entries(the_repository->index);
 	status = p->fn(argc, argv, prefix);
+	posix_status = status & 0xFF;
+	if (status != posix_status)
+		BUG("got status %d which will be cast to %d, returning error() perhaps?", status, posix_status);
 	validate_cache_entries(the_repository->index);
 
 	if (status)