{"thread":{"id":"59195","subject":"[PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","startedAt":"2023-02-04T19:10:35Z","lastAt":"2023-02-13T13:37:41Z","messageCount":13,"participants":["René Scharfe","Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471500","messageId":"ff3ac119-9b00-746f-470c-8db18c9c61a1@web.de","threadId":"59195","inReplyTo":null,"subject":"[PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-02-04T19:10:23Z","receivedAt":"2023-02-04T19:10:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use size_t to store the original length of the strbuf tree_len, as\nthat's the correct type.\n\nDon't double the allocated size of the strbuf when adding a subdirectory\nname.  Only extend it to fit that name and a slash.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n cache-tree.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 9af457f47c..35f7617164 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -760,7 +760,7 @@ static void prime_cache_tree_rec(struct repository *r,\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n \tint cnt;\n-\tint base_path_len = tree_path->len;\n+\tsize_t base_path_len = tree_path->len;\n\n \toidcpy(&it->oid, &tree->object.oid);\n\n@@ -785,7 +785,7 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\t */\n \t\t\tif (r->index->sparse_index) {\n \t\t\t\tstrbuf_setlen(tree_path, base_path_len);\n-\t\t\t\tstrbuf_grow(tree_path, base_path_len + entry.pathlen + 1);\n+\t\t\t\tstrbuf_grow(tree_path, entry.pathlen + 1);\n \t\t\t\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n \t\t\t\tstrbuf_addch(tree_path, '/');\n \t\t\t}\n--\n2.39.1\n"},{"id":"471544","messageId":"230205.86r0v37qdb.gmgdl@evledraar.gmail.com","threadId":"59195","inReplyTo":"ff3ac119-9b00-746f-470c-8db18c9c61a1@web.de","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-05T21:12:25Z","receivedAt":"2023-02-05T21:29:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Feb 04 2023, René Scharfe wrote:\n\n> Use size_t to store the original length of the strbuf tree_len, as\n> that's the correct type.\n>\n> Don't double the allocated size of the strbuf when adding a subdirectory\n> name.  Only extend it to fit that name and a slash.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  cache-tree.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 9af457f47c..35f7617164 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -760,7 +760,7 @@ static void prime_cache_tree_rec(struct repository *r,\n>  \tstruct tree_desc desc;\n>  \tstruct name_entry entry;\n>  \tint cnt;\n> -\tint base_path_len = tree_path->len;\n> +\tsize_t base_path_len = tree_path->len;\n>\n>  \toidcpy(&it->oid, &tree->object.oid);\n>\n> @@ -785,7 +785,7 @@ static void prime_cache_tree_rec(struct repository *r,\n>  \t\t\t */\n>  \t\t\tif (r->index->sparse_index) {\n>  \t\t\t\tstrbuf_setlen(tree_path, base_path_len);\n> -\t\t\t\tstrbuf_grow(tree_path, base_path_len + entry.pathlen + 1);\n> +\t\t\t\tstrbuf_grow(tree_path, entry.pathlen + 1);\n>  \t\t\t\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n>  \t\t\t\tstrbuf_addch(tree_path, '/');\n>  \t\t\t}\n\nThe size_t conversion is trivially correct.\n\nBut what do you mean with \"don't double the[...]\"? Do you mean that this\nmanages to evade growing these to 24 etc?\n\nOne wonders if (even for this index-related code) we really need such\ncareful management of growth, and could instead do with:\n\n\tstrbuf_setlen(tree_path, base_path_len);\n\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n\tstrbuf_addch(tree_path, '/');\n\nOr even just:\n\n\tstrbuf_addf(tree_path, \"%*.s/\", (int)entry.pathlen, entry.path);\n\nAs e.g. the t1092 test that runs this codepath shows we're looping\nthrough the index entires here and will (in that case) handle names & a\n\"tree_path\" like (these are the entry.path values):\n\n\t\"\"\n\t\"before\"\n\nWhich here are turned into:\n\n\t\"before/\"\n\nBut both before & after your change we'll grow the \"alloc\" to 24, which\nisn't surprising, as both the string length of (sans slash) 6 and 7 plus\n1 for \"\\0\" is rounde up to 24 with the standard growth pattern.\n\nSo I think if we're doing a \"while-at-it\" fixing of the off-by-one here\nwe might be better of asking whether it's needed at all, and whether\nthis case can't just be left to the growth smartness of the\nstrbuf+ALLOC_GROW() API instead.\n"},{"id":"471559","messageId":"b1a3e684-26f9-0053-9446-751d73e83961@github.com","threadId":"59195","inReplyTo":"230205.86r0v37qdb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-02-06T15:27:53Z","receivedAt":"2023-02-06T15:28:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/5/2023 4:12 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sat, Feb 04 2023, René Scharfe wrote:\n> \n>> Use size_t to store the original length of the strbuf tree_len, as\n>> that's the correct type.\n>>\n>> Don't double the allocated size of the strbuf when adding a subdirectory\n>> name.  Only extend it to fit that name and a slash.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  cache-tree.c | 4 ++--\n>>  1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/cache-tree.c b/cache-tree.c\n>> index 9af457f47c..35f7617164 100644\n>> --- a/cache-tree.c\n>> +++ b/cache-tree.c\n>> @@ -760,7 +760,7 @@ static void prime_cache_tree_rec(struct repository *r,\n>>  \tstruct tree_desc desc;\n>>  \tstruct name_entry entry;\n>>  \tint cnt;\n>> -\tint base_path_len = tree_path->len;\n>> +\tsize_t base_path_len = tree_path->len;\n>>\n>>  \toidcpy(&it->oid, &tree->object.oid);\n>>\n>> @@ -785,7 +785,7 @@ static void prime_cache_tree_rec(struct repository *r,\n>>  \t\t\t */\n>>  \t\t\tif (r->index->sparse_index) {\n>>  \t\t\t\tstrbuf_setlen(tree_path, base_path_len);\n>> -\t\t\t\tstrbuf_grow(tree_path, base_path_len + entry.pathlen + 1);\n>> +\t\t\t\tstrbuf_grow(tree_path, entry.pathlen + 1);\n>>  \t\t\t\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n>>  \t\t\t\tstrbuf_addch(tree_path, '/');\n>>  \t\t\t}\n> \n> The size_t conversion is trivially correct.\n\nI agree, and thanks for finding and fixing this issue.\n\nUpon reading strbuf_grow(), I would expect it to work the same\nas ALLOC_GROW(), but its documentation clearly states a very\ndifferent result.\n\n> One wonders if (even for this index-related code) we really need such\n> careful management of growth, and could instead do with:\n> \n> \tstrbuf_setlen(tree_path, base_path_len);\n> \tstrbuf_add(tree_path, entry.path, entry.pathlen);\n> \tstrbuf_addch(tree_path, '/');\n\nThis would be my preferred way to go here.\n\n> Or even just:\n> \n> \tstrbuf_addf(tree_path, \"%*.s/\", (int)entry.pathlen, entry.path);\n\nPlease do not add \"addf\" functions that can be run in tight loops.\nIt's faster to do strbuf_add() followed by strbuf_addch().\n\nThanks,\n-Stolee\n"},{"id":"471563","messageId":"230206.86a61q7o11.gmgdl@evledraar.gmail.com","threadId":"59195","inReplyTo":"b1a3e684-26f9-0053-9446-751d73e83961@github.com","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-06T16:18:06Z","receivedAt":"2023-02-06T16:32:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 06 2023, Derrick Stolee wrote:\n\n> On 2/5/2023 4:12 PM, Ævar Arnfjörð Bjarmason wrote:\n> [...]\n>> One wonders if (even for this index-related code) we really need such\n>> careful management of growth, and could instead do with:\n>> \n>> \tstrbuf_setlen(tree_path, base_path_len);\n>> \tstrbuf_add(tree_path, entry.path, entry.pathlen);\n>> \tstrbuf_addch(tree_path, '/');\n>\n> This would be my preferred way to go here.\n\n*nod*\n\n>> Or even just:\n>> \n>> \tstrbuf_addf(tree_path, \"%*.s/\", (int)entry.pathlen, entry.path);\n>\n> Please do not add \"addf\" functions that can be run in tight loops.\n> It's faster to do strbuf_add() followed by strbuf_addch().\n\nGood point.\n\nI wondered just how much slower, and it's up to 3x! At least according\nto this[1] artificial test case (where I usurped a random test helper).\n\nI wondered if we could just handle some common strbuf_addf() cases\nourselves, and the benchmark shows (manually annotated, too lazy to set\nup the -n option):\n\n\tgit hyperfine -L rev HEAD~5,HEAD~4,HEAD~3,HEAD~2,HEAD~1,HEAD~0 -s 'make CFLAGS=-O3' './t/helper/test-tool online-cpus' -r 3\n\t[...]\n\tSummary\n\t  './t/helper/test-tool online-cpus' in 'HEAD~0' ran <== strbuf_add() + strbuf_addch()\n\t    1.06 ± 0.11 times faster than './t/helper/test-tool online-cpus' in 'HEAD~1' <== strbuf_addstr() + strbuf_addch()\n\t    1.18 ± 0.12 times faster than './t/helper/test-tool online-cpus' in 'HEAD~4' <== hand optimized strbuf_addf() for \"%sC\"\n\t    1.33 ± 0.18 times faster than './t/helper/test-tool online-cpus' in 'HEAD~2' <== hand optimized strbuf_addf() for \"%*sC\"\n\t    2.63 ± 0.05 times faster than './t/helper/test-tool online-cpus' in 'HEAD~5' <== strbuf_addf(\"%s/\")\n\t    2.92 ± 0.25 times faster than './t/helper/test-tool online-cpus' in 'HEAD~3' <== strbuf_addf(\"%*s/\")\n\nThe \"hand optimization\" just being a very stupid handling of \"%sC\" for\narbitrary values of a single char \"C\", and ditto for \"%*sC\" (which\ncuriously is slower here).\n\nSo, for truly hot loops we'd still want to use the add + addch, but if\nanyone's interested (hashtag #leftoverbits) it looks like we could get\nsome easy wins (and reduction in code size, as we could stop worrying\nabout addf being slow in most cases) with some very dumb minimal\nvaddf(), which could handle these cases (but not anything involving\npadding etc.).\n\nI didn't dig, but wouldn't be surprised if the reason is that C\nlibraries need to carry a relatively fat & general sprintf() for all\nthose edge cases, locale handling etc, whereas most of our use could\ntrivially be represented as some sequence of addstr()/addf() etc.\n\nAnother interesting approach (and this is very #leftoverbits) would be\nto perform the same optimization with coccinelle.\n\nI.e. our current use of it is purely \"this code X should be written like\nY, and we should commit Y\".\n\nBut there's no reason for why we couldn't effectively implement our own\ncompiler optimizations for our own APIs with it, so just grab \"%s/\" etc,\nunpack that in OCaml, then emit strbuf_add() + strbuf_addch(), and that\nwould be what the C compiler would see.\n\n1.\n\t\n\t9d23ffb1117 addf + nolen\n\tdiff --git a/t/helper/test-online-cpus.c b/t/helper/test-online-cpus.c\n\tindex 8cb0d53840f..c802ec579d0 100644\n\t--- a/t/helper/test-online-cpus.c\n\t+++ b/t/helper/test-online-cpus.c\n\t@@ -1,9 +1,17 @@\n\t #include \"test-tool.h\"\n\t #include \"git-compat-util.h\"\n\t #include \"thread-utils.h\"\n*\t+#include \"strbuf.h\"\n\t \n\t int cmd__online_cpus(int argc, const char **argv)\n\t {\n\t-\tprintf(\"%d\\n\", online_cpus());\n\t+\tstruct strbuf sb = STRBUF_INIT;\n\t+\tconst char *const str = \"Hello, World\";\n\t+\n\t+\tfor (size_t i = 0; i < 10000000; i++) {\n\t+\t\tstrbuf_reset(&sb);\n\t+\t\tstrbuf_addf(&sb, \"%s/\", str);\n\t+\t\tputs(sb.buf);\n\t+\t}\n\t \treturn 0;\n\t }\n\t9f74eff5623 addf + nolen optimize\n\tdiff --git a/strbuf.c b/strbuf.c\n\tindex c383f41a3c5..750e5e6a5b4 100644\n\t--- a/strbuf.c\n\t+++ b/strbuf.c\n\t@@ -332,8 +332,16 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n)\n\t void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n\t {\n\t \tva_list ap;\n\t+\n\t \tva_start(ap, fmt);\n\t-\tstrbuf_vaddf(sb, fmt, ap);\n\t+\tif (*fmt == '%' && *(fmt + 1) == 's' && *(fmt + 2) && !*(fmt + 3)) {\n\t+\t\tconst char *arg = va_arg(ap, const char *);\n\t+\n\t+\t\tstrbuf_addstr(sb, arg);\n\t+\t\tstrbuf_addch(sb, *(fmt + 2));\n\t+\t} else {\n\t+\t\tstrbuf_vaddf(sb, fmt, ap);\n\t+\t}\n\t \tva_end(ap);\n\t }\n\t \n\tca60bb9b479 addf + len\n\tdiff --git a/t/helper/test-online-cpus.c b/t/helper/test-online-cpus.c\n\tindex c802ec579d0..7257e622015 100644\n\t--- a/t/helper/test-online-cpus.c\n\t+++ b/t/helper/test-online-cpus.c\n\t@@ -7,10 +7,11 @@ int cmd__online_cpus(int argc, const char **argv)\n\t {\n\t \tstruct strbuf sb = STRBUF_INIT;\n\t \tconst char *const str = \"Hello, World\";\n\t+\tconst size_t len = strlen(str);\n\t \n\t \tfor (size_t i = 0; i < 10000000; i++) {\n\t \t\tstrbuf_reset(&sb);\n\t-\t\tstrbuf_addf(&sb, \"%s/\", str);\n\t+\t\tstrbuf_addf(&sb, \"%*s/\", (int)len, str);\n\t \t\tputs(sb.buf);\n\t \t}\n\t \treturn 0;\n\t1f47987d095 addf + len optimize\n\tdiff --git a/strbuf.c b/strbuf.c\n\tindex 750e5e6a5b4..88801268f7a 100644\n\t--- a/strbuf.c\n\t+++ b/strbuf.c\n\t@@ -334,11 +334,16 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n\t \tva_list ap;\n\t \n\t \tva_start(ap, fmt);\n\t-\tif (*fmt == '%' && *(fmt + 1) == 's' && *(fmt + 2) && !*(fmt + 3)) {\n\t+\tif (*fmt == '%' &&\n\t+\t    *(fmt + 1) == '*' &&\n\t+\t    *(fmt + 2) == 's' &&\n\t+\t    *(fmt + 3) &&\n\t+\t    !*(fmt + 4)) {\n\t+\t\tint len = va_arg(ap, int);\n\t \t\tconst char *arg = va_arg(ap, const char *);\n\t \n\t-\t\tstrbuf_addstr(sb, arg);\n\t-\t\tstrbuf_addch(sb, *(fmt + 2));\n\t+\t\tstrbuf_add(sb, arg, len);\n\t+\t\tstrbuf_addch(sb, *(fmt + 3));\n\t \t} else {\n\t \t\tstrbuf_vaddf(sb, fmt, ap);\n\t \t}\n\t55c698c0b95 addstr\n\tdiff --git a/t/helper/test-online-cpus.c b/t/helper/test-online-cpus.c\n\tindex 7257e622015..2716b44ca15 100644\n\t--- a/t/helper/test-online-cpus.c\n\t+++ b/t/helper/test-online-cpus.c\n\t@@ -7,11 +7,11 @@ int cmd__online_cpus(int argc, const char **argv)\n\t {\n\t \tstruct strbuf sb = STRBUF_INIT;\n\t \tconst char *const str = \"Hello, World\";\n\t-\tconst size_t len = strlen(str);\n\t \n\t \tfor (size_t i = 0; i < 10000000; i++) {\n\t \t\tstrbuf_reset(&sb);\n\t-\t\tstrbuf_addf(&sb, \"%*s/\", (int)len, str);\n\t+\t\tstrbuf_addstr(&sb, str);\n\t+\t\tstrbuf_addch(&sb, '/');\n\t \t\tputs(sb.buf);\n\t \t}\n\t \treturn 0;\n\tb17fb99bf7e (HEAD -> master) add\n\tdiff --git a/t/helper/test-online-cpus.c b/t/helper/test-online-cpus.c\n\tindex 2716b44ca15..5e52b622c4d 100644\n\t--- a/t/helper/test-online-cpus.c\n\t+++ b/t/helper/test-online-cpus.c\n\t@@ -7,10 +7,11 @@ int cmd__online_cpus(int argc, const char **argv)\n\t {\n\t \tstruct strbuf sb = STRBUF_INIT;\n\t \tconst char *const str = \"Hello, World\";\n\t+\tconst size_t len = strlen(str);\n\t \n\t \tfor (size_t i = 0; i < 10000000; i++) {\n\t \t\tstrbuf_reset(&sb);\n\t-\t\tstrbuf_addstr(&sb, str);\n\t+\t\tstrbuf_add(&sb, str, len);\n\t \t\tstrbuf_addch(&sb, '/');\n\t \t\tputs(sb.buf);\n\t \t}\n"},{"id":"471575","messageId":"xmqq357i8vyv.fsf@gitster.g","threadId":"59195","inReplyTo":"b1a3e684-26f9-0053-9446-751d73e83961@github.com","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-06T18:55:36Z","receivedAt":"2023-02-06T18:55:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>>> -\tint base_path_len = tree_path->len;\n>>> +\tsize_t base_path_len = tree_path->len;\n>>> ...\n>>>  \t\t\t\tstrbuf_setlen(tree_path, base_path_len);\n>>> -\t\t\t\tstrbuf_grow(tree_path, base_path_len + entry.pathlen + 1);\n>>> +\t\t\t\tstrbuf_grow(tree_path, entry.pathlen + 1);\n>>>  \t\t\t\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n>>> ...\n>> \n>> The size_t conversion is trivially correct.\n>\n> I agree, and thanks for finding and fixing this issue.\n> ...\n>> One wonders if (even for this index-related code) we really need such\n>> careful management of growth, and could instead do with:\n>> \n>> \tstrbuf_setlen(tree_path, base_path_len);\n>> \tstrbuf_add(tree_path, entry.path, entry.pathlen);\n>> \tstrbuf_addch(tree_path, '/');\n>\n> This would be my preferred way to go here.\n\nYup.  _setlen() is still very useful to truncate an existing\ncontents in a strbuf, but we should look at each use of _grow() with\nsuspicion that it might be an mistaken attempt to \"optimize\" where\nthere is not a room for optimization.\n\nThere may be cases where tight and exact allocation is desirable but\nthis is not such a codepath.  tree_path will not be overly long that\nwe want to avoid extra allocation for giving us slack to prepare for\nreallocation.  And tree_path is passed to recusive calls to further\nbe grown, which is exactly why we would want to use ALLOW_GROW() kind\nof allocation with slack to amortize the allocation cost.\n\nThanks.\n"},{"id":"471943","messageId":"e6785722-da0d-4b7c-fcd2-06e76b1f1757@web.de","threadId":"59195","inReplyTo":"xmqq357i8vyv.fsf@gitster.g","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-02-10T20:20:22Z","receivedAt":"2023-02-10T20:21:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.02.23 um 19:55 schrieb Junio C Hamano:\n> Derrick Stolee <derrickstolee@github.com> writes:\n>\n>>>> -\tint base_path_len = tree_path->len;\n>>>> +\tsize_t base_path_len = tree_path->len;\n>>>> ...\n>>>>  \t\t\t\tstrbuf_setlen(tree_path, base_path_len);\n>>>> -\t\t\t\tstrbuf_grow(tree_path, base_path_len + entry.pathlen + 1);\n>>>> +\t\t\t\tstrbuf_grow(tree_path, entry.pathlen + 1);\n>>>>  \t\t\t\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n>>>> ...\n>>>\n>>> The size_t conversion is trivially correct.\n>>\n>> I agree, and thanks for finding and fixing this issue.\n>> ...\n>>> One wonders if (even for this index-related code) we really need such\n>>> careful management of growth, and could instead do with:\n>>>\n>>> \tstrbuf_setlen(tree_path, base_path_len);\n>>> \tstrbuf_add(tree_path, entry.path, entry.pathlen);\n>>> \tstrbuf_addch(tree_path, '/');\n>>\n>> This would be my preferred way to go here.\n>\n> Yup.  _setlen() is still very useful to truncate an existing\n> contents in a strbuf, but we should look at each use of _grow() with\n> suspicion that it might be an mistaken attempt to \"optimize\" where\n> there is not a room for optimization.\n>\n> There may be cases where tight and exact allocation is desirable but\n> this is not such a codepath.  tree_path will not be overly long that\n> we want to avoid extra allocation for giving us slack to prepare for\n> reallocation.  And tree_path is passed to recusive calls to further\n> be grown, which is exactly why we would want to use ALLOW_GROW() kind\n> of allocation with slack to amortize the allocation cost.\n\nRight you are.\n\nRené\n"},{"id":"471942","messageId":"a37623c8-d2fb-aec6-3423-2d402d717959@web.de","threadId":"59195","inReplyTo":"ff3ac119-9b00-746f-470c-8db18c9c61a1@web.de","subject":"[PATCH v2] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-02-10T20:20:30Z","receivedAt":"2023-02-10T20:21:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use size_t to store the original length of the strbuf tree_len, as\nthat's the correct type.\n\nDon't double the allocated size of the strbuf when adding a subdirectory\nname.  And the chance of the trailing slash fitting in the slack left by\nstrbuf_add() is very high, so stop pre-growing the strbuf at all.\n\nSuggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v1: Remove strbuf_grow() call\n\n cache-tree.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 9af457f47c..88c2c04f87 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -760,7 +760,7 @@ static void prime_cache_tree_rec(struct repository *r,\n \tstruct tree_desc desc;\n \tstruct name_entry entry;\n \tint cnt;\n-\tint base_path_len = tree_path->len;\n+\tsize_t base_path_len = tree_path->len;\n\n \toidcpy(&it->oid, &tree->object.oid);\n\n@@ -785,7 +785,6 @@ static void prime_cache_tree_rec(struct repository *r,\n \t\t\t */\n \t\t\tif (r->index->sparse_index) {\n \t\t\t\tstrbuf_setlen(tree_path, base_path_len);\n-\t\t\t\tstrbuf_grow(tree_path, base_path_len + entry.pathlen + 1);\n \t\t\t\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n \t\t\t\tstrbuf_addch(tree_path, '/');\n \t\t\t}\n--\n2.39.1\n"},{"id":"471944","messageId":"850c94d5-973a-335e-903f-e0ab7e52f37e@web.de","threadId":"59195","inReplyTo":"230205.86r0v37qdb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-02-10T20:20:19Z","receivedAt":"2023-02-10T20:21:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.02.23 um 22:12 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, Feb 04 2023, René Scharfe wrote:\n>\n>> Use size_t to store the original length of the strbuf tree_len, as\n>> that's the correct type.\n>>\n>> Don't double the allocated size of the strbuf when adding a subdirectory\n>> name.  Only extend it to fit that name and a slash.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  cache-tree.c | 4 ++--\n>>  1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/cache-tree.c b/cache-tree.c\n>> index 9af457f47c..35f7617164 100644\n>> --- a/cache-tree.c\n>> +++ b/cache-tree.c\n>> @@ -760,7 +760,7 @@ static void prime_cache_tree_rec(struct repository *r,\n>>  \tstruct tree_desc desc;\n>>  \tstruct name_entry entry;\n>>  \tint cnt;\n>> -\tint base_path_len = tree_path->len;\n>> +\tsize_t base_path_len = tree_path->len;\n>>\n>>  \toidcpy(&it->oid, &tree->object.oid);\n>>\n>> @@ -785,7 +785,7 @@ static void prime_cache_tree_rec(struct repository *r,\n>>  \t\t\t */\n>>  \t\t\tif (r->index->sparse_index) {\n>>  \t\t\t\tstrbuf_setlen(tree_path, base_path_len);\n>> -\t\t\t\tstrbuf_grow(tree_path, base_path_len + entry.pathlen + 1);\n>> +\t\t\t\tstrbuf_grow(tree_path, entry.pathlen + 1);\n>>  \t\t\t\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n>>  \t\t\t\tstrbuf_addch(tree_path, '/');\n>>  \t\t\t}\n>\n> The size_t conversion is trivially correct.\n>\n> But what do you mean with \"don't double the[...]\"? Do you mean that this\n> manages to evade growing these to 24 etc?\n\nstrbuf_setlen() truncates the string to the directory name.  strbuf_grow() then\nmakes enough room to add that directory name again (that's what I mean with\n\"double\") plus the entry path.  We don't add the directory name a second time,\nso we don't need to make room for it.\n\nRené\n"},{"id":"471945","messageId":"xmqqwn4pgt1f.fsf@gitster.g","threadId":"59195","inReplyTo":"850c94d5-973a-335e-903f-e0ab7e52f37e@web.de","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-10T20:33:00Z","receivedAt":"2023-02-10T20:33:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>>> -\t\t\t\tstrbuf_grow(tree_path, base_path_len + entry.pathlen + 1);\n>>> +\t\t\t\tstrbuf_grow(tree_path, entry.pathlen + 1);\n>>>  \t\t\t\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n>>>  \t\t\t\tstrbuf_addch(tree_path, '/');\n>>>  \t\t\t}\n>>\n>> The size_t conversion is trivially correct.\n>>\n>> But what do you mean with \"don't double the[...]\"? Do you mean that this\n>> manages to evade growing these to 24 etc?\n>\n> strbuf_setlen() truncates the string to the directory name.  strbuf_grow() then\n> makes enough room to add that directory name again (that's what I mean with\n> \"double\") plus the entry path.  We don't add the directory name a second time,\n> so we don't need to make room for it.\n\nYeah, I think I made the same mistake number of years ago, thinking\nthat strbuf_grow() was to grow the buffer to the given size, but in\nreality it is to grow the buffer by the given size, which felt a bit\nunnatural, at least to me.  I do not feel it too strongly but we\nmight want to rename _grow() to _grow_by() and make _grow() call it\nwhile giving deprecation warning X-<.\n\nThere are ~45 calls to strbuf_grow() in C files other than strbuf.c;\nI suspect probably a half or more of them can and should be removed\nto reduce the resulting code size without hurting anything.\n\n"},{"id":"471960","messageId":"Y+b6TAmxel48QHJa@coredump.intra.peff.net","threadId":"59195","inReplyTo":"xmqqwn4pgt1f.fsf@gitster.g","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-11T02:15:40Z","receivedAt":"2023-02-11T02:15:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2023 at 12:33:00PM -0800, Junio C Hamano wrote:\n\n> > strbuf_setlen() truncates the string to the directory name.  strbuf_grow() then\n> > makes enough room to add that directory name again (that's what I mean with\n> > \"double\") plus the entry path.  We don't add the directory name a second time,\n> > so we don't need to make room for it.\n> \n> Yeah, I think I made the same mistake number of years ago, thinking\n> that strbuf_grow() was to grow the buffer to the given size, but in\n> reality it is to grow the buffer by the given size, which felt a bit\n> unnatural, at least to me.  I do not feel it too strongly but we\n> might want to rename _grow() to _grow_by() and make _grow() call it\n> while giving deprecation warning X-<.\n\nHaving been confused by that myself, I would be happy to see such a\nname change.\n\n> There are ~45 calls to strbuf_grow() in C files other than strbuf.c;\n> I suspect probably a half or more of them can and should be removed\n> to reduce the resulting code size without hurting anything.\n\nMy gut feeling is that your suspicion is giving strbuf_grow() users too\nmuch credit. ;) And having looked at the first 7 grep hits, every single\none of them seemed pointless to me.\n\nI wonder if these would make a good #leftoverbits / micro-project\ncandidate.\n\n-Peff\n"},{"id":"471966","messageId":"xmqqa61kgbrl.fsf@gitster.g","threadId":"59195","inReplyTo":"Y+b6TAmxel48QHJa@coredump.intra.peff.net","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-11T02:46:06Z","receivedAt":"2023-02-11T02:46:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> ...  I do not feel it too strongly but we\n>> might want to rename _grow() to _grow_by() and make _grow() call it\n>> while giving deprecation warning X-<.\n>\n> Having been confused by that myself, I would be happy to see such a\n> name change.\n\nIf we did not know how useless explicit growth control is, we would\nlikely have a pair of helpers, _grow_by() and _grow_to(), but given\nthat ...\n\n>> There are ~45 calls to strbuf_grow() in C files other than strbuf.c;\n>> I suspect probably a half or more of them can and should be removed\n>> to reduce the resulting code size without hurting anything.\n>\n> My gut feeling is that your suspicion is giving strbuf_grow() users too\n> much credit. ;) And having looked at the first 7 grep hits, every single\n> one of them seemed pointless to me.\n\n... we'd only have a very limited number of callers for which the\nhelper makes sense, I am not sure if it is even worth the renaming.\n\nOr just rename it to _grow_to() while fixing what it does, as\ngrow_to() is what programmers would expect naturally?\n\n> I wonder if these would make a good #leftoverbits / micro-project\n> candidate.\n\nThe task is to pick one or two from these 45 hits, analyze what\nwould happen if we remove the _grow() calls.  For many of them, the\nresult of such analysis would say the calls are pointless, but for\nsome, there hopefully are solid reasons why explicit sizing makes\nsense.  The former will be just removed, while the latter will be\nkept with a new in-code comment to record why it is worth having the\ncall.  The parameter may need to be updated for them at the same\ntime.\n\nIt can be done extremely poorly without breaking anything in the\ntest suite, or it can be done expertly.  Unless the result are\nreviewed competently, it may make a rather poor micro-project\nexperience.\n\nSo, I dunno.\n"},{"id":"471991","messageId":"49071578-0007-bd9d-f286-58abfa848991@web.de","threadId":"59195","inReplyTo":"230206.86a61q7o11.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-02-12T11:20:49Z","receivedAt":"2023-02-12T11:21:02Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.02.23 um 17:18 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Mon, Feb 06 2023, Derrick Stolee wrote:\n>\n>> On 2/5/2023 4:12 PM, Ævar Arnfjörð Bjarmason wrote:\n>\n>>> Or even just:\n>>>\n>>> \tstrbuf_addf(tree_path, \"%*.s/\", (int)entry.pathlen, entry.path);\n>>\n>> Please do not add \"addf\" functions that can be run in tight loops.\n>> It's faster to do strbuf_add() followed by strbuf_addch().\n>\n> Good point.\n>\n> I wondered just how much slower, and it's up to 3x! At least according\n> to this[1] artificial test case (where I usurped a random test helper).\n>\n> I wondered if we could just handle some common strbuf_addf() cases\n> ourselves, and the benchmark shows (manually annotated, too lazy to set\n> up the -n option):\n>\n> \tgit hyperfine -L rev HEAD~5,HEAD~4,HEAD~3,HEAD~2,HEAD~1,HEAD~0 -s 'make CFLAGS=-O3' './t/helper/test-tool online-cpus' -r 3\n> \t[...]\n> \tSummary\n> \t  './t/helper/test-tool online-cpus' in 'HEAD~0' ran <== strbuf_add() + strbuf_addch()\n> \t    1.06 ± 0.11 times faster than './t/helper/test-tool online-cpus' in 'HEAD~1' <== strbuf_addstr() + strbuf_addch()\n> \t    1.18 ± 0.12 times faster than './t/helper/test-tool online-cpus' in 'HEAD~4' <== hand optimized strbuf_addf() for \"%sC\"\n> \t    1.33 ± 0.18 times faster than './t/helper/test-tool online-cpus' in 'HEAD~2' <== hand optimized strbuf_addf() for \"%*sC\"\n> \t    2.63 ± 0.05 times faster than './t/helper/test-tool online-cpus' in 'HEAD~5' <== strbuf_addf(\"%s/\")\n> \t    2.92 ± 0.25 times faster than './t/helper/test-tool online-cpus' in 'HEAD~3' <== strbuf_addf(\"%*s/\")\n\nWoah!\n\n> The \"hand optimization\" just being a very stupid handling of \"%sC\" for\n> arbitrary values of a single char \"C\", and ditto for \"%*sC\" (which\n> curiously is slower here).\n\n\"%*s\" adds padding if needed, your version doesn't.  Perhaps you thought\nof \"%.*s\"?  That might be relevant because for \"%*s\" vsnprintf(3) needs\nto run strlen(3) again on the argument, while for \"%.*s\" it can stop\nwhen the given length is reached.\n\n> So, for truly hot loops we'd still want to use the add + addch, but if\n> anyone's interested (hashtag #leftoverbits) it looks like we could get\n> some easy wins (and reduction in code size, as we could stop worrying\n> about addf being slow in most cases) with some very dumb minimal\n> vaddf(), which could handle these cases (but not anything involving\n> padding etc.).\n>\n> I didn't dig, but wouldn't be surprised if the reason is that C\n> libraries need to carry a relatively fat & general sprintf() for all\n> those edge cases, locale handling etc, whereas most of our use could\n> trivially be represented as some sequence of addstr()/addf() etc.\n\nIf that's the reason then resisting the urge to handle ever more cases\nin strbuf_addf() would be quite important.\n\n> Another interesting approach (and this is very #leftoverbits) would be\n> to perform the same optimization with coccinelle.\n>\n> I.e. our current use of it is purely \"this code X should be written like\n> Y, and we should commit Y\".\n>\n> But there's no reason for why we couldn't effectively implement our own\n> compiler optimizations for our own APIs with it, so just grab \"%s/\" etc,\n> unpack that in OCaml, then emit strbuf_add() + strbuf_addch(), and that\n> would be what the C compiler would see.\n\nExtracting the %s is technically possible using a semantic patch without\nscripting:\n\n   @@\n   expression sb, str;\n   format fmt =~ \"^s$\";\n   @@\n   + strbuf_addstr(sb, str);\n     strbuf_addf(sb, \"%@fmt@...\"\n   - , str\n   + + 2\n     );\n\nThe \"+ 2\" is ugly and runs afoul of compiler warning -Wstring-plus-int,\nthough.  Resolving this probably requires Python scripting as in\nhttps://github.com/coccinelle/coccinelle/blob/master/demos/format.cocci,\nor the OCaml magic you have in mind.  I have to admit that I don't even\nunderstand the linked examples, however. :-/\n\nThe warning can be avoided by using an array subscription, by the way,\nbut that's even uglier:\n\n   @@\n   expression sb, str;\n   format fmt =~ \"^s$\";\n   @@\n   + strbuf_addstr(sb, str);\n     strbuf_addf(sb,\n   + &\n     \"%@fmt@...\"\n   - , str\n   + [2]\n     );\n\n>\n> 1.\n>\n> \t9d23ffb1117 addf + nolen\n> \tdiff --git a/t/helper/test-online-cpus.c b/t/helper/test-online-cpus.c\n> \tindex 8cb0d53840f..c802ec579d0 100644\n> \t--- a/t/helper/test-online-cpus.c\n> \t+++ b/t/helper/test-online-cpus.c\n> \t@@ -1,9 +1,17 @@\n> \t #include \"test-tool.h\"\n> \t #include \"git-compat-util.h\"\n> \t #include \"thread-utils.h\"\n> *\t+#include \"strbuf.h\"\n>\n> \t int cmd__online_cpus(int argc, const char **argv)\n> \t {\n> \t-\tprintf(\"%d\\n\", online_cpus());\n> \t+\tstruct strbuf sb = STRBUF_INIT;\n> \t+\tconst char *const str = \"Hello, World\";\n> \t+\n> \t+\tfor (size_t i = 0; i < 10000000; i++) {\n> \t+\t\tstrbuf_reset(&sb);\n> \t+\t\tstrbuf_addf(&sb, \"%s/\", str);\n> \t+\t\tputs(sb.buf);\n> \t+\t}\n> \t \treturn 0;\n> \t }\n> \t9f74eff5623 addf + nolen optimize\n> \tdiff --git a/strbuf.c b/strbuf.c\n> \tindex c383f41a3c5..750e5e6a5b4 100644\n> \t--- a/strbuf.c\n> \t+++ b/strbuf.c\n> \t@@ -332,8 +332,16 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n)\n> \t void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n> \t {\n> \t \tva_list ap;\n> \t+\n> \t \tva_start(ap, fmt);\n> \t-\tstrbuf_vaddf(sb, fmt, ap);\n> \t+\tif (*fmt == '%' && *(fmt + 1) == 's' && *(fmt + 2) && !*(fmt + 3)) {\n> \t+\t\tconst char *arg = va_arg(ap, const char *);\n> \t+\n> \t+\t\tstrbuf_addstr(sb, arg);\n> \t+\t\tstrbuf_addch(sb, *(fmt + 2));\n> \t+\t} else {\n> \t+\t\tstrbuf_vaddf(sb, fmt, ap);\n> \t+\t}\n> \t \tva_end(ap);\n> \t }\n>\n> \tca60bb9b479 addf + len\n> \tdiff --git a/t/helper/test-online-cpus.c b/t/helper/test-online-cpus.c\n> \tindex c802ec579d0..7257e622015 100644\n> \t--- a/t/helper/test-online-cpus.c\n> \t+++ b/t/helper/test-online-cpus.c\n> \t@@ -7,10 +7,11 @@ int cmd__online_cpus(int argc, const char **argv)\n> \t {\n> \t \tstruct strbuf sb = STRBUF_INIT;\n> \t \tconst char *const str = \"Hello, World\";\n> \t+\tconst size_t len = strlen(str);\n>\n> \t \tfor (size_t i = 0; i < 10000000; i++) {\n> \t \t\tstrbuf_reset(&sb);\n> \t-\t\tstrbuf_addf(&sb, \"%s/\", str);\n> \t+\t\tstrbuf_addf(&sb, \"%*s/\", (int)len, str);\n> \t \t\tputs(sb.buf);\n> \t \t}\n> \t \treturn 0;\n> \t1f47987d095 addf + len optimize\n> \tdiff --git a/strbuf.c b/strbuf.c\n> \tindex 750e5e6a5b4..88801268f7a 100644\n> \t--- a/strbuf.c\n> \t+++ b/strbuf.c\n> \t@@ -334,11 +334,16 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n> \t \tva_list ap;\n>\n> \t \tva_start(ap, fmt);\n> \t-\tif (*fmt == '%' && *(fmt + 1) == 's' && *(fmt + 2) && !*(fmt + 3)) {\n> \t+\tif (*fmt == '%' &&\n> \t+\t    *(fmt + 1) == '*' &&\n> \t+\t    *(fmt + 2) == 's' &&\n> \t+\t    *(fmt + 3) &&\n> \t+\t    !*(fmt + 4)) {\n> \t+\t\tint len = va_arg(ap, int);\n> \t \t\tconst char *arg = va_arg(ap, const char *);\n>\n> \t-\t\tstrbuf_addstr(sb, arg);\n> \t-\t\tstrbuf_addch(sb, *(fmt + 2));\n> \t+\t\tstrbuf_add(sb, arg, len);\n> \t+\t\tstrbuf_addch(sb, *(fmt + 3));\n> \t \t} else {\n> \t \t\tstrbuf_vaddf(sb, fmt, ap);\n> \t \t}\n> \t55c698c0b95 addstr\n> \tdiff --git a/t/helper/test-online-cpus.c b/t/helper/test-online-cpus.c\n> \tindex 7257e622015..2716b44ca15 100644\n> \t--- a/t/helper/test-online-cpus.c\n> \t+++ b/t/helper/test-online-cpus.c\n> \t@@ -7,11 +7,11 @@ int cmd__online_cpus(int argc, const char **argv)\n> \t {\n> \t \tstruct strbuf sb = STRBUF_INIT;\n> \t \tconst char *const str = \"Hello, World\";\n> \t-\tconst size_t len = strlen(str);\n>\n> \t \tfor (size_t i = 0; i < 10000000; i++) {\n> \t \t\tstrbuf_reset(&sb);\n> \t-\t\tstrbuf_addf(&sb, \"%*s/\", (int)len, str);\n> \t+\t\tstrbuf_addstr(&sb, str);\n> \t+\t\tstrbuf_addch(&sb, '/');\n> \t \t\tputs(sb.buf);\n> \t \t}\n> \t \treturn 0;\n> \tb17fb99bf7e (HEAD -> master) add\n> \tdiff --git a/t/helper/test-online-cpus.c b/t/helper/test-online-cpus.c\n> \tindex 2716b44ca15..5e52b622c4d 100644\n> \t--- a/t/helper/test-online-cpus.c\n> \t+++ b/t/helper/test-online-cpus.c\n> \t@@ -7,10 +7,11 @@ int cmd__online_cpus(int argc, const char **argv)\n> \t {\n> \t \tstruct strbuf sb = STRBUF_INIT;\n> \t \tconst char *const str = \"Hello, World\";\n> \t+\tconst size_t len = strlen(str);\n>\n> \t \tfor (size_t i = 0; i < 10000000; i++) {\n> \t \t\tstrbuf_reset(&sb);\n> \t-\t\tstrbuf_addstr(&sb, str);\n> \t+\t\tstrbuf_add(&sb, str, len);\n> \t \t\tstrbuf_addch(&sb, '/');\n> \t \t\tputs(sb.buf);\n> \t \t}\n"},{"id":"472007","messageId":"7c8b6420-fe69-a190-9197-0ddbb3e4a964@github.com","threadId":"59195","inReplyTo":"a37623c8-d2fb-aec6-3423-2d402d717959@web.de","subject":"Re: [PATCH v2] cache-tree: fix strbuf growth in prime_cache_tree_rec()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-02-13T13:37:36Z","receivedAt":"2023-02-13T13:37:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/10/2023 3:20 PM, René Scharfe wrote:\n> Use size_t to store the original length of the strbuf tree_len, as\n> that's the correct type.\n> \n> Don't double the allocated size of the strbuf when adding a subdirectory\n> name.  And the chance of the trailing slash fitting in the slack left by\n> strbuf_add() is very high, so stop pre-growing the strbuf at all.\n\n> -\tint base_path_len = tree_path->len;\n> +\tsize_t base_path_len = tree_path->len;\n\n>  \t\t\t\tstrbuf_setlen(tree_path, base_path_len);\n> -\t\t\t\tstrbuf_grow(tree_path, base_path_len + entry.pathlen + 1);\n>  \t\t\t\tstrbuf_add(tree_path, entry.path, entry.pathlen);\n\nExcellent. LGTM.\n\n-Stolee\n"}]}