{"thread":{"id":"65193","subject":"[PATCH] builtin/help.c: move strbuf out of help loops","startedAt":"2026-03-10T07:03:37Z","lastAt":"2026-03-11T19:48:36Z","messageCount":7,"participants":["Siddharth Shrimali","Patrick Steinhardt","Junio C Hamano","Amisha Chhajed"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"538363","messageId":"20260310070328.29836-1-r.siddharth.shrimali@gmail.com","threadId":"65193","inReplyTo":null,"subject":"[PATCH] builtin/help.c: move strbuf out of help loops","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-03-10T07:03:28Z","receivedAt":"2026-03-10T07:03:37Z","isPatch":true,"sender":{"key":"r.siddharth.shrimali@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183274193?v=4"},"body":"In list_config_help(), a strbuf was being initialized and released\ninside two separate loops. This caused unnecessary memory allocation\nand deallocation on every iteration.\n\nMove the strbuf declaration to the top of the function and use\nstrbuf_reset() inside the loops to reuse the same buffer. Similarly\nrelease() the buffer at the end of the function to free the memory.\nThis improves performance by avoiding repeated heap pressure by reducing\nthe number of allocations.\n\nThis also fixes a minor memory leak when the SHOW_CONFIG_HUMAN case\ntriggers a continue.\n\nSigned-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>\n---\n builtin/help.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 86a3d03a9b..07398b430e 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -134,10 +134,10 @@ static void list_config_help(enum show_config_type type)\n \tstruct string_list keys = STRING_LIST_INIT_DUP;\n \tstruct string_list keys_uniq = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *item;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n \tfor (p = config_name_list; *p; p++) {\n \t\tconst char *var = *p;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tfor (e = slot_expansions; e->prefix; e++) {\n \n@@ -149,7 +149,6 @@ static void list_config_help(enum show_config_type type)\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n-\t\tstrbuf_release(&sb);\n \t\tif (!e->prefix)\n \t\t\tstring_list_append(&keys, var);\n \t}\n@@ -161,10 +160,10 @@ static void list_config_help(enum show_config_type type)\n \n \tstring_list_sort(&keys);\n \tfor (size_t i = 0; i < keys.nr; i++) {\n+\t\tstrbuf_reset(&sb);\n \t\tconst char *var = keys.items[i].string;\n \t\tconst char *wildcard, *tag, *cut;\n \t\tconst char *dot = NULL;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tswitch (type) {\n \t\tcase SHOW_CONFIG_HUMAN:\n@@ -195,13 +194,13 @@ static void list_config_help(enum show_config_type type)\n \n \t\tstrbuf_add(&sb, var, cut - var);\n \t\tstring_list_append(&keys_uniq, sb.buf);\n-\t\tstrbuf_release(&sb);\n \n \t}\n \tstring_list_clear(&keys, 0);\n \tstring_list_remove_duplicates(&keys_uniq, 0);\n \tfor_each_string_list_item(item, &keys_uniq)\n \t\tputs(item->string);\n+\tstrbuf_release(&sb);\n \tstring_list_clear(&keys_uniq, 0);\n }\n \n-- \n2.51.2\n\n"},{"id":"538412","messageId":"abARj_VI9n2nB_xT@pks.im","threadId":"65193","inReplyTo":"20260310070328.29836-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH] builtin/help.c: move strbuf out of help loops","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T12:41:51Z","receivedAt":"2026-03-10T12:41:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 10, 2026 at 12:33:28PM +0530, Siddharth Shrimali wrote:\n> In list_config_help(), a strbuf was being initialized and released\n> inside two separate loops. This caused unnecessary memory allocation\n> and deallocation on every iteration.\n> \n> Move the strbuf declaration to the top of the function and use\n> strbuf_reset() inside the loops to reuse the same buffer. Similarly\n> release() the buffer at the end of the function to free the memory.\n> This improves performance by avoiding repeated heap pressure by reducing\n> the number of allocations.\n> \n> This also fixes a minor memory leak when the SHOW_CONFIG_HUMAN case\n> triggers a continue.\n> \n> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>\n> ---\n>  builtin/help.c | 7 +++----\n>  1 file changed, 3 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/help.c b/builtin/help.c\n> index 86a3d03a9b..07398b430e 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -134,10 +134,10 @@ static void list_config_help(enum show_config_type type)\n>  \tstruct string_list keys = STRING_LIST_INIT_DUP;\n>  \tstruct string_list keys_uniq = STRING_LIST_INIT_DUP;\n>  \tstruct string_list_item *item;\n> +\tstruct strbuf sb = STRBUF_INIT;\n>  \n>  \tfor (p = config_name_list; *p; p++) {\n>  \t\tconst char *var = *p;\n> -\t\tstruct strbuf sb = STRBUF_INIT;\n>  \n>  \t\tfor (e = slot_expansions; e->prefix; e++) {\n>  \n\nWhat's missing from the context here is that the next line already knows\nto `strbuf_reset()`. You could do a trick and drop the empty newline\nhere while at it, as that would then make the reset call visible.\n\n> @@ -149,7 +149,6 @@ static void list_config_help(enum show_config_type type)\n>  \t\t\t\tbreak;\n>  \t\t\t}\n>  \t\t}\n> -\t\tstrbuf_release(&sb);\n>  \t\tif (!e->prefix)\n>  \t\t\tstring_list_append(&keys, var);\n>  \t}\n> @@ -161,10 +160,10 @@ static void list_config_help(enum show_config_type type)\n>  \n>  \tstring_list_sort(&keys);\n>  \tfor (size_t i = 0; i < keys.nr; i++) {\n> +\t\tstrbuf_reset(&sb);\n\nOur coding style says that statements should come after variable\ndeclarations.\n\nOther than that this patch looks good to me, thanks!\n\nPatrick\n"},{"id":"538468","messageId":"20260310160029.44605-1-r.siddharth.shrimali@gmail.com","threadId":"65193","inReplyTo":"abARj_VI9n2nB_xT@pks.im","subject":"[PATCH v2] builtin/help.c: move strbuf out of help loops","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-03-10T16:00:29Z","receivedAt":"2026-03-10T16:00:44Z","isPatch":true,"sender":{"key":"r.siddharth.shrimali@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183274193?v=4"},"body":"In list_config_help(), a strbuf was being initialized and released\ninside two separate loops. This caused unnecessary memory allocation\nand deallocation on every iteration.\n\nMove the strbuf declaration to the top of the function and use\nstrbuf_reset() inside the loops to reuse the same buffer. Similarly\nrelease() the buffer at the end of the function to free the memory.\nThis improves performance by avoiding repeated heap pressure by reducing\nthe number of allocations.\n\nThis also fixes a minor memory leak when the SHOW_CONFIG_HUMAN case\ntriggers a continue.\n\nSuggested-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>\n---\nChanges in v2:\n- Moved strbuf_reset() after variable declarations to follow \n  coding standards.\n- Removed unnecessary empty lines to tighten the code as suggested \n  by Patrick.\n\n builtin/help.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 86a3d03a9b..467a0763a6 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -134,13 +134,11 @@ static void list_config_help(enum show_config_type type)\n \tstruct string_list keys = STRING_LIST_INIT_DUP;\n \tstruct string_list keys_uniq = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *item;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n \tfor (p = config_name_list; *p; p++) {\n \t\tconst char *var = *p;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\n \t\tfor (e = slot_expansions; e->prefix; e++) {\n-\n \t\t\tstrbuf_reset(&sb);\n \t\t\tstrbuf_addf(&sb, \"%s.%s\", e->prefix, e->placeholder);\n \t\t\tif (!strcasecmp(var, sb.buf)) {\n@@ -149,7 +147,6 @@ static void list_config_help(enum show_config_type type)\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n-\t\tstrbuf_release(&sb);\n \t\tif (!e->prefix)\n \t\t\tstring_list_append(&keys, var);\n \t}\n@@ -164,7 +161,7 @@ static void list_config_help(enum show_config_type type)\n \t\tconst char *var = keys.items[i].string;\n \t\tconst char *wildcard, *tag, *cut;\n \t\tconst char *dot = NULL;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_reset(&sb);\n \n \t\tswitch (type) {\n \t\tcase SHOW_CONFIG_HUMAN:\n@@ -195,13 +192,13 @@ static void list_config_help(enum show_config_type type)\n \n \t\tstrbuf_add(&sb, var, cut - var);\n \t\tstring_list_append(&keys_uniq, sb.buf);\n-\t\tstrbuf_release(&sb);\n \n \t}\n \tstring_list_clear(&keys, 0);\n \tstring_list_remove_duplicates(&keys_uniq, 0);\n \tfor_each_string_list_item(item, &keys_uniq)\n \t\tputs(item->string);\n+\tstrbuf_release(&sb);\n \tstring_list_clear(&keys_uniq, 0);\n }\n \n-- \n2.51.2\n\n"},{"id":"538520","messageId":"xmqq1phrtoen.fsf@gitster.g","threadId":"65193","inReplyTo":"20260310160029.44605-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH v2] builtin/help.c: move strbuf out of help loops","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-10T20:33:52Z","receivedAt":"2026-03-10T20:33:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:\n\n> In list_config_help(), a strbuf was being initialized and released\n> inside two separate loops. This caused unnecessary memory allocation\n> and deallocation on every iteration.\n\nOK.  strbuf_init() followed by a loop that does strbuf_reset()\nfollowed by use of strbuf, concluded with strbuf_release() after\nleaving the loop, is a fairly common pattern to optimize such a use\npattern.\n\n> This also fixes a minor memory leak when the SHOW_CONFIG_HUMAN case\n> triggers a continue.\n\nDoes it?  You are essentially saying that\n\n\tfor (int i = 0; i < 10; i++) {\n\t\tstruct strbuf sb = STRBUF_INIT;\n \n\t\tswitch (SHOW_CONFIG_HUMAN) {\n                case SHOW_CONFIG_HUMAN:\n\t\t\tcontinue;\n\t\t}\n\t}\n\nleaks, but a strbuf merely initialized can safely be discarded\nwithout leaking any resources, can't it?\n\n> diff --git a/builtin/help.c b/builtin/help.c\n> index 86a3d03a9b..467a0763a6 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -134,13 +134,11 @@ static void list_config_help(enum show_config_type type)\n>  \tstruct string_list keys = STRING_LIST_INIT_DUP;\n>  \tstruct string_list keys_uniq = STRING_LIST_INIT_DUP;\n>  \tstruct string_list_item *item;\n> +\tstruct strbuf sb = STRBUF_INIT;\n>  \n>  \tfor (p = config_name_list; *p; p++) {\n>  \t\tconst char *var = *p;\n> -\t\tstruct strbuf sb = STRBUF_INIT;\n> -\n\nA blank between the variable declaration block and the first\nstatement makes the code easier to follow.  Loss of a blank line\nhere is of dubious value.\n\n>  \t\tfor (e = slot_expansions; e->prefix; e++) {\n> -\n\nThis removal is good.\n\n>  \t\t\tstrbuf_reset(&sb);\n\nSo we first reset, and then start building things in sb.\n\n>  \t\t\tstrbuf_addf(&sb, \"%s.%s\", e->prefix, e->placeholder);\n>  \t\t\tif (!strcasecmp(var, sb.buf)) {\n> @@ -149,7 +147,6 @@ static void list_config_help(enum show_config_type type)\n>  \t\t\t\tbreak;\n>  \t\t\t}\n>  \t\t}\n> -\t\tstrbuf_release(&sb);\n>  \t\tif (!e->prefix)\n>  \t\t\tstring_list_append(&keys, var);\n\nWe no longer have to release it inside the loop, as we will reset at\nthe beginning of the next iteration.\n\n>  \t}\n> @@ -164,7 +161,7 @@ static void list_config_help(enum show_config_type type)\n>  \t\tconst char *var = keys.items[i].string;\n>  \t\tconst char *wildcard, *tag, *cut;\n>  \t\tconst char *dot = NULL;\n> -\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tstrbuf_reset(&sb);\n>  \n\nThe arrangement of the blank line is wrong here.  The original was\nthe last line of a declaration block which should come before the\nblank.  Now you are removing it, and then adding a strbuf_reset() as\nthe first statement, which should come after the blank that delimits\nthe declarations and statements.\n\nIn any case, here we have a second loop that wants to use a scratch\nstrbuf, so again we reset it at the beginning of the loop before we\nuse it.\n\n>  \t\tswitch (type) {\n>  \t\tcase SHOW_CONFIG_HUMAN:\n> @@ -195,13 +192,13 @@ static void list_config_help(enum show_config_type type)\n>  \n>  \t\tstrbuf_add(&sb, var, cut - var);\n>  \t\tstring_list_append(&keys_uniq, sb.buf);\n> -\t\tstrbuf_release(&sb);\n>  \n\nAnd we no longer release it during iteration.  Instead ...\n\n\n>  \t}\n>  \tstring_list_clear(&keys, 0);\n>  \tstring_list_remove_duplicates(&keys_uniq, 0);\n>  \tfor_each_string_list_item(item, &keys_uniq)\n>  \t\tputs(item->string);\n> +\tstrbuf_release(&sb);\n\n... we release after we leave the loop.\n\n>  \tstring_list_clear(&keys_uniq, 0);\n>  }\n\n\nHaving looked at this patch, I recall somebody else is revamping\nthis function already, so this patch would step on their toes.\nPlease pay attention to what is going on in the project around the\ncode you are touching, and coordinate with others who are working on\nthe same code if necessary.\n\nhttps://lore.kernel.org/git/20260228104654.80831-2-amishhhaaaa@gmail.com/\n\nThanks.\n"},{"id":"538649","messageId":"CAGWgyh_dJX7TteKjwVXUwnmUL5kmZifpA0a4n1RiwRvCBEY5gw@mail.gmail.com","threadId":"65193","inReplyTo":"xmqq1phrtoen.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/help.c: move strbuf out of help loops","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-03-11T18:13:07Z","receivedAt":"2026-03-11T18:13:45Z","isPatch":true,"sender":{"key":"r.siddharth.shrimali@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183274193?v=4"},"body":"On Wed, 11 Mar 2026 at 02:03, Junio C Hamano <gitster@pobox.com> wrote:\n> Having looked at this patch, I recall somebody else is revamping\n> this function already, so this patch would step on their toes.\n> Please pay attention to what is going on in the project around the\n> code you are touching, and coordinate with others who are working on\n> the same code if necessary.\n>\n> https://lore.kernel.org/git/20260228104654.80831-2-amishhhaaaa@gmail.com/\n>\n> Thanks.\n\nHi Junio (CC'ing Amisha),\n\nAfter looking at the refactor of list_config_help() in the\nother active thread, I agree that my optimization is no longer\nnecessary.\n\nAmisha's new structure with set_config_vars() and set_config_sections()\nis much cleaner. Since the logic is now encapsulated in these helpers,\nmy proposed changes would not be applicable.\n\nI'll drop this patch and focus on my other contributions.\nThanks for the guidance!\n\nRegards,\nSiddharth\n"},{"id":"538666","messageId":"CAPvEtrd9Yri5LQu9DiMAO4EDquyd-JxwNBGn+h=+=E+oKJ2ERw@mail.gmail.com","threadId":"65193","inReplyTo":"CAGWgyh_dJX7TteKjwVXUwnmUL5kmZifpA0a4n1RiwRvCBEY5gw@mail.gmail.com","subject":"Re: [PATCH v2] builtin/help.c: move strbuf out of help loops","fromName":"Amisha Chhajed","fromEmail":"amishhhaaaa@gmail.com","sentAt":"2026-03-11T19:30:51Z","receivedAt":"2026-03-11T19:31:04Z","isPatch":true,"sender":{"key":"amishhhaaaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/136238836?v=4"},"body":"> After looking at the refactor of list_config_help() in the\n> other active thread, I agree that my optimization is no longer\n> necessary.\n>\n> Amisha's new structure with set_config_vars() and set_config_sections()\n> is much cleaner. Since the logic is now encapsulated in these helpers,\n> my proposed changes would not be applicable.\n\nI feel removing out the strbuf initialisation and release out of the\nloop is still applicable,\nI have added all the parts in v5 which were not fixed by my\nimprovements and tagged you,\ncheck it out here\nhttps://lore.kernel.org/git/20260311192151.60489-1-amishhhaaaa@gmail.com/\n\n-- \nThanks,\nAmisha\n"},{"id":"538672","messageId":"xmqq3426no4u.fsf@gitster.g","threadId":"65193","inReplyTo":"CAPvEtrd9Yri5LQu9DiMAO4EDquyd-JxwNBGn+h=+=E+oKJ2ERw@mail.gmail.com","subject":"Re: [PATCH v2] builtin/help.c: move strbuf out of help loops","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-11T19:48:33Z","receivedAt":"2026-03-11T19:48:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Amisha Chhajed <amishhhaaaa@gmail.com> writes:\n\n>> After looking at the refactor of list_config_help() in the\n>> other active thread, I agree that my optimization is no longer\n>> necessary.\n>>\n>> Amisha's new structure with set_config_vars() and set_config_sections()\n>> is much cleaner. Since the logic is now encapsulated in these helpers,\n>> my proposed changes would not be applicable.\n>\n> I feel removing out the strbuf initialisation and release out of the\n> loop is still applicable,\n> I have added all the parts in v5 which were not fixed by my\n> improvements and tagged you,\n> check it out here\n> https://lore.kernel.org/git/20260311192151.60489-1-amishhhaaaa@gmail.com/\n\nI love seeing two community members, both of which are relatively\nnewcomers, working well together ;-).\n"}]}