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

From: Patrick Steinhardt <[email-masked]>
Date: Thu, 30 May 2024 13:31:10 +0200

On Wed, May 29, 2024 at 02:01:08PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <[email-masked]> 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 <[email-masked]>
> > ---
> >  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