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