Re: [PATCH 2/2] help: ensure &keys_uniq follows sort -u

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 12, 2026, 19:58 UTC
Message-ID: <xmqqh5rlohsm.fsf@gitster.g>
In-Reply-To: <20260212041017.91370-3-amishhhaaaa@gmail.com>

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

Show 8 quoted lines

> From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
>
> uniqueness operation of &keys_uniq depends on the sort operation executed
> for &keys this might introduce regressions in future when the logic of
> forming &keys_uniq from &keys is changed.
>
> add string_list_sort_u operation for &keys_uniq after the processing of
> &keys so it follows the expected sort -u behaviour.

I am not sure the above reasoning is sound. With the original code, we

 - prepare empty keys_uniq
 - collect keys
 - sort keys
 - iterate over keys
   - add either the whole "section[.subsection].key" or "section" to keys_uniq

before we call remove_duplicates. keys_uniq would have duplicates, but because keys is sorted upfront, wouldn't the contents of keys_uniq be collected in sorted order anyway?

This is not a performance critical part of the system, so it is OK as a future-proof measure to sort keys_uniq immediately before we start doing something that we _care_ about its sortedness (e.g., presenting the final output to the user), even if keys_uniq is known to be already sorted with the current code. Using sort_u here would allow us not to worry about how keys_uniq is constructed in that ugly loop.

Yes, this function, especially the loop before the part you are touching, _is_ ugly. What drug the authors of it were under when it was written, I have to wonder X-<. For example, wouldn't readers wonder why CONFIG_HUMAN output mode does puts() right in the middle of the loop over keys string list, while the other two does not puts() and have a separate loop over keys_uniq instead?

I suspect that making a switch(type) that calls one of three helper functions for the three different output types after keys has been populated in the earlier part of this function, but immediately before it is sorted with string_list_sort(&keys), would be a low-hanging fruit clean-up that makes the result far easier to follow than the current code. The helper function to handle CONFIG_HUMAN mode may need to sort keys, but other two helper functions do not have to and iterate over unsorted keys to construct their output list, on which they can do sort_u before they output.

Thanks.

Show 18 quoted lines

> Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
> ---
>  builtin/help.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/help.c b/builtin/help.c
> index c09cbc8912..0c9c007214 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -196,7 +196,7 @@ static void list_config_help(enum show_config_type type)
>  
>  	}
>  	string_list_clear(&keys, 0);
> -	string_list_remove_duplicates(&keys_uniq, 0);
> +	string_list_sort_u(&keys_uniq, 0);
>  	for_each_string_list_item(item, &keys_uniq)
>  		puts(item->string);
>  	string_list_clear(&keys_uniq, 0);