Re: [PATCH v2 01/11] bisect--helper: fix `cmd_*()` function switch default return

[Junio C Hamano <gitster@xxxxxxxxx>]

Miriam Rubio <mirucam@xxxxxxxxx> writes:

> In a `cmd_*()` function, return `error()` cannot be used
> because that translates to `-1` and `cmd_*()` functions need
> to return exit codes.
>
> Let's fix switch default return.
>
> Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
> Signed-off-by: Miriam Rubio <mirucam@xxxxxxxxx>
> ---
>  builtin/bisect--helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c1c40b516d..1f81cff1d8 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		res = bisect_start(&terms, no_checkout, argv, argc);
>  		break;
>  	default:
> -		return error("BUG: unknown subcommand '%d'", cmdmode);
> +		res = error(_("BUG: unknown subcommand."));

The return value from error() is *NOT* taken from "enum
bisect_error"; its value (-1) happens to be the same as
BISECT_FAILED, but that is by accident, and not by design.

So the above code is accident waiting to happen, while

	default:
		error(_("BUG: ..."));
		res = BISECT_FAILED;

would be a lot more correct (by design).

>  	}
>  	free_terms(&terms);

After this part, there is this code:

       if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
                       res = BISECT_OK;

        return abs(res);

This is not a problem with this patch, but the use of abs() is very
misleading, as res is always non-positive, as it is (after fixing
the patch I am responding to) taken from "enum bisect_error"
vocabulary.  "return -res;" would make the intent of the code
clearer, I think.

By the way, under what condition can the "BUG:" be reached?  Would
it only be reachable by a programming error?  If so, it would be
correct to use BUG("...") and force it die there.  If it can be
reached in some other way (e.g. an incorrect input by the user,
corruption in state files "git bisect" uses on the filesystem), then
it is *not* a "BUG".

I think "bisect--helper" is *not* called by end-user, so an unknown
command would be a BUG in the calling program, which is still part
of git, so it probably is more prudent to do something like the
following instead.

Thanks.

 builtin/bisect--helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c1c40b516d..0fbd924aac 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		res = bisect_start(&terms, no_checkout, argv, argc);
 		break;
 	default:
-		return error("BUG: unknown subcommand '%d'", cmdmode);
+		BUG("unknown subcommand %d", (int)cmdmode);
 	}
 	free_terms(&terms);
 
@@ -722,5 +722,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
 		res = BISECT_OK;
 
-	return abs(res);
+	return -res;
 }