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

[Christian Couder <christian.couder@gmail.com>]

On Fri, Apr 3, 2020 at 6:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Miriam Rubio <mirucam@gmail.com> 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@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.

In bisect.h we have made sure that BISECT_FAILED would be -1, so it is
not by accident:

enum bisect_error {
        BISECT_OK = 0,
        BISECT_FAILED = -1,
        BISECT_ONLY_SKIPPED_LEFT = -2,
        BISECT_MERGE_BASE_CHECK = -3,
        BISECT_NO_TESTABLE_COMMIT = -4,
        BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND = -10,
        BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11
};

> So the above code is accident waiting to happen, while
>
>         default:
>                 error(_("BUG: ..."));
>                 res = BISECT_FAILED;
>
> would be a lot more correct (by design).

I think it is very unlikely that we will ever change the value
returned by error(), so I don't think there is an accident waiting to
happen.

Maybe we should make it clearer though in bisect.h in the comment
before the enum, that we chose -1 for BISECT_FAILED so that it is the
same as what error() returns. Maybe something like "BISECT_FAILED
error code: default error code, should be the same value as what
error() returns."

> 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.

I am ok with using "-res" here. There are other places where
"abs(res)" is needed though, so code could look a bit more consistent
if "abs(res)" was used here too.

> By the way, under what condition can the "BUG:" be reached?  Would
> it only be reachable by a programming error?

It could happen if a user would try to directly use `git
bisect--helper <cmd> ...` with an unsupported <cmd>. Users are not
supposed to directly use bisect--helper though.

It could also happen if a developer uses `git bisect--helper <cmd>
...` in a script, program or alias if <cmd> is not properly spelled or
is unavailable for some reason.

> 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".

In this case I think it's difficult to tell if it will be a bug or not.

> 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.

I am ok with both ways.

Thanks,
Christian.