On Fri, Apr 3, 2020 at 6:58 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> 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.
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.
