Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > On Mon, Dec 13 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
