[PATCH 0/4] rebase: cleanup merge strategy option handling

[PATCH 0/4] rebase: cleanup merge strategy option handling

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Cleanup the handling of --strategy-option now that we no longer need
to support "--preserve-merges" and properly quote the argument when
saving it to disc.

These patches are based on a merge of 'master' and
'ab/fix-strategy-opts-parsing'

Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/c2e329a52...3e02eeff7
Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v1

Phillip Wood (4):
  rebase: stop reading and writing unnecessary strategy state
  rebase -m: cleanup --strategy-option handling
  rebase -m: fix serialization of strategy options
  rebase: remove a couple of redundant strategy tests

 builtin/rebase.c               | 60 +++++++++-----------------------
 sequencer.c                    | 26 ++++++++++----
 sequencer.h                    |  1 -
 t/t3402-rebase-merge.sh        | 21 ------------
 t/t3418-rebase-continue.sh     | 62 +++++++++++-----------------------
 t/t3436-rebase-more-options.sh | 18 ----------
 6 files changed, 56 insertions(+), 132 deletions(-)

-- 
2.39.2

[PATCH 1/4] rebase: stop reading and writing unnecessary strategy state

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The state files for "--strategy" and "--strategy-option" are written and
read twice, once by builtin/rebase.c and then by sequencer.c. This is an
artifact of the scripted rebase and the need to support "rebase
--preserve-merges". Now that "--preserve-merges" no-longer exists we
only need to read and write these files in sequencer.c. This enables us
to remove a call to free() in read_strategy_opts() that was added by
f1f4ebf432 (sequencer.c: fix "opts->strategy" leak in
read_strategy_opts(), 2022-11-08) as this commit fixes the root cause of
that leak.

There is further scope for removing duplication in the reading and
writing of state files between builtin/rebase.c and sequencer.c but that
is left for a follow up series.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 24 ------------------------
 sequencer.c      |  1 -
 2 files changed, 25 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6635f10d52..516ad1b12a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -482,24 +482,6 @@ static int read_basic_state(struct rebase_options *opts)
 		opts->gpg_sign_opt = xstrdup(buf.buf);
 	}
 
-	if (file_exists(state_dir_path("strategy", opts))) {
-		strbuf_reset(&buf);
-		if (!read_oneliner(&buf, state_dir_path("strategy", opts),
-				   READ_ONELINER_WARN_MISSING))
-			return -1;
-		free(opts->strategy);
-		opts->strategy = xstrdup(buf.buf);
-	}
-
-	if (file_exists(state_dir_path("strategy_opts", opts))) {
-		strbuf_reset(&buf);
-		if (!read_oneliner(&buf, state_dir_path("strategy_opts", opts),
-				   READ_ONELINER_WARN_MISSING))
-			return -1;
-		free(opts->strategy_opts);
-		opts->strategy_opts = xstrdup(buf.buf);
-	}
-
 	strbuf_release(&buf);
 
 	return 0;
@@ -517,12 +499,6 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 		write_file(state_dir_path("quiet", opts), "%s", "");
 	if (opts->flags & REBASE_VERBOSE)
 		write_file(state_dir_path("verbose", opts), "%s", "");
