Re: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys

From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: clime <clime7@gmail.com>, Git List <git@vger.kernel.org>
Subject: Re: [PATCH 1/2] ref-filter: apply --ignore-case to all sorting keys
Date: Mon, 4 May 2020 18:13:36 -0600	[thread overview]
Message-ID: <20200505001336.GK45250@syl.local> (raw)
In-Reply-To: <20200503091157.GA170902@coredump.intra.peff.net>

On Sun, May 03, 2020 at 05:11:57AM -0400, Jeff King wrote:
> All of the ref-filter users (for-each-ref, branch, and tag) take an
> --ignore-case option which makes filtering and sorting case-insensitive.
> However, this option was applied only to the first element of the
> ref_sorting list. So:
>
>   git for-each-ref --ignore-case --sort=refname
>
> would do what you expect, but:
>
>   git for-each-ref --ignore-case --sort=refname --sort=taggername
>
> would sort the primary key (taggername) case-insensitively, but sort the
> refname case-sensitively. We have two options here:
>
>   - teach callers to set ignore_case on the whole list
>
>   - replace the ref_sorting list with a struct that contains both the
>     list of sorting keys, as well as options that apply to _all_
>     keys
>
> I went with the first one here, as it gives more flexibility if we later
> want to let the users set the flag per-key (presumably through some
> special syntax when defining the key; for now it's all or nothing
> through --ignore-case).

Makes sense, I think that this will provide us more flexibility in the
future in case we want to have per-flag keys or some such.

> The new test covers this by sorting on both tagger and subject
> case-insensitively, which should compare "a" and "A" identically, but
> still sort them before "b" and "B". We'll break ties by sorting on the
> refname to give ourselves a stable output (this is actually supposed to
> be done automatically, but there's another bug which will be fixed in
> the next commit).

Thanks for adding a test.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/branch.c        |  2 +-
>  builtin/for-each-ref.c  |  2 +-
>  builtin/tag.c           |  2 +-
>  ref-filter.c            |  6 ++++++
>  ref-filter.h            |  2 ++
>  t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index d8297f80ff..86341cc835 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -739,7 +739,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		 */
>  		if (!sorting)
>  			sorting = ref_default_sorting();
> -		sorting->ignore_case = icase;
> +		ref_sorting_icase_all(sorting, icase);
>  		print_ref_list(&filter, sorting, &format);
>  		print_columns(&output, colopts, NULL);
>  		string_list_clear(&output, 0);
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 465153e853..57489e4eab 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -70,7 +70,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>
>  	if (!sorting)
>  		sorting = ref_default_sorting();
> -	sorting->ignore_case = icase;
> +	ref_sorting_icase_all(sorting, icase);
>  	filter.ignore_case = icase;
>
>  	filter.name_patterns = argv;
> diff --git a/builtin/tag.c b/builtin/tag.c
> index dd160b49c7..ff7610b5c8 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -485,7 +485,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	}
>  	if (!sorting)
>  		sorting = ref_default_sorting();
> -	sorting->ignore_case = icase;
> +	ref_sorting_icase_all(sorting, icase);
>  	filter.ignore_case = icase;
>  	if (cmdmode == 'l') {
>  		int ret;
> diff --git a/ref-filter.c b/ref-filter.c
> index 35776838f4..bdb3535ce5 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2317,6 +2317,12 @@ static int compare_refs(const void *a_, const void *b_, void *ref_sorting)
>  	return 0;
>  }
>
> +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag)
> +{
> +	for (; sorting; sorting = sorting->next)
> +		sorting->ignore_case = !!flag;
> +}
> +
>  void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>  {
>  	QSORT_S(array->items, array->nr, compare_refs, sorting);
> diff --git a/ref-filter.h b/ref-filter.h
> index 64330e9601..8ecc33cdfa 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -114,6 +114,8 @@ void ref_array_clear(struct ref_array *array);
>  int verify_ref_format(struct ref_format *format);
>  /*  Sort the given ref_array as per the ref_sorting provided */
>  void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
> +/*  Set the ignore_case flag for all elements of a sorting list */
> +void ref_sorting_icase_all(struct ref_sorting *sorting, int flag);
>  /*  Based on the given format and quote_style, fill the strbuf */
>  int format_ref_array_item(struct ref_array_item *info,
>  			  const struct ref_format *format,
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index b3c1092338..c9caf26327 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -895,4 +895,44 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '
> +	# name refs numerically to avoid case-insensitive filesystem conflicts
> +	nr=0 &&
> +	for email in a A b B
> +	do
> +		for subject in a A b B
> +		do
> +			GIT_COMMITTER_EMAIL="$email@example.com" \
> +			git tag -m "tag $subject" icase-$(printf %02d $nr) &&
> +			nr=$((nr+1))||
> +			return 1
> +		done
> +	done &&
> +	git for-each-ref --ignore-case \
> +		--format="%(taggeremail) %(subject) %(refname)" \
> +		--sort=refname \
> +		--sort=subject \
> +		--sort=taggeremail \
> +		refs/tags/icase-* >actual &&
> +	cat >expect <<-\EOF &&
> +	<a@example.com> tag a refs/tags/icase-00
> +	<a@example.com> tag A refs/tags/icase-01
> +	<A@example.com> tag a refs/tags/icase-04
> +	<A@example.com> tag A refs/tags/icase-05
> +	<a@example.com> tag b refs/tags/icase-02
> +	<a@example.com> tag B refs/tags/icase-03
> +	<A@example.com> tag b refs/tags/icase-06
> +	<A@example.com> tag B refs/tags/icase-07
> +	<b@example.com> tag a refs/tags/icase-08
> +	<b@example.com> tag A refs/tags/icase-09
> +	<B@example.com> tag a refs/tags/icase-12
> +	<B@example.com> tag A refs/tags/icase-13
> +	<b@example.com> tag b refs/tags/icase-10
> +	<b@example.com> tag B refs/tags/icase-11
> +	<B@example.com> tag b refs/tags/icase-14
> +	<B@example.com> tag B refs/tags/icase-15
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.26.2.957.g6dc93e954a

All looks very reasonable, so:

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor