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. Thanks.
