| Message ID | 16d3d28243a0480c929ae3740db92ade238dd325.1716983704.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Compile with `-Wwrite-strings` | expand |
Patrick Steinhardt <ps@pks.im> writes: > When computing the rebase strategy we temporarily assign a string > constant to `options.strategy` before we call `xstrdup()` on it. > Furthermore, the default backend is being assigned a string constant via > `REBASE_OPTIONS_INIT`. Both of these will cause warnings once we enable > `-Wwrite-strings`. > > Adapt the code such that we only store allocated strings in those > variables. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/rebase.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) One gripe I have in this change is that it used to be crystal clear what the hardcoded default was (i.e. written in the initialization data), but now you have to follow the program flow to see what the hardcoded default is. > if (options.type == REBASE_UNSPECIFIED) { > - if (!strcmp(options.default_backend, "merge")) > + if (!options.default_backend) > + options.type = REBASE_MERGE; > + else if (!strcmp(options.default_backend, "merge")) > options.type = REBASE_MERGE; > else if (!strcmp(options.default_backend, "apply")) > options.type = REBASE_APPLY;
On Wed, May 29, 2024 at 02:01:08PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > When computing the rebase strategy we temporarily assign a string > > constant to `options.strategy` before we call `xstrdup()` on it. > > Furthermore, the default backend is being assigned a string constant via > > `REBASE_OPTIONS_INIT`. Both of these will cause warnings once we enable > > `-Wwrite-strings`. > > > > Adapt the code such that we only store allocated strings in those > > variables. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > builtin/rebase.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > One gripe I have in this change is that it used to be crystal clear > what the hardcoded default was (i.e. written in the initialization > data), but now you have to follow the program flow to see what the > hardcoded default is. True. We could adapt the macro to instead `xstrdup()` the default backend. But that feels a bit hidden, so I don't think this is a partiuclarly great option. An alternative would be to wrap initialization into a function `rebase_options_init()` that instead allocates the default backend string. We can then also create a `rebase_options_release()` counter part that releases its resources such that this whole thing becomes a bit more self contained. Patrick
diff --git a/builtin/rebase.c b/builtin/rebase.c index 14d4f0a5e6..b05c3b6be3 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -135,7 +135,6 @@ struct rebase_options { .type = REBASE_UNSPECIFIED, \ .empty = EMPTY_UNSPECIFIED, \ .keep_empty = 1, \ - .default_backend = "merge", \ .flags = REBASE_NO_QUIET, \ .git_am_opts = STRVEC_INIT, \ .exec = STRING_LIST_INIT_NODUP, \ @@ -796,6 +795,7 @@ static int rebase_config(const char *var, const char *value, } if (!strcmp(var, "rebase.backend")) { + FREE_AND_NULL(opts->default_backend); return git_config_string(&opts->default_backend, var, value); } @@ -1471,12 +1471,11 @@ 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); + options.strategy = xstrdup("ort"); + else + options.strategy = xstrdup_or_null(options.strategy); + if (options.strategy) imply_merge(&options, "--strategy"); - } if (options.root && !options.onto_name) imply_merge(&options, "--root without --onto"); @@ -1522,7 +1521,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } if (options.type == REBASE_UNSPECIFIED) { - if (!strcmp(options.default_backend, "merge")) + if (!options.default_backend) + options.type = REBASE_MERGE; + else if (!strcmp(options.default_backend, "merge")) options.type = REBASE_MERGE; else if (!strcmp(options.default_backend, "apply")) options.type = REBASE_APPLY; @@ -1833,6 +1834,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) cleanup: strbuf_release(&buf); strbuf_release(&revisions); + free(options.default_backend); free(options.reflog_action); free(options.head_name); strvec_clear(&options.git_am_opts);
When computing the rebase strategy we temporarily assign a string constant to `options.strategy` before we call `xstrdup()` on it. Furthermore, the default backend is being assigned a string constant via `REBASE_OPTIONS_INIT`. Both of these will cause warnings once we enable `-Wwrite-strings`. Adapt the code such that we only store allocated strings in those variables. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/rebase.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)