[04/10] builtin/revert.c: refactor run_sequencer() return pattern

Commit Message

Ævar Arnfjörð Bjarmason Dec. 30, 2022, 7:28 a.m. UTC

Refactor the return pattern in run_sequencer() to make it easier to
insert a "replay_opts_release()" call between the "fn(...)" invocation
and the eventual return.

Usually we'd name the "cbfun" here "fn", but by using this name we'll
nicely align all the "cmd == ?" comparisons.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---
 builtin/revert.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Comments

Junio C Hamano Jan. 1, 2023, 4:25 a.m. UTC | #1

Ævar Arnfjörð Bjarmason  <[e-mail]> writes:

> Refactor the return pattern in run_sequencer() to make it easier to
> insert a "replay_opts_release()" call between the "fn(...)" invocation
> and the eventual return.
>
> Usually we'd name the "cbfun" here "fn", but by using this name we'll
> nicely align all the "cmd == ?" comparisons.

If the new helper function sequencer_remove_branch_state() is used
elsewhere in later steps of the series, it is great that it is
extracted out of an existing code to handle the 'q(uit)' action.

However, I'd not appreciate this change from if/elseif/... cascade
to ? : ? : cascade, mixed into a creation of the new helper
function.  Such a change forces all conceivable future command
handlers to take only r and opts, and that is not a consistency we
do not know we need (yet---YAGNI).

Then you do not even have to talk about cbfun vs fn ;-).

And if sequencer_remove_branch_state() is not reused elsewhere in
the series, then simply dropping this step would be great.

Thanks.

Patch

diff --git a/builtin/revert.c b/builtin/revert.c
index f2d86d2a8f9..e956d125a2b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -93,6 +93,15 @@  static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 		die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
 }
 
+static int sequencer_remove_branch_state(struct repository *r,
+					 struct replay_opts *opts)
+{
+	int ret = sequencer_remove_state(opts);
+	if (!ret)
+		remove_branch_state(the_repository, 0);
+	return ret;
+}
+
 static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
@@ -120,6 +129,8 @@  static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END()
 	};
 	struct option *options = base_options;
+	int (*cbfun)(struct repository *repo, struct replay_opts *opts);
+	int ret;
 
 	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
@@ -223,19 +234,13 @@  static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 		opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
 	free(options);
 
-	if (cmd == 'q') {
-		int ret = sequencer_remove_state(opts);
-		if (!ret)
-			remove_branch_state(the_repository, 0);
-		return ret;
-	}
-	if (cmd == 'c')
-		return sequencer_continue(the_repository, opts);
-	if (cmd == 'a')
-		return sequencer_rollback(the_repository, opts);
-	if (cmd == 's')
-		return sequencer_skip(the_repository, opts);
-	return sequencer_pick_revisions(the_repository, opts);
+	cbfun = cmd == 'q' ? sequencer_remove_branch_state :
+		cmd == 'c' ? sequencer_continue :
+		cmd == 'a' ? sequencer_rollback :
+		cmd == 's' ? sequencer_skip :
+		sequencer_pick_revisions;
+	ret = cbfun(the_repository, opts);
+	return ret;
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)