On Sat, May 21 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
>
> 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.
