On Wed, Apr 5, 2023 at 8:22 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>
> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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. :-)
