* [PATCH 0/4] rebase: cleanup merge strategy option handling @ 2023-03-15 15:14 Phillip Wood 2023-03-15 15:14 ` [PATCH 1/4] rebase: stop reading and writing unnecessary strategy state Phillip Wood ` (9 more replies) 0 siblings, 10 replies; 36+ messages in thread From: Phillip Wood @ 2023-03-15 15:14 UTC (permalink / raw) To: git; +Cc: Ævar Arnfjörð Bjarmason, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Cleanup the handling of --strategy-option now that we no longer need to support "--preserve-merges" and properly quote the argument when saving it to disc. These patches are based on a merge of 'master' and 'ab/fix-strategy-opts-parsing' Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv1 View-Changes-At: https://github.com/phillipwood/git/compare/c2e329a52...3e02eeff7 Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v1 Phillip Wood (4): rebase: stop reading and writing unnecessary strategy state rebase -m: cleanup --strategy-option handling rebase -m: fix serialization of strategy options rebase: remove a couple of redundant strategy tests builtin/rebase.c | 60 +++++++++----------------------- sequencer.c | 26 ++++++++++---- sequencer.h | 1 - t/t3402-rebase-merge.sh | 21 ------------ t/t3418-rebase-continue.sh | 62 +++++++++++----------------------- t/t3436-rebase-more-options.sh | 18 ---------- 6 files changed, 56 insertions(+), 132 deletions(-) -- 2.39.2 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/4] rebase: stop reading and writing unnecessary strategy state 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood @ 2023-03-15 15:14 ` Phillip Wood 2023-03-15 15:14 ` [PATCH 2/4] rebase -m: cleanup --strategy-option handling Phillip Wood ` (8 subsequent siblings) 9 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-03-15 15:14 UTC (permalink / raw) To: git; +Cc: Ævar Arnfjörð Bjarmason, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The state files for "--strategy" and "--strategy-option" are written and read twice, once by builtin/rebase.c and then by sequencer.c. This is an artifact of the scripted rebase and the need to support "rebase --preserve-merges". Now that "--preserve-merges" no-longer exists we only need to read and write these files in sequencer.c. This enables us to remove a call to free() in read_strategy_opts() that was added by f1f4ebf432 (sequencer.c: fix "opts->strategy" leak in read_strategy_opts(), 2022-11-08) as this commit fixes the root cause of that leak. There is further scope for removing duplication in the reading and writing of state files between builtin/rebase.c and sequencer.c but that is left for a follow up series. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 24 ------------------------ sequencer.c | 1 - 2 files changed, 25 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 6635f10d52..516ad1b12a 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -482,24 +482,6 @@ static int read_basic_state(struct rebase_options *opts) opts->gpg_sign_opt = xstrdup(buf.buf); } - if (file_exists(state_dir_path("strategy", opts))) { - strbuf_reset(&buf); - if (!read_oneliner(&buf, state_dir_path("strategy", opts), - READ_ONELINER_WARN_MISSING)) - return -1; - free(opts->strategy); - opts->strategy = xstrdup(buf.buf); - } - - if (file_exists(state_dir_path("strategy_opts", opts))) { - strbuf_reset(&buf); - if (!read_oneliner(&buf, state_dir_path("strategy_opts", opts), - READ_ONELINER_WARN_MISSING)) - return -1; - free(opts->strategy_opts); - opts->strategy_opts = xstrdup(buf.buf); - } - strbuf_release(&buf); return 0; @@ -517,12 +499,6 @@ static int rebase_write_basic_state(struct rebase_options *opts) write_file(state_dir_path("quiet", opts), "%s", ""); if (opts->flags & REBASE_VERBOSE) write_file(state_dir_path("verbose", opts), "%s", ""); - if (opts->strategy) - write_file(state_dir_path("strategy", opts), "%s", - opts->strategy); - if (opts->strategy_opts) - write_file(state_dir_path("strategy_opts", opts), "%s", - opts->strategy_opts); if (opts->allow_rerere_autoupdate > 0) write_file(state_dir_path("allow_rerere_autoupdate", opts), "-%s-rerere-autoupdate", diff --git a/sequencer.c b/sequencer.c index 886fbc3616..55b3ba3a51 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2946,7 +2946,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf) strbuf_reset(buf); if (!read_oneliner(buf, rebase_path_strategy(), 0)) return; - free(opts->strategy); opts->strategy = strbuf_detach(buf, NULL); if (!read_oneliner(buf, rebase_path_strategy_opts(), 0)) return; -- 2.39.2 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 2/4] rebase -m: cleanup --strategy-option handling 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood 2023-03-15 15:14 ` [PATCH 1/4] rebase: stop reading and writing unnecessary strategy state Phillip Wood @ 2023-03-15 15:14 ` Phillip Wood 2023-03-15 15:14 ` [PATCH 3/4] rebase -m: fix serialization of strategy options Phillip Wood ` (7 subsequent siblings) 9 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-03-15 15:14 UTC (permalink / raw) To: git; +Cc: Ævar Arnfjörð Bjarmason, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When handling "--strategy-option" rebase collects the commands into a struct string_list, then concatenates them into a string, prepending "--" to each one before splitting the string and removing the "--" prefix. This is an artifact of the scripted rebase and the need to support "rebase --preserve-merges". Now that "--preserve-merges" no-longer exists we can cleanup the way the argument is handled. The tests for a bad strategy option are adjusted now that parse_strategy_opts() is no-longer called when starting a rebase. The fact that it only errors out when running "git rebase --continue" is a mixed blessing but the next commit will fix the root cause of the parsing problem so lets not worry about that here. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 36 +++++++++++++++------------------- sequencer.c | 2 +- sequencer.h | 1 - t/t3436-rebase-more-options.sh | 10 ++++++++-- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 516ad1b12a..5194d64c24 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -116,7 +116,8 @@ struct rebase_options { struct string_list exec; int allow_empty_message; int rebase_merges, rebase_cousins; - char *strategy, *strategy_opts; + char *strategy; + struct string_list strategy_opts; struct strbuf git_format_patch_opt; int reschedule_failed_exec; int reapply_cherry_picks; @@ -142,6 +143,7 @@ struct rebase_options { .config_autosquash = -1, \ .update_refs = -1, \ .config_update_refs = -1, \ + .strategy_opts = STRING_LIST_INIT_NODUP \ } static struct replay_opts get_replay_opts(const struct rebase_options *opts) @@ -175,8 +177,14 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) replay.default_strategy = NULL; } - if (opts->strategy_opts) - parse_strategy_opts(&replay, opts->strategy_opts); + if (opts->strategy_opts.nr) { + ALLOC_ARRAY(replay.xopts, opts->strategy_opts.nr); + replay.xopts_nr = opts->strategy_opts.nr; + replay.xopts_alloc = opts->strategy_opts.nr; + for (size_t i = 0; i < opts->strategy_opts.nr; i++) + replay.xopts[i] = + xstrdup(opts->strategy_opts.items[i].string); + } if (opts->squash_onto) { oidcpy(&replay.squash_onto, opts->squash_onto); @@ -1012,7 +1020,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) int ignore_whitespace = 0; const char *gpg_sign = NULL; const char *rebase_merges = NULL; - struct string_list strategy_options = STRING_LIST_INIT_NODUP; struct object_id squash_onto; char *squash_onto_name = NULL; char *keep_base_onto_name = NULL; @@ -1121,7 +1128,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) N_("use 'merge-base --fork-point' to refine upstream")), OPT_STRING('s', "strategy", &options.strategy, N_("strategy"), N_("use the given merge strategy")), - OPT_STRING_LIST('X', "strategy-option", &strategy_options, + OPT_STRING_LIST('X', "strategy-option", &options.strategy_opts, N_("option"), N_("pass the argument through to the merge " "strategy")), @@ -1435,23 +1442,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } else { /* REBASE_MERGE */ if (ignore_whitespace) { - string_list_append(&strategy_options, + string_list_append(&options.strategy_opts, "ignore-space-change"); } } - if (strategy_options.nr) { - int i; - - if (!options.strategy) - options.strategy = "ort"; - - strbuf_reset(&buf); - for (i = 0; i < strategy_options.nr; i++) - strbuf_addf(&buf, " --%s", - strategy_options.items[i].string); - options.strategy_opts = xstrdup(buf.buf); - } + if (options.strategy_opts.nr && !options.strategy) + options.strategy = "ort"; if (options.strategy) { options.strategy = xstrdup(options.strategy); @@ -1826,10 +1823,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) free(options.gpg_sign_opt); string_list_clear(&options.exec, 0); free(options.strategy); - free(options.strategy_opts); + string_list_clear(&options.strategy_opts, 0); strbuf_release(&options.git_format_patch_opt); free(squash_onto_name); free(keep_base_onto_name); - string_list_clear(&strategy_options, 0); return !!ret; } diff --git a/sequencer.c b/sequencer.c index 55b3ba3a51..83ea1016ae 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2918,7 +2918,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) return 0; } -void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) +static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) { int i; int count; diff --git a/sequencer.h b/sequencer.h index 3bcdfa1b58..5de5cfa664 100644 --- a/sequencer.h +++ b/sequencer.h @@ -247,7 +247,6 @@ int read_oneliner(struct strbuf *buf, const char *path, unsigned flags); int read_author_script(const char *path, char **name, char **email, char **date, int allow_missing); -void parse_strategy_opts(struct replay_opts *opts, char *raw_opts); int write_basic_state(struct replay_opts *opts, const char *head_name, struct commit *onto, const struct object_id *orig_head); void sequencer_post_commit_cleanup(struct repository *r, int verbose); diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh index c3184c9ade..3adf42f47d 100755 --- a/t/t3436-rebase-more-options.sh +++ b/t/t3436-rebase-more-options.sh @@ -41,19 +41,25 @@ test_expect_success 'setup' ' ' test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' + test_when_finished "test_might_fail git rebase --abort" && cat >expect <<-\EOF && fatal: could not split '\''--bad'\'': unclosed quote EOF - test_expect_code 128 git rebase -X"bad argument\"" side main >out 2>actual && + GIT_SEQUENCE_EDITOR="echo break >" \ + git rebase -i -X"bad argument\"" side main && + test_expect_code 128 git rebase --continue >out 2>actual && test_must_be_empty out && test_cmp expect actual ' test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' + test_when_finished "test_might_fail git rebase --abort" && cat >expect <<-\EOF && fatal: could not split '\''--bad'\'': cmdline ends with \ EOF - test_expect_code 128 git rebase -X"bad escape \\" side main >out 2>actual && + GIT_SEQUENCE_EDITOR="echo break >" \ + git rebase -i -X"bad escape \\" side main && + test_expect_code 128 git rebase --continue >out 2>actual && test_must_be_empty out && test_cmp expect actual ' -- 2.39.2 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 3/4] rebase -m: fix serialization of strategy options 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood 2023-03-15 15:14 ` [PATCH 1/4] rebase: stop reading and writing unnecessary strategy state Phillip Wood 2023-03-15 15:14 ` [PATCH 2/4] rebase -m: cleanup --strategy-option handling Phillip Wood @ 2023-03-15 15:14 ` Phillip Wood 2023-04-01 19:27 ` Elijah Newren 2023-03-15 15:14 ` [PATCH 4/4] rebase: remove a couple of redundant strategy tests Phillip Wood ` (6 subsequent siblings) 9 siblings, 1 reply; 36+ messages in thread From: Phillip Wood @ 2023-03-15 15:14 UTC (permalink / raw) To: git; +Cc: Ævar Arnfjörð Bjarmason, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> To store the strategy options rebase prepends " --" to each one and writes them to a file. To load them it reads the file and passes the contents to split_cmdline(). This roughly mimics the behavior of the scripted rebase but has a couple of limitations, (1) options containing whitespace are not properly preserved (this is true of the scripted rebase as well) and (2) options containing '"' or '\' are incorrectly parsed and may cause the parser to return an error. Fix these limitations by quoting each option when they are stored so that they can be parsed correctly. Now that "--preserve-merges" no longer exist this change also stops prepending "--" to the options when they are stored as that was an artifact of the scripted rebase. These changes are backwards compatible so the files written by an older version of git can be still be read. They are also forwards compatible, the file can still be parsed by recent versions of git as they treat the "--" prefix as optional. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- sequencer.c | 23 +++++++++++++++++++---- t/t3418-rebase-continue.sh | 34 ++++++++++++++++++++++------------ t/t3436-rebase-more-options.sh | 24 ------------------------ 3 files changed, 41 insertions(+), 40 deletions(-) diff --git a/sequencer.c b/sequencer.c index 83ea1016ae..8890d1f7a1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2930,7 +2930,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) count = split_cmdline(strategy_opts_string, (const char ***)&opts->xopts); if (count < 0) - die(_("could not split '%s': %s"), strategy_opts_string, + BUG("could not split '%s': %s", strategy_opts_string, split_cmdline_strerror(count)); opts->xopts_nr = count; for (i = 0; i < opts->xopts_nr; i++) { @@ -3054,12 +3054,27 @@ static int read_populate_opts(struct replay_opts *opts) static void write_strategy_opts(struct replay_opts *opts) { - int i; struct strbuf buf = STRBUF_INIT; - for (i = 0; i < opts->xopts_nr; ++i) - strbuf_addf(&buf, " --%s", opts->xopts[i]); + /* + * Quote strategy options so that they can be read correctly + * by split_cmdline(). + */ + for (size_t i = 0; i < opts->xopts_nr; i++) { + char *arg = opts->xopts[i]; + if (i) + strbuf_addch(&buf, ' '); + strbuf_addch(&buf, '"'); + for (size_t j = 0; arg[j]; j++) { + const char c = arg[j]; + + if (c == '"' || c =='\\') + strbuf_addch(&buf, '\\'); + strbuf_addch(&buf, c); + } + strbuf_addch(&buf, '"'); + } write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); strbuf_release(&buf); } diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 130e2f9b55..42c3954125 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F2-on-topic-branch && test_commit "commit-new-file-F3-on-topic-branch" F3 32 && - test_when_finished "rm -fr test-bin funny.was.run" && + test_when_finished "rm -fr test-bin" && mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - case "\$1" in --opt) ;; *) exit 2 ;; esac - shift && - >funny.was.run && - exec git merge-recursive "\$@" + + write_script test-bin/git-merge-funny <<-\EOF && + printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual + shift 3 && + exec git merge-recursive "$@" EOF - chmod +x test-bin/git-merge-funny && + + cat >expect <<-\EOF && + [7] + [--option=arg with space] + [--op"tion\] + [--new + line ] + [--] + EOF + + rm -f actual && ( PATH=./test-bin:$PATH && - test_must_fail git rebase -s funny -Xopt main topic + test_must_fail git rebase -s funny -X"option=arg with space" \ + -Xop\"tion\\ -X"new${LF}line " main topic ) && - test -f funny.was.run && - rm funny.was.run && + test_cmp expect actual && + rm actual && echo "Resolved" >F2 && git add F2 && ( PATH=./test-bin:$PATH && git rebase --continue ) && - test -f funny.was.run + test_cmp expect actual ' test_expect_success 'rebase -i --continue handles merge strategy and options' ' diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh index 3adf42f47d..94671d3c46 100755 --- a/t/t3436-rebase-more-options.sh +++ b/t/t3436-rebase-more-options.sh @@ -40,30 +40,6 @@ test_expect_success 'setup' ' EOF ' -test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' - test_when_finished "test_might_fail git rebase --abort" && - cat >expect <<-\EOF && - fatal: could not split '\''--bad'\'': unclosed quote - EOF - GIT_SEQUENCE_EDITOR="echo break >" \ - git rebase -i -X"bad argument\"" side main && - test_expect_code 128 git rebase --continue >out 2>actual && - test_must_be_empty out && - test_cmp expect actual -' - -test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' - test_when_finished "test_might_fail git rebase --abort" && - cat >expect <<-\EOF && - fatal: could not split '\''--bad'\'': cmdline ends with \ - EOF - GIT_SEQUENCE_EDITOR="echo break >" \ - git rebase -i -X"bad escape \\" side main && - test_expect_code 128 git rebase --continue >out 2>actual && - test_must_be_empty out && - test_cmp expect actual -' - test_expect_success '--ignore-whitespace works with apply backend' ' test_must_fail git rebase --apply main side && git rebase --abort && -- 2.39.2 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 3/4] rebase -m: fix serialization of strategy options 2023-03-15 15:14 ` [PATCH 3/4] rebase -m: fix serialization of strategy options Phillip Wood @ 2023-04-01 19:27 ` Elijah Newren 2023-04-04 13:02 ` Phillip Wood 0 siblings, 1 reply; 36+ messages in thread From: Elijah Newren @ 2023-04-01 19:27 UTC (permalink / raw) To: Phillip Wood; +Cc: git, Ævar Arnfjörð Bjarmason, Phillip Wood On Wed, Mar 15, 2023 at 8:51 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > To store the strategy options rebase prepends " --" to each one and > writes them to a file. To load them it reads the file and passes the > contents to split_cmdline(). This roughly mimics the behavior of the > scripted rebase but has a couple of limitations, (1) options containing > whitespace are not properly preserved (this is true of the scripted > rebase as well) and (2) options containing '"' or '\' are incorrectly > parsed and may cause the parser to return an error. > > Fix these limitations by quoting each option when they are stored so > that they can be parsed correctly. Now that "--preserve-merges" no > longer exist this change also stops prepending "--" to the options when > they are stored as that was an artifact of the scripted rebase. > > These changes are backwards compatible so the files written by an older > version of git can be still be read. They are also forwards compatible, > the file can still be parsed by recent versions of git as they treat the > "--" prefix as optional. I'm not sure we want to tie ourselves to support completing a rebase with git version X+1 that was started with git version X. But, it's really nice that you're paying attention to that level of detail. :-) > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > sequencer.c | 23 +++++++++++++++++++---- > t/t3418-rebase-continue.sh | 34 ++++++++++++++++++++++------------ > t/t3436-rebase-more-options.sh | 24 ------------------------ > 3 files changed, 41 insertions(+), 40 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 83ea1016ae..8890d1f7a1 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2930,7 +2930,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) > count = split_cmdline(strategy_opts_string, > (const char ***)&opts->xopts); > if (count < 0) > - die(_("could not split '%s': %s"), strategy_opts_string, > + BUG("could not split '%s': %s", strategy_opts_string, > split_cmdline_strerror(count)); > opts->xopts_nr = count; > for (i = 0; i < opts->xopts_nr; i++) { > @@ -3054,12 +3054,27 @@ static int read_populate_opts(struct replay_opts *opts) > > static void write_strategy_opts(struct replay_opts *opts) > { > - int i; > struct strbuf buf = STRBUF_INIT; > > - for (i = 0; i < opts->xopts_nr; ++i) > - strbuf_addf(&buf, " --%s", opts->xopts[i]); > + /* > + * Quote strategy options so that they can be read correctly > + * by split_cmdline(). > + */ > + for (size_t i = 0; i < opts->xopts_nr; i++) { > + char *arg = opts->xopts[i]; > > + if (i) > + strbuf_addch(&buf, ' '); > + strbuf_addch(&buf, '"'); > + for (size_t j = 0; arg[j]; j++) { > + const char c = arg[j]; > + > + if (c == '"' || c =='\\') > + strbuf_addch(&buf, '\\'); > + strbuf_addch(&buf, c); > + } > + strbuf_addch(&buf, '"'); > + } Do we not have a function in quote.[ch] that can handle this? If not, should we add this code to a function in that file and call it? > write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); > strbuf_release(&buf); > } > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh > index 130e2f9b55..42c3954125 100755 > --- a/t/t3418-rebase-continue.sh > +++ b/t/t3418-rebase-continue.sh > @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' > rm -fr .git/rebase-* && > git reset --hard commit-new-file-F2-on-topic-branch && > test_commit "commit-new-file-F3-on-topic-branch" F3 32 && > - test_when_finished "rm -fr test-bin funny.was.run" && > + test_when_finished "rm -fr test-bin" && > mkdir test-bin && > - cat >test-bin/git-merge-funny <<-EOF && > - #!$SHELL_PATH > - case "\$1" in --opt) ;; *) exit 2 ;; esac > - shift && > - >funny.was.run && > - exec git merge-recursive "\$@" > + > + write_script test-bin/git-merge-funny <<-\EOF && > + printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual > + shift 3 && > + exec git merge-recursive "$@" > EOF > - chmod +x test-bin/git-merge-funny && > + > + cat >expect <<-\EOF && > + [7] > + [--option=arg with space] > + [--op"tion\] > + [--new > + line ] > + [--] > + EOF > + > + rm -f actual && > ( > PATH=./test-bin:$PATH && > - test_must_fail git rebase -s funny -Xopt main topic > + test_must_fail git rebase -s funny -X"option=arg with space" \ > + -Xop\"tion\\ -X"new${LF}line " main topic > ) && > - test -f funny.was.run && > - rm funny.was.run && > + test_cmp expect actual && > + rm actual && > echo "Resolved" >F2 && > git add F2 && > ( > PATH=./test-bin:$PATH && > git rebase --continue > ) && > - test -f funny.was.run > + test_cmp expect actual > ' I appreciate the more stringent test to cover these new special cases. :-) > > test_expect_success 'rebase -i --continue handles merge strategy and options' ' > diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh > index 3adf42f47d..94671d3c46 100755 > --- a/t/t3436-rebase-more-options.sh > +++ b/t/t3436-rebase-more-options.sh > @@ -40,30 +40,6 @@ test_expect_success 'setup' ' > EOF > ' > > -test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' > - test_when_finished "test_might_fail git rebase --abort" && > - cat >expect <<-\EOF && > - fatal: could not split '\''--bad'\'': unclosed quote > - EOF > - GIT_SEQUENCE_EDITOR="echo break >" \ > - git rebase -i -X"bad argument\"" side main && > - test_expect_code 128 git rebase --continue >out 2>actual && > - test_must_be_empty out && > - test_cmp expect actual > -' > - > -test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' > - test_when_finished "test_might_fail git rebase --abort" && > - cat >expect <<-\EOF && > - fatal: could not split '\''--bad'\'': cmdline ends with \ > - EOF > - GIT_SEQUENCE_EDITOR="echo break >" \ > - git rebase -i -X"bad escape \\" side main && > - test_expect_code 128 git rebase --continue >out 2>actual && > - test_must_be_empty out && > - test_cmp expect actual > -' > - > test_expect_success '--ignore-whitespace works with apply backend' ' > test_must_fail git rebase --apply main side && > git rebase --abort && > -- > 2.39.2 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/4] rebase -m: fix serialization of strategy options 2023-04-01 19:27 ` Elijah Newren @ 2023-04-04 13:02 ` Phillip Wood 0 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-04 13:02 UTC (permalink / raw) To: Elijah Newren, Phillip Wood; +Cc: git, Ævar Arnfjörð Bjarmason On 01/04/2023 20:27, Elijah Newren wrote: > On Wed, Mar 15, 2023 at 8:51 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> To store the strategy options rebase prepends " --" to each one and >> writes them to a file. To load them it reads the file and passes the >> contents to split_cmdline(). This roughly mimics the behavior of the >> scripted rebase but has a couple of limitations, (1) options containing >> whitespace are not properly preserved (this is true of the scripted >> rebase as well) and (2) options containing '"' or '\' are incorrectly >> parsed and may cause the parser to return an error. >> >> Fix these limitations by quoting each option when they are stored so >> that they can be parsed correctly. Now that "--preserve-merges" no >> longer exist this change also stops prepending "--" to the options when >> they are stored as that was an artifact of the scripted rebase. >> >> These changes are backwards compatible so the files written by an older >> version of git can be still be read. They are also forwards compatible, >> the file can still be parsed by recent versions of git as they treat the >> "--" prefix as optional. > > I'm not sure we want to tie ourselves to support completing a rebase > with git version X+1 that was started with git version X. But, it's > really nice that you're paying attention to that level of detail. :-) In the past we've had complaints when we've made backwards incompatible changes but I'm not sure we've ever promised never to do it. Part of the problem for users is that they may not be in control of when git gets upgraded. Also we've had reports where a macos user started a rebase with tig which used a bundled version of git that was different to what the user had available on the command line when the continued the rebase. >> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> sequencer.c | 23 +++++++++++++++++++---- >> t/t3418-rebase-continue.sh | 34 ++++++++++++++++++++++------------ >> t/t3436-rebase-more-options.sh | 24 ------------------------ >> 3 files changed, 41 insertions(+), 40 deletions(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index 83ea1016ae..8890d1f7a1 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -2930,7 +2930,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) >> count = split_cmdline(strategy_opts_string, >> (const char ***)&opts->xopts); >> if (count < 0) >> - die(_("could not split '%s': %s"), strategy_opts_string, >> + BUG("could not split '%s': %s", strategy_opts_string, >> split_cmdline_strerror(count)); >> opts->xopts_nr = count; >> for (i = 0; i < opts->xopts_nr; i++) { >> @@ -3054,12 +3054,27 @@ static int read_populate_opts(struct replay_opts *opts) >> >> static void write_strategy_opts(struct replay_opts *opts) >> { >> - int i; >> struct strbuf buf = STRBUF_INIT; >> >> - for (i = 0; i < opts->xopts_nr; ++i) >> - strbuf_addf(&buf, " --%s", opts->xopts[i]); >> + /* >> + * Quote strategy options so that they can be read correctly >> + * by split_cmdline(). >> + */ >> + for (size_t i = 0; i < opts->xopts_nr; i++) { >> + char *arg = opts->xopts[i]; >> >> + if (i) >> + strbuf_addch(&buf, ' '); >> + strbuf_addch(&buf, '"'); >> + for (size_t j = 0; arg[j]; j++) { >> + const char c = arg[j]; >> + >> + if (c == '"' || c =='\\') >> + strbuf_addch(&buf, '\\'); >> + strbuf_addch(&buf, c); >> + } >> + strbuf_addch(&buf, '"'); >> + } > > Do we not have a function in quote.[ch] that can handle this? If not, > should we add this code to a function in that file and call it? No we don't and yes we should, though in the end I've added it to alias.c as that is where split_cmdline() lives. Thanks for you comments, I'll post a re-roll in the next couple of days Phillip > >> write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); >> strbuf_release(&buf); >> } >> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh >> index 130e2f9b55..42c3954125 100755 >> --- a/t/t3418-rebase-continue.sh >> +++ b/t/t3418-rebase-continue.sh >> @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' >> rm -fr .git/rebase-* && >> git reset --hard commit-new-file-F2-on-topic-branch && >> test_commit "commit-new-file-F3-on-topic-branch" F3 32 && >> - test_when_finished "rm -fr test-bin funny.was.run" && >> + test_when_finished "rm -fr test-bin" && >> mkdir test-bin && >> - cat >test-bin/git-merge-funny <<-EOF && >> - #!$SHELL_PATH >> - case "\$1" in --opt) ;; *) exit 2 ;; esac >> - shift && >> - >funny.was.run && >> - exec git merge-recursive "\$@" >> + >> + write_script test-bin/git-merge-funny <<-\EOF && >> + printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual >> + shift 3 && >> + exec git merge-recursive "$@" >> EOF >> - chmod +x test-bin/git-merge-funny && >> + >> + cat >expect <<-\EOF && >> + [7] >> + [--option=arg with space] >> + [--op"tion\] >> + [--new >> + line ] >> + [--] >> + EOF >> + >> + rm -f actual && >> ( >> PATH=./test-bin:$PATH && >> - test_must_fail git rebase -s funny -Xopt main topic >> + test_must_fail git rebase -s funny -X"option=arg with space" \ >> + -Xop\"tion\\ -X"new${LF}line " main topic >> ) && >> - test -f funny.was.run && >> - rm funny.was.run && >> + test_cmp expect actual && >> + rm actual && >> echo "Resolved" >F2 && >> git add F2 && >> ( >> PATH=./test-bin:$PATH && >> git rebase --continue >> ) && >> - test -f funny.was.run >> + test_cmp expect actual >> ' > > I appreciate the more stringent test to cover these new special cases. :-) > >> >> test_expect_success 'rebase -i --continue handles merge strategy and options' ' >> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh >> index 3adf42f47d..94671d3c46 100755 >> --- a/t/t3436-rebase-more-options.sh >> +++ b/t/t3436-rebase-more-options.sh >> @@ -40,30 +40,6 @@ test_expect_success 'setup' ' >> EOF >> ' >> >> -test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' >> - test_when_finished "test_might_fail git rebase --abort" && >> - cat >expect <<-\EOF && >> - fatal: could not split '\''--bad'\'': unclosed quote >> - EOF >> - GIT_SEQUENCE_EDITOR="echo break >" \ >> - git rebase -i -X"bad argument\"" side main && >> - test_expect_code 128 git rebase --continue >out 2>actual && >> - test_must_be_empty out && >> - test_cmp expect actual >> -' >> - >> -test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' >> - test_when_finished "test_might_fail git rebase --abort" && >> - cat >expect <<-\EOF && >> - fatal: could not split '\''--bad'\'': cmdline ends with \ >> - EOF >> - GIT_SEQUENCE_EDITOR="echo break >" \ >> - git rebase -i -X"bad escape \\" side main && >> - test_expect_code 128 git rebase --continue >out 2>actual && >> - test_must_be_empty out && >> - test_cmp expect actual >> -' >> - >> test_expect_success '--ignore-whitespace works with apply backend' ' >> test_must_fail git rebase --apply main side && >> git rebase --abort && >> -- >> 2.39.2 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 4/4] rebase: remove a couple of redundant strategy tests 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood ` (2 preceding siblings ...) 2023-03-15 15:14 ` [PATCH 3/4] rebase -m: fix serialization of strategy options Phillip Wood @ 2023-03-15 15:14 ` Phillip Wood 2023-04-01 19:31 ` Elijah Newren 2023-03-15 16:03 ` [PATCH 0/4] rebase: cleanup merge strategy option handling Junio C Hamano ` (5 subsequent siblings) 9 siblings, 1 reply; 36+ messages in thread From: Phillip Wood @ 2023-03-15 15:14 UTC (permalink / raw) To: git; +Cc: Ævar Arnfjörð Bjarmason, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The test removed in t3402 has been redundant ever since 80ff47957b (rebase: remember strategy and strategy options, 2011-02-06) which added a new test the first part of which (as noted in the commit message) duplicated the existing test. The test removed in t3418 has been redundant since the merge backend was removed in 68aa495b59 (rebase: implement --merge via the interactive machinery, 2018-12-11) as it now tests the same code paths as the preceding test. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- t/t3402-rebase-merge.sh | 21 --------------------- t/t3418-rebase-continue.sh | 32 -------------------------------- 2 files changed, 53 deletions(-) diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh index 7e46f4ca85..79b0640c00 100755 --- a/t/t3402-rebase-merge.sh +++ b/t/t3402-rebase-merge.sh @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' ' esac ' -test_expect_success 'rebase -s funny -Xopt' ' - test_when_finished "rm -fr test-bin funny.was.run" && - mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - case "\$1" in --opt) ;; *) exit 2 ;; esac - shift && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && - git reset --hard && - git checkout -b test-funny main^ && - test_commit funny && - ( - PATH=./test-bin:$PATH && - git rebase -s funny -Xopt main - ) && - test -f funny.was.run -' - test_expect_success 'rebase --skip works with two conflicts in a row' ' git checkout second-side && tr "[A-Z]" "[a-z]" <newfile >tmp && diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 42c3954125..2d0789e554 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' test_cmp expect actual ' -test_expect_success 'rebase -i --continue handles merge strategy and options' ' - rm -fr .git/rebase-* && - git reset --hard commit-new-file-F2-on-topic-branch && - test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && - test_when_finished "rm -fr test-bin funny.was.run funny.args" && - mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - echo "\$@" >>funny.args - case "\$1" in --opt) ;; *) exit 2 ;; esac - case "\$2" in --foo) ;; *) exit 2 ;; esac - case "\$4" in --) ;; *) exit 2 ;; esac - shift 2 && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && - ( - PATH=./test-bin:$PATH && - test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic - ) && - test -f funny.was.run && - rm funny.was.run && - echo "Resolved" >F2 && - git add F2 && - ( - PATH=./test-bin:$PATH && - git rebase --continue - ) && - test -f funny.was.run -' - test_expect_success 'rebase -r passes merge strategy options correctly' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F3-on-topic-branch && -- 2.39.2 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 4/4] rebase: remove a couple of redundant strategy tests 2023-03-15 15:14 ` [PATCH 4/4] rebase: remove a couple of redundant strategy tests Phillip Wood @ 2023-04-01 19:31 ` Elijah Newren 2023-04-04 13:04 ` Phillip Wood 0 siblings, 1 reply; 36+ messages in thread From: Elijah Newren @ 2023-04-01 19:31 UTC (permalink / raw) To: Phillip Wood; +Cc: git, Ævar Arnfjörð Bjarmason, Phillip Wood On Wed, Mar 15, 2023 at 8:38 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > The test removed in t3402 has been redundant ever since 80ff47957b > (rebase: remember strategy and strategy options, 2011-02-06) which added > a new test the first part of which (as noted in the commit message) > duplicated the existing test. The test removed in t3418 has been > redundant since the merge backend was removed in 68aa495b59 (rebase: > implement --merge via the interactive machinery, 2018-12-11) as it now > tests the same code paths as the preceding test. I was really confused by this commit message at first. Eventually, I figured out you were talking about the changes in this commit, just in past tense. Could we change it to imperative tense? E.g. Remove a test in t3402 that has been redundant ever since 80ff47957b (rebase: remember strategy and strategy options, 2011-02-06). That commit added a new test, the first part of which (as noted in the old commit message) duplicated an existing test. Also remove a test t3418 that has been redundant since the merge backend was removed in 68aa495b59 (rebase: implement --merge via the interactive machinery, 2018-12-11), since it now tests the same code paths as the preceding test. > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > t/t3402-rebase-merge.sh | 21 --------------------- > t/t3418-rebase-continue.sh | 32 -------------------------------- > 2 files changed, 53 deletions(-) > > diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh > index 7e46f4ca85..79b0640c00 100755 > --- a/t/t3402-rebase-merge.sh > +++ b/t/t3402-rebase-merge.sh > @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' ' > esac > ' > > -test_expect_success 'rebase -s funny -Xopt' ' > - test_when_finished "rm -fr test-bin funny.was.run" && > - mkdir test-bin && > - cat >test-bin/git-merge-funny <<-EOF && > - #!$SHELL_PATH > - case "\$1" in --opt) ;; *) exit 2 ;; esac > - shift && > - >funny.was.run && > - exec git merge-recursive "\$@" > - EOF > - chmod +x test-bin/git-merge-funny && > - git reset --hard && > - git checkout -b test-funny main^ && > - test_commit funny && > - ( > - PATH=./test-bin:$PATH && > - git rebase -s funny -Xopt main > - ) && > - test -f funny.was.run > -' > - > test_expect_success 'rebase --skip works with two conflicts in a row' ' > git checkout second-side && > tr "[A-Z]" "[a-z]" <newfile >tmp && > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh > index 42c3954125..2d0789e554 100755 > --- a/t/t3418-rebase-continue.sh > +++ b/t/t3418-rebase-continue.sh > @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' > test_cmp expect actual > ' > > -test_expect_success 'rebase -i --continue handles merge strategy and options' ' > - rm -fr .git/rebase-* && > - git reset --hard commit-new-file-F2-on-topic-branch && > - test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && > - test_when_finished "rm -fr test-bin funny.was.run funny.args" && > - mkdir test-bin && > - cat >test-bin/git-merge-funny <<-EOF && > - #!$SHELL_PATH > - echo "\$@" >>funny.args > - case "\$1" in --opt) ;; *) exit 2 ;; esac > - case "\$2" in --foo) ;; *) exit 2 ;; esac > - case "\$4" in --) ;; *) exit 2 ;; esac > - shift 2 && > - >funny.was.run && > - exec git merge-recursive "\$@" > - EOF > - chmod +x test-bin/git-merge-funny && > - ( > - PATH=./test-bin:$PATH && > - test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic > - ) && > - test -f funny.was.run && > - rm funny.was.run && > - echo "Resolved" >F2 && > - git add F2 && > - ( > - PATH=./test-bin:$PATH && > - git rebase --continue > - ) && > - test -f funny.was.run > -' > - > test_expect_success 'rebase -r passes merge strategy options correctly' ' > rm -fr .git/rebase-* && > git reset --hard commit-new-file-F3-on-topic-branch && > -- > 2.39.2 Looks good otherwise. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 4/4] rebase: remove a couple of redundant strategy tests 2023-04-01 19:31 ` Elijah Newren @ 2023-04-04 13:04 ` Phillip Wood 0 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-04 13:04 UTC (permalink / raw) To: Elijah Newren, Phillip Wood; +Cc: git, Ævar Arnfjörð Bjarmason On 01/04/2023 20:31, Elijah Newren wrote: > On Wed, Mar 15, 2023 at 8:38 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> The test removed in t3402 has been redundant ever since 80ff47957b >> (rebase: remember strategy and strategy options, 2011-02-06) which added >> a new test the first part of which (as noted in the commit message) >> duplicated the existing test. The test removed in t3418 has been >> redundant since the merge backend was removed in 68aa495b59 (rebase: >> implement --merge via the interactive machinery, 2018-12-11) as it now >> tests the same code paths as the preceding test. > > I was really confused by this commit message at first. Eventually, I > figured out you were talking about the changes in this commit, just in > past tense. Could we change it to imperative tense? E.g. > > Remove a test in t3402 that has been redundant ever since 80ff47957b > (rebase: remember strategy and strategy options, 2011-02-06). That commit added > a new test, the first part of which (as noted in the old commit message) > duplicated an existing test. > > Also remove a test t3418 that has been redundant since the merge > backend was removed in 68aa495b59 (rebase: implement --merge > via the interactive machinery, 2018-12-11), since it now tests the > same code paths as the preceding test. Thanks, that is clearer, I'll use your suggested wording when I re-roll Best Wishes Phillip >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> t/t3402-rebase-merge.sh | 21 --------------------- >> t/t3418-rebase-continue.sh | 32 -------------------------------- >> 2 files changed, 53 deletions(-) >> >> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh >> index 7e46f4ca85..79b0640c00 100755 >> --- a/t/t3402-rebase-merge.sh >> +++ b/t/t3402-rebase-merge.sh >> @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' ' >> esac >> ' >> >> -test_expect_success 'rebase -s funny -Xopt' ' >> - test_when_finished "rm -fr test-bin funny.was.run" && >> - mkdir test-bin && >> - cat >test-bin/git-merge-funny <<-EOF && >> - #!$SHELL_PATH >> - case "\$1" in --opt) ;; *) exit 2 ;; esac >> - shift && >> - >funny.was.run && >> - exec git merge-recursive "\$@" >> - EOF >> - chmod +x test-bin/git-merge-funny && >> - git reset --hard && >> - git checkout -b test-funny main^ && >> - test_commit funny && >> - ( >> - PATH=./test-bin:$PATH && >> - git rebase -s funny -Xopt main >> - ) && >> - test -f funny.was.run >> -' >> - >> test_expect_success 'rebase --skip works with two conflicts in a row' ' >> git checkout second-side && >> tr "[A-Z]" "[a-z]" <newfile >tmp && >> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh >> index 42c3954125..2d0789e554 100755 >> --- a/t/t3418-rebase-continue.sh >> +++ b/t/t3418-rebase-continue.sh >> @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' >> test_cmp expect actual >> ' >> >> -test_expect_success 'rebase -i --continue handles merge strategy and options' ' >> - rm -fr .git/rebase-* && >> - git reset --hard commit-new-file-F2-on-topic-branch && >> - test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && >> - test_when_finished "rm -fr test-bin funny.was.run funny.args" && >> - mkdir test-bin && >> - cat >test-bin/git-merge-funny <<-EOF && >> - #!$SHELL_PATH >> - echo "\$@" >>funny.args >> - case "\$1" in --opt) ;; *) exit 2 ;; esac >> - case "\$2" in --foo) ;; *) exit 2 ;; esac >> - case "\$4" in --) ;; *) exit 2 ;; esac >> - shift 2 && >> - >funny.was.run && >> - exec git merge-recursive "\$@" >> - EOF >> - chmod +x test-bin/git-merge-funny && >> - ( >> - PATH=./test-bin:$PATH && >> - test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic >> - ) && >> - test -f funny.was.run && >> - rm funny.was.run && >> - echo "Resolved" >F2 && >> - git add F2 && >> - ( >> - PATH=./test-bin:$PATH && >> - git rebase --continue >> - ) && >> - test -f funny.was.run >> -' >> - >> test_expect_success 'rebase -r passes merge strategy options correctly' ' >> rm -fr .git/rebase-* && >> git reset --hard commit-new-file-F3-on-topic-branch && >> -- >> 2.39.2 > > Looks good otherwise. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/4] rebase: cleanup merge strategy option handling 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood ` (3 preceding siblings ...) 2023-03-15 15:14 ` [PATCH 4/4] rebase: remove a couple of redundant strategy tests Phillip Wood @ 2023-03-15 16:03 ` Junio C Hamano 2023-03-15 16:50 ` Felipe Contreras ` (4 subsequent siblings) 9 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2023-03-15 16:03 UTC (permalink / raw) To: Phillip Wood; +Cc: git, Ævar Arnfjörð Bjarmason, Phillip Wood Phillip Wood <phillip.wood123@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Cleanup the handling of --strategy-option now that we no longer need > to support "--preserve-merges" and properly quote the argument when > saving it to disc. Nice. > > These patches are based on a merge of 'master' and > 'ab/fix-strategy-opts-parsing' > > Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv1 > View-Changes-At: https://github.com/phillipwood/git/compare/c2e329a52...3e02eeff7 > Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v1 > > Phillip Wood (4): > rebase: stop reading and writing unnecessary strategy state > rebase -m: cleanup --strategy-option handling > rebase -m: fix serialization of strategy options > rebase: remove a couple of redundant strategy tests > > builtin/rebase.c | 60 +++++++++----------------------- > sequencer.c | 26 ++++++++++---- > sequencer.h | 1 - > t/t3402-rebase-merge.sh | 21 ------------ > t/t3418-rebase-continue.sh | 62 +++++++++++----------------------- > t/t3436-rebase-more-options.sh | 18 ---------- > 6 files changed, 56 insertions(+), 132 deletions(-) ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/4] rebase: cleanup merge strategy option handling 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood ` (4 preceding siblings ...) 2023-03-15 16:03 ` [PATCH 0/4] rebase: cleanup merge strategy option handling Junio C Hamano @ 2023-03-15 16:50 ` Felipe Contreras 2023-04-01 19:32 ` Elijah Newren ` (3 subsequent siblings) 9 siblings, 0 replies; 36+ messages in thread From: Felipe Contreras @ 2023-03-15 16:50 UTC (permalink / raw) To: Phillip Wood; +Cc: git, Ævar Arnfjörð Bjarmason On Wed, Mar 15, 2023 at 9:39 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Cleanup the handling of --strategy-option now that we no longer need > to support "--preserve-merges" and properly quote the argument when > saving it to disc. > > These patches are based on a merge of 'master' and > 'ab/fix-strategy-opts-parsing' > > Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv1 > View-Changes-At: https://github.com/phillipwood/git/compare/c2e329a52...3e02eeff7 > Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v1 FWIW I reviewed the changes and they all look good to me. -- Felipe Contreras ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/4] rebase: cleanup merge strategy option handling 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood ` (5 preceding siblings ...) 2023-03-15 16:50 ` Felipe Contreras @ 2023-04-01 19:32 ` Elijah Newren 2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood ` (2 subsequent siblings) 9 siblings, 0 replies; 36+ messages in thread From: Elijah Newren @ 2023-04-01 19:32 UTC (permalink / raw) To: Phillip Wood; +Cc: git, Ævar Arnfjörð Bjarmason On Wed, Mar 15, 2023 at 8:39 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Cleanup the handling of --strategy-option now that we no longer need > to support "--preserve-merges" and properly quote the argument when > saving it to disc. Nice! Thanks for taking the time to do these kinds of cleanups. Overall the code looks pretty good, but I left a few comments/questions on patches 3 & 4. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 0/5] rebase: cleanup merge strategy option handling 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood ` (6 preceding siblings ...) 2023-04-01 19:32 ` Elijah Newren @ 2023-04-05 15:21 ` Phillip Wood 2023-04-05 15:21 ` [PATCH v2 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood ` (5 more replies) 2023-04-07 13:55 ` [PATCH v3 " Phillip Wood 2023-04-10 9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood 9 siblings, 6 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-05 15:21 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin From: Phillip Wood <phillip.wood@dunelm.org.uk> Cleanup the handling of --strategy-option now that we no longer need to support "--preserve-merges" and properly quote the argument when saving it to disc. Thanks to Elijah for his comments on V1. Changes since V1: I've rebased these patches onto 'master' to avoid conflicts with 'sg/parse-options-h-initializers' in the new patch 2 (this series depends on 'ab/fix-strategy-opts-parsing' but that has now been merged to master). Patch 1 - Unchanged. Patch 2 - New patch to store the merge strategy options in an "struct strvec". This patch also introduces a new macro OPT_STRVEC() to collect options into an "struct strvec". Patch 3 - Small simplification due to the changes in patch 2. Patch 4 - Moved the code to quote a list so it can split by split_cmdline() into a new function quote_cmdline() as suggested by Elijah. Patch 5 - Reworded the commit message as suggested by Elijah. Base-Commit: 140b9478dad5d19543c1cb4fd293ccec228f1240 Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv2 View-Changes-At: https://github.com/phillipwood/git/compare/140b9478d...3515c31b4 Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v2 Phillip Wood (5): rebase: stop reading and writing unnecessary strategy state sequencer: use struct strvec to store merge strategy options rebase -m: cleanup --strategy-option handling rebase -m: fix serialization of strategy options rebase: remove a couple of redundant strategy tests alias.c | 18 +++++++ alias.h | 3 ++ builtin/rebase.c | 54 ++++----------------- builtin/revert.c | 20 ++------ parse-options-cb.c | 16 +++++++ parse-options.h | 10 ++++ sequencer.c | 57 ++++++++++------------ sequencer.h | 12 +++-- t/t3402-rebase-merge.sh | 21 -------- t/t3418-rebase-continue.sh | 88 +++++++++++++--------------------- t/t3436-rebase-more-options.sh | 18 ------- 11 files changed, 126 insertions(+), 191 deletions(-) Range-diff against v1: 1: 029d0bf2b6 = 1: 2353c753f5 rebase: stop reading and writing unnecessary strategy state -: ---------- > 2: dd7b82cdd5 sequencer: use struct strvec to store merge strategy options 2: a01b182644 ! 3: ccef0e6f4b rebase -m: cleanup --strategy-option handling @@ builtin/rebase.c: struct rebase_options { .config_autosquash = -1, \ .update_refs = -1, \ .config_update_refs = -1, \ -+ .strategy_opts = STRING_LIST_INIT_NODUP \ ++ .strategy_opts = STRING_LIST_INIT_NODUP,\ } static struct replay_opts get_replay_opts(const struct rebase_options *opts) @@ builtin/rebase.c: static struct replay_opts get_replay_opts(const struct rebase_ - if (opts->strategy_opts) - parse_strategy_opts(&replay, opts->strategy_opts); -+ if (opts->strategy_opts.nr) { -+ ALLOC_ARRAY(replay.xopts, opts->strategy_opts.nr); -+ replay.xopts_nr = opts->strategy_opts.nr; -+ replay.xopts_alloc = opts->strategy_opts.nr; -+ for (size_t i = 0; i < opts->strategy_opts.nr; i++) -+ replay.xopts[i] = -+ xstrdup(opts->strategy_opts.items[i].string); -+ } ++ for (size_t i = 0; i < opts->strategy_opts.nr; i++) ++ strvec_push(&replay.xopts, opts->strategy_opts.items[i].string); if (opts->squash_onto) { oidcpy(&replay.squash_onto, opts->squash_onto); 3: 9af68cb065 ! 4: 9a90212ef2 rebase -m: fix serialization of strategy options @@ Commit message Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> + ## alias.c ## +@@ + #include "alloc.h" + #include "config.h" + #include "gettext.h" ++#include "strbuf.h" + #include "string-list.h" + + struct config_alias_data { +@@ alias.c: void list_aliases(struct string_list *list) + read_early_config(config_alias_cb, &data); + } + ++void quote_cmdline(struct strbuf *buf, const char **argv) ++{ ++ for (const char **argp = argv; *argp; argp++) { ++ if (argp != argv) ++ strbuf_addch(buf, ' '); ++ strbuf_addch(buf, '"'); ++ for (const char *p = *argp; *p; p++) { ++ const char c = *p; ++ ++ if (c == '"' || c =='\\') ++ strbuf_addch(buf, '\\'); ++ strbuf_addch(buf, c); ++ } ++ strbuf_addch(buf, '"'); ++ } ++} ++ + #define SPLIT_CMDLINE_BAD_ENDING 1 + #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 + #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 + + ## alias.h ## +@@ + #ifndef ALIAS_H + #define ALIAS_H + ++struct strbuf; + struct string_list; + + char *alias_lookup(const char *alias); ++/* Quote argv so buf can be parsed by split_cmdline() */ ++void quote_cmdline(struct strbuf *buf, const char **argv); + int split_cmdline(char *cmdline, const char ***argv); + /* Takes a negative value returned by split_cmdline */ + const char *split_cmdline_strerror(int cmdline_errno); + ## sequencer.c ## @@ sequencer.c: static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) - count = split_cmdline(strategy_opts_string, - (const char ***)&opts->xopts); + + count = split_cmdline(strategy_opts_string, &argv); if (count < 0) - die(_("could not split '%s': %s"), strategy_opts_string, + BUG("could not split '%s': %s", strategy_opts_string, split_cmdline_strerror(count)); - opts->xopts_nr = count; - for (i = 0; i < opts->xopts_nr; i++) { + for (i = 0; i < count; i++) { + const char *arg = argv[i]; @@ sequencer.c: static int read_populate_opts(struct replay_opts *opts) static void write_strategy_opts(struct replay_opts *opts) { - int i; struct strbuf buf = STRBUF_INIT; -- for (i = 0; i < opts->xopts_nr; ++i) -- strbuf_addf(&buf, " --%s", opts->xopts[i]); +- for (i = 0; i < opts->xopts.nr; ++i) +- strbuf_addf(&buf, " --%s", opts->xopts.v[i]); - + /* + * Quote strategy options so that they can be read correctly + * by split_cmdline(). + */ -+ for (size_t i = 0; i < opts->xopts_nr; i++) { -+ char *arg = opts->xopts[i]; -+ -+ if (i) -+ strbuf_addch(&buf, ' '); -+ strbuf_addch(&buf, '"'); -+ for (size_t j = 0; arg[j]; j++) { -+ const char c = arg[j]; -+ -+ if (c == '"' || c =='\\') -+ strbuf_addch(&buf, '\\'); -+ strbuf_addch(&buf, c); -+ } -+ strbuf_addch(&buf, '"'); -+ } ++ quote_cmdline(&buf, opts->xopts.v); write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); strbuf_release(&buf); } 4: 3e02eeff78 ! 5: 3515c31b40 rebase: remove a couple of redundant strategy tests @@ Metadata ## Commit message ## rebase: remove a couple of redundant strategy tests - The test removed in t3402 has been redundant ever since 80ff47957b - (rebase: remember strategy and strategy options, 2011-02-06) which added - a new test the first part of which (as noted in the commit message) - duplicated the existing test. The test removed in t3418 has been - redundant since the merge backend was removed in 68aa495b59 (rebase: - implement --merge via the interactive machinery, 2018-12-11) as it now - tests the same code paths as the preceding test. - + Remove a test in t3402 that has been redundant ever since 80ff47957b + (rebase: remember strategy and strategy options, 2011-02-06). That + commit added a new test, the first part of which (as noted in the old + commit message) duplicated an existing test. + + Also remove a test t3418 that has been redundant since the merge backend + was removed in 68aa495b59 (rebase: implement --merge via the interactive + machinery, 2018-12-11), since it now tests the same code paths as the + preceding test. + + Helped-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> ## t/t3402-rebase-merge.sh ## -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/5] rebase: stop reading and writing unnecessary strategy state 2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood @ 2023-04-05 15:21 ` Phillip Wood 2023-04-05 15:21 ` [PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood ` (4 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-05 15:21 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The state files for "--strategy" and "--strategy-option" are written and read twice, once by builtin/rebase.c and then by sequencer.c. This is an artifact of the scripted rebase and the need to support "rebase --preserve-merges". Now that "--preserve-merges" no-longer exists we only need to read and write these files in sequencer.c. This enables us to remove a call to free() in read_strategy_opts() that was added by f1f4ebf432 (sequencer.c: fix "opts->strategy" leak in read_strategy_opts(), 2022-11-08) as this commit fixes the root cause of that leak. There is further scope for removing duplication in the reading and writing of state files between builtin/rebase.c and sequencer.c but that is left for a follow up series. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 24 ------------------------ sequencer.c | 1 - 2 files changed, 25 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 5b7b908b66..3bd215c771 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -483,24 +483,6 @@ static int read_basic_state(struct rebase_options *opts) opts->gpg_sign_opt = xstrdup(buf.buf); } - if (file_exists(state_dir_path("strategy", opts))) { - strbuf_reset(&buf); - if (!read_oneliner(&buf, state_dir_path("strategy", opts), - READ_ONELINER_WARN_MISSING)) - return -1; - free(opts->strategy); - opts->strategy = xstrdup(buf.buf); - } - - if (file_exists(state_dir_path("strategy_opts", opts))) { - strbuf_reset(&buf); - if (!read_oneliner(&buf, state_dir_path("strategy_opts", opts), - READ_ONELINER_WARN_MISSING)) - return -1; - free(opts->strategy_opts); - opts->strategy_opts = xstrdup(buf.buf); - } - strbuf_release(&buf); return 0; @@ -518,12 +500,6 @@ static int rebase_write_basic_state(struct rebase_options *opts) write_file(state_dir_path("quiet", opts), "%s", ""); if (opts->flags & REBASE_VERBOSE) write_file(state_dir_path("verbose", opts), "%s", ""); - if (opts->strategy) - write_file(state_dir_path("strategy", opts), "%s", - opts->strategy); - if (opts->strategy_opts) - write_file(state_dir_path("strategy_opts", opts), "%s", - opts->strategy_opts); if (opts->allow_rerere_autoupdate > 0) write_file(state_dir_path("allow_rerere_autoupdate", opts), "-%s-rerere-autoupdate", diff --git a/sequencer.c b/sequencer.c index 3be23d7ca2..c35a67e104 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2944,7 +2944,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf) strbuf_reset(buf); if (!read_oneliner(buf, rebase_path_strategy(), 0)) return; - free(opts->strategy); opts->strategy = strbuf_detach(buf, NULL); if (!read_oneliner(buf, rebase_path_strategy_opts(), 0)) return; -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options 2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood 2023-04-05 15:21 ` [PATCH v2 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood @ 2023-04-05 15:21 ` Phillip Wood 2023-04-07 5:44 ` Elijah Newren 2023-04-05 15:21 ` [PATCH v2 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood ` (3 subsequent siblings) 5 siblings, 1 reply; 36+ messages in thread From: Phillip Wood @ 2023-04-05 15:21 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The sequencer stores the merge strategy options in an array of strings which allocated with ALLOC_GROW(). Using "struct strvec" avoids manually managing the memory of that array and simplifies the code. Aside from memory allocation the changes to the sequencer are largely mechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A new option parsing macro OPT_STRVEC() is also added to collect the strategy options. Hopefully this can be used to simplify the code in builtin/merge.c in the future. Note that there is a change of behavior to "git cherry-pick" and "git revert" as passing “--no-strategy-option” will now clear any previous strategy options whereas before this change it did nothing. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/revert.c | 20 +++----------------- parse-options-cb.c | 16 ++++++++++++++++ parse-options.h | 10 ++++++++++ sequencer.c | 47 ++++++++++++++++++++-------------------------- sequencer.h | 11 ++++++++--- 5 files changed, 57 insertions(+), 47 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 62986a7b1b..362857da65 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts) return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage; } -static int option_parse_x(const struct option *opt, - const char *arg, int unset) -{ - struct replay_opts **opts_ptr = opt->value; - struct replay_opts *opts = *opts_ptr; - - if (unset) - return 0; - - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); - opts->xopts[opts->xopts_nr++] = xstrdup(arg); - return 0; -} - static int option_parse_m(const struct option *opt, const char *arg, int unset) { @@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) N_("select mainline parent"), option_parse_m), OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto), OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")), - OPT_CALLBACK('X', "strategy-option", &opts, N_("option"), - N_("option for merge strategy"), option_parse_x), + OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"), + N_("option for merge strategy")), { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, OPT_END() @@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) "--signoff", opts->signoff, "--mainline", opts->mainline, "--strategy", opts->strategy ? 1 : 0, - "--strategy-option", opts->xopts ? 1 : 0, + "--strategy-option", opts->xopts.nr ? 1 : 0, "-x", opts->record_origin, "--ff", opts->allow_ff, "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, diff --git a/parse-options-cb.c b/parse-options-cb.c index d346dbe210..8eb96c5ca9 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset) return 0; } +int parse_opt_strvec(const struct option *opt, const char *arg, int unset) +{ + struct strvec *v = opt->value; + + if (unset) { + strvec_clear(v); + return 0; + } + + if (!arg) + return -1; + + strvec_push(v, arg); + return 0; +} + int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset) { return 0; diff --git a/parse-options.h b/parse-options.h index 26f19384e5..2d1d4d052f 100644 --- a/parse-options.h +++ b/parse-options.h @@ -285,6 +285,15 @@ struct option { .help = (h), \ .callback = &parse_opt_string_list, \ } +#define OPT_STRVEC(s, l, v, a, h) { \ + .type = OPTION_CALLBACK, \ + .short_name = (s), \ + .long_name = (l), \ + .value = (v), \ + .argh = (a), \ + .help = (h), \ + .callback = &parse_opt_strvec, \ +} #define OPT_UYN(s, l, v, h) { \ .type = OPTION_CALLBACK, \ .short_name = (s), \ @@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int); int parse_opt_commit(const struct option *, const char *, int); int parse_opt_tertiary(const struct option *, const char *, int); int parse_opt_string_list(const struct option *, const char *, int); +int parse_opt_strvec(const struct option *, const char *, int); int parse_opt_noop_cb(const struct option *, const char *, int); enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, diff --git a/sequencer.c b/sequencer.c index c35a67e104..6e2f3357c8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts) free(opts->reflog_action); free(opts->default_strategy); free(opts->strategy); - for (size_t i = 0; i < opts->xopts_nr; i++) - free(opts->xopts[i]); - free(opts->xopts); + strvec_clear (&opts->xopts); strbuf_release(&opts->current_fixups); if (opts->revs) release_revisions(opts->revs); @@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r, next_tree = next ? get_commit_tree(next) : empty_tree(r); base_tree = base ? get_commit_tree(base) : empty_tree(r); - for (i = 0; i < opts->xopts_nr; i++) - parse_merge_opt(&o, opts->xopts[i]); + for (i = 0; i < opts->xopts.nr; i++) + parse_merge_opt(&o, opts->xopts.v[i]); if (!opts->strategy || !strcmp(opts->strategy, "ort")) { memset(&result, 0, sizeof(result)); @@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r, commit_list_insert(base, &common); commit_list_insert(next, &remotes); res |= try_merge_command(r, opts->strategy, - opts->xopts_nr, (const char **)opts->xopts, + opts->xopts.nr, opts->xopts.v, common, oid_to_hex(&head), remotes); free_commit_list(common); free_commit_list(remotes); @@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) else if (!strcmp(key, "options.gpg-sign")) git_config_string_dup(&opts->gpg_sign, key, value); else if (!strcmp(key, "options.strategy-option")) { - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); - opts->xopts[opts->xopts_nr++] = xstrdup(value); + strvec_push(&opts->xopts, value); } else if (!strcmp(key, "options.allow-rerere-auto")) opts->allow_rerere_auto = git_config_bool_or_int(key, value, &error_flag) ? @@ -2920,22 +2917,21 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) { int i; int count; + const char **argv; char *strategy_opts_string = raw_opts; if (*strategy_opts_string == ' ') strategy_opts_string++; - count = split_cmdline(strategy_opts_string, - (const char ***)&opts->xopts); + count = split_cmdline(strategy_opts_string, &argv); if (count < 0) die(_("could not split '%s': %s"), strategy_opts_string, split_cmdline_strerror(count)); - opts->xopts_nr = count; - for (i = 0; i < opts->xopts_nr; i++) { - const char *arg = opts->xopts[i]; + for (i = 0; i < count; i++) { + const char *arg = argv[i]; skip_prefix(arg, "--", &arg); - opts->xopts[i] = xstrdup(arg); + strvec_push(&opts->xopts, arg); } } @@ -3055,8 +3051,8 @@ static void write_strategy_opts(struct replay_opts *opts) int i; struct strbuf buf = STRBUF_INIT; - for (i = 0; i < opts->xopts_nr; ++i) - strbuf_addf(&buf, " --%s", opts->xopts[i]); + for (i = 0; i < opts->xopts.nr; ++i) + strbuf_addf(&buf, " --%s", opts->xopts.v[i]); write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); strbuf_release(&buf); @@ -3080,7 +3076,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, write_file(rebase_path_verbose(), "%s", ""); if (opts->strategy) write_file(rebase_path_strategy(), "%s\n", opts->strategy); - if (opts->xopts_nr > 0) + if (opts->xopts.nr > 0) write_strategy_opts(opts); if (opts->allow_rerere_auto == RERERE_AUTOUPDATE) @@ -3464,13 +3460,10 @@ static int save_opts(struct replay_opts *opts) if (opts->gpg_sign) res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign); - if (opts->xopts) { - int i; - for (i = 0; i < opts->xopts_nr; i++) - res |= git_config_set_multivar_in_file_gently(opts_file, - "options.strategy-option", - opts->xopts[i], "^$", 0); - } + for (size_t i = 0; i < opts->xopts.nr; i++) + res |= git_config_set_multivar_in_file_gently(opts_file, + "options.strategy-option", + opts->xopts.v[i], "^$", 0); if (opts->allow_rerere_auto) res |= git_config_set_in_file_gently(opts_file, "options.allow-rerere-auto", @@ -3880,7 +3873,7 @@ static int do_merge(struct repository *r, struct commit *head_commit, *merge_commit, *i; struct commit_list *bases, *j; struct commit_list *to_merge = NULL, **tail = &to_merge; - const char *strategy = !opts->xopts_nr && + const char *strategy = !opts->xopts.nr && (!opts->strategy || !strcmp(opts->strategy, "recursive") || !strcmp(opts->strategy, "ort")) ? @@ -4063,9 +4056,9 @@ static int do_merge(struct repository *r, strvec_push(&cmd.args, "octopus"); else { strvec_push(&cmd.args, strategy); - for (k = 0; k < opts->xopts_nr; k++) + for (k = 0; k < opts->xopts.nr; k++) strvec_pushf(&cmd.args, - "-X%s", opts->xopts[k]); + "-X%s", opts->xopts.v[k]); } if (!(flags & TODO_EDIT_MERGE_MSG)) strvec_push(&cmd.args, "--no-edit"); diff --git a/sequencer.h b/sequencer.h index 33dbaf5b66..8a79d6b200 100644 --- a/sequencer.h +++ b/sequencer.h @@ -2,6 +2,7 @@ #define SEQUENCER_H #include "strbuf.h" +#include "strvec.h" #include "wt-status.h" struct commit; @@ -60,8 +61,7 @@ struct replay_opts { /* Merge strategy */ char *default_strategy; /* from config options */ char *strategy; - char **xopts; - size_t xopts_nr, xopts_alloc; + struct strvec xopts; /* Reflog */ char *reflog_action; @@ -80,7 +80,12 @@ struct replay_opts { /* Private use */ const char *reflog_message; }; -#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT } +#define REPLAY_OPTS_INIT { \ + .edit = -1, \ + .action = -1, \ + .current_fixups = STRBUF_INIT, \ + .xopts = STRVEC_INIT, \ +} /* * Note that ordering matters in this enum. Not only must it match the mapping -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options 2023-04-05 15:21 ` [PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood @ 2023-04-07 5:44 ` Elijah Newren 0 siblings, 0 replies; 36+ messages in thread From: Elijah Newren @ 2023-04-07 5:44 UTC (permalink / raw) To: Phillip Wood Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Phillip Wood On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > The sequencer stores the merge strategy options in an array of strings > which allocated with ALLOC_GROW(). Using "struct strvec" avoids manually > managing the memory of that array and simplifies the code. > > Aside from memory allocation the changes to the sequencer are largely > mechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A > new option parsing macro OPT_STRVEC() is also added to collect the > strategy options. Hopefully this can be used to simplify the code in > builtin/merge.c in the future. > > Note that there is a change of behavior to "git cherry-pick" and "git > revert" as passing “--no-strategy-option” will now clear any previous > strategy options whereas before this change it did nothing. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > builtin/revert.c | 20 +++----------------- > parse-options-cb.c | 16 ++++++++++++++++ > parse-options.h | 10 ++++++++++ > sequencer.c | 47 ++++++++++++++++++++-------------------------- > sequencer.h | 11 ++++++++--- > 5 files changed, 57 insertions(+), 47 deletions(-) > > diff --git a/builtin/revert.c b/builtin/revert.c > index 62986a7b1b..362857da65 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts) > return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage; > } > > -static int option_parse_x(const struct option *opt, > - const char *arg, int unset) > -{ > - struct replay_opts **opts_ptr = opt->value; > - struct replay_opts *opts = *opts_ptr; > - > - if (unset) > - return 0; > - > - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); > - opts->xopts[opts->xopts_nr++] = xstrdup(arg); > - return 0; > -} > - > static int option_parse_m(const struct option *opt, > const char *arg, int unset) > { > @@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > N_("select mainline parent"), option_parse_m), > OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto), > OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")), > - OPT_CALLBACK('X', "strategy-option", &opts, N_("option"), > - N_("option for merge strategy"), option_parse_x), > + OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"), > + N_("option for merge strategy")), > { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), > N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, > OPT_END() > @@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > "--signoff", opts->signoff, > "--mainline", opts->mainline, > "--strategy", opts->strategy ? 1 : 0, > - "--strategy-option", opts->xopts ? 1 : 0, > + "--strategy-option", opts->xopts.nr ? 1 : 0, > "-x", opts->record_origin, > "--ff", opts->allow_ff, > "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, > diff --git a/parse-options-cb.c b/parse-options-cb.c > index d346dbe210..8eb96c5ca9 100644 > --- a/parse-options-cb.c > +++ b/parse-options-cb.c > @@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset) > return 0; > } > > +int parse_opt_strvec(const struct option *opt, const char *arg, int unset) > +{ > + struct strvec *v = opt->value; > + > + if (unset) { > + strvec_clear(v); > + return 0; > + } > + > + if (!arg) > + return -1; > + > + strvec_push(v, arg); > + return 0; > +} > + > int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset) > { > return 0; > diff --git a/parse-options.h b/parse-options.h > index 26f19384e5..2d1d4d052f 100644 > --- a/parse-options.h > +++ b/parse-options.h > @@ -285,6 +285,15 @@ struct option { > .help = (h), \ > .callback = &parse_opt_string_list, \ > } > +#define OPT_STRVEC(s, l, v, a, h) { \ > + .type = OPTION_CALLBACK, \ > + .short_name = (s), \ > + .long_name = (l), \ > + .value = (v), \ > + .argh = (a), \ > + .help = (h), \ > + .callback = &parse_opt_strvec, \ > +} > #define OPT_UYN(s, l, v, h) { \ > .type = OPTION_CALLBACK, \ > .short_name = (s), \ > @@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int); > int parse_opt_commit(const struct option *, const char *, int); > int parse_opt_tertiary(const struct option *, const char *, int); > int parse_opt_string_list(const struct option *, const char *, int); > +int parse_opt_strvec(const struct option *, const char *, int); > int parse_opt_noop_cb(const struct option *, const char *, int); > enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, > const struct option *, > diff --git a/sequencer.c b/sequencer.c > index c35a67e104..6e2f3357c8 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts) > free(opts->reflog_action); > free(opts->default_strategy); > free(opts->strategy); > - for (size_t i = 0; i < opts->xopts_nr; i++) > - free(opts->xopts[i]); > - free(opts->xopts); > + strvec_clear (&opts->xopts); > strbuf_release(&opts->current_fixups); > if (opts->revs) > release_revisions(opts->revs); > @@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r, > next_tree = next ? get_commit_tree(next) : empty_tree(r); > base_tree = base ? get_commit_tree(base) : empty_tree(r); > > - for (i = 0; i < opts->xopts_nr; i++) > - parse_merge_opt(&o, opts->xopts[i]); > + for (i = 0; i < opts->xopts.nr; i++) > + parse_merge_opt(&o, opts->xopts.v[i]); > > if (!opts->strategy || !strcmp(opts->strategy, "ort")) { > memset(&result, 0, sizeof(result)); > @@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r, > commit_list_insert(base, &common); > commit_list_insert(next, &remotes); > res |= try_merge_command(r, opts->strategy, > - opts->xopts_nr, (const char **)opts->xopts, > + opts->xopts.nr, opts->xopts.v, > common, oid_to_hex(&head), remotes); > free_commit_list(common); > free_commit_list(remotes); > @@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) > else if (!strcmp(key, "options.gpg-sign")) > git_config_string_dup(&opts->gpg_sign, key, value); > else if (!strcmp(key, "options.strategy-option")) { > - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); > - opts->xopts[opts->xopts_nr++] = xstrdup(value); > + strvec_push(&opts->xopts, value); > } else if (!strcmp(key, "options.allow-rerere-auto")) > opts->allow_rerere_auto = > git_config_bool_or_int(key, value, &error_flag) ? > @@ -2920,22 +2917,21 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) > { > int i; > int count; > + const char **argv; > char *strategy_opts_string = raw_opts; > > if (*strategy_opts_string == ' ') > strategy_opts_string++; > > - count = split_cmdline(strategy_opts_string, > - (const char ***)&opts->xopts); > + count = split_cmdline(strategy_opts_string, &argv); > if (count < 0) > die(_("could not split '%s': %s"), strategy_opts_string, > split_cmdline_strerror(count)); > - opts->xopts_nr = count; > - for (i = 0; i < opts->xopts_nr; i++) { > - const char *arg = opts->xopts[i]; > + for (i = 0; i < count; i++) { > + const char *arg = argv[i]; > > skip_prefix(arg, "--", &arg); > - opts->xopts[i] = xstrdup(arg); > + strvec_push(&opts->xopts, arg); > } > } > > @@ -3055,8 +3051,8 @@ static void write_strategy_opts(struct replay_opts *opts) > int i; > struct strbuf buf = STRBUF_INIT; > > - for (i = 0; i < opts->xopts_nr; ++i) > - strbuf_addf(&buf, " --%s", opts->xopts[i]); > + for (i = 0; i < opts->xopts.nr; ++i) > + strbuf_addf(&buf, " --%s", opts->xopts.v[i]); > > write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); > strbuf_release(&buf); > @@ -3080,7 +3076,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, > write_file(rebase_path_verbose(), "%s", ""); > if (opts->strategy) > write_file(rebase_path_strategy(), "%s\n", opts->strategy); > - if (opts->xopts_nr > 0) > + if (opts->xopts.nr > 0) > write_strategy_opts(opts); > > if (opts->allow_rerere_auto == RERERE_AUTOUPDATE) > @@ -3464,13 +3460,10 @@ static int save_opts(struct replay_opts *opts) > if (opts->gpg_sign) > res |= git_config_set_in_file_gently(opts_file, > "options.gpg-sign", opts->gpg_sign); > - if (opts->xopts) { > - int i; > - for (i = 0; i < opts->xopts_nr; i++) > - res |= git_config_set_multivar_in_file_gently(opts_file, > - "options.strategy-option", > - opts->xopts[i], "^$", 0); > - } > + for (size_t i = 0; i < opts->xopts.nr; i++) > + res |= git_config_set_multivar_in_file_gently(opts_file, > + "options.strategy-option", > + opts->xopts.v[i], "^$", 0); > if (opts->allow_rerere_auto) > res |= git_config_set_in_file_gently(opts_file, > "options.allow-rerere-auto", > @@ -3880,7 +3873,7 @@ static int do_merge(struct repository *r, > struct commit *head_commit, *merge_commit, *i; > struct commit_list *bases, *j; > struct commit_list *to_merge = NULL, **tail = &to_merge; > - const char *strategy = !opts->xopts_nr && > + const char *strategy = !opts->xopts.nr && > (!opts->strategy || > !strcmp(opts->strategy, "recursive") || > !strcmp(opts->strategy, "ort")) ? > @@ -4063,9 +4056,9 @@ static int do_merge(struct repository *r, > strvec_push(&cmd.args, "octopus"); > else { > strvec_push(&cmd.args, strategy); > - for (k = 0; k < opts->xopts_nr; k++) > + for (k = 0; k < opts->xopts.nr; k++) > strvec_pushf(&cmd.args, > - "-X%s", opts->xopts[k]); > + "-X%s", opts->xopts.v[k]); > } > if (!(flags & TODO_EDIT_MERGE_MSG)) > strvec_push(&cmd.args, "--no-edit"); > diff --git a/sequencer.h b/sequencer.h > index 33dbaf5b66..8a79d6b200 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -2,6 +2,7 @@ > #define SEQUENCER_H > > #include "strbuf.h" > +#include "strvec.h" > #include "wt-status.h" > > struct commit; > @@ -60,8 +61,7 @@ struct replay_opts { > /* Merge strategy */ > char *default_strategy; /* from config options */ > char *strategy; > - char **xopts; > - size_t xopts_nr, xopts_alloc; > + struct strvec xopts; > > /* Reflog */ > char *reflog_action; > @@ -80,7 +80,12 @@ struct replay_opts { > /* Private use */ > const char *reflog_message; > }; > -#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT } > +#define REPLAY_OPTS_INIT { \ > + .edit = -1, \ > + .action = -1, \ > + .current_fixups = STRBUF_INIT, \ > + .xopts = STRVEC_INIT, \ > +} > > /* > * Note that ordering matters in this enum. Not only must it match the mapping > -- > 2.40.0.670.g64ef305212.dirty Simple, but very nice cleanup. :-) ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 3/5] rebase -m: cleanup --strategy-option handling 2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood 2023-04-05 15:21 ` [PATCH v2 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood 2023-04-05 15:21 ` [PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood @ 2023-04-05 15:21 ` Phillip Wood 2023-04-05 15:21 ` [PATCH v2 4/5] rebase -m: fix serialization of strategy options Phillip Wood ` (2 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-05 15:21 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When handling "--strategy-option" rebase collects the commands into a struct string_list, then concatenates them into a string, prepending "--" to each one before splitting the string and removing the "--" prefix. This is an artifact of the scripted rebase and the need to support "rebase --preserve-merges". Now that "--preserve-merges" no-longer exists we can cleanup the way the argument is handled. The tests for a bad strategy option are adjusted now that parse_strategy_opts() is no-longer called when starting a rebase. The fact that it only errors out when running "git rebase --continue" is a mixed blessing but the next commit will fix the root cause of the parsing problem so lets not worry about that here. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 30 ++++++++++-------------------- sequencer.c | 2 +- sequencer.h | 1 - t/t3436-rebase-more-options.sh | 10 ++++++++-- 4 files changed, 19 insertions(+), 24 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 3bd215c771..511922c6fc 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -117,7 +117,8 @@ struct rebase_options { struct string_list exec; int allow_empty_message; int rebase_merges, rebase_cousins; - char *strategy, *strategy_opts; + char *strategy; + struct string_list strategy_opts; struct strbuf git_format_patch_opt; int reschedule_failed_exec; int reapply_cherry_picks; @@ -143,6 +144,7 @@ struct rebase_options { .config_autosquash = -1, \ .update_refs = -1, \ .config_update_refs = -1, \ + .strategy_opts = STRING_LIST_INIT_NODUP,\ } static struct replay_opts get_replay_opts(const struct rebase_options *opts) @@ -176,8 +178,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) replay.default_strategy = NULL; } - if (opts->strategy_opts) - parse_strategy_opts(&replay, opts->strategy_opts); + for (size_t i = 0; i < opts->strategy_opts.nr; i++) + strvec_push(&replay.xopts, opts->strategy_opts.items[i].string); if (opts->squash_onto) { oidcpy(&replay.squash_onto, opts->squash_onto); @@ -1013,7 +1015,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) int ignore_whitespace = 0; const char *gpg_sign = NULL; const char *rebase_merges = NULL; - struct string_list strategy_options = STRING_LIST_INIT_NODUP; struct object_id squash_onto; char *squash_onto_name = NULL; char *keep_base_onto_name = NULL; @@ -1122,7 +1123,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) N_("use 'merge-base --fork-point' to refine upstream")), OPT_STRING('s', "strategy", &options.strategy, N_("strategy"), N_("use the given merge strategy")), - OPT_STRING_LIST('X', "strategy-option", &strategy_options, + OPT_STRING_LIST('X', "strategy-option", &options.strategy_opts, N_("option"), N_("pass the argument through to the merge " "strategy")), @@ -1436,23 +1437,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } else { /* REBASE_MERGE */ if (ignore_whitespace) { - string_list_append(&strategy_options, + string_list_append(&options.strategy_opts, "ignore-space-change"); } } - if (strategy_options.nr) { - int i; - - if (!options.strategy) - options.strategy = "ort"; - - strbuf_reset(&buf); - for (i = 0; i < strategy_options.nr; i++) - strbuf_addf(&buf, " --%s", - strategy_options.items[i].string); - options.strategy_opts = xstrdup(buf.buf); - } + if (options.strategy_opts.nr && !options.strategy) + options.strategy = "ort"; if (options.strategy) { options.strategy = xstrdup(options.strategy); @@ -1827,10 +1818,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) free(options.gpg_sign_opt); string_list_clear(&options.exec, 0); free(options.strategy); - free(options.strategy_opts); + string_list_clear(&options.strategy_opts, 0); strbuf_release(&options.git_format_patch_opt); free(squash_onto_name); free(keep_base_onto_name); - string_list_clear(&strategy_options, 0); return !!ret; } diff --git a/sequencer.c b/sequencer.c index 6e2f3357c8..045d549042 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2913,7 +2913,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) return 0; } -void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) +static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) { int i; int count; diff --git a/sequencer.h b/sequencer.h index 8a79d6b200..913a0f652d 100644 --- a/sequencer.h +++ b/sequencer.h @@ -252,7 +252,6 @@ int read_oneliner(struct strbuf *buf, const char *path, unsigned flags); int read_author_script(const char *path, char **name, char **email, char **date, int allow_missing); -void parse_strategy_opts(struct replay_opts *opts, char *raw_opts); int write_basic_state(struct replay_opts *opts, const char *head_name, struct commit *onto, const struct object_id *orig_head); void sequencer_post_commit_cleanup(struct repository *r, int verbose); diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh index c3184c9ade..3adf42f47d 100755 --- a/t/t3436-rebase-more-options.sh +++ b/t/t3436-rebase-more-options.sh @@ -41,19 +41,25 @@ test_expect_success 'setup' ' ' test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' + test_when_finished "test_might_fail git rebase --abort" && cat >expect <<-\EOF && fatal: could not split '\''--bad'\'': unclosed quote EOF - test_expect_code 128 git rebase -X"bad argument\"" side main >out 2>actual && + GIT_SEQUENCE_EDITOR="echo break >" \ + git rebase -i -X"bad argument\"" side main && + test_expect_code 128 git rebase --continue >out 2>actual && test_must_be_empty out && test_cmp expect actual ' test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' + test_when_finished "test_might_fail git rebase --abort" && cat >expect <<-\EOF && fatal: could not split '\''--bad'\'': cmdline ends with \ EOF - test_expect_code 128 git rebase -X"bad escape \\" side main >out 2>actual && + GIT_SEQUENCE_EDITOR="echo break >" \ + git rebase -i -X"bad escape \\" side main && + test_expect_code 128 git rebase --continue >out 2>actual && test_must_be_empty out && test_cmp expect actual ' -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 4/5] rebase -m: fix serialization of strategy options 2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood ` (2 preceding siblings ...) 2023-04-05 15:21 ` [PATCH v2 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood @ 2023-04-05 15:21 ` Phillip Wood 2023-04-07 5:47 ` Elijah Newren 2023-04-05 15:21 ` [PATCH v2 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood 2023-04-07 5:49 ` [PATCH v2 0/5] rebase: cleanup merge strategy option handling Elijah Newren 5 siblings, 1 reply; 36+ messages in thread From: Phillip Wood @ 2023-04-05 15:21 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> To store the strategy options rebase prepends " --" to each one and writes them to a file. To load them it reads the file and passes the contents to split_cmdline(). This roughly mimics the behavior of the scripted rebase but has a couple of limitations, (1) options containing whitespace are not properly preserved (this is true of the scripted rebase as well) and (2) options containing '"' or '\' are incorrectly parsed and may cause the parser to return an error. Fix these limitations by quoting each option when they are stored so that they can be parsed correctly. Now that "--preserve-merges" no longer exist this change also stops prepending "--" to the options when they are stored as that was an artifact of the scripted rebase. These changes are backwards compatible so the files written by an older version of git can be still be read. They are also forwards compatible, the file can still be parsed by recent versions of git as they treat the "--" prefix as optional. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- alias.c | 18 +++++++++++++++++ alias.h | 3 +++ sequencer.c | 11 ++++++----- t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++------------ t/t3436-rebase-more-options.sh | 24 ----------------------- 5 files changed, 50 insertions(+), 42 deletions(-) diff --git a/alias.c b/alias.c index e814948ced..54a1a23d2c 100644 --- a/alias.c +++ b/alias.c @@ -3,6 +3,7 @@ #include "alloc.h" #include "config.h" #include "gettext.h" +#include "strbuf.h" #include "string-list.h" struct config_alias_data { @@ -46,6 +47,23 @@ void list_aliases(struct string_list *list) read_early_config(config_alias_cb, &data); } +void quote_cmdline(struct strbuf *buf, const char **argv) +{ + for (const char **argp = argv; *argp; argp++) { + if (argp != argv) + strbuf_addch(buf, ' '); + strbuf_addch(buf, '"'); + for (const char *p = *argp; *p; p++) { + const char c = *p; + + if (c == '"' || c =='\\') + strbuf_addch(buf, '\\'); + strbuf_addch(buf, c); + } + strbuf_addch(buf, '"'); + } +} + #define SPLIT_CMDLINE_BAD_ENDING 1 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 diff --git a/alias.h b/alias.h index aef4843bb7..43db736484 100644 --- a/alias.h +++ b/alias.h @@ -1,9 +1,12 @@ #ifndef ALIAS_H #define ALIAS_H +struct strbuf; struct string_list; char *alias_lookup(const char *alias); +/* Quote argv so buf can be parsed by split_cmdline() */ +void quote_cmdline(struct strbuf *buf, const char **argv); int split_cmdline(char *cmdline, const char ***argv); /* Takes a negative value returned by split_cmdline */ const char *split_cmdline_strerror(int cmdline_errno); diff --git a/sequencer.c b/sequencer.c index 045d549042..9907100c8a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) count = split_cmdline(strategy_opts_string, &argv); if (count < 0) - die(_("could not split '%s': %s"), strategy_opts_string, + BUG("could not split '%s': %s", strategy_opts_string, split_cmdline_strerror(count)); for (i = 0; i < count; i++) { const char *arg = argv[i]; @@ -3048,12 +3048,13 @@ static int read_populate_opts(struct replay_opts *opts) static void write_strategy_opts(struct replay_opts *opts) { - int i; struct strbuf buf = STRBUF_INIT; - for (i = 0; i < opts->xopts.nr; ++i) - strbuf_addf(&buf, " --%s", opts->xopts.v[i]); - + /* + * Quote strategy options so that they can be read correctly + * by split_cmdline(). + */ + quote_cmdline(&buf, opts->xopts.v); write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); strbuf_release(&buf); } diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 130e2f9b55..42c3954125 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F2-on-topic-branch && test_commit "commit-new-file-F3-on-topic-branch" F3 32 && - test_when_finished "rm -fr test-bin funny.was.run" && + test_when_finished "rm -fr test-bin" && mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - case "\$1" in --opt) ;; *) exit 2 ;; esac - shift && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && + + write_script test-bin/git-merge-funny <<-\EOF && + printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual + shift 3 && + exec git merge-recursive "$@" + EOF + + cat >expect <<-\EOF && + [7] + [--option=arg with space] + [--op"tion\] + [--new + line ] + [--] + EOF + + rm -f actual && ( PATH=./test-bin:$PATH && - test_must_fail git rebase -s funny -Xopt main topic + test_must_fail git rebase -s funny -X"option=arg with space" \ + -Xop\"tion\\ -X"new${LF}line " main topic ) && - test -f funny.was.run && - rm funny.was.run && + test_cmp expect actual && + rm actual && echo "Resolved" >F2 && git add F2 && ( PATH=./test-bin:$PATH && git rebase --continue ) && - test -f funny.was.run + test_cmp expect actual ' test_expect_success 'rebase -i --continue handles merge strategy and options' ' diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh index 3adf42f47d..94671d3c46 100755 --- a/t/t3436-rebase-more-options.sh +++ b/t/t3436-rebase-more-options.sh @@ -40,30 +40,6 @@ test_expect_success 'setup' ' EOF ' -test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' - test_when_finished "test_might_fail git rebase --abort" && - cat >expect <<-\EOF && - fatal: could not split '\''--bad'\'': unclosed quote - EOF - GIT_SEQUENCE_EDITOR="echo break >" \ - git rebase -i -X"bad argument\"" side main && - test_expect_code 128 git rebase --continue >out 2>actual && - test_must_be_empty out && - test_cmp expect actual -' - -test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' - test_when_finished "test_might_fail git rebase --abort" && - cat >expect <<-\EOF && - fatal: could not split '\''--bad'\'': cmdline ends with \ - EOF - GIT_SEQUENCE_EDITOR="echo break >" \ - git rebase -i -X"bad escape \\" side main && - test_expect_code 128 git rebase --continue >out 2>actual && - test_must_be_empty out && - test_cmp expect actual -' - test_expect_success '--ignore-whitespace works with apply backend' ' test_must_fail git rebase --apply main side && git rebase --abort && -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/5] rebase -m: fix serialization of strategy options 2023-04-05 15:21 ` [PATCH v2 4/5] rebase -m: fix serialization of strategy options Phillip Wood @ 2023-04-07 5:47 ` Elijah Newren 0 siblings, 0 replies; 36+ messages in thread From: Elijah Newren @ 2023-04-07 5:47 UTC (permalink / raw) To: Phillip Wood Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin, Phillip Wood On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > To store the strategy options rebase prepends " --" to each one and > writes them to a file. To load them it reads the file and passes the > contents to split_cmdline(). This roughly mimics the behavior of the > scripted rebase but has a couple of limitations, (1) options containing > whitespace are not properly preserved (this is true of the scripted > rebase as well) and (2) options containing '"' or '\' are incorrectly > parsed and may cause the parser to return an error. > > Fix these limitations by quoting each option when they are stored so > that they can be parsed correctly. Now that "--preserve-merges" no > longer exist this change also stops prepending "--" to the options when > they are stored as that was an artifact of the scripted rebase. > > These changes are backwards compatible so the files written by an older > version of git can be still be read. They are also forwards compatible, s/be still be/still be/ Sorry for not noticing this last time. > the file can still be parsed by recent versions of git as they treat the > "--" prefix as optional. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > alias.c | 18 +++++++++++++++++ > alias.h | 3 +++ > sequencer.c | 11 ++++++----- > t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++------------ > t/t3436-rebase-more-options.sh | 24 ----------------------- > 5 files changed, 50 insertions(+), 42 deletions(-) > > diff --git a/alias.c b/alias.c > index e814948ced..54a1a23d2c 100644 > --- a/alias.c > +++ b/alias.c > @@ -3,6 +3,7 @@ > #include "alloc.h" > #include "config.h" > #include "gettext.h" > +#include "strbuf.h" > #include "string-list.h" > > struct config_alias_data { > @@ -46,6 +47,23 @@ void list_aliases(struct string_list *list) > read_early_config(config_alias_cb, &data); > } > > +void quote_cmdline(struct strbuf *buf, const char **argv) > +{ > + for (const char **argp = argv; *argp; argp++) { > + if (argp != argv) > + strbuf_addch(buf, ' '); > + strbuf_addch(buf, '"'); > + for (const char *p = *argp; *p; p++) { > + const char c = *p; > + > + if (c == '"' || c =='\\') > + strbuf_addch(buf, '\\'); > + strbuf_addch(buf, c); > + } > + strbuf_addch(buf, '"'); > + } > +} > + > #define SPLIT_CMDLINE_BAD_ENDING 1 > #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 > #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 > diff --git a/alias.h b/alias.h > index aef4843bb7..43db736484 100644 > --- a/alias.h > +++ b/alias.h > @@ -1,9 +1,12 @@ > #ifndef ALIAS_H > #define ALIAS_H > > +struct strbuf; > struct string_list; > > char *alias_lookup(const char *alias); > +/* Quote argv so buf can be parsed by split_cmdline() */ > +void quote_cmdline(struct strbuf *buf, const char **argv); > int split_cmdline(char *cmdline, const char ***argv); > /* Takes a negative value returned by split_cmdline */ > const char *split_cmdline_strerror(int cmdline_errno); > diff --git a/sequencer.c b/sequencer.c > index 045d549042..9907100c8a 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) > > count = split_cmdline(strategy_opts_string, &argv); > if (count < 0) > - die(_("could not split '%s': %s"), strategy_opts_string, > + BUG("could not split '%s': %s", strategy_opts_string, > split_cmdline_strerror(count)); > for (i = 0; i < count; i++) { > const char *arg = argv[i]; > @@ -3048,12 +3048,13 @@ static int read_populate_opts(struct replay_opts *opts) > > static void write_strategy_opts(struct replay_opts *opts) > { > - int i; > struct strbuf buf = STRBUF_INIT; > > - for (i = 0; i < opts->xopts.nr; ++i) > - strbuf_addf(&buf, " --%s", opts->xopts.v[i]); > - > + /* > + * Quote strategy options so that they can be read correctly > + * by split_cmdline(). > + */ > + quote_cmdline(&buf, opts->xopts.v); > write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); > strbuf_release(&buf); > } > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh > index 130e2f9b55..42c3954125 100755 > --- a/t/t3418-rebase-continue.sh > +++ b/t/t3418-rebase-continue.sh > @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' > rm -fr .git/rebase-* && > git reset --hard commit-new-file-F2-on-topic-branch && > test_commit "commit-new-file-F3-on-topic-branch" F3 32 && > - test_when_finished "rm -fr test-bin funny.was.run" && > + test_when_finished "rm -fr test-bin" && > mkdir test-bin && > - cat >test-bin/git-merge-funny <<-EOF && > - #!$SHELL_PATH > - case "\$1" in --opt) ;; *) exit 2 ;; esac > - shift && > - >funny.was.run && > - exec git merge-recursive "\$@" > - EOF > - chmod +x test-bin/git-merge-funny && > + > + write_script test-bin/git-merge-funny <<-\EOF && > + printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual > + shift 3 && > + exec git merge-recursive "$@" > + EOF > + > + cat >expect <<-\EOF && > + [7] > + [--option=arg with space] > + [--op"tion\] > + [--new > + line ] > + [--] > + EOF > + > + rm -f actual && > ( > PATH=./test-bin:$PATH && > - test_must_fail git rebase -s funny -Xopt main topic > + test_must_fail git rebase -s funny -X"option=arg with space" \ > + -Xop\"tion\\ -X"new${LF}line " main topic > ) && > - test -f funny.was.run && > - rm funny.was.run && > + test_cmp expect actual && > + rm actual && > echo "Resolved" >F2 && > git add F2 && > ( > PATH=./test-bin:$PATH && > git rebase --continue > ) && > - test -f funny.was.run > + test_cmp expect actual > ' > > test_expect_success 'rebase -i --continue handles merge strategy and options' ' > diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh > index 3adf42f47d..94671d3c46 100755 > --- a/t/t3436-rebase-more-options.sh > +++ b/t/t3436-rebase-more-options.sh > @@ -40,30 +40,6 @@ test_expect_success 'setup' ' > EOF > ' > > -test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' > - test_when_finished "test_might_fail git rebase --abort" && > - cat >expect <<-\EOF && > - fatal: could not split '\''--bad'\'': unclosed quote > - EOF > - GIT_SEQUENCE_EDITOR="echo break >" \ > - git rebase -i -X"bad argument\"" side main && > - test_expect_code 128 git rebase --continue >out 2>actual && > - test_must_be_empty out && > - test_cmp expect actual > -' > - > -test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' > - test_when_finished "test_might_fail git rebase --abort" && > - cat >expect <<-\EOF && > - fatal: could not split '\''--bad'\'': cmdline ends with \ > - EOF > - GIT_SEQUENCE_EDITOR="echo break >" \ > - git rebase -i -X"bad escape \\" side main && > - test_expect_code 128 git rebase --continue >out 2>actual && > - test_must_be_empty out && > - test_cmp expect actual > -' > - > test_expect_success '--ignore-whitespace works with apply backend' ' > test_must_fail git rebase --apply main side && > git rebase --abort && > -- > 2.40.0.670.g64ef305212.dirty Looks good. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 5/5] rebase: remove a couple of redundant strategy tests 2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood ` (3 preceding siblings ...) 2023-04-05 15:21 ` [PATCH v2 4/5] rebase -m: fix serialization of strategy options Phillip Wood @ 2023-04-05 15:21 ` Phillip Wood 2023-04-07 5:49 ` [PATCH v2 0/5] rebase: cleanup merge strategy option handling Elijah Newren 5 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-05 15:21 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Remove a test in t3402 that has been redundant ever since 80ff47957b (rebase: remember strategy and strategy options, 2011-02-06). That commit added a new test, the first part of which (as noted in the old commit message) duplicated an existing test. Also remove a test t3418 that has been redundant since the merge backend was removed in 68aa495b59 (rebase: implement --merge via the interactive machinery, 2018-12-11), since it now tests the same code paths as the preceding test. Helped-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- t/t3402-rebase-merge.sh | 21 --------------------- t/t3418-rebase-continue.sh | 32 -------------------------------- 2 files changed, 53 deletions(-) diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh index 7e46f4ca85..79b0640c00 100755 --- a/t/t3402-rebase-merge.sh +++ b/t/t3402-rebase-merge.sh @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' ' esac ' -test_expect_success 'rebase -s funny -Xopt' ' - test_when_finished "rm -fr test-bin funny.was.run" && - mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - case "\$1" in --opt) ;; *) exit 2 ;; esac - shift && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && - git reset --hard && - git checkout -b test-funny main^ && - test_commit funny && - ( - PATH=./test-bin:$PATH && - git rebase -s funny -Xopt main - ) && - test -f funny.was.run -' - test_expect_success 'rebase --skip works with two conflicts in a row' ' git checkout second-side && tr "[A-Z]" "[a-z]" <newfile >tmp && diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 42c3954125..2d0789e554 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' test_cmp expect actual ' -test_expect_success 'rebase -i --continue handles merge strategy and options' ' - rm -fr .git/rebase-* && - git reset --hard commit-new-file-F2-on-topic-branch && - test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && - test_when_finished "rm -fr test-bin funny.was.run funny.args" && - mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - echo "\$@" >>funny.args - case "\$1" in --opt) ;; *) exit 2 ;; esac - case "\$2" in --foo) ;; *) exit 2 ;; esac - case "\$4" in --) ;; *) exit 2 ;; esac - shift 2 && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && - ( - PATH=./test-bin:$PATH && - test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic - ) && - test -f funny.was.run && - rm funny.was.run && - echo "Resolved" >F2 && - git add F2 && - ( - PATH=./test-bin:$PATH && - git rebase --continue - ) && - test -f funny.was.run -' - test_expect_success 'rebase -r passes merge strategy options correctly' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F3-on-topic-branch && -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 0/5] rebase: cleanup merge strategy option handling 2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood ` (4 preceding siblings ...) 2023-04-05 15:21 ` [PATCH v2 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood @ 2023-04-07 5:49 ` Elijah Newren 2023-04-07 13:57 ` Phillip Wood 5 siblings, 1 reply; 36+ messages in thread From: Elijah Newren @ 2023-04-07 5:49 UTC (permalink / raw) To: Phillip Wood Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Cleanup the handling of --strategy-option now that we no longer need to > support "--preserve-merges" and properly quote the argument when saving > it to disc. > > Thanks to Elijah for his comments on V1. > > Changes since V1: > > I've rebased these patches onto 'master' to avoid conflicts with > 'sg/parse-options-h-initializers' in the new patch 2 (this series > depends on 'ab/fix-strategy-opts-parsing' but that has now been merged > to master). > > Patch 1 - Unchanged. > > Patch 2 - New patch to store the merge strategy options in an "struct > strvec". This patch also introduces a new macro OPT_STRVEC() > to collect options into an "struct strvec". > > Patch 3 - Small simplification due to the changes in patch 2. > > Patch 4 - Moved the code to quote a list so it can split by > split_cmdline() into a new function quote_cmdline() as > suggested by Elijah. > > Patch 5 - Reworded the commit message as suggested by Elijah. I noticed a small typo/grammo in the commit message of Patch 4 that I missed in V1, but otherwise this series looks good. I'm not sure fixing the tiny grammo is even all that important; I'll leave it up to you for whether you want to bother re-rolling to fix it. Either way: Reviewed-by: Elijah Newren <newren@gmail.com> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 0/5] rebase: cleanup merge strategy option handling 2023-04-07 5:49 ` [PATCH v2 0/5] rebase: cleanup merge strategy option handling Elijah Newren @ 2023-04-07 13:57 ` Phillip Wood 2023-04-07 17:53 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Phillip Wood @ 2023-04-07 13:57 UTC (permalink / raw) To: Elijah Newren Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin Hi Elijah On 07/04/2023 06:49, Elijah Newren wrote: > On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > I noticed a small typo/grammo in the commit message of Patch 4 that I > missed in V1, but otherwise this series looks good. I'm not sure > fixing the tiny grammo is even all that important; I'll leave it up to > you for whether you want to bother re-rolling to fix it. Either way: > > Reviewed-by: Elijah Newren <newren@gmail.com> Thanks for reviewing this series. I've sent v3 with the typo fix and your Reviewed-by: trailer. Best Wishes Phillip ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 0/5] rebase: cleanup merge strategy option handling 2023-04-07 13:57 ` Phillip Wood @ 2023-04-07 17:53 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2023-04-07 17:53 UTC (permalink / raw) To: Phillip Wood Cc: Elijah Newren, git, Ævar Arnfjörð Bjarmason, Johannes Schindelin Phillip Wood <phillip.wood123@gmail.com> writes: > Thanks for reviewing this series. I've sent v3 with the typo fix and > your Reviewed-by: trailer. Thanks, both, for writing, reviewing and polishing. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 0/5] rebase: cleanup merge strategy option handling 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood ` (7 preceding siblings ...) 2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood @ 2023-04-07 13:55 ` Phillip Wood 2023-04-07 13:55 ` [PATCH v3 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood ` (4 more replies) 2023-04-10 9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood 9 siblings, 5 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-07 13:55 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin From: Phillip Wood <phillip.wood@dunelm.org.uk> Cleanup the handling of --strategy-option now that we no longer need to support "--preserve-merges" and properly quote the argument when saving it to disc. Thanks to Elijah for his comments. Changes since V2: - Added Elijah's Reviewed-by: trailer. - Fixed a typo in patch 4 spotted by Elijah. Changes since V1: I've rebased these patches onto 'master' to avoid conflicts with 'sg/parse-options-h-initializers' in the new patch 2 (this series depends on 'ab/fix-strategy-opts-parsing' but that has now been merged to master). Patch 1 - Unchanged. Patch 2 - New patch to store the merge strategy options in an "struct strvec". This patch also introduces a new macro OPT_STRVEC() to collect options into an "struct strvec". Patch 3 - Small simplification due to the changes in patch 2. Patch 4 - Moved the code to quote a list so it can split by split_cmdline() into a new function quote_cmdline() as suggested by Elijah. Patch 5 - Reworded the commit message as suggested by Elijah. Base-Commit: 140b9478dad5d19543c1cb4fd293ccec228f1240 Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv3 View-Changes-At: https://github.com/phillipwood/git/compare/140b9478d...2d1d32811 Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v3 Phillip Wood (5): rebase: stop reading and writing unnecessary strategy state sequencer: use struct strvec to store merge strategy options rebase -m: cleanup --strategy-option handling rebase -m: fix serialization of strategy options rebase: remove a couple of redundant strategy tests alias.c | 18 +++++++ alias.h | 3 ++ builtin/rebase.c | 54 ++++----------------- builtin/revert.c | 20 ++------ parse-options-cb.c | 16 +++++++ parse-options.h | 10 ++++ sequencer.c | 57 ++++++++++------------ sequencer.h | 12 +++-- t/t3402-rebase-merge.sh | 21 -------- t/t3418-rebase-continue.sh | 88 +++++++++++++--------------------- t/t3436-rebase-more-options.sh | 18 ------- 11 files changed, 126 insertions(+), 191 deletions(-) Range-diff against v2: 1: 2353c753f5 ! 1: 882b403423 rebase: stop reading and writing unnecessary strategy state @@ Commit message writing of state files between builtin/rebase.c and sequencer.c but that is left for a follow up series. + Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> ## builtin/rebase.c ## 2: dd7b82cdd5 ! 2: 1d8e59aa16 sequencer: use struct strvec to store merge strategy options @@ Commit message revert" as passing “--no-strategy-option” will now clear any previous strategy options whereas before this change it did nothing. + Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> ## builtin/revert.c ## 3: ccef0e6f4b ! 3: e98ef5ce8c rebase -m: cleanup --strategy-option handling @@ Commit message mixed blessing but the next commit will fix the root cause of the parsing problem so lets not worry about that here. + Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> ## builtin/rebase.c ## 4: 9a90212ef2 ! 4: a5e940e2d0 rebase -m: fix serialization of strategy options @@ Commit message they are stored as that was an artifact of the scripted rebase. These changes are backwards compatible so the files written by an older - version of git can be still be read. They are also forwards compatible, + version of git can still be read. They are also forwards compatible, the file can still be parsed by recent versions of git as they treat the "--" prefix as optional. + Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> ## alias.c ## 5: 3515c31b40 ! 5: 2d1d328110 rebase: remove a couple of redundant strategy tests @@ Commit message preceding test. Helped-by: Elijah Newren <newren@gmail.com> + Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> ## t/t3402-rebase-merge.sh ## -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 1/5] rebase: stop reading and writing unnecessary strategy state 2023-04-07 13:55 ` [PATCH v3 " Phillip Wood @ 2023-04-07 13:55 ` Phillip Wood 2023-04-07 13:55 ` [PATCH v3 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood ` (3 subsequent siblings) 4 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-07 13:55 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The state files for "--strategy" and "--strategy-option" are written and read twice, once by builtin/rebase.c and then by sequencer.c. This is an artifact of the scripted rebase and the need to support "rebase --preserve-merges". Now that "--preserve-merges" no-longer exists we only need to read and write these files in sequencer.c. This enables us to remove a call to free() in read_strategy_opts() that was added by f1f4ebf432 (sequencer.c: fix "opts->strategy" leak in read_strategy_opts(), 2022-11-08) as this commit fixes the root cause of that leak. There is further scope for removing duplication in the reading and writing of state files between builtin/rebase.c and sequencer.c but that is left for a follow up series. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 24 ------------------------ sequencer.c | 1 - 2 files changed, 25 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 5b7b908b66..3bd215c771 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -483,24 +483,6 @@ static int read_basic_state(struct rebase_options *opts) opts->gpg_sign_opt = xstrdup(buf.buf); } - if (file_exists(state_dir_path("strategy", opts))) { - strbuf_reset(&buf); - if (!read_oneliner(&buf, state_dir_path("strategy", opts), - READ_ONELINER_WARN_MISSING)) - return -1; - free(opts->strategy); - opts->strategy = xstrdup(buf.buf); - } - - if (file_exists(state_dir_path("strategy_opts", opts))) { - strbuf_reset(&buf); - if (!read_oneliner(&buf, state_dir_path("strategy_opts", opts), - READ_ONELINER_WARN_MISSING)) - return -1; - free(opts->strategy_opts); - opts->strategy_opts = xstrdup(buf.buf); - } - strbuf_release(&buf); return 0; @@ -518,12 +500,6 @@ static int rebase_write_basic_state(struct rebase_options *opts) write_file(state_dir_path("quiet", opts), "%s", ""); if (opts->flags & REBASE_VERBOSE) write_file(state_dir_path("verbose", opts), "%s", ""); - if (opts->strategy) - write_file(state_dir_path("strategy", opts), "%s", - opts->strategy); - if (opts->strategy_opts) - write_file(state_dir_path("strategy_opts", opts), "%s", - opts->strategy_opts); if (opts->allow_rerere_autoupdate > 0) write_file(state_dir_path("allow_rerere_autoupdate", opts), "-%s-rerere-autoupdate", diff --git a/sequencer.c b/sequencer.c index 3be23d7ca2..c35a67e104 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2944,7 +2944,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf) strbuf_reset(buf); if (!read_oneliner(buf, rebase_path_strategy(), 0)) return; - free(opts->strategy); opts->strategy = strbuf_detach(buf, NULL); if (!read_oneliner(buf, rebase_path_strategy_opts(), 0)) return; -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 2/5] sequencer: use struct strvec to store merge strategy options 2023-04-07 13:55 ` [PATCH v3 " Phillip Wood 2023-04-07 13:55 ` [PATCH v3 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood @ 2023-04-07 13:55 ` Phillip Wood 2023-04-07 13:55 ` [PATCH v3 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood ` (2 subsequent siblings) 4 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-07 13:55 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The sequencer stores the merge strategy options in an array of strings which allocated with ALLOC_GROW(). Using "struct strvec" avoids manually managing the memory of that array and simplifies the code. Aside from memory allocation the changes to the sequencer are largely mechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A new option parsing macro OPT_STRVEC() is also added to collect the strategy options. Hopefully this can be used to simplify the code in builtin/merge.c in the future. Note that there is a change of behavior to "git cherry-pick" and "git revert" as passing “--no-strategy-option” will now clear any previous strategy options whereas before this change it did nothing. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/revert.c | 20 +++----------------- parse-options-cb.c | 16 ++++++++++++++++ parse-options.h | 10 ++++++++++ sequencer.c | 47 ++++++++++++++++++++-------------------------- sequencer.h | 11 ++++++++--- 5 files changed, 57 insertions(+), 47 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 62986a7b1b..362857da65 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts) return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage; } -static int option_parse_x(const struct option *opt, - const char *arg, int unset) -{ - struct replay_opts **opts_ptr = opt->value; - struct replay_opts *opts = *opts_ptr; - - if (unset) - return 0; - - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); - opts->xopts[opts->xopts_nr++] = xstrdup(arg); - return 0; -} - static int option_parse_m(const struct option *opt, const char *arg, int unset) { @@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) N_("select mainline parent"), option_parse_m), OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto), OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")), - OPT_CALLBACK('X', "strategy-option", &opts, N_("option"), - N_("option for merge strategy"), option_parse_x), + OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"), + N_("option for merge strategy")), { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, OPT_END() @@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) "--signoff", opts->signoff, "--mainline", opts->mainline, "--strategy", opts->strategy ? 1 : 0, - "--strategy-option", opts->xopts ? 1 : 0, + "--strategy-option", opts->xopts.nr ? 1 : 0, "-x", opts->record_origin, "--ff", opts->allow_ff, "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, diff --git a/parse-options-cb.c b/parse-options-cb.c index d346dbe210..8eb96c5ca9 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset) return 0; } +int parse_opt_strvec(const struct option *opt, const char *arg, int unset) +{ + struct strvec *v = opt->value; + + if (unset) { + strvec_clear(v); + return 0; + } + + if (!arg) + return -1; + + strvec_push(v, arg); + return 0; +} + int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset) { return 0; diff --git a/parse-options.h b/parse-options.h index 26f19384e5..2d1d4d052f 100644 --- a/parse-options.h +++ b/parse-options.h @@ -285,6 +285,15 @@ struct option { .help = (h), \ .callback = &parse_opt_string_list, \ } +#define OPT_STRVEC(s, l, v, a, h) { \ + .type = OPTION_CALLBACK, \ + .short_name = (s), \ + .long_name = (l), \ + .value = (v), \ + .argh = (a), \ + .help = (h), \ + .callback = &parse_opt_strvec, \ +} #define OPT_UYN(s, l, v, h) { \ .type = OPTION_CALLBACK, \ .short_name = (s), \ @@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int); int parse_opt_commit(const struct option *, const char *, int); int parse_opt_tertiary(const struct option *, const char *, int); int parse_opt_string_list(const struct option *, const char *, int); +int parse_opt_strvec(const struct option *, const char *, int); int parse_opt_noop_cb(const struct option *, const char *, int); enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, diff --git a/sequencer.c b/sequencer.c index c35a67e104..6e2f3357c8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts) free(opts->reflog_action); free(opts->default_strategy); free(opts->strategy); - for (size_t i = 0; i < opts->xopts_nr; i++) - free(opts->xopts[i]); - free(opts->xopts); + strvec_clear (&opts->xopts); strbuf_release(&opts->current_fixups); if (opts->revs) release_revisions(opts->revs); @@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r, next_tree = next ? get_commit_tree(next) : empty_tree(r); base_tree = base ? get_commit_tree(base) : empty_tree(r); - for (i = 0; i < opts->xopts_nr; i++) - parse_merge_opt(&o, opts->xopts[i]); + for (i = 0; i < opts->xopts.nr; i++) + parse_merge_opt(&o, opts->xopts.v[i]); if (!opts->strategy || !strcmp(opts->strategy, "ort")) { memset(&result, 0, sizeof(result)); @@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r, commit_list_insert(base, &common); commit_list_insert(next, &remotes); res |= try_merge_command(r, opts->strategy, - opts->xopts_nr, (const char **)opts->xopts, + opts->xopts.nr, opts->xopts.v, common, oid_to_hex(&head), remotes); free_commit_list(common); free_commit_list(remotes); @@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) else if (!strcmp(key, "options.gpg-sign")) git_config_string_dup(&opts->gpg_sign, key, value); else if (!strcmp(key, "options.strategy-option")) { - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); - opts->xopts[opts->xopts_nr++] = xstrdup(value); + strvec_push(&opts->xopts, value); } else if (!strcmp(key, "options.allow-rerere-auto")) opts->allow_rerere_auto = git_config_bool_or_int(key, value, &error_flag) ? @@ -2920,22 +2917,21 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) { int i; int count; + const char **argv; char *strategy_opts_string = raw_opts; if (*strategy_opts_string == ' ') strategy_opts_string++; - count = split_cmdline(strategy_opts_string, - (const char ***)&opts->xopts); + count = split_cmdline(strategy_opts_string, &argv); if (count < 0) die(_("could not split '%s': %s"), strategy_opts_string, split_cmdline_strerror(count)); - opts->xopts_nr = count; - for (i = 0; i < opts->xopts_nr; i++) { - const char *arg = opts->xopts[i]; + for (i = 0; i < count; i++) { + const char *arg = argv[i]; skip_prefix(arg, "--", &arg); - opts->xopts[i] = xstrdup(arg); + strvec_push(&opts->xopts, arg); } } @@ -3055,8 +3051,8 @@ static void write_strategy_opts(struct replay_opts *opts) int i; struct strbuf buf = STRBUF_INIT; - for (i = 0; i < opts->xopts_nr; ++i) - strbuf_addf(&buf, " --%s", opts->xopts[i]); + for (i = 0; i < opts->xopts.nr; ++i) + strbuf_addf(&buf, " --%s", opts->xopts.v[i]); write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); strbuf_release(&buf); @@ -3080,7 +3076,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, write_file(rebase_path_verbose(), "%s", ""); if (opts->strategy) write_file(rebase_path_strategy(), "%s\n", opts->strategy); - if (opts->xopts_nr > 0) + if (opts->xopts.nr > 0) write_strategy_opts(opts); if (opts->allow_rerere_auto == RERERE_AUTOUPDATE) @@ -3464,13 +3460,10 @@ static int save_opts(struct replay_opts *opts) if (opts->gpg_sign) res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign); - if (opts->xopts) { - int i; - for (i = 0; i < opts->xopts_nr; i++) - res |= git_config_set_multivar_in_file_gently(opts_file, - "options.strategy-option", - opts->xopts[i], "^$", 0); - } + for (size_t i = 0; i < opts->xopts.nr; i++) + res |= git_config_set_multivar_in_file_gently(opts_file, + "options.strategy-option", + opts->xopts.v[i], "^$", 0); if (opts->allow_rerere_auto) res |= git_config_set_in_file_gently(opts_file, "options.allow-rerere-auto", @@ -3880,7 +3873,7 @@ static int do_merge(struct repository *r, struct commit *head_commit, *merge_commit, *i; struct commit_list *bases, *j; struct commit_list *to_merge = NULL, **tail = &to_merge; - const char *strategy = !opts->xopts_nr && + const char *strategy = !opts->xopts.nr && (!opts->strategy || !strcmp(opts->strategy, "recursive") || !strcmp(opts->strategy, "ort")) ? @@ -4063,9 +4056,9 @@ static int do_merge(struct repository *r, strvec_push(&cmd.args, "octopus"); else { strvec_push(&cmd.args, strategy); - for (k = 0; k < opts->xopts_nr; k++) + for (k = 0; k < opts->xopts.nr; k++) strvec_pushf(&cmd.args, - "-X%s", opts->xopts[k]); + "-X%s", opts->xopts.v[k]); } if (!(flags & TODO_EDIT_MERGE_MSG)) strvec_push(&cmd.args, "--no-edit"); diff --git a/sequencer.h b/sequencer.h index 33dbaf5b66..8a79d6b200 100644 --- a/sequencer.h +++ b/sequencer.h @@ -2,6 +2,7 @@ #define SEQUENCER_H #include "strbuf.h" +#include "strvec.h" #include "wt-status.h" struct commit; @@ -60,8 +61,7 @@ struct replay_opts { /* Merge strategy */ char *default_strategy; /* from config options */ char *strategy; - char **xopts; - size_t xopts_nr, xopts_alloc; + struct strvec xopts; /* Reflog */ char *reflog_action; @@ -80,7 +80,12 @@ struct replay_opts { /* Private use */ const char *reflog_message; }; -#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT } +#define REPLAY_OPTS_INIT { \ + .edit = -1, \ + .action = -1, \ + .current_fixups = STRBUF_INIT, \ + .xopts = STRVEC_INIT, \ +} /* * Note that ordering matters in this enum. Not only must it match the mapping -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 3/5] rebase -m: cleanup --strategy-option handling 2023-04-07 13:55 ` [PATCH v3 " Phillip Wood 2023-04-07 13:55 ` [PATCH v3 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood 2023-04-07 13:55 ` [PATCH v3 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood @ 2023-04-07 13:55 ` Phillip Wood 2023-04-07 13:55 ` [PATCH v3 4/5] rebase -m: fix serialization of strategy options Phillip Wood 2023-04-07 13:55 ` [PATCH v3 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood 4 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-07 13:55 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When handling "--strategy-option" rebase collects the commands into a struct string_list, then concatenates them into a string, prepending "--" to each one before splitting the string and removing the "--" prefix. This is an artifact of the scripted rebase and the need to support "rebase --preserve-merges". Now that "--preserve-merges" no-longer exists we can cleanup the way the argument is handled. The tests for a bad strategy option are adjusted now that parse_strategy_opts() is no-longer called when starting a rebase. The fact that it only errors out when running "git rebase --continue" is a mixed blessing but the next commit will fix the root cause of the parsing problem so lets not worry about that here. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 30 ++++++++++-------------------- sequencer.c | 2 +- sequencer.h | 1 - t/t3436-rebase-more-options.sh | 10 ++++++++-- 4 files changed, 19 insertions(+), 24 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 3bd215c771..511922c6fc 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -117,7 +117,8 @@ struct rebase_options { struct string_list exec; int allow_empty_message; int rebase_merges, rebase_cousins; - char *strategy, *strategy_opts; + char *strategy; + struct string_list strategy_opts; struct strbuf git_format_patch_opt; int reschedule_failed_exec; int reapply_cherry_picks; @@ -143,6 +144,7 @@ struct rebase_options { .config_autosquash = -1, \ .update_refs = -1, \ .config_update_refs = -1, \ + .strategy_opts = STRING_LIST_INIT_NODUP,\ } static struct replay_opts get_replay_opts(const struct rebase_options *opts) @@ -176,8 +178,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) replay.default_strategy = NULL; } - if (opts->strategy_opts) - parse_strategy_opts(&replay, opts->strategy_opts); + for (size_t i = 0; i < opts->strategy_opts.nr; i++) + strvec_push(&replay.xopts, opts->strategy_opts.items[i].string); if (opts->squash_onto) { oidcpy(&replay.squash_onto, opts->squash_onto); @@ -1013,7 +1015,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) int ignore_whitespace = 0; const char *gpg_sign = NULL; const char *rebase_merges = NULL; - struct string_list strategy_options = STRING_LIST_INIT_NODUP; struct object_id squash_onto; char *squash_onto_name = NULL; char *keep_base_onto_name = NULL; @@ -1122,7 +1123,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) N_("use 'merge-base --fork-point' to refine upstream")), OPT_STRING('s', "strategy", &options.strategy, N_("strategy"), N_("use the given merge strategy")), - OPT_STRING_LIST('X', "strategy-option", &strategy_options, + OPT_STRING_LIST('X', "strategy-option", &options.strategy_opts, N_("option"), N_("pass the argument through to the merge " "strategy")), @@ -1436,23 +1437,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } else { /* REBASE_MERGE */ if (ignore_whitespace) { - string_list_append(&strategy_options, + string_list_append(&options.strategy_opts, "ignore-space-change"); } } - if (strategy_options.nr) { - int i; - - if (!options.strategy) - options.strategy = "ort"; - - strbuf_reset(&buf); - for (i = 0; i < strategy_options.nr; i++) - strbuf_addf(&buf, " --%s", - strategy_options.items[i].string); - options.strategy_opts = xstrdup(buf.buf); - } + if (options.strategy_opts.nr && !options.strategy) + options.strategy = "ort"; if (options.strategy) { options.strategy = xstrdup(options.strategy); @@ -1827,10 +1818,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) free(options.gpg_sign_opt); string_list_clear(&options.exec, 0); free(options.strategy); - free(options.strategy_opts); + string_list_clear(&options.strategy_opts, 0); strbuf_release(&options.git_format_patch_opt); free(squash_onto_name); free(keep_base_onto_name); - string_list_clear(&strategy_options, 0); return !!ret; } diff --git a/sequencer.c b/sequencer.c index 6e2f3357c8..045d549042 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2913,7 +2913,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) return 0; } -void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) +static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) { int i; int count; diff --git a/sequencer.h b/sequencer.h index 8a79d6b200..913a0f652d 100644 --- a/sequencer.h +++ b/sequencer.h @@ -252,7 +252,6 @@ int read_oneliner(struct strbuf *buf, const char *path, unsigned flags); int read_author_script(const char *path, char **name, char **email, char **date, int allow_missing); -void parse_strategy_opts(struct replay_opts *opts, char *raw_opts); int write_basic_state(struct replay_opts *opts, const char *head_name, struct commit *onto, const struct object_id *orig_head); void sequencer_post_commit_cleanup(struct repository *r, int verbose); diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh index c3184c9ade..3adf42f47d 100755 --- a/t/t3436-rebase-more-options.sh +++ b/t/t3436-rebase-more-options.sh @@ -41,19 +41,25 @@ test_expect_success 'setup' ' ' test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' + test_when_finished "test_might_fail git rebase --abort" && cat >expect <<-\EOF && fatal: could not split '\''--bad'\'': unclosed quote EOF - test_expect_code 128 git rebase -X"bad argument\"" side main >out 2>actual && + GIT_SEQUENCE_EDITOR="echo break >" \ + git rebase -i -X"bad argument\"" side main && + test_expect_code 128 git rebase --continue >out 2>actual && test_must_be_empty out && test_cmp expect actual ' test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' + test_when_finished "test_might_fail git rebase --abort" && cat >expect <<-\EOF && fatal: could not split '\''--bad'\'': cmdline ends with \ EOF - test_expect_code 128 git rebase -X"bad escape \\" side main >out 2>actual && + GIT_SEQUENCE_EDITOR="echo break >" \ + git rebase -i -X"bad escape \\" side main && + test_expect_code 128 git rebase --continue >out 2>actual && test_must_be_empty out && test_cmp expect actual ' -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 4/5] rebase -m: fix serialization of strategy options 2023-04-07 13:55 ` [PATCH v3 " Phillip Wood ` (2 preceding siblings ...) 2023-04-07 13:55 ` [PATCH v3 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood @ 2023-04-07 13:55 ` Phillip Wood 2023-04-07 13:55 ` [PATCH v3 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood 4 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-07 13:55 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> To store the strategy options rebase prepends " --" to each one and writes them to a file. To load them it reads the file and passes the contents to split_cmdline(). This roughly mimics the behavior of the scripted rebase but has a couple of limitations, (1) options containing whitespace are not properly preserved (this is true of the scripted rebase as well) and (2) options containing '"' or '\' are incorrectly parsed and may cause the parser to return an error. Fix these limitations by quoting each option when they are stored so that they can be parsed correctly. Now that "--preserve-merges" no longer exist this change also stops prepending "--" to the options when they are stored as that was an artifact of the scripted rebase. These changes are backwards compatible so the files written by an older version of git can still be read. They are also forwards compatible, the file can still be parsed by recent versions of git as they treat the "--" prefix as optional. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- alias.c | 18 +++++++++++++++++ alias.h | 3 +++ sequencer.c | 11 ++++++----- t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++------------ t/t3436-rebase-more-options.sh | 24 ----------------------- 5 files changed, 50 insertions(+), 42 deletions(-) diff --git a/alias.c b/alias.c index e814948ced..54a1a23d2c 100644 --- a/alias.c +++ b/alias.c @@ -3,6 +3,7 @@ #include "alloc.h" #include "config.h" #include "gettext.h" +#include "strbuf.h" #include "string-list.h" struct config_alias_data { @@ -46,6 +47,23 @@ void list_aliases(struct string_list *list) read_early_config(config_alias_cb, &data); } +void quote_cmdline(struct strbuf *buf, const char **argv) +{ + for (const char **argp = argv; *argp; argp++) { + if (argp != argv) + strbuf_addch(buf, ' '); + strbuf_addch(buf, '"'); + for (const char *p = *argp; *p; p++) { + const char c = *p; + + if (c == '"' || c =='\\') + strbuf_addch(buf, '\\'); + strbuf_addch(buf, c); + } + strbuf_addch(buf, '"'); + } +} + #define SPLIT_CMDLINE_BAD_ENDING 1 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 diff --git a/alias.h b/alias.h index aef4843bb7..43db736484 100644 --- a/alias.h +++ b/alias.h @@ -1,9 +1,12 @@ #ifndef ALIAS_H #define ALIAS_H +struct strbuf; struct string_list; char *alias_lookup(const char *alias); +/* Quote argv so buf can be parsed by split_cmdline() */ +void quote_cmdline(struct strbuf *buf, const char **argv); int split_cmdline(char *cmdline, const char ***argv); /* Takes a negative value returned by split_cmdline */ const char *split_cmdline_strerror(int cmdline_errno); diff --git a/sequencer.c b/sequencer.c index 045d549042..9907100c8a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) count = split_cmdline(strategy_opts_string, &argv); if (count < 0) - die(_("could not split '%s': %s"), strategy_opts_string, + BUG("could not split '%s': %s", strategy_opts_string, split_cmdline_strerror(count)); for (i = 0; i < count; i++) { const char *arg = argv[i]; @@ -3048,12 +3048,13 @@ static int read_populate_opts(struct replay_opts *opts) static void write_strategy_opts(struct replay_opts *opts) { - int i; struct strbuf buf = STRBUF_INIT; - for (i = 0; i < opts->xopts.nr; ++i) - strbuf_addf(&buf, " --%s", opts->xopts.v[i]); - + /* + * Quote strategy options so that they can be read correctly + * by split_cmdline(). + */ + quote_cmdline(&buf, opts->xopts.v); write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); strbuf_release(&buf); } diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 130e2f9b55..42c3954125 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F2-on-topic-branch && test_commit "commit-new-file-F3-on-topic-branch" F3 32 && - test_when_finished "rm -fr test-bin funny.was.run" && + test_when_finished "rm -fr test-bin" && mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - case "\$1" in --opt) ;; *) exit 2 ;; esac - shift && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && + + write_script test-bin/git-merge-funny <<-\EOF && + printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual + shift 3 && + exec git merge-recursive "$@" + EOF + + cat >expect <<-\EOF && + [7] + [--option=arg with space] + [--op"tion\] + [--new + line ] + [--] + EOF + + rm -f actual && ( PATH=./test-bin:$PATH && - test_must_fail git rebase -s funny -Xopt main topic + test_must_fail git rebase -s funny -X"option=arg with space" \ + -Xop\"tion\\ -X"new${LF}line " main topic ) && - test -f funny.was.run && - rm funny.was.run && + test_cmp expect actual && + rm actual && echo "Resolved" >F2 && git add F2 && ( PATH=./test-bin:$PATH && git rebase --continue ) && - test -f funny.was.run + test_cmp expect actual ' test_expect_success 'rebase -i --continue handles merge strategy and options' ' diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh index 3adf42f47d..94671d3c46 100755 --- a/t/t3436-rebase-more-options.sh +++ b/t/t3436-rebase-more-options.sh @@ -40,30 +40,6 @@ test_expect_success 'setup' ' EOF ' -test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' - test_when_finished "test_might_fail git rebase --abort" && - cat >expect <<-\EOF && - fatal: could not split '\''--bad'\'': unclosed quote - EOF - GIT_SEQUENCE_EDITOR="echo break >" \ - git rebase -i -X"bad argument\"" side main && - test_expect_code 128 git rebase --continue >out 2>actual && - test_must_be_empty out && - test_cmp expect actual -' - -test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' - test_when_finished "test_might_fail git rebase --abort" && - cat >expect <<-\EOF && - fatal: could not split '\''--bad'\'': cmdline ends with \ - EOF - GIT_SEQUENCE_EDITOR="echo break >" \ - git rebase -i -X"bad escape \\" side main && - test_expect_code 128 git rebase --continue >out 2>actual && - test_must_be_empty out && - test_cmp expect actual -' - test_expect_success '--ignore-whitespace works with apply backend' ' test_must_fail git rebase --apply main side && git rebase --abort && -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 5/5] rebase: remove a couple of redundant strategy tests 2023-04-07 13:55 ` [PATCH v3 " Phillip Wood ` (3 preceding siblings ...) 2023-04-07 13:55 ` [PATCH v3 4/5] rebase -m: fix serialization of strategy options Phillip Wood @ 2023-04-07 13:55 ` Phillip Wood 4 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-07 13:55 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Remove a test in t3402 that has been redundant ever since 80ff47957b (rebase: remember strategy and strategy options, 2011-02-06). That commit added a new test, the first part of which (as noted in the old commit message) duplicated an existing test. Also remove a test t3418 that has been redundant since the merge backend was removed in 68aa495b59 (rebase: implement --merge via the interactive machinery, 2018-12-11), since it now tests the same code paths as the preceding test. Helped-by: Elijah Newren <newren@gmail.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- t/t3402-rebase-merge.sh | 21 --------------------- t/t3418-rebase-continue.sh | 32 -------------------------------- 2 files changed, 53 deletions(-) diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh index 7e46f4ca85..79b0640c00 100755 --- a/t/t3402-rebase-merge.sh +++ b/t/t3402-rebase-merge.sh @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' ' esac ' -test_expect_success 'rebase -s funny -Xopt' ' - test_when_finished "rm -fr test-bin funny.was.run" && - mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - case "\$1" in --opt) ;; *) exit 2 ;; esac - shift && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && - git reset --hard && - git checkout -b test-funny main^ && - test_commit funny && - ( - PATH=./test-bin:$PATH && - git rebase -s funny -Xopt main - ) && - test -f funny.was.run -' - test_expect_success 'rebase --skip works with two conflicts in a row' ' git checkout second-side && tr "[A-Z]" "[a-z]" <newfile >tmp && diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 42c3954125..2d0789e554 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' test_cmp expect actual ' -test_expect_success 'rebase -i --continue handles merge strategy and options' ' - rm -fr .git/rebase-* && - git reset --hard commit-new-file-F2-on-topic-branch && - test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && - test_when_finished "rm -fr test-bin funny.was.run funny.args" && - mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - echo "\$@" >>funny.args - case "\$1" in --opt) ;; *) exit 2 ;; esac - case "\$2" in --foo) ;; *) exit 2 ;; esac - case "\$4" in --) ;; *) exit 2 ;; esac - shift 2 && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && - ( - PATH=./test-bin:$PATH && - test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic - ) && - test -f funny.was.run && - rm funny.was.run && - echo "Resolved" >F2 && - git add F2 && - ( - PATH=./test-bin:$PATH && - git rebase --continue - ) && - test -f funny.was.run -' - test_expect_success 'rebase -r passes merge strategy options correctly' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F3-on-topic-branch && -- 2.40.0.670.g64ef305212.dirty ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 0/5] rebase: cleanup merge strategy option handling 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood ` (8 preceding siblings ...) 2023-04-07 13:55 ` [PATCH v3 " Phillip Wood @ 2023-04-10 9:08 ` Phillip Wood 2023-04-10 9:08 ` [PATCH v4 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood ` (5 more replies) 9 siblings, 6 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-10 9:08 UTC (permalink / raw) To: git, Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin From: Phillip Wood <phillip.wood@dunelm.org.uk> Cleanup the handling of --strategy-option now that we no longer need to support "--preserve-merges" and properly quote the argument when saving it to disc. A hopefully final re-roll to fix a memory leak I spotted in V3. Changes since V3: - Fixed a memory leak added in V3. The array returned by split_cmdline() is allocated on the heap so we need to fee it. The array elements come from the string passed to split_cmdline() so do not need to be individually freed. Changes since V2: - Added Elijah's Reviewed-by: trailer. - Fixed a typo in patch 4 spotted by Elijah. Changes since V1: I've rebased these patches onto 'master' to avoid conflicts with 'sg/parse-options-h-initializers' in the new patch 2 (this series depends on 'ab/fix-strategy-opts-parsing' but that has now been merged to master). Patch 1 - Unchanged. Patch 2 - New patch to store the merge strategy options in an "struct strvec". This patch also introduces a new macro OPT_STRVEC() to collect options into an "struct strvec". Patch 3 - Small simplification due to the changes in patch 2. Patch 4 - Moved the code to quote a list so it can split by split_cmdline() into a new function quote_cmdline() as suggested by Elijah. Patch 5 - Reworded the commit message as suggested by Elijah. Base-Commit: 140b9478dad5d19543c1cb4fd293ccec228f1240 Published-As: https://github.com/phillipwood/git/releases/tag/sequencer-merge-strategy-options%2Fv4 View-Changes-At: https://github.com/phillipwood/git/compare/140b9478d...7de1aa101 Fetch-It-Via: git fetch https://github.com/phillipwood/git sequencer-merge-strategy-options/v4 Phillip Wood (5): rebase: stop reading and writing unnecessary strategy state sequencer: use struct strvec to store merge strategy options rebase -m: cleanup --strategy-option handling rebase -m: fix serialization of strategy options rebase: remove a couple of redundant strategy tests alias.c | 18 +++++++ alias.h | 3 ++ builtin/rebase.c | 54 ++++----------------- builtin/revert.c | 20 ++------ parse-options-cb.c | 16 +++++++ parse-options.h | 10 ++++ sequencer.c | 58 ++++++++++------------ sequencer.h | 12 +++-- t/t3402-rebase-merge.sh | 21 -------- t/t3418-rebase-continue.sh | 88 +++++++++++++--------------------- t/t3436-rebase-more-options.sh | 18 ------- 11 files changed, 127 insertions(+), 191 deletions(-) Range-diff against v3: 1: 882b403423 = 1: 882b403423 rebase: stop reading and writing unnecessary strategy state 2: 1d8e59aa16 ! 2: 4b288e883d sequencer: use struct strvec to store merge strategy options @@ sequencer.c: void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) - opts->xopts[i] = xstrdup(arg); + strvec_push(&opts->xopts, arg); } ++ free(argv); } + static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf) @@ sequencer.c: static void write_strategy_opts(struct replay_opts *opts) int i; struct strbuf buf = STRBUF_INIT; 3: e98ef5ce8c = 3: 4e040c1214 rebase -m: cleanup --strategy-option handling 4: a5e940e2d0 = 4: 671ee03503 rebase -m: fix serialization of strategy options 5: 2d1d328110 = 5: 7de1aa1016 rebase: remove a couple of redundant strategy tests -- 2.40.0.672.gbd2d2ac924 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v4 1/5] rebase: stop reading and writing unnecessary strategy state 2023-04-10 9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood @ 2023-04-10 9:08 ` Phillip Wood 2023-04-10 9:08 ` [PATCH v4 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood ` (4 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-10 9:08 UTC (permalink / raw) To: git, Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The state files for "--strategy" and "--strategy-option" are written and read twice, once by builtin/rebase.c and then by sequencer.c. This is an artifact of the scripted rebase and the need to support "rebase --preserve-merges". Now that "--preserve-merges" no-longer exists we only need to read and write these files in sequencer.c. This enables us to remove a call to free() in read_strategy_opts() that was added by f1f4ebf432 (sequencer.c: fix "opts->strategy" leak in read_strategy_opts(), 2022-11-08) as this commit fixes the root cause of that leak. There is further scope for removing duplication in the reading and writing of state files between builtin/rebase.c and sequencer.c but that is left for a follow up series. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 24 ------------------------ sequencer.c | 1 - 2 files changed, 25 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 5b7b908b66..3bd215c771 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -483,24 +483,6 @@ static int read_basic_state(struct rebase_options *opts) opts->gpg_sign_opt = xstrdup(buf.buf); } - if (file_exists(state_dir_path("strategy", opts))) { - strbuf_reset(&buf); - if (!read_oneliner(&buf, state_dir_path("strategy", opts), - READ_ONELINER_WARN_MISSING)) - return -1; - free(opts->strategy); - opts->strategy = xstrdup(buf.buf); - } - - if (file_exists(state_dir_path("strategy_opts", opts))) { - strbuf_reset(&buf); - if (!read_oneliner(&buf, state_dir_path("strategy_opts", opts), - READ_ONELINER_WARN_MISSING)) - return -1; - free(opts->strategy_opts); - opts->strategy_opts = xstrdup(buf.buf); - } - strbuf_release(&buf); return 0; @@ -518,12 +500,6 @@ static int rebase_write_basic_state(struct rebase_options *opts) write_file(state_dir_path("quiet", opts), "%s", ""); if (opts->flags & REBASE_VERBOSE) write_file(state_dir_path("verbose", opts), "%s", ""); - if (opts->strategy) - write_file(state_dir_path("strategy", opts), "%s", - opts->strategy); - if (opts->strategy_opts) - write_file(state_dir_path("strategy_opts", opts), "%s", - opts->strategy_opts); if (opts->allow_rerere_autoupdate > 0) write_file(state_dir_path("allow_rerere_autoupdate", opts), "-%s-rerere-autoupdate", diff --git a/sequencer.c b/sequencer.c index 3be23d7ca2..c35a67e104 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2944,7 +2944,6 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf) strbuf_reset(buf); if (!read_oneliner(buf, rebase_path_strategy(), 0)) return; - free(opts->strategy); opts->strategy = strbuf_detach(buf, NULL); if (!read_oneliner(buf, rebase_path_strategy_opts(), 0)) return; -- 2.40.0.672.gbd2d2ac924 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 2/5] sequencer: use struct strvec to store merge strategy options 2023-04-10 9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood 2023-04-10 9:08 ` [PATCH v4 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood @ 2023-04-10 9:08 ` Phillip Wood 2023-04-10 9:08 ` [PATCH v4 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood ` (3 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-10 9:08 UTC (permalink / raw) To: git, Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> The sequencer stores the merge strategy options in an array of strings which allocated with ALLOC_GROW(). Using "struct strvec" avoids manually managing the memory of that array and simplifies the code. Aside from memory allocation the changes to the sequencer are largely mechanical, changing xopts_nr to xopts.nr and xopts[i] to xopts.v[i]. A new option parsing macro OPT_STRVEC() is also added to collect the strategy options. Hopefully this can be used to simplify the code in builtin/merge.c in the future. Note that there is a change of behavior to "git cherry-pick" and "git revert" as passing “--no-strategy-option” will now clear any previous strategy options whereas before this change it did nothing. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/revert.c | 20 +++---------------- parse-options-cb.c | 16 ++++++++++++++++ parse-options.h | 10 ++++++++++ sequencer.c | 48 ++++++++++++++++++++-------------------------- sequencer.h | 11 ++++++++--- 5 files changed, 58 insertions(+), 47 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 62986a7b1b..362857da65 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts) return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage; } -static int option_parse_x(const struct option *opt, - const char *arg, int unset) -{ - struct replay_opts **opts_ptr = opt->value; - struct replay_opts *opts = *opts_ptr; - - if (unset) - return 0; - - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); - opts->xopts[opts->xopts_nr++] = xstrdup(arg); - return 0; -} - static int option_parse_m(const struct option *opt, const char *arg, int unset) { @@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) N_("select mainline parent"), option_parse_m), OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto), OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")), - OPT_CALLBACK('X', "strategy-option", &opts, N_("option"), - N_("option for merge strategy"), option_parse_x), + OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"), + N_("option for merge strategy")), { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"), N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, OPT_END() @@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) "--signoff", opts->signoff, "--mainline", opts->mainline, "--strategy", opts->strategy ? 1 : 0, - "--strategy-option", opts->xopts ? 1 : 0, + "--strategy-option", opts->xopts.nr ? 1 : 0, "-x", opts->record_origin, "--ff", opts->allow_ff, "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, diff --git a/parse-options-cb.c b/parse-options-cb.c index d346dbe210..8eb96c5ca9 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset) return 0; } +int parse_opt_strvec(const struct option *opt, const char *arg, int unset) +{ + struct strvec *v = opt->value; + + if (unset) { + strvec_clear(v); + return 0; + } + + if (!arg) + return -1; + + strvec_push(v, arg); + return 0; +} + int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset) { return 0; diff --git a/parse-options.h b/parse-options.h index 26f19384e5..2d1d4d052f 100644 --- a/parse-options.h +++ b/parse-options.h @@ -285,6 +285,15 @@ struct option { .help = (h), \ .callback = &parse_opt_string_list, \ } +#define OPT_STRVEC(s, l, v, a, h) { \ + .type = OPTION_CALLBACK, \ + .short_name = (s), \ + .long_name = (l), \ + .value = (v), \ + .argh = (a), \ + .help = (h), \ + .callback = &parse_opt_strvec, \ +} #define OPT_UYN(s, l, v, h) { \ .type = OPTION_CALLBACK, \ .short_name = (s), \ @@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int); int parse_opt_commit(const struct option *, const char *, int); int parse_opt_tertiary(const struct option *, const char *, int); int parse_opt_string_list(const struct option *, const char *, int); +int parse_opt_strvec(const struct option *, const char *, int); int parse_opt_noop_cb(const struct option *, const char *, int); enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, diff --git a/sequencer.c b/sequencer.c index c35a67e104..50d469812a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts) free(opts->reflog_action); free(opts->default_strategy); free(opts->strategy); - for (size_t i = 0; i < opts->xopts_nr; i++) - free(opts->xopts[i]); - free(opts->xopts); + strvec_clear (&opts->xopts); strbuf_release(&opts->current_fixups); if (opts->revs) release_revisions(opts->revs); @@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r, next_tree = next ? get_commit_tree(next) : empty_tree(r); base_tree = base ? get_commit_tree(base) : empty_tree(r); - for (i = 0; i < opts->xopts_nr; i++) - parse_merge_opt(&o, opts->xopts[i]); + for (i = 0; i < opts->xopts.nr; i++) + parse_merge_opt(&o, opts->xopts.v[i]); if (!opts->strategy || !strcmp(opts->strategy, "ort")) { memset(&result, 0, sizeof(result)); @@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r, commit_list_insert(base, &common); commit_list_insert(next, &remotes); res |= try_merge_command(r, opts->strategy, - opts->xopts_nr, (const char **)opts->xopts, + opts->xopts.nr, opts->xopts.v, common, oid_to_hex(&head), remotes); free_commit_list(common); free_commit_list(remotes); @@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) else if (!strcmp(key, "options.gpg-sign")) git_config_string_dup(&opts->gpg_sign, key, value); else if (!strcmp(key, "options.strategy-option")) { - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); - opts->xopts[opts->xopts_nr++] = xstrdup(value); + strvec_push(&opts->xopts, value); } else if (!strcmp(key, "options.allow-rerere-auto")) opts->allow_rerere_auto = git_config_bool_or_int(key, value, &error_flag) ? @@ -2920,23 +2917,23 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) { int i; int count; + const char **argv; char *strategy_opts_string = raw_opts; if (*strategy_opts_string == ' ') strategy_opts_string++; - count = split_cmdline(strategy_opts_string, - (const char ***)&opts->xopts); + count = split_cmdline(strategy_opts_string, &argv); if (count < 0) die(_("could not split '%s': %s"), strategy_opts_string, split_cmdline_strerror(count)); - opts->xopts_nr = count; - for (i = 0; i < opts->xopts_nr; i++) { - const char *arg = opts->xopts[i]; + for (i = 0; i < count; i++) { + const char *arg = argv[i]; skip_prefix(arg, "--", &arg); - opts->xopts[i] = xstrdup(arg); + strvec_push(&opts->xopts, arg); } + free(argv); } static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf) @@ -3055,8 +3052,8 @@ static void write_strategy_opts(struct replay_opts *opts) int i; struct strbuf buf = STRBUF_INIT; - for (i = 0; i < opts->xopts_nr; ++i) - strbuf_addf(&buf, " --%s", opts->xopts[i]); + for (i = 0; i < opts->xopts.nr; ++i) + strbuf_addf(&buf, " --%s", opts->xopts.v[i]); write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); strbuf_release(&buf); @@ -3080,7 +3077,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, write_file(rebase_path_verbose(), "%s", ""); if (opts->strategy) write_file(rebase_path_strategy(), "%s\n", opts->strategy); - if (opts->xopts_nr > 0) + if (opts->xopts.nr > 0) write_strategy_opts(opts); if (opts->allow_rerere_auto == RERERE_AUTOUPDATE) @@ -3464,13 +3461,10 @@ static int save_opts(struct replay_opts *opts) if (opts->gpg_sign) res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign); - if (opts->xopts) { - int i; - for (i = 0; i < opts->xopts_nr; i++) - res |= git_config_set_multivar_in_file_gently(opts_file, - "options.strategy-option", - opts->xopts[i], "^$", 0); - } + for (size_t i = 0; i < opts->xopts.nr; i++) + res |= git_config_set_multivar_in_file_gently(opts_file, + "options.strategy-option", + opts->xopts.v[i], "^$", 0); if (opts->allow_rerere_auto) res |= git_config_set_in_file_gently(opts_file, "options.allow-rerere-auto", @@ -3880,7 +3874,7 @@ static int do_merge(struct repository *r, struct commit *head_commit, *merge_commit, *i; struct commit_list *bases, *j; struct commit_list *to_merge = NULL, **tail = &to_merge; - const char *strategy = !opts->xopts_nr && + const char *strategy = !opts->xopts.nr && (!opts->strategy || !strcmp(opts->strategy, "recursive") || !strcmp(opts->strategy, "ort")) ? @@ -4063,9 +4057,9 @@ static int do_merge(struct repository *r, strvec_push(&cmd.args, "octopus"); else { strvec_push(&cmd.args, strategy); - for (k = 0; k < opts->xopts_nr; k++) + for (k = 0; k < opts->xopts.nr; k++) strvec_pushf(&cmd.args, - "-X%s", opts->xopts[k]); + "-X%s", opts->xopts.v[k]); } if (!(flags & TODO_EDIT_MERGE_MSG)) strvec_push(&cmd.args, "--no-edit"); diff --git a/sequencer.h b/sequencer.h index 33dbaf5b66..8a79d6b200 100644 --- a/sequencer.h +++ b/sequencer.h @@ -2,6 +2,7 @@ #define SEQUENCER_H #include "strbuf.h" +#include "strvec.h" #include "wt-status.h" struct commit; @@ -60,8 +61,7 @@ struct replay_opts { /* Merge strategy */ char *default_strategy; /* from config options */ char *strategy; - char **xopts; - size_t xopts_nr, xopts_alloc; + struct strvec xopts; /* Reflog */ char *reflog_action; @@ -80,7 +80,12 @@ struct replay_opts { /* Private use */ const char *reflog_message; }; -#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT } +#define REPLAY_OPTS_INIT { \ + .edit = -1, \ + .action = -1, \ + .current_fixups = STRBUF_INIT, \ + .xopts = STRVEC_INIT, \ +} /* * Note that ordering matters in this enum. Not only must it match the mapping -- 2.40.0.672.gbd2d2ac924 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 3/5] rebase -m: cleanup --strategy-option handling 2023-04-10 9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood 2023-04-10 9:08 ` [PATCH v4 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood 2023-04-10 9:08 ` [PATCH v4 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood @ 2023-04-10 9:08 ` Phillip Wood 2023-04-10 9:08 ` [PATCH v4 4/5] rebase -m: fix serialization of strategy options Phillip Wood ` (2 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-10 9:08 UTC (permalink / raw) To: git, Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When handling "--strategy-option" rebase collects the commands into a struct string_list, then concatenates them into a string, prepending "--" to each one before splitting the string and removing the "--" prefix. This is an artifact of the scripted rebase and the need to support "rebase --preserve-merges". Now that "--preserve-merges" no-longer exists we can cleanup the way the argument is handled. The tests for a bad strategy option are adjusted now that parse_strategy_opts() is no-longer called when starting a rebase. The fact that it only errors out when running "git rebase --continue" is a mixed blessing but the next commit will fix the root cause of the parsing problem so lets not worry about that here. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 30 ++++++++++-------------------- sequencer.c | 2 +- sequencer.h | 1 - t/t3436-rebase-more-options.sh | 10 ++++++++-- 4 files changed, 19 insertions(+), 24 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 3bd215c771..511922c6fc 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -117,7 +117,8 @@ struct rebase_options { struct string_list exec; int allow_empty_message; int rebase_merges, rebase_cousins; - char *strategy, *strategy_opts; + char *strategy; + struct string_list strategy_opts; struct strbuf git_format_patch_opt; int reschedule_failed_exec; int reapply_cherry_picks; @@ -143,6 +144,7 @@ struct rebase_options { .config_autosquash = -1, \ .update_refs = -1, \ .config_update_refs = -1, \ + .strategy_opts = STRING_LIST_INIT_NODUP,\ } static struct replay_opts get_replay_opts(const struct rebase_options *opts) @@ -176,8 +178,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) replay.default_strategy = NULL; } - if (opts->strategy_opts) - parse_strategy_opts(&replay, opts->strategy_opts); + for (size_t i = 0; i < opts->strategy_opts.nr; i++) + strvec_push(&replay.xopts, opts->strategy_opts.items[i].string); if (opts->squash_onto) { oidcpy(&replay.squash_onto, opts->squash_onto); @@ -1013,7 +1015,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) int ignore_whitespace = 0; const char *gpg_sign = NULL; const char *rebase_merges = NULL; - struct string_list strategy_options = STRING_LIST_INIT_NODUP; struct object_id squash_onto; char *squash_onto_name = NULL; char *keep_base_onto_name = NULL; @@ -1122,7 +1123,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) N_("use 'merge-base --fork-point' to refine upstream")), OPT_STRING('s', "strategy", &options.strategy, N_("strategy"), N_("use the given merge strategy")), - OPT_STRING_LIST('X', "strategy-option", &strategy_options, + OPT_STRING_LIST('X', "strategy-option", &options.strategy_opts, N_("option"), N_("pass the argument through to the merge " "strategy")), @@ -1436,23 +1437,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } else { /* REBASE_MERGE */ if (ignore_whitespace) { - string_list_append(&strategy_options, + string_list_append(&options.strategy_opts, "ignore-space-change"); } } - if (strategy_options.nr) { - int i; - - if (!options.strategy) - options.strategy = "ort"; - - strbuf_reset(&buf); - for (i = 0; i < strategy_options.nr; i++) - strbuf_addf(&buf, " --%s", - strategy_options.items[i].string); - options.strategy_opts = xstrdup(buf.buf); - } + if (options.strategy_opts.nr && !options.strategy) + options.strategy = "ort"; if (options.strategy) { options.strategy = xstrdup(options.strategy); @@ -1827,10 +1818,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) free(options.gpg_sign_opt); string_list_clear(&options.exec, 0); free(options.strategy); - free(options.strategy_opts); + string_list_clear(&options.strategy_opts, 0); strbuf_release(&options.git_format_patch_opt); free(squash_onto_name); free(keep_base_onto_name); - string_list_clear(&strategy_options, 0); return !!ret; } diff --git a/sequencer.c b/sequencer.c index 50d469812a..587a473d6e 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2913,7 +2913,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) return 0; } -void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) +static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) { int i; int count; diff --git a/sequencer.h b/sequencer.h index 8a79d6b200..913a0f652d 100644 --- a/sequencer.h +++ b/sequencer.h @@ -252,7 +252,6 @@ int read_oneliner(struct strbuf *buf, const char *path, unsigned flags); int read_author_script(const char *path, char **name, char **email, char **date, int allow_missing); -void parse_strategy_opts(struct replay_opts *opts, char *raw_opts); int write_basic_state(struct replay_opts *opts, const char *head_name, struct commit *onto, const struct object_id *orig_head); void sequencer_post_commit_cleanup(struct repository *r, int verbose); diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh index c3184c9ade..3adf42f47d 100755 --- a/t/t3436-rebase-more-options.sh +++ b/t/t3436-rebase-more-options.sh @@ -41,19 +41,25 @@ test_expect_success 'setup' ' ' test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' + test_when_finished "test_might_fail git rebase --abort" && cat >expect <<-\EOF && fatal: could not split '\''--bad'\'': unclosed quote EOF - test_expect_code 128 git rebase -X"bad argument\"" side main >out 2>actual && + GIT_SEQUENCE_EDITOR="echo break >" \ + git rebase -i -X"bad argument\"" side main && + test_expect_code 128 git rebase --continue >out 2>actual && test_must_be_empty out && test_cmp expect actual ' test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' + test_when_finished "test_might_fail git rebase --abort" && cat >expect <<-\EOF && fatal: could not split '\''--bad'\'': cmdline ends with \ EOF - test_expect_code 128 git rebase -X"bad escape \\" side main >out 2>actual && + GIT_SEQUENCE_EDITOR="echo break >" \ + git rebase -i -X"bad escape \\" side main && + test_expect_code 128 git rebase --continue >out 2>actual && test_must_be_empty out && test_cmp expect actual ' -- 2.40.0.672.gbd2d2ac924 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 4/5] rebase -m: fix serialization of strategy options 2023-04-10 9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood ` (2 preceding siblings ...) 2023-04-10 9:08 ` [PATCH v4 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood @ 2023-04-10 9:08 ` Phillip Wood 2023-04-10 9:08 ` [PATCH v4 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood 2023-04-10 16:46 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Elijah Newren 5 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-10 9:08 UTC (permalink / raw) To: git, Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> To store the strategy options rebase prepends " --" to each one and writes them to a file. To load them it reads the file and passes the contents to split_cmdline(). This roughly mimics the behavior of the scripted rebase but has a couple of limitations, (1) options containing whitespace are not properly preserved (this is true of the scripted rebase as well) and (2) options containing '"' or '\' are incorrectly parsed and may cause the parser to return an error. Fix these limitations by quoting each option when they are stored so that they can be parsed correctly. Now that "--preserve-merges" no longer exist this change also stops prepending "--" to the options when they are stored as that was an artifact of the scripted rebase. These changes are backwards compatible so the files written by an older version of git can still be read. They are also forwards compatible, the file can still be parsed by recent versions of git as they treat the "--" prefix as optional. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- alias.c | 18 +++++++++++++++++ alias.h | 3 +++ sequencer.c | 11 ++++++----- t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++------------ t/t3436-rebase-more-options.sh | 24 ----------------------- 5 files changed, 50 insertions(+), 42 deletions(-) diff --git a/alias.c b/alias.c index e814948ced..54a1a23d2c 100644 --- a/alias.c +++ b/alias.c @@ -3,6 +3,7 @@ #include "alloc.h" #include "config.h" #include "gettext.h" +#include "strbuf.h" #include "string-list.h" struct config_alias_data { @@ -46,6 +47,23 @@ void list_aliases(struct string_list *list) read_early_config(config_alias_cb, &data); } +void quote_cmdline(struct strbuf *buf, const char **argv) +{ + for (const char **argp = argv; *argp; argp++) { + if (argp != argv) + strbuf_addch(buf, ' '); + strbuf_addch(buf, '"'); + for (const char *p = *argp; *p; p++) { + const char c = *p; + + if (c == '"' || c =='\\') + strbuf_addch(buf, '\\'); + strbuf_addch(buf, c); + } + strbuf_addch(buf, '"'); + } +} + #define SPLIT_CMDLINE_BAD_ENDING 1 #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 diff --git a/alias.h b/alias.h index aef4843bb7..43db736484 100644 --- a/alias.h +++ b/alias.h @@ -1,9 +1,12 @@ #ifndef ALIAS_H #define ALIAS_H +struct strbuf; struct string_list; char *alias_lookup(const char *alias); +/* Quote argv so buf can be parsed by split_cmdline() */ +void quote_cmdline(struct strbuf *buf, const char **argv); int split_cmdline(char *cmdline, const char ***argv); /* Takes a negative value returned by split_cmdline */ const char *split_cmdline_strerror(int cmdline_errno); diff --git a/sequencer.c b/sequencer.c index 587a473d6e..fc6ea75895 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2925,7 +2925,7 @@ static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) count = split_cmdline(strategy_opts_string, &argv); if (count < 0) - die(_("could not split '%s': %s"), strategy_opts_string, + BUG("could not split '%s': %s", strategy_opts_string, split_cmdline_strerror(count)); for (i = 0; i < count; i++) { const char *arg = argv[i]; @@ -3049,12 +3049,13 @@ static int read_populate_opts(struct replay_opts *opts) static void write_strategy_opts(struct replay_opts *opts) { - int i; struct strbuf buf = STRBUF_INIT; - for (i = 0; i < opts->xopts.nr; ++i) - strbuf_addf(&buf, " --%s", opts->xopts.v[i]); - + /* + * Quote strategy options so that they can be read correctly + * by split_cmdline(). + */ + quote_cmdline(&buf, opts->xopts.v); write_file(rebase_path_strategy_opts(), "%s\n", buf.buf); strbuf_release(&buf); } diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 130e2f9b55..42c3954125 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -62,29 +62,39 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F2-on-topic-branch && test_commit "commit-new-file-F3-on-topic-branch" F3 32 && - test_when_finished "rm -fr test-bin funny.was.run" && + test_when_finished "rm -fr test-bin" && mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - case "\$1" in --opt) ;; *) exit 2 ;; esac - shift && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && + + write_script test-bin/git-merge-funny <<-\EOF && + printf "[%s]\n" $# "$1" "$2" "$3" "$5" >actual + shift 3 && + exec git merge-recursive "$@" + EOF + + cat >expect <<-\EOF && + [7] + [--option=arg with space] + [--op"tion\] + [--new + line ] + [--] + EOF + + rm -f actual && ( PATH=./test-bin:$PATH && - test_must_fail git rebase -s funny -Xopt main topic + test_must_fail git rebase -s funny -X"option=arg with space" \ + -Xop\"tion\\ -X"new${LF}line " main topic ) && - test -f funny.was.run && - rm funny.was.run && + test_cmp expect actual && + rm actual && echo "Resolved" >F2 && git add F2 && ( PATH=./test-bin:$PATH && git rebase --continue ) && - test -f funny.was.run + test_cmp expect actual ' test_expect_success 'rebase -i --continue handles merge strategy and options' ' diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh index 3adf42f47d..94671d3c46 100755 --- a/t/t3436-rebase-more-options.sh +++ b/t/t3436-rebase-more-options.sh @@ -40,30 +40,6 @@ test_expect_success 'setup' ' EOF ' -test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' ' - test_when_finished "test_might_fail git rebase --abort" && - cat >expect <<-\EOF && - fatal: could not split '\''--bad'\'': unclosed quote - EOF - GIT_SEQUENCE_EDITOR="echo break >" \ - git rebase -i -X"bad argument\"" side main && - test_expect_code 128 git rebase --continue >out 2>actual && - test_must_be_empty out && - test_cmp expect actual -' - -test_expect_success 'bad -X <strategy-option> arguments: bad escape' ' - test_when_finished "test_might_fail git rebase --abort" && - cat >expect <<-\EOF && - fatal: could not split '\''--bad'\'': cmdline ends with \ - EOF - GIT_SEQUENCE_EDITOR="echo break >" \ - git rebase -i -X"bad escape \\" side main && - test_expect_code 128 git rebase --continue >out 2>actual && - test_must_be_empty out && - test_cmp expect actual -' - test_expect_success '--ignore-whitespace works with apply backend' ' test_must_fail git rebase --apply main side && git rebase --abort && -- 2.40.0.672.gbd2d2ac924 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 5/5] rebase: remove a couple of redundant strategy tests 2023-04-10 9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood ` (3 preceding siblings ...) 2023-04-10 9:08 ` [PATCH v4 4/5] rebase -m: fix serialization of strategy options Phillip Wood @ 2023-04-10 9:08 ` Phillip Wood 2023-04-10 16:46 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Elijah Newren 5 siblings, 0 replies; 36+ messages in thread From: Phillip Wood @ 2023-04-10 9:08 UTC (permalink / raw) To: git, Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Elijah Newren, Johannes Schindelin, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Remove a test in t3402 that has been redundant ever since 80ff47957b (rebase: remember strategy and strategy options, 2011-02-06). That commit added a new test, the first part of which (as noted in the old commit message) duplicated an existing test. Also remove a test t3418 that has been redundant since the merge backend was removed in 68aa495b59 (rebase: implement --merge via the interactive machinery, 2018-12-11), since it now tests the same code paths as the preceding test. Helped-by: Elijah Newren <newren@gmail.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- t/t3402-rebase-merge.sh | 21 --------------------- t/t3418-rebase-continue.sh | 32 -------------------------------- 2 files changed, 53 deletions(-) diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh index 7e46f4ca85..79b0640c00 100755 --- a/t/t3402-rebase-merge.sh +++ b/t/t3402-rebase-merge.sh @@ -131,27 +131,6 @@ test_expect_success 'picking rebase' ' esac ' -test_expect_success 'rebase -s funny -Xopt' ' - test_when_finished "rm -fr test-bin funny.was.run" && - mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - case "\$1" in --opt) ;; *) exit 2 ;; esac - shift && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && - git reset --hard && - git checkout -b test-funny main^ && - test_commit funny && - ( - PATH=./test-bin:$PATH && - git rebase -s funny -Xopt main - ) && - test -f funny.was.run -' - test_expect_success 'rebase --skip works with two conflicts in a row' ' git checkout second-side && tr "[A-Z]" "[a-z]" <newfile >tmp && diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index 42c3954125..2d0789e554 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -97,38 +97,6 @@ test_expect_success 'rebase --continue remembers merge strategy and options' ' test_cmp expect actual ' -test_expect_success 'rebase -i --continue handles merge strategy and options' ' - rm -fr .git/rebase-* && - git reset --hard commit-new-file-F2-on-topic-branch && - test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && - test_when_finished "rm -fr test-bin funny.was.run funny.args" && - mkdir test-bin && - cat >test-bin/git-merge-funny <<-EOF && - #!$SHELL_PATH - echo "\$@" >>funny.args - case "\$1" in --opt) ;; *) exit 2 ;; esac - case "\$2" in --foo) ;; *) exit 2 ;; esac - case "\$4" in --) ;; *) exit 2 ;; esac - shift 2 && - >funny.was.run && - exec git merge-recursive "\$@" - EOF - chmod +x test-bin/git-merge-funny && - ( - PATH=./test-bin:$PATH && - test_must_fail git rebase -i -s funny -Xopt -Xfoo main topic - ) && - test -f funny.was.run && - rm funny.was.run && - echo "Resolved" >F2 && - git add F2 && - ( - PATH=./test-bin:$PATH && - git rebase --continue - ) && - test -f funny.was.run -' - test_expect_success 'rebase -r passes merge strategy options correctly' ' rm -fr .git/rebase-* && git reset --hard commit-new-file-F3-on-topic-branch && -- 2.40.0.672.gbd2d2ac924 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/5] rebase: cleanup merge strategy option handling 2023-04-10 9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood ` (4 preceding siblings ...) 2023-04-10 9:08 ` [PATCH v4 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood @ 2023-04-10 16:46 ` Elijah Newren 5 siblings, 0 replies; 36+ messages in thread From: Elijah Newren @ 2023-04-10 16:46 UTC (permalink / raw) To: Phillip Wood Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason, Johannes Schindelin On Mon, Apr 10, 2023 at 2:08 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Cleanup the handling of --strategy-option now that we no longer need to > support "--preserve-merges" and properly quote the argument when saving > it to disc. > > A hopefully final re-roll to fix a memory leak I spotted in V3. Doh, I clearly missed that leak too. > Changes since V3: > - Fixed a memory leak added in V3. The array returned by > split_cmdline() is allocated on the heap so we need to fee it. The > array elements come from the string passed to split_cmdline() so do > not need to be individually freed. And in particular, that string that stores those array elements will be freed by the strbuf_reset() that comes immediately after the read_strategy_opts() call (read_strategy_opts() is the sole caller of parse_strategy_opts()), and the fact that the string is freed is fine since your parse_strategy_opts() passes them to strvec_push() which will xstrdup() them and then own them. All that to say I agree with the fix. ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2023-04-10 16:46 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-15 15:14 [PATCH 0/4] rebase: cleanup merge strategy option handling Phillip Wood 2023-03-15 15:14 ` [PATCH 1/4] rebase: stop reading and writing unnecessary strategy state Phillip Wood 2023-03-15 15:14 ` [PATCH 2/4] rebase -m: cleanup --strategy-option handling Phillip Wood 2023-03-15 15:14 ` [PATCH 3/4] rebase -m: fix serialization of strategy options Phillip Wood 2023-04-01 19:27 ` Elijah Newren 2023-04-04 13:02 ` Phillip Wood 2023-03-15 15:14 ` [PATCH 4/4] rebase: remove a couple of redundant strategy tests Phillip Wood 2023-04-01 19:31 ` Elijah Newren 2023-04-04 13:04 ` Phillip Wood 2023-03-15 16:03 ` [PATCH 0/4] rebase: cleanup merge strategy option handling Junio C Hamano 2023-03-15 16:50 ` Felipe Contreras 2023-04-01 19:32 ` Elijah Newren 2023-04-05 15:21 ` [PATCH v2 0/5] " Phillip Wood 2023-04-05 15:21 ` [PATCH v2 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood 2023-04-05 15:21 ` [PATCH v2 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood 2023-04-07 5:44 ` Elijah Newren 2023-04-05 15:21 ` [PATCH v2 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood 2023-04-05 15:21 ` [PATCH v2 4/5] rebase -m: fix serialization of strategy options Phillip Wood 2023-04-07 5:47 ` Elijah Newren 2023-04-05 15:21 ` [PATCH v2 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood 2023-04-07 5:49 ` [PATCH v2 0/5] rebase: cleanup merge strategy option handling Elijah Newren 2023-04-07 13:57 ` Phillip Wood 2023-04-07 17:53 ` Junio C Hamano 2023-04-07 13:55 ` [PATCH v3 " Phillip Wood 2023-04-07 13:55 ` [PATCH v3 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood 2023-04-07 13:55 ` [PATCH v3 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood 2023-04-07 13:55 ` [PATCH v3 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood 2023-04-07 13:55 ` [PATCH v3 4/5] rebase -m: fix serialization of strategy options Phillip Wood 2023-04-07 13:55 ` [PATCH v3 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood 2023-04-10 9:08 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Phillip Wood 2023-04-10 9:08 ` [PATCH v4 1/5] rebase: stop reading and writing unnecessary strategy state Phillip Wood 2023-04-10 9:08 ` [PATCH v4 2/5] sequencer: use struct strvec to store merge strategy options Phillip Wood 2023-04-10 9:08 ` [PATCH v4 3/5] rebase -m: cleanup --strategy-option handling Phillip Wood 2023-04-10 9:08 ` [PATCH v4 4/5] rebase -m: fix serialization of strategy options Phillip Wood 2023-04-10 9:08 ` [PATCH v4 5/5] rebase: remove a couple of redundant strategy tests Phillip Wood 2023-04-10 16:46 ` [PATCH v4 0/5] rebase: cleanup merge strategy option handling Elijah Newren
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).