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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

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