Re: [PATCH v2 2/2] string-list: add string_list_sort_u() that mimics "sort -u"

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

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> Many callsites of string_list_remove_duplicates() call it
> immdediately after calling string_list_sort(), understandably
> as the former requires string-list to be sorted, it is clear
> that these places are sorting only to remove duplicates and
> for no other reason.
>
> Introduce a helper function string_list_sort_u that combines
> these two calls that often appear together, to simplify
> these callsites. Replace the current calls of those methods with
> string_list_sort_u().

After this, only two callers of string_list_remove_duplicates()
remain in the codebase.

The one in builtin/fetch.c::cmd_fetch() smells somewhat fishy.  It
prepares a string_list "list", populates it with for_each_remote()
by appending remotes found in the configuration when asked to do
"--all", or append named ones with "--multiple", and then calls
"remove duplicates" without sorting the resulting list first.

 - A test should be able to demonstrate that the call to
   string_list_remove_duplicates() is not operating on a sorted
   string list.

 - Once a breakage is demonstrated, we need to devise a fix.
   Sorting the string list before removing would certainly fix the
   duplicates removal, but it will change the order in which the
   remotes are consulted.  I think it is currently "whatever order
   these remotes appear in your configuration file(s)", but that
   does not mean it is a random order.  It is very likely that they
   are in the order the user has learned to expect the remotes are
   to be consulted, so "sort and then dedup" might appear as a
   regression in behaviour.  I dunno.

The one in builtin/help.c::list_config_help() is somewhat fishy as
well.  I didn't read it too carefully, but it walks over keys which
is in sorted string_list, and sometimes pushes the key intact to
keys_uniq, and some other times munges the key and pushes the result
to keys_uniq.  I do not know if presence of these these munged keys
in the keys_uniq string list breaks the sortedness of keys_uniq.
If keys_uniq is *not* sorted, then running "remove duplicates" would
be broken, of course.  Again, a test should be able to demonstrate
if this is the case, and we should fix it as well if it is broken.

Thanks.