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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 10, 2021, 02:21 UTC
Message-ID: <211210.86ee6ldwlc.gmgdl@evledraar.gmail.com>
In-Reply-To: <20211209184928.71413-6-chooglen@google.com>

On Thu, Dec 09 2021, Glen Choo wrote:

Show 44 quoted lines

> 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