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

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 5, 2024, 05:40 UTC
Message-ID: <Zl_6anUl_C3W2j2Z@tanuki>
In-Reply-To: <be8a315a-3fa7-40d3-80e1-6a5c01afee70@gmail.com>

On Tue, Jun 04, 2024 at 03:06:38PM +0100, Phillip Wood wrote:

Show 18 quoted lines

> 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