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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 3, 2020, 18:30 UTC
Message-ID: <xmqqv9mgzajx.fsf@gitster.c.googlers.com>
In-Reply-To: <CAP8UFD3t5ZukXqfEr9W8FFror=SemmoB0hokri2BK6ZYcq619Q@mail.gmail.com>

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

Show 6 quoted lines

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

Show 25 quoted lines

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

Show 10 quoted lines

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