| Message ID | 20200321161020.22817-2-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/11] bisect--helper: fix `cmd_*()` function switch default return | expand |
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. 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; }
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.
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.
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.")); } free_terms(&terms);
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(-)