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

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

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.

Thanks.