[PATCH v2] 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>
---
Hi Phillip!

Thanks for the feedback!

On 11/01/2024 16:57, Phillip Wood wrote:
> I can see the attraction of using an exhaustive switch() here but as
> we don't want to do anything in the _START case it gets a bit messy
> with the extra conditional below. I wonder if we'd be better to
> replace "if (cmd) {" with "if (cmd != REVERT_ACTION_START) {".
> Alternatively if you want to stick with the switch then declaring
> "this_operation" at the beginning of the function would mean you can
> get rid of the new "{}" block.

> The extra indentation here is unfortunate as some of the lines are
> rather long already. In the current code it is clear that we only call
> verify_opt_compatible() when cmd is non-nul, I think it would be
> clearer to use "if (cmd != REVERT_ACTION_START)" here.

Totally agreed - an alternative to the `if` would be a `goto` (see this
version of the patch). This would keep the benefit of the exhaustive
switch, but I don't know if that would fit the style used in this
project... (at least it is a jump forward...)

Changes compared to the previous patch:
- initialize `this_operation` pointer with NULL instead of 0
- drop "REVERT_" prefix of enum and its members
- declare `this_operation` at the toplevel to get rid of codeblock
- skip the verify_opt_compatible in case of ACTION_START with a `goto`

Best wishes
Michael

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 89821bab95..19e6653f99 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)"),
@@ -85,12 +93,13 @@ 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;
+	char *this_operation = NULL;
+	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,32 +153,36 @@ 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,
-				"--no-commit", opts->no_commit,
-				"--signoff", opts->signoff,
-				"--mainline", opts->mainline,
-				"--strategy", opts->strategy ? 1 : 0,
-				"--strategy-option", opts->xopts.nr ? 1 : 0,
-				"-x", opts->record_origin,
-				"--ff", opts->allow_ff,
-				"--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
-				"--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
-				NULL);
+	switch (cmd) {
+	case ACTION_START:
+		goto skip_opt_compatibility_verification;
+	case ACTION_QUIT:
+		this_operation = "--quit";
+		break;
+	case ACTION_CONTINUE:
+		this_operation = "--continue";
+		break;
+	case ACTION_SKIP:
+		this_operation = "--skip";
+		break;
+	case ACTION_ABORT:
+		this_operation = "--abort";
+		break;
 	}
 
+	verify_opt_compatible(me, this_operation,
+			"--no-commit", opts->no_commit,
+			"--signoff", opts->signoff,
+			"--mainline", opts->mainline,
+			"--strategy", opts->strategy ? 1 : 0,
+			"--strategy-option", opts->xopts.nr ? 1 : 0,
+			"-x", opts->record_origin,
+			"--ff", opts->allow_ff,
+			"--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
+			"--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
+			NULL);
+
+skip_opt_compatibility_verification:
 	if (!opts->strategy && opts->default_strategy) {
 		opts->strategy = opts->default_strategy;
 		opts->default_strategy = NULL;
@@ -183,9 +196,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 +209,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 +223,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)