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

From: Jeff King <peff@peff.net>
Date: Nov 24, 2016, 20:52 UTC
Message-ID: <20161124205242.ts4qhvfz2hvxez36@sigill.intra.peff.net>
In-Reply-To: <20161124114536.22249-1-pclouds@gmail.com>

On Thu, Nov 24, 2016 at 06:45:36PM +0700, Nguyễn Thái Ngọc Duy wrote:

Show 8 quoted lines

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

Show 9 quoted lines

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

-Peff