Re: [PATCH 17/19] builtin/rebase: adapt code to not assign string constants to non-const

From: Junio C Hamano <gitster@pobox.com>
Date: May 29, 2024, 21:01 UTC
Message-ID: <xmqqh6eguycr.fsf@gitster.g>
In-Reply-To: <16d3d28243a0480c929ae3740db92ade238dd325.1716983704.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 13 quoted lines

> 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.

Show 8 quoted lines

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