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

From: Junio C Hamano <[email-masked]>
Date: Wed, 05 Jun 2024 09:11:52 -0700

Patrick Steinhardt <[email-masked]> writes:

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

FWIW, I am OK either way.

Having _init() and _release() may look symmetrical and there _might_
be cases where an initialization with a simple _INIT macro may not
suffice and we must have _init() function.

But otherwise, especially with .designated_initializers, the _INIT
macro makes it slightly easier to read (but not all much for this
particular case, whose initial values have too many non-zero values
and pretty much everything must be spelled out).

Thanks.