Jeff King <peff@xxxxxxxx> 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().
