Re: [PATCH v2] merge-recursive.c: use string_list_sort instead of qsort

[Duy Nguyen <pclouds@gmail.com>]

On Fri, Nov 25, 2016 at 3:52 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Nov 24, 2016 at 06:45:36PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> This started out to as a hunt for remaining qsort() calls after rs/qsort
>> series because qsort() API is a bit easy to get wrong (*). However,
>> since we have string_list_sort(), it's conceptually a better way to sort
>> here.
>>
>> (*) In this particular case, it's even more confusing when you sort one
>> variable but you use the number of items and item size from an unrelated
>> variable (from a first glance)
>
> Makes sense, though I think I probably would have explained it in
> reverse order:
>
>   Merge-recursive sorts a string list using a raw qsort(), where it
>   feeds the "items" from one struct but the "nr" and size fields from
>   another struct. This isn't a bug because one list is a copy of the
>   other, but it's unnecessarily confusing (and also caused our recent
>   QSORT() cleanups via coccinelle to miss this call site).
>
>   Let's use string_list_sort() instead, which is more concise and harder
>   to get wrong. Note that we need to adjust our comparison function,
>   which gets fed only the strings now, not the string_list_items. That's
>   OK because we don't use the "util" field as part of our sort.
>
> Feel free to use or ignore my description as you see fit. :)

I delegate the decision to Junio. He can amend the commit if he
decides so. I suspect it's a good idea to do so.

>> -static int string_list_df_name_compare(const void *a, const void *b)
>> +static int string_list_df_name_compare(const char *one, const char *two)
>>  {
>> -     const struct string_list_item *one = a;
>> -     const struct string_list_item *two = b;
>> -     int onelen = strlen(one->string);
>> -     int twolen = strlen(two->string);
>> +     int onelen = strlen(one);
>> +     int twolen = strlen(two);
>
> I guess I haven't used string_list_sort() in a while, but I was
> surprised to find that it just feeds the strings to the comparator. That
> makes sense for using a raw strcmp() as the comparator, but I wonder if
> any callers would ever want to take the util field into account (e.g.,
> to break ties).
>
> We don't seem to care here, though (which can be verified by reading the
> code, but also because any mention of one->util would be a compilation
> error after your patch). So I guess we can punt on it until the day that
> some caller does need it.

Some callers do need it, or at least fmt-merge-msg.c:add_people_info()
does, maybe builtin/remote.c:show() and shortlog.c:shortlog_output()
too. But I'll stop here and get back to my worktree stuff.
-- 
Duy