diff mbox series

[v3,10/15] bisect--helper: return only correct exit codes in `cmd_*()`

Message ID 1236a7319033a67befe34ed892db0eb5200490fd.1653144546.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series Finish converting git bisect into a built-in | expand

Commit Message

Johannes Schindelin May 21, 2022, 2:49 p.m. UTC
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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/bisect--helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Ævar Arnfjörð Bjarmason May 21, 2022, 4:45 p.m. UTC | #1
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 mbox series

Patch

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;