{"thread":{"id":"59400","subject":"[PATCH 0/4] rebase: cleanup merge strategy option handling","startedAt":"2023-03-15T15:15:38Z","lastAt":"2023-04-10T16:46:22Z","messageCount":36,"participants":["Phillip Wood","Junio C Hamano","Felipe Contreras","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"473530","messageId":"cover.1678893298.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":null,"subject":"[PATCH 0/4] rebase: cleanup merge strategy option handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-15T15:14:55Z","receivedAt":"2023-03-15T15:15:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCleanup the handling of --strategy-option now that we no longer need\nto support \"--preserve-merges\" and properly quote the argument when\nsaving it to disc.\n\nThese patches are based on a merge of 'master' and\n'ab/fix-strategy-opts-parsing'\n\nPublished-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv1\nView-Changes-At: https://github.com/phillipwood/git/compare/c2e329a52...3e02eeff7\nFetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v1\n\nPhillip Wood (4):\n rebase: stop reading and writing unnecessary strategy state\n rebase -m: cleanup --strategy-option handling\n rebase -m: fix serialization of strategy options\n rebase: remove a couple of redundant strategy tests\n\n builtin/rebase.c | 60 +++++++++-----------------------\n sequencer.c | 26 ++++++++++----\n sequencer.h | 1 -\n t/t3402-rebase-merge.sh | 21 ------------\n t/t3418-rebase-continue.sh | 62 +++++++++++-----------------------\n t/t3436-rebase-more-options.sh | 18 ----------\n 6 files changed, 56 insertions(+), 132 deletions(-)\n\n-- \n2.39.2\n\n"},{"id":"473531","messageId":"029d0bf2b6249ddc3e79fe1bf3df3e05d22348ad.1678893298.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 1/4] rebase: stop reading and writing unnecessary strategy state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-15T15:14:56Z","receivedAt":"2023-03-15T15:15:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe state files for \"--strategy\" and \"--strategy-option\" are written and\nread twice, once by builtin/rebase.c and then by sequencer.c. This is an\nartifact of the scripted rebase and the need to support \"rebase\n--preserve-merges\". Now that \"--preserve-merges\" no-longer exists we\nonly need to read and write these files in sequencer.c. This enables us\nto remove a call to free() in read_strategy_opts() that was added by\nf1f4ebf432 (sequencer.c: fix \"opts->strategy\" leak in\nread_strategy_opts(), 2022-11-08) as this commit fixes the root cause of\nthat leak.\n\nThere is further scope for removing duplication in the reading and\nwriting of state files between builtin/rebase.c and sequencer.c but that\nis left for a follow up series.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 24 ------------------------\n sequencer.c | 1 -\n 2 files changed, 25 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6635f10d52..516ad1b12a 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -482,24 +482,6 @@ static int read_basic_state(struct rebase_options *opts)\n \t\topts->gpg_sign_opt = xstrdup(buf.buf);\n \t}\n \n-\tif (file_exists(state_dir_path(\"strategy\", opts))) {\n-\t\tstrbuf_reset(&buf);\n-\t\tif (!read_oneliner(&buf, state_dir_path(\"strategy\", opts),\n-\t\t\t\t READ_ONELINER_WARN_MISSING))\n-\t\t\treturn -1;\n-\t\tfree(opts->strategy);\n-\t\topts->strategy = xstrdup(buf.buf);\n-\t}\n-\n-\tif (file_exists(state_dir_path(\"strategy_opts\", opts))) {\n-\t\tstrbuf_reset(&buf);\n-\t\tif (!read_oneliner(&buf, state_dir_path(\"strategy_opts\", opts),\n-\t\t\t\t READ_ONELINER_WARN_MISSING))\n-\t\t\treturn -1;\n-\t\tfree(opts->strategy_opts);\n-\t\topts->strategy_opts = xstrdup(buf.buf);\n-\t}\n-\n \tstrbuf_release(&buf);\n \n \treturn 0;\n@@ -517,12 +499,6 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n \t\twrite_file(state_dir_path(\"quiet\", opts), \"%s\", \"\");\n \tif (opts->flags & REBASE_VERBOSE)\n \t\twrite_file(state_dir_path(\"verbose\", opts), \"%s\", \"\");\n-\tif (opts->strategy)\n-\t\twrite_file(state_dir_path(\"strategy\", opts), \"%s\",\n-\t\t\t opts->strategy);\n-\tif (opts->strategy_opts)\n-\t\twrite_file(state_dir_path(\"strategy_opts\", opts), \"%s\",\n-\t\t\t opts->strategy_opts);\n \tif (opts->allow_rerere_autoupdate > 0)\n \t\twrite_file(state_dir_path(\"allow_rerere_autoupdate\", opts),\n \t\t\t \"-%s-rerere-autoupdate\",\ndiff --git a/sequencer.c b/sequencer.c\nindex 886fbc3616..55b3ba3a51 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2946,7 +2946,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)\n \tstrbuf_reset(buf);\n \tif (!read_oneliner(buf, rebase_path_strategy(), 0))\n \t\treturn;\n-\tfree(opts->strategy);\n \topts->strategy = strbuf_detach(buf, NULL);\n \tif (!read_oneliner(buf, rebase_path_strategy_opts(), 0))\n \t\treturn;\n-- \n2.39.2\n\n"},{"id":"473532","messageId":"a01b182644f4ad3b4cf30cf1895dc9ea1e9fa197.1678893298.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 2/4] rebase -m: cleanup --strategy-option handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-15T15:14:57Z","receivedAt":"2023-03-15T15:15:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen handling \"--strategy-option\" rebase collects the commands into a\nstruct string_list, then concatenates them into a string, prepending \"--\"\nto each one before splitting the string and removing the \"--\" prefix.\nThis is an artifact of the scripted rebase and the need to support\n\"rebase --preserve-merges\". Now that \"--preserve-merges\" no-longer\nexists we can cleanup the way the argument is handled.\n\nThe tests for a bad strategy option are adjusted now that\nparse_strategy_opts() is no-longer called when starting a rebase. The\nfact that it only errors out when running \"git rebase --continue\" is a\nmixed blessing but the next commit will fix the root cause of the\nparsing problem so lets not worry about that here.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 36 +++++++++++++++-------------------\n sequencer.c | 2 +-\n sequencer.h | 1 -\n t/t3436-rebase-more-options.sh | 10 ++++++++--\n 4 files changed, 25 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 516ad1b12a..5194d64c24 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -116,7 +116,8 @@ struct rebase_options {\n \tstruct string_list exec;\n \tint allow_empty_message;\n \tint rebase_merges, rebase_cousins;\n-\tchar *strategy, *strategy_opts;\n+\tchar *strategy;\n+\tstruct string_list strategy_opts;\n \tstruct strbuf git_format_patch_opt;\n \tint reschedule_failed_exec;\n \tint reapply_cherry_picks;\n@@ -142,6 +143,7 @@ struct rebase_options {\n \t\t.config_autosquash = -1, \\\n \t\t.update_refs = -1, \\\n \t\t.config_update_refs = -1, \\\n+\t\t.strategy_opts = STRING_LIST_INIT_NODUP\t\\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -175,8 +177,14 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\treplay.default_strategy = NULL;\n \t}\n \n-\tif (opts->strategy_opts)\n-\t\tparse_strategy_opts(&replay, opts->strategy_opts);\n+\tif (opts->strategy_opts.nr) {\n+\t\tALLOC_ARRAY(replay.xopts, opts->strategy_opts.nr);\n+\t\treplay.xopts_nr = opts->strategy_opts.nr;\n+\t\treplay.xopts_alloc = opts->strategy_opts.nr;\n+\t\tfor (size_t i = 0; i < opts->strategy_opts.nr; i++)\n+\t\t\treplay.xopts[i] =\n+\t\t\t\txstrdup(opts->strategy_opts.items[i].string);\n+\t}\n \n \tif (opts->squash_onto) {\n \t\toidcpy(&replay.squash_onto, opts->squash_onto);\n@@ -1012,7 +1020,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tint ignore_whitespace = 0;\n \tconst char *gpg_sign = NULL;\n \tconst char *rebase_merges = NULL;\n-\tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n \tchar *squash_onto_name = NULL;\n \tchar *keep_base_onto_name = NULL;\n@@ -1121,7 +1128,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"use 'merge-base --fork-point' to refine upstream\")),\n \t\tOPT_STRING('s', \"strategy\", &options.strategy,\n \t\t\t N_(\"strategy\"), N_(\"use the given merge strategy\")),\n-\t\tOPT_STRING_LIST('X', \"strategy-option\", &strategy_options,\n+\t\tOPT_STRING_LIST('X', \"strategy-option\", &options.strategy_opts,\n \t\t\t\tN_(\"option\"),\n \t\t\t\tN_(\"pass the argument through to the merge \"\n \t\t\t\t \"strategy\")),\n@@ -1435,23 +1442,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t} else {\n \t\t/* REBASE_MERGE */\n \t\tif (ignore_whitespace) {\n-\t\t\tstring_list_append(&strategy_options,\n+\t\t\tstring_list_append(&options.strategy_opts,\n \t\t\t\t\t \"ignore-space-change\");\n \t\t}\n \t}\n \n-\tif (strategy_options.nr) {\n-\t\tint i;\n-\n-\t\tif (!options.strategy)\n-\t\t\toptions.strategy = \"ort\";\n-\n-\t\tstrbuf_reset(&buf);\n-\t\tfor (i = 0; i < strategy_options.nr; i++)\n-\t\t\tstrbuf_addf(&buf, \" --%s\",\n-\t\t\t\t strategy_options.items[i].string);\n-\t\toptions.strategy_opts = xstrdup(buf.buf);\n-\t}\n+\tif (options.strategy_opts.nr && !options.strategy)\n+\t\toptions.strategy = \"ort\";\n \n \tif (options.strategy) {\n \t\toptions.strategy = xstrdup(options.strategy);\n@@ -1826,10 +1823,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tfree(options.gpg_sign_opt);\n \tstring_list_clear(&options.exec, 0);\n \tfree(options.strategy);\n-\tfree(options.strategy_opts);\n+\tstring_list_clear(&options.strategy_opts, 0);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n \tfree(keep_base_onto_name);\n-\tstring_list_clear(&strategy_options, 0);\n \treturn !!ret;\n }\ndiff --git a/sequencer.c b/sequencer.c\nindex 55b3ba3a51..83ea1016ae 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2918,7 +2918,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \treturn 0;\n }\n \n-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n+static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n {\n \tint i;\n \tint count;\ndiff --git a/sequencer.h b/sequencer.h\nindex 3bcdfa1b58..5de5cfa664 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -247,7 +247,6 @@ int read_oneliner(struct strbuf *buf,\n \tconst char *path, unsigned flags);\n int read_author_script(const char *path, char **name, char **email, char **date,\n \t\t int allow_missing);\n-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\t struct commit *onto, const struct object_id *orig_head);\n void sequencer_post_commit_cleanup(struct repository *r, int verbose);\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex c3184c9ade..3adf42f47d 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -41,19 +41,25 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'bad -X arguments: unclosed quote' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tcat >expect <<-\\EOF &&\n \tfatal: could not split '\\''--bad'\\'': unclosed quote\n \tEOF\n-\ttest_expect_code 128 git rebase -X\"bad argument\\\"\" side main >out 2>actual &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n+\t\tgit rebase -i -X\"bad argument\\\"\" side main &&\n+\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n \ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'bad -X arguments: bad escape' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tcat >expect <<-\\EOF &&\n \tfatal: could not split '\\''--bad'\\'': cmdline ends with \\\n \tEOF\n-\ttest_expect_code 128 git rebase -X\"bad escape \\\\\" side main >out 2>actual &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n+\t\tgit rebase -i -X\"bad escape \\\\\" side main &&\n+\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n \ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n-- \n2.39.2\n\n"},{"id":"473533","messageId":"9af68cb065871d9b89e99ef6b48870d322bb5faa.1678893298.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 3/4] rebase -m: fix serialization of strategy options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-15T15:14:58Z","receivedAt":"2023-03-15T15:15:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nTo store the strategy options rebase prepends \" --\" to each one and\nwrites them to a file. To load them it reads the file and passes the\ncontents to split_cmdline(). This roughly mimics the behavior of the\nscripted rebase but has a couple of limitations, (1) options containing\nwhitespace are not properly preserved (this is true of the scripted\nrebase as well) and (2) options containing '\"' or '\\' are incorrectly\nparsed and may cause the parser to return an error.\n\nFix these limitations by quoting each option when they are stored so\nthat they can be parsed correctly. Now that \"--preserve-merges\" no\nlonger exist this change also stops prepending \"--\" to the options when\nthey are stored as that was an artifact of the scripted rebase.\n\nThese changes are backwards compatible so the files written by an older\nversion of git can be still be read. They are also forwards compatible,\nthe file can still be parsed by recent versions of git as they treat the\n\"--\" prefix as optional.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 23 +++++++++++++++++++----\n t/t3418-rebase-continue.sh | 34 ++++++++++++++++++++++------------\n t/t3436-rebase-more-options.sh | 24 ------------------------\n 3 files changed, 41 insertions(+), 40 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 83ea1016ae..8890d1f7a1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2930,7 +2930,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n \tcount = split_cmdline(strategy_opts_string,\n \t\t\t (const char ***)&opts->xopts);\n \tif (count < 0)\n-\t\tdie(_(\"could not split '%s': %s\"), strategy_opts_string,\n+\t\tBUG(\"could not split '%s': %s\", strategy_opts_string,\n \t\t\t split_cmdline_strerror(count));\n \topts->xopts_nr = count;\n \tfor (i = 0; i < opts->xopts_nr; i++) {\n@@ -3054,12 +3054,27 @@ static int read_populate_opts(struct replay_opts *opts)\n \n static void write_strategy_opts(struct replay_opts *opts)\n {\n-\tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfor (i = 0; i < opts->xopts_nr; ++i)\n-\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts[i]);\n+\t/*\n+\t * Quote strategy options so that they can be read correctly\n+\t * by split_cmdline().\n+\t */\n+\tfor (size_t i = 0; i < opts->xopts_nr; i++) {\n+\t\tchar *arg = opts->xopts[i];\n \n+\t\tif (i)\n+\t\t\tstrbuf_addch(&buf, ' ');\n+\t\tstrbuf_addch(&buf, '\"');\n+\t\tfor (size_t j = 0; arg[j]; j++) {\n+\t\t\tconst char c = arg[j];\n+\n+\t\t\tif (c == '\"' || c =='\\\\')\n+\t\t\t\tstrbuf_addch(&buf, '\\\\');\n+\t\t\tstrbuf_addch(&buf, c);\n+\t\t}\n+\t\tstrbuf_addch(&buf, '\"');\n+\t}\n \twrite_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n }\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 130e2f9b55..42c3954125 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard commit-new-file-F2-on-topic-branch &&\n \ttest_commit \"commit-new-file-F3-on-topic-branch\" F3 32 &&\n-\ttest_when_finished \"rm -fr test-bin funny.was.run\" &&\n+\ttest_when_finished \"rm -fr test-bin\" &&\n \tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tshift &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n+\n+\twrite_script test-bin/git-merge-funny <<-\\EOF &&\n+\tprintf \"[%s]\\n\" $# \"$1\" \"$2\" \"$3\" \"$5\" >actual\n+\tshift 3 &&\n+\texec git merge-recursive \"$@\"\n \tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t[7]\n+\t[--option=arg with space]\n+\t[--op\"tion\\]\n+\t[--new\n+\tline ]\n+\t[--]\n+\tEOF\n+\n+\trm -f actual &&\n \t(\n \t\tPATH=./test-bin:$PATH &&\n-\t\ttest_must_fail git rebase -s funny -Xopt main topic\n+\t\ttest_must_fail git rebase -s funny -X\"option=arg with space\" \\\n+\t\t\t\t-Xop\\\"tion\\\\ -X\"new${LF}line \" main topic\n \t) &&\n-\ttest -f funny.was.run &&\n-\trm funny.was.run &&\n+\ttest_cmp expect actual &&\n+\trm actual &&\n \techo \"Resolved\" >F2 &&\n \tgit add F2 &&\n \t(\n \t\tPATH=./test-bin:$PATH &&\n \t\tgit rebase --continue\n \t) &&\n-\ttest -f funny.was.run\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'rebase -i --continue handles merge strategy and options' '\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 3adf42f47d..94671d3c46 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -40,30 +40,6 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n-test_expect_success 'bad -X arguments: unclosed quote' '\n-\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n-\tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': unclosed quote\n-\tEOF\n-\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n-\t\tgit rebase -i -X\"bad argument\\\"\" side main &&\n-\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'bad -X arguments: bad escape' '\n-\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n-\tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': cmdline ends with \\\n-\tEOF\n-\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n-\t\tgit rebase -i -X\"bad escape \\\\\" side main &&\n-\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect actual\n-'\n-\n test_expect_success '--ignore-whitespace works with apply backend' '\n \ttest_must_fail git rebase --apply main side &&\n \tgit rebase --abort &&\n-- \n2.39.2\n\n"},{"id":"473534","messageId":"3e02eeff78b23711187de47a1a820f9bde683200.1678893298.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 4/4] rebase: remove a couple of redundant strategy tests","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-15T15:14:59Z","receivedAt":"2023-03-15T15:15:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe test removed in t3402 has been redundant ever since 80ff47957b\n(rebase: remember strategy and strategy options, 2011-02-06) which added\na new test the first part of which (as noted in the commit message)\nduplicated the existing test. The test removed in t3418 has been\nredundant since the merge backend was removed in 68aa495b59 (rebase:\nimplement --merge via the interactive machinery, 2018-12-11) as it now\ntests the same code paths as the preceding test.\n\nSigned-off-by: Phillip Wood \n---\n t/t3402-rebase-merge.sh | 21 ---------------------\n t/t3418-rebase-continue.sh | 32 --------------------------------\n 2 files changed, 53 deletions(-)\n\ndiff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\nindex 7e46f4ca85..79b0640c00 100755\n--- a/t/t3402-rebase-merge.sh\n+++ b/t/t3402-rebase-merge.sh\n@@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '\n \tesac\n '\n \n-test_expect_success 'rebase -s funny -Xopt' '\n-\ttest_when_finished \"rm -fr test-bin funny.was.run\" &&\n-\tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tshift &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n-\tgit reset --hard &&\n-\tgit checkout -b test-funny main^ &&\n-\ttest_commit funny &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\tgit rebase -s funny -Xopt main\n-\t) &&\n-\ttest -f funny.was.run\n-'\n-\n test_expect_success 'rebase --skip works with two conflicts in a row' '\n \tgit checkout second-side &&\n \ttr \"[A-Z]\" \"[a-z]\" tmp &&\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 42c3954125..2d0789e554 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'rebase -i --continue handles merge strategy and options' '\n-\trm -fr .git/rebase-* &&\n-\tgit reset --hard commit-new-file-F2-on-topic-branch &&\n-\ttest_commit \"commit-new-file-F3-on-topic-branch-for-dash-i\" F3 32 &&\n-\ttest_when_finished \"rm -fr test-bin funny.was.run funny.args\" &&\n-\tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\techo \"\\$@\" >>funny.args\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tcase \"\\$2\" in --foo) ;; *) exit 2 ;; esac\n-\tcase \"\\$4\" in --) ;; *) exit 2 ;; esac\n-\tshift 2 &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\ttest_must_fail git rebase -i -s funny -Xopt -Xfoo main topic\n-\t) &&\n-\ttest -f funny.was.run &&\n-\trm funny.was.run &&\n-\techo \"Resolved\" >F2 &&\n-\tgit add F2 &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\tgit rebase --continue\n-\t) &&\n-\ttest -f funny.was.run\n-'\n-\n test_expect_success 'rebase -r passes merge strategy options correctly' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard commit-new-file-F3-on-topic-branch &&\n-- \n2.39.2\n\n"},{"id":"473537","messageId":"xmqq8rfyovbs.fsf@gitster.g","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 0/4] rebase: cleanup merge strategy option handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-15T16:03:51Z","receivedAt":"2023-03-15T16:05:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> From: Phillip Wood \n>\n> Cleanup the handling of --strategy-option now that we no longer need\n> to support \"--preserve-merges\" and properly quote the argument when\n> saving it to disc.\n\nNice.\n\n>\n> These patches are based on a merge of 'master' and\n> 'ab/fix-strategy-opts-parsing'\n>\n> Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv1\n> View-Changes-At: https://github.com/phillipwood/git/compare/c2e329a52...3e02eeff7\n> Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v1\n>\n> Phillip Wood (4):\n> rebase: stop reading and writing unnecessary strategy state\n> rebase -m: cleanup --strategy-option handling\n> rebase -m: fix serialization of strategy options\n> rebase: remove a couple of redundant strategy tests\n>\n> builtin/rebase.c | 60 +++++++++-----------------------\n> sequencer.c | 26 ++++++++++----\n> sequencer.h | 1 -\n> t/t3402-rebase-merge.sh | 21 ------------\n> t/t3418-rebase-continue.sh | 62 +++++++++++-----------------------\n> t/t3436-rebase-more-options.sh | 18 ----------\n> 6 files changed, 56 insertions(+), 132 deletions(-)\n"},{"id":"473544","messageId":"CAMP44s1y9mp7-dyLtrKsvgzenG9buVM-V6OBnaTKD0cv+aA7zg@mail.gmail.com","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 0/4] rebase: cleanup merge strategy option handling","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-03-15T16:50:42Z","receivedAt":"2023-03-15T16:51:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Mar 15, 2023 at 9:39 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> Cleanup the handling of --strategy-option now that we no longer need\n> to support \"--preserve-merges\" and properly quote the argument when\n> saving it to disc.\n>\n> These patches are based on a merge of 'master' and\n> 'ab/fix-strategy-opts-parsing'\n>\n> Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv1\n> View-Changes-At: https://github.com/phillipwood/git/compare/c2e329a52...3e02eeff7\n> Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v1\n\nFWIW I reviewed the changes and they all look good to me.\n\n-- \nFelipe Contreras\n"},{"id":"474637","messageId":"CABPp-BF6_gmDCTo5j2fR5_KQfJYVuYZuYv6TdhsMvPYshXzu+g@mail.gmail.com","threadId":"59400","inReplyTo":"9af68cb065871d9b89e99ef6b48870d322bb5faa.1678893298.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 3/4] rebase -m: fix serialization of strategy options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-01T19:27:42Z","receivedAt":"2023-04-01T19:28:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 15, 2023 at 8:51 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> To store the strategy options rebase prepends \" --\" to each one and\n> writes them to a file. To load them it reads the file and passes the\n> contents to split_cmdline(). This roughly mimics the behavior of the\n> scripted rebase but has a couple of limitations, (1) options containing\n> whitespace are not properly preserved (this is true of the scripted\n> rebase as well) and (2) options containing '\"' or '\\' are incorrectly\n> parsed and may cause the parser to return an error.\n>\n> Fix these limitations by quoting each option when they are stored so\n> that they can be parsed correctly. Now that \"--preserve-merges\" no\n> longer exist this change also stops prepending \"--\" to the options when\n> they are stored as that was an artifact of the scripted rebase.\n>\n> These changes are backwards compatible so the files written by an older\n> version of git can be still be read. They are also forwards compatible,\n> the file can still be parsed by recent versions of git as they treat the\n> \"--\" prefix as optional.\n\nI'm not sure we want to tie ourselves to support completing a rebase\nwith git version X+1 that was started with git version X. But, it's\nreally nice that you're paying attention to that level of detail. :-)\n\n>\n> Signed-off-by: Phillip Wood \n> ---\n> sequencer.c | 23 +++++++++++++++++++----\n> t/t3418-rebase-continue.sh | 34 ++++++++++++++++++++++------------\n> t/t3436-rebase-more-options.sh | 24 ------------------------\n> 3 files changed, 41 insertions(+), 40 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 83ea1016ae..8890d1f7a1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2930,7 +2930,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n> count = split_cmdline(strategy_opts_string,\n> (const char ***)&opts->xopts);\n> if (count < 0)\n> - die(_(\"could not split '%s': %s\"), strategy_opts_string,\n> + BUG(\"could not split '%s': %s\", strategy_opts_string,\n> split_cmdline_strerror(count));\n> opts->xopts_nr = count;\n> for (i = 0; i < opts->xopts_nr; i++) {\n> @@ -3054,12 +3054,27 @@ static int read_populate_opts(struct replay_opts *opts)\n>\n> static void write_strategy_opts(struct replay_opts *opts)\n> {\n> - int i;\n> struct strbuf buf = STRBUF_INIT;\n>\n> - for (i = 0; i < opts->xopts_nr; ++i)\n> - strbuf_addf(&buf, \" --%s\", opts->xopts[i]);\n> + /*\n> + * Quote strategy options so that they can be read correctly\n> + * by split_cmdline().\n> + */\n> + for (size_t i = 0; i < opts->xopts_nr; i++) {\n> + char *arg = opts->xopts[i];\n>\n> + if (i)\n> + strbuf_addch(&buf, ' ');\n> + strbuf_addch(&buf, '\"');\n> + for (size_t j = 0; arg[j]; j++) {\n> + const char c = arg[j];\n> +\n> + if (c == '\"' || c =='\\\\')\n> + strbuf_addch(&buf, '\\\\');\n> + strbuf_addch(&buf, c);\n> + }\n> + strbuf_addch(&buf, '\"');\n> + }\n\nDo we not have a function in quote.[ch] that can handle this? If not,\nshould we add this code to a function in that file and call it?\n\n> write_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n> strbuf_release(&buf);\n> }\n> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n> index 130e2f9b55..42c3954125 100755\n> --- a/t/t3418-rebase-continue.sh\n> +++ b/t/t3418-rebase-continue.sh\n> @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n> rm -fr .git/rebase-* &&\n> git reset --hard commit-new-file-F2-on-topic-branch &&\n> test_commit \"commit-new-file-F3-on-topic-branch\" F3 32 &&\n> - test_when_finished \"rm -fr test-bin funny.was.run\" &&\n> + test_when_finished \"rm -fr test-bin\" &&\n> mkdir test-bin &&\n> - cat >test-bin/git-merge-funny <<-EOF &&\n> - #!$SHELL_PATH\n> - case \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n> - shift &&\n> - >funny.was.run &&\n> - exec git merge-recursive \"\\$@\"\n> +\n> + write_script test-bin/git-merge-funny <<-\\EOF &&\n> + printf \"[%s]\\n\" $# \"$1\" \"$2\" \"$3\" \"$5\" >actual\n> + shift 3 &&\n> + exec git merge-recursive \"$@\"\n> EOF\n> - chmod +x test-bin/git-merge-funny &&\n> +\n> + cat >expect <<-\\EOF &&\n> + [7]\n> + [--option=arg with space]\n> + [--op\"tion\\]\n> + [--new\n> + line ]\n> + [--]\n> + EOF\n> +\n> + rm -f actual &&\n> (\n> PATH=./test-bin:$PATH &&\n> - test_must_fail git rebase -s funny -Xopt main topic\n> + test_must_fail git rebase -s funny -X\"option=arg with space\" \\\n> + -Xop\\\"tion\\\\ -X\"new${LF}line \" main topic\n> ) &&\n> - test -f funny.was.run &&\n> - rm funny.was.run &&\n> + test_cmp expect actual &&\n> + rm actual &&\n> echo \"Resolved\" >F2 &&\n> git add F2 &&\n> (\n> PATH=./test-bin:$PATH &&\n> git rebase --continue\n> ) &&\n> - test -f funny.was.run\n> + test_cmp expect actual\n> '\n\nI appreciate the more stringent test to cover these new special cases. :-)\n\n>\n> test_expect_success 'rebase -i --continue handles merge strategy and options' '\n> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\n> index 3adf42f47d..94671d3c46 100755\n> --- a/t/t3436-rebase-more-options.sh\n> +++ b/t/t3436-rebase-more-options.sh\n> @@ -40,30 +40,6 @@ test_expect_success 'setup' '\n> EOF\n> '\n>\n> -test_expect_success 'bad -X arguments: unclosed quote' '\n> - test_when_finished \"test_might_fail git rebase --abort\" &&\n> - cat >expect <<-\\EOF &&\n> - fatal: could not split '\\''--bad'\\'': unclosed quote\n> - EOF\n> - GIT_SEQUENCE_EDITOR=\"echo break >\" \\\n> - git rebase -i -X\"bad argument\\\"\" side main &&\n> - test_expect_code 128 git rebase --continue >out 2>actual &&\n> - test_must_be_empty out &&\n> - test_cmp expect actual\n> -'\n> -\n> -test_expect_success 'bad -X arguments: bad escape' '\n> - test_when_finished \"test_might_fail git rebase --abort\" &&\n> - cat >expect <<-\\EOF &&\n> - fatal: could not split '\\''--bad'\\'': cmdline ends with \\\n> - EOF\n> - GIT_SEQUENCE_EDITOR=\"echo break >\" \\\n> - git rebase -i -X\"bad escape \\\\\" side main &&\n> - test_expect_code 128 git rebase --continue >out 2>actual &&\n> - test_must_be_empty out &&\n> - test_cmp expect actual\n> -'\n> -\n> test_expect_success '--ignore-whitespace works with apply backend' '\n> test_must_fail git rebase --apply main side &&\n> git rebase --abort &&\n> --\n> 2.39.2\n"},{"id":"474638","messageId":"CABPp-BGwj-_JRWose89tg2HXQoOoriPqHzW6XC0s5qJDayPTmQ@mail.gmail.com","threadId":"59400","inReplyTo":"3e02eeff78b23711187de47a1a820f9bde683200.1678893298.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 4/4] rebase: remove a couple of redundant strategy tests","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-01T19:31:09Z","receivedAt":"2023-04-01T19:31:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 15, 2023 at 8:38 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> The test removed in t3402 has been redundant ever since 80ff47957b\n> (rebase: remember strategy and strategy options, 2011-02-06) which added\n> a new test the first part of which (as noted in the commit message)\n> duplicated the existing test. The test removed in t3418 has been\n> redundant since the merge backend was removed in 68aa495b59 (rebase:\n> implement --merge via the interactive machinery, 2018-12-11) as it now\n> tests the same code paths as the preceding test.\n\nI was really confused by this commit message at first. Eventually, I\nfigured out you were talking about the changes in this commit, just in\npast tense. Could we change it to imperative tense? E.g.\n\nRemove a test in t3402 that has been redundant ever since 80ff47957b\n(rebase: remember strategy and strategy options, 2011-02-06). That commit added\na new test, the first part of which (as noted in the old commit message)\nduplicated an existing test.\n\nAlso remove a test t3418 that has been redundant since the merge\nbackend was removed in 68aa495b59 (rebase: implement --merge\nvia the interactive machinery, 2018-12-11), since it now tests the\nsame code paths as the preceding test.\n\n> Signed-off-by: Phillip Wood \n> ---\n> t/t3402-rebase-merge.sh | 21 ---------------------\n> t/t3418-rebase-continue.sh | 32 --------------------------------\n> 2 files changed, 53 deletions(-)\n>\n> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\n> index 7e46f4ca85..79b0640c00 100755\n> --- a/t/t3402-rebase-merge.sh\n> +++ b/t/t3402-rebase-merge.sh\n> @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '\n> esac\n> '\n>\n> -test_expect_success 'rebase -s funny -Xopt' '\n> - test_when_finished \"rm -fr test-bin funny.was.run\" &&\n> - mkdir test-bin &&\n> - cat >test-bin/git-merge-funny <<-EOF &&\n> - #!$SHELL_PATH\n> - case \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n> - shift &&\n> - >funny.was.run &&\n> - exec git merge-recursive \"\\$@\"\n> - EOF\n> - chmod +x test-bin/git-merge-funny &&\n> - git reset --hard &&\n> - git checkout -b test-funny main^ &&\n> - test_commit funny &&\n> - (\n> - PATH=./test-bin:$PATH &&\n> - git rebase -s funny -Xopt main\n> - ) &&\n> - test -f funny.was.run\n> -'\n> -\n> test_expect_success 'rebase --skip works with two conflicts in a row' '\n> git checkout second-side &&\n> tr \"[A-Z]\" \"[a-z]\" tmp &&\n> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n> index 42c3954125..2d0789e554 100755\n> --- a/t/t3418-rebase-continue.sh\n> +++ b/t/t3418-rebase-continue.sh\n> @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n> test_cmp expect actual\n> '\n>\n> -test_expect_success 'rebase -i --continue handles merge strategy and options' '\n> - rm -fr .git/rebase-* &&\n> - git reset --hard commit-new-file-F2-on-topic-branch &&\n> - test_commit \"commit-new-file-F3-on-topic-branch-for-dash-i\" F3 32 &&\n> - test_when_finished \"rm -fr test-bin funny.was.run funny.args\" &&\n> - mkdir test-bin &&\n> - cat >test-bin/git-merge-funny <<-EOF &&\n> - #!$SHELL_PATH\n> - echo \"\\$@\" >>funny.args\n> - case \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n> - case \"\\$2\" in --foo) ;; *) exit 2 ;; esac\n> - case \"\\$4\" in --) ;; *) exit 2 ;; esac\n> - shift 2 &&\n> - >funny.was.run &&\n> - exec git merge-recursive \"\\$@\"\n> - EOF\n> - chmod +x test-bin/git-merge-funny &&\n> - (\n> - PATH=./test-bin:$PATH &&\n> - test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic\n> - ) &&\n> - test -f funny.was.run &&\n> - rm funny.was.run &&\n> - echo \"Resolved\" >F2 &&\n> - git add F2 &&\n> - (\n> - PATH=./test-bin:$PATH &&\n> - git rebase --continue\n> - ) &&\n> - test -f funny.was.run\n> -'\n> -\n> test_expect_success 'rebase -r passes merge strategy options correctly' '\n> rm -fr .git/rebase-* &&\n> git reset --hard commit-new-file-F3-on-topic-branch &&\n> --\n> 2.39.2\n\nLooks good otherwise.\n"},{"id":"474639","messageId":"CABPp-BHP1fp2-Dnc4O=yhDJq2-iNGAn7rJn0p_rhjYv2LaBOag@mail.gmail.com","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 0/4] rebase: cleanup merge strategy option handling","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-01T19:32:48Z","receivedAt":"2023-04-01T19:33:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 15, 2023 at 8:39 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> Cleanup the handling of --strategy-option now that we no longer need\n> to support \"--preserve-merges\" and properly quote the argument when\n> saving it to disc.\n\nNice! Thanks for taking the time to do these kinds of cleanups.\nOverall the code looks pretty good, but I left a few\ncomments/questions on patches 3 & 4.\n"},{"id":"474775","messageId":"743d228c-99d0-c753-e606-41208d00f246@gmail.com","threadId":"59400","inReplyTo":"CABPp-BF6_gmDCTo5j2fR5_KQfJYVuYZuYv6TdhsMvPYshXzu+g@mail.gmail.com","subject":"Re: [PATCH 3/4] rebase -m: fix serialization of strategy options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-04T13:02:58Z","receivedAt":"2023-04-04T13:03:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 01/04/2023 20:27, Elijah Newren wrote:\n> On Wed, Mar 15, 2023 at 8:51 AM Phillip Wood wrote:\n>>\n>> From: Phillip Wood \n>>\n>> To store the strategy options rebase prepends \" --\" to each one and\n>> writes them to a file. To load them it reads the file and passes the\n>> contents to split_cmdline(). This roughly mimics the behavior of the\n>> scripted rebase but has a couple of limitations, (1) options containing\n>> whitespace are not properly preserved (this is true of the scripted\n>> rebase as well) and (2) options containing '\"' or '\\' are incorrectly\n>> parsed and may cause the parser to return an error.\n>>\n>> Fix these limitations by quoting each option when they are stored so\n>> that they can be parsed correctly. Now that \"--preserve-merges\" no\n>> longer exist this change also stops prepending \"--\" to the options when\n>> they are stored as that was an artifact of the scripted rebase.\n>>\n>> These changes are backwards compatible so the files written by an older\n>> version of git can be still be read. They are also forwards compatible,\n>> the file can still be parsed by recent versions of git as they treat the\n>> \"--\" prefix as optional.\n> \n> I'm not sure we want to tie ourselves to support completing a rebase\n> with git version X+1 that was started with git version X. But, it's\n> really nice that you're paying attention to that level of detail. :-)\n\nIn the past we've had complaints when we've made backwards incompatible \nchanges but I'm not sure we've ever promised never to do it. Part of the \nproblem for users is that they may not be in control of when git gets \nupgraded. Also we've had reports where a macos user started a rebase \nwith tig which used a bundled version of git that was different to what \nthe user had available on the command line when the continued the rebase.\n\n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n>> sequencer.c | 23 +++++++++++++++++++----\n>> t/t3418-rebase-continue.sh | 34 ++++++++++++++++++++++------------\n>> t/t3436-rebase-more-options.sh | 24 ------------------------\n>> 3 files changed, 41 insertions(+), 40 deletions(-)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 83ea1016ae..8890d1f7a1 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -2930,7 +2930,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n>> count = split_cmdline(strategy_opts_string,\n>> (const char ***)&opts->xopts);\n>> if (count < 0)\n>> - die(_(\"could not split '%s': %s\"), strategy_opts_string,\n>> + BUG(\"could not split '%s': %s\", strategy_opts_string,\n>> split_cmdline_strerror(count));\n>> opts->xopts_nr = count;\n>> for (i = 0; i < opts->xopts_nr; i++) {\n>> @@ -3054,12 +3054,27 @@ static int read_populate_opts(struct replay_opts *opts)\n>>\n>> static void write_strategy_opts(struct replay_opts *opts)\n>> {\n>> - int i;\n>> struct strbuf buf = STRBUF_INIT;\n>>\n>> - for (i = 0; i < opts->xopts_nr; ++i)\n>> - strbuf_addf(&buf, \" --%s\", opts->xopts[i]);\n>> + /*\n>> + * Quote strategy options so that they can be read correctly\n>> + * by split_cmdline().\n>> + */\n>> + for (size_t i = 0; i < opts->xopts_nr; i++) {\n>> + char *arg = opts->xopts[i];\n>>\n>> + if (i)\n>> + strbuf_addch(&buf, ' ');\n>> + strbuf_addch(&buf, '\"');\n>> + for (size_t j = 0; arg[j]; j++) {\n>> + const char c = arg[j];\n>> +\n>> + if (c == '\"' || c =='\\\\')\n>> + strbuf_addch(&buf, '\\\\');\n>> + strbuf_addch(&buf, c);\n>> + }\n>> + strbuf_addch(&buf, '\"');\n>> + }\n> \n> Do we not have a function in quote.[ch] that can handle this? If not,\n> should we add this code to a function in that file and call it?\n\nNo we don't and yes we should, though in the end I've added it to \nalias.c as that is where split_cmdline() lives.\n\nThanks for you comments, I'll post a re-roll in the next couple of days\n\nPhillip\n\n> \n>> write_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n>> strbuf_release(&buf);\n>> }\n>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n>> index 130e2f9b55..42c3954125 100755\n>> --- a/t/t3418-rebase-continue.sh\n>> +++ b/t/t3418-rebase-continue.sh\n>> @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n>> rm -fr .git/rebase-* &&\n>> git reset --hard commit-new-file-F2-on-topic-branch &&\n>> test_commit \"commit-new-file-F3-on-topic-branch\" F3 32 &&\n>> - test_when_finished \"rm -fr test-bin funny.was.run\" &&\n>> + test_when_finished \"rm -fr test-bin\" &&\n>> mkdir test-bin &&\n>> - cat >test-bin/git-merge-funny <<-EOF &&\n>> - #!$SHELL_PATH\n>> - case \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n>> - shift &&\n>> - >funny.was.run &&\n>> - exec git merge-recursive \"\\$@\"\n>> +\n>> + write_script test-bin/git-merge-funny <<-\\EOF &&\n>> + printf \"[%s]\\n\" $# \"$1\" \"$2\" \"$3\" \"$5\" >actual\n>> + shift 3 &&\n>> + exec git merge-recursive \"$@\"\n>> EOF\n>> - chmod +x test-bin/git-merge-funny &&\n>> +\n>> + cat >expect <<-\\EOF &&\n>> + [7]\n>> + [--option=arg with space]\n>> + [--op\"tion\\]\n>> + [--new\n>> + line ]\n>> + [--]\n>> + EOF\n>> +\n>> + rm -f actual &&\n>> (\n>> PATH=./test-bin:$PATH &&\n>> - test_must_fail git rebase -s funny -Xopt main topic\n>> + test_must_fail git rebase -s funny -X\"option=arg with space\" \\\n>> + -Xop\\\"tion\\\\ -X\"new${LF}line \" main topic\n>> ) &&\n>> - test -f funny.was.run &&\n>> - rm funny.was.run &&\n>> + test_cmp expect actual &&\n>> + rm actual &&\n>> echo \"Resolved\" >F2 &&\n>> git add F2 &&\n>> (\n>> PATH=./test-bin:$PATH &&\n>> git rebase --continue\n>> ) &&\n>> - test -f funny.was.run\n>> + test_cmp expect actual\n>> '\n> \n> I appreciate the more stringent test to cover these new special cases. :-)\n> \n>>\n>> test_expect_success 'rebase -i --continue handles merge strategy and options' '\n>> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\n>> index 3adf42f47d..94671d3c46 100755\n>> --- a/t/t3436-rebase-more-options.sh\n>> +++ b/t/t3436-rebase-more-options.sh\n>> @@ -40,30 +40,6 @@ test_expect_success 'setup' '\n>> EOF\n>> '\n>>\n>> -test_expect_success 'bad -X arguments: unclosed quote' '\n>> - test_when_finished \"test_might_fail git rebase --abort\" &&\n>> - cat >expect <<-\\EOF &&\n>> - fatal: could not split '\\''--bad'\\'': unclosed quote\n>> - EOF\n>> - GIT_SEQUENCE_EDITOR=\"echo break >\" \\\n>> - git rebase -i -X\"bad argument\\\"\" side main &&\n>> - test_expect_code 128 git rebase --continue >out 2>actual &&\n>> - test_must_be_empty out &&\n>> - test_cmp expect actual\n>> -'\n>> -\n>> -test_expect_success 'bad -X arguments: bad escape' '\n>> - test_when_finished \"test_might_fail git rebase --abort\" &&\n>> - cat >expect <<-\\EOF &&\n>> - fatal: could not split '\\''--bad'\\'': cmdline ends with \\\n>> - EOF\n>> - GIT_SEQUENCE_EDITOR=\"echo break >\" \\\n>> - git rebase -i -X\"bad escape \\\\\" side main &&\n>> - test_expect_code 128 git rebase --continue >out 2>actual &&\n>> - test_must_be_empty out &&\n>> - test_cmp expect actual\n>> -'\n>> -\n>> test_expect_success '--ignore-whitespace works with apply backend' '\n>> test_must_fail git rebase --apply main side &&\n>> git rebase --abort &&\n>> --\n>> 2.39.2\n\n"},{"id":"474776","messageId":"f3bbbed3-b2d7-9d92-00cd-0a53ad4c676f@gmail.com","threadId":"59400","inReplyTo":"CABPp-BGwj-_JRWose89tg2HXQoOoriPqHzW6XC0s5qJDayPTmQ@mail.gmail.com","subject":"Re: [PATCH 4/4] rebase: remove a couple of redundant strategy tests","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-04T13:04:01Z","receivedAt":"2023-04-04T13:04:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 01/04/2023 20:31, Elijah Newren wrote:\n> On Wed, Mar 15, 2023 at 8:38 AM Phillip Wood wrote:\n>>\n>> From: Phillip Wood \n>>\n>> The test removed in t3402 has been redundant ever since 80ff47957b\n>> (rebase: remember strategy and strategy options, 2011-02-06) which added\n>> a new test the first part of which (as noted in the commit message)\n>> duplicated the existing test. The test removed in t3418 has been\n>> redundant since the merge backend was removed in 68aa495b59 (rebase:\n>> implement --merge via the interactive machinery, 2018-12-11) as it now\n>> tests the same code paths as the preceding test.\n> \n> I was really confused by this commit message at first. Eventually, I\n> figured out you were talking about the changes in this commit, just in\n> past tense. Could we change it to imperative tense? E.g.\n> \n> Remove a test in t3402 that has been redundant ever since 80ff47957b\n> (rebase: remember strategy and strategy options, 2011-02-06). That commit added\n> a new test, the first part of which (as noted in the old commit message)\n> duplicated an existing test.\n> \n> Also remove a test t3418 that has been redundant since the merge\n> backend was removed in 68aa495b59 (rebase: implement --merge\n> via the interactive machinery, 2018-12-11), since it now tests the\n> same code paths as the preceding test.\n\nThanks, that is clearer, I'll use your suggested wording when I re-roll\n\nBest Wishes\n\nPhillip\n\n>> Signed-off-by: Phillip Wood \n>> ---\n>> t/t3402-rebase-merge.sh | 21 ---------------------\n>> t/t3418-rebase-continue.sh | 32 --------------------------------\n>> 2 files changed, 53 deletions(-)\n>>\n>> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\n>> index 7e46f4ca85..79b0640c00 100755\n>> --- a/t/t3402-rebase-merge.sh\n>> +++ b/t/t3402-rebase-merge.sh\n>> @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '\n>> esac\n>> '\n>>\n>> -test_expect_success 'rebase -s funny -Xopt' '\n>> - test_when_finished \"rm -fr test-bin funny.was.run\" &&\n>> - mkdir test-bin &&\n>> - cat >test-bin/git-merge-funny <<-EOF &&\n>> - #!$SHELL_PATH\n>> - case \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n>> - shift &&\n>> - >funny.was.run &&\n>> - exec git merge-recursive \"\\$@\"\n>> - EOF\n>> - chmod +x test-bin/git-merge-funny &&\n>> - git reset --hard &&\n>> - git checkout -b test-funny main^ &&\n>> - test_commit funny &&\n>> - (\n>> - PATH=./test-bin:$PATH &&\n>> - git rebase -s funny -Xopt main\n>> - ) &&\n>> - test -f funny.was.run\n>> -'\n>> -\n>> test_expect_success 'rebase --skip works with two conflicts in a row' '\n>> git checkout second-side &&\n>> tr \"[A-Z]\" \"[a-z]\" tmp &&\n>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n>> index 42c3954125..2d0789e554 100755\n>> --- a/t/t3418-rebase-continue.sh\n>> +++ b/t/t3418-rebase-continue.sh\n>> @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n>> test_cmp expect actual\n>> '\n>>\n>> -test_expect_success 'rebase -i --continue handles merge strategy and options' '\n>> - rm -fr .git/rebase-* &&\n>> - git reset --hard commit-new-file-F2-on-topic-branch &&\n>> - test_commit \"commit-new-file-F3-on-topic-branch-for-dash-i\" F3 32 &&\n>> - test_when_finished \"rm -fr test-bin funny.was.run funny.args\" &&\n>> - mkdir test-bin &&\n>> - cat >test-bin/git-merge-funny <<-EOF &&\n>> - #!$SHELL_PATH\n>> - echo \"\\$@\" >>funny.args\n>> - case \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n>> - case \"\\$2\" in --foo) ;; *) exit 2 ;; esac\n>> - case \"\\$4\" in --) ;; *) exit 2 ;; esac\n>> - shift 2 &&\n>> - >funny.was.run &&\n>> - exec git merge-recursive \"\\$@\"\n>> - EOF\n>> - chmod +x test-bin/git-merge-funny &&\n>> - (\n>> - PATH=./test-bin:$PATH &&\n>> - test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic\n>> - ) &&\n>> - test -f funny.was.run &&\n>> - rm funny.was.run &&\n>> - echo \"Resolved\" >F2 &&\n>> - git add F2 &&\n>> - (\n>> - PATH=./test-bin:$PATH &&\n>> - git rebase --continue\n>> - ) &&\n>> - test -f funny.was.run\n>> -'\n>> -\n>> test_expect_success 'rebase -r passes merge strategy options correctly' '\n>> rm -fr .git/rebase-* &&\n>> git reset --hard commit-new-file-F3-on-topic-branch &&\n>> --\n>> 2.39.2\n> \n> Looks good otherwise.\n\n"},{"id":"474852","messageId":"cover.1680708043.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 0/5] rebase: cleanup merge strategy option handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-05T15:21:43Z","receivedAt":"2023-04-05T15:22:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCleanup the handling of --strategy-option now that we no longer need to\nsupport \"--preserve-merges\" and properly quote the argument when saving\nit to disc.\n\nThanks to Elijah for his comments on V1.\n\nChanges since V1:\n\nI've rebased these patches onto 'master' to avoid conflicts with\n'sg/parse-options-h-initializers' in the new patch 2 (this series\ndepends on 'ab/fix-strategy-opts-parsing' but that has now been merged\nto master).\n\nPatch 1 - Unchanged.\n\nPatch 2 - New patch to store the merge strategy options in an \"struct\n strvec\". This patch also introduces a new macro OPT_STRVEC()\n to collect options into an \"struct strvec\".\n\nPatch 3 - Small simplification due to the changes in patch 2.\n\nPatch 4 - Moved the code to quote a list so it can split by\n split_cmdline() into a new function quote_cmdline() as\n\t suggested by Elijah.\n\nPatch 5 - Reworded the commit message as suggested by Elijah.\n\n\nBase-Commit: 140b9478dad5d19543c1cb4fd293ccec228f1240\nPublished-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv2\nView-Changes-At: https://github.com/phillipwood/git/compare/140b9478d...3515c31b4\nFetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v2\n\nPhillip Wood (5):\n rebase: stop reading and writing unnecessary strategy state\n sequencer: use struct strvec to store merge strategy options\n rebase -m: cleanup --strategy-option handling\n rebase -m: fix serialization of strategy options\n rebase: remove a couple of redundant strategy tests\n\n alias.c | 18 +++++++\n alias.h | 3 ++\n builtin/rebase.c | 54 ++++-----------------\n builtin/revert.c | 20 ++------\n parse-options-cb.c | 16 +++++++\n parse-options.h | 10 ++++\n sequencer.c | 57 ++++++++++------------\n sequencer.h | 12 +++--\n t/t3402-rebase-merge.sh | 21 --------\n t/t3418-rebase-continue.sh | 88 +++++++++++++---------------------\n t/t3436-rebase-more-options.sh | 18 -------\n 11 files changed, 126 insertions(+), 191 deletions(-)\n\nRange-diff against v1:\n1: 029d0bf2b6 = 1: 2353c753f5 rebase: stop reading and writing unnecessary strategy state\n-: ---------- > 2: dd7b82cdd5 sequencer: use struct strvec to store merge strategy options\n2: a01b182644 ! 3: ccef0e6f4b rebase -m: cleanup --strategy-option handling\n @@ builtin/rebase.c: struct rebase_options {\n \t\t.config_autosquash = -1, \\\n \t\t.update_refs = -1, \\\n \t\t.config_update_refs = -1, \\\n -+\t\t.strategy_opts = STRING_LIST_INIT_NODUP\t\\\n ++\t\t.strategy_opts = STRING_LIST_INIT_NODUP,\\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n @@ builtin/rebase.c: static struct replay_opts get_replay_opts(const struct rebase_\n \n -\tif (opts->strategy_opts)\n -\t\tparse_strategy_opts(&replay, opts->strategy_opts);\n -+\tif (opts->strategy_opts.nr) {\n -+\t\tALLOC_ARRAY(replay.xopts, opts->strategy_opts.nr);\n -+\t\treplay.xopts_nr = opts->strategy_opts.nr;\n -+\t\treplay.xopts_alloc = opts->strategy_opts.nr;\n -+\t\tfor (size_t i = 0; i < opts->strategy_opts.nr; i++)\n -+\t\t\treplay.xopts[i] =\n -+\t\t\t\txstrdup(opts->strategy_opts.items[i].string);\n -+\t}\n ++\tfor (size_t i = 0; i < opts->strategy_opts.nr; i++)\n ++\t\tstrvec_push(&replay.xopts, opts->strategy_opts.items[i].string);\n \n \tif (opts->squash_onto) {\n \t\toidcpy(&replay.squash_onto, opts->squash_onto);\n3: 9af68cb065 ! 4: 9a90212ef2 rebase -m: fix serialization of strategy options\n @@ Commit message\n \n Signed-off-by: Phillip Wood \n \n + ## alias.c ##\n +@@\n + #include \"alloc.h\"\n + #include \"config.h\"\n + #include \"gettext.h\"\n ++#include \"strbuf.h\"\n + #include \"string-list.h\"\n + \n + struct config_alias_data {\n +@@ alias.c: void list_aliases(struct string_list *list)\n + \tread_early_config(config_alias_cb, &data);\n + }\n + \n ++void quote_cmdline(struct strbuf *buf, const char **argv)\n ++{\n ++\tfor (const char **argp = argv; *argp; argp++) {\n ++\t\tif (argp != argv)\n ++\t\t\tstrbuf_addch(buf, ' ');\n ++\t\tstrbuf_addch(buf, '\"');\n ++\t\tfor (const char *p = *argp; *p; p++) {\n ++\t\t\tconst char c = *p;\n ++\n ++\t\t\tif (c == '\"' || c =='\\\\')\n ++\t\t\t\tstrbuf_addch(buf, '\\\\');\n ++\t\t\tstrbuf_addch(buf, c);\n ++\t\t}\n ++\t\tstrbuf_addch(buf, '\"');\n ++\t}\n ++}\n ++\n + #define SPLIT_CMDLINE_BAD_ENDING 1\n + #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2\n + #define SPLIT_CMDLINE_ARGC_OVERFLOW 3\n +\n + ## alias.h ##\n +@@\n + #ifndef ALIAS_H\n + #define ALIAS_H\n + \n ++struct strbuf;\n + struct string_list;\n + \n + char *alias_lookup(const char *alias);\n ++/* Quote argv so buf can be parsed by split_cmdline() */\n ++void quote_cmdline(struct strbuf *buf, const char **argv);\n + int split_cmdline(char *cmdline, const char ***argv);\n + /* Takes a negative value returned by split_cmdline */\n + const char *split_cmdline_strerror(int cmdline_errno);\n +\n ## sequencer.c ##\n @@ sequencer.c: static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n - \tcount = split_cmdline(strategy_opts_string,\n - \t\t\t (const char ***)&opts->xopts);\n + \n + \tcount = split_cmdline(strategy_opts_string, &argv);\n \tif (count < 0)\n -\t\tdie(_(\"could not split '%s': %s\"), strategy_opts_string,\n +\t\tBUG(\"could not split '%s': %s\", strategy_opts_string,\n \t\t\t split_cmdline_strerror(count));\n - \topts->xopts_nr = count;\n - \tfor (i = 0; i < opts->xopts_nr; i++) {\n + \tfor (i = 0; i < count; i++) {\n + \t\tconst char *arg = argv[i];\n @@ sequencer.c: static int read_populate_opts(struct replay_opts *opts)\n \n static void write_strategy_opts(struct replay_opts *opts)\n {\n -\tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n --\tfor (i = 0; i < opts->xopts_nr; ++i)\n --\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts[i]);\n +-\tfor (i = 0; i < opts->xopts.nr; ++i)\n +-\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts.v[i]);\n -\n +\t/*\n +\t * Quote strategy options so that they can be read correctly\n +\t * by split_cmdline().\n +\t */\n -+\tfor (size_t i = 0; i < opts->xopts_nr; i++) {\n -+\t\tchar *arg = opts->xopts[i];\n -+\n -+\t\tif (i)\n -+\t\t\tstrbuf_addch(&buf, ' ');\n -+\t\tstrbuf_addch(&buf, '\"');\n -+\t\tfor (size_t j = 0; arg[j]; j++) {\n -+\t\t\tconst char c = arg[j];\n -+\n -+\t\t\tif (c == '\"' || c =='\\\\')\n -+\t\t\t\tstrbuf_addch(&buf, '\\\\');\n -+\t\t\tstrbuf_addch(&buf, c);\n -+\t\t}\n -+\t\tstrbuf_addch(&buf, '\"');\n -+\t}\n ++\tquote_cmdline(&buf, opts->xopts.v);\n \twrite_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n }\n4: 3e02eeff78 ! 5: 3515c31b40 rebase: remove a couple of redundant strategy tests\n @@ Metadata\n ## Commit message ##\n rebase: remove a couple of redundant strategy tests\n \n - The test removed in t3402 has been redundant ever since 80ff47957b\n - (rebase: remember strategy and strategy options, 2011-02-06) which added\n - a new test the first part of which (as noted in the commit message)\n - duplicated the existing test. The test removed in t3418 has been\n - redundant since the merge backend was removed in 68aa495b59 (rebase:\n - implement --merge via the interactive machinery, 2018-12-11) as it now\n - tests the same code paths as the preceding test.\n -\n + Remove a test in t3402 that has been redundant ever since 80ff47957b\n + (rebase: remember strategy and strategy options, 2011-02-06). That\n + commit added a new test, the first part of which (as noted in the old\n + commit message) duplicated an existing test.\n +\n + Also remove a test t3418 that has been redundant since the merge backend\n + was removed in 68aa495b59 (rebase: implement --merge via the interactive\n + machinery, 2018-12-11), since it now tests the same code paths as the\n + preceding test.\n +\n + Helped-by: Elijah Newren \n Signed-off-by: Phillip Wood \n \n ## t/t3402-rebase-merge.sh ##\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474853","messageId":"2353c753f5fe56b4ebc72e8c3acef788f31e6345.1680708043.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680708043.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 1/5] rebase: stop reading and writing unnecessary strategy state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-05T15:21:44Z","receivedAt":"2023-04-05T15:22:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe state files for \"--strategy\" and \"--strategy-option\" are written and\nread twice, once by builtin/rebase.c and then by sequencer.c. This is an\nartifact of the scripted rebase and the need to support \"rebase\n--preserve-merges\". Now that \"--preserve-merges\" no-longer exists we\nonly need to read and write these files in sequencer.c. This enables us\nto remove a call to free() in read_strategy_opts() that was added by\nf1f4ebf432 (sequencer.c: fix \"opts->strategy\" leak in\nread_strategy_opts(), 2022-11-08) as this commit fixes the root cause of\nthat leak.\n\nThere is further scope for removing duplication in the reading and\nwriting of state files between builtin/rebase.c and sequencer.c but that\nis left for a follow up series.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 24 ------------------------\n sequencer.c | 1 -\n 2 files changed, 25 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5b7b908b66..3bd215c771 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -483,24 +483,6 @@ static int read_basic_state(struct rebase_options *opts)\n \t\topts->gpg_sign_opt = xstrdup(buf.buf);\n \t}\n \n-\tif (file_exists(state_dir_path(\"strategy\", opts))) {\n-\t\tstrbuf_reset(&buf);\n-\t\tif (!read_oneliner(&buf, state_dir_path(\"strategy\", opts),\n-\t\t\t\t READ_ONELINER_WARN_MISSING))\n-\t\t\treturn -1;\n-\t\tfree(opts->strategy);\n-\t\topts->strategy = xstrdup(buf.buf);\n-\t}\n-\n-\tif (file_exists(state_dir_path(\"strategy_opts\", opts))) {\n-\t\tstrbuf_reset(&buf);\n-\t\tif (!read_oneliner(&buf, state_dir_path(\"strategy_opts\", opts),\n-\t\t\t\t READ_ONELINER_WARN_MISSING))\n-\t\t\treturn -1;\n-\t\tfree(opts->strategy_opts);\n-\t\topts->strategy_opts = xstrdup(buf.buf);\n-\t}\n-\n \tstrbuf_release(&buf);\n \n \treturn 0;\n@@ -518,12 +500,6 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n \t\twrite_file(state_dir_path(\"quiet\", opts), \"%s\", \"\");\n \tif (opts->flags & REBASE_VERBOSE)\n \t\twrite_file(state_dir_path(\"verbose\", opts), \"%s\", \"\");\n-\tif (opts->strategy)\n-\t\twrite_file(state_dir_path(\"strategy\", opts), \"%s\",\n-\t\t\t opts->strategy);\n-\tif (opts->strategy_opts)\n-\t\twrite_file(state_dir_path(\"strategy_opts\", opts), \"%s\",\n-\t\t\t opts->strategy_opts);\n \tif (opts->allow_rerere_autoupdate > 0)\n \t\twrite_file(state_dir_path(\"allow_rerere_autoupdate\", opts),\n \t\t\t \"-%s-rerere-autoupdate\",\ndiff --git a/sequencer.c b/sequencer.c\nindex 3be23d7ca2..c35a67e104 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2944,7 +2944,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)\n \tstrbuf_reset(buf);\n \tif (!read_oneliner(buf, rebase_path_strategy(), 0))\n \t\treturn;\n-\tfree(opts->strategy);\n \topts->strategy = strbuf_detach(buf, NULL);\n \tif (!read_oneliner(buf, rebase_path_strategy_opts(), 0))\n \t\treturn;\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474854","messageId":"ccef0e6f4bfc9a6c1ef378ec5974edec5d103fcd.1680708043.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680708043.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 3/5] rebase -m: cleanup --strategy-option handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-05T15:21:46Z","receivedAt":"2023-04-05T15:22:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen handling \"--strategy-option\" rebase collects the commands into a\nstruct string_list, then concatenates them into a string, prepending \"--\"\nto each one before splitting the string and removing the \"--\" prefix.\nThis is an artifact of the scripted rebase and the need to support\n\"rebase --preserve-merges\". Now that \"--preserve-merges\" no-longer\nexists we can cleanup the way the argument is handled.\n\nThe tests for a bad strategy option are adjusted now that\nparse_strategy_opts() is no-longer called when starting a rebase. The\nfact that it only errors out when running \"git rebase --continue\" is a\nmixed blessing but the next commit will fix the root cause of the\nparsing problem so lets not worry about that here.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 30 ++++++++++--------------------\n sequencer.c | 2 +-\n sequencer.h | 1 -\n t/t3436-rebase-more-options.sh | 10 ++++++++--\n 4 files changed, 19 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 3bd215c771..511922c6fc 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -117,7 +117,8 @@ struct rebase_options {\n \tstruct string_list exec;\n \tint allow_empty_message;\n \tint rebase_merges, rebase_cousins;\n-\tchar *strategy, *strategy_opts;\n+\tchar *strategy;\n+\tstruct string_list strategy_opts;\n \tstruct strbuf git_format_patch_opt;\n \tint reschedule_failed_exec;\n \tint reapply_cherry_picks;\n@@ -143,6 +144,7 @@ struct rebase_options {\n \t\t.config_autosquash = -1, \\\n \t\t.update_refs = -1, \\\n \t\t.config_update_refs = -1, \\\n+\t\t.strategy_opts = STRING_LIST_INIT_NODUP,\\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -176,8 +178,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\treplay.default_strategy = NULL;\n \t}\n \n-\tif (opts->strategy_opts)\n-\t\tparse_strategy_opts(&replay, opts->strategy_opts);\n+\tfor (size_t i = 0; i < opts->strategy_opts.nr; i++)\n+\t\tstrvec_push(&replay.xopts, opts->strategy_opts.items[i].string);\n \n \tif (opts->squash_onto) {\n \t\toidcpy(&replay.squash_onto, opts->squash_onto);\n@@ -1013,7 +1015,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tint ignore_whitespace = 0;\n \tconst char *gpg_sign = NULL;\n \tconst char *rebase_merges = NULL;\n-\tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n \tchar *squash_onto_name = NULL;\n \tchar *keep_base_onto_name = NULL;\n@@ -1122,7 +1123,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"use 'merge-base --fork-point' to refine upstream\")),\n \t\tOPT_STRING('s', \"strategy\", &options.strategy,\n \t\t\t N_(\"strategy\"), N_(\"use the given merge strategy\")),\n-\t\tOPT_STRING_LIST('X', \"strategy-option\", &strategy_options,\n+\t\tOPT_STRING_LIST('X', \"strategy-option\", &options.strategy_opts,\n \t\t\t\tN_(\"option\"),\n \t\t\t\tN_(\"pass the argument through to the merge \"\n \t\t\t\t \"strategy\")),\n@@ -1436,23 +1437,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t} else {\n \t\t/* REBASE_MERGE */\n \t\tif (ignore_whitespace) {\n-\t\t\tstring_list_append(&strategy_options,\n+\t\t\tstring_list_append(&options.strategy_opts,\n \t\t\t\t\t \"ignore-space-change\");\n \t\t}\n \t}\n \n-\tif (strategy_options.nr) {\n-\t\tint i;\n-\n-\t\tif (!options.strategy)\n-\t\t\toptions.strategy = \"ort\";\n-\n-\t\tstrbuf_reset(&buf);\n-\t\tfor (i = 0; i < strategy_options.nr; i++)\n-\t\t\tstrbuf_addf(&buf, \" --%s\",\n-\t\t\t\t strategy_options.items[i].string);\n-\t\toptions.strategy_opts = xstrdup(buf.buf);\n-\t}\n+\tif (options.strategy_opts.nr && !options.strategy)\n+\t\toptions.strategy = \"ort\";\n \n \tif (options.strategy) {\n \t\toptions.strategy = xstrdup(options.strategy);\n@@ -1827,10 +1818,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tfree(options.gpg_sign_opt);\n \tstring_list_clear(&options.exec, 0);\n \tfree(options.strategy);\n-\tfree(options.strategy_opts);\n+\tstring_list_clear(&options.strategy_opts, 0);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n \tfree(keep_base_onto_name);\n-\tstring_list_clear(&strategy_options, 0);\n \treturn !!ret;\n }\ndiff --git a/sequencer.c b/sequencer.c\nindex 6e2f3357c8..045d549042 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2913,7 +2913,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \treturn 0;\n }\n \n-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n+static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n {\n \tint i;\n \tint count;\ndiff --git a/sequencer.h b/sequencer.h\nindex 8a79d6b200..913a0f652d 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -252,7 +252,6 @@ int read_oneliner(struct strbuf *buf,\n \tconst char *path, unsigned flags);\n int read_author_script(const char *path, char **name, char **email, char **date,\n \t\t int allow_missing);\n-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\t struct commit *onto, const struct object_id *orig_head);\n void sequencer_post_commit_cleanup(struct repository *r, int verbose);\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex c3184c9ade..3adf42f47d 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -41,19 +41,25 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'bad -X arguments: unclosed quote' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tcat >expect <<-\\EOF &&\n \tfatal: could not split '\\''--bad'\\'': unclosed quote\n \tEOF\n-\ttest_expect_code 128 git rebase -X\"bad argument\\\"\" side main >out 2>actual &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n+\t\tgit rebase -i -X\"bad argument\\\"\" side main &&\n+\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n \ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'bad -X arguments: bad escape' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tcat >expect <<-\\EOF &&\n \tfatal: could not split '\\''--bad'\\'': cmdline ends with \\\n \tEOF\n-\ttest_expect_code 128 git rebase -X\"bad escape \\\\\" side main >out 2>actual &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n+\t\tgit rebase -i -X\"bad escape \\\\\" side main &&\n+\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n \ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474855","messageId":"dd7b82cdd54cc962385dff8a1adab4c1d4fb2167.1680708043.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680708043.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-05T15:21:45Z","receivedAt":"2023-04-05T15:22:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe sequencer stores the merge strategy options in an array of strings\nwhich allocated with ALLOC_GROW(). Using \"struct strvec\" avoids manually\nmanaging the memory of that array and simplifies the code.\n\nAside from memory allocation the changes to the sequencer are largely\nmechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A\nnew option parsing macro OPT_STRVEC() is also added to collect the\nstrategy options. Hopefully this can be used to simplify the code in\nbuiltin/merge.c in the future.\n\nNote that there is a change of behavior to \"git cherry-pick\" and \"git\nrevert\" as passing “--no-strategy-option” will now clear any previous\nstrategy options whereas before this change it did nothing.\n\nSigned-off-by: Phillip Wood \n---\n builtin/revert.c | 20 +++-----------------\n parse-options-cb.c | 16 ++++++++++++++++\n parse-options.h | 10 ++++++++++\n sequencer.c | 47 ++++++++++++++++++++--------------------------\n sequencer.h | 11 ++++++++---\n 5 files changed, 57 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 62986a7b1b..362857da65 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n \treturn opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n }\n \n-static int option_parse_x(const struct option *opt,\n-\t\t\t const char *arg, int unset)\n-{\n-\tstruct replay_opts **opts_ptr = opt->value;\n-\tstruct replay_opts *opts = *opts_ptr;\n-\n-\tif (unset)\n-\t\treturn 0;\n-\n-\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n-\topts->xopts[opts->xopts_nr++] = xstrdup(arg);\n-\treturn 0;\n-}\n-\n static int option_parse_m(const struct option *opt,\n \t\t\t const char *arg, int unset)\n {\n@@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t N_(\"select mainline parent\"), option_parse_m),\n \t\tOPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),\n \t\tOPT_STRING(0, \"strategy\", &opts->strategy, N_(\"strategy\"), N_(\"merge strategy\")),\n-\t\tOPT_CALLBACK('X', \"strategy-option\", &opts, N_(\"option\"),\n-\t\t\tN_(\"option for merge strategy\"), option_parse_x),\n+\t\tOPT_STRVEC('X', \"strategy-option\", &opts->xopts, N_(\"option\"),\n+\t\t\tN_(\"option for merge strategy\")),\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \t\tOPT_END()\n@@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"--signoff\", opts->signoff,\n \t\t\t\t\"--mainline\", opts->mainline,\n \t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n-\t\t\t\t\"--strategy-option\", opts->xopts ? 1 : 0,\n+\t\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n \t\t\t\t\"-x\", opts->record_origin,\n \t\t\t\t\"--ff\", opts->allow_ff,\n \t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex d346dbe210..8eb96c5ca9 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+int parse_opt_strvec(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct strvec *v = opt->value;\n+\n+\tif (unset) {\n+\t\tstrvec_clear(v);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!arg)\n+\t\treturn -1;\n+\n+\tstrvec_push(v, arg);\n+\treturn 0;\n+}\n+\n int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)\n {\n \treturn 0;\ndiff --git a/parse-options.h b/parse-options.h\nindex 26f19384e5..2d1d4d052f 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -285,6 +285,15 @@ struct option {\n \t.help = (h), \\\n \t.callback = &parse_opt_string_list, \\\n }\n+#define OPT_STRVEC(s, l, v, a, h) { \\\n+\t.type = OPTION_CALLBACK, \\\n+\t.short_name = (s), \\\n+\t.long_name = (l), \\\n+\t.value = (v), \\\n+\t.argh = (a), \\\n+\t.help = (h), \\\n+\t.callback = &parse_opt_strvec, \\\n+}\n #define OPT_UYN(s, l, v, h) { \\\n \t.type = OPTION_CALLBACK, \\\n \t.short_name = (s), \\\n@@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int);\n int parse_opt_commit(const struct option *, const char *, int);\n int parse_opt_tertiary(const struct option *, const char *, int);\n int parse_opt_string_list(const struct option *, const char *, int);\n+int parse_opt_strvec(const struct option *, const char *, int);\n int parse_opt_noop_cb(const struct option *, const char *, int);\n enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t const struct option *,\ndiff --git a/sequencer.c b/sequencer.c\nindex c35a67e104..6e2f3357c8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts)\n \tfree(opts->reflog_action);\n \tfree(opts->default_strategy);\n \tfree(opts->strategy);\n-\tfor (size_t i = 0; i < opts->xopts_nr; i++)\n-\t\tfree(opts->xopts[i]);\n-\tfree(opts->xopts);\n+\tstrvec_clear (&opts->xopts);\n \tstrbuf_release(&opts->current_fixups);\n \tif (opts->revs)\n \t\trelease_revisions(opts->revs);\n@@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r,\n \tnext_tree = next ? get_commit_tree(next) : empty_tree(r);\n \tbase_tree = base ? get_commit_tree(base) : empty_tree(r);\n \n-\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\tparse_merge_opt(&o, opts->xopts[i]);\n+\tfor (i = 0; i < opts->xopts.nr; i++)\n+\t\tparse_merge_opt(&o, opts->xopts.v[i]);\n \n \tif (!opts->strategy || !strcmp(opts->strategy, \"ort\")) {\n \t\tmemset(&result, 0, sizeof(result));\n@@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r,\n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n \t\tres |= try_merge_command(r, opts->strategy,\n-\t\t\t\t\t opts->xopts_nr, (const char **)opts->xopts,\n+\t\t\t\t\t opts->xopts.nr, opts->xopts.v,\n \t\t\t\t\tcommon, oid_to_hex(&head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n@@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \telse if (!strcmp(key, \"options.gpg-sign\"))\n \t\tgit_config_string_dup(&opts->gpg_sign, key, value);\n \telse if (!strcmp(key, \"options.strategy-option\")) {\n-\t\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n-\t\topts->xopts[opts->xopts_nr++] = xstrdup(value);\n+\t\tstrvec_push(&opts->xopts, value);\n \t} else if (!strcmp(key, \"options.allow-rerere-auto\"))\n \t\topts->allow_rerere_auto =\n \t\t\tgit_config_bool_or_int(key, value, &error_flag) ?\n@@ -2920,22 +2917,21 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n {\n \tint i;\n \tint count;\n+\tconst char **argv;\n \tchar *strategy_opts_string = raw_opts;\n \n \tif (*strategy_opts_string == ' ')\n \t\tstrategy_opts_string++;\n \n-\tcount = split_cmdline(strategy_opts_string,\n-\t\t\t (const char ***)&opts->xopts);\n+\tcount = split_cmdline(strategy_opts_string, &argv);\n \tif (count < 0)\n \t\tdie(_(\"could not split '%s': %s\"), strategy_opts_string,\n \t\t\t split_cmdline_strerror(count));\n-\topts->xopts_nr = count;\n-\tfor (i = 0; i < opts->xopts_nr; i++) {\n-\t\tconst char *arg = opts->xopts[i];\n+\tfor (i = 0; i < count; i++) {\n+\t\tconst char *arg = argv[i];\n \n \t\tskip_prefix(arg, \"--\", &arg);\n-\t\topts->xopts[i] = xstrdup(arg);\n+\t\tstrvec_push(&opts->xopts, arg);\n \t}\n }\n \n@@ -3055,8 +3051,8 @@ static void write_strategy_opts(struct replay_opts *opts)\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfor (i = 0; i < opts->xopts_nr; ++i)\n-\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts[i]);\n+\tfor (i = 0; i < opts->xopts.nr; ++i)\n+\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts.v[i]);\n \n \twrite_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n@@ -3080,7 +3076,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_verbose(), \"%s\", \"\");\n \tif (opts->strategy)\n \t\twrite_file(rebase_path_strategy(), \"%s\\n\", opts->strategy);\n-\tif (opts->xopts_nr > 0)\n+\tif (opts->xopts.nr > 0)\n \t\twrite_strategy_opts(opts);\n \n \tif (opts->allow_rerere_auto == RERERE_AUTOUPDATE)\n@@ -3464,13 +3460,10 @@ static int save_opts(struct replay_opts *opts)\n \tif (opts->gpg_sign)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.gpg-sign\", opts->gpg_sign);\n-\tif (opts->xopts) {\n-\t\tint i;\n-\t\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.strategy-option\",\n-\t\t\t\t\topts->xopts[i], \"^$\", 0);\n-\t}\n+\tfor (size_t i = 0; i < opts->xopts.nr; i++)\n+\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n+\t\t\t\t\"options.strategy-option\",\n+\t\t\t\topts->xopts.v[i], \"^$\", 0);\n \tif (opts->allow_rerere_auto)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\"options.allow-rerere-auto\",\n@@ -3880,7 +3873,7 @@ static int do_merge(struct repository *r,\n \tstruct commit *head_commit, *merge_commit, *i;\n \tstruct commit_list *bases, *j;\n \tstruct commit_list *to_merge = NULL, **tail = &to_merge;\n-\tconst char *strategy = !opts->xopts_nr &&\n+\tconst char *strategy = !opts->xopts.nr &&\n \t\t(!opts->strategy ||\n \t\t !strcmp(opts->strategy, \"recursive\") ||\n \t\t !strcmp(opts->strategy, \"ort\")) ?\n@@ -4063,9 +4056,9 @@ static int do_merge(struct repository *r,\n \t\t\tstrvec_push(&cmd.args, \"octopus\");\n \t\telse {\n \t\t\tstrvec_push(&cmd.args, strategy);\n-\t\t\tfor (k = 0; k < opts->xopts_nr; k++)\n+\t\t\tfor (k = 0; k < opts->xopts.nr; k++)\n \t\t\t\tstrvec_pushf(&cmd.args,\n-\t\t\t\t\t \"-X%s\", opts->xopts[k]);\n+\t\t\t\t\t \"-X%s\", opts->xopts.v[k]);\n \t\t}\n \t\tif (!(flags & TODO_EDIT_MERGE_MSG))\n \t\t\tstrvec_push(&cmd.args, \"--no-edit\");\ndiff --git a/sequencer.h b/sequencer.h\nindex 33dbaf5b66..8a79d6b200 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -2,6 +2,7 @@\n #define SEQUENCER_H\n \n #include \"strbuf.h\"\n+#include \"strvec.h\"\n #include \"wt-status.h\"\n \n struct commit;\n@@ -60,8 +61,7 @@ struct replay_opts {\n \t/* Merge strategy */\n \tchar *default_strategy; /* from config options */\n \tchar *strategy;\n-\tchar **xopts;\n-\tsize_t xopts_nr, xopts_alloc;\n+\tstruct strvec xopts;\n \n \t/* Reflog */\n \tchar *reflog_action;\n@@ -80,7 +80,12 @@ struct replay_opts {\n \t/* Private use */\n \tconst char *reflog_message;\n };\n-#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n+#define REPLAY_OPTS_INIT {\t\t\t\\\n+\t.edit = -1,\t\t\t\t\\\n+\t.action = -1,\t\t\t\t\\\n+\t.current_fixups = STRBUF_INIT,\t\t\\\n+\t.xopts = STRVEC_INIT,\t\t\t\\\n+}\n \n /*\n * Note that ordering matters in this enum. Not only must it match the mapping\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474856","messageId":"9a90212ef284510fa691b8eebd6bdd61098282fd.1680708043.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680708043.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 4/5] rebase -m: fix serialization of strategy options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-05T15:21:47Z","receivedAt":"2023-04-05T15:22:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nTo store the strategy options rebase prepends \" --\" to each one and\nwrites them to a file. To load them it reads the file and passes the\ncontents to split_cmdline(). This roughly mimics the behavior of the\nscripted rebase but has a couple of limitations, (1) options containing\nwhitespace are not properly preserved (this is true of the scripted\nrebase as well) and (2) options containing '\"' or '\\' are incorrectly\nparsed and may cause the parser to return an error.\n\nFix these limitations by quoting each option when they are stored so\nthat they can be parsed correctly. Now that \"--preserve-merges\" no\nlonger exist this change also stops prepending \"--\" to the options when\nthey are stored as that was an artifact of the scripted rebase.\n\nThese changes are backwards compatible so the files written by an older\nversion of git can be still be read. They are also forwards compatible,\nthe file can still be parsed by recent versions of git as they treat the\n\"--\" prefix as optional.\n\nSigned-off-by: Phillip Wood \n---\n alias.c | 18 +++++++++++++++++\n alias.h | 3 +++\n sequencer.c | 11 ++++++-----\n t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++------------\n t/t3436-rebase-more-options.sh | 24 -----------------------\n 5 files changed, 50 insertions(+), 42 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex e814948ced..54a1a23d2c 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -3,6 +3,7 @@\n #include \"alloc.h\"\n #include \"config.h\"\n #include \"gettext.h\"\n+#include \"strbuf.h\"\n #include \"string-list.h\"\n \n struct config_alias_data {\n@@ -46,6 +47,23 @@ void list_aliases(struct string_list *list)\n \tread_early_config(config_alias_cb, &data);\n }\n \n+void quote_cmdline(struct strbuf *buf, const char **argv)\n+{\n+\tfor (const char **argp = argv; *argp; argp++) {\n+\t\tif (argp != argv)\n+\t\t\tstrbuf_addch(buf, ' ');\n+\t\tstrbuf_addch(buf, '\"');\n+\t\tfor (const char *p = *argp; *p; p++) {\n+\t\t\tconst char c = *p;\n+\n+\t\t\tif (c == '\"' || c =='\\\\')\n+\t\t\t\tstrbuf_addch(buf, '\\\\');\n+\t\t\tstrbuf_addch(buf, c);\n+\t\t}\n+\t\tstrbuf_addch(buf, '\"');\n+\t}\n+}\n+\n #define SPLIT_CMDLINE_BAD_ENDING 1\n #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2\n #define SPLIT_CMDLINE_ARGC_OVERFLOW 3\ndiff --git a/alias.h b/alias.h\nindex aef4843bb7..43db736484 100644\n--- a/alias.h\n+++ b/alias.h\n@@ -1,9 +1,12 @@\n #ifndef ALIAS_H\n #define ALIAS_H\n \n+struct strbuf;\n struct string_list;\n \n char *alias_lookup(const char *alias);\n+/* Quote argv so buf can be parsed by split_cmdline() */\n+void quote_cmdline(struct strbuf *buf, const char **argv);\n int split_cmdline(char *cmdline, const char ***argv);\n /* Takes a negative value returned by split_cmdline */\n const char *split_cmdline_strerror(int cmdline_errno);\ndiff --git a/sequencer.c b/sequencer.c\nindex 045d549042..9907100c8a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n \n \tcount = split_cmdline(strategy_opts_string, &argv);\n \tif (count < 0)\n-\t\tdie(_(\"could not split '%s': %s\"), strategy_opts_string,\n+\t\tBUG(\"could not split '%s': %s\", strategy_opts_string,\n \t\t\t split_cmdline_strerror(count));\n \tfor (i = 0; i < count; i++) {\n \t\tconst char *arg = argv[i];\n@@ -3048,12 +3048,13 @@ static int read_populate_opts(struct replay_opts *opts)\n \n static void write_strategy_opts(struct replay_opts *opts)\n {\n-\tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfor (i = 0; i < opts->xopts.nr; ++i)\n-\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts.v[i]);\n-\n+\t/*\n+\t * Quote strategy options so that they can be read correctly\n+\t * by split_cmdline().\n+\t */\n+\tquote_cmdline(&buf, opts->xopts.v);\n \twrite_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n }\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 130e2f9b55..42c3954125 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard commit-new-file-F2-on-topic-branch &&\n \ttest_commit \"commit-new-file-F3-on-topic-branch\" F3 32 &&\n-\ttest_when_finished \"rm -fr test-bin funny.was.run\" &&\n+\ttest_when_finished \"rm -fr test-bin\" &&\n \tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tshift &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n+\n+\twrite_script test-bin/git-merge-funny <<-\\EOF &&\n+\tprintf \"[%s]\\n\" $# \"$1\" \"$2\" \"$3\" \"$5\" >actual\n+\tshift 3 &&\n+\texec git merge-recursive \"$@\"\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\t[7]\n+\t[--option=arg with space]\n+\t[--op\"tion\\]\n+\t[--new\n+\tline ]\n+\t[--]\n+\tEOF\n+\n+\trm -f actual &&\n \t(\n \t\tPATH=./test-bin:$PATH &&\n-\t\ttest_must_fail git rebase -s funny -Xopt main topic\n+\t\ttest_must_fail git rebase -s funny -X\"option=arg with space\" \\\n+\t\t\t\t-Xop\\\"tion\\\\ -X\"new${LF}line \" main topic\n \t) &&\n-\ttest -f funny.was.run &&\n-\trm funny.was.run &&\n+\ttest_cmp expect actual &&\n+\trm actual &&\n \techo \"Resolved\" >F2 &&\n \tgit add F2 &&\n \t(\n \t\tPATH=./test-bin:$PATH &&\n \t\tgit rebase --continue\n \t) &&\n-\ttest -f funny.was.run\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'rebase -i --continue handles merge strategy and options' '\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 3adf42f47d..94671d3c46 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -40,30 +40,6 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n-test_expect_success 'bad -X arguments: unclosed quote' '\n-\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n-\tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': unclosed quote\n-\tEOF\n-\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n-\t\tgit rebase -i -X\"bad argument\\\"\" side main &&\n-\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'bad -X arguments: bad escape' '\n-\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n-\tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': cmdline ends with \\\n-\tEOF\n-\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n-\t\tgit rebase -i -X\"bad escape \\\\\" side main &&\n-\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect actual\n-'\n-\n test_expect_success '--ignore-whitespace works with apply backend' '\n \ttest_must_fail git rebase --apply main side &&\n \tgit rebase --abort &&\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474857","messageId":"3515c31b40e171018e8288220765ca6f0725a17f.1680708043.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680708043.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 5/5] rebase: remove a couple of redundant strategy tests","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-05T15:21:48Z","receivedAt":"2023-04-05T15:22:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nRemove a test in t3402 that has been redundant ever since 80ff47957b\n(rebase: remember strategy and strategy options, 2011-02-06). That\ncommit added a new test, the first part of which (as noted in the old\ncommit message) duplicated an existing test.\n\nAlso remove a test t3418 that has been redundant since the merge backend\nwas removed in 68aa495b59 (rebase: implement --merge via the interactive\nmachinery, 2018-12-11), since it now tests the same code paths as the\npreceding test.\n\nHelped-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n t/t3402-rebase-merge.sh | 21 ---------------------\n t/t3418-rebase-continue.sh | 32 --------------------------------\n 2 files changed, 53 deletions(-)\n\ndiff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\nindex 7e46f4ca85..79b0640c00 100755\n--- a/t/t3402-rebase-merge.sh\n+++ b/t/t3402-rebase-merge.sh\n@@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '\n \tesac\n '\n \n-test_expect_success 'rebase -s funny -Xopt' '\n-\ttest_when_finished \"rm -fr test-bin funny.was.run\" &&\n-\tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tshift &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n-\tgit reset --hard &&\n-\tgit checkout -b test-funny main^ &&\n-\ttest_commit funny &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\tgit rebase -s funny -Xopt main\n-\t) &&\n-\ttest -f funny.was.run\n-'\n-\n test_expect_success 'rebase --skip works with two conflicts in a row' '\n \tgit checkout second-side &&\n \ttr \"[A-Z]\" \"[a-z]\" tmp &&\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 42c3954125..2d0789e554 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'rebase -i --continue handles merge strategy and options' '\n-\trm -fr .git/rebase-* &&\n-\tgit reset --hard commit-new-file-F2-on-topic-branch &&\n-\ttest_commit \"commit-new-file-F3-on-topic-branch-for-dash-i\" F3 32 &&\n-\ttest_when_finished \"rm -fr test-bin funny.was.run funny.args\" &&\n-\tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\techo \"\\$@\" >>funny.args\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tcase \"\\$2\" in --foo) ;; *) exit 2 ;; esac\n-\tcase \"\\$4\" in --) ;; *) exit 2 ;; esac\n-\tshift 2 &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\ttest_must_fail git rebase -i -s funny -Xopt -Xfoo main topic\n-\t) &&\n-\ttest -f funny.was.run &&\n-\trm funny.was.run &&\n-\techo \"Resolved\" >F2 &&\n-\tgit add F2 &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\tgit rebase --continue\n-\t) &&\n-\ttest -f funny.was.run\n-'\n-\n test_expect_success 'rebase -r passes merge strategy options correctly' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard commit-new-file-F3-on-topic-branch &&\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474969","messageId":"CABPp-BF8ND=Q_ONb3=WV+4F8ZVJ6h+0FeVNOr_YV-7s02uJXSA@mail.gmail.com","threadId":"59400","inReplyTo":"dd7b82cdd54cc962385dff8a1adab4c1d4fb2167.1680708043.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-07T05:44:39Z","receivedAt":"2023-04-07T05:45:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> The sequencer stores the merge strategy options in an array of strings\n> which allocated with ALLOC_GROW(). Using \"struct strvec\" avoids manually\n> managing the memory of that array and simplifies the code.\n>\n> Aside from memory allocation the changes to the sequencer are largely\n> mechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A\n> new option parsing macro OPT_STRVEC() is also added to collect the\n> strategy options. Hopefully this can be used to simplify the code in\n> builtin/merge.c in the future.\n>\n> Note that there is a change of behavior to \"git cherry-pick\" and \"git\n> revert\" as passing “--no-strategy-option” will now clear any previous\n> strategy options whereas before this change it did nothing.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> builtin/revert.c | 20 +++-----------------\n> parse-options-cb.c | 16 ++++++++++++++++\n> parse-options.h | 10 ++++++++++\n> sequencer.c | 47 ++++++++++++++++++++--------------------------\n> sequencer.h | 11 ++++++++---\n> 5 files changed, 57 insertions(+), 47 deletions(-)\n>\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 62986a7b1b..362857da65 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n> return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n> }\n>\n> -static int option_parse_x(const struct option *opt,\n> - const char *arg, int unset)\n> -{\n> - struct replay_opts **opts_ptr = opt->value;\n> - struct replay_opts *opts = *opts_ptr;\n> -\n> - if (unset)\n> - return 0;\n> -\n> - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n> - opts->xopts[opts->xopts_nr++] = xstrdup(arg);\n> - return 0;\n> -}\n> -\n> static int option_parse_m(const struct option *opt,\n> const char *arg, int unset)\n> {\n> @@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> N_(\"select mainline parent\"), option_parse_m),\n> OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),\n> OPT_STRING(0, \"strategy\", &opts->strategy, N_(\"strategy\"), N_(\"merge strategy\")),\n> - OPT_CALLBACK('X', \"strategy-option\", &opts, N_(\"option\"),\n> - N_(\"option for merge strategy\"), option_parse_x),\n> + OPT_STRVEC('X', \"strategy-option\", &opts->xopts, N_(\"option\"),\n> + N_(\"option for merge strategy\")),\n> { OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n> N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> OPT_END()\n> @@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \"--signoff\", opts->signoff,\n> \"--mainline\", opts->mainline,\n> \"--strategy\", opts->strategy ? 1 : 0,\n> - \"--strategy-option\", opts->xopts ? 1 : 0,\n> + \"--strategy-option\", opts->xopts.nr ? 1 : 0,\n> \"-x\", opts->record_origin,\n> \"--ff\", opts->allow_ff,\n> \"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n> diff --git a/parse-options-cb.c b/parse-options-cb.c\n> index d346dbe210..8eb96c5ca9 100644\n> --- a/parse-options-cb.c\n> +++ b/parse-options-cb.c\n> @@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n> return 0;\n> }\n>\n> +int parse_opt_strvec(const struct option *opt, const char *arg, int unset)\n> +{\n> + struct strvec *v = opt->value;\n> +\n> + if (unset) {\n> + strvec_clear(v);\n> + return 0;\n> + }\n> +\n> + if (!arg)\n> + return -1;\n> +\n> + strvec_push(v, arg);\n> + return 0;\n> +}\n> +\n> int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)\n> {\n> return 0;\n> diff --git a/parse-options.h b/parse-options.h\n> index 26f19384e5..2d1d4d052f 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -285,6 +285,15 @@ struct option {\n> .help = (h), \\\n> .callback = &parse_opt_string_list, \\\n> }\n> +#define OPT_STRVEC(s, l, v, a, h) { \\\n> + .type = OPTION_CALLBACK, \\\n> + .short_name = (s), \\\n> + .long_name = (l), \\\n> + .value = (v), \\\n> + .argh = (a), \\\n> + .help = (h), \\\n> + .callback = &parse_opt_strvec, \\\n> +}\n> #define OPT_UYN(s, l, v, h) { \\\n> .type = OPTION_CALLBACK, \\\n> .short_name = (s), \\\n> @@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int);\n> int parse_opt_commit(const struct option *, const char *, int);\n> int parse_opt_tertiary(const struct option *, const char *, int);\n> int parse_opt_string_list(const struct option *, const char *, int);\n> +int parse_opt_strvec(const struct option *, const char *, int);\n> int parse_opt_noop_cb(const struct option *, const char *, int);\n> enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,\n> const struct option *,\n> diff --git a/sequencer.c b/sequencer.c\n> index c35a67e104..6e2f3357c8 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts)\n> free(opts->reflog_action);\n> free(opts->default_strategy);\n> free(opts->strategy);\n> - for (size_t i = 0; i < opts->xopts_nr; i++)\n> - free(opts->xopts[i]);\n> - free(opts->xopts);\n> + strvec_clear (&opts->xopts);\n> strbuf_release(&opts->current_fixups);\n> if (opts->revs)\n> release_revisions(opts->revs);\n> @@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r,\n> next_tree = next ? get_commit_tree(next) : empty_tree(r);\n> base_tree = base ? get_commit_tree(base) : empty_tree(r);\n>\n> - for (i = 0; i < opts->xopts_nr; i++)\n> - parse_merge_opt(&o, opts->xopts[i]);\n> + for (i = 0; i < opts->xopts.nr; i++)\n> + parse_merge_opt(&o, opts->xopts.v[i]);\n>\n> if (!opts->strategy || !strcmp(opts->strategy, \"ort\")) {\n> memset(&result, 0, sizeof(result));\n> @@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r,\n> commit_list_insert(base, &common);\n> commit_list_insert(next, &remotes);\n> res |= try_merge_command(r, opts->strategy,\n> - opts->xopts_nr, (const char **)opts->xopts,\n> + opts->xopts.nr, opts->xopts.v,\n> common, oid_to_hex(&head), remotes);\n> free_commit_list(common);\n> free_commit_list(remotes);\n> @@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n> else if (!strcmp(key, \"options.gpg-sign\"))\n> git_config_string_dup(&opts->gpg_sign, key, value);\n> else if (!strcmp(key, \"options.strategy-option\")) {\n> - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n> - opts->xopts[opts->xopts_nr++] = xstrdup(value);\n> + strvec_push(&opts->xopts, value);\n> } else if (!strcmp(key, \"options.allow-rerere-auto\"))\n> opts->allow_rerere_auto =\n> git_config_bool_or_int(key, value, &error_flag) ?\n> @@ -2920,22 +2917,21 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n> {\n> int i;\n> int count;\n> + const char **argv;\n> char *strategy_opts_string = raw_opts;\n>\n> if (*strategy_opts_string == ' ')\n> strategy_opts_string++;\n>\n> - count = split_cmdline(strategy_opts_string,\n> - (const char ***)&opts->xopts);\n> + count = split_cmdline(strategy_opts_string, &argv);\n> if (count < 0)\n> die(_(\"could not split '%s': %s\"), strategy_opts_string,\n> split_cmdline_strerror(count));\n> - opts->xopts_nr = count;\n> - for (i = 0; i < opts->xopts_nr; i++) {\n> - const char *arg = opts->xopts[i];\n> + for (i = 0; i < count; i++) {\n> + const char *arg = argv[i];\n>\n> skip_prefix(arg, \"--\", &arg);\n> - opts->xopts[i] = xstrdup(arg);\n> + strvec_push(&opts->xopts, arg);\n> }\n> }\n>\n> @@ -3055,8 +3051,8 @@ static void write_strategy_opts(struct replay_opts *opts)\n> int i;\n> struct strbuf buf = STRBUF_INIT;\n>\n> - for (i = 0; i < opts->xopts_nr; ++i)\n> - strbuf_addf(&buf, \" --%s\", opts->xopts[i]);\n> + for (i = 0; i < opts->xopts.nr; ++i)\n> + strbuf_addf(&buf, \" --%s\", opts->xopts.v[i]);\n>\n> write_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n> strbuf_release(&buf);\n> @@ -3080,7 +3076,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n> write_file(rebase_path_verbose(), \"%s\", \"\");\n> if (opts->strategy)\n> write_file(rebase_path_strategy(), \"%s\\n\", opts->strategy);\n> - if (opts->xopts_nr > 0)\n> + if (opts->xopts.nr > 0)\n> write_strategy_opts(opts);\n>\n> if (opts->allow_rerere_auto == RERERE_AUTOUPDATE)\n> @@ -3464,13 +3460,10 @@ static int save_opts(struct replay_opts *opts)\n> if (opts->gpg_sign)\n> res |= git_config_set_in_file_gently(opts_file,\n> \"options.gpg-sign\", opts->gpg_sign);\n> - if (opts->xopts) {\n> - int i;\n> - for (i = 0; i < opts->xopts_nr; i++)\n> - res |= git_config_set_multivar_in_file_gently(opts_file,\n> - \"options.strategy-option\",\n> - opts->xopts[i], \"^$\", 0);\n> - }\n> + for (size_t i = 0; i < opts->xopts.nr; i++)\n> + res |= git_config_set_multivar_in_file_gently(opts_file,\n> + \"options.strategy-option\",\n> + opts->xopts.v[i], \"^$\", 0);\n> if (opts->allow_rerere_auto)\n> res |= git_config_set_in_file_gently(opts_file,\n> \"options.allow-rerere-auto\",\n> @@ -3880,7 +3873,7 @@ static int do_merge(struct repository *r,\n> struct commit *head_commit, *merge_commit, *i;\n> struct commit_list *bases, *j;\n> struct commit_list *to_merge = NULL, **tail = &to_merge;\n> - const char *strategy = !opts->xopts_nr &&\n> + const char *strategy = !opts->xopts.nr &&\n> (!opts->strategy ||\n> !strcmp(opts->strategy, \"recursive\") ||\n> !strcmp(opts->strategy, \"ort\")) ?\n> @@ -4063,9 +4056,9 @@ static int do_merge(struct repository *r,\n> strvec_push(&cmd.args, \"octopus\");\n> else {\n> strvec_push(&cmd.args, strategy);\n> - for (k = 0; k < opts->xopts_nr; k++)\n> + for (k = 0; k < opts->xopts.nr; k++)\n> strvec_pushf(&cmd.args,\n> - \"-X%s\", opts->xopts[k]);\n> + \"-X%s\", opts->xopts.v[k]);\n> }\n> if (!(flags & TODO_EDIT_MERGE_MSG))\n> strvec_push(&cmd.args, \"--no-edit\");\n> diff --git a/sequencer.h b/sequencer.h\n> index 33dbaf5b66..8a79d6b200 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -2,6 +2,7 @@\n> #define SEQUENCER_H\n>\n> #include \"strbuf.h\"\n> +#include \"strvec.h\"\n> #include \"wt-status.h\"\n>\n> struct commit;\n> @@ -60,8 +61,7 @@ struct replay_opts {\n> /* Merge strategy */\n> char *default_strategy; /* from config options */\n> char *strategy;\n> - char **xopts;\n> - size_t xopts_nr, xopts_alloc;\n> + struct strvec xopts;\n>\n> /* Reflog */\n> char *reflog_action;\n> @@ -80,7 +80,12 @@ struct replay_opts {\n> /* Private use */\n> const char *reflog_message;\n> };\n> -#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n> +#define REPLAY_OPTS_INIT { \\\n> + .edit = -1, \\\n> + .action = -1, \\\n> + .current_fixups = STRBUF_INIT, \\\n> + .xopts = STRVEC_INIT, \\\n> +}\n>\n> /*\n> * Note that ordering matters in this enum. Not only must it match the mapping\n> --\n> 2.40.0.670.g64ef305212.dirty\n\nSimple, but very nice cleanup. :-)\n"},{"id":"474970","messageId":"CABPp-BGPyVZu1iOW5TLUE+tfU+uFs=UNNzFX+=DFY5D2Wt4-+w@mail.gmail.com","threadId":"59400","inReplyTo":"9a90212ef284510fa691b8eebd6bdd61098282fd.1680708043.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 4/5] rebase -m: fix serialization of strategy options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-07T05:47:42Z","receivedAt":"2023-04-07T05:48:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> To store the strategy options rebase prepends \" --\" to each one and\n> writes them to a file. To load them it reads the file and passes the\n> contents to split_cmdline(). This roughly mimics the behavior of the\n> scripted rebase but has a couple of limitations, (1) options containing\n> whitespace are not properly preserved (this is true of the scripted\n> rebase as well) and (2) options containing '\"' or '\\' are incorrectly\n> parsed and may cause the parser to return an error.\n>\n> Fix these limitations by quoting each option when they are stored so\n> that they can be parsed correctly. Now that \"--preserve-merges\" no\n> longer exist this change also stops prepending \"--\" to the options when\n> they are stored as that was an artifact of the scripted rebase.\n>\n> These changes are backwards compatible so the files written by an older\n> version of git can be still be read. They are also forwards compatible,\n\ns/be still be/still be/\n\nSorry for not noticing this last time.\n\n> the file can still be parsed by recent versions of git as they treat the\n> \"--\" prefix as optional.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> alias.c | 18 +++++++++++++++++\n> alias.h | 3 +++\n> sequencer.c | 11 ++++++-----\n> t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++------------\n> t/t3436-rebase-more-options.sh | 24 -----------------------\n> 5 files changed, 50 insertions(+), 42 deletions(-)\n>\n> diff --git a/alias.c b/alias.c\n> index e814948ced..54a1a23d2c 100644\n> --- a/alias.c\n> +++ b/alias.c\n> @@ -3,6 +3,7 @@\n> #include \"alloc.h\"\n> #include \"config.h\"\n> #include \"gettext.h\"\n> +#include \"strbuf.h\"\n> #include \"string-list.h\"\n>\n> struct config_alias_data {\n> @@ -46,6 +47,23 @@ void list_aliases(struct string_list *list)\n> read_early_config(config_alias_cb, &data);\n> }\n>\n> +void quote_cmdline(struct strbuf *buf, const char **argv)\n> +{\n> + for (const char **argp = argv; *argp; argp++) {\n> + if (argp != argv)\n> + strbuf_addch(buf, ' ');\n> + strbuf_addch(buf, '\"');\n> + for (const char *p = *argp; *p; p++) {\n> + const char c = *p;\n> +\n> + if (c == '\"' || c =='\\\\')\n> + strbuf_addch(buf, '\\\\');\n> + strbuf_addch(buf, c);\n> + }\n> + strbuf_addch(buf, '\"');\n> + }\n> +}\n> +\n> #define SPLIT_CMDLINE_BAD_ENDING 1\n> #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2\n> #define SPLIT_CMDLINE_ARGC_OVERFLOW 3\n> diff --git a/alias.h b/alias.h\n> index aef4843bb7..43db736484 100644\n> --- a/alias.h\n> +++ b/alias.h\n> @@ -1,9 +1,12 @@\n> #ifndef ALIAS_H\n> #define ALIAS_H\n>\n> +struct strbuf;\n> struct string_list;\n>\n> char *alias_lookup(const char *alias);\n> +/* Quote argv so buf can be parsed by split_cmdline() */\n> +void quote_cmdline(struct strbuf *buf, const char **argv);\n> int split_cmdline(char *cmdline, const char ***argv);\n> /* Takes a negative value returned by split_cmdline */\n> const char *split_cmdline_strerror(int cmdline_errno);\n> diff --git a/sequencer.c b/sequencer.c\n> index 045d549042..9907100c8a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n>\n> count = split_cmdline(strategy_opts_string, &argv);\n> if (count < 0)\n> - die(_(\"could not split '%s': %s\"), strategy_opts_string,\n> + BUG(\"could not split '%s': %s\", strategy_opts_string,\n> split_cmdline_strerror(count));\n> for (i = 0; i < count; i++) {\n> const char *arg = argv[i];\n> @@ -3048,12 +3048,13 @@ static int read_populate_opts(struct replay_opts *opts)\n>\n> static void write_strategy_opts(struct replay_opts *opts)\n> {\n> - int i;\n> struct strbuf buf = STRBUF_INIT;\n>\n> - for (i = 0; i < opts->xopts.nr; ++i)\n> - strbuf_addf(&buf, \" --%s\", opts->xopts.v[i]);\n> -\n> + /*\n> + * Quote strategy options so that they can be read correctly\n> + * by split_cmdline().\n> + */\n> + quote_cmdline(&buf, opts->xopts.v);\n> write_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n> strbuf_release(&buf);\n> }\n> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n> index 130e2f9b55..42c3954125 100755\n> --- a/t/t3418-rebase-continue.sh\n> +++ b/t/t3418-rebase-continue.sh\n> @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n> rm -fr .git/rebase-* &&\n> git reset --hard commit-new-file-F2-on-topic-branch &&\n> test_commit \"commit-new-file-F3-on-topic-branch\" F3 32 &&\n> - test_when_finished \"rm -fr test-bin funny.was.run\" &&\n> + test_when_finished \"rm -fr test-bin\" &&\n> mkdir test-bin &&\n> - cat >test-bin/git-merge-funny <<-EOF &&\n> - #!$SHELL_PATH\n> - case \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n> - shift &&\n> - >funny.was.run &&\n> - exec git merge-recursive \"\\$@\"\n> - EOF\n> - chmod +x test-bin/git-merge-funny &&\n> +\n> + write_script test-bin/git-merge-funny <<-\\EOF &&\n> + printf \"[%s]\\n\" $# \"$1\" \"$2\" \"$3\" \"$5\" >actual\n> + shift 3 &&\n> + exec git merge-recursive \"$@\"\n> + EOF\n> +\n> + cat >expect <<-\\EOF &&\n> + [7]\n> + [--option=arg with space]\n> + [--op\"tion\\]\n> + [--new\n> + line ]\n> + [--]\n> + EOF\n> +\n> + rm -f actual &&\n> (\n> PATH=./test-bin:$PATH &&\n> - test_must_fail git rebase -s funny -Xopt main topic\n> + test_must_fail git rebase -s funny -X\"option=arg with space\" \\\n> + -Xop\\\"tion\\\\ -X\"new${LF}line \" main topic\n> ) &&\n> - test -f funny.was.run &&\n> - rm funny.was.run &&\n> + test_cmp expect actual &&\n> + rm actual &&\n> echo \"Resolved\" >F2 &&\n> git add F2 &&\n> (\n> PATH=./test-bin:$PATH &&\n> git rebase --continue\n> ) &&\n> - test -f funny.was.run\n> + test_cmp expect actual\n> '\n>\n> test_expect_success 'rebase -i --continue handles merge strategy and options' '\n> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\n> index 3adf42f47d..94671d3c46 100755\n> --- a/t/t3436-rebase-more-options.sh\n> +++ b/t/t3436-rebase-more-options.sh\n> @@ -40,30 +40,6 @@ test_expect_success 'setup' '\n> EOF\n> '\n>\n> -test_expect_success 'bad -X arguments: unclosed quote' '\n> - test_when_finished \"test_might_fail git rebase --abort\" &&\n> - cat >expect <<-\\EOF &&\n> - fatal: could not split '\\''--bad'\\'': unclosed quote\n> - EOF\n> - GIT_SEQUENCE_EDITOR=\"echo break >\" \\\n> - git rebase -i -X\"bad argument\\\"\" side main &&\n> - test_expect_code 128 git rebase --continue >out 2>actual &&\n> - test_must_be_empty out &&\n> - test_cmp expect actual\n> -'\n> -\n> -test_expect_success 'bad -X arguments: bad escape' '\n> - test_when_finished \"test_might_fail git rebase --abort\" &&\n> - cat >expect <<-\\EOF &&\n> - fatal: could not split '\\''--bad'\\'': cmdline ends with \\\n> - EOF\n> - GIT_SEQUENCE_EDITOR=\"echo break >\" \\\n> - git rebase -i -X\"bad escape \\\\\" side main &&\n> - test_expect_code 128 git rebase --continue >out 2>actual &&\n> - test_must_be_empty out &&\n> - test_cmp expect actual\n> -'\n> -\n> test_expect_success '--ignore-whitespace works with apply backend' '\n> test_must_fail git rebase --apply main side &&\n> git rebase --abort &&\n> --\n> 2.40.0.670.g64ef305212.dirty\n\nLooks good.\n"},{"id":"474971","messageId":"CABPp-BFMiU8cYzUr1-m6P8w-YAtqPRyMfah8UAqdO_N4cdxUKg@mail.gmail.com","threadId":"59400","inReplyTo":"cover.1680708043.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 0/5] rebase: cleanup merge strategy option handling","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-07T05:49:38Z","receivedAt":"2023-04-07T05:49:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> Cleanup the handling of --strategy-option now that we no longer need to\n> support \"--preserve-merges\" and properly quote the argument when saving\n> it to disc.\n>\n> Thanks to Elijah for his comments on V1.\n>\n> Changes since V1:\n>\n> I've rebased these patches onto 'master' to avoid conflicts with\n> 'sg/parse-options-h-initializers' in the new patch 2 (this series\n> depends on 'ab/fix-strategy-opts-parsing' but that has now been merged\n> to master).\n>\n> Patch 1 - Unchanged.\n>\n> Patch 2 - New patch to store the merge strategy options in an \"struct\n> strvec\". This patch also introduces a new macro OPT_STRVEC()\n> to collect options into an \"struct strvec\".\n>\n> Patch 3 - Small simplification due to the changes in patch 2.\n>\n> Patch 4 - Moved the code to quote a list so it can split by\n> split_cmdline() into a new function quote_cmdline() as\n> suggested by Elijah.\n>\n> Patch 5 - Reworded the commit message as suggested by Elijah.\n\nI noticed a small typo/grammo in the commit message of Patch 4 that I\nmissed in V1, but otherwise this series looks good. I'm not sure\nfixing the tiny grammo is even all that important; I'll leave it up to\nyou for whether you want to bother re-rolling to fix it. Either way:\n\nReviewed-by: Elijah Newren \n"},{"id":"474993","messageId":"cover.1680875701.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 0/5] rebase: cleanup merge strategy option handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-07T13:55:30Z","receivedAt":"2023-04-07T13:57:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCleanup the handling of --strategy-option now that we no longer need to\nsupport \"--preserve-merges\" and properly quote the argument when saving\nit to disc.\n\nThanks to Elijah for his comments.\n\nChanges since V2:\n\n - Added Elijah's Reviewed-by: trailer.\n - Fixed a typo in patch 4 spotted by Elijah.\n\nChanges since V1:\n\nI've rebased these patches onto 'master' to avoid conflicts with\n'sg/parse-options-h-initializers' in the new patch 2 (this series\ndepends on 'ab/fix-strategy-opts-parsing' but that has now been merged\nto master).\n\nPatch 1 - Unchanged.\n\nPatch 2 - New patch to store the merge strategy options in an \"struct\n strvec\". This patch also introduces a new macro OPT_STRVEC()\n to collect options into an \"struct strvec\".\n\nPatch 3 - Small simplification due to the changes in patch 2.\n\nPatch 4 - Moved the code to quote a list so it can split by\n split_cmdline() into a new function quote_cmdline() as\n\t suggested by Elijah.\n\nPatch 5 - Reworded the commit message as suggested by Elijah.\n\n\nBase-Commit: 140b9478dad5d19543c1cb4fd293ccec228f1240\nPublished-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv3\nView-Changes-At: https://github.com/phillipwood/git/compare/140b9478d...2d1d32811\nFetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v3\n\nPhillip Wood (5):\n rebase: stop reading and writing unnecessary strategy state\n sequencer: use struct strvec to store merge strategy options\n rebase -m: cleanup --strategy-option handling\n rebase -m: fix serialization of strategy options\n rebase: remove a couple of redundant strategy tests\n\n alias.c | 18 +++++++\n alias.h | 3 ++\n builtin/rebase.c | 54 ++++-----------------\n builtin/revert.c | 20 ++------\n parse-options-cb.c | 16 +++++++\n parse-options.h | 10 ++++\n sequencer.c | 57 ++++++++++------------\n sequencer.h | 12 +++--\n t/t3402-rebase-merge.sh | 21 --------\n t/t3418-rebase-continue.sh | 88 +++++++++++++---------------------\n t/t3436-rebase-more-options.sh | 18 -------\n 11 files changed, 126 insertions(+), 191 deletions(-)\n\nRange-diff against v2:\n1: 2353c753f5 ! 1: 882b403423 rebase: stop reading and writing unnecessary strategy state\n @@ Commit message\n writing of state files between builtin/rebase.c and sequencer.c but that\n is left for a follow up series.\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Phillip Wood \n \n ## builtin/rebase.c ##\n2: dd7b82cdd5 ! 2: 1d8e59aa16 sequencer: use struct strvec to store merge strategy options\n @@ Commit message\n revert\" as passing “--no-strategy-option” will now clear any previous\n strategy options whereas before this change it did nothing.\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Phillip Wood \n \n ## builtin/revert.c ##\n3: ccef0e6f4b ! 3: e98ef5ce8c rebase -m: cleanup --strategy-option handling\n @@ Commit message\n mixed blessing but the next commit will fix the root cause of the\n parsing problem so lets not worry about that here.\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Phillip Wood \n \n ## builtin/rebase.c ##\n4: 9a90212ef2 ! 4: a5e940e2d0 rebase -m: fix serialization of strategy options\n @@ Commit message\n they are stored as that was an artifact of the scripted rebase.\n \n These changes are backwards compatible so the files written by an older\n - version of git can be still be read. They are also forwards compatible,\n + version of git can still be read. They are also forwards compatible,\n the file can still be parsed by recent versions of git as they treat the\n \"--\" prefix as optional.\n \n + Reviewed-by: Elijah Newren \n Signed-off-by: Phillip Wood \n \n ## alias.c ##\n5: 3515c31b40 ! 5: 2d1d328110 rebase: remove a couple of redundant strategy tests\n @@ Commit message\n preceding test.\n \n Helped-by: Elijah Newren \n + Reviewed-by: Elijah Newren \n Signed-off-by: Phillip Wood \n \n ## t/t3402-rebase-merge.sh ##\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474994","messageId":"882b403423e5c2d90c3afc4eefb02bb499c4a02b.1680875701.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680875701.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 1/5] rebase: stop reading and writing unnecessary strategy state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-07T13:55:31Z","receivedAt":"2023-04-07T13:57:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe state files for \"--strategy\" and \"--strategy-option\" are written and\nread twice, once by builtin/rebase.c and then by sequencer.c. This is an\nartifact of the scripted rebase and the need to support \"rebase\n--preserve-merges\". Now that \"--preserve-merges\" no-longer exists we\nonly need to read and write these files in sequencer.c. This enables us\nto remove a call to free() in read_strategy_opts() that was added by\nf1f4ebf432 (sequencer.c: fix \"opts->strategy\" leak in\nread_strategy_opts(), 2022-11-08) as this commit fixes the root cause of\nthat leak.\n\nThere is further scope for removing duplication in the reading and\nwriting of state files between builtin/rebase.c and sequencer.c but that\nis left for a follow up series.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 24 ------------------------\n sequencer.c | 1 -\n 2 files changed, 25 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5b7b908b66..3bd215c771 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -483,24 +483,6 @@ static int read_basic_state(struct rebase_options *opts)\n \t\topts->gpg_sign_opt = xstrdup(buf.buf);\n \t}\n \n-\tif (file_exists(state_dir_path(\"strategy\", opts))) {\n-\t\tstrbuf_reset(&buf);\n-\t\tif (!read_oneliner(&buf, state_dir_path(\"strategy\", opts),\n-\t\t\t\t READ_ONELINER_WARN_MISSING))\n-\t\t\treturn -1;\n-\t\tfree(opts->strategy);\n-\t\topts->strategy = xstrdup(buf.buf);\n-\t}\n-\n-\tif (file_exists(state_dir_path(\"strategy_opts\", opts))) {\n-\t\tstrbuf_reset(&buf);\n-\t\tif (!read_oneliner(&buf, state_dir_path(\"strategy_opts\", opts),\n-\t\t\t\t READ_ONELINER_WARN_MISSING))\n-\t\t\treturn -1;\n-\t\tfree(opts->strategy_opts);\n-\t\topts->strategy_opts = xstrdup(buf.buf);\n-\t}\n-\n \tstrbuf_release(&buf);\n \n \treturn 0;\n@@ -518,12 +500,6 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n \t\twrite_file(state_dir_path(\"quiet\", opts), \"%s\", \"\");\n \tif (opts->flags & REBASE_VERBOSE)\n \t\twrite_file(state_dir_path(\"verbose\", opts), \"%s\", \"\");\n-\tif (opts->strategy)\n-\t\twrite_file(state_dir_path(\"strategy\", opts), \"%s\",\n-\t\t\t opts->strategy);\n-\tif (opts->strategy_opts)\n-\t\twrite_file(state_dir_path(\"strategy_opts\", opts), \"%s\",\n-\t\t\t opts->strategy_opts);\n \tif (opts->allow_rerere_autoupdate > 0)\n \t\twrite_file(state_dir_path(\"allow_rerere_autoupdate\", opts),\n \t\t\t \"-%s-rerere-autoupdate\",\ndiff --git a/sequencer.c b/sequencer.c\nindex 3be23d7ca2..c35a67e104 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2944,7 +2944,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)\n \tstrbuf_reset(buf);\n \tif (!read_oneliner(buf, rebase_path_strategy(), 0))\n \t\treturn;\n-\tfree(opts->strategy);\n \topts->strategy = strbuf_detach(buf, NULL);\n \tif (!read_oneliner(buf, rebase_path_strategy_opts(), 0))\n \t\treturn;\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474995","messageId":"1d8e59aa1611daf7a0dfcd67ce055157588ecdc4.1680875701.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680875701.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 2/5] sequencer: use struct strvec to store merge strategy options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-07T13:55:32Z","receivedAt":"2023-04-07T13:57:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe sequencer stores the merge strategy options in an array of strings\nwhich allocated with ALLOC_GROW(). Using \"struct strvec\" avoids manually\nmanaging the memory of that array and simplifies the code.\n\nAside from memory allocation the changes to the sequencer are largely\nmechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A\nnew option parsing macro OPT_STRVEC() is also added to collect the\nstrategy options. Hopefully this can be used to simplify the code in\nbuiltin/merge.c in the future.\n\nNote that there is a change of behavior to \"git cherry-pick\" and \"git\nrevert\" as passing “--no-strategy-option” will now clear any previous\nstrategy options whereas before this change it did nothing.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n builtin/revert.c | 20 +++-----------------\n parse-options-cb.c | 16 ++++++++++++++++\n parse-options.h | 10 ++++++++++\n sequencer.c | 47 ++++++++++++++++++++--------------------------\n sequencer.h | 11 ++++++++---\n 5 files changed, 57 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 62986a7b1b..362857da65 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n \treturn opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n }\n \n-static int option_parse_x(const struct option *opt,\n-\t\t\t const char *arg, int unset)\n-{\n-\tstruct replay_opts **opts_ptr = opt->value;\n-\tstruct replay_opts *opts = *opts_ptr;\n-\n-\tif (unset)\n-\t\treturn 0;\n-\n-\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n-\topts->xopts[opts->xopts_nr++] = xstrdup(arg);\n-\treturn 0;\n-}\n-\n static int option_parse_m(const struct option *opt,\n \t\t\t const char *arg, int unset)\n {\n@@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t N_(\"select mainline parent\"), option_parse_m),\n \t\tOPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),\n \t\tOPT_STRING(0, \"strategy\", &opts->strategy, N_(\"strategy\"), N_(\"merge strategy\")),\n-\t\tOPT_CALLBACK('X', \"strategy-option\", &opts, N_(\"option\"),\n-\t\t\tN_(\"option for merge strategy\"), option_parse_x),\n+\t\tOPT_STRVEC('X', \"strategy-option\", &opts->xopts, N_(\"option\"),\n+\t\t\tN_(\"option for merge strategy\")),\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \t\tOPT_END()\n@@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"--signoff\", opts->signoff,\n \t\t\t\t\"--mainline\", opts->mainline,\n \t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n-\t\t\t\t\"--strategy-option\", opts->xopts ? 1 : 0,\n+\t\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n \t\t\t\t\"-x\", opts->record_origin,\n \t\t\t\t\"--ff\", opts->allow_ff,\n \t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex d346dbe210..8eb96c5ca9 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+int parse_opt_strvec(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct strvec *v = opt->value;\n+\n+\tif (unset) {\n+\t\tstrvec_clear(v);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!arg)\n+\t\treturn -1;\n+\n+\tstrvec_push(v, arg);\n+\treturn 0;\n+}\n+\n int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)\n {\n \treturn 0;\ndiff --git a/parse-options.h b/parse-options.h\nindex 26f19384e5..2d1d4d052f 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -285,6 +285,15 @@ struct option {\n \t.help = (h), \\\n \t.callback = &parse_opt_string_list, \\\n }\n+#define OPT_STRVEC(s, l, v, a, h) { \\\n+\t.type = OPTION_CALLBACK, \\\n+\t.short_name = (s), \\\n+\t.long_name = (l), \\\n+\t.value = (v), \\\n+\t.argh = (a), \\\n+\t.help = (h), \\\n+\t.callback = &parse_opt_strvec, \\\n+}\n #define OPT_UYN(s, l, v, h) { \\\n \t.type = OPTION_CALLBACK, \\\n \t.short_name = (s), \\\n@@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int);\n int parse_opt_commit(const struct option *, const char *, int);\n int parse_opt_tertiary(const struct option *, const char *, int);\n int parse_opt_string_list(const struct option *, const char *, int);\n+int parse_opt_strvec(const struct option *, const char *, int);\n int parse_opt_noop_cb(const struct option *, const char *, int);\n enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t const struct option *,\ndiff --git a/sequencer.c b/sequencer.c\nindex c35a67e104..6e2f3357c8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts)\n \tfree(opts->reflog_action);\n \tfree(opts->default_strategy);\n \tfree(opts->strategy);\n-\tfor (size_t i = 0; i < opts->xopts_nr; i++)\n-\t\tfree(opts->xopts[i]);\n-\tfree(opts->xopts);\n+\tstrvec_clear (&opts->xopts);\n \tstrbuf_release(&opts->current_fixups);\n \tif (opts->revs)\n \t\trelease_revisions(opts->revs);\n@@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r,\n \tnext_tree = next ? get_commit_tree(next) : empty_tree(r);\n \tbase_tree = base ? get_commit_tree(base) : empty_tree(r);\n \n-\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\tparse_merge_opt(&o, opts->xopts[i]);\n+\tfor (i = 0; i < opts->xopts.nr; i++)\n+\t\tparse_merge_opt(&o, opts->xopts.v[i]);\n \n \tif (!opts->strategy || !strcmp(opts->strategy, \"ort\")) {\n \t\tmemset(&result, 0, sizeof(result));\n@@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r,\n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n \t\tres |= try_merge_command(r, opts->strategy,\n-\t\t\t\t\t opts->xopts_nr, (const char **)opts->xopts,\n+\t\t\t\t\t opts->xopts.nr, opts->xopts.v,\n \t\t\t\t\tcommon, oid_to_hex(&head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n@@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \telse if (!strcmp(key, \"options.gpg-sign\"))\n \t\tgit_config_string_dup(&opts->gpg_sign, key, value);\n \telse if (!strcmp(key, \"options.strategy-option\")) {\n-\t\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n-\t\topts->xopts[opts->xopts_nr++] = xstrdup(value);\n+\t\tstrvec_push(&opts->xopts, value);\n \t} else if (!strcmp(key, \"options.allow-rerere-auto\"))\n \t\topts->allow_rerere_auto =\n \t\t\tgit_config_bool_or_int(key, value, &error_flag) ?\n@@ -2920,22 +2917,21 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n {\n \tint i;\n \tint count;\n+\tconst char **argv;\n \tchar *strategy_opts_string = raw_opts;\n \n \tif (*strategy_opts_string == ' ')\n \t\tstrategy_opts_string++;\n \n-\tcount = split_cmdline(strategy_opts_string,\n-\t\t\t (const char ***)&opts->xopts);\n+\tcount = split_cmdline(strategy_opts_string, &argv);\n \tif (count < 0)\n \t\tdie(_(\"could not split '%s': %s\"), strategy_opts_string,\n \t\t\t split_cmdline_strerror(count));\n-\topts->xopts_nr = count;\n-\tfor (i = 0; i < opts->xopts_nr; i++) {\n-\t\tconst char *arg = opts->xopts[i];\n+\tfor (i = 0; i < count; i++) {\n+\t\tconst char *arg = argv[i];\n \n \t\tskip_prefix(arg, \"--\", &arg);\n-\t\topts->xopts[i] = xstrdup(arg);\n+\t\tstrvec_push(&opts->xopts, arg);\n \t}\n }\n \n@@ -3055,8 +3051,8 @@ static void write_strategy_opts(struct replay_opts *opts)\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfor (i = 0; i < opts->xopts_nr; ++i)\n-\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts[i]);\n+\tfor (i = 0; i < opts->xopts.nr; ++i)\n+\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts.v[i]);\n \n \twrite_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n@@ -3080,7 +3076,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_verbose(), \"%s\", \"\");\n \tif (opts->strategy)\n \t\twrite_file(rebase_path_strategy(), \"%s\\n\", opts->strategy);\n-\tif (opts->xopts_nr > 0)\n+\tif (opts->xopts.nr > 0)\n \t\twrite_strategy_opts(opts);\n \n \tif (opts->allow_rerere_auto == RERERE_AUTOUPDATE)\n@@ -3464,13 +3460,10 @@ static int save_opts(struct replay_opts *opts)\n \tif (opts->gpg_sign)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.gpg-sign\", opts->gpg_sign);\n-\tif (opts->xopts) {\n-\t\tint i;\n-\t\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.strategy-option\",\n-\t\t\t\t\topts->xopts[i], \"^$\", 0);\n-\t}\n+\tfor (size_t i = 0; i < opts->xopts.nr; i++)\n+\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n+\t\t\t\t\"options.strategy-option\",\n+\t\t\t\topts->xopts.v[i], \"^$\", 0);\n \tif (opts->allow_rerere_auto)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\"options.allow-rerere-auto\",\n@@ -3880,7 +3873,7 @@ static int do_merge(struct repository *r,\n \tstruct commit *head_commit, *merge_commit, *i;\n \tstruct commit_list *bases, *j;\n \tstruct commit_list *to_merge = NULL, **tail = &to_merge;\n-\tconst char *strategy = !opts->xopts_nr &&\n+\tconst char *strategy = !opts->xopts.nr &&\n \t\t(!opts->strategy ||\n \t\t !strcmp(opts->strategy, \"recursive\") ||\n \t\t !strcmp(opts->strategy, \"ort\")) ?\n@@ -4063,9 +4056,9 @@ static int do_merge(struct repository *r,\n \t\t\tstrvec_push(&cmd.args, \"octopus\");\n \t\telse {\n \t\t\tstrvec_push(&cmd.args, strategy);\n-\t\t\tfor (k = 0; k < opts->xopts_nr; k++)\n+\t\t\tfor (k = 0; k < opts->xopts.nr; k++)\n \t\t\t\tstrvec_pushf(&cmd.args,\n-\t\t\t\t\t \"-X%s\", opts->xopts[k]);\n+\t\t\t\t\t \"-X%s\", opts->xopts.v[k]);\n \t\t}\n \t\tif (!(flags & TODO_EDIT_MERGE_MSG))\n \t\t\tstrvec_push(&cmd.args, \"--no-edit\");\ndiff --git a/sequencer.h b/sequencer.h\nindex 33dbaf5b66..8a79d6b200 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -2,6 +2,7 @@\n #define SEQUENCER_H\n \n #include \"strbuf.h\"\n+#include \"strvec.h\"\n #include \"wt-status.h\"\n \n struct commit;\n@@ -60,8 +61,7 @@ struct replay_opts {\n \t/* Merge strategy */\n \tchar *default_strategy; /* from config options */\n \tchar *strategy;\n-\tchar **xopts;\n-\tsize_t xopts_nr, xopts_alloc;\n+\tstruct strvec xopts;\n \n \t/* Reflog */\n \tchar *reflog_action;\n@@ -80,7 +80,12 @@ struct replay_opts {\n \t/* Private use */\n \tconst char *reflog_message;\n };\n-#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n+#define REPLAY_OPTS_INIT {\t\t\t\\\n+\t.edit = -1,\t\t\t\t\\\n+\t.action = -1,\t\t\t\t\\\n+\t.current_fixups = STRBUF_INIT,\t\t\\\n+\t.xopts = STRVEC_INIT,\t\t\t\\\n+}\n \n /*\n * Note that ordering matters in this enum. Not only must it match the mapping\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474996","messageId":"e98ef5ce8cb5245d432cc88f72aa9a7b160077e3.1680875701.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680875701.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 3/5] rebase -m: cleanup --strategy-option handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-07T13:55:33Z","receivedAt":"2023-04-07T13:57:12Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen handling \"--strategy-option\" rebase collects the commands into a\nstruct string_list, then concatenates them into a string, prepending \"--\"\nto each one before splitting the string and removing the \"--\" prefix.\nThis is an artifact of the scripted rebase and the need to support\n\"rebase --preserve-merges\". Now that \"--preserve-merges\" no-longer\nexists we can cleanup the way the argument is handled.\n\nThe tests for a bad strategy option are adjusted now that\nparse_strategy_opts() is no-longer called when starting a rebase. The\nfact that it only errors out when running \"git rebase --continue\" is a\nmixed blessing but the next commit will fix the root cause of the\nparsing problem so lets not worry about that here.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 30 ++++++++++--------------------\n sequencer.c | 2 +-\n sequencer.h | 1 -\n t/t3436-rebase-more-options.sh | 10 ++++++++--\n 4 files changed, 19 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 3bd215c771..511922c6fc 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -117,7 +117,8 @@ struct rebase_options {\n \tstruct string_list exec;\n \tint allow_empty_message;\n \tint rebase_merges, rebase_cousins;\n-\tchar *strategy, *strategy_opts;\n+\tchar *strategy;\n+\tstruct string_list strategy_opts;\n \tstruct strbuf git_format_patch_opt;\n \tint reschedule_failed_exec;\n \tint reapply_cherry_picks;\n@@ -143,6 +144,7 @@ struct rebase_options {\n \t\t.config_autosquash = -1, \\\n \t\t.update_refs = -1, \\\n \t\t.config_update_refs = -1, \\\n+\t\t.strategy_opts = STRING_LIST_INIT_NODUP,\\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -176,8 +178,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\treplay.default_strategy = NULL;\n \t}\n \n-\tif (opts->strategy_opts)\n-\t\tparse_strategy_opts(&replay, opts->strategy_opts);\n+\tfor (size_t i = 0; i < opts->strategy_opts.nr; i++)\n+\t\tstrvec_push(&replay.xopts, opts->strategy_opts.items[i].string);\n \n \tif (opts->squash_onto) {\n \t\toidcpy(&replay.squash_onto, opts->squash_onto);\n@@ -1013,7 +1015,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tint ignore_whitespace = 0;\n \tconst char *gpg_sign = NULL;\n \tconst char *rebase_merges = NULL;\n-\tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n \tchar *squash_onto_name = NULL;\n \tchar *keep_base_onto_name = NULL;\n@@ -1122,7 +1123,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"use 'merge-base --fork-point' to refine upstream\")),\n \t\tOPT_STRING('s', \"strategy\", &options.strategy,\n \t\t\t N_(\"strategy\"), N_(\"use the given merge strategy\")),\n-\t\tOPT_STRING_LIST('X', \"strategy-option\", &strategy_options,\n+\t\tOPT_STRING_LIST('X', \"strategy-option\", &options.strategy_opts,\n \t\t\t\tN_(\"option\"),\n \t\t\t\tN_(\"pass the argument through to the merge \"\n \t\t\t\t \"strategy\")),\n@@ -1436,23 +1437,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t} else {\n \t\t/* REBASE_MERGE */\n \t\tif (ignore_whitespace) {\n-\t\t\tstring_list_append(&strategy_options,\n+\t\t\tstring_list_append(&options.strategy_opts,\n \t\t\t\t\t \"ignore-space-change\");\n \t\t}\n \t}\n \n-\tif (strategy_options.nr) {\n-\t\tint i;\n-\n-\t\tif (!options.strategy)\n-\t\t\toptions.strategy = \"ort\";\n-\n-\t\tstrbuf_reset(&buf);\n-\t\tfor (i = 0; i < strategy_options.nr; i++)\n-\t\t\tstrbuf_addf(&buf, \" --%s\",\n-\t\t\t\t strategy_options.items[i].string);\n-\t\toptions.strategy_opts = xstrdup(buf.buf);\n-\t}\n+\tif (options.strategy_opts.nr && !options.strategy)\n+\t\toptions.strategy = \"ort\";\n \n \tif (options.strategy) {\n \t\toptions.strategy = xstrdup(options.strategy);\n@@ -1827,10 +1818,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tfree(options.gpg_sign_opt);\n \tstring_list_clear(&options.exec, 0);\n \tfree(options.strategy);\n-\tfree(options.strategy_opts);\n+\tstring_list_clear(&options.strategy_opts, 0);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n \tfree(keep_base_onto_name);\n-\tstring_list_clear(&strategy_options, 0);\n \treturn !!ret;\n }\ndiff --git a/sequencer.c b/sequencer.c\nindex 6e2f3357c8..045d549042 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2913,7 +2913,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \treturn 0;\n }\n \n-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n+static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n {\n \tint i;\n \tint count;\ndiff --git a/sequencer.h b/sequencer.h\nindex 8a79d6b200..913a0f652d 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -252,7 +252,6 @@ int read_oneliner(struct strbuf *buf,\n \tconst char *path, unsigned flags);\n int read_author_script(const char *path, char **name, char **email, char **date,\n \t\t int allow_missing);\n-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\t struct commit *onto, const struct object_id *orig_head);\n void sequencer_post_commit_cleanup(struct repository *r, int verbose);\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex c3184c9ade..3adf42f47d 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -41,19 +41,25 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'bad -X arguments: unclosed quote' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tcat >expect <<-\\EOF &&\n \tfatal: could not split '\\''--bad'\\'': unclosed quote\n \tEOF\n-\ttest_expect_code 128 git rebase -X\"bad argument\\\"\" side main >out 2>actual &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n+\t\tgit rebase -i -X\"bad argument\\\"\" side main &&\n+\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n \ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'bad -X arguments: bad escape' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tcat >expect <<-\\EOF &&\n \tfatal: could not split '\\''--bad'\\'': cmdline ends with \\\n \tEOF\n-\ttest_expect_code 128 git rebase -X\"bad escape \\\\\" side main >out 2>actual &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n+\t\tgit rebase -i -X\"bad escape \\\\\" side main &&\n+\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n \ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474997","messageId":"a5e940e2d072f2becee3be00df7799470047a762.1680875701.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680875701.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 4/5] rebase -m: fix serialization of strategy options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-07T13:55:34Z","receivedAt":"2023-04-07T13:57:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nTo store the strategy options rebase prepends \" --\" to each one and\nwrites them to a file. To load them it reads the file and passes the\ncontents to split_cmdline(). This roughly mimics the behavior of the\nscripted rebase but has a couple of limitations, (1) options containing\nwhitespace are not properly preserved (this is true of the scripted\nrebase as well) and (2) options containing '\"' or '\\' are incorrectly\nparsed and may cause the parser to return an error.\n\nFix these limitations by quoting each option when they are stored so\nthat they can be parsed correctly. Now that \"--preserve-merges\" no\nlonger exist this change also stops prepending \"--\" to the options when\nthey are stored as that was an artifact of the scripted rebase.\n\nThese changes are backwards compatible so the files written by an older\nversion of git can still be read. They are also forwards compatible,\nthe file can still be parsed by recent versions of git as they treat the\n\"--\" prefix as optional.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n alias.c | 18 +++++++++++++++++\n alias.h | 3 +++\n sequencer.c | 11 ++++++-----\n t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++------------\n t/t3436-rebase-more-options.sh | 24 -----------------------\n 5 files changed, 50 insertions(+), 42 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex e814948ced..54a1a23d2c 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -3,6 +3,7 @@\n #include \"alloc.h\"\n #include \"config.h\"\n #include \"gettext.h\"\n+#include \"strbuf.h\"\n #include \"string-list.h\"\n \n struct config_alias_data {\n@@ -46,6 +47,23 @@ void list_aliases(struct string_list *list)\n \tread_early_config(config_alias_cb, &data);\n }\n \n+void quote_cmdline(struct strbuf *buf, const char **argv)\n+{\n+\tfor (const char **argp = argv; *argp; argp++) {\n+\t\tif (argp != argv)\n+\t\t\tstrbuf_addch(buf, ' ');\n+\t\tstrbuf_addch(buf, '\"');\n+\t\tfor (const char *p = *argp; *p; p++) {\n+\t\t\tconst char c = *p;\n+\n+\t\t\tif (c == '\"' || c =='\\\\')\n+\t\t\t\tstrbuf_addch(buf, '\\\\');\n+\t\t\tstrbuf_addch(buf, c);\n+\t\t}\n+\t\tstrbuf_addch(buf, '\"');\n+\t}\n+}\n+\n #define SPLIT_CMDLINE_BAD_ENDING 1\n #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2\n #define SPLIT_CMDLINE_ARGC_OVERFLOW 3\ndiff --git a/alias.h b/alias.h\nindex aef4843bb7..43db736484 100644\n--- a/alias.h\n+++ b/alias.h\n@@ -1,9 +1,12 @@\n #ifndef ALIAS_H\n #define ALIAS_H\n \n+struct strbuf;\n struct string_list;\n \n char *alias_lookup(const char *alias);\n+/* Quote argv so buf can be parsed by split_cmdline() */\n+void quote_cmdline(struct strbuf *buf, const char **argv);\n int split_cmdline(char *cmdline, const char ***argv);\n /* Takes a negative value returned by split_cmdline */\n const char *split_cmdline_strerror(int cmdline_errno);\ndiff --git a/sequencer.c b/sequencer.c\nindex 045d549042..9907100c8a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n \n \tcount = split_cmdline(strategy_opts_string, &argv);\n \tif (count < 0)\n-\t\tdie(_(\"could not split '%s': %s\"), strategy_opts_string,\n+\t\tBUG(\"could not split '%s': %s\", strategy_opts_string,\n \t\t\t split_cmdline_strerror(count));\n \tfor (i = 0; i < count; i++) {\n \t\tconst char *arg = argv[i];\n@@ -3048,12 +3048,13 @@ static int read_populate_opts(struct replay_opts *opts)\n \n static void write_strategy_opts(struct replay_opts *opts)\n {\n-\tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfor (i = 0; i < opts->xopts.nr; ++i)\n-\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts.v[i]);\n-\n+\t/*\n+\t * Quote strategy options so that they can be read correctly\n+\t * by split_cmdline().\n+\t */\n+\tquote_cmdline(&buf, opts->xopts.v);\n \twrite_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n }\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 130e2f9b55..42c3954125 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard commit-new-file-F2-on-topic-branch &&\n \ttest_commit \"commit-new-file-F3-on-topic-branch\" F3 32 &&\n-\ttest_when_finished \"rm -fr test-bin funny.was.run\" &&\n+\ttest_when_finished \"rm -fr test-bin\" &&\n \tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tshift &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n+\n+\twrite_script test-bin/git-merge-funny <<-\\EOF &&\n+\tprintf \"[%s]\\n\" $# \"$1\" \"$2\" \"$3\" \"$5\" >actual\n+\tshift 3 &&\n+\texec git merge-recursive \"$@\"\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\t[7]\n+\t[--option=arg with space]\n+\t[--op\"tion\\]\n+\t[--new\n+\tline ]\n+\t[--]\n+\tEOF\n+\n+\trm -f actual &&\n \t(\n \t\tPATH=./test-bin:$PATH &&\n-\t\ttest_must_fail git rebase -s funny -Xopt main topic\n+\t\ttest_must_fail git rebase -s funny -X\"option=arg with space\" \\\n+\t\t\t\t-Xop\\\"tion\\\\ -X\"new${LF}line \" main topic\n \t) &&\n-\ttest -f funny.was.run &&\n-\trm funny.was.run &&\n+\ttest_cmp expect actual &&\n+\trm actual &&\n \techo \"Resolved\" >F2 &&\n \tgit add F2 &&\n \t(\n \t\tPATH=./test-bin:$PATH &&\n \t\tgit rebase --continue\n \t) &&\n-\ttest -f funny.was.run\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'rebase -i --continue handles merge strategy and options' '\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 3adf42f47d..94671d3c46 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -40,30 +40,6 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n-test_expect_success 'bad -X arguments: unclosed quote' '\n-\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n-\tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': unclosed quote\n-\tEOF\n-\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n-\t\tgit rebase -i -X\"bad argument\\\"\" side main &&\n-\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'bad -X arguments: bad escape' '\n-\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n-\tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': cmdline ends with \\\n-\tEOF\n-\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n-\t\tgit rebase -i -X\"bad escape \\\\\" side main &&\n-\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect actual\n-'\n-\n test_expect_success '--ignore-whitespace works with apply backend' '\n \ttest_must_fail git rebase --apply main side &&\n \tgit rebase --abort &&\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474998","messageId":"2d1d3281102f824b3eeb491ddc7e92239aee539f.1680875701.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1680875701.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 5/5] rebase: remove a couple of redundant strategy tests","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-07T13:55:35Z","receivedAt":"2023-04-07T13:57:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nRemove a test in t3402 that has been redundant ever since 80ff47957b\n(rebase: remember strategy and strategy options, 2011-02-06). That\ncommit added a new test, the first part of which (as noted in the old\ncommit message) duplicated an existing test.\n\nAlso remove a test t3418 that has been redundant since the merge backend\nwas removed in 68aa495b59 (rebase: implement --merge via the interactive\nmachinery, 2018-12-11), since it now tests the same code paths as the\npreceding test.\n\nHelped-by: Elijah Newren \nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n t/t3402-rebase-merge.sh | 21 ---------------------\n t/t3418-rebase-continue.sh | 32 --------------------------------\n 2 files changed, 53 deletions(-)\n\ndiff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\nindex 7e46f4ca85..79b0640c00 100755\n--- a/t/t3402-rebase-merge.sh\n+++ b/t/t3402-rebase-merge.sh\n@@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '\n \tesac\n '\n \n-test_expect_success 'rebase -s funny -Xopt' '\n-\ttest_when_finished \"rm -fr test-bin funny.was.run\" &&\n-\tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tshift &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n-\tgit reset --hard &&\n-\tgit checkout -b test-funny main^ &&\n-\ttest_commit funny &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\tgit rebase -s funny -Xopt main\n-\t) &&\n-\ttest -f funny.was.run\n-'\n-\n test_expect_success 'rebase --skip works with two conflicts in a row' '\n \tgit checkout second-side &&\n \ttr \"[A-Z]\" \"[a-z]\" tmp &&\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 42c3954125..2d0789e554 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'rebase -i --continue handles merge strategy and options' '\n-\trm -fr .git/rebase-* &&\n-\tgit reset --hard commit-new-file-F2-on-topic-branch &&\n-\ttest_commit \"commit-new-file-F3-on-topic-branch-for-dash-i\" F3 32 &&\n-\ttest_when_finished \"rm -fr test-bin funny.was.run funny.args\" &&\n-\tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\techo \"\\$@\" >>funny.args\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tcase \"\\$2\" in --foo) ;; *) exit 2 ;; esac\n-\tcase \"\\$4\" in --) ;; *) exit 2 ;; esac\n-\tshift 2 &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\ttest_must_fail git rebase -i -s funny -Xopt -Xfoo main topic\n-\t) &&\n-\ttest -f funny.was.run &&\n-\trm funny.was.run &&\n-\techo \"Resolved\" >F2 &&\n-\tgit add F2 &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\tgit rebase --continue\n-\t) &&\n-\ttest -f funny.was.run\n-'\n-\n test_expect_success 'rebase -r passes merge strategy options correctly' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard commit-new-file-F3-on-topic-branch &&\n-- \n2.40.0.670.g64ef305212.dirty\n\n"},{"id":"474999","messageId":"fe8fe698-28a0-3db7-f5b7-57456175fc74@dunelm.org.uk","threadId":"59400","inReplyTo":"CABPp-BFMiU8cYzUr1-m6P8w-YAtqPRyMfah8UAqdO_N4cdxUKg@mail.gmail.com","subject":"Re: [PATCH v2 0/5] rebase: cleanup merge strategy option handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-07T13:57:45Z","receivedAt":"2023-04-07T13:58:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 07/04/2023 06:49, Elijah Newren wrote:\n> On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood wrote:\n> I noticed a small typo/grammo in the commit message of Patch 4 that I\n> missed in V1, but otherwise this series looks good. I'm not sure\n> fixing the tiny grammo is even all that important; I'll leave it up to\n> you for whether you want to bother re-rolling to fix it. Either way:\n> \n> Reviewed-by: Elijah Newren \n\nThanks for reviewing this series. I've sent v3 with the typo fix and \nyour Reviewed-by: trailer.\n\nBest Wishes\n\nPhillip\n"},{"id":"475013","messageId":"xmqqmt3jk291.fsf@gitster.g","threadId":"59400","inReplyTo":"fe8fe698-28a0-3db7-f5b7-57456175fc74@dunelm.org.uk","subject":"Re: [PATCH v2 0/5] rebase: cleanup merge strategy option handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-07T17:53:30Z","receivedAt":"2023-04-07T17:53:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Thanks for reviewing this series. I've sent v3 with the typo fix and\n> your Reviewed-by: trailer.\n\nThanks, both, for writing, reviewing and polishing.\n"},{"id":"475055","messageId":"cover.1681117706.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1678893298.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v4 0/5] rebase: cleanup merge strategy option handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-10T09:08:26Z","receivedAt":"2023-04-10T09:08:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCleanup the handling of --strategy-option now that we no longer need to\nsupport \"--preserve-merges\" and properly quote the argument when saving\nit to disc.\n\nA hopefully final re-roll to fix a memory leak I spotted in V3.\n\nChanges since V3:\n - Fixed a memory leak added in V3. The array returned by\n split_cmdline() is allocated on the heap so we need to fee it. The\n array elements come from the string passed to split_cmdline() so do\n not need to be individually freed.\n\nChanges since V2:\n\n - Added Elijah's Reviewed-by: trailer.\n - Fixed a typo in patch 4 spotted by Elijah.\n\nChanges since V1:\n\nI've rebased these patches onto 'master' to avoid conflicts with\n'sg/parse-options-h-initializers' in the new patch 2 (this series\ndepends on 'ab/fix-strategy-opts-parsing' but that has now been merged\nto master).\n\nPatch 1 - Unchanged.\n\nPatch 2 - New patch to store the merge strategy options in an \"struct\n strvec\". This patch also introduces a new macro OPT_STRVEC()\n to collect options into an \"struct strvec\".\n\nPatch 3 - Small simplification due to the changes in patch 2.\n\nPatch 4 - Moved the code to quote a list so it can split by\n split_cmdline() into a new function quote_cmdline() as\n\t suggested by Elijah.\n\nPatch 5 - Reworded the commit message as suggested by Elijah.\n\n\nBase-Commit: 140b9478dad5d19543c1cb4fd293ccec228f1240\nPublished-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv4\nView-Changes-At: https://github.com/phillipwood/git/compare/140b9478d...7de1aa101\nFetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v4\n\nPhillip Wood (5):\n rebase: stop reading and writing unnecessary strategy state\n sequencer: use struct strvec to store merge strategy options\n rebase -m: cleanup --strategy-option handling\n rebase -m: fix serialization of strategy options\n rebase: remove a couple of redundant strategy tests\n\n alias.c | 18 +++++++\n alias.h | 3 ++\n builtin/rebase.c | 54 ++++-----------------\n builtin/revert.c | 20 ++------\n parse-options-cb.c | 16 +++++++\n parse-options.h | 10 ++++\n sequencer.c | 58 ++++++++++------------\n sequencer.h | 12 +++--\n t/t3402-rebase-merge.sh | 21 --------\n t/t3418-rebase-continue.sh | 88 +++++++++++++---------------------\n t/t3436-rebase-more-options.sh | 18 -------\n 11 files changed, 127 insertions(+), 191 deletions(-)\n\nRange-diff against v3:\n1: 882b403423 = 1: 882b403423 rebase: stop reading and writing unnecessary strategy state\n2: 1d8e59aa16 ! 2: 4b288e883d sequencer: use struct strvec to store merge strategy options\n @@ sequencer.c: void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n -\t\topts->xopts[i] = xstrdup(arg);\n +\t\tstrvec_push(&opts->xopts, arg);\n \t}\n ++\tfree(argv);\n }\n \n + static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)\n @@ sequencer.c: static void write_strategy_opts(struct replay_opts *opts)\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n3: e98ef5ce8c = 3: 4e040c1214 rebase -m: cleanup --strategy-option handling\n4: a5e940e2d0 = 4: 671ee03503 rebase -m: fix serialization of strategy options\n5: 2d1d328110 = 5: 7de1aa1016 rebase: remove a couple of redundant strategy tests\n-- \n2.40.0.672.gbd2d2ac924\n\n"},{"id":"475056","messageId":"882b403423e5c2d90c3afc4eefb02bb499c4a02b.1681117706.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1681117706.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v4 1/5] rebase: stop reading and writing unnecessary strategy state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-10T09:08:27Z","receivedAt":"2023-04-10T09:09:00Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe state files for \"--strategy\" and \"--strategy-option\" are written and\nread twice, once by builtin/rebase.c and then by sequencer.c. This is an\nartifact of the scripted rebase and the need to support \"rebase\n--preserve-merges\". Now that \"--preserve-merges\" no-longer exists we\nonly need to read and write these files in sequencer.c. This enables us\nto remove a call to free() in read_strategy_opts() that was added by\nf1f4ebf432 (sequencer.c: fix \"opts->strategy\" leak in\nread_strategy_opts(), 2022-11-08) as this commit fixes the root cause of\nthat leak.\n\nThere is further scope for removing duplication in the reading and\nwriting of state files between builtin/rebase.c and sequencer.c but that\nis left for a follow up series.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 24 ------------------------\n sequencer.c | 1 -\n 2 files changed, 25 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5b7b908b66..3bd215c771 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -483,24 +483,6 @@ static int read_basic_state(struct rebase_options *opts)\n \t\topts->gpg_sign_opt = xstrdup(buf.buf);\n \t}\n \n-\tif (file_exists(state_dir_path(\"strategy\", opts))) {\n-\t\tstrbuf_reset(&buf);\n-\t\tif (!read_oneliner(&buf, state_dir_path(\"strategy\", opts),\n-\t\t\t\t READ_ONELINER_WARN_MISSING))\n-\t\t\treturn -1;\n-\t\tfree(opts->strategy);\n-\t\topts->strategy = xstrdup(buf.buf);\n-\t}\n-\n-\tif (file_exists(state_dir_path(\"strategy_opts\", opts))) {\n-\t\tstrbuf_reset(&buf);\n-\t\tif (!read_oneliner(&buf, state_dir_path(\"strategy_opts\", opts),\n-\t\t\t\t READ_ONELINER_WARN_MISSING))\n-\t\t\treturn -1;\n-\t\tfree(opts->strategy_opts);\n-\t\topts->strategy_opts = xstrdup(buf.buf);\n-\t}\n-\n \tstrbuf_release(&buf);\n \n \treturn 0;\n@@ -518,12 +500,6 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n \t\twrite_file(state_dir_path(\"quiet\", opts), \"%s\", \"\");\n \tif (opts->flags & REBASE_VERBOSE)\n \t\twrite_file(state_dir_path(\"verbose\", opts), \"%s\", \"\");\n-\tif (opts->strategy)\n-\t\twrite_file(state_dir_path(\"strategy\", opts), \"%s\",\n-\t\t\t opts->strategy);\n-\tif (opts->strategy_opts)\n-\t\twrite_file(state_dir_path(\"strategy_opts\", opts), \"%s\",\n-\t\t\t opts->strategy_opts);\n \tif (opts->allow_rerere_autoupdate > 0)\n \t\twrite_file(state_dir_path(\"allow_rerere_autoupdate\", opts),\n \t\t\t \"-%s-rerere-autoupdate\",\ndiff --git a/sequencer.c b/sequencer.c\nindex 3be23d7ca2..c35a67e104 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2944,7 +2944,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)\n \tstrbuf_reset(buf);\n \tif (!read_oneliner(buf, rebase_path_strategy(), 0))\n \t\treturn;\n-\tfree(opts->strategy);\n \topts->strategy = strbuf_detach(buf, NULL);\n \tif (!read_oneliner(buf, rebase_path_strategy_opts(), 0))\n \t\treturn;\n-- \n2.40.0.672.gbd2d2ac924\n\n"},{"id":"475057","messageId":"4e040c1214e133be2a614eb85380f582118ff246.1681117706.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1681117706.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v4 3/5] rebase -m: cleanup --strategy-option handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-10T09:08:29Z","receivedAt":"2023-04-10T09:09:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen handling \"--strategy-option\" rebase collects the commands into a\nstruct string_list, then concatenates them into a string, prepending \"--\"\nto each one before splitting the string and removing the \"--\" prefix.\nThis is an artifact of the scripted rebase and the need to support\n\"rebase --preserve-merges\". Now that \"--preserve-merges\" no-longer\nexists we can cleanup the way the argument is handled.\n\nThe tests for a bad strategy option are adjusted now that\nparse_strategy_opts() is no-longer called when starting a rebase. The\nfact that it only errors out when running \"git rebase --continue\" is a\nmixed blessing but the next commit will fix the root cause of the\nparsing problem so lets not worry about that here.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 30 ++++++++++--------------------\n sequencer.c | 2 +-\n sequencer.h | 1 -\n t/t3436-rebase-more-options.sh | 10 ++++++++--\n 4 files changed, 19 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 3bd215c771..511922c6fc 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -117,7 +117,8 @@ struct rebase_options {\n \tstruct string_list exec;\n \tint allow_empty_message;\n \tint rebase_merges, rebase_cousins;\n-\tchar *strategy, *strategy_opts;\n+\tchar *strategy;\n+\tstruct string_list strategy_opts;\n \tstruct strbuf git_format_patch_opt;\n \tint reschedule_failed_exec;\n \tint reapply_cherry_picks;\n@@ -143,6 +144,7 @@ struct rebase_options {\n \t\t.config_autosquash = -1, \\\n \t\t.update_refs = -1, \\\n \t\t.config_update_refs = -1, \\\n+\t\t.strategy_opts = STRING_LIST_INIT_NODUP,\\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -176,8 +178,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\treplay.default_strategy = NULL;\n \t}\n \n-\tif (opts->strategy_opts)\n-\t\tparse_strategy_opts(&replay, opts->strategy_opts);\n+\tfor (size_t i = 0; i < opts->strategy_opts.nr; i++)\n+\t\tstrvec_push(&replay.xopts, opts->strategy_opts.items[i].string);\n \n \tif (opts->squash_onto) {\n \t\toidcpy(&replay.squash_onto, opts->squash_onto);\n@@ -1013,7 +1015,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tint ignore_whitespace = 0;\n \tconst char *gpg_sign = NULL;\n \tconst char *rebase_merges = NULL;\n-\tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n \tchar *squash_onto_name = NULL;\n \tchar *keep_base_onto_name = NULL;\n@@ -1122,7 +1123,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"use 'merge-base --fork-point' to refine upstream\")),\n \t\tOPT_STRING('s', \"strategy\", &options.strategy,\n \t\t\t N_(\"strategy\"), N_(\"use the given merge strategy\")),\n-\t\tOPT_STRING_LIST('X', \"strategy-option\", &strategy_options,\n+\t\tOPT_STRING_LIST('X', \"strategy-option\", &options.strategy_opts,\n \t\t\t\tN_(\"option\"),\n \t\t\t\tN_(\"pass the argument through to the merge \"\n \t\t\t\t \"strategy\")),\n@@ -1436,23 +1437,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t} else {\n \t\t/* REBASE_MERGE */\n \t\tif (ignore_whitespace) {\n-\t\t\tstring_list_append(&strategy_options,\n+\t\t\tstring_list_append(&options.strategy_opts,\n \t\t\t\t\t \"ignore-space-change\");\n \t\t}\n \t}\n \n-\tif (strategy_options.nr) {\n-\t\tint i;\n-\n-\t\tif (!options.strategy)\n-\t\t\toptions.strategy = \"ort\";\n-\n-\t\tstrbuf_reset(&buf);\n-\t\tfor (i = 0; i < strategy_options.nr; i++)\n-\t\t\tstrbuf_addf(&buf, \" --%s\",\n-\t\t\t\t strategy_options.items[i].string);\n-\t\toptions.strategy_opts = xstrdup(buf.buf);\n-\t}\n+\tif (options.strategy_opts.nr && !options.strategy)\n+\t\toptions.strategy = \"ort\";\n \n \tif (options.strategy) {\n \t\toptions.strategy = xstrdup(options.strategy);\n@@ -1827,10 +1818,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tfree(options.gpg_sign_opt);\n \tstring_list_clear(&options.exec, 0);\n \tfree(options.strategy);\n-\tfree(options.strategy_opts);\n+\tstring_list_clear(&options.strategy_opts, 0);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n \tfree(keep_base_onto_name);\n-\tstring_list_clear(&strategy_options, 0);\n \treturn !!ret;\n }\ndiff --git a/sequencer.c b/sequencer.c\nindex 50d469812a..587a473d6e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2913,7 +2913,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \treturn 0;\n }\n \n-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n+static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n {\n \tint i;\n \tint count;\ndiff --git a/sequencer.h b/sequencer.h\nindex 8a79d6b200..913a0f652d 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -252,7 +252,6 @@ int read_oneliner(struct strbuf *buf,\n \tconst char *path, unsigned flags);\n int read_author_script(const char *path, char **name, char **email, char **date,\n \t\t int allow_missing);\n-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\t struct commit *onto, const struct object_id *orig_head);\n void sequencer_post_commit_cleanup(struct repository *r, int verbose);\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex c3184c9ade..3adf42f47d 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -41,19 +41,25 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'bad -X arguments: unclosed quote' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tcat >expect <<-\\EOF &&\n \tfatal: could not split '\\''--bad'\\'': unclosed quote\n \tEOF\n-\ttest_expect_code 128 git rebase -X\"bad argument\\\"\" side main >out 2>actual &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n+\t\tgit rebase -i -X\"bad argument\\\"\" side main &&\n+\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n \ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'bad -X arguments: bad escape' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tcat >expect <<-\\EOF &&\n \tfatal: could not split '\\''--bad'\\'': cmdline ends with \\\n \tEOF\n-\ttest_expect_code 128 git rebase -X\"bad escape \\\\\" side main >out 2>actual &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n+\t\tgit rebase -i -X\"bad escape \\\\\" side main &&\n+\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n \ttest_must_be_empty out &&\n \ttest_cmp expect actual\n '\n-- \n2.40.0.672.gbd2d2ac924\n\n"},{"id":"475058","messageId":"4b288e883d803a75ecd032058d14e9d20eaac9c8.1681117706.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1681117706.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v4 2/5] sequencer: use struct strvec to store merge strategy options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-10T09:08:28Z","receivedAt":"2023-04-10T09:09:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe sequencer stores the merge strategy options in an array of strings\nwhich allocated with ALLOC_GROW(). Using \"struct strvec\" avoids manually\nmanaging the memory of that array and simplifies the code.\n\nAside from memory allocation the changes to the sequencer are largely\nmechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A\nnew option parsing macro OPT_STRVEC() is also added to collect the\nstrategy options. Hopefully this can be used to simplify the code in\nbuiltin/merge.c in the future.\n\nNote that there is a change of behavior to \"git cherry-pick\" and \"git\nrevert\" as passing “--no-strategy-option” will now clear any previous\nstrategy options whereas before this change it did nothing.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n builtin/revert.c | 20 +++----------------\n parse-options-cb.c | 16 ++++++++++++++++\n parse-options.h | 10 ++++++++++\n sequencer.c | 48 ++++++++++++++++++++--------------------------\n sequencer.h | 11 ++++++++---\n 5 files changed, 58 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 62986a7b1b..362857da65 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n \treturn opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n }\n \n-static int option_parse_x(const struct option *opt,\n-\t\t\t const char *arg, int unset)\n-{\n-\tstruct replay_opts **opts_ptr = opt->value;\n-\tstruct replay_opts *opts = *opts_ptr;\n-\n-\tif (unset)\n-\t\treturn 0;\n-\n-\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n-\topts->xopts[opts->xopts_nr++] = xstrdup(arg);\n-\treturn 0;\n-}\n-\n static int option_parse_m(const struct option *opt,\n \t\t\t const char *arg, int unset)\n {\n@@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t N_(\"select mainline parent\"), option_parse_m),\n \t\tOPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),\n \t\tOPT_STRING(0, \"strategy\", &opts->strategy, N_(\"strategy\"), N_(\"merge strategy\")),\n-\t\tOPT_CALLBACK('X', \"strategy-option\", &opts, N_(\"option\"),\n-\t\t\tN_(\"option for merge strategy\"), option_parse_x),\n+\t\tOPT_STRVEC('X', \"strategy-option\", &opts->xopts, N_(\"option\"),\n+\t\t\tN_(\"option for merge strategy\")),\n \t\t{ OPTION_STRING, 'S', \"gpg-sign\", &opts->gpg_sign, N_(\"key-id\"),\n \t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \t\tOPT_END()\n@@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"--signoff\", opts->signoff,\n \t\t\t\t\"--mainline\", opts->mainline,\n \t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n-\t\t\t\t\"--strategy-option\", opts->xopts ? 1 : 0,\n+\t\t\t\t\"--strategy-option\", opts->xopts.nr ? 1 : 0,\n \t\t\t\t\"-x\", opts->record_origin,\n \t\t\t\t\"--ff\", opts->allow_ff,\n \t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex d346dbe210..8eb96c5ca9 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+int parse_opt_strvec(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct strvec *v = opt->value;\n+\n+\tif (unset) {\n+\t\tstrvec_clear(v);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!arg)\n+\t\treturn -1;\n+\n+\tstrvec_push(v, arg);\n+\treturn 0;\n+}\n+\n int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)\n {\n \treturn 0;\ndiff --git a/parse-options.h b/parse-options.h\nindex 26f19384e5..2d1d4d052f 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -285,6 +285,15 @@ struct option {\n \t.help = (h), \\\n \t.callback = &parse_opt_string_list, \\\n }\n+#define OPT_STRVEC(s, l, v, a, h) { \\\n+\t.type = OPTION_CALLBACK, \\\n+\t.short_name = (s), \\\n+\t.long_name = (l), \\\n+\t.value = (v), \\\n+\t.argh = (a), \\\n+\t.help = (h), \\\n+\t.callback = &parse_opt_strvec, \\\n+}\n #define OPT_UYN(s, l, v, h) { \\\n \t.type = OPTION_CALLBACK, \\\n \t.short_name = (s), \\\n@@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int);\n int parse_opt_commit(const struct option *, const char *, int);\n int parse_opt_tertiary(const struct option *, const char *, int);\n int parse_opt_string_list(const struct option *, const char *, int);\n+int parse_opt_strvec(const struct option *, const char *, int);\n int parse_opt_noop_cb(const struct option *, const char *, int);\n enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t const struct option *,\ndiff --git a/sequencer.c b/sequencer.c\nindex c35a67e104..50d469812a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts)\n \tfree(opts->reflog_action);\n \tfree(opts->default_strategy);\n \tfree(opts->strategy);\n-\tfor (size_t i = 0; i < opts->xopts_nr; i++)\n-\t\tfree(opts->xopts[i]);\n-\tfree(opts->xopts);\n+\tstrvec_clear (&opts->xopts);\n \tstrbuf_release(&opts->current_fixups);\n \tif (opts->revs)\n \t\trelease_revisions(opts->revs);\n@@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r,\n \tnext_tree = next ? get_commit_tree(next) : empty_tree(r);\n \tbase_tree = base ? get_commit_tree(base) : empty_tree(r);\n \n-\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\tparse_merge_opt(&o, opts->xopts[i]);\n+\tfor (i = 0; i < opts->xopts.nr; i++)\n+\t\tparse_merge_opt(&o, opts->xopts.v[i]);\n \n \tif (!opts->strategy || !strcmp(opts->strategy, \"ort\")) {\n \t\tmemset(&result, 0, sizeof(result));\n@@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r,\n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n \t\tres |= try_merge_command(r, opts->strategy,\n-\t\t\t\t\t opts->xopts_nr, (const char **)opts->xopts,\n+\t\t\t\t\t opts->xopts.nr, opts->xopts.v,\n \t\t\t\t\tcommon, oid_to_hex(&head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n@@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \telse if (!strcmp(key, \"options.gpg-sign\"))\n \t\tgit_config_string_dup(&opts->gpg_sign, key, value);\n \telse if (!strcmp(key, \"options.strategy-option\")) {\n-\t\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n-\t\topts->xopts[opts->xopts_nr++] = xstrdup(value);\n+\t\tstrvec_push(&opts->xopts, value);\n \t} else if (!strcmp(key, \"options.allow-rerere-auto\"))\n \t\topts->allow_rerere_auto =\n \t\t\tgit_config_bool_or_int(key, value, &error_flag) ?\n@@ -2920,23 +2917,23 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n {\n \tint i;\n \tint count;\n+\tconst char **argv;\n \tchar *strategy_opts_string = raw_opts;\n \n \tif (*strategy_opts_string == ' ')\n \t\tstrategy_opts_string++;\n \n-\tcount = split_cmdline(strategy_opts_string,\n-\t\t\t (const char ***)&opts->xopts);\n+\tcount = split_cmdline(strategy_opts_string, &argv);\n \tif (count < 0)\n \t\tdie(_(\"could not split '%s': %s\"), strategy_opts_string,\n \t\t\t split_cmdline_strerror(count));\n-\topts->xopts_nr = count;\n-\tfor (i = 0; i < opts->xopts_nr; i++) {\n-\t\tconst char *arg = opts->xopts[i];\n+\tfor (i = 0; i < count; i++) {\n+\t\tconst char *arg = argv[i];\n \n \t\tskip_prefix(arg, \"--\", &arg);\n-\t\topts->xopts[i] = xstrdup(arg);\n+\t\tstrvec_push(&opts->xopts, arg);\n \t}\n+\tfree(argv);\n }\n \n static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)\n@@ -3055,8 +3052,8 @@ static void write_strategy_opts(struct replay_opts *opts)\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfor (i = 0; i < opts->xopts_nr; ++i)\n-\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts[i]);\n+\tfor (i = 0; i < opts->xopts.nr; ++i)\n+\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts.v[i]);\n \n \twrite_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n@@ -3080,7 +3077,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\twrite_file(rebase_path_verbose(), \"%s\", \"\");\n \tif (opts->strategy)\n \t\twrite_file(rebase_path_strategy(), \"%s\\n\", opts->strategy);\n-\tif (opts->xopts_nr > 0)\n+\tif (opts->xopts.nr > 0)\n \t\twrite_strategy_opts(opts);\n \n \tif (opts->allow_rerere_auto == RERERE_AUTOUPDATE)\n@@ -3464,13 +3461,10 @@ static int save_opts(struct replay_opts *opts)\n \tif (opts->gpg_sign)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.gpg-sign\", opts->gpg_sign);\n-\tif (opts->xopts) {\n-\t\tint i;\n-\t\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.strategy-option\",\n-\t\t\t\t\topts->xopts[i], \"^$\", 0);\n-\t}\n+\tfor (size_t i = 0; i < opts->xopts.nr; i++)\n+\t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n+\t\t\t\t\"options.strategy-option\",\n+\t\t\t\topts->xopts.v[i], \"^$\", 0);\n \tif (opts->allow_rerere_auto)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\"options.allow-rerere-auto\",\n@@ -3880,7 +3874,7 @@ static int do_merge(struct repository *r,\n \tstruct commit *head_commit, *merge_commit, *i;\n \tstruct commit_list *bases, *j;\n \tstruct commit_list *to_merge = NULL, **tail = &to_merge;\n-\tconst char *strategy = !opts->xopts_nr &&\n+\tconst char *strategy = !opts->xopts.nr &&\n \t\t(!opts->strategy ||\n \t\t !strcmp(opts->strategy, \"recursive\") ||\n \t\t !strcmp(opts->strategy, \"ort\")) ?\n@@ -4063,9 +4057,9 @@ static int do_merge(struct repository *r,\n \t\t\tstrvec_push(&cmd.args, \"octopus\");\n \t\telse {\n \t\t\tstrvec_push(&cmd.args, strategy);\n-\t\t\tfor (k = 0; k < opts->xopts_nr; k++)\n+\t\t\tfor (k = 0; k < opts->xopts.nr; k++)\n \t\t\t\tstrvec_pushf(&cmd.args,\n-\t\t\t\t\t \"-X%s\", opts->xopts[k]);\n+\t\t\t\t\t \"-X%s\", opts->xopts.v[k]);\n \t\t}\n \t\tif (!(flags & TODO_EDIT_MERGE_MSG))\n \t\t\tstrvec_push(&cmd.args, \"--no-edit\");\ndiff --git a/sequencer.h b/sequencer.h\nindex 33dbaf5b66..8a79d6b200 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -2,6 +2,7 @@\n #define SEQUENCER_H\n \n #include \"strbuf.h\"\n+#include \"strvec.h\"\n #include \"wt-status.h\"\n \n struct commit;\n@@ -60,8 +61,7 @@ struct replay_opts {\n \t/* Merge strategy */\n \tchar *default_strategy; /* from config options */\n \tchar *strategy;\n-\tchar **xopts;\n-\tsize_t xopts_nr, xopts_alloc;\n+\tstruct strvec xopts;\n \n \t/* Reflog */\n \tchar *reflog_action;\n@@ -80,7 +80,12 @@ struct replay_opts {\n \t/* Private use */\n \tconst char *reflog_message;\n };\n-#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n+#define REPLAY_OPTS_INIT {\t\t\t\\\n+\t.edit = -1,\t\t\t\t\\\n+\t.action = -1,\t\t\t\t\\\n+\t.current_fixups = STRBUF_INIT,\t\t\\\n+\t.xopts = STRVEC_INIT,\t\t\t\\\n+}\n \n /*\n * Note that ordering matters in this enum. Not only must it match the mapping\n-- \n2.40.0.672.gbd2d2ac924\n\n"},{"id":"475059","messageId":"671ee03503dd1642a8d38b9d86b913ec39b57ba3.1681117706.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1681117706.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v4 4/5] rebase -m: fix serialization of strategy options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-10T09:08:30Z","receivedAt":"2023-04-10T09:09:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nTo store the strategy options rebase prepends \" --\" to each one and\nwrites them to a file. To load them it reads the file and passes the\ncontents to split_cmdline(). This roughly mimics the behavior of the\nscripted rebase but has a couple of limitations, (1) options containing\nwhitespace are not properly preserved (this is true of the scripted\nrebase as well) and (2) options containing '\"' or '\\' are incorrectly\nparsed and may cause the parser to return an error.\n\nFix these limitations by quoting each option when they are stored so\nthat they can be parsed correctly. Now that \"--preserve-merges\" no\nlonger exist this change also stops prepending \"--\" to the options when\nthey are stored as that was an artifact of the scripted rebase.\n\nThese changes are backwards compatible so the files written by an older\nversion of git can still be read. They are also forwards compatible,\nthe file can still be parsed by recent versions of git as they treat the\n\"--\" prefix as optional.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n alias.c | 18 +++++++++++++++++\n alias.h | 3 +++\n sequencer.c | 11 ++++++-----\n t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++------------\n t/t3436-rebase-more-options.sh | 24 -----------------------\n 5 files changed, 50 insertions(+), 42 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex e814948ced..54a1a23d2c 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -3,6 +3,7 @@\n #include \"alloc.h\"\n #include \"config.h\"\n #include \"gettext.h\"\n+#include \"strbuf.h\"\n #include \"string-list.h\"\n \n struct config_alias_data {\n@@ -46,6 +47,23 @@ void list_aliases(struct string_list *list)\n \tread_early_config(config_alias_cb, &data);\n }\n \n+void quote_cmdline(struct strbuf *buf, const char **argv)\n+{\n+\tfor (const char **argp = argv; *argp; argp++) {\n+\t\tif (argp != argv)\n+\t\t\tstrbuf_addch(buf, ' ');\n+\t\tstrbuf_addch(buf, '\"');\n+\t\tfor (const char *p = *argp; *p; p++) {\n+\t\t\tconst char c = *p;\n+\n+\t\t\tif (c == '\"' || c =='\\\\')\n+\t\t\t\tstrbuf_addch(buf, '\\\\');\n+\t\t\tstrbuf_addch(buf, c);\n+\t\t}\n+\t\tstrbuf_addch(buf, '\"');\n+\t}\n+}\n+\n #define SPLIT_CMDLINE_BAD_ENDING 1\n #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2\n #define SPLIT_CMDLINE_ARGC_OVERFLOW 3\ndiff --git a/alias.h b/alias.h\nindex aef4843bb7..43db736484 100644\n--- a/alias.h\n+++ b/alias.h\n@@ -1,9 +1,12 @@\n #ifndef ALIAS_H\n #define ALIAS_H\n \n+struct strbuf;\n struct string_list;\n \n char *alias_lookup(const char *alias);\n+/* Quote argv so buf can be parsed by split_cmdline() */\n+void quote_cmdline(struct strbuf *buf, const char **argv);\n int split_cmdline(char *cmdline, const char ***argv);\n /* Takes a negative value returned by split_cmdline */\n const char *split_cmdline_strerror(int cmdline_errno);\ndiff --git a/sequencer.c b/sequencer.c\nindex 587a473d6e..fc6ea75895 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n \n \tcount = split_cmdline(strategy_opts_string, &argv);\n \tif (count < 0)\n-\t\tdie(_(\"could not split '%s': %s\"), strategy_opts_string,\n+\t\tBUG(\"could not split '%s': %s\", strategy_opts_string,\n \t\t\t split_cmdline_strerror(count));\n \tfor (i = 0; i < count; i++) {\n \t\tconst char *arg = argv[i];\n@@ -3049,12 +3049,13 @@ static int read_populate_opts(struct replay_opts *opts)\n \n static void write_strategy_opts(struct replay_opts *opts)\n {\n-\tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfor (i = 0; i < opts->xopts.nr; ++i)\n-\t\tstrbuf_addf(&buf, \" --%s\", opts->xopts.v[i]);\n-\n+\t/*\n+\t * Quote strategy options so that they can be read correctly\n+\t * by split_cmdline().\n+\t */\n+\tquote_cmdline(&buf, opts->xopts.v);\n \twrite_file(rebase_path_strategy_opts(), \"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n }\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 130e2f9b55..42c3954125 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard commit-new-file-F2-on-topic-branch &&\n \ttest_commit \"commit-new-file-F3-on-topic-branch\" F3 32 &&\n-\ttest_when_finished \"rm -fr test-bin funny.was.run\" &&\n+\ttest_when_finished \"rm -fr test-bin\" &&\n \tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tshift &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n+\n+\twrite_script test-bin/git-merge-funny <<-\\EOF &&\n+\tprintf \"[%s]\\n\" $# \"$1\" \"$2\" \"$3\" \"$5\" >actual\n+\tshift 3 &&\n+\texec git merge-recursive \"$@\"\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\t[7]\n+\t[--option=arg with space]\n+\t[--op\"tion\\]\n+\t[--new\n+\tline ]\n+\t[--]\n+\tEOF\n+\n+\trm -f actual &&\n \t(\n \t\tPATH=./test-bin:$PATH &&\n-\t\ttest_must_fail git rebase -s funny -Xopt main topic\n+\t\ttest_must_fail git rebase -s funny -X\"option=arg with space\" \\\n+\t\t\t\t-Xop\\\"tion\\\\ -X\"new${LF}line \" main topic\n \t) &&\n-\ttest -f funny.was.run &&\n-\trm funny.was.run &&\n+\ttest_cmp expect actual &&\n+\trm actual &&\n \techo \"Resolved\" >F2 &&\n \tgit add F2 &&\n \t(\n \t\tPATH=./test-bin:$PATH &&\n \t\tgit rebase --continue\n \t) &&\n-\ttest -f funny.was.run\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'rebase -i --continue handles merge strategy and options' '\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 3adf42f47d..94671d3c46 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -40,30 +40,6 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n-test_expect_success 'bad -X arguments: unclosed quote' '\n-\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n-\tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': unclosed quote\n-\tEOF\n-\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n-\t\tgit rebase -i -X\"bad argument\\\"\" side main &&\n-\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'bad -X arguments: bad escape' '\n-\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n-\tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': cmdline ends with \\\n-\tEOF\n-\tGIT_SEQUENCE_EDITOR=\"echo break >\" \\\n-\t\tgit rebase -i -X\"bad escape \\\\\" side main &&\n-\ttest_expect_code 128 git rebase --continue >out 2>actual &&\n-\ttest_must_be_empty out &&\n-\ttest_cmp expect actual\n-'\n-\n test_expect_success '--ignore-whitespace works with apply backend' '\n \ttest_must_fail git rebase --apply main side &&\n \tgit rebase --abort &&\n-- \n2.40.0.672.gbd2d2ac924\n\n"},{"id":"475060","messageId":"7de1aa1016bb22448f10d912b62ebd984779be19.1681117706.git.phillip.wood@dunelm.org.uk","threadId":"59400","inReplyTo":"cover.1681117706.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v4 5/5] rebase: remove a couple of redundant strategy tests","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-10T09:08:31Z","receivedAt":"2023-04-10T09:09:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nRemove a test in t3402 that has been redundant ever since 80ff47957b\n(rebase: remember strategy and strategy options, 2011-02-06). That\ncommit added a new test, the first part of which (as noted in the old\ncommit message) duplicated an existing test.\n\nAlso remove a test t3418 that has been redundant since the merge backend\nwas removed in 68aa495b59 (rebase: implement --merge via the interactive\nmachinery, 2018-12-11), since it now tests the same code paths as the\npreceding test.\n\nHelped-by: Elijah Newren \nReviewed-by: Elijah Newren \nSigned-off-by: Phillip Wood \n---\n t/t3402-rebase-merge.sh | 21 ---------------------\n t/t3418-rebase-continue.sh | 32 --------------------------------\n 2 files changed, 53 deletions(-)\n\ndiff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\nindex 7e46f4ca85..79b0640c00 100755\n--- a/t/t3402-rebase-merge.sh\n+++ b/t/t3402-rebase-merge.sh\n@@ -131,27 +131,6 @@ test_expect_success 'picking rebase' '\n \tesac\n '\n \n-test_expect_success 'rebase -s funny -Xopt' '\n-\ttest_when_finished \"rm -fr test-bin funny.was.run\" &&\n-\tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tshift &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n-\tgit reset --hard &&\n-\tgit checkout -b test-funny main^ &&\n-\ttest_commit funny &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\tgit rebase -s funny -Xopt main\n-\t) &&\n-\ttest -f funny.was.run\n-'\n-\n test_expect_success 'rebase --skip works with two conflicts in a row' '\n \tgit checkout second-side &&\n \ttr \"[A-Z]\" \"[a-z]\" tmp &&\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 42c3954125..2d0789e554 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'rebase -i --continue handles merge strategy and options' '\n-\trm -fr .git/rebase-* &&\n-\tgit reset --hard commit-new-file-F2-on-topic-branch &&\n-\ttest_commit \"commit-new-file-F3-on-topic-branch-for-dash-i\" F3 32 &&\n-\ttest_when_finished \"rm -fr test-bin funny.was.run funny.args\" &&\n-\tmkdir test-bin &&\n-\tcat >test-bin/git-merge-funny <<-EOF &&\n-\t#!$SHELL_PATH\n-\techo \"\\$@\" >>funny.args\n-\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n-\tcase \"\\$2\" in --foo) ;; *) exit 2 ;; esac\n-\tcase \"\\$4\" in --) ;; *) exit 2 ;; esac\n-\tshift 2 &&\n-\t>funny.was.run &&\n-\texec git merge-recursive \"\\$@\"\n-\tEOF\n-\tchmod +x test-bin/git-merge-funny &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\ttest_must_fail git rebase -i -s funny -Xopt -Xfoo main topic\n-\t) &&\n-\ttest -f funny.was.run &&\n-\trm funny.was.run &&\n-\techo \"Resolved\" >F2 &&\n-\tgit add F2 &&\n-\t(\n-\t\tPATH=./test-bin:$PATH &&\n-\t\tgit rebase --continue\n-\t) &&\n-\ttest -f funny.was.run\n-'\n-\n test_expect_success 'rebase -r passes merge strategy options correctly' '\n \trm -fr .git/rebase-* &&\n \tgit reset --hard commit-new-file-F3-on-topic-branch &&\n-- \n2.40.0.672.gbd2d2ac924\n\n"},{"id":"475068","messageId":"CABPp-BGUa_Aot_2iPmTWh1KvuL7mS4OSwXX-ArqgFSd8nU-+ig@mail.gmail.com","threadId":"59400","inReplyTo":"cover.1681117706.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v4 0/5] rebase: cleanup merge strategy option handling","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-10T16:46:04Z","receivedAt":"2023-04-10T16:46:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Apr 10, 2023 at 2:08 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> Cleanup the handling of --strategy-option now that we no longer need to\n> support \"--preserve-merges\" and properly quote the argument when saving\n> it to disc.\n>\n> A hopefully final re-roll to fix a memory leak I spotted in V3.\n\nDoh, I clearly missed that leak too.\n\n> Changes since V3:\n> - Fixed a memory leak added in V3. The array returned by\n> split_cmdline() is allocated on the heap so we need to fee it. The\n> array elements come from the string passed to split_cmdline() so do\n> not need to be individually freed.\n\nAnd in particular, that string that stores those array elements will\nbe freed by the strbuf_reset() that comes immediately after the\nread_strategy_opts() call (read_strategy_opts() is the sole caller of\nparse_strategy_opts()), and the fact that the string is freed is fine\nsince your parse_strategy_opts() passes them to strvec_push() which\nwill xstrdup() them and then own them.\n\nAll that to say I agree with the fix.\n"}]}