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

From: Patrick Steinhardt <ps@pks.im>
Date: May 30, 2024, 11:31 UTC
Message-ID: <ZlhjfmZ89nO14jUv@tanuki>
In-Reply-To: <xmqqh6eguycr.fsf@gitster.g>

On Wed, May 29, 2024 at 02:01:08PM -0700, Junio C Hamano wrote:

Show 20 quoted lines

> 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