-	if (opts->strategy)
-		write_file(state_dir_path("strategy", opts), "%s",
-			   opts->strategy);
-	if (opts->strategy_opts)
-		write_file(state_dir_path("strategy_opts", opts), "%s",
-			   opts->strategy_opts);
 	if (opts->allow_rerere_autoupdate > 0)
 		write_file(state_dir_path("allow_rerere_autoupdate", opts),
 			   "-%s-rerere-autoupdate",
diff --git a/sequencer.c b/sequencer.c
index 886fbc3616..55b3ba3a51 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2946,7 +2946,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 	strbuf_reset(buf);
 	if (!read_oneliner(buf, rebase_path_strategy(), 0))
 		return;
-	free(opts->strategy);
 	opts->strategy = strbuf_detach(buf, NULL);
 	if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
 		return;
-- 
2.39.2

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

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c               | 36 +++++++++++++++-------------------
 sequencer.c                    |  2 +-
 sequencer.h                    |  1 -
 t/t3436-rebase-more-options.sh | 10 ++++++++--
 4 files changed, 25 insertions(+), 24 deletions(-)

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
 '
-- 
2.39.2

[PATCH 3/4] rebase -m: fix serialization of strategy options

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

To store the strategy options rebase prepends " --" to each one and
writes them to a file. To load them it reads the file and passes the
contents to split_cmdline(). This roughly mimics the behavior of the
scripted rebase but has a couple of limitations, (1) options containing
whitespace are not properly preserved (this is true of the scripted
rebase as well) and (2) options containing '"' or '\' are incorrectly
parsed and may cause the parser to return an error.

Fix these limitations by quoting each option when they are stored so
that they can be parsed correctly. Now that "--preserve-merges" no
longer exist this change also stops prepending "--" to the options when
they are stored as that was an artifact of the scripted rebase.

These changes are backwards compatible so the files written by an older
version of git can be still be read. They are also forwards compatible,
the file can still be parsed by recent versions of git as they treat the
"--" prefix as optional.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                    | 23 +++++++++++++++++++----
 t/t3418-rebase-continue.sh     | 34 ++++++++++++++++++++++------------
 t/t3436-rebase-more-options.sh | 24 ------------------------
 3 files changed, 41 insertions(+), 40 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 83ea1016ae..8890d1f7a1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2930,7 +2930,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 	count = split_cmdline(strategy_opts_string,
 			      (const char ***)&opts->xopts);
 	if (count < 0)
-		die(_("could not split '%s': %s"), strategy_opts_string,
+		BUG("could not split '%s': %s", strategy_opts_string,
 			    split_cmdline_strerror(count));
 	opts->xopts_nr = count;
 	for (i = 0; i < opts->xopts_nr; i++) {
@@ -3054,12 +3054,27 @@ static int read_populate_opts(struct replay_opts *opts)
 
 static void write_strategy_opts(struct replay_opts *opts)
 {
-	int i;
 	struct strbuf buf = STRBUF_INIT;
 
-	for (i = 0; i < opts->xopts_nr; ++i)
-		strbuf_addf(&buf, " --%s", opts->xopts[i]);
+	/*
+	 * Quote strategy options so that they can be read correctly
+	 * by split_cmdline().
+	 */
+	for (size_t i = 0; i < opts->xopts_nr; i++) {
+		char *arg = opts->xopts[i];
 
+		if (i)
+			strbuf_addch(&buf, ' ');
+		strbuf_addch(&buf, '"');
+		for (size_t j = 0; arg[j]; j++) {
+			const char c = arg[j];
+
+			if (c == '"' || c =='\\')
+				strbuf_addch(&buf, '\\');
+			strbuf_addch(&buf, c);
+		}
+		strbuf_addch(&buf, '"');
+	}
 	write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
 	strbuf_release(&buf);
 }
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 130e2f9b55..42c3954125 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F2-on-topic-branch &&
 	test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run" &&
+	test_when_finished "rm -fr test-bin" &&
 	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
+
+	write_script test-bin/git-merge-funny <<-\EOF &&
+	printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual
+	shift 3 &&
+	exec git merge-recursive "$@"
 	EOF
-	chmod +x test-bin/git-merge-funny &&
+
+	cat >expect <<-\EOF &&
+	[7]
+	[--option=arg with space]
+	[--op"tion\]
+	[--new
+	line ]
+	[--]
+	EOF
+
+	rm -f actual &&
 	(
 		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -s funny -Xopt main topic
+		test_must_fail git rebase -s funny -X"option=arg with space" \
+				-Xop\"tion\\ -X"new${LF}line " main topic
 	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
+	test_cmp expect actual &&
+	rm actual &&
 	echo "Resolved" >F2 &&
 	git add F2 &&
 	(
 		PATH=./test-bin:$PATH &&
 		git rebase --continue
 	) &&
-	test -f funny.was.run
+	test_cmp expect actual
 '
 
 test_expect_success 'rebase -i --continue handles merge strategy and options' '
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 3adf42f47d..94671d3c46 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -40,30 +40,6 @@ test_expect_success 'setup' '
 	EOF
 '
 
-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
-	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
-	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
-'
-
 test_expect_success '--ignore-whitespace works with apply backend' '
 	test_must_fail git rebase --apply main side &&
 	git rebase --abort &&
-- 
2.39.2

Re: [PATCH 3/4] rebase -m: fix serialization of strategy options

[Elijah Newren]

On Wed, Mar 15, 2023 at 8:51 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> To store the strategy options rebase prepends " --" to each one and
> writes them to a file. To load them it reads the file and passes the
> contents to split_cmdline(). This roughly mimics the behavior of the
> scripted rebase but has a couple of limitations, (1) options containing
> whitespace are not properly preserved (this is true of the scripted
> rebase as well) and (2) options containing '"' or '\' are incorrectly
> parsed and may cause the parser to return an error.
>
> Fix these limitations by quoting each option when they are stored so
> that they can be parsed correctly. Now that "--preserve-merges" no
> longer exist this change also stops prepending "--" to the options when
> they are stored as that was an artifact of the scripted rebase.
>
> These changes are backwards compatible so the files written by an older
> version of git can be still be read. They are also forwards compatible,
> the file can still be parsed by recent versions of git as they treat the
> "--" prefix as optional.

I'm not sure we want to tie ourselves to support completing a rebase
with git version X+1 that was started with git version X.  But, it's
really nice that you're paying attention to that level of detail.  :-)

>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c                    | 23 +++++++++++++++++++----
>  t/t3418-rebase-continue.sh     | 34 ++++++++++++++++++++++------------
>  t/t3436-rebase-more-options.sh | 24 ------------------------
>  3 files changed, 41 insertions(+), 40 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 83ea1016ae..8890d1f7a1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2930,7 +2930,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
>         count = split_cmdline(strategy_opts_string,
>                               (const char ***)&opts->xopts);
>         if (count < 0)
> -               die(_("could not split '%s': %s"), strategy_opts_string,
> +               BUG("could not split '%s': %s", strategy_opts_string,
>                             split_cmdline_strerror(count));
>         opts->xopts_nr = count;
>         for (i = 0; i < opts->xopts_nr; i++) {
> @@ -3054,12 +3054,27 @@ static int read_populate_opts(struct replay_opts *opts)
>
>  static void write_strategy_opts(struct replay_opts *opts)
>  {
> -       int i;
>         struct strbuf buf = STRBUF_INIT;
>
> -       for (i = 0; i < opts->xopts_nr; ++i)
> -               strbuf_addf(&buf, " --%s", opts->xopts[i]);
> +       /*
> +        * Quote strategy options so that they can be read correctly
> +        * by split_cmdline().
> +        */
> +       for (size_t i = 0; i < opts->xopts_nr; i++) {
> +               char *arg = opts->xopts[i];
>
> +               if (i)
> +                       strbuf_addch(&buf, ' ');
> +               strbuf_addch(&buf, '"');
> +               for (size_t j = 0; arg[j]; j++) {
> +                       const char c = arg[j];
> +
> +                       if (c == '"' || c =='\\')
> +                               strbuf_addch(&buf, '\\');
> +                       strbuf_addch(&buf, c);
> +               }
> +               strbuf_addch(&buf, '"');
> +       }

Do we not have a function in quote.[ch] that can handle this?  If not,
should we add this code to a function in that file and call it?

>         write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
>         strbuf_release(&buf);
>  }
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 130e2f9b55..42c3954125 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
>         rm -fr .git/rebase-* &&
>         git reset --hard commit-new-file-F2-on-topic-branch &&
>         test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
> -       test_when_finished "rm -fr test-bin funny.was.run" &&
> +       test_when_finished "rm -fr test-bin" &&
>         mkdir test-bin &&
> -       cat >test-bin/git-merge-funny <<-EOF &&
> -       #!$SHELL_PATH
> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
> -       shift &&
> -       >funny.was.run &&
> -       exec git merge-recursive "\$@"
> +
> +       write_script test-bin/git-merge-funny <<-\EOF &&
> +       printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual
> +       shift 3 &&
> +       exec git merge-recursive "$@"
>         EOF
> -       chmod +x test-bin/git-merge-funny &&
> +
> +       cat >expect <<-\EOF &&
> +       [7]
> +       [--option=arg with space]
> +       [--op"tion\]
> +       [--new
> +       line ]
> +       [--]
> +       EOF
> +
> +       rm -f actual &&
>         (
>                 PATH=./test-bin:$PATH &&
> -               test_must_fail git rebase -s funny -Xopt main topic
> +               test_must_fail git rebase -s funny -X"option=arg with space" \
> +                               -Xop\"tion\\ -X"new${LF}line " main topic
>         ) &&
> -       test -f funny.was.run &&
> -       rm funny.was.run &&
> +       test_cmp expect actual &&
> +       rm actual &&
>         echo "Resolved" >F2 &&
>         git add F2 &&
>         (
>                 PATH=./test-bin:$PATH &&
>                 git rebase --continue
>         ) &&
> -       test -f funny.was.run
> +       test_cmp expect actual
>  '

I appreciate the more stringent test to cover these new special cases.  :-)

>
>  test_expect_success 'rebase -i --continue handles merge strategy and options' '
> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> index 3adf42f47d..94671d3c46 100755
> --- a/t/t3436-rebase-more-options.sh
> +++ b/t/t3436-rebase-more-options.sh
> @@ -40,30 +40,6 @@ test_expect_success 'setup' '
>         EOF
>  '
>
> -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
> -       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
> -       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
> -'
> -
>  test_expect_success '--ignore-whitespace works with apply backend' '
>         test_must_fail git rebase --apply main side &&
>         git rebase --abort &&
> --
> 2.39.2

Re: [PATCH 3/4] rebase -m: fix serialization of strategy options

[Phillip Wood]

On 01/04/2023 20:27, Elijah Newren wrote:
> On Wed, Mar 15, 2023 at 8:51 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> To store the strategy options rebase prepends " --" to each one and
>> writes them to a file. To load them it reads the file and passes the
>> contents to split_cmdline(). This roughly mimics the behavior of the
>> scripted rebase but has a couple of limitations, (1) options containing
>> whitespace are not properly preserved (this is true of the scripted
>> rebase as well) and (2) options containing '"' or '\' are incorrectly
>> parsed and may cause the parser to return an error.
>>
>> Fix these limitations by quoting each option when they are stored so
>> that they can be parsed correctly. Now that "--preserve-merges" no
>> longer exist this change also stops prepending "--" to the options when
>> they are stored as that was an artifact of the scripted rebase.
>>
>> These changes are backwards compatible so the files written by an older
>> version of git can be still be read. They are also forwards compatible,
>> the file can still be parsed by recent versions of git as they treat the
>> "--" prefix as optional.
> 
> I'm not sure we want to tie ourselves to support completing a rebase
> with git version X+1 that was started with git version X.  But, it's
> really nice that you're paying attention to that level of detail.  :-)

In the past we've had complaints when we've made backwards incompatible 
changes but I'm not sure we've ever promised never to do it. Part of the 
problem for users is that they may not be in control of when git gets 
upgraded. Also we've had reports where a macos user started a rebase 
with tig which used a bundled version of git that was different to what 
the user had available on the command line when the continued the rebase.

>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   sequencer.c                    | 23 +++++++++++++++++++----
>>   t/t3418-rebase-continue.sh     | 34 ++++++++++++++++++++++------------
>>   t/t3436-rebase-more-options.sh | 24 ------------------------
>>   3 files changed, 41 insertions(+), 40 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index 83ea1016ae..8890d1f7a1 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2930,7 +2930,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
>>          count = split_cmdline(strategy_opts_string,
>>                                (const char ***)&opts->xopts);
>>          if (count < 0)
>> -               die(_("could not split '%s': %s"), strategy_opts_string,
>> +               BUG("could not split '%s': %s", strategy_opts_string,
>>                              split_cmdline_strerror(count));
>>          opts->xopts_nr = count;
>>          for (i = 0; i < opts->xopts_nr; i++) {
>> @@ -3054,12 +3054,27 @@ static int read_populate_opts(struct replay_opts *opts)
>>
>>   static void write_strategy_opts(struct replay_opts *opts)
>>   {
>> -       int i;
>>          struct strbuf buf = STRBUF_INIT;
>>
>> -       for (i = 0; i < opts->xopts_nr; ++i)
>> -               strbuf_addf(&buf, " --%s", opts->xopts[i]);
>> +       /*
>> +        * Quote strategy options so that they can be read correctly
>> +        * by split_cmdline().
>> +        */
>> +       for (size_t i = 0; i < opts->xopts_nr; i++) {
>> +               char *arg = opts->xopts[i];
>>
>> +               if (i)
>> +                       strbuf_addch(&buf, ' ');
>> +               strbuf_addch(&buf, '"');
>> +               for (size_t j = 0; arg[j]; j++) {
>> +                       const char c = arg[j];
>> +
>> +                       if (c == '"' || c =='\\')
>> +                               strbuf_addch(&buf, '\\');
>> +                       strbuf_addch(&buf, c);
>> +               }
>> +               strbuf_addch(&buf, '"');
>> +       }
> 
> Do we not have a function in quote.[ch] that can handle this?  If not,
> should we add this code to a function in that file and call it?

No we don't and yes we should, though in the end I've added it to 
alias.c as that is where split_cmdline() lives.

Thanks for you comments, I'll post a re-roll in the next couple of days

Phillip

> 
>>          write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
>>          strbuf_release(&buf);
>>   }
>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
>> index 130e2f9b55..42c3954125 100755
>> --- a/t/t3418-rebase-continue.sh
>> +++ b/t/t3418-rebase-continue.sh
>> @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
>>          rm -fr .git/rebase-* &&
>>          git reset --hard commit-new-file-F2-on-topic-branch &&
>>          test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
>> -       test_when_finished "rm -fr test-bin funny.was.run" &&
>> +       test_when_finished "rm -fr test-bin" &&
>>          mkdir test-bin &&
>> -       cat >test-bin/git-merge-funny <<-EOF &&
>> -       #!$SHELL_PATH
>> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
>> -       shift &&
>> -       >funny.was.run &&
>> -       exec git merge-recursive "\$@"
>> +
>> +       write_script test-bin/git-merge-funny <<-\EOF &&
>> +       printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual
>> +       shift 3 &&
>> +       exec git merge-recursive "$@"
>>          EOF
>> -       chmod +x test-bin/git-merge-funny &&
>> +
>> +       cat >expect <<-\EOF &&
>> +       [7]
>> +       [--option=arg with space]
>> +       [--op"tion\]
>> +       [--new
>> +       line ]
>> +       [--]
>> +       EOF
>> +
>> +       rm -f actual &&
>>          (
>>                  PATH=./test-bin:$PATH &&
>> -               test_must_fail git rebase -s funny -Xopt main topic
>> +               test_must_fail git rebase -s funny -X"option=arg with space" \
>> +                               -Xop\"tion\\ -X"new${LF}line " main topic
>>          ) &&
>> -       test -f funny.was.run &&
>> -       rm funny.was.run &&
>> +       test_cmp expect actual &&
>> +       rm actual &&
>>          echo "Resolved" >F2 &&
>>          git add F2 &&
>>          (
>>                  PATH=./test-bin:$PATH &&
>>                  git rebase --continue
>>          ) &&
>> -       test -f funny.was.run
>> +       test_cmp expect actual
>>   '
> 
> I appreciate the more stringent test to cover these new special cases.  :-)
> 
>>
>>   test_expect_success 'rebase -i --continue handles merge strategy and options' '
>> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
>> index 3adf42f47d..94671d3c46 100755
>> --- a/t/t3436-rebase-more-options.sh
>> +++ b/t/t3436-rebase-more-options.sh
>> @@ -40,30 +40,6 @@ test_expect_success 'setup' '
>>          EOF
>>   '
>>
>> -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
>> -       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
>> -       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
>> -'
>> -
>>   test_expect_success '--ignore-whitespace works with apply backend' '
>>          test_must_fail git rebase --apply main side &&
>>          git rebase --abort &&
>> --
>> 2.39.2

[PATCH 4/4] rebase: remove a couple of redundant strategy tests

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The test removed in t3402 has been redundant ever since 80ff47957b
(rebase: remember strategy and strategy options, 2011-02-06) which added
a new test the first part of which (as noted in the commit message)
duplicated the existing test. The test removed in t3418 has been
redundant since the merge backend was removed in 68aa495b59 (rebase:
implement --merge via the interactive machinery, 2018-12-11) as it now
tests the same code paths as the preceding test.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3402-rebase-merge.sh    | 21 ---------------------
 t/t3418-rebase-continue.sh | 32 --------------------------------
 2 files changed, 53 deletions(-)

diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
index 7e46f4ca85..79b0640c00 100755
--- a/t/t3402-rebase-merge.sh
+++ b/t/t3402-rebase-merge.sh
@@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '
 	esac
 '
 
-test_expect_success 'rebase -s funny -Xopt' '
-	test_when_finished "rm -fr test-bin funny.was.run" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	git reset --hard &&
-	git checkout -b test-funny main^ &&
-	test_commit funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase -s funny -Xopt main
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase --skip works with two conflicts in a row' '
 	git checkout second-side  &&
 	tr "[A-Z]" "[a-z]" <newfile >tmp &&
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 42c3954125..2d0789e554 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --continue handles merge strategy and options' '
-	rm -fr .git/rebase-* &&
-	git reset --hard commit-new-file-F2-on-topic-branch &&
-	test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	echo "\$@" >>funny.args
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	case "\$2" in --foo) ;; *) exit 2 ;; esac
-	case "\$4" in --) ;; *) exit 2 ;; esac
-	shift 2 &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic
-	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
-	echo "Resolved" >F2 &&
-	git add F2 &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase --continue
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase -r passes merge strategy options correctly' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&
-- 
2.39.2

Re: [PATCH 4/4] rebase: remove a couple of redundant strategy tests

[Elijah Newren]

On Wed, Mar 15, 2023 at 8:38 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The test removed in t3402 has been redundant ever since 80ff47957b
> (rebase: remember strategy and strategy options, 2011-02-06) which added
> a new test the first part of which (as noted in the commit message)
> duplicated the existing test. The test removed in t3418 has been
> redundant since the merge backend was removed in 68aa495b59 (rebase:
> implement --merge via the interactive machinery, 2018-12-11) as it now
> tests the same code paths as the preceding test.

I was really confused by this commit message at first.  Eventually, I
figured out you were talking about the changes in this commit, just in
past tense.  Could we change it to imperative tense?  E.g.

Remove a test in t3402 that has been redundant ever since 80ff47957b
(rebase: remember strategy and strategy options, 2011-02-06).  That commit added
a new test, the first part of which (as noted in the old commit message)
duplicated an existing test.

Also remove a test t3418 that has been redundant since the merge
backend was removed in 68aa495b59 (rebase: implement --merge
via the interactive machinery, 2018-12-11), since it now tests the
same code paths as the preceding test.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  t/t3402-rebase-merge.sh    | 21 ---------------------
>  t/t3418-rebase-continue.sh | 32 --------------------------------
>  2 files changed, 53 deletions(-)
>
> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
> index 7e46f4ca85..79b0640c00 100755
> --- a/t/t3402-rebase-merge.sh
> +++ b/t/t3402-rebase-merge.sh
> @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '
>         esac
>  '
>
> -test_expect_success 'rebase -s funny -Xopt' '
> -       test_when_finished "rm -fr test-bin funny.was.run" &&
> -       mkdir test-bin &&
> -       cat >test-bin/git-merge-funny <<-EOF &&
> -       #!$SHELL_PATH
> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
> -       shift &&
> -       >funny.was.run &&
> -       exec git merge-recursive "\$@"
> -       EOF
> -       chmod +x test-bin/git-merge-funny &&
> -       git reset --hard &&
> -       git checkout -b test-funny main^ &&
> -       test_commit funny &&
> -       (
> -               PATH=./test-bin:$PATH &&
> -               git rebase -s funny -Xopt main
> -       ) &&
> -       test -f funny.was.run
> -'
> -
>  test_expect_success 'rebase --skip works with two conflicts in a row' '
>         git checkout second-side  &&
>         tr "[A-Z]" "[a-z]" <newfile >tmp &&
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 42c3954125..2d0789e554 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
>         test_cmp expect actual
>  '
>
> -test_expect_success 'rebase -i --continue handles merge strategy and options' '
> -       rm -fr .git/rebase-* &&
> -       git reset --hard commit-new-file-F2-on-topic-branch &&
> -       test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
> -       test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
> -       mkdir test-bin &&
> -       cat >test-bin/git-merge-funny <<-EOF &&
> -       #!$SHELL_PATH
> -       echo "\$@" >>funny.args
> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
> -       case "\$2" in --foo) ;; *) exit 2 ;; esac
> -       case "\$4" in --) ;; *) exit 2 ;; esac
> -       shift 2 &&
> -       >funny.was.run &&
> -       exec git merge-recursive "\$@"
> -       EOF
> -       chmod +x test-bin/git-merge-funny &&
> -       (
> -               PATH=./test-bin:$PATH &&
> -               test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic
> -       ) &&
> -       test -f funny.was.run &&
> -       rm funny.was.run &&
> -       echo "Resolved" >F2 &&
> -       git add F2 &&
> -       (
> -               PATH=./test-bin:$PATH &&
> -               git rebase --continue
> -       ) &&
> -       test -f funny.was.run
> -'
> -
>  test_expect_success 'rebase -r passes merge strategy options correctly' '
>         rm -fr .git/rebase-* &&
>         git reset --hard commit-new-file-F3-on-topic-branch &&
> --
> 2.39.2

Looks good otherwise.

Re: [PATCH 4/4] rebase: remove a couple of redundant strategy tests

[Phillip Wood]

On 01/04/2023 20:31, Elijah Newren wrote:
> On Wed, Mar 15, 2023 at 8:38 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The test removed in t3402 has been redundant ever since 80ff47957b
>> (rebase: remember strategy and strategy options, 2011-02-06) which added
>> a new test the first part of which (as noted in the commit message)
>> duplicated the existing test. The test removed in t3418 has been
>> redundant since the merge backend was removed in 68aa495b59 (rebase:
>> implement --merge via the interactive machinery, 2018-12-11) as it now
>> tests the same code paths as the preceding test.
> 
> I was really confused by this commit message at first.  Eventually, I
> figured out you were talking about the changes in this commit, just in
> past tense.  Could we change it to imperative tense?  E.g.
> 
> Remove a test in t3402 that has been redundant ever since 80ff47957b
> (rebase: remember strategy and strategy options, 2011-02-06).  That commit added
> a new test, the first part of which (as noted in the old commit message)
> duplicated an existing test.
> 
> Also remove a test t3418 that has been redundant since the merge
> backend was removed in 68aa495b59 (rebase: implement --merge
> via the interactive machinery, 2018-12-11), since it now tests the
> same code paths as the preceding test.

Thanks, that is clearer, I'll use your suggested wording when I re-roll

Best Wishes

Phillip

>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   t/t3402-rebase-merge.sh    | 21 ---------------------
>>   t/t3418-rebase-continue.sh | 32 --------------------------------
>>   2 files changed, 53 deletions(-)
>>
>> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
>> index 7e46f4ca85..79b0640c00 100755
>> --- a/t/t3402-rebase-merge.sh
>> +++ b/t/t3402-rebase-merge.sh
>> @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '
>>          esac
>>   '
>>
>> -test_expect_success 'rebase -s funny -Xopt' '
>> -       test_when_finished "rm -fr test-bin funny.was.run" &&
>> -       mkdir test-bin &&
>> -       cat >test-bin/git-merge-funny <<-EOF &&
>> -       #!$SHELL_PATH
>> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
>> -       shift &&
>> -       >funny.was.run &&
>> -       exec git merge-recursive "\$@"
>> -       EOF
>> -       chmod +x test-bin/git-merge-funny &&
>> -       git reset --hard &&
>> -       git checkout -b test-funny main^ &&
>> -       test_commit funny &&
>> -       (
>> -               PATH=./test-bin:$PATH &&
>> -               git rebase -s funny -Xopt main
>> -       ) &&
>> -       test -f funny.was.run
>> -'
>> -
>>   test_expect_success 'rebase --skip works with two conflicts in a row' '
>>          git checkout second-side  &&
>>          tr "[A-Z]" "[a-z]" <newfile >tmp &&
>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
>> index 42c3954125..2d0789e554 100755
>> --- a/t/t3418-rebase-continue.sh
>> +++ b/t/t3418-rebase-continue.sh
>> @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
>>          test_cmp expect actual
>>   '
>>
>> -test_expect_success 'rebase -i --continue handles merge strategy and options' '
>> -       rm -fr .git/rebase-* &&
>> -       git reset --hard commit-new-file-F2-on-topic-branch &&
>> -       test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
>> -       test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
>> -       mkdir test-bin &&
>> -       cat >test-bin/git-merge-funny <<-EOF &&
>> -       #!$SHELL_PATH
>> -       echo "\$@" >>funny.args
>> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
>> -       case "\$2" in --foo) ;; *) exit 2 ;; esac
>> -       case "\$4" in --) ;; *) exit 2 ;; esac
>> -       shift 2 &&
>> -       >funny.was.run &&
>> -       exec git merge-recursive "\$@"
>> -       EOF
>> -       chmod +x test-bin/git-merge-funny &&
>> -       (
>> -               PATH=./test-bin:$PATH &&
>> -               test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic
>> -       ) &&
>> -       test -f funny.was.run &&
>> -       rm funny.was.run &&
>> -       echo "Resolved" >F2 &&
>> -       git add F2 &&
>> -       (
>> -               PATH=./test-bin:$PATH &&
>> -               git rebase --continue
>> -       ) &&
>> -       test -f funny.was.run
>> -'
>> -
>>   test_expect_success 'rebase -r passes merge strategy options correctly' '
>>          rm -fr .git/rebase-* &&
>>          git reset --hard commit-new-file-F3-on-topic-branch &&
>> --
>> 2.39.2
> 
> Looks good otherwise.

Re: [PATCH 0/4] rebase: cleanup merge strategy option handling

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Cleanup the handling of --strategy-option now that we no longer need
> to support "--preserve-merges" and properly quote the argument when
> saving it to disc.

Nice.

>
> These patches are based on a merge of 'master' and
> 'ab/fix-strategy-opts-parsing'
>
> Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv1
> View-Changes-At: https://github.com/phillipwood/git/compare/c2e329a52...3e02eeff7
> Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v1
>
> Phillip Wood (4):
>   rebase: stop reading and writing unnecessary strategy state
>   rebase -m: cleanup --strategy-option handling
>   rebase -m: fix serialization of strategy options
>   rebase: remove a couple of redundant strategy tests
>
>  builtin/rebase.c               | 60 +++++++++-----------------------
>  sequencer.c                    | 26 ++++++++++----
>  sequencer.h                    |  1 -
>  t/t3402-rebase-merge.sh        | 21 ------------
>  t/t3418-rebase-continue.sh     | 62 +++++++++++-----------------------
>  t/t3436-rebase-more-options.sh | 18 ----------
>  6 files changed, 56 insertions(+), 132 deletions(-)

Re: [PATCH 0/4] rebase: cleanup merge strategy option handling

[Felipe Contreras]

On Wed, Mar 15, 2023 at 9:39 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Cleanup the handling of --strategy-option now that we no longer need
> to support "--preserve-merges" and properly quote the argument when
> saving it to disc.
>
> These patches are based on a merge of 'master' and
> 'ab/fix-strategy-opts-parsing'
>
> Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv1
> View-Changes-At: https://github.com/phillipwood/git/compare/c2e329a52...3e02eeff7
> Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v1

FWIW I reviewed the changes and they all look good to me.

-- 
Felipe Contreras

Re: [PATCH 0/4] rebase: cleanup merge strategy option handling

[Elijah Newren]

On Wed, Mar 15, 2023 at 8:39 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Cleanup the handling of --strategy-option now that we no longer need
> to support "--preserve-merges" and properly quote the argument when
> saving it to disc.

Nice!  Thanks for taking the time to do these kinds of cleanups.
Overall the code looks pretty good, but I left a few
comments/questions on patches 3 & 4.

[PATCH v2 0/5] rebase: cleanup merge strategy option handling

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Cleanup the handling of --strategy-option now that we no longer need to
support "--preserve-merges" and properly quote the argument when saving
it to disc.

Thanks to Elijah for his comments on V1.

Changes since V1:

I've rebased these patches onto 'master' to avoid conflicts with
'sg/parse-options-h-initializers' in the new patch 2 (this series
depends on 'ab/fix-strategy-opts-parsing' but that has now been merged
to master).

Patch 1 - Unchanged.

