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

From: Junio C Hamano <[email-masked]>
Date: Mon, 13 Dec 2021 11:26:29 -0800

Ævar Arnfjörð Bjarmason <[email-masked]> writes:

> On Mon, Dec 13 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <[email-masked]> 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.

Only when they are not calling die() for some technical reasons,
though.  IOW, if you would have called die() if you could, that is a
good indication that you would want to consistently use 128.

Capturing return value from die_message(), giving more message and
then dying with the 128 squarely falls into that pattern.  I am not
sure if the install_branch_config_multiple_remotes() case falls into
it, though.

And more importantly in the context of this topic, I am not
convinced install_branch_config_multiple_remotes() helper function
itself is a good idea to begin with.  It is to handle a case where
branch.$name.remote is set multiple times right?

This is because I do not think I saw anybody defined the right
semantics during the discussion (or written in the documentation)
and explained why being able to do so makes sense in the first
place, and it is not known if it makes sense to copy such a
configuration blindly to a new branch.  If it punts without doing
anything with a warning(), or calls a die(), it would be a more
sensible first step for this topic.  Users with real need for such a
configuration will then come to us with real use case, and what they
need may turn out to be something different from a blind copying of
the original.

Thanks.