| Message ID | 20240111200627.64199-1-mi.al.lohmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] builtin/revert.c: refactor using an enum for cmd-action | expand |
Michael Lohmann <mi.al.lohmann@gmail.com> writes: > This is done to avoid having to keep the char values in sync in > different places and also to get compiler warnings on non-exhaustive > switches. > > In the rebase `action` enum there is the enumeration constant > `ACTION_NONE` which is not particularly descriptive, since it seems to > imply that no action should be taken. Instead it signals a start of a > revert/cherry-pick process, so here `ACTION_START` was chosen. > > Co-authored-by: Wanja Henze <wanja.hentze@bevuta.com> > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> > --- > > On 11. Jan 2024, at 20:37, Junio C Hamano <gitster@pobox.com> wrote: >> Phillip Wood <phillip.wood123@gmail.com> writes: >> > I think ACTION_NONE was intended to covey that the user did not pass >> > one of the OPT_CMDMODE() options like "--continue" as there isn't a >> > "--start" option. I don't have a strong opinion between "_NONE" and >> > "_START". >> >> I agree with you why NONE is called as such. If "revert" does not >> take "--start" (I do not remember offhand), I would think it would >> be better to follow suit. > My point was that yes, it might be in sync with what the user passes in > as arguments, but when I followed the code and saw lots of references to > ACTION_NONE I was puzzled, since my intuition of that name was that > _no action_ should be taken (which did not make sense to me). I know you wrote that ;-). But _NONE is "no action was specified", and has been so for a long time in the context of "rebase". I do not see any confusion expressed there. I do not expect to see any confusion here, either, if we were to introduce these new enum.
Michael Lohmann <mi.al.lohmann@gmail.com> writes: > - if (cmd == 'c') > + case ACTION_CONTINUE: > return sequencer_continue(the_repository, opts); > - if (cmd == 'a') > + case ACTION_ABORT: > return sequencer_rollback(the_repository, opts); > - if (cmd == 's') > + case ACTION_SKIP: > return sequencer_skip(the_repository, opts); > - return sequencer_pick_revisions(the_repository, opts); > + case ACTION_START: > + return sequencer_pick_revisions(the_repository, opts); > + } > } This change broke the build when merged to 'seen' like so ... builtin/revert.c: In function 'run_sequencer': builtin/revert.c:242:1: error: control reaches end of non-void function [-Werror=return-typ ] 242 | } | ^ ... so I'm discarding it out of my tree and redoing today's integration cycle. Different compilers are smart in different ways, and we shouldn't overly rely on the fact that some compilers may be happy by seeing a switch that has all the enum values and notice that one of the return will be triggered in its case arms.
On Thu, Jan 11, 2024 at 09:06:27PM +0100, Michael Lohmann wrote: > > I agree with you why NONE is called as such. If "revert" does not > > take "--start" (I do not remember offhand), I would think it would > > be better to follow suit. > My point was that yes, it might be in sync with what the user passes in > as arguments, but when I followed the code and saw lots of references to > ACTION_NONE I was puzzled, since my intuition of that name was that > _no action_ should be taken (which did not make sense to me). Just my two cents as an observer who is very familiar with the idioms of Git's codebase: it's common for us to use NONE here to mean "an action has not been selected", which the code then translates to a default action. So that's what I would have chosen. But your way of seeing it also makes sense to me. I think I just find the "START" name jarring because we do not use that word elsewhere to describe the action. What if you called it ACTION_DEFAULT? Then it is both the "default" value we give it, and also the default action (which is not otherwise named in the code). As far as the enum vs char thing, I do not have a strong opinion (though I do tend to like enums myself). Here are a few minor style comments (again that are idiomatic for our code base): > +enum action { > + ACTION_START = 0, > + ACTION_CONTINUE, > + ACTION_SKIP, > + ACTION_ABORT, > + ACTION_QUIT, > +}; Explicitly setting ACTION_START to 0 is good (even though it is not strictly necessary) because it makes it clear that we expect to use its truthiness later. But later... > @@ -183,9 +189,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, > "--edit", opts->edit > 0, > NULL); > > - if (cmd) { > - opts->revs = NULL; > - } else { > + if (cmd == ACTION_START) { > struct setup_revision_opt s_r_opt; > opts->revs = xmalloc(sizeof(*opts->revs)); > repo_init_revisions(the_repository, opts->revs, NULL); > @@ -198,6 +202,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, > memset(&s_r_opt, 0, sizeof(s_r_opt)); > s_r_opt.assume_dashdash = 1; > argc = setup_revisions(argc, argv, opts->revs, &s_r_opt); > + } else { > + opts->revs = NULL; We do not take advantage of that. It is still OK to do "if (cmd)" with the enum, and that's what I'd usually expect in our code base. There is no need for this hunk at all (which also switches the order of the conditional, which just seems like churn to me). > @@ -33,6 +41,17 @@ static const char * const cherry_pick_usage[] = { > NULL > }; > > +static char* get_cmd_optionname(enum action cmd) From CodingGuidelines: When declaring pointers, the star sides with the variable name, i.e. "char *string", not "char* string" or "char * string". This makes it easier to understand code like "char *string, c;". (Yes, I know there are arguments for the other way, too; but consistency is the most important thing, I think). > +{ > + switch (cmd) { > + case ACTION_CONTINUE: return "--continue"; > + case ACTION_SKIP: return "--skip"; > + case ACTION_ABORT: return "--abort"; > + case ACTION_QUIT: return "--quit"; > + case ACTION_START: BUG("no commandline flag for ACTION_START"); > + } > +} I find this perfectly readable, and is likely the way I'd write it in a personal project. But in this project I find we tend to stick to more conventional formatting, like: switch (cmd) { case ACTION_CONTINUE: return "--continue"; case ACTION_SKIP: return "--skip"; ...and so on... > @@ -144,20 +163,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, > } > > /* Check for incompatible command line arguments */ > - if (cmd) { > - char *this_operation; > - if (cmd == 'q') > - this_operation = "--quit"; > - else if (cmd == 'c') > - this_operation = "--continue"; > - else if (cmd == 's') > - this_operation = "--skip"; > - else { > - assert(cmd == 'a'); > - this_operation = "--abort"; > - } > - > - verify_opt_compatible(me, this_operation, > + if (cmd != ACTION_START) Likewise here I'd probably leave this as "if (cmd)". > [...] Everything else looked pretty good to me. -Peff
Jeff King <peff@peff.net> writes: > But your way of seeing it also makes sense to me. I think I just find > the "START" name jarring because we do not use that word elsewhere to > describe the action. Thanks. I forgot to say that I share the same feeling, both about "NONE could mean no-op" (but then seriously why would anybody sane want that?) and "START is not how we spell these things". I can see how DEFAULT could make sense, but if somebody picked DEFAULT between two sensible choices NONE and DEFAULT here, especially if they claim that they started this enum to mimick what is done in another place, and after they were told that the other place they are imitating follows the convention of using NONE for "nothing specified, so use default", I would have to say that they are trying to be different for the sake of being different, which is not a good sign. I'd want our contributors to be original where being original matters more. >> +{ >> + switch (cmd) { >> + case ACTION_CONTINUE: return "--continue"; >> + case ACTION_SKIP: return "--skip"; >> + case ACTION_ABORT: return "--abort"; >> + case ACTION_QUIT: return "--quit"; >> + case ACTION_START: BUG("no commandline flag for ACTION_START"); >> + } >> +} > > I find this perfectly readable, and is likely the way I'd write it in a > personal project. But in this project I find we tend to stick to more > conventional formatting, like: > > switch (cmd) { > case ACTION_CONTINUE: > return "--continue"; > case ACTION_SKIP: > return "--skip"; > ...and so on... Same. I try to do the latter while working on this project, but I do admit I use the former in small one-page tools I write outside the context of this project. >> + if (cmd != ACTION_START) > > Likewise here I'd probably leave this as "if (cmd)". I 100% agree with the suggestion to explicitly define something to be 0 when we are going to use it for its Boolean value. So an alternative would be to treat all ACTION_* enum values the same, and not define the first one explicitly to 0. Especially in the context of a patch that wants to turn if/elseif cascades to switch, I would suspect that the latter, as switch/case does not special case the falsehood among other possible values of integer type, might be easier to maintain in the longer term. Thanks.
On 12. Jan 2024, at 19:13, Junio C Hamano <gitster@pobox.com> wrote: > > But your way of seeing it also makes sense to me. I think I just find > > the "START" name jarring because we do not use that word elsewhere to > > describe the action. > > Thanks. I forgot to say that I share the same feeling, both about > "NONE could mean no-op" (but then seriously why would anybody sane > want that?) and "START is not how we spell these things". I can see > how DEFAULT could make sense, but if somebody picked DEFAULT between > two sensible choices NONE and DEFAULT here, especially if they claim > that they started this enum to mimick what is done in another place, > and after they were told that the other place they are imitating > follows the convention of using NONE for "nothing specified, so use > default", I would have to say that they are trying to be different > for the sake of being different, which is not a good sign. I'd want > our contributors to be original where being original matters more. I am sorry to have left this feeling in you. It was not my intention to be original, but I just did not understand the reason for the other name. If I wanted to be "sneaky" and wasn't truly open for a discussion I would not have mentioned that it is different in the other file. I don't try to be original for the sake of it, but yes indeed if I have a hard time understanding some reasoning, in my day job it is my role to ask these. But I think I am indeed questioning a bit too much here. Sorry for that! You as the project lead constantly have to do the same and I am in awe as how you handle it. I am sorry that this discussion did get out of hand. Especially since this patch does not even introduce a feature, but is only a refactoring of an already perfectly fine codebase. My only intention was to align builtin/refactor.c a bit more to builtin/rebase.c but the current state is 100% good as is, so I think we should just drop this discussion. > > + if (cmd != ACTION_START) > > > > Likewise here I'd probably leave this as "if (cmd)". > > I 100% agree with the suggestion to explicitly define something to > be 0 when we are going to use it for its Boolean value. So an > alternative would be to treat all ACTION_* enum values the same, and > not define the first one explicitly to 0. > > Especially in the context of a patch that wants to turn if/elseif > cascades to switch, I would suspect that the latter, as switch/case > does not special case the falsehood among other possible values of > integer type, might be easier to maintain in the longer term. That is indeed what v4 of the patch did that I prepared half a day ago and just did not have the time to properly check again before I submit it. It also tackles the other issues you mentioned, but my feeling is that the current state is good as it is with the characters and so we should just drop this discussion. Sorry to have caused such a stir and that I took so much of all of your valuable time! I for myself have learned a great deal from all of you and your interactions, so thank you! Michael Lohmann
diff --git a/builtin/revert.c b/builtin/revert.c index 89821bab95..891aa1d720 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -20,6 +20,14 @@ * Copyright (c) 2005 Junio C Hamano */ +enum action { + ACTION_START = 0, + ACTION_CONTINUE, + ACTION_SKIP, + ACTION_ABORT, + ACTION_QUIT, +}; + static const char * const revert_usage[] = { N_("git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>..."), N_("git revert (--continue | --skip | --abort | --quit)"), @@ -33,6 +41,17 @@ static const char * const cherry_pick_usage[] = { NULL }; +static char* get_cmd_optionname(enum action cmd) +{ + switch (cmd) { + case ACTION_CONTINUE: return "--continue"; + case ACTION_SKIP: return "--skip"; + case ACTION_ABORT: return "--abort"; + case ACTION_QUIT: return "--quit"; + case ACTION_START: BUG("no commandline flag for ACTION_START"); + } +} + static const char *action_name(const struct replay_opts *opts) { return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick"; @@ -85,12 +104,12 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, const char * const * usage_str = revert_or_cherry_pick_usage(opts); const char *me = action_name(opts); const char *cleanup_arg = NULL; - int cmd = 0; + enum action cmd = ACTION_START; struct option base_options[] = { - OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'), - OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'), - OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'), - OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'), + OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), ACTION_QUIT), + OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), ACTION_CONTINUE), + OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), ACTION_ABORT), + OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), ACTION_SKIP), OPT_CLEANUP(&cleanup_arg), OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")), OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")), @@ -144,20 +163,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, } /* Check for incompatible command line arguments */ - if (cmd) { - char *this_operation; - if (cmd == 'q') - this_operation = "--quit"; - else if (cmd == 'c') - this_operation = "--continue"; - else if (cmd == 's') - this_operation = "--skip"; - else { - assert(cmd == 'a'); - this_operation = "--abort"; - } - - verify_opt_compatible(me, this_operation, + if (cmd != ACTION_START) + verify_opt_compatible(me, get_cmd_optionname(cmd), "--no-commit", opts->no_commit, "--signoff", opts->signoff, "--mainline", opts->mainline, @@ -168,7 +175,6 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE, NULL); - } if (!opts->strategy && opts->default_strategy) { opts->strategy = opts->default_strategy; @@ -183,9 +189,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, "--edit", opts->edit > 0, NULL); - if (cmd) { - opts->revs = NULL; - } else { + if (cmd == ACTION_START) { struct setup_revision_opt s_r_opt; opts->revs = xmalloc(sizeof(*opts->revs)); repo_init_revisions(the_repository, opts->revs, NULL); @@ -198,6 +202,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, memset(&s_r_opt, 0, sizeof(s_r_opt)); s_r_opt.assume_dashdash = 1; argc = setup_revisions(argc, argv, opts->revs, &s_r_opt); + } else { + opts->revs = NULL; } if (argc > 1) @@ -210,19 +216,22 @@ static int run_sequencer(int argc, const char **argv, const char *prefix, opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM")); free(options); - if (cmd == 'q') { + switch (cmd) { + case ACTION_QUIT: { int ret = sequencer_remove_state(opts); if (!ret) remove_branch_state(the_repository, 0); return ret; } - if (cmd == 'c') + case ACTION_CONTINUE: return sequencer_continue(the_repository, opts); - if (cmd == 'a') + case ACTION_ABORT: return sequencer_rollback(the_repository, opts); - if (cmd == 's') + case ACTION_SKIP: return sequencer_skip(the_repository, opts); - return sequencer_pick_revisions(the_repository, opts); + case ACTION_START: + return sequencer_pick_revisions(the_repository, opts); + } } int cmd_revert(int argc, const char **argv, const char *prefix)