Patch 2 - New patch to store the merge strategy options in an "struct
          strvec". This patch also introduces a new macro OPT_STRVEC()
          to collect options into an "struct strvec".

Patch 3 - Small simplification due to the changes in patch 2.

Patch 4 - Moved the code to quote a list so it can split by
          split_cmdline() into a new function quote_cmdline() as
	  suggested by Elijah.

Patch 5 - Reworded the commit message as suggested by Elijah.


Base-Commit: 140b9478dad5d19543c1cb4fd293ccec228f1240
Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv2
View-Changes-At: https://github.com/phillipwood/git/compare/140b9478d...3515c31b4
Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v2

Phillip Wood (5):
  rebase: stop reading and writing unnecessary strategy state
  sequencer: use struct strvec to store merge strategy options
  rebase -m: cleanup --strategy-option handling
  rebase -m: fix serialization of strategy options
  rebase: remove a couple of redundant strategy tests

 alias.c                        | 18 +++++++
 alias.h                        |  3 ++
 builtin/rebase.c               | 54 ++++-----------------
 builtin/revert.c               | 20 ++------
 parse-options-cb.c             | 16 +++++++
 parse-options.h                | 10 ++++
 sequencer.c                    | 57 ++++++++++------------
 sequencer.h                    | 12 +++--
 t/t3402-rebase-merge.sh        | 21 --------
 t/t3418-rebase-continue.sh     | 88 +++++++++++++---------------------
 t/t3436-rebase-more-options.sh | 18 -------
 11 files changed, 126 insertions(+), 191 deletions(-)

