From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 16 Jul 2025 14:32:50 GMT
Subject: Re: [PATCH] strbuf: add compound literal test balloon
Message-ID: <9532e4de-1f19-43a0-bf9d-9f93ca0fafd5@gmail.com>
In-Reply-To: <xmqqa556x2z4.fsf@gitster.g>

On 14/07/2025 15:26, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> A C99 compound literal creates an unnamed object whose value is given by
>> an initializer list. This allows us to simplify code where we cannot use
>> a designated initalizer because the values of some members of the object
>> need to be calculated first. For example this code from builtin/rebase.c
>>
>> 	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
>> 	struct reset_head_opts ropts = { 0 };
>> 	int ret;
>>
>> 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
>> 		    opts->reflog_action,
>> 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
>> 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
>> 		    opts->reflog_action, opts->head_name);
>> 	ropts.branch = opts->head_name;
>> 	ropts.flags = RESET_HEAD_REFS_ONLY;
>> 	ropts.branch_msg = branch_reflog.buf;
>> 	ropts.head_msg = head_reflog.buf;
>> 	ret = reset_head(the_repository, &ropts);
>>
>> can be be simplified to
>>
>> 	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
>> 	int ret;
>>
>> 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
>> 		    opts->reflog_action,
>> 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
>> 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
>> 		    opts->reflog_action, opts->head_name);
>>          ret = reset_head(the_repository, &(struct reset_head_opts) {
>>                  .branch = opts->head_name,
>>          	.flags = RESET_HEAD_REFS_ONLY,
>>          	.branch_msg = branch_reflog.buf,
>>          	.head_msg = head_reflog.buf,
>>          });
>>
> > One thing the above rewrite did is to make it clear to readers that
> the struct instance is used just once and then immediately got
> discarded.  As long as the object that gets passed this way does not
> hold resources that need to be discarded itself (in other words,
> does not require a call to reset_head_opts_release()), it makes the
> code easier to follow.

That's a good point - this example would not work if reset_head_opts() 
took ownership of `branch_msg` and `head_msg`.

> But once the struct gains members that need to be released, I am not
> sure if this construct does not make it harder to spot leaks.
> Somebody who adds a member to _release() to the struct presumably
> audits and find all places that need to call _release(), but among
> them they find this place---now what?  They need to first convert it
> to the old fashioned way and then call _release() after the
> reset_head() call returns, I guess.
Another possibility is to do something like

	struct reset_head_opts ropts;

	/* ... */

	ropts = (struct replay_head_opts) {
		...
	};

	ret = reset_head(the_repository, &ropts);

	reset_head_opts_release(&ropts);

which initializes all the members of `ropts` in one place though I'm not 
sure if it is really better in practice.
> I am not arguing against all uses of literals---I am just
> anticipating future fallouts of encouraging overuse of this pattern,
> and preparing what we would say when somebody adds a new use to
> inappropriate places.  Simple cases like the initialier should be
> fine.

Yes, we'd want to be careful where we use them. I like the way we use 
designated initializers and this gives us the opportunity to have a 
similar style of initialization in a few more places.

Thanks

Phillip