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

[Junio C Hamano <gitster@pobox.com>]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

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

> 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);