Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 6, 2023, 18:55 UTC
Message-ID: <xmqq357i8vyv.fsf@gitster.g>
In-Reply-To: <b1a3e684-26f9-0053-9446-751d73e83961@github.com>

Derrick Stolee <derrickstolee@github.com> writes:

Show 21 quoted lines

>>> -	int base_path_len = tree_path->len;
>>> +	size_t base_path_len = tree_path->len;
>>> ...
>>>  				strbuf_setlen(tree_path, base_path_len);
>>> -				strbuf_grow(tree_path, base_path_len + entry.pathlen + 1);
>>> +				strbuf_grow(tree_path, entry.pathlen + 1);
>>>  				strbuf_add(tree_path, entry.path, entry.pathlen);
>>> ...
>> 
>> The size_t conversion is trivially correct.
>
> I agree, and thanks for finding and fixing this issue.
> ...
>> One wonders if (even for this index-related code) we really need such
>> careful management of growth, and could instead do with:
>> 
>> 	strbuf_setlen(tree_path, base_path_len);
>> 	strbuf_add(tree_path, entry.path, entry.pathlen);
>> 	strbuf_addch(tree_path, '/');
>
> This would be my preferred way to go here.

Yup. _setlen() is still very useful to truncate an existing contents in a strbuf, but we should look at each use of _grow() with suspicion that it might be an mistaken attempt to "optimize" where there is not a room for optimization.

There may be cases where tight and exact allocation is desirable but this is not such a codepath. tree_path will not be overly long that we want to avoid extra allocation for giving us slack to prepare for reallocation. And tree_path is passed to recusive calls to further be grown, which is exactly why we would want to use ALLOW_GROW() kind of allocation with slack to amortize the allocation cost.

Thanks.