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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Dec 09 2021, Glen Choo wrote:

> 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 <chooglen@google.com>
> ---
> 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?

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.

The latter should really be "exit(1)", not 128. We should reserve that
for die(). 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).

1. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20211207T182419Z-avarab@gmail.com/#t