@matheusbsbs / matheus-bagliano / commits / fb60b9f37f

sequencer: use struct strvec to store merge strategy options

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Phillip Wood committed Apr 10, 2023 at 10:08 UTC fb60b9f37f87241a886e183f0ec6261debb9c1c5

5 files changed +58 -47

builtin/revert.c

+3 -17

@@ -44,20 +44,6 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)

44 return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;

45 }

47 -static int option_parse_x(const struct option *opt,

48 - const char *arg, int unset)

49 -{

50 - struct replay_opts **opts_ptr = opt->value;

51 - struct replay_opts *opts = *opts_ptr;

52 -

53 - if (unset)

54 - return 0;

55 -

56 - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);

57 - opts->xopts[opts->xopts_nr++] = xstrdup(arg);

58 - return 0;

59 -}

60 -

47 static int option_parse_m(const struct option *opt,

48 const char *arg, int unset)

49 {

@@ -114,8 +100,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)

100 N_("select mainline parent"), option_parse_m),

101 OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),

102 OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")),

117 - OPT_CALLBACK('X', "strategy-option", &opts, N_("option"),

118 - N_("option for merge strategy"), option_parse_x),

103 + OPT_STRVEC('X', "strategy-option", &opts->xopts, N_("option"),

104 + N_("option for merge strategy")),

105 { OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),

106 N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },

107 OPT_END()

@@ -176,7 +162,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)

162 "--signoff", opts->signoff,

163 "--mainline", opts->mainline,

164 "--strategy", opts->strategy ? 1 : 0,

179 - "--strategy-option", opts->xopts ? 1 : 0,

165 + "--strategy-option", opts->xopts.nr ? 1 : 0,

166 "-x", opts->record_origin,

167 "--ff", opts->allow_ff,

168 "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,

parse-options-cb.c

+16

@@ -208,6 +208,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)

208 return 0;

209 }

210

211 +int parse_opt_strvec(const struct option *opt, const char *arg, int unset)

212 +{

213 + struct strvec *v = opt->value;

214 +

215 + if (unset) {

216 + strvec_clear(v);

217 + return 0;

218 + }

219 +

220 + if (!arg)

221 + return -1;

222 +

223 + strvec_push(v, arg);

224 + return 0;

225 +}

226 +

227 int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)

228 {

229 return 0;

parse-options.h

+10

@@ -285,6 +285,15 @@ struct option {

285 .help = (h), \

286 .callback = &parse_opt_string_list, \

287 }

288 +#define OPT_STRVEC(s, l, v, a, h) { \

289 + .type = OPTION_CALLBACK, \

290 + .short_name = (s), \

291 + .long_name = (l), \

292 + .value = (v), \

293 + .argh = (a), \

294 + .help = (h), \

295 + .callback = &parse_opt_strvec, \

296 +}

297 #define OPT_UYN(s, l, v, h) { \

298 .type = OPTION_CALLBACK, \

299 .short_name = (s), \

@@ -478,6 +487,7 @@ int parse_opt_commits(const struct option *, const char *, int);

487 int parse_opt_commit(const struct option *, const char *, int);

488 int parse_opt_tertiary(const struct option *, const char *, int);

489 int parse_opt_string_list(const struct option *, const char *, int);

490 +int parse_opt_strvec(const struct option *, const char *, int);

491 int parse_opt_noop_cb(const struct option *, const char *, int);

492 enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,

493 const struct option *,

sequencer.c

+21 -27

@@ -355,9 +355,7 @@ void replay_opts_release(struct replay_opts *opts)

355 free(opts->reflog_action);

356 free(opts->default_strategy);

357 free(opts->strategy);

358 - for (size_t i = 0; i < opts->xopts_nr; i++)

359 - free(opts->xopts[i]);

360 - free(opts->xopts);

358 + strvec_clear (&opts->xopts);

359 strbuf_release(&opts->current_fixups);

360 if (opts->revs)

361 release_revisions(opts->revs);

@@ -693,8 +691,8 @@ static int do_recursive_merge(struct repository *r,

691 next_tree = next ? get_commit_tree(next) : empty_tree(r);

692 base_tree = base ? get_commit_tree(base) : empty_tree(r);

693

696 - for (i = 0; i < opts->xopts_nr; i++)

697 - parse_merge_opt(&o, opts->xopts[i]);

694 + for (i = 0; i < opts->xopts.nr; i++)

695 + parse_merge_opt(&o, opts->xopts.v[i]);

696

697 if (!opts->strategy || !strcmp(opts->strategy, "ort")) {

698 memset(&result, 0, sizeof(result));

@@ -2325,7 +2323,7 @@ static int do_pick_commit(struct repository *r,

2323 commit_list_insert(base, &common);

2324 commit_list_insert(next, &remotes);

2325 res |= try_merge_command(r, opts->strategy,

2328 - opts->xopts_nr, (const char **)opts->xopts,

2326 + opts->xopts.nr, opts->xopts.v,

2327 common, oid_to_hex(&head), remotes);

2328 free_commit_list(common);

2329 free_commit_list(remotes);

@@ -2898,8 +2896,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)

2896 else if (!strcmp(key, "options.gpg-sign"))

2897 git_config_string_dup(&opts->gpg_sign, key, value);

2898 else if (!strcmp(key, "options.strategy-option")) {

2901 - ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);

2902 - opts->xopts[opts->xopts_nr++] = xstrdup(value);

2899 + strvec_push(&opts->xopts, value);

2900 } else if (!strcmp(key, "options.allow-rerere-auto"))

2901 opts->allow_rerere_auto =

2902 git_config_bool_or_int(key, value, &error_flag) ?

@@ -2920,23 +2917,23 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)

