[PATCH v3] builtin/revert.c: refactor using an enum for cmd-action

[Michael Lohmann <mi.al.lohmann@gmail.com>]

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

So the (provocative) question is: Do we want to keep the variable name
in sync with some input parameters, or rather with the real action that
should be taken?

(Depending on the outcome of this discussion I would also prepare a
patch renaming it in builtin/rebase.c)

What do you think about this version which keeps the
`if (cmd != ACTION_START)` in favour of the `goto` and instead of the
constant if/else checks for the `verify_opt_compatible` (with the
`assert` at the last one) here is one version with a
`get_cmd_flag`-function (I am not that happy with the name...) that has
a `switch` and it has a runtime error handling with `BUG`.

I think it is the most concise of the options so far.

Ciao
Michael

 builtin/revert.c | 65 +++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 28 deletions(-)

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)
-- 
2.39.3 (Apple Git-145)