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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 3, 2020, 04:58 UTC
Message-ID: <xmqqk12x17za.fsf@gitster.c.googlers.com>
In-Reply-To: <20200321161020.22817-2-mirucam@gmail.com>

Miriam Rubio <mirucam@gmail.com> writes:

Show 23 quoted lines

> 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@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>  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;
 }