Re: [PATCH v4 25/27] builtin/rebase: always store allocated string in `options.strategy`

[Phillip Wood <phillip.wood123@gmail.com>]

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");