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

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

On Thu, Dec 09 2021, Glen Choo wrote:

quoted

Replace exit() calls in branch.c that have questionable exit codes:

* in setup_tracking(), exit(-1) was introduced in 27852b2c53 (branch:
  report errors in tracking branch setup, 2016-02-22). This may have
  been a mechanical typo because the same commit changes the return type
  of setup_tracking() from int to void.

* in validate_branch_start(), the exit code changes depending on whether
  or not advice is enabled. This behavior was not discussed
  upstream (see caa2036b3b (branch: give advice when tracking
  start-point is missing, 2013-04-02)).

Signed-off-by: Glen Choo <redacted>
---
I don't know what the 'correct' exit codes should be, only that Junio
makes a good case that the existing exit codes are wrong. My best,
non-prescriptive, choice is 128, to be consistent with the surrounding
code and Documentation/technical/api-error-handling.txt.

 branch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/branch.c b/branch.c
index 305154de0b..ad70ddd120 100644
--- a/branch.c
+++ b/branch.c

@@ -324,7 +324,7 @@ static void validate_branch_start(struct repository *r, const char *start_name,
 			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
 				error(_(upstream_missing), start_name);
 				advise(_(upstream_advice));
-				exit(1);
+				exit(128);
 			}
 			die(_(upstream_missing), start_name);
 		}

@@ -398,7 +398,7 @@ void setup_tracking(const char *new_ref, const char *orig_ref,
 		string_list_append(tracking.srcs, full_orig_ref);
 	if (install_branch_config_multiple_remotes(config_flags, new_ref, tracking.remote,
 			      tracking.srcs) < 0)
-		exit(-1);
+		exit(128);
 
 cleanup:
 	string_list_clear(tracking.srcs, 0);

Junio noted in <xmqqbl1tcptq.fsf@gitster.g>:
    
    This is not a problem with this patch, and it should not be fixed as
    part of this series, but since I noticed it, I'll mention it as a
    leftover low-hanging fruit to be fixed after the dust settles.  The
    exit(1) looks wrong.  We should exit with 128 just like die() does.
    Issuing of an advice message should not affect the exit code.

I think it's good to fix these inconsistencies, but also that we
shouldn't be doing it as part of this series, or does it conflict in
some way that's hard to untangle?

There isn't any conflict. Probably a leftover habit from previous
projects, but I thought that this would be right time to clean up. Looks
like we think it'll be better to clean this up in a separate series, so
I'll do that instead.

FWIW the former hunk is a perfect candidate for the new die_message()
function[1]. I.e. we should be doing:

    int code = die_message(_(upsream_missing), start_name);
    if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE))
        advise(_(upstream_advice));
    exit(code);

That we print an "error" when giving the advice but "fatal" when not is
really UX wart, and also that the exit code differs.

Ah, thanks!

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

Thanks, this is exactly what I was looking for guidance on.
Documentation/technical/api-error-handling.txt is silent on what exit
code to use when a command does 90% of what the caller wants (so it's
not really an application error) but fails on the 10% that the user
doesn't care so much about - in this case, creating a branch but failing
to setup tracking.

FWIW I had some local hacks to detect all these cases of exit -1 via
the test suite, they're almost all cases where we want to exit with 1,
but just conflated an error() return value with a return from main()
(or exit).

Yes, this sounds like what happened here.