Am 06.02.23 um 19:55 schrieb Junio C Hamano: > Derrick Stolee <derrickstolee@xxxxxxxxxx> 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. Right you are. René
