| Message ID | 20211209184928.71413-6-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | implement branch --recurse-submodules | expand |
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
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@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? 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.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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. Either case, -1 is a definite no-no.
On Mon, Dec 13 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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. > Either case, -1 is a definite no-no. I've got a local WIP patch to fix those that I can polish up, if you're interested. It's the result of adding the below & running the test suite against it: diff --git a/git.c b/git.c index 60c2784be45..d6bdb3571df 100644 --- a/git.c +++ b/git.c @@ -419,6 +419,7 @@ static int handle_alias(int *argcp, const char ***argv) static int run_builtin(struct cmd_struct *p, int argc, const char **argv) { int status, help; + int posix_status; struct stat st; const char *prefix; @@ -459,6 +460,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) validate_cache_entries(the_repository->index); status = p->fn(argc, argv, prefix); + posix_status = status & 0xFF; + if (status != posix_status) + BUG("got status %d which will be cast to %d, returning error() perhaps?", status, posix_status); validate_cache_entries(the_repository->index); if (status)
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Mon, Dec 13 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
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);
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(-)