{"thread":{"id":"60722","subject":"[PATCH] builtin/revert.c: refactor using an enum for cmd-action","startedAt":"2024-01-11T08:04:52Z","lastAt":"2024-01-12T19:34:37Z","messageCount":11,"participants":["Michael Lohmann","Phillip Wood","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486608","messageId":"20240111080417.59346-1-mi.al.lohmann@gmail.com","threadId":"60722","inReplyTo":null,"subject":"[PATCH] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-11T08:04:17Z","receivedAt":"2024-01-11T08:04:52Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"This is done to avoid having to keep the char values in sync in\ndifferent places and also to get compiler warnings on non-exhaustive\nswitches.\n\nThe newly introduced `revert_action`-enum aligns with the\nbuiltin/rebase.c `action`-enum though the name `revert_action` is chosen\nto better differentiate it from `replay_opts->action` with a different\nfunction. For rebase the equivalent of the latter is\n`rebase_options->type` and `rebase_options->action` corresponds to the\n`cmd` variable in revert.c.\n\nIn the rebase `action` enum there is the enumeration constant\n`ACTION_NONE` which is not particularly descriptive, since it seems to\nimply that no action should be taken. Instead it signals a start of a\nrevert/cherry-pick process, so here `REVERT_ACTION_START` was chosen.\n\nCo-authored-by: Wanja Henze <wanja.hentze@bevuta.com>\nSigned-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>\n---\nHello!\n\nWhen I was working on `revert/cherry-pick --show-current-patch` (still\nneeds a little bit more discussion, if actually wanted, see thread [1])\nI found the construct with the `cmd` as an int a bit irritating. I hope\nthis patch makes it more obvious what is actually going on.\n\nIs there a reason why `ACTION_NONE` was chosen as a name in\nbuiltin/rebase.c? My best guess is that it came along because it is the\nimplied action when no other specific action is passed in, but I don't\nfind that particularly descriptive on what its actual function is...\n(Yes, naming things is hard... :D)\n\nAn alternative to prefixing the enum name with \"revert_\" would be to\nrename `replay_opts->action` to `replay_opts->type` so it aligns with\nrebase. Would you prefer that instead?\n\nCheers\nMichael\n\n[1] https://lore.kernel.org/git/20231218121048.68290-1-mi.al.lohmann@gmail.com/\n\n builtin/revert.c | 80 +++++++++++++++++++++++++++++-------------------\n 1 file changed, 49 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 89821bab95..b5278b7a3b 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -20,6 +20,14 @@\n  * Copyright (c) 2005 Junio C Hamano\n  */\n \n+enum revert_action {\n+\tREVERT_ACTION_START = 0,\n+\tREVERT_ACTION_CONTINUE,\n+\tREVERT_ACTION_SKIP,\n+\tREVERT_ACTION_ABORT,\n+\tREVERT_ACTION_QUIT,\n+};\n+\n static const char * const revert_usage[] = {\n \tN_(\"git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>...\"),\n \tN_(\"git revert (--continue | --skip | --abort | --quit)\"),\n@@ -85,12 +93,12 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tconst char *me = action_name(opts);\n \tconst char *cleanup_arg = NULL;\n-\tint cmd = 0;\n+\tenum revert_action cmd = REVERT_ACTION_START;\n \tstruct option base_options[] = {\n-\t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), 'q'),\n-\t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), 'c'),\n-\t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), 'a'),\n-\t\tOPT_CMDMODE(0, \"skip\", &cmd, N_(\"skip current commit and continue\"), 's'),\n+\t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), REVERT_ACTION_QUIT),\n+\t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), REVERT_ACTION_CONTINUE),\n+\t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), REVERT_ACTION_ABORT),\n+\t\tOPT_CMDMODE(0, \"skip\", &cmd, N_(\"skip current commit and continue\"), REVERT_ACTION_SKIP),\n \t\tOPT_CLEANUP(&cleanup_arg),\n \t\tOPT_BOOL('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n \t\tOPT_BOOL('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n@@ -144,30 +152,37 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t}\n \n \t/* Check for incompatible command line arguments */\n-\tif (cmd) {\n-\t\tchar *this_operation;\n-\t\tif (cmd == 'q')\n+\t{\n+\t\tchar *this_operation = 0;\n+\t\tswitch (cmd) {\n+\t\tcase REVERT_ACTION_START:\n+\t\t\tbreak;\n+\t\tcase REVERT_ACTION_QUIT:\n \t\t\tthis_operation = \"--quit\";\n-\t\telse if (cmd == 'c')\n+\t\t\tbreak;\n+\t\tcase REVERT_ACTION_CONTINUE:\n \t\t\tthis_operation = \"--continue\";\n-\t\telse if (cmd == 's')\n+\t\t\tbreak;\n+\t\tcase REVERT_ACTION_SKIP:\n \t\t\tthis_operation = \"--skip\";\n-\t\telse {\n-\t\t\tassert(cmd == 'a');\n+\t\t\tbreak;\n+\t\tcase REVERT_ACTION_ABORT:\n \t\t\tthis_operation = \"--abort\";\n+\t\t\tbreak;\n \t\t}\n \n-\t\tverify_opt_compatible(me, this_operation,\n-\t\t\t\t\"--no-commit\", opts->no_commit,\n-\t\t\t\t\"--signoff\", opts->signoff,\n-\t\t\t\t\"--mainline\", opts->mainline,\n-\t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n-\t\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n-\t\t\t\t\"-x\", opts->record_origin,\n-\t\t\t\t\"--ff\", opts->allow_ff,\n-\t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n-\t\t\t\t\"--no-rerere-autoupdate\", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,\n-\t\t\t\tNULL);\n+\t\tif (this_operation)\n+\t\t\tverify_opt_compatible(me, this_operation,\n+\t\t\t\t\t\"--no-commit\", opts->no_commit,\n+\t\t\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\t\t\"--mainline\", opts->mainline,\n+\t\t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n+\t\t\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n+\t\t\t\t\t\"-x\", opts->record_origin,\n+\t\t\t\t\t\"--ff\", opts->allow_ff,\n+\t\t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n+\t\t\t\t\t\"--no-rerere-autoupdate\", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,\n+\t\t\t\t\tNULL);\n \t}\n \n \tif (!opts->strategy && opts->default_strategy) {\n@@ -183,9 +198,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\t\t\t\"--edit\", opts->edit > 0,\n \t\t\t\tNULL);\n \n-\tif (cmd) {\n-\t\topts->revs = NULL;\n-\t} else {\n+\tif (cmd == REVERT_ACTION_START) {\n \t\tstruct setup_revision_opt s_r_opt;\n \t\topts->revs = xmalloc(sizeof(*opts->revs));\n \t\trepo_init_revisions(the_repository, opts->revs, NULL);\n@@ -198,6 +211,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \t\ts_r_opt.assume_dashdash = 1;\n \t\targc = setup_revisions(argc, argv, opts->revs, &s_r_opt);\n+\t} else {\n+\t\topts->revs = NULL;\n \t}\n \n \tif (argc > 1)\n@@ -210,19 +225,22 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\topts->strategy = xstrdup(getenv(\"GIT_TEST_MERGE_ALGORITHM\"));\n \tfree(options);\n \n-\tif (cmd == 'q') {\n+\tswitch (cmd) {\n+\tcase REVERT_ACTION_QUIT: {\n \t\tint ret = sequencer_remove_state(opts);\n \t\tif (!ret)\n \t\t\tremove_branch_state(the_repository, 0);\n \t\treturn ret;\n \t}\n-\tif (cmd == 'c')\n+\tcase REVERT_ACTION_CONTINUE:\n \t\treturn sequencer_continue(the_repository, opts);\n-\tif (cmd == 'a')\n+\tcase REVERT_ACTION_ABORT:\n \t\treturn sequencer_rollback(the_repository, opts);\n-\tif (cmd == 's')\n+\tcase REVERT_ACTION_SKIP:\n \t\treturn sequencer_skip(the_repository, opts);\n-\treturn sequencer_pick_revisions(the_repository, opts);\n+\tcase REVERT_ACTION_START:\n+\t\treturn sequencer_pick_revisions(the_repository, opts);\n+\t}\n }\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\n-- \n2.42.0\n\n"},{"id":"486636","messageId":"1ed0e249-dab9-4cf3-9b76-c8797b97c9c5@gmail.com","threadId":"60722","inReplyTo":"20240111080417.59346-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-11T16:57:26Z","receivedAt":"2024-01-11T16:57:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Michael\n\nOn 11/01/2024 08:04, Michael Lohmann wrote:\n> This is done to avoid having to keep the char values in sync in\n> different places and also to get compiler warnings on non-exhaustive\n> switches.\n\nI think this is a reasonable change, thanks for working on it.\n\n> The newly introduced `revert_action`-enum aligns with the\n> builtin/rebase.c `action`-enum though the name `revert_action` is chosen\n> to better differentiate it from `replay_opts->action` with a different\n> function. For rebase the equivalent of the latter is\n> `rebase_options->type` and `rebase_options->action` corresponds to the\n> `cmd` variable in revert.c.\n> \n> In the rebase `action` enum there is the enumeration constant\n> `ACTION_NONE` which is not particularly descriptive, since it seems to\n> imply that no action should be taken. Instead it signals a start of a\n> revert/cherry-pick process, so here `REVERT_ACTION_START` was chosen.\n\nI think ACTION_NONE was intended to covey that the user did not pass one \nof the OPT_CMDMODE() options like \"--continue\" as there isn't a \n\"--start\" option. I don't have a strong opinion between \"_NONE\" and \n\"_START\".\n\n> +enum revert_action {\n> +\tREVERT_ACTION_START = 0,\n> +\tREVERT_ACTION_CONTINUE,\n> +\tREVERT_ACTION_SKIP,\n> +\tREVERT_ACTION_ABORT,\n> +\tREVERT_ACTION_QUIT,\n> +};\n\nThe \"REVERT_\" prefix is a bit unfortunate as this is used by cherry-pick \nas well but it does match the filename. As this enum is only used in \nthis file I'd be quite happy to drop the \"REVERT_\" prefix. (I don't \nthink we need to go messing with the \"action\" member of struct \nreplay_opts to do that)\n\n>   \t/* Check for incompatible command line arguments */\n> -\tif (cmd) {\n> -\t\tchar *this_operation;\n> -\t\tif (cmd == 'q')\n> +\t{\n> +\t\tchar *this_operation = 0;\n\nstyle note: we use \"NULL\" rather than \"0\" when initializing pointers. \nIdeally this would be a \"const char *\" as we assign string literals but \nthat is not a new problem with this patch.\n\n> +\t\tswitch (cmd) {\n> +\t\tcase REVERT_ACTION_START:\n> +\t\t\tbreak;\n\nI can see the attraction of using an exhaustive switch() here but as we \ndon't want to do anything in the _START case it gets a bit messy with \nthe extra conditional below. I wonder if we'd be better to replace \"if \n(cmd) {\" with \"if (cmd != REVERT_ACTION_START) {\". Alternatively if you \nwant to stick with the switch then declaring \"this_operation\" at the \nbeginning of the function would mean you can get rid of the new \"{}\" block.\n\n> +\t\tcase REVERT_ACTION_QUIT:\n>   \t\t\tthis_operation = \"--quit\";\n> -\t\telse if (cmd == 'c')\n> +\t\t\tbreak;\n> +\t\tcase REVERT_ACTION_CONTINUE:\n>   \t\t\tthis_operation = \"--continue\";\n> -\t\telse if (cmd == 's')\n> +\t\t\tbreak;\n> +\t\tcase REVERT_ACTION_SKIP:\n>   \t\t\tthis_operation = \"--skip\";\n> -\t\telse {\n> -\t\t\tassert(cmd == 'a'); > +\t\t\tbreak;\n> +\t\tcase REVERT_ACTION_ABORT:\n>   \t\t\tthis_operation = \"--abort\";\n> +\t\t\tbreak;\n>   \t\t}\n>   \n> -\t\tverify_opt_compatible(me, this_operation,\n> -\t\t\t\t\"--no-commit\", opts->no_commit,\n> -\t\t\t\t\"--signoff\", opts->signoff,\n> -\t\t\t\t\"--mainline\", opts->mainline,\n> -\t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n> -\t\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n> -\t\t\t\t\"-x\", opts->record_origin,\n> -\t\t\t\t\"--ff\", opts->allow_ff,\n> -\t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n> -\t\t\t\t\"--no-rerere-autoupdate\", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,\n> -\t\t\t\tNULL);\n> +\t\tif (this_operation)\n\nThe extra indentation here is unfortunate as some of the lines are \nrather long already. In the current code it is clear that we only call \nverify_opt_compatible() when cmd is non-nul, I think it would be clearer \nto use \"if (cmd != REVERT_ACTION_START)\" here.\n\n> +\t\t\tverify_opt_compatible(me, this_operation,\n> +\t\t\t\t\t\"--no-commit\", opts->no_commit,\n> +\t\t\t\t\t\"--signoff\", opts->signoff,\n> +\t\t\t\t\t\"--mainline\", opts->mainline,\n> +\t\t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n> +\t\t\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n> +\t\t\t\t\t\"-x\", opts->record_origin,\n> +\t\t\t\t\t\"--ff\", opts->allow_ff,\n> +\t\t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n> +\t\t\t\t\t\"--no-rerere-autoupdate\", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,\n> +\t\t\t\t\tNULL);\n>   \t}\n > [...]\n> -\tif (cmd) {\n> -\t\topts->revs = NULL;\n> -\t} else {\n> +\tif (cmd == REVERT_ACTION_START) {\n\nI was momentarily confused by this change but you're reversing the \nconditional. I agree that the result is an improvement.\n\nBest Wishes\n\nPhillip\n"},{"id":"486640","messageId":"20240111174727.55189-1-mi.al.lohmann@gmail.com","threadId":"60722","inReplyTo":"1ed0e249-dab9-4cf3-9b76-c8797b97c9c5@gmail.com","subject":"[PATCH v2] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-11T17:47:27Z","receivedAt":"2024-01-11T17:48:27Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"This is done to avoid having to keep the char values in sync in\ndifferent places and also to get compiler warnings on non-exhaustive\nswitches.\n\nIn the rebase `action` enum there is the enumeration constant\n`ACTION_NONE` which is not particularly descriptive, since it seems to\nimply that no action should be taken. Instead it signals a start of a\nrevert/cherry-pick process, so here `ACTION_START` was chosen.\n\nCo-authored-by: Wanja Henze <wanja.hentze@bevuta.com>\nSigned-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>\n---\nHi Phillip!\n\nThanks for the feedback!\n\nOn 11/01/2024 16:57, Phillip Wood wrote:\n> I can see the attraction of using an exhaustive switch() here but as\n> we don't want to do anything in the _START case it gets a bit messy\n> with the extra conditional below. I wonder if we'd be better to\n> replace \"if (cmd) {\" with \"if (cmd != REVERT_ACTION_START) {\".\n> Alternatively if you want to stick with the switch then declaring\n> \"this_operation\" at the beginning of the function would mean you can\n> get rid of the new \"{}\" block.\n\n> The extra indentation here is unfortunate as some of the lines are\n> rather long already. In the current code it is clear that we only call\n> verify_opt_compatible() when cmd is non-nul, I think it would be\n> clearer to use \"if (cmd != REVERT_ACTION_START)\" here.\n\nTotally agreed - an alternative to the `if` would be a `goto` (see this\nversion of the patch). This would keep the benefit of the exhaustive\nswitch, but I don't know if that would fit the style used in this\nproject... (at least it is a jump forward...)\n\nChanges compared to the previous patch:\n- initialize `this_operation` pointer with NULL instead of 0\n- drop \"REVERT_\" prefix of enum and its members\n- declare `this_operation` at the toplevel to get rid of codeblock\n- skip the verify_opt_compatible in case of ACTION_START with a `goto`\n\nBest wishes\nMichael\n\n builtin/revert.c | 90 ++++++++++++++++++++++++++++--------------------\n 1 file changed, 53 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 89821bab95..19e6653f99 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -20,6 +20,14 @@\n  * Copyright (c) 2005 Junio C Hamano\n  */\n \n+enum action {\n+\tACTION_START = 0,\n+\tACTION_CONTINUE,\n+\tACTION_SKIP,\n+\tACTION_ABORT,\n+\tACTION_QUIT,\n+};\n+\n static const char * const revert_usage[] = {\n \tN_(\"git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>...\"),\n \tN_(\"git revert (--continue | --skip | --abort | --quit)\"),\n@@ -85,12 +93,13 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tconst char *me = action_name(opts);\n \tconst char *cleanup_arg = NULL;\n-\tint cmd = 0;\n+\tchar *this_operation = NULL;\n+\tenum action cmd = ACTION_START;\n \tstruct option base_options[] = {\n-\t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), 'q'),\n-\t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), 'c'),\n-\t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), 'a'),\n-\t\tOPT_CMDMODE(0, \"skip\", &cmd, N_(\"skip current commit and continue\"), 's'),\n+\t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), ACTION_QUIT),\n+\t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), ACTION_CONTINUE),\n+\t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), ACTION_ABORT),\n+\t\tOPT_CMDMODE(0, \"skip\", &cmd, N_(\"skip current commit and continue\"), ACTION_SKIP),\n \t\tOPT_CLEANUP(&cleanup_arg),\n \t\tOPT_BOOL('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n \t\tOPT_BOOL('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n@@ -144,32 +153,36 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t}\n \n \t/* Check for incompatible command line arguments */\n-\tif (cmd) {\n-\t\tchar *this_operation;\n-\t\tif (cmd == 'q')\n-\t\t\tthis_operation = \"--quit\";\n-\t\telse if (cmd == 'c')\n-\t\t\tthis_operation = \"--continue\";\n-\t\telse if (cmd == 's')\n-\t\t\tthis_operation = \"--skip\";\n-\t\telse {\n-\t\t\tassert(cmd == 'a');\n-\t\t\tthis_operation = \"--abort\";\n-\t\t}\n-\n-\t\tverify_opt_compatible(me, this_operation,\n-\t\t\t\t\"--no-commit\", opts->no_commit,\n-\t\t\t\t\"--signoff\", opts->signoff,\n-\t\t\t\t\"--mainline\", opts->mainline,\n-\t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n-\t\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n-\t\t\t\t\"-x\", opts->record_origin,\n-\t\t\t\t\"--ff\", opts->allow_ff,\n-\t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n-\t\t\t\t\"--no-rerere-autoupdate\", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,\n-\t\t\t\tNULL);\n+\tswitch (cmd) {\n+\tcase ACTION_START:\n+\t\tgoto skip_opt_compatibility_verification;\n+\tcase ACTION_QUIT:\n+\t\tthis_operation = \"--quit\";\n+\t\tbreak;\n+\tcase ACTION_CONTINUE:\n+\t\tthis_operation = \"--continue\";\n+\t\tbreak;\n+\tcase ACTION_SKIP:\n+\t\tthis_operation = \"--skip\";\n+\t\tbreak;\n+\tcase ACTION_ABORT:\n+\t\tthis_operation = \"--abort\";\n+\t\tbreak;\n \t}\n \n+\tverify_opt_compatible(me, this_operation,\n+\t\t\t\"--no-commit\", opts->no_commit,\n+\t\t\t\"--signoff\", opts->signoff,\n+\t\t\t\"--mainline\", opts->mainline,\n+\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n+\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n+\t\t\t\"-x\", opts->record_origin,\n+\t\t\t\"--ff\", opts->allow_ff,\n+\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n+\t\t\t\"--no-rerere-autoupdate\", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,\n+\t\t\tNULL);\n+\n+skip_opt_compatibility_verification:\n \tif (!opts->strategy && opts->default_strategy) {\n \t\topts->strategy = opts->default_strategy;\n \t\topts->default_strategy = NULL;\n@@ -183,9 +196,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\t\t\t\"--edit\", opts->edit > 0,\n \t\t\t\tNULL);\n \n-\tif (cmd) {\n-\t\topts->revs = NULL;\n-\t} else {\n+\tif (cmd == ACTION_START) {\n \t\tstruct setup_revision_opt s_r_opt;\n \t\topts->revs = xmalloc(sizeof(*opts->revs));\n \t\trepo_init_revisions(the_repository, opts->revs, NULL);\n@@ -198,6 +209,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \t\ts_r_opt.assume_dashdash = 1;\n \t\targc = setup_revisions(argc, argv, opts->revs, &s_r_opt);\n+\t} else {\n+\t\topts->revs = NULL;\n \t}\n \n \tif (argc > 1)\n@@ -210,19 +223,22 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\topts->strategy = xstrdup(getenv(\"GIT_TEST_MERGE_ALGORITHM\"));\n \tfree(options);\n \n-\tif (cmd == 'q') {\n+\tswitch (cmd) {\n+\tcase ACTION_QUIT: {\n \t\tint ret = sequencer_remove_state(opts);\n \t\tif (!ret)\n \t\t\tremove_branch_state(the_repository, 0);\n \t\treturn ret;\n \t}\n-\tif (cmd == 'c')\n+\tcase ACTION_CONTINUE:\n \t\treturn sequencer_continue(the_repository, opts);\n-\tif (cmd == 'a')\n+\tcase ACTION_ABORT:\n \t\treturn sequencer_rollback(the_repository, opts);\n-\tif (cmd == 's')\n+\tcase ACTION_SKIP:\n \t\treturn sequencer_skip(the_repository, opts);\n-\treturn sequencer_pick_revisions(the_repository, opts);\n+\tcase ACTION_START:\n+\t\treturn sequencer_pick_revisions(the_repository, opts);\n+\t}\n }\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\n-- \n2.39.3 (Apple Git-145)\n\n"},{"id":"486643","messageId":"xmqq5xzzhda6.fsf@gitster.g","threadId":"60722","inReplyTo":"1ed0e249-dab9-4cf3-9b76-c8797b97c9c5@gmail.com","subject":"Re: [PATCH] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-11T19:37:37Z","receivedAt":"2024-01-11T19:37:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I think ACTION_NONE was intended to covey that the user did not pass\n> one of the OPT_CMDMODE() options like \"--continue\" as there isn't a\n> \"--start\" option. I don't have a strong opinion between \"_NONE\" and\n> \"_START\".\n\nI agree with you why NONE is called as such.  If \"revert\" does not\ntake \"--start\" (I do not remember offhand), I would think it would\nbe better to follow suit.\n\n"},{"id":"486644","messageId":"xmqqsf33fy3t.fsf@gitster.g","threadId":"60722","inReplyTo":"20240111174727.55189-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH v2] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-11T19:50:46Z","receivedAt":"2024-01-11T19:50:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lohmann <mi.al.lohmann@gmail.com> writes:\n\n>  \tstruct option base_options[] = {\n> -\t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), 'q'),\n> -\t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), 'c'),\n> -\t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), 'a'),\n> -\t\tOPT_CMDMODE(0, \"skip\", &cmd, N_(\"skip current commit and continue\"), 's'),\n> +\t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), ACTION_QUIT),\n> +\t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), ACTION_CONTINUE),\n> +\t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), ACTION_ABORT),\n> +\t\tOPT_CMDMODE(0, \"skip\", &cmd, N_(\"skip current commit and continue\"), ACTION_SKIP),\n>  \t\tOPT_CLEANUP(&cleanup_arg),\n>  \t\tOPT_BOOL('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n>  \t\tOPT_BOOL('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n\nI actually do not terribly mind reusing the single letter option\n(e.g. 'x' in \"-x\") and the command mode, as it is descriptive\nenough.  The argument that an enum allows compilers warn about\nnon-exhausitve switches is valid if we have such a switch.\n\n> +\tswitch (cmd) {\n> +\tcase ACTION_START:\n> +\t\tgoto skip_opt_compatibility_verification;\n> +\tcase ACTION_QUIT:\n> +\t\tthis_operation = \"--quit\";\n> +\t\tbreak;\n> +\tcase ACTION_CONTINUE:\n> +\t\tthis_operation = \"--continue\";\n> +\t\tbreak;\n> +\tcase ACTION_SKIP:\n> +\t\tthis_operation = \"--skip\";\n> +\t\tbreak;\n> +\tcase ACTION_ABORT:\n> +\t\tthis_operation = \"--abort\";\n> +\t\tbreak;\n>  \t}\n>  \n> +\tverify_opt_compatible(me, this_operation,\n\nAnd indeed the if/elseif cascade in the original is easier to ensure\nits exhaustiveness by turning it into a switch.\n\nHOWEVER.\n\nIf I were writing this patch, I would rather do it like so:\n\n\tthis_operation = NULL;\n\tswitch (cmd) {\n\tcase 'q':\n\t\tthis_operation = '--quit\";\n                break;\n\t...\n\tcase 'a':\n\t\tthis_operation = '--abort\";\n                break;\n\tdefault: /* everything else is compatible with others */\n\t\tbreak;\n\t}\n\tif (this_operation)\n\t\tverify_opt_compatible(me, this_operation, ...);\n\nUse of enum is optional; I just didn't like too much churning to\nillustrate a single idea (here, the single idea being \"switch is\nmore appropriate then if/else cascade in this case\"), and I think\nthis is easier to read than with enums [*].\n\n\tSide note: After all the single letter option names are\n\tmeant to be mnemonic.  \"case 'q'\" is just as descriptive as\n\t\"case ACTION_QUIT\" in the context of this switch statement.\n\nHTH.\n"},{"id":"486646","messageId":"20240111200627.64199-1-mi.al.lohmann@gmail.com","threadId":"60722","inReplyTo":"xmqqsf33fy3t.fsf@gitster.g","subject":"[PATCH v3] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-11T20:06:27Z","receivedAt":"2024-01-11T20:06:33Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"This is done to avoid having to keep the char values in sync in\ndifferent places and also to get compiler warnings on non-exhaustive\nswitches.\n\nIn the rebase `action` enum there is the enumeration constant\n`ACTION_NONE` which is not particularly descriptive, since it seems to\nimply that no action should be taken. Instead it signals a start of a\nrevert/cherry-pick process, so here `ACTION_START` was chosen.\n\nCo-authored-by: Wanja Henze <wanja.hentze@bevuta.com>\nSigned-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>\n---\n\nOn 11. Jan 2024, at 20:37, Junio C Hamano <gitster@pobox.com> wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> > I think ACTION_NONE was intended to covey that the user did not pass\n> > one of the OPT_CMDMODE() options like \"--continue\" as there isn't a\n> > \"--start\" option. I don't have a strong opinion between \"_NONE\" and\n> > \"_START\".\n>\n> I agree with you why NONE is called as such.  If \"revert\" does not\n> take \"--start\" (I do not remember offhand), I would think it would\n> be better to follow suit.\nMy point was that yes, it might be in sync with what the user passes in\nas arguments, but when I followed the code and saw lots of references to\nACTION_NONE I was puzzled, since my intuition of that name was that\n_no action_ should be taken (which did not make sense to me).\n\nSo the (provocative) question is: Do we want to keep the variable name\nin sync with some input parameters, or rather with the real action that\nshould be taken?\n\n(Depending on the outcome of this discussion I would also prepare a\npatch renaming it in builtin/rebase.c)\n\nWhat do you think about this version which keeps the\n`if (cmd != ACTION_START)` in favour of the `goto` and instead of the\nconstant if/else checks for the `verify_opt_compatible` (with the\n`assert` at the last one) here is one version with a\n`get_cmd_flag`-function (I am not that happy with the name...) that has\na `switch` and it has a runtime error handling with `BUG`.\n\nI think it is the most concise of the options so far.\n\nCiao\nMichael\n\n builtin/revert.c | 65 +++++++++++++++++++++++++++---------------------\n 1 file changed, 37 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 89821bab95..891aa1d720 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -20,6 +20,14 @@\n  * Copyright (c) 2005 Junio C Hamano\n  */\n \n+enum action {\n+\tACTION_START = 0,\n+\tACTION_CONTINUE,\n+\tACTION_SKIP,\n+\tACTION_ABORT,\n+\tACTION_QUIT,\n+};\n+\n static const char * const revert_usage[] = {\n \tN_(\"git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>...\"),\n \tN_(\"git revert (--continue | --skip | --abort | --quit)\"),\n@@ -33,6 +41,17 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n+static char* get_cmd_optionname(enum action cmd)\n+{\n+\tswitch (cmd) {\n+\tcase ACTION_CONTINUE: return \"--continue\";\n+\tcase ACTION_SKIP: return \"--skip\";\n+\tcase ACTION_ABORT: return \"--abort\";\n+\tcase ACTION_QUIT: return \"--quit\";\n+\tcase ACTION_START: BUG(\"no commandline flag for ACTION_START\");\n+\t}\n+}\n+\n static const char *action_name(const struct replay_opts *opts)\n {\n \treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n@@ -85,12 +104,12 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tconst char *me = action_name(opts);\n \tconst char *cleanup_arg = NULL;\n-\tint cmd = 0;\n+\tenum action cmd = ACTION_START;\n \tstruct option base_options[] = {\n-\t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), 'q'),\n-\t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), 'c'),\n-\t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), 'a'),\n-\t\tOPT_CMDMODE(0, \"skip\", &cmd, N_(\"skip current commit and continue\"), 's'),\n+\t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), ACTION_QUIT),\n+\t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), ACTION_CONTINUE),\n+\t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), ACTION_ABORT),\n+\t\tOPT_CMDMODE(0, \"skip\", &cmd, N_(\"skip current commit and continue\"), ACTION_SKIP),\n \t\tOPT_CLEANUP(&cleanup_arg),\n \t\tOPT_BOOL('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n \t\tOPT_BOOL('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n@@ -144,20 +163,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t}\n \n \t/* Check for incompatible command line arguments */\n-\tif (cmd) {\n-\t\tchar *this_operation;\n-\t\tif (cmd == 'q')\n-\t\t\tthis_operation = \"--quit\";\n-\t\telse if (cmd == 'c')\n-\t\t\tthis_operation = \"--continue\";\n-\t\telse if (cmd == 's')\n-\t\t\tthis_operation = \"--skip\";\n-\t\telse {\n-\t\t\tassert(cmd == 'a');\n-\t\t\tthis_operation = \"--abort\";\n-\t\t}\n-\n-\t\tverify_opt_compatible(me, this_operation,\n+\tif (cmd != ACTION_START)\n+\t\tverify_opt_compatible(me, get_cmd_optionname(cmd),\n \t\t\t\t\"--no-commit\", opts->no_commit,\n \t\t\t\t\"--signoff\", opts->signoff,\n \t\t\t\t\"--mainline\", opts->mainline,\n@@ -168,7 +175,6 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n \t\t\t\t\"--no-rerere-autoupdate\", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,\n \t\t\t\tNULL);\n-\t}\n \n \tif (!opts->strategy && opts->default_strategy) {\n \t\topts->strategy = opts->default_strategy;\n@@ -183,9 +189,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\t\t\t\"--edit\", opts->edit > 0,\n \t\t\t\tNULL);\n \n-\tif (cmd) {\n-\t\topts->revs = NULL;\n-\t} else {\n+\tif (cmd == ACTION_START) {\n \t\tstruct setup_revision_opt s_r_opt;\n \t\topts->revs = xmalloc(sizeof(*opts->revs));\n \t\trepo_init_revisions(the_repository, opts->revs, NULL);\n@@ -198,6 +202,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \t\ts_r_opt.assume_dashdash = 1;\n \t\targc = setup_revisions(argc, argv, opts->revs, &s_r_opt);\n+\t} else {\n+\t\topts->revs = NULL;\n \t}\n \n \tif (argc > 1)\n@@ -210,19 +216,22 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \t\topts->strategy = xstrdup(getenv(\"GIT_TEST_MERGE_ALGORITHM\"));\n \tfree(options);\n \n-\tif (cmd == 'q') {\n+\tswitch (cmd) {\n+\tcase ACTION_QUIT: {\n \t\tint ret = sequencer_remove_state(opts);\n \t\tif (!ret)\n \t\t\tremove_branch_state(the_repository, 0);\n \t\treturn ret;\n \t}\n-\tif (cmd == 'c')\n+\tcase ACTION_CONTINUE:\n \t\treturn sequencer_continue(the_repository, opts);\n-\tif (cmd == 'a')\n+\tcase ACTION_ABORT:\n \t\treturn sequencer_rollback(the_repository, opts);\n-\tif (cmd == 's')\n+\tcase ACTION_SKIP:\n \t\treturn sequencer_skip(the_repository, opts);\n-\treturn sequencer_pick_revisions(the_repository, opts);\n+\tcase ACTION_START:\n+\t\treturn sequencer_pick_revisions(the_repository, opts);\n+\t}\n }\n \n int cmd_revert(int argc, const char **argv, const char *prefix)\n-- \n2.39.3 (Apple Git-145)\n\n"},{"id":"486669","messageId":"xmqqzfxbee4b.fsf@gitster.g","threadId":"60722","inReplyTo":"20240111200627.64199-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-11T21:47:48Z","receivedAt":"2024-01-11T21:47:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lohmann <mi.al.lohmann@gmail.com> writes:\n\n> This is done to avoid having to keep the char values in sync in\n> different places and also to get compiler warnings on non-exhaustive\n> switches.\n>\n> In the rebase `action` enum there is the enumeration constant\n> `ACTION_NONE` which is not particularly descriptive, since it seems to\n> imply that no action should be taken. Instead it signals a start of a\n> revert/cherry-pick process, so here `ACTION_START` was chosen.\n>\n> Co-authored-by: Wanja Henze <wanja.hentze@bevuta.com>\n> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>\n> ---\n>\n> On 11. Jan 2024, at 20:37, Junio C Hamano <gitster@pobox.com> wrote:\n>> Phillip Wood <phillip.wood123@gmail.com> writes:\n>> > I think ACTION_NONE was intended to covey that the user did not pass\n>> > one of the OPT_CMDMODE() options like \"--continue\" as there isn't a\n>> > \"--start\" option. I don't have a strong opinion between \"_NONE\" and\n>> > \"_START\".\n>>\n>> I agree with you why NONE is called as such.  If \"revert\" does not\n>> take \"--start\" (I do not remember offhand), I would think it would\n>> be better to follow suit.\n> My point was that yes, it might be in sync with what the user passes in\n> as arguments, but when I followed the code and saw lots of references to\n> ACTION_NONE I was puzzled, since my intuition of that name was that\n> _no action_ should be taken (which did not make sense to me).\n\nI know you wrote that ;-).  But _NONE is \"no action was specified\",\nand has been so for a long time in the context of \"rebase\". I do not\nsee any confusion expressed there.  I do not expect to see any\nconfusion here, either, if we were to introduce these new enum.\n\n"},{"id":"486677","messageId":"xmqqttnjcrk6.fsf@gitster.g","threadId":"60722","inReplyTo":"20240111200627.64199-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T00:40:25Z","receivedAt":"2024-01-12T00:40:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lohmann <mi.al.lohmann@gmail.com> writes:\n\n> -\tif (cmd == 'c')\n> +\tcase ACTION_CONTINUE:\n>  \t\treturn sequencer_continue(the_repository, opts);\n> -\tif (cmd == 'a')\n> +\tcase ACTION_ABORT:\n>  \t\treturn sequencer_rollback(the_repository, opts);\n> -\tif (cmd == 's')\n> +\tcase ACTION_SKIP:\n>  \t\treturn sequencer_skip(the_repository, opts);\n> -\treturn sequencer_pick_revisions(the_repository, opts);\n> +\tcase ACTION_START:\n> +\t\treturn sequencer_pick_revisions(the_repository, opts);\n> +\t}\n>  }\n\nThis change broke the build when merged to 'seen' like so ...\n\nbuiltin/revert.c: In function 'run_sequencer':\nbuiltin/revert.c:242:1: error: control reaches end of non-void function [-Werror=return-typ ]\n  242 | }\n      | ^\n \n... so I'm discarding\nit out of my tree and redoing today's integration cycle.\n\nDifferent compilers are smart in different ways, and we shouldn't\noverly rely on the fact that some compilers may be happy by seeing a\nswitch that has all the enum values and notice that one of the return\nwill be triggered in its case arms.\n\n\n\n"},{"id":"486690","messageId":"20240112072404.GF618729@coredump.intra.peff.net","threadId":"60722","inReplyTo":"20240111200627.64199-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-12T07:24:04Z","receivedAt":"2024-01-12T07:24:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 11, 2024 at 09:06:27PM +0100, Michael Lohmann wrote:\n\n> > I agree with you why NONE is called as such.  If \"revert\" does not\n> > take \"--start\" (I do not remember offhand), I would think it would\n> > be better to follow suit.\n> My point was that yes, it might be in sync with what the user passes in\n> as arguments, but when I followed the code and saw lots of references to\n> ACTION_NONE I was puzzled, since my intuition of that name was that\n> _no action_ should be taken (which did not make sense to me).\n\nJust my two cents as an observer who is very familiar with the idioms of\nGit's codebase: it's common for us to use NONE here to mean \"an action\nhas not been selected\", which the code then translates to a default\naction. So that's what I would have chosen.\n\nBut your way of seeing it also makes sense to me. I think I just find\nthe \"START\" name jarring because we do not use that word elsewhere to\ndescribe the action. What if you called it ACTION_DEFAULT? Then it is\nboth the \"default\" value we give it, and also the default action (which\nis not otherwise named in the code).\n\n\nAs far as the enum vs char thing, I do not have a strong opinion (though\nI do tend to like enums myself). Here are a few minor style comments\n(again that are idiomatic for our code base):\n\n> +enum action {\n> +\tACTION_START = 0,\n> +\tACTION_CONTINUE,\n> +\tACTION_SKIP,\n> +\tACTION_ABORT,\n> +\tACTION_QUIT,\n> +};\n\nExplicitly setting ACTION_START to 0 is good (even though it is not\nstrictly necessary) because it makes it clear that we expect to use its\ntruthiness later. But later...\n\n> @@ -183,9 +189,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n>  \t\t\t\t\"--edit\", opts->edit > 0,\n>  \t\t\t\tNULL);\n>  \n> -\tif (cmd) {\n> -\t\topts->revs = NULL;\n> -\t} else {\n> +\tif (cmd == ACTION_START) {\n>  \t\tstruct setup_revision_opt s_r_opt;\n>  \t\topts->revs = xmalloc(sizeof(*opts->revs));\n>  \t\trepo_init_revisions(the_repository, opts->revs, NULL);\n> @@ -198,6 +202,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n>  \t\tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n>  \t\ts_r_opt.assume_dashdash = 1;\n>  \t\targc = setup_revisions(argc, argv, opts->revs, &s_r_opt);\n> +\t} else {\n> +\t\topts->revs = NULL;\n\nWe do not take advantage of that. It is still OK to do \"if (cmd)\" with\nthe enum, and that's what I'd usually expect in our code base. There is\nno need for this hunk at all (which also switches the order of the\nconditional, which just seems like churn to me).\n\n> @@ -33,6 +41,17 @@ static const char * const cherry_pick_usage[] = {\n>  \tNULL\n>  };\n>  \n> +static char* get_cmd_optionname(enum action cmd)\n\nFrom CodingGuidelines:\n\n  When declaring pointers, the star sides with the variable name, i.e.\n  \"char *string\", not \"char* string\" or \"char * string\".  This makes it\n  easier to understand code like \"char *string, c;\".\n\n(Yes, I know there are arguments for the other way, too; but consistency\nis the most important thing, I think).\n\n> +{\n> +\tswitch (cmd) {\n> +\tcase ACTION_CONTINUE: return \"--continue\";\n> +\tcase ACTION_SKIP: return \"--skip\";\n> +\tcase ACTION_ABORT: return \"--abort\";\n> +\tcase ACTION_QUIT: return \"--quit\";\n> +\tcase ACTION_START: BUG(\"no commandline flag for ACTION_START\");\n> +\t}\n> +}\n\nI find this perfectly readable, and is likely the way I'd write it in a\npersonal project. But in this project I find we tend to stick to more\nconventional formatting, like:\n\n  switch (cmd) {\n  case ACTION_CONTINUE:\n\treturn \"--continue\";\n  case ACTION_SKIP:\n\treturn \"--skip\";\n  ...and so on...\n\n> @@ -144,20 +163,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n>  \t}\n>  \n>  \t/* Check for incompatible command line arguments */\n> -\tif (cmd) {\n> -\t\tchar *this_operation;\n> -\t\tif (cmd == 'q')\n> -\t\t\tthis_operation = \"--quit\";\n> -\t\telse if (cmd == 'c')\n> -\t\t\tthis_operation = \"--continue\";\n> -\t\telse if (cmd == 's')\n> -\t\t\tthis_operation = \"--skip\";\n> -\t\telse {\n> -\t\t\tassert(cmd == 'a');\n> -\t\t\tthis_operation = \"--abort\";\n> -\t\t}\n> -\n> -\t\tverify_opt_compatible(me, this_operation,\n> +\tif (cmd != ACTION_START)\n\nLikewise here I'd probably leave this as \"if (cmd)\".\n\n> [...]\n\nEverything else looked pretty good to me.\n\n-Peff\n"},{"id":"486723","messageId":"xmqqil3ybets.fsf@gitster.g","threadId":"60722","inReplyTo":"20240112072404.GF618729@coredump.intra.peff.net","subject":"Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T18:13:03Z","receivedAt":"2024-01-12T18:13:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But your way of seeing it also makes sense to me. I think I just find\n> the \"START\" name jarring because we do not use that word elsewhere to\n> describe the action.\n\nThanks.  I forgot to say that I share the same feeling, both about\n\"NONE could mean no-op\" (but then seriously why would anybody sane\nwant that?) and \"START is not how we spell these things\".  I can see\nhow DEFAULT could make sense, but if somebody picked DEFAULT between\ntwo sensible choices NONE and DEFAULT here, especially if they claim\nthat they started this enum to mimick what is done in another place,\nand after they were told that the other place they are imitating\nfollows the convention of using NONE for \"nothing specified, so use\ndefault\", I would have to say that they are trying to be different\nfor the sake of being different, which is not a good sign.  I'd want\nour contributors to be original where being original matters more.\n\n>> +{\n>> +\tswitch (cmd) {\n>> +\tcase ACTION_CONTINUE: return \"--continue\";\n>> +\tcase ACTION_SKIP: return \"--skip\";\n>> +\tcase ACTION_ABORT: return \"--abort\";\n>> +\tcase ACTION_QUIT: return \"--quit\";\n>> +\tcase ACTION_START: BUG(\"no commandline flag for ACTION_START\");\n>> +\t}\n>> +}\n>\n> I find this perfectly readable, and is likely the way I'd write it in a\n> personal project. But in this project I find we tend to stick to more\n> conventional formatting, like:\n>\n>   switch (cmd) {\n>   case ACTION_CONTINUE:\n> \treturn \"--continue\";\n>   case ACTION_SKIP:\n> \treturn \"--skip\";\n>   ...and so on...\n\nSame.  I try to do the latter while working on this project, but I\ndo admit I use the former in small one-page tools I write outside\nthe context of this project.\n\n>> +\tif (cmd != ACTION_START)\n>\n> Likewise here I'd probably leave this as \"if (cmd)\".\n\nI 100% agree with the suggestion to explicitly define something to\nbe 0 when we are going to use it for its Boolean value.  So an\nalternative would be to treat all ACTION_* enum values the same, and\nnot define the first one explicitly to 0.  \n\nEspecially in the context of a patch that wants to turn if/elseif\ncascades to switch, I would suspect that the latter, as switch/case\ndoes not special case the falsehood among other possible values of\ninteger type, might be easier to maintain in the longer term.\n\nThanks.\n\n"},{"id":"486726","messageId":"20240112193359.2332-1-mi.al.lohmann@gmail.com","threadId":"60722","inReplyTo":"xmqqil3ybets.fsf@gitster.g","subject":"Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-12T19:33:59Z","receivedAt":"2024-01-12T19:34:37Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"On 12. Jan 2024, at 19:13, Junio C Hamano <gitster@pobox.com> wrote:\n> > But your way of seeing it also makes sense to me. I think I just find\n> > the \"START\" name jarring because we do not use that word elsewhere to\n> > describe the action.\n> \n> Thanks.  I forgot to say that I share the same feeling, both about\n> \"NONE could mean no-op\" (but then seriously why would anybody sane\n> want that?) and \"START is not how we spell these things\".  I can see\n> how DEFAULT could make sense, but if somebody picked DEFAULT between\n> two sensible choices NONE and DEFAULT here, especially if they claim\n> that they started this enum to mimick what is done in another place,\n> and after they were told that the other place they are imitating\n> follows the convention of using NONE for \"nothing specified, so use\n> default\", I would have to say that they are trying to be different\n> for the sake of being different, which is not a good sign.  I'd want\n> our contributors to be original where being original matters more.\n\nI am sorry to have left this feeling in you. It was not my intention to\nbe original, but I just did not understand the reason for the other\nname. If I wanted to be \"sneaky\" and wasn't truly open for a discussion\nI would not have mentioned that it is different in the other file. I\ndon't try to be original for the sake of it, but yes indeed if I have a\nhard time understanding some reasoning, in my day job it is my role to\nask these. But I think I am indeed questioning a bit too much here.\nSorry for that! You as the project lead constantly have to do the same\nand I am in awe as how you handle it.\n\nI am sorry that this discussion did get out of hand. Especially since\nthis patch does not even introduce a feature, but is only a refactoring\nof an already perfectly fine codebase. My only intention was to align\nbuiltin/refactor.c a bit more to builtin/rebase.c but the current state\nis 100% good as is, so I think we should just drop this discussion.\n\n> > + if (cmd != ACTION_START)\n> > \n> > Likewise here I'd probably leave this as \"if (cmd)\".\n> \n> I 100% agree with the suggestion to explicitly define something to\n> be 0 when we are going to use it for its Boolean value.  So an\n> alternative would be to treat all ACTION_* enum values the same, and\n> not define the first one explicitly to 0.  \n> \n> Especially in the context of a patch that wants to turn if/elseif\n> cascades to switch, I would suspect that the latter, as switch/case\n> does not special case the falsehood among other possible values of\n> integer type, might be easier to maintain in the longer term.\n\nThat is indeed what v4 of the patch did that I prepared half a day ago\nand just did not have the time to properly check again before I submit\nit. It also tackles the other issues you mentioned, but my feeling is\nthat the current state is good as it is with the characters and so we\nshould just drop this discussion.\n\nSorry to have caused such a stir and that I took so much of all of your\nvaluable time! I for myself have learned a great deal from all of you\nand your interactions, so thank you!\n\nMichael Lohmann\n"}]}