merge-recursive.c: use QSORT macro

This is the follow up of rs/qsort series, merged in b8688ad (Merge branch 'rs/qsort' - 2016-10-10), where coccinelle was used to do automatic transformation.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
coccinelle missed this place, understandably, because it can't know that

      sizeof(*entries->items)

is the same as

sizeof(*df_name_compare.items)

without some semantic analysis.

Quoted 9 lines · Nguyễn Thái Ngọc Duy

This is the follow up of rs/qsort series, merged in b8688ad (Merge branch 'rs/qsort' - 2016-10-10), where coccinelle was used to do automatic transformation.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
coccinelle missed this place, understandably, because it can't know that

Quoted 7 lines

      sizeof(*entries->items)

is the same as

sizeof(*df_name_compare.items)

without some semantic analysis.

That made me wonder why "entries" is used at all. Does it point to the same struct? But no, df_name_compare is a string list we create with the same list of strings.

Which is why...

merge-recursive.c static void record_df_conflict_files(struct merge_options *o, · line 455

Quoted 3 lines

merge-recursive.c +1 -1

-     qsort(df_sorted_entries.items, entries->nr, sizeof(*entries->items),
+     QSORT(df_sorted_entries.items, entries->nr,
            string_list_df_name_compare);

...it's OK to use entries->nr here, and not df_sorted_entries.nr. It still seems a bit odd, though.

Maybe it's worth making this:

QSORT(df_sorted_entries.items, df_sorted_entries.nr,

        string_list_df_name_compare);

while we're at it. Another possibility is:

  df_sorted_entries.cmp = string_list_df_name_compare;
  string_list_sort(&df_sorted_entries);

It's not any shorter, but maybe it's conceptually simpler.

Another possibility is:

  df_sorted_entries.cmp = string_list_df_name_compare;
  string_list_sort(&df_sorted_entries);

It's not any shorter, but maybe it's conceptually simpler.

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)

Quoted 8 lines · Nguyễn Thái Ngọc Duy

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

merge-recursive.c static struct string_list *get_unmerged(void) · line 394

Quoted 9 lines

merge-recursive.c +3 -5

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

Quoted 6 lines

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

Quoted 4 lines

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.