| Message ID | 1236a7319033a67befe34ed892db0eb5200490fd.1653144546.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect into a built-in | expand |
On Sat, May 21 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Exit codes cannot be negative, but `error()` returns -1. > > Let's just go with the common pattern and call `die()` in > `cmd_bisect__helper()` when incorrect arguments were detected. This is good in that before we'd return e.g. code 255 here: $ ./git bisect--helper --bisect-terms foo bar; echo $? error: --bisect-terms requires 0 or 1 argument 255 But now say: $ ./git bisect--helper --bisect-terms foo bar; echo $? fatal: --bisect-terms requires 0 or 1 argument 128 But after this patch we emit e.g. this: $ ./git bisect--helper --bisect-terms ; echo $? error: no terms defined 1 We should instead treat all these usage errors the same. A better fix would be to either use usage_msg_opt[f]() consistently instead of die(). Or just this, which would narrowly fix the inconsistency and the exit code: diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 21a3b913ed3..e44d894e2ec 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -1325,7 +1325,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) break; case BISECT_TERMS: if (argc > 1) - return error(_("--bisect-terms requires 0 or 1 argument")); + return -error(_("--bisect-terms requires 0 or 1 argument")); res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); break; case BISECT_SKIP: But returning 129 instead of 1 or 128 is better here, as that's the exit code we specifically use for bad usage messages. I'll read on, but changing "error" to "fatal" and the exit code from 255 and 1 to 128 and 1 instead of either always 129 or always 1 in these cases seems odd, especially as the last part of the function has this code: return -res; I.e. it's expecting "res" to be e.g. -1 or 0, and to convert that to 1 or 0.
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 21a3b913ed3..824f84ae76f 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -1325,7 +1325,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) break; case BISECT_TERMS: if (argc > 1) - return error(_("--bisect-terms requires 0 or 1 argument")); + die(_("--bisect-terms requires 0 or 1 argument")); res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); break; case BISECT_SKIP: @@ -1335,13 +1335,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) break; case BISECT_NEXT: if (argc) - return error(_("--bisect-next requires 0 arguments")); + die(_("--bisect-next requires 0 arguments")); get_terms(&terms); res = bisect_next(&terms, prefix); break; case BISECT_RESET: if (argc > 1) - return error(_("--bisect-reset requires either no argument or a commit")); + die(_("--bisect-reset requires either no argument or a commit")); res = bisect_reset(argc ? argv[0] : NULL); break; case BISECT_VISUALIZE: @@ -1350,18 +1350,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) break; case BISECT_REPLAY: if (argc != 1) - return error(_("no logfile given")); + die(_("no logfile given")); set_terms(&terms, "bad", "good"); res = bisect_replay(&terms, argv[0]); break; case BISECT_LOG: if (argc) - return error(_("--bisect-log requires 0 arguments")); + die(_("--bisect-log requires 0 arguments")); res = bisect_log(); break; case BISECT_RUN: if (!argc) - return error(_("bisect run failed: no command provided.")); + die(_("bisect run failed: no command provided.")); get_terms(&terms); res = bisect_run(&terms, argv, argc); break;