2917 {

2918 int i;

2919 int count;

2920 + const char **argv;

2921 char *strategy_opts_string = raw_opts;

2922

2923 if (*strategy_opts_string == ' ')

2924 strategy_opts_string++;

2925

2928 - count = split_cmdline(strategy_opts_string,

2929 - (const char ***)&opts->xopts);

2926 + count = split_cmdline(strategy_opts_string, &argv);

2927 if (count < 0)

2928 die(_("could not split '%s': %s"), strategy_opts_string,

2929 split_cmdline_strerror(count));

2933 - opts->xopts_nr = count;

2934 - for (i = 0; i < opts->xopts_nr; i++) {

2935 - const char *arg = opts->xopts[i];

2930 + for (i = 0; i < count; i++) {

2931 + const char *arg = argv[i];

2932

2933 skip_prefix(arg, "--", &arg);

2938 - opts->xopts[i] = xstrdup(arg);

2934 + strvec_push(&opts->xopts, arg);

2935 }

2936 + free(argv);

2937 }

2938

2939 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)

3052 int i;

3053 struct strbuf buf = STRBUF_INIT;

3054

3058 - for (i = 0; i < opts->xopts_nr; ++i)

3059 - strbuf_addf(&buf, " --%s", opts->xopts[i]);

3055 + for (i = 0; i < opts->xopts.nr; ++i)

3056 + strbuf_addf(&buf, " --%s", opts->xopts.v[i]);

3057

3058 write_file(rebase_path_strategy_opts(), "%s\n", buf.buf);

3059 strbuf_release(&buf);

@@ -3080,7 +3077,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,

3077 write_file(rebase_path_verbose(), "%s", "");

3078 if (opts->strategy)

3079 write_file(rebase_path_strategy(), "%s\n", opts->strategy);

3083 - if (opts->xopts_nr > 0)

3080 + if (opts->xopts.nr > 0)

3081 write_strategy_opts(opts);

3082

3083 if (opts->allow_rerere_auto == RERERE_AUTOUPDATE)

@@ -3464,13 +3461,10 @@ static int save_opts(struct replay_opts *opts)

3461 if (opts->gpg_sign)

3462 res |= git_config_set_in_file_gently(opts_file,

3463 "options.gpg-sign", opts->gpg_sign);

3467 - if (opts->xopts) {

3468 - int i;

3469 - for (i = 0; i < opts->xopts_nr; i++)

3470 - res |= git_config_set_multivar_in_file_gently(opts_file,

3471 - "options.strategy-option",

3472 - opts->xopts[i], "^$", 0);

3473 - }

3464 + for (size_t i = 0; i < opts->xopts.nr; i++)

3465 + res |= git_config_set_multivar_in_file_gently(opts_file,

3466 + "options.strategy-option",

3467 + opts->xopts.v[i], "^$", 0);

3468 if (opts->allow_rerere_auto)

3469 res |= git_config_set_in_file_gently(opts_file,

3470 "options.allow-rerere-auto",

@@ -3880,7 +3874,7 @@ static int do_merge(struct repository *r,

3874 struct commit *head_commit, *merge_commit, *i;

3875 struct commit_list *bases, *j;

3876 struct commit_list *to_merge = NULL, **tail = &to_merge;

3883 - const char *strategy = !opts->xopts_nr &&

3877 + const char *strategy = !opts->xopts.nr &&

3878 (!opts->strategy ||

3879 !strcmp(opts->strategy, "recursive") ||

3880 !strcmp(opts->strategy, "ort")) ?

@@ -4063,9 +4057,9 @@ static int do_merge(struct repository *r,

4057 strvec_push(&cmd.args, "octopus");

4058 else {

4059 strvec_push(&cmd.args, strategy);

4066 - for (k = 0; k < opts->xopts_nr; k++)

4060 + for (k = 0; k < opts->xopts.nr; k++)

4061 strvec_pushf(&cmd.args,

4068 - "-X%s", opts->xopts[k]);

4062 + "-X%s", opts->xopts.v[k]);

4063 }

4064 if (!(flags & TODO_EDIT_MERGE_MSG))

4065 strvec_push(&cmd.args, "--no-edit");

sequencer.h

+8 -3

@@ -2,6 +2,7 @@

2 #define SEQUENCER_H

4 #include "strbuf.h"

5 +#include "strvec.h"

6 #include "wt-status.h"

8 struct commit;

@@ -60,8 +61,7 @@ struct replay_opts {

61 /* Merge strategy */

62 char *default_strategy; /* from config options */

63 char *strategy;

63 - char **xopts;

64 - size_t xopts_nr, xopts_alloc;

64 + struct strvec xopts;

66 /* Reflog */

67 char *reflog_action;

@@ -80,7 +80,12 @@ struct replay_opts {

80 /* Private use */

81 const char *reflog_message;

82 };

83 -#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }

83 +#define REPLAY_OPTS_INIT { \

84 + .edit = -1, \

85 + .action = -1, \

86 + .current_fixups = STRBUF_INIT, \

87 + .xopts = STRVEC_INIT, \

88 +}

90 /*

91 * Note that ordering matters in this enum. Not only must it match the mapping