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

> Derrick Stolee <derrickstolee@github.com> writes:
>
>>>> -	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.