Re: [PATCH v2 02/11] clean: do not pass strbuf by value

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2025, 16:44 UTC
Message-ID: <xmqqtt2pisg0.fsf@gitster.g>
In-Reply-To: <20250802083846.GC3711639@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

> On Thu, Jul 31, 2025 at 03:54:24PM -0700, Junio C Hamano wrote:
>
>> This is a catastrophe waiting to happen.  If the callee causes the
>> strbuf to be reallocated, the buf[] the caller has will become
>> dangling, and when the caller does strbuf_release(), it would result
>> in double-free.
>> 
>> Stop calling the function with misleading call-by-value with strbuf.
>
> This is definitely an improvement, though I wonder if we could go
> further.

Yes, but in short, between these two

 - think twice before you pass struct by value
 - do not insist taking the whole struct, take only what you need

lessons, I happened to pick the former one more important to carve in stone than the latter one. Both are valuable guidance we should give to our developers, though.

Show 11 quoted lines

>  		nr = parse_choice(stuff,
>  				  opts->flags & MENU_OPTS_SINGLETON,
> -				  choice,
> +				  choice.buf,
>  				  &chosen);
>  
>  		if (opts->flags & MENU_OPTS_SINGLETON) {
>
> I dunno. Maybe it is nitpicking, but I think "don't take a strbuf if you
> only need a string" is a good general rule. Of course there is only one
> caller here, so flexibility is probably not that important.

But I think we engrave both lessons in the history by keeping this step as-is, do the string_list_split_in_place_f() thing, and then add a new patch to pass just the .buf member to parse_choice().