From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 10 Mar 2026 12:41:51 GMT
Subject: Re: [PATCH] builtin/help.c: move strbuf out of help loops
Message-ID: <abARj_VI9n2nB_xT@pks.im>
In-Reply-To: <20260310070328.29836-1-r.siddharth.shrimali@gmail.com>

On Tue, Mar 10, 2026 at 12:33:28PM +0530, Siddharth Shrimali wrote:
> In list_config_help(), a strbuf was being initialized and released
> inside two separate loops. This caused unnecessary memory allocation
> and deallocation on every iteration.
> 
> Move the strbuf declaration to the top of the function and use
> strbuf_reset() inside the loops to reuse the same buffer. Similarly
> release() the buffer at the end of the function to free the memory.
> This improves performance by avoiding repeated heap pressure by reducing
> the number of allocations.
> 
> This also fixes a minor memory leak when the SHOW_CONFIG_HUMAN case
> triggers a continue.
> 
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
>  builtin/help.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/help.c b/builtin/help.c
> index 86a3d03a9b..07398b430e 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -134,10 +134,10 @@ static void list_config_help(enum show_config_type type)
>  	struct string_list keys = STRING_LIST_INIT_DUP;
>  	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
>  	struct string_list_item *item;
> +	struct strbuf sb = STRBUF_INIT;
>  
>  	for (p = config_name_list; *p; p++) {
>  		const char *var = *p;
> -		struct strbuf sb = STRBUF_INIT;
>  
>  		for (e = slot_expansions; e->prefix; e++) {
>  

What's missing from the context here is that the next line already knows
to `strbuf_reset()`. You could do a trick and drop the empty newline
here while at it, as that would then make the reset call visible.

> @@ -149,7 +149,6 @@ static void list_config_help(enum show_config_type type)
>  				break;
>  			}
>  		}
> -		strbuf_release(&sb);
>  		if (!e->prefix)
>  			string_list_append(&keys, var);
>  	}
> @@ -161,10 +160,10 @@ static void list_config_help(enum show_config_type type)
>  
>  	string_list_sort(&keys);
>  	for (size_t i = 0; i < keys.nr; i++) {
> +		strbuf_reset(&sb);

Our coding style says that statements should come after variable
declarations.

Other than that this patch looks good to me, thanks!

Patrick