[2/4] rebase -m: cleanup --strategy-option handling

Commit Message

Phillip Wood March 15, 2023, 3:14 p.m. UTC

From: Phillip Wood <[e-mail]>

When handling "--strategy-option" rebase collects the commands into a
struct string_list, then concatenates them into a string, prepending "--"
to each one before splitting the string and removing the "--" prefix.
This is an artifact of the scripted rebase and the need to support
"rebase --preserve-merges". Now that "--preserve-merges" no-longer
exists we can cleanup the way the argument is handled.

The tests for a bad strategy option are adjusted now that
parse_strategy_opts() is no-longer called when starting a rebase. The
fact that it only errors out when running "git rebase --continue" is a
mixed blessing but the next commit will fix the root cause of the
parsing problem so lets not worry about that here.

Signed-off-by: Phillip Wood <[e-mail]>
---
 builtin/rebase.c               | 36 +++++++++++++++-------------------
 sequencer.c                    |  2 +-
 sequencer.h                    |  1 -
 t/t3436-rebase-more-options.sh | 10 ++++++++--
 4 files changed, 25 insertions(+), 24 deletions(-)

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 516ad1b12a..5194d64c24 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -116,7 +116,8 @@  struct rebase_options {
 	struct string_list exec;
 	int allow_empty_message;
 	int rebase_merges, rebase_cousins;
-	char *strategy, *strategy_opts;
+	char *strategy;
+	struct string_list strategy_opts;
 	struct strbuf git_format_patch_opt;
 	int reschedule_failed_exec;
 	int reapply_cherry_picks;
@@ -142,6 +143,7 @@  struct rebase_options {
 		.config_autosquash = -1,                \
 		.update_refs = -1,                      \
 		.config_update_refs = -1,               \
+		.strategy_opts = STRING_LIST_INIT_NODUP	\
 	}
 
 static struct replay_opts get_replay_opts(const struct rebase_options *opts)
@@ -175,8 +177,14 @@  static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 		replay.default_strategy = NULL;
 	}
 
-	if (opts->strategy_opts)
-		parse_strategy_opts(&replay, opts->strategy_opts);
+	if (opts->strategy_opts.nr) {
+		ALLOC_ARRAY(replay.xopts, opts->strategy_opts.nr);
+		replay.xopts_nr = opts->strategy_opts.nr;
+		replay.xopts_alloc = opts->strategy_opts.nr;
+		for (size_t i = 0; i < opts->strategy_opts.nr; i++)
+			replay.xopts[i] =
+				xstrdup(opts->strategy_opts.items[i].string);
+	}
 
 	if (opts->squash_onto) {
 		oidcpy(&replay.squash_onto, opts->squash_onto);
@@ -1012,7 +1020,6 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	int ignore_whitespace = 0;
 	const char *gpg_sign = NULL;
 	const char *rebase_merges = NULL;
-	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
 	struct object_id squash_onto;
 	char *squash_onto_name = NULL;
 	char *keep_base_onto_name = NULL;
@@ -1121,7 +1128,7 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 			 N_("use 'merge-base --fork-point' to refine upstream")),
 		OPT_STRING('s', "strategy", &options.strategy,
 			   N_("strategy"), N_("use the given merge strategy")),
-		OPT_STRING_LIST('X', "strategy-option", &strategy_options,
+		OPT_STRING_LIST('X', "strategy-option", &options.strategy_opts,
 				N_("option"),
 				N_("pass the argument through to the merge "
 				   "strategy")),
@@ -1435,23 +1442,13 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} else {
 		/* REBASE_MERGE */
 		if (ignore_whitespace) {
-			string_list_append(&strategy_options,
+			string_list_append(&options.strategy_opts,
 					   "ignore-space-change");
 		}
 	}
 
-	if (strategy_options.nr) {
-		int i;
-
-		if (!options.strategy)
-			options.strategy = "ort";
-
-		strbuf_reset(&buf);
-		for (i = 0; i < strategy_options.nr; i++)
-			strbuf_addf(&buf, " --%s",
-				    strategy_options.items[i].string);
-		options.strategy_opts = xstrdup(buf.buf);
-	}
+	if (options.strategy_opts.nr && !options.strategy)
+		options.strategy = "ort";
 
 	if (options.strategy) {
 		options.strategy = xstrdup(options.strategy);
@@ -1826,10 +1823,9 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.gpg_sign_opt);
 	string_list_clear(&options.exec, 0);
 	free(options.strategy);
-	free(options.strategy_opts);
+	string_list_clear(&options.strategy_opts, 0);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
 	free(keep_base_onto_name);
-	string_list_clear(&strategy_options, 0);
 	return !!ret;
 }
diff --git a/sequencer.c b/sequencer.c
index 55b3ba3a51..83ea1016ae 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2918,7 +2918,7 @@  static int populate_opts_cb(const char *key, const char *value, void *data)
 	return 0;
 }
 
-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
+static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 {
 	int i;
 	int count;
diff --git a/sequencer.h b/sequencer.h
index 3bcdfa1b58..5de5cfa664 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -247,7 +247,6 @@  int read_oneliner(struct strbuf *buf,
 	const char *path, unsigned flags);
 int read_author_script(const char *path, char **name, char **email, char **date,
 		       int allow_missing);
-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);
 int write_basic_state(struct replay_opts *opts, const char *head_name,
 		      struct commit *onto, const struct object_id *orig_head);
 void sequencer_post_commit_cleanup(struct repository *r, int verbose);
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index c3184c9ade..3adf42f47d 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -41,19 +41,25 @@  test_expect_success 'setup' '
 '
 
 test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' '
+	test_when_finished "test_might_fail git rebase --abort" &&
 	cat >expect <<-\EOF &&
 	fatal: could not split '\''--bad'\'': unclosed quote
 	EOF
-	test_expect_code 128 git rebase -X"bad argument\"" side main >out 2>actual &&
+	GIT_SEQUENCE_EDITOR="echo break >" \
+		git rebase -i -X"bad argument\"" side main &&
+	test_expect_code 128 git rebase --continue >out 2>actual &&
 	test_must_be_empty out &&
 	test_cmp expect actual
 '
 
 test_expect_success 'bad -X <strategy-option> arguments: bad escape' '
+	test_when_finished "test_might_fail git rebase --abort" &&
 	cat >expect <<-\EOF &&
 	fatal: could not split '\''--bad'\'': cmdline ends with \
 	EOF
-	test_expect_code 128 git rebase -X"bad escape \\" side main >out 2>actual &&
+	GIT_SEQUENCE_EDITOR="echo break >" \
+		git rebase -i -X"bad escape \\" side main &&
+	test_expect_code 128 git rebase --continue >out 2>actual &&
 	test_must_be_empty out &&
 	test_cmp expect actual
 '