Subject: Re: [PATCH v4 24/27] builtin/rebase: do not assign default backend to non-constant field

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 5 Jun 2024 07:40:58 +0200

On Tue, Jun 04, 2024 at 03:06:38PM +0100, Phillip Wood wrote:
> Hi Patrick
> 
> On 04/06/2024 13:38, Patrick Steinhardt wrote:
> > The `struct rebase_options::default_backend` field is a non-constant
> > string, but is being assigned a constant via `REBASE_OPTIONS_INIT`.
> > Refactor the code to initialize and release options via two functions
> > `rebase_options_init()` and `rebase_options_release()`. Like this, we
> > can easily adapt the former funnction to use `xstrdup()` on the default
> > value without hiding it away in a macro.
> 
> Personally I'd be happy with
> 
> -		.default_backend = "merge",		\
> +		.default_backend = xstrdup("merge"),	\
> 
> rather than adding an init function. I do agree that adding
> rebase_options_release() is a good idea and the rest of the changes look
> good to me

Do we have any other cases where we allocate inside of a `_INIT` style
macro? If so, I'd go with that precedence and just allocate inside of
the macro. But if we don't, then I think I'm leaning more towards the
way I did it in this patch.

Happy to be convinced otherwise, I don't really feel all that strongly
about this. I'm merely aiming for the interface wth least surprises.

Patrick