| Message ID | f548241960d1f41e010516d68df9107528567514.1717504517.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Compile with `-Wwrite-strings` | expand |
Hi Patrick On 04/06/2024 13:38, Patrick Steinhardt wrote: > The `struct rebase_options::strategy` field is a `char *`, but we do end > up assigning string constants to it in two cases: > > - When being passed a `--strategy=` option via the command line. > > - When being passed a strategy option via `--strategy-option=`, but > not a strategy. > > This will cause warnings once we enable `-Wwrite-strings`. > > Ideally, we'd just convert the field to be a `const char *`. But we also > assign to this field via the GIT_TEST_MERGE_ALGORITHM envvar, which we > have to strdup(3P) into it. > > Instead, refactor the code to make sure that we only ever assign > allocated strings to this field. This looks sensible Thanks Phillip > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/rebase.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 11f276012c..26068cf542 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1063,6 +1063,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > { > struct rebase_options options; > const char *branch_name; > + const char *strategy_opt = NULL; > int ret, flags, total_argc, in_progress = 0; > int keep_base = 0; > int ok_to_skip_pre_rebase = 0; > @@ -1177,7 +1178,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > PARSE_OPT_OPTARG, parse_opt_rebase_merges), > OPT_BOOL(0, "fork-point", &options.fork_point, > N_("use 'merge-base --fork-point' to refine upstream")), > - OPT_STRING('s', "strategy", &options.strategy, > + OPT_STRING('s', "strategy", &strategy_opt, > N_("strategy"), N_("use the given merge strategy")), > OPT_STRING_LIST('X', "strategy-option", &options.strategy_opts, > N_("option"), > @@ -1488,13 +1489,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > } > } > > - if (options.strategy_opts.nr && !options.strategy) > - options.strategy = "ort"; > - > - if (options.strategy) { > - options.strategy = xstrdup(options.strategy); > + if (strategy_opt) > + options.strategy = xstrdup(strategy_opt); > + else if (options.strategy_opts.nr && !options.strategy) > + options.strategy = xstrdup("ort"); > + if (options.strategy) > imply_merge(&options, "--strategy"); > - } > > if (options.root && !options.onto_name) > imply_merge(&options, "--root without --onto");
diff --git a/builtin/rebase.c b/builtin/rebase.c index 11f276012c..26068cf542 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1063,6 +1063,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) { struct rebase_options options; const char *branch_name; + const char *strategy_opt = NULL; int ret, flags, total_argc, in_progress = 0; int keep_base = 0; int ok_to_skip_pre_rebase = 0; @@ -1177,7 +1178,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) PARSE_OPT_OPTARG, parse_opt_rebase_merges), OPT_BOOL(0, "fork-point", &options.fork_point, N_("use 'merge-base --fork-point' to refine upstream")), - OPT_STRING('s', "strategy", &options.strategy, + OPT_STRING('s', "strategy", &strategy_opt, N_("strategy"), N_("use the given merge strategy")), OPT_STRING_LIST('X', "strategy-option", &options.strategy_opts, N_("option"), @@ -1488,13 +1489,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } } - if (options.strategy_opts.nr && !options.strategy) - options.strategy = "ort"; - - if (options.strategy) { - options.strategy = xstrdup(options.strategy); + if (strategy_opt) + options.strategy = xstrdup(strategy_opt); + else if (options.strategy_opts.nr && !options.strategy) + options.strategy = xstrdup("ort"); + if (options.strategy) imply_merge(&options, "--strategy"); - } if (options.root && !options.onto_name) imply_merge(&options, "--root without --onto");
The `struct rebase_options::strategy` field is a `char *`, but we do end up assigning string constants to it in two cases: - When being passed a `--strategy=` option via the command line. - When being passed a strategy option via `--strategy-option=`, but not a strategy. This will cause warnings once we enable `-Wwrite-strings`. Ideally, we'd just convert the field to be a `const char *`. But we also assign to this field via the GIT_TEST_MERGE_ALGORITHM envvar, which we have to strdup(3P) into it. Instead, refactor the code to make sure that we only ever assign allocated strings to this field. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/rebase.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)