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

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

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

It *is* accident waiting to happen, unless you have a comment to
tell future developers that they are forbidden from changing the
assignment of values; "We've made sure" alone is not a good excuse.

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

In this particular case, you do not even need to rely on such a
comment to tie hands of future developers' needs (e.g. they may need
to add a new enum value that must come between OK and FAILED because
they will find "if (err < FAILED)" is an easy way to do something
they need to do; an ordering requirement similar to how "enum
todo_command" in sequencer.h wants to enforce certain ordering of
values is not uncommon, and they will find it awkward if they are
told that they cannot move FAILED to some value other than -1).  You
were even shown a better way to separate "res" from the value
error() returns (which will always be -1) and BISECT_FAILED (which
may be -1 right now, but future developers may want to change it,
and you have the power to allow it).

I do not see why you are still giving a lame excuse after that.

I even do not like the fact that you are doing so in the context of
being a mentor---please do not spoil the opportunity to educate good
developers of our future; instead please lead them by showing a good
example.

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

If there are two kinds of codepaths, some *need* to deal with both
positive and negative for good reasons, and others only need to deal
with non-positive values, it would make it easier to understand the
code by consistently using -res for the latter while using abs() for
the former.

This is a tangent, but a codepath that needs abs(res) may need to be
reexaimined for correctness, as it is likely that it is a sign that
a sloppy developer swept a deeper underlying problem under the rug.

Imagine that a function A, in one if() statement in it, returns
error() whose value is -1, and in some other if() statement returns
BAD_XYZZY whose value is 1.  The function A also returns BAD_FROTZ
whose value is 2.  The only guarantee the caller gets from the
function A is that an error is signaled by non-zero value, and zero
means success.

And if you use abs() to squash an error and BAD_XYZZY into 1 in your
function B that calls A, what good are you doing to the callers of
your B?  They cannot tell between error and BAD_XYZZY, but they can
tell them from BAD_FROTZ, but does such an arrangement make any
sense?  It would be far more rational to make your B either (1)
return -1 for any error, if B thinks callers do not have to care
(which could be a valid stance to take, depending on the nature of
B), or (2) add an error code to BAD_{XYZZY,FROTZ} family and map -1
that comes from an error to that value, so that the callers can tell
them apart, or (3) do the equivalent of (2) but do so inside A (not
in B), and update call the callers of A.

Any of the above is more sensible and future-proof, compared to
blindly using abs(res) and claim that you are safe because you are
not returning a negative value.

>> 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 the user can legitimately trigger it, it is not a "BUG:".  Let's
make sure we use "BUG:" (whether it comes from BUG("...") or handcrafted
message like this one here) only when there is a bug in our program.
In other words, when a user sees "BUG:" emitted from our program and
reports it to us, there shouldn't be a room for us to say, "eh,
thanks for reporting, but it is an intended behaviour---you are just
holding it wrong".

If I did not know bisect--helper is its way out (which would be the
endgame of making "git bisect" fully converted to C), I would say
that we should just mark it as an error, but in the endgame state,
there won't be any end-user visible bisect--helper, so I am OK to
label it as a "BUG:" in this case.  It will be in the endgame state.

Thanks.