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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Glen Choo <chooglen@google.com>,
	git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>,
	Josh Steadmon <steadmon@google.com>,
	Emily Shaffer <emilyshaffer@google.com>,
	Philippe Blain <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH v3 5/5] branch.c: replace questionable exit() codes
Date: Mon, 13 Dec 2021 10:19:16 +0100	[thread overview]
Message-ID: <211213.8635mwamah.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqq5yrsg9n2.fsf@gitster.g>


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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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)
