On Fri, Nov 25, 2016 at 3:52 AM, Jeff King <peff@xxxxxxxx> 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