Range-diff against v1:
1:  029d0bf2b6 = 1:  2353c753f5 rebase: stop reading and writing unnecessary strategy state
-:  ---------- > 2:  dd7b82cdd5 sequencer: use struct strvec to store merge strategy options
2:  a01b182644 ! 3:  ccef0e6f4b rebase -m: cleanup --strategy-option handling
    @@ builtin/rebase.c: struct rebase_options {
      		.config_autosquash = -1,                \
      		.update_refs = -1,                      \
      		.config_update_refs = -1,               \
    -+		.strategy_opts = STRING_LIST_INIT_NODUP	\
    ++		.strategy_opts = STRING_LIST_INIT_NODUP,\
      	}
      
      static struct replay_opts get_replay_opts(const struct rebase_options *opts)
    @@ builtin/rebase.c: static struct replay_opts get_replay_opts(const struct rebase_
      
     -	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);
    -+	}
    ++	for (size_t i = 0; i < opts->strategy_opts.nr; i++)
    ++		strvec_push(&replay.xopts, opts->strategy_opts.items[i].string);
      
      	if (opts->squash_onto) {
      		oidcpy(&replay.squash_onto, opts->squash_onto);
3:  9af68cb065 ! 4:  9a90212ef2 rebase -m: fix serialization of strategy options
    @@ Commit message
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
    + ## alias.c ##
    +@@
    + #include "alloc.h"
    + #include "config.h"
    + #include "gettext.h"
    ++#include "strbuf.h"
    + #include "string-list.h"
    + 
    + struct config_alias_data {
    +@@ alias.c: void list_aliases(struct string_list *list)
    + 	read_early_config(config_alias_cb, &data);
    + }
    + 
    ++void quote_cmdline(struct strbuf *buf, const char **argv)
    ++{
    ++	for (const char **argp = argv; *argp; argp++) {
    ++		if (argp != argv)
    ++			strbuf_addch(buf, ' ');
    ++		strbuf_addch(buf, '"');
    ++		for (const char *p = *argp; *p; p++) {
    ++			const char c = *p;
    ++
    ++			if (c == '"' || c =='\\')
    ++				strbuf_addch(buf, '\\');
    ++			strbuf_addch(buf, c);
    ++		}
    ++		strbuf_addch(buf, '"');
    ++	}
    ++}
    ++
    + #define SPLIT_CMDLINE_BAD_ENDING 1
    + #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
    + #define SPLIT_CMDLINE_ARGC_OVERFLOW 3
    +
    + ## alias.h ##
    +@@
    + #ifndef ALIAS_H
    + #define ALIAS_H
    + 
    ++struct strbuf;
    + struct string_list;
    + 
    + char *alias_lookup(const char *alias);
    ++/* Quote argv so buf can be parsed by split_cmdline() */
    ++void quote_cmdline(struct strbuf *buf, const char **argv);
    + int split_cmdline(char *cmdline, const char ***argv);
    + /* Takes a negative value returned by split_cmdline */
    + const char *split_cmdline_strerror(int cmdline_errno);
    +
      ## sequencer.c ##
     @@ sequencer.c: static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
    - 	count = split_cmdline(strategy_opts_string,
    - 			      (const char ***)&opts->xopts);
    + 
    + 	count = split_cmdline(strategy_opts_string, &argv);
      	if (count < 0)
     -		die(_("could not split '%s': %s"), strategy_opts_string,
     +		BUG("could not split '%s': %s", strategy_opts_string,
      			    split_cmdline_strerror(count));
    - 	opts->xopts_nr = count;
    - 	for (i = 0; i < opts->xopts_nr; i++) {
    + 	for (i = 0; i < count; i++) {
    + 		const char *arg = argv[i];
     @@ sequencer.c: static int read_populate_opts(struct replay_opts *opts)
      
      static void write_strategy_opts(struct replay_opts *opts)
      {
     -	int i;
      	struct strbuf buf = STRBUF_INIT;
      
    --	for (i = 0; i < opts->xopts_nr; ++i)
    --		strbuf_addf(&buf, " --%s", opts->xopts[i]);
    +-	for (i = 0; i < opts->xopts.nr; ++i)
    +-		strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
     -
     +	/*
     +	 * Quote strategy options so that they can be read correctly
     +	 * by split_cmdline().
     +	 */
    -+	for (size_t i = 0; i < opts->xopts_nr; i++) {
    -+		char *arg = opts->xopts[i];
    -+
    -+		if (i)
    -+			strbuf_addch(&buf, ' ');
    -+		strbuf_addch(&buf, '"');
    -+		for (size_t j = 0; arg[j]; j++) {
    -+			const char c = arg[j];
    -+
    -+			if (c == '"' || c =='\\')
    -+				strbuf_addch(&buf, '\\');
    -+			strbuf_addch(&buf, c);
    -+		}
    -+		strbuf_addch(&buf, '"');
    -+	}
    ++	quote_cmdline(&buf, opts->xopts.v);
      	write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
      	strbuf_release(&buf);
      }
4:  3e02eeff78 ! 5:  3515c31b40 rebase: remove a couple of redundant strategy tests
    @@ Metadata
      ## Commit message ##
         rebase: remove a couple of redundant strategy tests
     
    -    The test removed in t3402 has been redundant ever since 80ff47957b
    -    (rebase: remember strategy and strategy options, 2011-02-06) which added
    -    a new test the first part of which (as noted in the commit message)
    -    duplicated the existing test. The test removed in t3418 has been
    -    redundant since the merge backend was removed in 68aa495b59 (rebase:
    -    implement --merge via the interactive machinery, 2018-12-11) as it now
    -    tests the same code paths as the preceding test.
    -
    +    Remove a test in t3402 that has been redundant ever since 80ff47957b
    +    (rebase: remember strategy and strategy options, 2011-02-06).  That
    +    commit added a new test, the first part of which (as noted in the old
    +    commit message) duplicated an existing test.
    +
    +    Also remove a test t3418 that has been redundant since the merge backend
    +    was removed in 68aa495b59 (rebase: implement --merge via the interactive
    +    machinery, 2018-12-11), since it now tests the same code paths as the
    +    preceding test.
    +
    +    Helped-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## t/t3402-rebase-merge.sh ##
-- 
2.40.0.670.g64ef305212.dirty

[PATCH v2 1/5] rebase: stop reading and writing unnecessary strategy state

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The state files for "--strategy" and "--strategy-option" are written and
read twice, once by builtin/rebase.c and then by sequencer.c. This is an
artifact of the scripted rebase and the need to support "rebase
--preserve-merges". Now that "--preserve-merges" no-longer exists we
only need to read and write these files in sequencer.c. This enables us
to remove a call to free() in read_strategy_opts() that was added by
f1f4ebf432 (sequencer.c: fix "opts->strategy" leak in
read_strategy_opts(), 2022-11-08) as this commit fixes the root cause of
that leak.

There is further scope for removing duplication in the reading and
writing of state files between builtin/rebase.c and sequencer.c but that
is left for a follow up series.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 24 ------------------------
 sequencer.c      |  1 -
 2 files changed, 25 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b7b908b66..3bd215c771 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -483,24 +483,6 @@ static int read_basic_state(struct rebase_options *opts)
 		opts->gpg_sign_opt = xstrdup(buf.buf);
 	}
 
-	if (file_exists(state_dir_path("strategy", opts))) {
-		strbuf_reset(&buf);
-		if (!read_oneliner(&buf, state_dir_path("strategy", opts),
-				   READ_ONELINER_WARN_MISSING))
-			return -1;
-		free(opts->strategy);
-		opts->strategy = xstrdup(buf.buf);
-	}
-
-	if (file_exists(state_dir_path("strategy_opts", opts))) {
-		strbuf_reset(&buf);
-		if (!read_oneliner(&buf, state_dir_path("strategy_opts", opts),
-				   READ_ONELINER_WARN_MISSING))
-			return -1;
-		free(opts->strategy_opts);
-		opts->strategy_opts = xstrdup(buf.buf);
-	}
-
 	strbuf_release(&buf);
 
 	return 0;
@@ -518,12 +500,6 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 		write_file(state_dir_path("quiet", opts), "%s", "");
 	if (opts->flags & REBASE_VERBOSE)
 		write_file(state_dir_path("verbose", opts), "%s", "");
-	if (opts->strategy)
-		write_file(state_dir_path("strategy", opts), "%s",
-			   opts->strategy);
-	if (opts->strategy_opts)
-		write_file(state_dir_path("strategy_opts", opts), "%s",
-			   opts->strategy_opts);
 	if (opts->allow_rerere_autoupdate > 0)
 		write_file(state_dir_path("allow_rerere_autoupdate", opts),
 			   "-%s-rerere-autoupdate",
diff --git a/sequencer.c b/sequencer.c
index 3be23d7ca2..c35a67e104 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2944,7 +2944,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 	strbuf_reset(buf);
 	if (!read_oneliner(buf, rebase_path_strategy(), 0))
 		return;
-	free(opts->strategy);
 	opts->strategy = strbuf_detach(buf, NULL);
 	if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
 		return;
-- 
2.40.0.670.g64ef305212.dirty

[PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The sequencer stores the merge strategy options in an array of strings
which allocated with ALLOC_GROW(). Using "struct strvec" avoids manually
managing the memory of that array and simplifies the code.

Aside from memory allocation the changes to the sequencer are largely
mechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A
new option parsing macro OPT_STRVEC() is also added to collect the
strategy options.  Hopefully this can be used to simplify the code in
builtin/merge.c in the future.

Note that there is a change of behavior to "git cherry-pick" and "git
revert" as passing “--no-strategy-option” will now clear any previous
strategy options whereas before this change it did nothing.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/revert.c   | 20 +++-----------------
 parse-options-cb.c | 16 ++++++++++++++++
 parse-options.h    | 10 ++++++++++
 sequencer.c        | 47 ++++++++++++++++++++--------------------------
 sequencer.h        | 11 ++++++++---
 5 files changed, 57 insertions(+), 47 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 62986a7b1b..362857da65 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
 }
 
-static int option_parse_x(const struct option *opt,
-			  const char *arg, int unset)
-{
-	struct replay_opts **opts_ptr = opt->value;
-	struct replay_opts *opts = *opts_ptr;
-
-	if (unset)
-		return 0;
-
-	ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-	opts->xopts[opts->xopts_nr++] = xstrdup(arg);
-	return 0;
-}
-
 static int option_parse_m(const struct option *opt,
 			  const char *arg, int unset)
 {
@@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 			     N_("select mainline parent"), option_parse_m),
 		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
 		OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")),
-		OPT_CALLBACK('X', "strategy-option", &opts, N_("option"),
-			N_("option for merge strategy"), option_parse_x),
+		OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"),
+			N_("option for merge strategy")),
 		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		OPT_END()
@@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 				"--signoff", opts->signoff,
 				"--mainline", opts->mainline,
 				"--strategy", opts->strategy ? 1 : 0,
-				"--strategy-option", opts->xopts ? 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,
diff --git a/parse-options-cb.c b/parse-options-cb.c
index d346dbe210..8eb96c5ca9 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
+{
+	struct strvec *v = opt->value;
+
+	if (unset) {
+		strvec_clear(v);
+		return 0;
+	}
+
+	if (!arg)
+		return -1;
+
+	strvec_push(v, arg);
+	return 0;
+}
+
 int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
 {
 	return 0;
diff --git a/parse-options.h b/parse-options.h
index 26f19384e5..2d1d4d052f 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -285,6 +285,15 @@ struct option {
 	.help = (h), \
 	.callback = &parse_opt_string_list, \
 }
+#define OPT_STRVEC(s, l, v, a, h) { \
+	.type = OPTION_CALLBACK, \
+	.short_name = (s), \
+	.long_name = (l), \
+	.value = (v), \
+	.argh = (a), \
+	.help = (h), \
+	.callback = &parse_opt_strvec, \
+}
 #define OPT_UYN(s, l, v, h) { \
 	.type = OPTION_CALLBACK, \
 	.short_name = (s), \
@@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int);
 int parse_opt_commit(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
+int parse_opt_strvec(const struct option *, const char *, int);
 int parse_opt_noop_cb(const struct option *, const char *, int);
 enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
 					   const struct option *,
diff --git a/sequencer.c b/sequencer.c
index c35a67e104..6e2f3357c8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts)
 	free(opts->reflog_action);
 	free(opts->default_strategy);
 	free(opts->strategy);
-	for (size_t i = 0; i < opts->xopts_nr; i++)
-		free(opts->xopts[i]);
-	free(opts->xopts);
+	strvec_clear (&opts->xopts);
 	strbuf_release(&opts->current_fixups);
 	if (opts->revs)
 		release_revisions(opts->revs);
@@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r,
 	next_tree = next ? get_commit_tree(next) : empty_tree(r);
 	base_tree = base ? get_commit_tree(base) : empty_tree(r);
 
-	for (i = 0; i < opts->xopts_nr; i++)
-		parse_merge_opt(&o, opts->xopts[i]);
+	for (i = 0; i < opts->xopts.nr; i++)
+		parse_merge_opt(&o, opts->xopts.v[i]);
 
 	if (!opts->strategy || !strcmp(opts->strategy, "ort")) {
 		memset(&result, 0, sizeof(result));
@@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r,
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
 		res |= try_merge_command(r, opts->strategy,
-					 opts->xopts_nr, (const char **)opts->xopts,
+					 opts->xopts.nr, opts->xopts.v,
 					common, oid_to_hex(&head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
@@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 	else if (!strcmp(key, "options.gpg-sign"))
 		git_config_string_dup(&opts->gpg_sign, key, value);
 	else if (!strcmp(key, "options.strategy-option")) {
-		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-		opts->xopts[opts->xopts_nr++] = xstrdup(value);
+		strvec_push(&opts->xopts, value);
 	} else if (!strcmp(key, "options.allow-rerere-auto"))
 		opts->allow_rerere_auto =
 			git_config_bool_or_int(key, value, &error_flag) ?
@@ -2920,22 +2917,21 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 {
 	int i;
 	int count;
+	const char **argv;
 	char *strategy_opts_string = raw_opts;
 
 	if (*strategy_opts_string == ' ')
 		strategy_opts_string++;
 
-	count = split_cmdline(strategy_opts_string,
-			      (const char ***)&opts->xopts);
+	count = split_cmdline(strategy_opts_string, &argv);
 	if (count < 0)
 		die(_("could not split '%s': %s"), strategy_opts_string,
 			    split_cmdline_strerror(count));
-	opts->xopts_nr = count;
-	for (i = 0; i < opts->xopts_nr; i++) {
-		const char *arg = opts->xopts[i];
+	for (i = 0; i < count; i++) {
+		const char *arg = argv[i];
 
 		skip_prefix(arg, "--", &arg);
-		opts->xopts[i] = xstrdup(arg);
+		strvec_push(&opts->xopts, arg);
 	}
 }
 
@@ -3055,8 +3051,8 @@ static void write_strategy_opts(struct replay_opts *opts)
 	int i;
 	struct strbuf buf = STRBUF_INIT;
 
-	for (i = 0; i < opts->xopts_nr; ++i)
-		strbuf_addf(&buf, " --%s", opts->xopts[i]);
+	for (i = 0; i < opts->xopts.nr; ++i)
+		strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
 
 	write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
 	strbuf_release(&buf);
@@ -3080,7 +3076,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_verbose(), "%s", "");
 	if (opts->strategy)
 		write_file(rebase_path_strategy(), "%s\n", opts->strategy);
-	if (opts->xopts_nr > 0)
+	if (opts->xopts.nr > 0)
 		write_strategy_opts(opts);
 
 	if (opts->allow_rerere_auto == RERERE_AUTOUPDATE)
@@ -3464,13 +3460,10 @@ static int save_opts(struct replay_opts *opts)
 	if (opts->gpg_sign)
 		res |= git_config_set_in_file_gently(opts_file,
 					"options.gpg-sign", opts->gpg_sign);
-	if (opts->xopts) {
-		int i;
-		for (i = 0; i < opts->xopts_nr; i++)
-			res |= git_config_set_multivar_in_file_gently(opts_file,
-					"options.strategy-option",
-					opts->xopts[i], "^$", 0);
-	}
+	for (size_t i = 0; i < opts->xopts.nr; i++)
+		res |= git_config_set_multivar_in_file_gently(opts_file,
+				"options.strategy-option",
+				opts->xopts.v[i], "^$", 0);
 	if (opts->allow_rerere_auto)
 		res |= git_config_set_in_file_gently(opts_file,
 				"options.allow-rerere-auto",
@@ -3880,7 +3873,7 @@ static int do_merge(struct repository *r,
 	struct commit *head_commit, *merge_commit, *i;
 	struct commit_list *bases, *j;
 	struct commit_list *to_merge = NULL, **tail = &to_merge;
-	const char *strategy = !opts->xopts_nr &&
+	const char *strategy = !opts->xopts.nr &&
 		(!opts->strategy ||
 		 !strcmp(opts->strategy, "recursive") ||
 		 !strcmp(opts->strategy, "ort")) ?
@@ -4063,9 +4056,9 @@ static int do_merge(struct repository *r,
 			strvec_push(&cmd.args, "octopus");
 		else {
 			strvec_push(&cmd.args, strategy);
-			for (k = 0; k < opts->xopts_nr; k++)
+			for (k = 0; k < opts->xopts.nr; k++)
 				strvec_pushf(&cmd.args,
-					     "-X%s", opts->xopts[k]);
+					     "-X%s", opts->xopts.v[k]);
 		}
 		if (!(flags & TODO_EDIT_MERGE_MSG))
 			strvec_push(&cmd.args, "--no-edit");
diff --git a/sequencer.h b/sequencer.h
index 33dbaf5b66..8a79d6b200 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -2,6 +2,7 @@
 #define SEQUENCER_H
 
 #include "strbuf.h"
+#include "strvec.h"
 #include "wt-status.h"
 
 struct commit;
@@ -60,8 +61,7 @@ struct replay_opts {
 	/* Merge strategy */
 	char *default_strategy;  /* from config options */
 	char *strategy;
-	char **xopts;
-	size_t xopts_nr, xopts_alloc;
+	struct strvec xopts;
 
 	/* Reflog */
 	char *reflog_action;
@@ -80,7 +80,12 @@ struct replay_opts {
 	/* Private use */
 	const char *reflog_message;
 };
-#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }
+#define REPLAY_OPTS_INIT {			\
+	.edit = -1,				\
+	.action = -1,				\
+	.current_fixups = STRBUF_INIT,		\
+	.xopts = STRVEC_INIT,			\
+}
 
 /*
  * Note that ordering matters in this enum. Not only must it match the mapping
-- 
2.40.0.670.g64ef305212.dirty

Re: [PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options

[Elijah Newren]

On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The sequencer stores the merge strategy options in an array of strings
> which allocated with ALLOC_GROW(). Using "struct strvec" avoids manually
> managing the memory of that array and simplifies the code.
>
> Aside from memory allocation the changes to the sequencer are largely
> mechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A
> new option parsing macro OPT_STRVEC() is also added to collect the
> strategy options.  Hopefully this can be used to simplify the code in
> builtin/merge.c in the future.
>
> Note that there is a change of behavior to "git cherry-pick" and "git
> revert" as passing “--no-strategy-option” will now clear any previous
> strategy options whereas before this change it did nothing.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/revert.c   | 20 +++-----------------
>  parse-options-cb.c | 16 ++++++++++++++++
>  parse-options.h    | 10 ++++++++++
>  sequencer.c        | 47 ++++++++++++++++++++--------------------------
>  sequencer.h        | 11 ++++++++---
>  5 files changed, 57 insertions(+), 47 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 62986a7b1b..362857da65 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
>         return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
>  }
>
> -static int option_parse_x(const struct option *opt,
> -                         const char *arg, int unset)
> -{
> -       struct replay_opts **opts_ptr = opt->value;
> -       struct replay_opts *opts = *opts_ptr;
> -
> -       if (unset)
> -               return 0;
> -
> -       ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> -       opts->xopts[opts->xopts_nr++] = xstrdup(arg);
> -       return 0;
> -}
> -
>  static int option_parse_m(const struct option *opt,
>                           const char *arg, int unset)
>  {
> @@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>                              N_("select mainline parent"), option_parse_m),
>                 OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
>                 OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")),
> -               OPT_CALLBACK('X', "strategy-option", &opts, N_("option"),
> -                       N_("option for merge strategy"), option_parse_x),
> +               OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"),
> +                       N_("option for merge strategy")),
>                 { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
>                   N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>                 OPT_END()
> @@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>                                 "--signoff", opts->signoff,
>                                 "--mainline", opts->mainline,
>                                 "--strategy", opts->strategy ? 1 : 0,
> -                               "--strategy-option", opts->xopts ? 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,
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index d346dbe210..8eb96c5ca9 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
>         return 0;
>  }
>
> +int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
> +{
> +       struct strvec *v = opt->value;
> +
> +       if (unset) {
> +               strvec_clear(v);
> +               return 0;
> +       }
> +
> +       if (!arg)
> +               return -1;
> +
> +       strvec_push(v, arg);
> +       return 0;
> +}
> +
>  int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
>  {
>         return 0;
> diff --git a/parse-options.h b/parse-options.h
> index 26f19384e5..2d1d4d052f 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -285,6 +285,15 @@ struct option {
>         .help = (h), \
>         .callback = &parse_opt_string_list, \
>  }
> +#define OPT_STRVEC(s, l, v, a, h) { \
> +       .type = OPTION_CALLBACK, \
> +       .short_name = (s), \
> +       .long_name = (l), \
> +       .value = (v), \
> +       .argh = (a), \
> +       .help = (h), \
> +       .callback = &parse_opt_strvec, \
> +}
>  #define OPT_UYN(s, l, v, h) { \
>         .type = OPTION_CALLBACK, \
>         .short_name = (s), \
> @@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int);
>  int parse_opt_commit(const struct option *, const char *, int);
>  int parse_opt_tertiary(const struct option *, const char *, int);
>  int parse_opt_string_list(const struct option *, const char *, int);
> +int parse_opt_strvec(const struct option *, const char *, int);
>  int parse_opt_noop_cb(const struct option *, const char *, int);
>  enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
>                                            const struct option *,
> diff --git a/sequencer.c b/sequencer.c
> index c35a67e104..6e2f3357c8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts)
>         free(opts->reflog_action);
>         free(opts->default_strategy);
>         free(opts->strategy);
> -       for (size_t i = 0; i < opts->xopts_nr; i++)
> -               free(opts->xopts[i]);
> -       free(opts->xopts);
> +       strvec_clear (&opts->xopts);
>         strbuf_release(&opts->current_fixups);
>         if (opts->revs)
>                 release_revisions(opts->revs);
> @@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r,
>         next_tree = next ? get_commit_tree(next) : empty_tree(r);
>         base_tree = base ? get_commit_tree(base) : empty_tree(r);
>
> -       for (i = 0; i < opts->xopts_nr; i++)
> -               parse_merge_opt(&o, opts->xopts[i]);
> +       for (i = 0; i < opts->xopts.nr; i++)
> +               parse_merge_opt(&o, opts->xopts.v[i]);
>
>         if (!opts->strategy || !strcmp(opts->strategy, "ort")) {
>                 memset(&result, 0, sizeof(result));
> @@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r,
>                 commit_list_insert(base, &common);
>                 commit_list_insert(next, &remotes);
>                 res |= try_merge_command(r, opts->strategy,
> -                                        opts->xopts_nr, (const char **)opts->xopts,
> +                                        opts->xopts.nr, opts->xopts.v,
>                                         common, oid_to_hex(&head), remotes);
>                 free_commit_list(common);
>                 free_commit_list(remotes);
> @@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>         else if (!strcmp(key, "options.gpg-sign"))
>                 git_config_string_dup(&opts->gpg_sign, key, value);
>         else if (!strcmp(key, "options.strategy-option")) {
> -               ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> -               opts->xopts[opts->xopts_nr++] = xstrdup(value);
> +               strvec_push(&opts->xopts, value);
>         } else if (!strcmp(key, "options.allow-rerere-auto"))
>                 opts->allow_rerere_auto =
>                         git_config_bool_or_int(key, value, &error_flag) ?
> @@ -2920,22 +2917,21 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
>  {
>         int i;
>         int count;
> +       const char **argv;
>         char *strategy_opts_string = raw_opts;
>
>         if (*strategy_opts_string == ' ')
>                 strategy_opts_string++;
>
> -       count = split_cmdline(strategy_opts_string,
> -                             (const char ***)&opts->xopts);
> +       count = split_cmdline(strategy_opts_string, &argv);
>         if (count < 0)
>                 die(_("could not split '%s': %s"), strategy_opts_string,
>                             split_cmdline_strerror(count));
> -       opts->xopts_nr = count;
> -       for (i = 0; i < opts->xopts_nr; i++) {
> -               const char *arg = opts->xopts[i];
> +       for (i = 0; i < count; i++) {
> +               const char *arg = argv[i];
>
>                 skip_prefix(arg, "--", &arg);
> -               opts->xopts[i] = xstrdup(arg);
> +               strvec_push(&opts->xopts, arg);
>         }
>  }
>
> @@ -3055,8 +3051,8 @@ static void write_strategy_opts(struct replay_opts *opts)
>         int i;
>         struct strbuf buf = STRBUF_INIT;
>
> -       for (i = 0; i < opts->xopts_nr; ++i)
> -               strbuf_addf(&buf, " --%s", opts->xopts[i]);
> +       for (i = 0; i < opts->xopts.nr; ++i)
> +               strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
>
>         write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
>         strbuf_release(&buf);
> @@ -3080,7 +3076,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
>                 write_file(rebase_path_verbose(), "%s", "");
>         if (opts->strategy)
>                 write_file(rebase_path_strategy(), "%s\n", opts->strategy);
> -       if (opts->xopts_nr > 0)
> +       if (opts->xopts.nr > 0)
>                 write_strategy_opts(opts);
>
>         if (opts->allow_rerere_auto == RERERE_AUTOUPDATE)
> @@ -3464,13 +3460,10 @@ static int save_opts(struct replay_opts *opts)
>         if (opts->gpg_sign)
>                 res |= git_config_set_in_file_gently(opts_file,
>                                         "options.gpg-sign", opts->gpg_sign);
> -       if (opts->xopts) {
> -               int i;
> -               for (i = 0; i < opts->xopts_nr; i++)
> -                       res |= git_config_set_multivar_in_file_gently(opts_file,
> -                                       "options.strategy-option",
> -                                       opts->xopts[i], "^$", 0);
> -       }
> +       for (size_t i = 0; i < opts->xopts.nr; i++)
> +               res |= git_config_set_multivar_in_file_gently(opts_file,
> +                               "options.strategy-option",
> +                               opts->xopts.v[i], "^$", 0);
>         if (opts->allow_rerere_auto)
>                 res |= git_config_set_in_file_gently(opts_file,
>                                 "options.allow-rerere-auto",
> @@ -3880,7 +3873,7 @@ static int do_merge(struct repository *r,
>         struct commit *head_commit, *merge_commit, *i;
>         struct commit_list *bases, *j;
>         struct commit_list *to_merge = NULL, **tail = &to_merge;
> -       const char *strategy = !opts->xopts_nr &&
> +       const char *strategy = !opts->xopts.nr &&
>                 (!opts->strategy ||
>                  !strcmp(opts->strategy, "recursive") ||
>                  !strcmp(opts->strategy, "ort")) ?
> @@ -4063,9 +4056,9 @@ static int do_merge(struct repository *r,
>                         strvec_push(&cmd.args, "octopus");
>                 else {
>                         strvec_push(&cmd.args, strategy);
> -                       for (k = 0; k < opts->xopts_nr; k++)
> +                       for (k = 0; k < opts->xopts.nr; k++)
>                                 strvec_pushf(&cmd.args,
> -                                            "-X%s", opts->xopts[k]);
> +                                            "-X%s", opts->xopts.v[k]);
>                 }
>                 if (!(flags & TODO_EDIT_MERGE_MSG))
>                         strvec_push(&cmd.args, "--no-edit");
> diff --git a/sequencer.h b/sequencer.h
> index 33dbaf5b66..8a79d6b200 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -2,6 +2,7 @@
>  #define SEQUENCER_H
>
>  #include "strbuf.h"
> +#include "strvec.h"
>  #include "wt-status.h"
>
>  struct commit;
> @@ -60,8 +61,7 @@ struct replay_opts {
>         /* Merge strategy */
>         char *default_strategy;  /* from config options */
>         char *strategy;
> -       char **xopts;
> -       size_t xopts_nr, xopts_alloc;
> +       struct strvec xopts;
>
>         /* Reflog */
>         char *reflog_action;
> @@ -80,7 +80,12 @@ struct replay_opts {
>         /* Private use */
>         const char *reflog_message;
>  };
> -#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }
> +#define REPLAY_OPTS_INIT {                     \
> +       .edit = -1,                             \
> +       .action = -1,                           \
> +       .current_fixups = STRBUF_INIT,          \
> +       .xopts = STRVEC_INIT,                   \
> +}
>
>  /*
>   * Note that ordering matters in this enum. Not only must it match the mapping
> --
> 2.40.0.670.g64ef305212.dirty

Simple, but very nice cleanup.  :-)

[PATCH v2 3/5] rebase -m: cleanup --strategy-option handling

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c               | 30 ++++++++++--------------------
 sequencer.c                    |  2 +-
 sequencer.h                    |  1 -
 t/t3436-rebase-more-options.sh | 10 ++++++++--
 4 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 3bd215c771..511922c6fc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,7 +117,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;
@@ -143,6 +144,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)
@@ -176,8 +178,8 @@ 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);
+	for (size_t i = 0; i < opts->strategy_opts.nr; i++)
+		strvec_push(&replay.xopts, opts->strategy_opts.items[i].string);
 
 	if (opts->squash_onto) {
 		oidcpy(&replay.squash_onto, opts->squash_onto);
@@ -1013,7 +1015,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;
@@ -1122,7 +1123,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")),
@@ -1436,23 +1437,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);
@@ -1827,10 +1818,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 6e2f3357c8..045d549042 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2913,7 +2913,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 8a79d6b200..913a0f652d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -252,7 +252,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
 '
-- 
2.40.0.670.g64ef305212.dirty

[PATCH v2 4/5] rebase -m: fix serialization of strategy options

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

To store the strategy options rebase prepends " --" to each one and
writes them to a file. To load them it reads the file and passes the
contents to split_cmdline(). This roughly mimics the behavior of the
scripted rebase but has a couple of limitations, (1) options containing
whitespace are not properly preserved (this is true of the scripted
rebase as well) and (2) options containing '"' or '\' are incorrectly
parsed and may cause the parser to return an error.

Fix these limitations by quoting each option when they are stored so
that they can be parsed correctly. Now that "--preserve-merges" no
longer exist this change also stops prepending "--" to the options when
they are stored as that was an artifact of the scripted rebase.

These changes are backwards compatible so the files written by an older
version of git can be still be read. They are also forwards compatible,
the file can still be parsed by recent versions of git as they treat the
"--" prefix as optional.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 alias.c                        | 18 +++++++++++++++++
 alias.h                        |  3 +++
 sequencer.c                    | 11 ++++++-----
 t/t3418-rebase-continue.sh     | 36 ++++++++++++++++++++++------------
 t/t3436-rebase-more-options.sh | 24 -----------------------
 5 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/alias.c b/alias.c
index e814948ced..54a1a23d2c 100644
--- a/alias.c
+++ b/alias.c
@@ -3,6 +3,7 @@
 #include "alloc.h"
 #include "config.h"
 #include "gettext.h"
+#include "strbuf.h"
 #include "string-list.h"
 
 struct config_alias_data {
@@ -46,6 +47,23 @@ void list_aliases(struct string_list *list)
 	read_early_config(config_alias_cb, &data);
 }
 
+void quote_cmdline(struct strbuf *buf, const char **argv)
+{
+	for (const char **argp = argv; *argp; argp++) {
+		if (argp != argv)
+			strbuf_addch(buf, ' ');
+		strbuf_addch(buf, '"');
+		for (const char *p = *argp; *p; p++) {
+			const char c = *p;
+
+			if (c == '"' || c =='\\')
+				strbuf_addch(buf, '\\');
+			strbuf_addch(buf, c);
+		}
+		strbuf_addch(buf, '"');
+	}
+}
+
 #define SPLIT_CMDLINE_BAD_ENDING 1
 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
 #define SPLIT_CMDLINE_ARGC_OVERFLOW 3
diff --git a/alias.h b/alias.h
index aef4843bb7..43db736484 100644
--- a/alias.h
+++ b/alias.h
@@ -1,9 +1,12 @@
 #ifndef ALIAS_H
 #define ALIAS_H
 
+struct strbuf;
 struct string_list;
 
 char *alias_lookup(const char *alias);
+/* Quote argv so buf can be parsed by split_cmdline() */
+void quote_cmdline(struct strbuf *buf, const char **argv);
 int split_cmdline(char *cmdline, const char ***argv);
 /* Takes a negative value returned by split_cmdline */
 const char *split_cmdline_strerror(int cmdline_errno);
diff --git a/sequencer.c b/sequencer.c
index 045d549042..9907100c8a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 
 	count = split_cmdline(strategy_opts_string, &argv);
 	if (count < 0)
-		die(_("could not split '%s': %s"), strategy_opts_string,
+		BUG("could not split '%s': %s", strategy_opts_string,
 			    split_cmdline_strerror(count));
 	for (i = 0; i < count; i++) {
 		const char *arg = argv[i];
@@ -3048,12 +3048,13 @@ static int read_populate_opts(struct replay_opts *opts)
 
 static void write_strategy_opts(struct replay_opts *opts)
 {
-	int i;
 	struct strbuf buf = STRBUF_INIT;
 
-	for (i = 0; i < opts->xopts.nr; ++i)
-		strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
-
+	/*
+	 * Quote strategy options so that they can be read correctly
+	 * by split_cmdline().
+	 */
+	quote_cmdline(&buf, opts->xopts.v);
 	write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
 	strbuf_release(&buf);
 }
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 130e2f9b55..42c3954125 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F2-on-topic-branch &&
 	test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run" &&
+	test_when_finished "rm -fr test-bin" &&
 	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
+
+	write_script test-bin/git-merge-funny <<-\EOF &&
+	printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual
+	shift 3 &&
+	exec git merge-recursive "$@"
+	EOF
+
+	cat >expect <<-\EOF &&
+	[7]
+	[--option=arg with space]
+	[--op"tion\]
+	[--new
+	line ]
+	[--]
+	EOF
+
+	rm -f actual &&
 	(
 		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -s funny -Xopt main topic
+		test_must_fail git rebase -s funny -X"option=arg with space" \
+				-Xop\"tion\\ -X"new${LF}line " main topic
 	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
+	test_cmp expect actual &&
+	rm actual &&
 	echo "Resolved" >F2 &&
 	git add F2 &&
 	(
 		PATH=./test-bin:$PATH &&
 		git rebase --continue
 	) &&
-	test -f funny.was.run
+	test_cmp expect actual
 '
 
 test_expect_success 'rebase -i --continue handles merge strategy and options' '
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 3adf42f47d..94671d3c46 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -40,30 +40,6 @@ test_expect_success 'setup' '
 	EOF
 '
 
-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
-	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
-	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
-'
-
 test_expect_success '--ignore-whitespace works with apply backend' '
 	test_must_fail git rebase --apply main side &&
 	git rebase --abort &&
-- 
2.40.0.670.g64ef305212.dirty

Re: [PATCH v2 4/5] rebase -m: fix serialization of strategy options

[Elijah Newren]

On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> To store the strategy options rebase prepends " --" to each one and
> writes them to a file. To load them it reads the file and passes the
> contents to split_cmdline(). This roughly mimics the behavior of the
> scripted rebase but has a couple of limitations, (1) options containing
> whitespace are not properly preserved (this is true of the scripted
> rebase as well) and (2) options containing '"' or '\' are incorrectly
> parsed and may cause the parser to return an error.
>
> Fix these limitations by quoting each option when they are stored so
> that they can be parsed correctly. Now that "--preserve-merges" no
> longer exist this change also stops prepending "--" to the options when
> they are stored as that was an artifact of the scripted rebase.
>
> These changes are backwards compatible so the files written by an older
> version of git can be still be read. They are also forwards compatible,

s/be still be/still be/

Sorry for not noticing this last time.

> the file can still be parsed by recent versions of git as they treat the
> "--" prefix as optional.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  alias.c                        | 18 +++++++++++++++++
>  alias.h                        |  3 +++
>  sequencer.c                    | 11 ++++++-----
>  t/t3418-rebase-continue.sh     | 36 ++++++++++++++++++++++------------
>  t/t3436-rebase-more-options.sh | 24 -----------------------
>  5 files changed, 50 insertions(+), 42 deletions(-)
>
> diff --git a/alias.c b/alias.c
> index e814948ced..54a1a23d2c 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -3,6 +3,7 @@
>  #include "alloc.h"
>  #include "config.h"
>  #include "gettext.h"
> +#include "strbuf.h"
>  #include "string-list.h"
>
>  struct config_alias_data {
> @@ -46,6 +47,23 @@ void list_aliases(struct string_list *list)
>         read_early_config(config_alias_cb, &data);
>  }
>
> +void quote_cmdline(struct strbuf *buf, const char **argv)
> +{
> +       for (const char **argp = argv; *argp; argp++) {
> +               if (argp != argv)
> +                       strbuf_addch(buf, ' ');
> +               strbuf_addch(buf, '"');
> +               for (const char *p = *argp; *p; p++) {
> +                       const char c = *p;
> +
> +                       if (c == '"' || c =='\\')
> +                               strbuf_addch(buf, '\\');
> +                       strbuf_addch(buf, c);
> +               }
> +               strbuf_addch(buf, '"');
> +       }
> +}
> +
>  #define SPLIT_CMDLINE_BAD_ENDING 1
>  #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
>  #define SPLIT_CMDLINE_ARGC_OVERFLOW 3
> diff --git a/alias.h b/alias.h
> index aef4843bb7..43db736484 100644
> --- a/alias.h
> +++ b/alias.h
> @@ -1,9 +1,12 @@
>  #ifndef ALIAS_H
>  #define ALIAS_H
>
> +struct strbuf;
>  struct string_list;
>
>  char *alias_lookup(const char *alias);
> +/* Quote argv so buf can be parsed by split_cmdline() */
> +void quote_cmdline(struct strbuf *buf, const char **argv);
>  int split_cmdline(char *cmdline, const char ***argv);
>  /* Takes a negative value returned by split_cmdline */
>  const char *split_cmdline_strerror(int cmdline_errno);
> diff --git a/sequencer.c b/sequencer.c
> index 045d549042..9907100c8a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
>
>         count = split_cmdline(strategy_opts_string, &argv);
>         if (count < 0)
> -               die(_("could not split '%s': %s"), strategy_opts_string,
> +               BUG("could not split '%s': %s", strategy_opts_string,
>                             split_cmdline_strerror(count));
>         for (i = 0; i < count; i++) {
>                 const char *arg = argv[i];
> @@ -3048,12 +3048,13 @@ static int read_populate_opts(struct replay_opts *opts)
>
>  static void write_strategy_opts(struct replay_opts *opts)
>  {
> -       int i;
>         struct strbuf buf = STRBUF_INIT;
>
> -       for (i = 0; i < opts->xopts.nr; ++i)
> -               strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
> -
> +       /*
> +        * Quote strategy options so that they can be read correctly
> +        * by split_cmdline().
> +        */
> +       quote_cmdline(&buf, opts->xopts.v);
>         write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
>         strbuf_release(&buf);
>  }
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 130e2f9b55..42c3954125 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
>         rm -fr .git/rebase-* &&
>         git reset --hard commit-new-file-F2-on-topic-branch &&
>         test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
> -       test_when_finished "rm -fr test-bin funny.was.run" &&
> +       test_when_finished "rm -fr test-bin" &&
>         mkdir test-bin &&
> -       cat >test-bin/git-merge-funny <<-EOF &&
> -       #!$SHELL_PATH
> -       case "\$1" in --opt) ;; *) exit 2 ;; esac
> -       shift &&
> -       >funny.was.run &&
> -       exec git merge-recursive "\$@"
> -       EOF
> -       chmod +x test-bin/git-merge-funny &&
> +
> +       write_script test-bin/git-merge-funny <<-\EOF &&
> +       printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual
> +       shift 3 &&
> +       exec git merge-recursive "$@"
> +       EOF
> +
> +       cat >expect <<-\EOF &&
> +       [7]
> +       [--option=arg with space]
> +       [--op"tion\]
> +       [--new
> +       line ]
> +       [--]
> +       EOF
> +
> +       rm -f actual &&
>         (
>                 PATH=./test-bin:$PATH &&
> -               test_must_fail git rebase -s funny -Xopt main topic
> +               test_must_fail git rebase -s funny -X"option=arg with space" \
> +                               -Xop\"tion\\ -X"new${LF}line " main topic
>         ) &&
> -       test -f funny.was.run &&
> -       rm funny.was.run &&
> +       test_cmp expect actual &&
> +       rm actual &&
>         echo "Resolved" >F2 &&
>         git add F2 &&
>         (
>                 PATH=./test-bin:$PATH &&
>                 git rebase --continue
>         ) &&
> -       test -f funny.was.run
> +       test_cmp expect actual
>  '
>
>  test_expect_success 'rebase -i --continue handles merge strategy and options' '
> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> index 3adf42f47d..94671d3c46 100755
> --- a/t/t3436-rebase-more-options.sh
> +++ b/t/t3436-rebase-more-options.sh
> @@ -40,30 +40,6 @@ test_expect_success 'setup' '
>         EOF
>  '
>
> -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
> -       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
> -       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
> -'
> -
>  test_expect_success '--ignore-whitespace works with apply backend' '
>         test_must_fail git rebase --apply main side &&
>         git rebase --abort &&
> --
> 2.40.0.670.g64ef305212.dirty

Looks good.

[PATCH v2 5/5] rebase: remove a couple of redundant strategy tests

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Remove a test in t3402 that has been redundant ever since 80ff47957b
(rebase: remember strategy and strategy options, 2011-02-06).  That
commit added a new test, the first part of which (as noted in the old
commit message) duplicated an existing test.

Also remove a test t3418 that has been redundant since the merge backend
was removed in 68aa495b59 (rebase: implement --merge via the interactive
machinery, 2018-12-11), since it now tests the same code paths as the
preceding test.

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3402-rebase-merge.sh    | 21 ---------------------
 t/t3418-rebase-continue.sh | 32 --------------------------------
 2 files changed, 53 deletions(-)

diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
index 7e46f4ca85..79b0640c00 100755
--- a/t/t3402-rebase-merge.sh
+++ b/t/t3402-rebase-merge.sh
@@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '
 	esac
 '
 
-test_expect_success 'rebase -s funny -Xopt' '
-	test_when_finished "rm -fr test-bin funny.was.run" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	git reset --hard &&
-	git checkout -b test-funny main^ &&
-	test_commit funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase -s funny -Xopt main
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase --skip works with two conflicts in a row' '
 	git checkout second-side  &&
 	tr "[A-Z]" "[a-z]" <newfile >tmp &&
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 42c3954125..2d0789e554 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --continue handles merge strategy and options' '
-	rm -fr .git/rebase-* &&
-	git reset --hard commit-new-file-F2-on-topic-branch &&
-	test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	echo "\$@" >>funny.args
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	case "\$2" in --foo) ;; *) exit 2 ;; esac
-	case "\$4" in --) ;; *) exit 2 ;; esac
-	shift 2 &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic
-	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
-	echo "Resolved" >F2 &&
-	git add F2 &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase --continue
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase -r passes merge strategy options correctly' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&
-- 
2.40.0.670.g64ef305212.dirty

Re: [PATCH v2 0/5] rebase: cleanup merge strategy option handling

[Elijah Newren]

On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Cleanup the handling of --strategy-option now that we no longer need to
> support "--preserve-merges" and properly quote the argument when saving
> it to disc.
>
> Thanks to Elijah for his comments on V1.
>
> Changes since V1:
>
> I've rebased these patches onto 'master' to avoid conflicts with
> 'sg/parse-options-h-initializers' in the new patch 2 (this series
> depends on 'ab/fix-strategy-opts-parsing' but that has now been merged
> to master).
>
> Patch 1 - Unchanged.
>
> Patch 2 - New patch to store the merge strategy options in an "struct
>           strvec". This patch also introduces a new macro OPT_STRVEC()
>           to collect options into an "struct strvec".
>
> Patch 3 - Small simplification due to the changes in patch 2.
>
> Patch 4 - Moved the code to quote a list so it can split by
>           split_cmdline() into a new function quote_cmdline() as
>           suggested by Elijah.
>
> Patch 5 - Reworded the commit message as suggested by Elijah.

I noticed a small typo/grammo in the commit message of Patch 4 that I
missed in V1, but otherwise this series looks good.  I'm not sure
fixing the tiny grammo is even all that important; I'll leave it up to
you for whether you want to bother re-rolling to fix it.  Either way:

Reviewed-by: Elijah Newren <newren@gmail.com>

Re: [PATCH v2 0/5] rebase: cleanup merge strategy option handling

[Phillip Wood]

Hi Elijah

On 07/04/2023 06:49, Elijah Newren wrote:
> On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> I noticed a small typo/grammo in the commit message of Patch 4 that I
> missed in V1, but otherwise this series looks good.  I'm not sure
> fixing the tiny grammo is even all that important; I'll leave it up to
> you for whether you want to bother re-rolling to fix it.  Either way:
> 
> Reviewed-by: Elijah Newren <newren@gmail.com>

Thanks for reviewing this series. I've sent v3 with the typo fix and 
your Reviewed-by: trailer.

Best Wishes

Phillip

Re: [PATCH v2 0/5] rebase: cleanup merge strategy option handling

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Thanks for reviewing this series. I've sent v3 with the typo fix and
> your Reviewed-by: trailer.

Thanks, both, for writing, reviewing and polishing.

[PATCH v3 0/5] rebase: cleanup merge strategy option handling

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Cleanup the handling of --strategy-option now that we no longer need to
support "--preserve-merges" and properly quote the argument when saving
it to disc.

Thanks to Elijah for his comments.

Changes since V2:

 - Added Elijah's Reviewed-by: trailer.
 - Fixed a typo in patch 4 spotted by Elijah.

Changes since V1:

I've rebased these patches onto 'master' to avoid conflicts with
'sg/parse-options-h-initializers' in the new patch 2 (this series
depends on 'ab/fix-strategy-opts-parsing' but that has now been merged
to master).

Patch 1 - Unchanged.

Patch 2 - New patch to store the merge strategy options in an "struct
          strvec". This patch also introduces a new macro OPT_STRVEC()
          to collect options into an "struct strvec".

Patch 3 - Small simplification due to the changes in patch 2.

Patch 4 - Moved the code to quote a list so it can split by
          split_cmdline() into a new function quote_cmdline() as
	  suggested by Elijah.

Patch 5 - Reworded the commit message as suggested by Elijah.


Base-Commit: 140b9478dad5d19543c1cb4fd293ccec228f1240
Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv3
View-Changes-At: https://github.com/phillipwood/git/compare/140b9478d...2d1d32811
Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v3

Phillip Wood (5):
  rebase: stop reading and writing unnecessary strategy state
  sequencer: use struct strvec to store merge strategy options
  rebase -m: cleanup --strategy-option handling
  rebase -m: fix serialization of strategy options
  rebase: remove a couple of redundant strategy tests

 alias.c                        | 18 +++++++
 alias.h                        |  3 ++
 builtin/rebase.c               | 54 ++++-----------------
 builtin/revert.c               | 20 ++------
 parse-options-cb.c             | 16 +++++++
 parse-options.h                | 10 ++++
 sequencer.c                    | 57 ++++++++++------------
 sequencer.h                    | 12 +++--
 t/t3402-rebase-merge.sh        | 21 --------
 t/t3418-rebase-continue.sh     | 88 +++++++++++++---------------------
 t/t3436-rebase-more-options.sh | 18 -------
 11 files changed, 126 insertions(+), 191 deletions(-)

Range-diff against v2:
1:  2353c753f5 ! 1:  882b403423 rebase: stop reading and writing unnecessary strategy state
    @@ Commit message
         writing of state files between builtin/rebase.c and sequencer.c but that
         is left for a follow up series.
     
    +    Reviewed-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## builtin/rebase.c ##
2:  dd7b82cdd5 ! 2:  1d8e59aa16 sequencer: use struct strvec to store merge strategy options
    @@ Commit message
         revert" as passing “--no-strategy-option” will now clear any previous
         strategy options whereas before this change it did nothing.
     
    +    Reviewed-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## builtin/revert.c ##
3:  ccef0e6f4b ! 3:  e98ef5ce8c rebase -m: cleanup --strategy-option handling
    @@ Commit message
         mixed blessing but the next commit will fix the root cause of the
         parsing problem so lets not worry about that here.
     
    +    Reviewed-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## builtin/rebase.c ##
4:  9a90212ef2 ! 4:  a5e940e2d0 rebase -m: fix serialization of strategy options
    @@ Commit message
         they are stored as that was an artifact of the scripted rebase.
     
         These changes are backwards compatible so the files written by an older
    -    version of git can be still be read. They are also forwards compatible,
    +    version of git can still be read. They are also forwards compatible,
         the file can still be parsed by recent versions of git as they treat the
         "--" prefix as optional.
     
    +    Reviewed-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## alias.c ##
5:  3515c31b40 ! 5:  2d1d328110 rebase: remove a couple of redundant strategy tests
    @@ Commit message
         preceding test.
     
         Helped-by: Elijah Newren <newren@gmail.com>
    +    Reviewed-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## t/t3402-rebase-merge.sh ##
-- 
2.40.0.670.g64ef305212.dirty

[PATCH v3 1/5] rebase: stop reading and writing unnecessary strategy state

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The state files for "--strategy" and "--strategy-option" are written and
read twice, once by builtin/rebase.c and then by sequencer.c. This is an
artifact of the scripted rebase and the need to support "rebase
--preserve-merges". Now that "--preserve-merges" no-longer exists we
only need to read and write these files in sequencer.c. This enables us
to remove a call to free() in read_strategy_opts() that was added by
f1f4ebf432 (sequencer.c: fix "opts->strategy" leak in
read_strategy_opts(), 2022-11-08) as this commit fixes the root cause of
that leak.

There is further scope for removing duplication in the reading and
writing of state files between builtin/rebase.c and sequencer.c but that
is left for a follow up series.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 24 ------------------------
 sequencer.c      |  1 -
 2 files changed, 25 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b7b908b66..3bd215c771 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -483,24 +483,6 @@ static int read_basic_state(struct rebase_options *opts)
 		opts->gpg_sign_opt = xstrdup(buf.buf);
 	}
 
-	if (file_exists(state_dir_path("strategy", opts))) {
-		strbuf_reset(&buf);
-		if (!read_oneliner(&buf, state_dir_path("strategy", opts),
-				   READ_ONELINER_WARN_MISSING))
-			return -1;
-		free(opts->strategy);
-		opts->strategy = xstrdup(buf.buf);
-	}
-
-	if (file_exists(state_dir_path("strategy_opts", opts))) {
-		strbuf_reset(&buf);
-		if (!read_oneliner(&buf, state_dir_path("strategy_opts", opts),
-				   READ_ONELINER_WARN_MISSING))
-			return -1;
-		free(opts->strategy_opts);
-		opts->strategy_opts = xstrdup(buf.buf);
-	}
-
 	strbuf_release(&buf);
 
 	return 0;
@@ -518,12 +500,6 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 		write_file(state_dir_path("quiet", opts), "%s", "");
 	if (opts->flags & REBASE_VERBOSE)
 		write_file(state_dir_path("verbose", opts), "%s", "");
-	if (opts->strategy)
-		write_file(state_dir_path("strategy", opts), "%s",
-			   opts->strategy);
-	if (opts->strategy_opts)
-		write_file(state_dir_path("strategy_opts", opts), "%s",
-			   opts->strategy_opts);
 	if (opts->allow_rerere_autoupdate > 0)
 		write_file(state_dir_path("allow_rerere_autoupdate", opts),
 			   "-%s-rerere-autoupdate",
diff --git a/sequencer.c b/sequencer.c
index 3be23d7ca2..c35a67e104 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2944,7 +2944,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 	strbuf_reset(buf);
 	if (!read_oneliner(buf, rebase_path_strategy(), 0))
 		return;
-	free(opts->strategy);
 	opts->strategy = strbuf_detach(buf, NULL);
 	if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
 		return;
-- 
2.40.0.670.g64ef305212.dirty

[PATCH v3 2/5] sequencer: use struct strvec to store merge strategy options

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The sequencer stores the merge strategy options in an array of strings
which allocated with ALLOC_GROW(). Using "struct strvec" avoids manually
managing the memory of that array and simplifies the code.

Aside from memory allocation the changes to the sequencer are largely
mechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A
new option parsing macro OPT_STRVEC() is also added to collect the
strategy options.  Hopefully this can be used to simplify the code in
builtin/merge.c in the future.

Note that there is a change of behavior to "git cherry-pick" and "git
revert" as passing “--no-strategy-option” will now clear any previous
strategy options whereas before this change it did nothing.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/revert.c   | 20 +++-----------------
 parse-options-cb.c | 16 ++++++++++++++++
 parse-options.h    | 10 ++++++++++
 sequencer.c        | 47 ++++++++++++++++++++--------------------------
 sequencer.h        | 11 ++++++++---
 5 files changed, 57 insertions(+), 47 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 62986a7b1b..362857da65 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
 }
 
-static int option_parse_x(const struct option *opt,
-			  const char *arg, int unset)
-{
-	struct replay_opts **opts_ptr = opt->value;
-	struct replay_opts *opts = *opts_ptr;
-
-	if (unset)
-		return 0;
-
-	ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-	opts->xopts[opts->xopts_nr++] = xstrdup(arg);
-	return 0;
-}
-
 static int option_parse_m(const struct option *opt,
 			  const char *arg, int unset)
 {
@@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 			     N_("select mainline parent"), option_parse_m),
 		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
 		OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")),
-		OPT_CALLBACK('X', "strategy-option", &opts, N_("option"),
-			N_("option for merge strategy"), option_parse_x),
+		OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"),
+			N_("option for merge strategy")),
 		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		OPT_END()
@@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 				"--signoff", opts->signoff,
 				"--mainline", opts->mainline,
 				"--strategy", opts->strategy ? 1 : 0,
-				"--strategy-option", opts->xopts ? 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,
diff --git a/parse-options-cb.c b/parse-options-cb.c
index d346dbe210..8eb96c5ca9 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
+{
+	struct strvec *v = opt->value;
+
+	if (unset) {
+		strvec_clear(v);
+		return 0;
+	}
+
+	if (!arg)
+		return -1;
+
+	strvec_push(v, arg);
+	return 0;
+}
+
 int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
 {
 	return 0;
diff --git a/parse-options.h b/parse-options.h
index 26f19384e5..2d1d4d052f 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -285,6 +285,15 @@ struct option {
 	.help = (h), \
 	.callback = &parse_opt_string_list, \
 }
+#define OPT_STRVEC(s, l, v, a, h) { \
+	.type = OPTION_CALLBACK, \
+	.short_name = (s), \
+	.long_name = (l), \
+	.value = (v), \
+	.argh = (a), \
+	.help = (h), \
+	.callback = &parse_opt_strvec, \
+}
 #define OPT_UYN(s, l, v, h) { \
 	.type = OPTION_CALLBACK, \
 	.short_name = (s), \
@@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int);
 int parse_opt_commit(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
+int parse_opt_strvec(const struct option *, const char *, int);
 int parse_opt_noop_cb(const struct option *, const char *, int);
 enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
 					   const struct option *,
diff --git a/sequencer.c b/sequencer.c
index c35a67e104..6e2f3357c8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts)
 	free(opts->reflog_action);
 	free(opts->default_strategy);
 	free(opts->strategy);
-	for (size_t i = 0; i < opts->xopts_nr; i++)
-		free(opts->xopts[i]);
-	free(opts->xopts);
+	strvec_clear (&opts->xopts);
 	strbuf_release(&opts->current_fixups);
 	if (opts->revs)
 		release_revisions(opts->revs);
@@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r,
 	next_tree = next ? get_commit_tree(next) : empty_tree(r);
 	base_tree = base ? get_commit_tree(base) : empty_tree(r);
 
-	for (i = 0; i < opts->xopts_nr; i++)
-		parse_merge_opt(&o, opts->xopts[i]);
+	for (i = 0; i < opts->xopts.nr; i++)
+		parse_merge_opt(&o, opts->xopts.v[i]);
 
 	if (!opts->strategy || !strcmp(opts->strategy, "ort")) {
 		memset(&result, 0, sizeof(result));
@@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r,
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
 		res |= try_merge_command(r, opts->strategy,
-					 opts->xopts_nr, (const char **)opts->xopts,
+					 opts->xopts.nr, opts->xopts.v,
 					common, oid_to_hex(&head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
@@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 	else if (!strcmp(key, "options.gpg-sign"))
 		git_config_string_dup(&opts->gpg_sign, key, value);
 	else if (!strcmp(key, "options.strategy-option")) {
-		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-		opts->xopts[opts->xopts_nr++] = xstrdup(value);
+		strvec_push(&opts->xopts, value);
 	} else if (!strcmp(key, "options.allow-rerere-auto"))
 		opts->allow_rerere_auto =
 			git_config_bool_or_int(key, value, &error_flag) ?
@@ -2920,22 +2917,21 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 {
 	int i;
 	int count;
+	const char **argv;
 	char *strategy_opts_string = raw_opts;
 
 	if (*strategy_opts_string == ' ')
 		strategy_opts_string++;
 
-	count = split_cmdline(strategy_opts_string,
-			      (const char ***)&opts->xopts);
+	count = split_cmdline(strategy_opts_string, &argv);
 	if (count < 0)
 		die(_("could not split '%s': %s"), strategy_opts_string,
 			    split_cmdline_strerror(count));
-	opts->xopts_nr = count;
-	for (i = 0; i < opts->xopts_nr; i++) {
-		const char *arg = opts->xopts[i];
+	for (i = 0; i < count; i++) {
+		const char *arg = argv[i];
 
 		skip_prefix(arg, "--", &arg);
-		opts->xopts[i] = xstrdup(arg);
+		strvec_push(&opts->xopts, arg);
 	}
 }
 
@@ -3055,8 +3051,8 @@ static void write_strategy_opts(struct replay_opts *opts)
 	int i;
 	struct strbuf buf = STRBUF_INIT;
 
-	for (i = 0; i < opts->xopts_nr; ++i)
-		strbuf_addf(&buf, " --%s", opts->xopts[i]);
+	for (i = 0; i < opts->xopts.nr; ++i)
+		strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
 
 	write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
 	strbuf_release(&buf);
@@ -3080,7 +3076,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_verbose(), "%s", "");
 	if (opts->strategy)
 		write_file(rebase_path_strategy(), "%s\n", opts->strategy);
-	if (opts->xopts_nr > 0)
+	if (opts->xopts.nr > 0)
 		write_strategy_opts(opts);
 
 	if (opts->allow_rerere_auto == RERERE_AUTOUPDATE)
@@ -3464,13 +3460,10 @@ static int save_opts(struct replay_opts *opts)
 	if (opts->gpg_sign)
 		res |= git_config_set_in_file_gently(opts_file,
 					"options.gpg-sign", opts->gpg_sign);
-	if (opts->xopts) {
-		int i;
-		for (i = 0; i < opts->xopts_nr; i++)
-			res |= git_config_set_multivar_in_file_gently(opts_file,
-					"options.strategy-option",
-					opts->xopts[i], "^$", 0);
-	}
+	for (size_t i = 0; i < opts->xopts.nr; i++)
+		res |= git_config_set_multivar_in_file_gently(opts_file,
+				"options.strategy-option",
+				opts->xopts.v[i], "^$", 0);
 	if (opts->allow_rerere_auto)
 		res |= git_config_set_in_file_gently(opts_file,
 				"options.allow-rerere-auto",
@@ -3880,7 +3873,7 @@ static int do_merge(struct repository *r,
 	struct commit *head_commit, *merge_commit, *i;
 	struct commit_list *bases, *j;
 	struct commit_list *to_merge = NULL, **tail = &to_merge;
-	const char *strategy = !opts->xopts_nr &&
+	const char *strategy = !opts->xopts.nr &&
 		(!opts->strategy ||
 		 !strcmp(opts->strategy, "recursive") ||
 		 !strcmp(opts->strategy, "ort")) ?
@@ -4063,9 +4056,9 @@ static int do_merge(struct repository *r,
 			strvec_push(&cmd.args, "octopus");
 		else {
 			strvec_push(&cmd.args, strategy);
-			for (k = 0; k < opts->xopts_nr; k++)
+			for (k = 0; k < opts->xopts.nr; k++)
 				strvec_pushf(&cmd.args,
-					     "-X%s", opts->xopts[k]);
+					     "-X%s", opts->xopts.v[k]);
 		}
 		if (!(flags & TODO_EDIT_MERGE_MSG))
 			strvec_push(&cmd.args, "--no-edit");
diff --git a/sequencer.h b/sequencer.h
index 33dbaf5b66..8a79d6b200 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -2,6 +2,7 @@
 #define SEQUENCER_H
 
 #include "strbuf.h"
+#include "strvec.h"
 #include "wt-status.h"
 
 struct commit;
@@ -60,8 +61,7 @@ struct replay_opts {
 	/* Merge strategy */
 	char *default_strategy;  /* from config options */
 	char *strategy;
-	char **xopts;
-	size_t xopts_nr, xopts_alloc;
+	struct strvec xopts;
 
 	/* Reflog */
 	char *reflog_action;
@@ -80,7 +80,12 @@ struct replay_opts {
 	/* Private use */
 	const char *reflog_message;
 };
-#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }
+#define REPLAY_OPTS_INIT {			\
+	.edit = -1,				\
+	.action = -1,				\
+	.current_fixups = STRBUF_INIT,		\
+	.xopts = STRVEC_INIT,			\
+}
 
 /*
  * Note that ordering matters in this enum. Not only must it match the mapping
-- 
2.40.0.670.g64ef305212.dirty

[PATCH v3 3/5] rebase -m: cleanup --strategy-option handling

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c               | 30 ++++++++++--------------------
 sequencer.c                    |  2 +-
 sequencer.h                    |  1 -
 t/t3436-rebase-more-options.sh | 10 ++++++++--
 4 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 3bd215c771..511922c6fc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,7 +117,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;
@@ -143,6 +144,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)
@@ -176,8 +178,8 @@ 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);
+	for (size_t i = 0; i < opts->strategy_opts.nr; i++)
+		strvec_push(&replay.xopts, opts->strategy_opts.items[i].string);
 
 	if (opts->squash_onto) {
 		oidcpy(&replay.squash_onto, opts->squash_onto);
@@ -1013,7 +1015,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;
@@ -1122,7 +1123,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")),
@@ -1436,23 +1437,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);
@@ -1827,10 +1818,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 6e2f3357c8..045d549042 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2913,7 +2913,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 8a79d6b200..913a0f652d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -252,7 +252,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
 '
-- 
2.40.0.670.g64ef305212.dirty

[PATCH v3 4/5] rebase -m: fix serialization of strategy options

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

To store the strategy options rebase prepends " --" to each one and
writes them to a file. To load them it reads the file and passes the
contents to split_cmdline(). This roughly mimics the behavior of the
scripted rebase but has a couple of limitations, (1) options containing
whitespace are not properly preserved (this is true of the scripted
rebase as well) and (2) options containing '"' or '\' are incorrectly
parsed and may cause the parser to return an error.

Fix these limitations by quoting each option when they are stored so
that they can be parsed correctly. Now that "--preserve-merges" no
longer exist this change also stops prepending "--" to the options when
they are stored as that was an artifact of the scripted rebase.

These changes are backwards compatible so the files written by an older
version of git can still be read. They are also forwards compatible,
the file can still be parsed by recent versions of git as they treat the
"--" prefix as optional.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 alias.c                        | 18 +++++++++++++++++
 alias.h                        |  3 +++
 sequencer.c                    | 11 ++++++-----
 t/t3418-rebase-continue.sh     | 36 ++++++++++++++++++++++------------
 t/t3436-rebase-more-options.sh | 24 -----------------------
 5 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/alias.c b/alias.c
index e814948ced..54a1a23d2c 100644
--- a/alias.c
+++ b/alias.c
@@ -3,6 +3,7 @@
 #include "alloc.h"
 #include "config.h"
 #include "gettext.h"
+#include "strbuf.h"
 #include "string-list.h"
 
 struct config_alias_data {
@@ -46,6 +47,23 @@ void list_aliases(struct string_list *list)
 	read_early_config(config_alias_cb, &data);
 }
 
+void quote_cmdline(struct strbuf *buf, const char **argv)
+{
+	for (const char **argp = argv; *argp; argp++) {
+		if (argp != argv)
+			strbuf_addch(buf, ' ');
+		strbuf_addch(buf, '"');
+		for (const char *p = *argp; *p; p++) {
+			const char c = *p;
+
+			if (c == '"' || c =='\\')
+				strbuf_addch(buf, '\\');
+			strbuf_addch(buf, c);
+		}
+		strbuf_addch(buf, '"');
+	}
+}
+
 #define SPLIT_CMDLINE_BAD_ENDING 1
 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
 #define SPLIT_CMDLINE_ARGC_OVERFLOW 3
diff --git a/alias.h b/alias.h
index aef4843bb7..43db736484 100644
--- a/alias.h
+++ b/alias.h
@@ -1,9 +1,12 @@
 #ifndef ALIAS_H
 #define ALIAS_H
 
+struct strbuf;
 struct string_list;
 
 char *alias_lookup(const char *alias);
+/* Quote argv so buf can be parsed by split_cmdline() */
+void quote_cmdline(struct strbuf *buf, const char **argv);
 int split_cmdline(char *cmdline, const char ***argv);
 /* Takes a negative value returned by split_cmdline */
 const char *split_cmdline_strerror(int cmdline_errno);
diff --git a/sequencer.c b/sequencer.c
index 045d549042..9907100c8a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 
 	count = split_cmdline(strategy_opts_string, &argv);
 	if (count < 0)
-		die(_("could not split '%s': %s"), strategy_opts_string,
+		BUG("could not split '%s': %s", strategy_opts_string,
 			    split_cmdline_strerror(count));
 	for (i = 0; i < count; i++) {
 		const char *arg = argv[i];
@@ -3048,12 +3048,13 @@ static int read_populate_opts(struct replay_opts *opts)
 
 static void write_strategy_opts(struct replay_opts *opts)
 {
-	int i;
 	struct strbuf buf = STRBUF_INIT;
 
-	for (i = 0; i < opts->xopts.nr; ++i)
-		strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
-
+	/*
+	 * Quote strategy options so that they can be read correctly
+	 * by split_cmdline().
+	 */
+	quote_cmdline(&buf, opts->xopts.v);
 	write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
 	strbuf_release(&buf);
 }
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 130e2f9b55..42c3954125 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F2-on-topic-branch &&
 	test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run" &&
+	test_when_finished "rm -fr test-bin" &&
 	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
+
+	write_script test-bin/git-merge-funny <<-\EOF &&
+	printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual
+	shift 3 &&
+	exec git merge-recursive "$@"
+	EOF
+
+	cat >expect <<-\EOF &&
+	[7]
+	[--option=arg with space]
+	[--op"tion\]
+	[--new
+	line ]
+	[--]
+	EOF
+
+	rm -f actual &&
 	(
 		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -s funny -Xopt main topic
+		test_must_fail git rebase -s funny -X"option=arg with space" \
+				-Xop\"tion\\ -X"new${LF}line " main topic
 	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
+	test_cmp expect actual &&
+	rm actual &&
 	echo "Resolved" >F2 &&
 	git add F2 &&
 	(
 		PATH=./test-bin:$PATH &&
 		git rebase --continue
 	) &&
-	test -f funny.was.run
+	test_cmp expect actual
 '
 
 test_expect_success 'rebase -i --continue handles merge strategy and options' '
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 3adf42f47d..94671d3c46 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -40,30 +40,6 @@ test_expect_success 'setup' '
 	EOF
 '
 
-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
-	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
-	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
-'
-
 test_expect_success '--ignore-whitespace works with apply backend' '
 	test_must_fail git rebase --apply main side &&
 	git rebase --abort &&
-- 
2.40.0.670.g64ef305212.dirty

[PATCH v3 5/5] rebase: remove a couple of redundant strategy tests

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Remove a test in t3402 that has been redundant ever since 80ff47957b
(rebase: remember strategy and strategy options, 2011-02-06).  That
commit added a new test, the first part of which (as noted in the old
commit message) duplicated an existing test.

Also remove a test t3418 that has been redundant since the merge backend
was removed in 68aa495b59 (rebase: implement --merge via the interactive
machinery, 2018-12-11), since it now tests the same code paths as the
preceding test.

Helped-by: Elijah Newren <newren@gmail.com>
Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3402-rebase-merge.sh    | 21 ---------------------
 t/t3418-rebase-continue.sh | 32 --------------------------------
 2 files changed, 53 deletions(-)

diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
index 7e46f4ca85..79b0640c00 100755
--- a/t/t3402-rebase-merge.sh
+++ b/t/t3402-rebase-merge.sh
@@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '
 	esac
 '
 
-test_expect_success 'rebase -s funny -Xopt' '
-	test_when_finished "rm -fr test-bin funny.was.run" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	git reset --hard &&
-	git checkout -b test-funny main^ &&
-	test_commit funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase -s funny -Xopt main
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase --skip works with two conflicts in a row' '
 	git checkout second-side  &&
 	tr "[A-Z]" "[a-z]" <newfile >tmp &&
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 42c3954125..2d0789e554 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --continue handles merge strategy and options' '
-	rm -fr .git/rebase-* &&
-	git reset --hard commit-new-file-F2-on-topic-branch &&
-	test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	echo "\$@" >>funny.args
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	case "\$2" in --foo) ;; *) exit 2 ;; esac
-	case "\$4" in --) ;; *) exit 2 ;; esac
-	shift 2 &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic
-	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
-	echo "Resolved" >F2 &&
-	git add F2 &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase --continue
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase -r passes merge strategy options correctly' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&
-- 
2.40.0.670.g64ef305212.dirty

[PATCH v4 0/5] rebase: cleanup merge strategy option handling

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Cleanup the handling of --strategy-option now that we no longer need to
support "--preserve-merges" and properly quote the argument when saving
it to disc.

A hopefully final re-roll to fix a memory leak I spotted in V3.

Changes since V3:
 - Fixed a memory leak added in V3. The array returned by
   split_cmdline() is allocated on the heap so we need to fee it. The
   array elements come from the string passed to split_cmdline() so do
   not need to be individually freed.

Changes since V2:

 - Added Elijah's Reviewed-by: trailer.
 - Fixed a typo in patch 4 spotted by Elijah.

Changes since V1:

I've rebased these patches onto 'master' to avoid conflicts with
'sg/parse-options-h-initializers' in the new patch 2 (this series
depends on 'ab/fix-strategy-opts-parsing' but that has now been merged
to master).

Patch 1 - Unchanged.

Patch 2 - New patch to store the merge strategy options in an "struct
          strvec". This patch also introduces a new macro OPT_STRVEC()
          to collect options into an "struct strvec".

Patch 3 - Small simplification due to the changes in patch 2.

Patch 4 - Moved the code to quote a list so it can split by
          split_cmdline() into a new function quote_cmdline() as
	  suggested by Elijah.

Patch 5 - Reworded the commit message as suggested by Elijah.


Base-Commit: 140b9478dad5d19543c1cb4fd293ccec228f1240
Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv4
View-Changes-At: https://github.com/phillipwood/git/compare/140b9478d...7de1aa101
Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v4

Phillip Wood (5):
  rebase: stop reading and writing unnecessary strategy state
  sequencer: use struct strvec to store merge strategy options
  rebase -m: cleanup --strategy-option handling
  rebase -m: fix serialization of strategy options
  rebase: remove a couple of redundant strategy tests

 alias.c                        | 18 +++++++
 alias.h                        |  3 ++
 builtin/rebase.c               | 54 ++++-----------------
 builtin/revert.c               | 20 ++------
 parse-options-cb.c             | 16 +++++++
 parse-options.h                | 10 ++++
 sequencer.c                    | 58 ++++++++++------------
 sequencer.h                    | 12 +++--
 t/t3402-rebase-merge.sh        | 21 --------
 t/t3418-rebase-continue.sh     | 88 +++++++++++++---------------------
 t/t3436-rebase-more-options.sh | 18 -------
 11 files changed, 127 insertions(+), 191 deletions(-)

Range-diff against v3:
1:  882b403423 = 1:  882b403423 rebase: stop reading and writing unnecessary strategy state
2:  1d8e59aa16 ! 2:  4b288e883d sequencer: use struct strvec to store merge strategy options
    @@ sequencer.c: void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
     -		opts->xopts[i] = xstrdup(arg);
     +		strvec_push(&opts->xopts, arg);
      	}
    ++	free(argv);
      }
      
    + static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
     @@ sequencer.c: static void write_strategy_opts(struct replay_opts *opts)
      	int i;
      	struct strbuf buf = STRBUF_INIT;
3:  e98ef5ce8c = 3:  4e040c1214 rebase -m: cleanup --strategy-option handling
4:  a5e940e2d0 = 4:  671ee03503 rebase -m: fix serialization of strategy options
5:  2d1d328110 = 5:  7de1aa1016 rebase: remove a couple of redundant strategy tests
-- 
2.40.0.672.gbd2d2ac924

[PATCH v4 1/5] rebase: stop reading and writing unnecessary strategy state

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The state files for "--strategy" and "--strategy-option" are written and
read twice, once by builtin/rebase.c and then by sequencer.c. This is an
artifact of the scripted rebase and the need to support "rebase
--preserve-merges". Now that "--preserve-merges" no-longer exists we
only need to read and write these files in sequencer.c. This enables us
to remove a call to free() in read_strategy_opts() that was added by
f1f4ebf432 (sequencer.c: fix "opts->strategy" leak in
read_strategy_opts(), 2022-11-08) as this commit fixes the root cause of
that leak.

There is further scope for removing duplication in the reading and
writing of state files between builtin/rebase.c and sequencer.c but that
is left for a follow up series.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 24 ------------------------
 sequencer.c      |  1 -
 2 files changed, 25 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b7b908b66..3bd215c771 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -483,24 +483,6 @@ static int read_basic_state(struct rebase_options *opts)
 		opts->gpg_sign_opt = xstrdup(buf.buf);
 	}
 
-	if (file_exists(state_dir_path("strategy", opts))) {
-		strbuf_reset(&buf);
-		if (!read_oneliner(&buf, state_dir_path("strategy", opts),
-				   READ_ONELINER_WARN_MISSING))
-			return -1;
-		free(opts->strategy);
-		opts->strategy = xstrdup(buf.buf);
-	}
-
-	if (file_exists(state_dir_path("strategy_opts", opts))) {
-		strbuf_reset(&buf);
-		if (!read_oneliner(&buf, state_dir_path("strategy_opts", opts),
-				   READ_ONELINER_WARN_MISSING))
-			return -1;
-		free(opts->strategy_opts);
-		opts->strategy_opts = xstrdup(buf.buf);
-	}
-
 	strbuf_release(&buf);
 
 	return 0;
@@ -518,12 +500,6 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 		write_file(state_dir_path("quiet", opts), "%s", "");
 	if (opts->flags & REBASE_VERBOSE)
 		write_file(state_dir_path("verbose", opts), "%s", "");
-	if (opts->strategy)
-		write_file(state_dir_path("strategy", opts), "%s",
-			   opts->strategy);
-	if (opts->strategy_opts)
-		write_file(state_dir_path("strategy_opts", opts), "%s",
-			   opts->strategy_opts);
 	if (opts->allow_rerere_autoupdate > 0)
 		write_file(state_dir_path("allow_rerere_autoupdate", opts),
 			   "-%s-rerere-autoupdate",
diff --git a/sequencer.c b/sequencer.c
index 3be23d7ca2..c35a67e104 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2944,7 +2944,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 	strbuf_reset(buf);
 	if (!read_oneliner(buf, rebase_path_strategy(), 0))
 		return;
-	free(opts->strategy);
 	opts->strategy = strbuf_detach(buf, NULL);
 	if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
 		return;
-- 
2.40.0.672.gbd2d2ac924

[PATCH v4 2/5] sequencer: use struct strvec to store merge strategy options

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The sequencer stores the merge strategy options in an array of strings
which allocated with ALLOC_GROW(). Using "struct strvec" avoids manually
managing the memory of that array and simplifies the code.

Aside from memory allocation the changes to the sequencer are largely
mechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A
new option parsing macro OPT_STRVEC() is also added to collect the
strategy options.  Hopefully this can be used to simplify the code in
builtin/merge.c in the future.

Note that there is a change of behavior to "git cherry-pick" and "git
revert" as passing “--no-strategy-option” will now clear any previous
strategy options whereas before this change it did nothing.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/revert.c   | 20 +++----------------
 parse-options-cb.c | 16 ++++++++++++++++
 parse-options.h    | 10 ++++++++++
 sequencer.c        | 48 ++++++++++++++++++++--------------------------
 sequencer.h        | 11 ++++++++---
 5 files changed, 58 insertions(+), 47 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 62986a7b1b..362857da65 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
 }
 
-static int option_parse_x(const struct option *opt,
-			  const char *arg, int unset)
-{
-	struct replay_opts **opts_ptr = opt->value;
-	struct replay_opts *opts = *opts_ptr;
-
-	if (unset)
-		return 0;
-
-	ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-	opts->xopts[opts->xopts_nr++] = xstrdup(arg);
-	return 0;
-}
-
 static int option_parse_m(const struct option *opt,
 			  const char *arg, int unset)
 {
@@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 			     N_("select mainline parent"), option_parse_m),
 		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
 		OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")),
-		OPT_CALLBACK('X', "strategy-option", &opts, N_("option"),
-			N_("option for merge strategy"), option_parse_x),
+		OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"),
+			N_("option for merge strategy")),
 		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		OPT_END()
@@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 				"--signoff", opts->signoff,
 				"--mainline", opts->mainline,
 				"--strategy", opts->strategy ? 1 : 0,
-				"--strategy-option", opts->xopts ? 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,
diff --git a/parse-options-cb.c b/parse-options-cb.c
index d346dbe210..8eb96c5ca9 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_strvec(const struct option *opt, const char *arg, int unset)
+{
+	struct strvec *v = opt->value;
+
+	if (unset) {
+		strvec_clear(v);
+		return 0;
+	}
+
+	if (!arg)
+		return -1;
+
+	strvec_push(v, arg);
+	return 0;
+}
+
 int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
 {
 	return 0;
diff --git a/parse-options.h b/parse-options.h
index 26f19384e5..2d1d4d052f 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -285,6 +285,15 @@ struct option {
 	.help = (h), \
 	.callback = &parse_opt_string_list, \
 }
+#define OPT_STRVEC(s, l, v, a, h) { \
+	.type = OPTION_CALLBACK, \
+	.short_name = (s), \
+	.long_name = (l), \
+	.value = (v), \
+	.argh = (a), \
+	.help = (h), \
+	.callback = &parse_opt_strvec, \
+}
 #define OPT_UYN(s, l, v, h) { \
 	.type = OPTION_CALLBACK, \
 	.short_name = (s), \
@@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int);
 int parse_opt_commit(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
+int parse_opt_strvec(const struct option *, const char *, int);
 int parse_opt_noop_cb(const struct option *, const char *, int);
 enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
 					   const struct option *,
diff --git a/sequencer.c b/sequencer.c
index c35a67e104..50d469812a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts)
 	free(opts->reflog_action);
 	free(opts->default_strategy);
 	free(opts->strategy);
-	for (size_t i = 0; i < opts->xopts_nr; i++)
-		free(opts->xopts[i]);
-	free(opts->xopts);
+	strvec_clear (&opts->xopts);
 	strbuf_release(&opts->current_fixups);
 	if (opts->revs)
 		release_revisions(opts->revs);
@@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r,
 	next_tree = next ? get_commit_tree(next) : empty_tree(r);
 	base_tree = base ? get_commit_tree(base) : empty_tree(r);
 
-	for (i = 0; i < opts->xopts_nr; i++)
-		parse_merge_opt(&o, opts->xopts[i]);
+	for (i = 0; i < opts->xopts.nr; i++)
+		parse_merge_opt(&o, opts->xopts.v[i]);
 
 	if (!opts->strategy || !strcmp(opts->strategy, "ort")) {
 		memset(&result, 0, sizeof(result));
@@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r,
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
 		res |= try_merge_command(r, opts->strategy,
-					 opts->xopts_nr, (const char **)opts->xopts,
+					 opts->xopts.nr, opts->xopts.v,
 					common, oid_to_hex(&head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
@@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 	else if (!strcmp(key, "options.gpg-sign"))
 		git_config_string_dup(&opts->gpg_sign, key, value);
 	else if (!strcmp(key, "options.strategy-option")) {
-		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-		opts->xopts[opts->xopts_nr++] = xstrdup(value);
+		strvec_push(&opts->xopts, value);
 	} else if (!strcmp(key, "options.allow-rerere-auto"))
 		opts->allow_rerere_auto =
 			git_config_bool_or_int(key, value, &error_flag) ?
@@ -2920,23 +2917,23 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 {
 	int i;
 	int count;
+	const char **argv;
 	char *strategy_opts_string = raw_opts;
 
 	if (*strategy_opts_string == ' ')
 		strategy_opts_string++;
 
-	count = split_cmdline(strategy_opts_string,
-			      (const char ***)&opts->xopts);
+	count = split_cmdline(strategy_opts_string, &argv);
 	if (count < 0)
 		die(_("could not split '%s': %s"), strategy_opts_string,
 			    split_cmdline_strerror(count));
-	opts->xopts_nr = count;
-	for (i = 0; i < opts->xopts_nr; i++) {
-		const char *arg = opts->xopts[i];
+	for (i = 0; i < count; i++) {
+		const char *arg = argv[i];
 
 		skip_prefix(arg, "--", &arg);
-		opts->xopts[i] = xstrdup(arg);
+		strvec_push(&opts->xopts, arg);
 	}
+	free(argv);
 }
 
 static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
@@ -3055,8 +3052,8 @@ static void write_strategy_opts(struct replay_opts *opts)
 	int i;
 	struct strbuf buf = STRBUF_INIT;
 
-	for (i = 0; i < opts->xopts_nr; ++i)
-		strbuf_addf(&buf, " --%s", opts->xopts[i]);
+	for (i = 0; i < opts->xopts.nr; ++i)
+		strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
 
 	write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
 	strbuf_release(&buf);
@@ -3080,7 +3077,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_verbose(), "%s", "");
 	if (opts->strategy)
 		write_file(rebase_path_strategy(), "%s\n", opts->strategy);
-	if (opts->xopts_nr > 0)
+	if (opts->xopts.nr > 0)
 		write_strategy_opts(opts);
 
 	if (opts->allow_rerere_auto == RERERE_AUTOUPDATE)
@@ -3464,13 +3461,10 @@ static int save_opts(struct replay_opts *opts)
 	if (opts->gpg_sign)
 		res |= git_config_set_in_file_gently(opts_file,
 					"options.gpg-sign", opts->gpg_sign);
-	if (opts->xopts) {
-		int i;
-		for (i = 0; i < opts->xopts_nr; i++)
-			res |= git_config_set_multivar_in_file_gently(opts_file,
-					"options.strategy-option",
-					opts->xopts[i], "^$", 0);
-	}
+	for (size_t i = 0; i < opts->xopts.nr; i++)
+		res |= git_config_set_multivar_in_file_gently(opts_file,
+				"options.strategy-option",
+				opts->xopts.v[i], "^$", 0);
 	if (opts->allow_rerere_auto)
 		res |= git_config_set_in_file_gently(opts_file,
 				"options.allow-rerere-auto",
@@ -3880,7 +3874,7 @@ static int do_merge(struct repository *r,
 	struct commit *head_commit, *merge_commit, *i;
 	struct commit_list *bases, *j;
 	struct commit_list *to_merge = NULL, **tail = &to_merge;
-	const char *strategy = !opts->xopts_nr &&
+	const char *strategy = !opts->xopts.nr &&
 		(!opts->strategy ||
 		 !strcmp(opts->strategy, "recursive") ||
 		 !strcmp(opts->strategy, "ort")) ?
@@ -4063,9 +4057,9 @@ static int do_merge(struct repository *r,
 			strvec_push(&cmd.args, "octopus");
 		else {
 			strvec_push(&cmd.args, strategy);
-			for (k = 0; k < opts->xopts_nr; k++)
+			for (k = 0; k < opts->xopts.nr; k++)
 				strvec_pushf(&cmd.args,
-					     "-X%s", opts->xopts[k]);
+					     "-X%s", opts->xopts.v[k]);
 		}
 		if (!(flags & TODO_EDIT_MERGE_MSG))
 			strvec_push(&cmd.args, "--no-edit");
diff --git a/sequencer.h b/sequencer.h
index 33dbaf5b66..8a79d6b200 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -2,6 +2,7 @@
 #define SEQUENCER_H
 
 #include "strbuf.h"
+#include "strvec.h"
 #include "wt-status.h"
 
 struct commit;
@@ -60,8 +61,7 @@ struct replay_opts {
 	/* Merge strategy */
 	char *default_strategy;  /* from config options */
 	char *strategy;
-	char **xopts;
-	size_t xopts_nr, xopts_alloc;
+	struct strvec xopts;
 
 	/* Reflog */
 	char *reflog_action;
@@ -80,7 +80,12 @@ struct replay_opts {
 	/* Private use */
 	const char *reflog_message;
 };
-#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }
+#define REPLAY_OPTS_INIT {			\
+	.edit = -1,				\
+	.action = -1,				\
+	.current_fixups = STRBUF_INIT,		\
+	.xopts = STRVEC_INIT,			\
+}
 
 /*
  * Note that ordering matters in this enum. Not only must it match the mapping
-- 
2.40.0.672.gbd2d2ac924

[PATCH v4 3/5] rebase -m: cleanup --strategy-option handling

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c               | 30 ++++++++++--------------------
 sequencer.c                    |  2 +-
 sequencer.h                    |  1 -
 t/t3436-rebase-more-options.sh | 10 ++++++++--
 4 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 3bd215c771..511922c6fc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,7 +117,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;
@@ -143,6 +144,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)
@@ -176,8 +178,8 @@ 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);
+	for (size_t i = 0; i < opts->strategy_opts.nr; i++)
+		strvec_push(&replay.xopts, opts->strategy_opts.items[i].string);
 
 	if (opts->squash_onto) {
 		oidcpy(&replay.squash_onto, opts->squash_onto);
@@ -1013,7 +1015,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;
@@ -1122,7 +1123,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")),
@@ -1436,23 +1437,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);
@@ -1827,10 +1818,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 50d469812a..587a473d6e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2913,7 +2913,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 8a79d6b200..913a0f652d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -252,7 +252,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
 '
-- 
2.40.0.672.gbd2d2ac924

[PATCH v4 4/5] rebase -m: fix serialization of strategy options

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

To store the strategy options rebase prepends " --" to each one and
writes them to a file. To load them it reads the file and passes the
contents to split_cmdline(). This roughly mimics the behavior of the
scripted rebase but has a couple of limitations, (1) options containing
whitespace are not properly preserved (this is true of the scripted
rebase as well) and (2) options containing '"' or '\' are incorrectly
parsed and may cause the parser to return an error.

Fix these limitations by quoting each option when they are stored so
that they can be parsed correctly. Now that "--preserve-merges" no
longer exist this change also stops prepending "--" to the options when
they are stored as that was an artifact of the scripted rebase.

These changes are backwards compatible so the files written by an older
version of git can still be read. They are also forwards compatible,
the file can still be parsed by recent versions of git as they treat the
"--" prefix as optional.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 alias.c                        | 18 +++++++++++++++++
 alias.h                        |  3 +++
 sequencer.c                    | 11 ++++++-----
 t/t3418-rebase-continue.sh     | 36 ++++++++++++++++++++++------------
 t/t3436-rebase-more-options.sh | 24 -----------------------
 5 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/alias.c b/alias.c
index e814948ced..54a1a23d2c 100644
--- a/alias.c
+++ b/alias.c
@@ -3,6 +3,7 @@
 #include "alloc.h"
 #include "config.h"
 #include "gettext.h"
+#include "strbuf.h"
 #include "string-list.h"
 
 struct config_alias_data {
@@ -46,6 +47,23 @@ void list_aliases(struct string_list *list)
 	read_early_config(config_alias_cb, &data);
 }
 
+void quote_cmdline(struct strbuf *buf, const char **argv)
+{
+	for (const char **argp = argv; *argp; argp++) {
+		if (argp != argv)
+			strbuf_addch(buf, ' ');
+		strbuf_addch(buf, '"');
+		for (const char *p = *argp; *p; p++) {
+			const char c = *p;
+
+			if (c == '"' || c =='\\')
+				strbuf_addch(buf, '\\');
+			strbuf_addch(buf, c);
+		}
+		strbuf_addch(buf, '"');
+	}
+}
+
 #define SPLIT_CMDLINE_BAD_ENDING 1
 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
 #define SPLIT_CMDLINE_ARGC_OVERFLOW 3
diff --git a/alias.h b/alias.h
index aef4843bb7..43db736484 100644
--- a/alias.h
+++ b/alias.h
@@ -1,9 +1,12 @@
 #ifndef ALIAS_H
 #define ALIAS_H
 
+struct strbuf;
 struct string_list;
 
 char *alias_lookup(const char *alias);
+/* Quote argv so buf can be parsed by split_cmdline() */
+void quote_cmdline(struct strbuf *buf, const char **argv);
 int split_cmdline(char *cmdline, const char ***argv);
 /* Takes a negative value returned by split_cmdline */
 const char *split_cmdline_strerror(int cmdline_errno);
diff --git a/sequencer.c b/sequencer.c
index 587a473d6e..fc6ea75895 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 
 	count = split_cmdline(strategy_opts_string, &argv);
 	if (count < 0)
-		die(_("could not split '%s': %s"), strategy_opts_string,
+		BUG("could not split '%s': %s", strategy_opts_string,
 			    split_cmdline_strerror(count));
 	for (i = 0; i < count; i++) {
 		const char *arg = argv[i];
@@ -3049,12 +3049,13 @@ static int read_populate_opts(struct replay_opts *opts)
 
 static void write_strategy_opts(struct replay_opts *opts)
 {
-	int i;
 	struct strbuf buf = STRBUF_INIT;
 
-	for (i = 0; i < opts->xopts.nr; ++i)
-		strbuf_addf(&buf, " --%s", opts->xopts.v[i]);
-
+	/*
+	 * Quote strategy options so that they can be read correctly
+	 * by split_cmdline().
+	 */
+	quote_cmdline(&buf, opts->xopts.v);
 	write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);
 	strbuf_release(&buf);
 }
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 130e2f9b55..42c3954125 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F2-on-topic-branch &&
 	test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run" &&
+	test_when_finished "rm -fr test-bin" &&
 	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
+
+	write_script test-bin/git-merge-funny <<-\EOF &&
+	printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual
+	shift 3 &&
+	exec git merge-recursive "$@"
+	EOF
+
+	cat >expect <<-\EOF &&
+	[7]
+	[--option=arg with space]
+	[--op"tion\]
+	[--new
+	line ]
+	[--]
+	EOF
+
+	rm -f actual &&
 	(
 		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -s funny -Xopt main topic
+		test_must_fail git rebase -s funny -X"option=arg with space" \
+				-Xop\"tion\\ -X"new${LF}line " main topic
 	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
+	test_cmp expect actual &&
+	rm actual &&
 	echo "Resolved" >F2 &&
 	git add F2 &&
 	(
 		PATH=./test-bin:$PATH &&
 		git rebase --continue
 	) &&
-	test -f funny.was.run
+	test_cmp expect actual
 '
 
 test_expect_success 'rebase -i --continue handles merge strategy and options' '
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 3adf42f47d..94671d3c46 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -40,30 +40,6 @@ test_expect_success 'setup' '
 	EOF
 '
 
-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
-	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
-	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
-'
-
 test_expect_success '--ignore-whitespace works with apply backend' '
 	test_must_fail git rebase --apply main side &&
 	git rebase --abort &&
-- 
2.40.0.672.gbd2d2ac924

[PATCH v4 5/5] rebase: remove a couple of redundant strategy tests

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Remove a test in t3402 that has been redundant ever since 80ff47957b
(rebase: remember strategy and strategy options, 2011-02-06).  That
commit added a new test, the first part of which (as noted in the old
commit message) duplicated an existing test.

Also remove a test t3418 that has been redundant since the merge backend
was removed in 68aa495b59 (rebase: implement --merge via the interactive
machinery, 2018-12-11), since it now tests the same code paths as the
preceding test.

Helped-by: Elijah Newren <newren@gmail.com>
Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3402-rebase-merge.sh    | 21 ---------------------
 t/t3418-rebase-continue.sh | 32 --------------------------------
 2 files changed, 53 deletions(-)

diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
index 7e46f4ca85..79b0640c00 100755
--- a/t/t3402-rebase-merge.sh
+++ b/t/t3402-rebase-merge.sh
@@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '
 	esac
 '
 
-test_expect_success 'rebase -s funny -Xopt' '
-	test_when_finished "rm -fr test-bin funny.was.run" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	shift &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	git reset --hard &&
-	git checkout -b test-funny main^ &&
-	test_commit funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase -s funny -Xopt main
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase --skip works with two conflicts in a row' '
 	git checkout second-side  &&
 	tr "[A-Z]" "[a-z]" <newfile >tmp &&
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 42c3954125..2d0789e554 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --continue handles merge strategy and options' '
-	rm -fr .git/rebase-* &&
-	git reset --hard commit-new-file-F2-on-topic-branch &&
-	test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 &&
-	test_when_finished "rm -fr test-bin funny.was.run funny.args" &&
-	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF &&
-	#!$SHELL_PATH
-	echo "\$@" >>funny.args
-	case "\$1" in --opt) ;; *) exit 2 ;; esac
-	case "\$2" in --foo) ;; *) exit 2 ;; esac
-	case "\$4" in --) ;; *) exit 2 ;; esac
-	shift 2 &&
-	>funny.was.run &&
-	exec git merge-recursive "\$@"
-	EOF
-	chmod +x test-bin/git-merge-funny &&
-	(
-		PATH=./test-bin:$PATH &&
-		test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic
-	) &&
-	test -f funny.was.run &&
-	rm funny.was.run &&
-	echo "Resolved" >F2 &&
-	git add F2 &&
-	(
-		PATH=./test-bin:$PATH &&
-		git rebase --continue
-	) &&
-	test -f funny.was.run
-'
-
 test_expect_success 'rebase -r passes merge strategy options correctly' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&
-- 
2.40.0.672.gbd2d2ac924

Re: [PATCH v4 0/5] rebase: cleanup merge strategy option handling

[Elijah Newren]

On Mon, Apr 10, 2023 at 2:08 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Cleanup the handling of --strategy-option now that we no longer need to
> support "--preserve-merges" and properly quote the argument when saving
> it to disc.
>
> A hopefully final re-roll to fix a memory leak I spotted in V3.

Doh, I clearly missed that leak too.

> Changes since V3:
>  - Fixed a memory leak added in V3. The array returned by
>    split_cmdline() is allocated on the heap so we need to fee it. The
>    array elements come from the string passed to split_cmdline() so do
>    not need to be individually freed.

And in particular, that string that stores those array elements will
be freed by the strbuf_reset() that comes immediately after the
read_strategy_opts() call (read_strategy_opts() is the sole caller of
parse_strategy_opts()), and the fact that the string is freed is fine
since your parse_strategy_opts() passes them to strvec_push() which
will xstrdup() them and then own them.

All that to say I agree with the fix.