Re: [PATCH 1/9] ref-filter.c: really don't sort when using --no-sort

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 13178 bytes --]

On Tue, Nov 07, 2023 at 01:25:53AM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
> 
> Update 'ref_sorting_options()' to return a NULL 'struct ref_sorting *' if
> the string list provided to it is empty, rather than returning the default
> refname sort structure. Also update 'ref_array_sort()' to explicitly skip
> sorting if its 'struct ref_sorting *' arg is NULL. Other functions using
> 'struct ref_sorting *' do not need any changes because they already properly
> ignore NULL values.
> 
> The goal of this change is to have the '--no-sort' option truly disable
> sorting in commands like 'for-each-ref, 'tag', and 'branch'. Right now,
> '--no-sort' will still trigger refname sorting by default in 'for-each-ref',
> 'tag', and 'branch'.
> 
> To match existing behavior as closely as possible, explicitly add "refname"
> to the list of sort keys in 'for-each-ref', 'tag', and 'branch' before
> parsing options (if no config-based sort keys are set). This ensures that
> sorting will only be fully disabled if '--no-sort' is provided as an option;
> otherwise, "refname" sorting will remain the default. Note: this also means
> that even when sort keys are provided on the command line, "refname" will be
> the final sort key in the sorting structure. This doesn't actually change
> any behavior, since 'compare_refs()' already falls back on comparing
> refnames if two refs are equal w.r.t all other sort keys.
> 
> Finally, remove the condition around sorting in 'ls-remote', since it's no
> longer necessary. Unlike 'for-each-ref' et. al., it does *not* set any sort
> keys by default. The default empty list of sort keys will produce a NULL
> 'struct ref_sorting *', which causes the sorting to be skipped in
> 'ref_array_sort()'.

I found the order in this commit message a bit funny because you first
explain what you're doing, then explain the goal, and then jump into the
changes again. The message might be a bit easier to read if the goal was
stated up front.

I was also briefly wondering whether it would make sense to split up
this commit, as you're doing two different things:

    - Refactor how git-for-each-ref(1), git-tag(1) and git-branch(1) set
      up their default sorting.

    - Change `ref_array_sort()` to not sort when its sorting option is
      `NULL`.

If this was split up into two commits, then the result might be a bit
easier to reason about. But I don't feel strongly about this.

> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  builtin/branch.c        |  6 ++++
>  builtin/for-each-ref.c  |  3 ++
>  builtin/ls-remote.c     | 10 ++----
>  builtin/tag.c           |  6 ++++
>  ref-filter.c            | 19 ++----------
>  t/t3200-branch.sh       | 68 +++++++++++++++++++++++++++++++++++++++--
>  t/t6300-for-each-ref.sh | 21 +++++++++++++
>  t/t7004-tag.sh          | 45 +++++++++++++++++++++++++++
>  8 files changed, 152 insertions(+), 26 deletions(-)
> 
> diff --git a/builtin/branch.c b/builtin/branch.c
> index e7ee9bd0f15..d67738bbcaa 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -767,7 +767,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage_with_options(builtin_branch_usage, options);
>  
> +	/*
> +	 * Try to set sort keys from config. If config does not set any,
> +	 * fall back on default (refname) sorting.
> +	 */
>  	git_config(git_branch_config, &sorting_options);
> +	if (!sorting_options.nr)
> +		string_list_append(&sorting_options, "refname");
>  
>  	track = git_branch_track;
>  
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 350bfa6e811..93b370f550b 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -67,6 +67,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  
>  	git_config(git_default_config, NULL);
>  
> +	/* Set default (refname) sorting */
> +	string_list_append(&sorting_options, "refname");
> +
>  	parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);
>  	if (maxcount < 0) {
>  		error("invalid --count argument: `%d'", maxcount);
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index fc765754305..436249b720c 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -58,6 +58,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	struct transport *transport;
>  	const struct ref *ref;
>  	struct ref_array ref_array;
> +	struct ref_sorting *sorting;
>  	struct string_list sorting_options = STRING_LIST_INIT_DUP;
>  
>  	struct option options[] = {
> @@ -141,13 +142,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  		item->symref = xstrdup_or_null(ref->symref);
>  	}
>  
> -	if (sorting_options.nr) {
> -		struct ref_sorting *sorting;
> -
> -		sorting = ref_sorting_options(&sorting_options);
> -		ref_array_sort(sorting, &ref_array);
> -		ref_sorting_release(sorting);
> -	}
> +	sorting = ref_sorting_options(&sorting_options);
> +	ref_array_sort(sorting, &ref_array);

We stopped calling `ref_sorting_release()`. Doesn't that cause us to
leak memory?

>  	for (i = 0; i < ref_array.nr; i++) {
>  		const struct ref_array_item *ref = ref_array.items[i];
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 3918eacbb57..64f3196cd4c 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -501,7 +501,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  
>  	setup_ref_filter_porcelain_msg();
>  
> +	/*
> +	 * Try to set sort keys from config. If config does not set any,
> +	 * fall back on default (refname) sorting.
> +	 */
>  	git_config(git_tag_config, &sorting_options);
> +	if (!sorting_options.nr)
> +		string_list_append(&sorting_options, "refname");
>  
>  	memset(&opt, 0, sizeof(opt));
>  	filter.lines = -1;
> diff --git a/ref-filter.c b/ref-filter.c
> index e4d3510e28e..7250089b7c6 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3142,7 +3142,8 @@ void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting,
>  
>  void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>  {
> -	QSORT_S(array->items, array->nr, compare_refs, sorting);
> +	if (sorting)
> +		QSORT_S(array->items, array->nr, compare_refs, sorting);
>  }
>  
>  static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)
> @@ -3248,18 +3249,6 @@ static int parse_sorting_atom(const char *atom)
>  	return res;
>  }
>  
> -/*  If no sorting option is given, use refname to sort as default */
> -static struct ref_sorting *ref_default_sorting(void)
> -{
> -	static const char cstr_name[] = "refname";
> -
> -	struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));
> -
> -	sorting->next = NULL;
> -	sorting->atom = parse_sorting_atom(cstr_name);
> -	return sorting;
> -}
> -
>  static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
>  {
>  	struct ref_sorting *s;
> @@ -3283,9 +3272,7 @@ struct ref_sorting *ref_sorting_options(struct string_list *options)
>  	struct string_list_item *item;
>  	struct ref_sorting *sorting = NULL, **tail = &sorting;
>  
> -	if (!options->nr) {
> -		sorting = ref_default_sorting();
> -	} else {
> +	if (options->nr) {
>  		for_each_string_list_item(item, options)
>  			parse_ref_sorting(tail, item->string);
>  	}
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 3182abde27f..9918ba05dec 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1570,9 +1570,10 @@ test_expect_success 'tracking with unexpected .fetch refspec' '
>  
>  test_expect_success 'configured committerdate sort' '
>  	git init -b main sort &&
> +	test_config -C sort branch.sort "committerdate" &&
> +
>  	(
>  		cd sort &&
> -		git config branch.sort committerdate &&
>  		test_commit initial &&
>  		git checkout -b a &&
>  		test_commit a &&
> @@ -1592,9 +1593,10 @@ test_expect_success 'configured committerdate sort' '
>  '
>  
>  test_expect_success 'option override configured sort' '
> +	test_config -C sort branch.sort "committerdate" &&
> +
>  	(
>  		cd sort &&
> -		git config branch.sort committerdate &&
>  		git branch --sort=refname >actual &&
>  		cat >expect <<-\EOF &&
>  		  a
> @@ -1606,10 +1608,70 @@ test_expect_success 'option override configured sort' '
>  	)
>  '
>  
> +test_expect_success '--no-sort cancels config sort keys' '
> +	test_config -C sort branch.sort "-refname" &&
> +
> +	(
> +		cd sort &&
> +
> +		# objecttype is identical for all of them, so sort falls back on
> +		# default (ascending refname)
> +		git branch \
> +			--no-sort \
> +			--sort="objecttype" >actual &&

This test is a bit confusing to me. Shouldn't we in fact ignore the
configured sorting order as soon as we pass `--sort=` anyway? In other
words, I would expect the `--no-sort` option to not make a difference
here. What should make a difference is if you _only_ passed `--no-sort`.

> +		cat >expect <<-\EOF &&
> +		  a
> +		* b
> +		  c
> +		  main
> +		EOF
> +		test_cmp expect actual
> +	)
> +
> +'
> +
> +test_expect_success '--no-sort cancels command line sort keys' '
> +	(
> +		cd sort &&
> +
> +		# objecttype is identical for all of them, so sort falls back on
> +		# default (ascending refname)
> +		git branch \
> +			--sort="-refname" \
> +			--no-sort \
> +			--sort="objecttype" >actual &&
> +		cat >expect <<-\EOF &&
> +		  a
> +		* b
> +		  c
> +		  main
> +		EOF
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success '--no-sort without subsequent --sort prints expected branches' '
> +	(
> +		cd sort &&
> +
> +		# Sort the results with `sort` for a consistent comparison
> +		# against expected
> +		git branch --no-sort | sort >actual &&
> +		cat >expect <<-\EOF &&
> +		  a
> +		  c
> +		  main
> +		* b
> +		EOF
> +		test_cmp expect actual
> +	)
> +'
> +
>  test_expect_success 'invalid sort parameter in configuration' '
> +	test_config -C sort branch.sort "v:notvalid" &&
> +
>  	(
>  		cd sort &&
> -		git config branch.sort "v:notvalid" &&
>  
>  		# this works in the "listing" mode, so bad sort key
>  		# is a dying offence.
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 00a060df0b5..0613e5e3623 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -1335,6 +1335,27 @@ test_expect_success '--no-sort cancels the previous sort keys' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success '--no-sort without subsequent --sort prints expected refs' '
> +	cat >expected <<-\EOF &&
> +	refs/tags/multi-ref1-100000-user1
> +	refs/tags/multi-ref1-100000-user2
> +	refs/tags/multi-ref1-200000-user1
> +	refs/tags/multi-ref1-200000-user2
> +	refs/tags/multi-ref2-100000-user1
> +	refs/tags/multi-ref2-100000-user2
> +	refs/tags/multi-ref2-200000-user1
> +	refs/tags/multi-ref2-200000-user2
> +	EOF
> +
> +	# Sort the results with `sort` for a consistent comparison against
> +	# expected
> +	git for-each-ref \
> +		--format="%(refname)" \
> +		--no-sort \
> +		"refs/tags/multi-*" | sort >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
>  	test_when_finished "git checkout main" &&
>  	git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ >actual &&
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index e689db42929..b41a47eb943 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1862,6 +1862,51 @@ test_expect_success 'option override configured sort' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '--no-sort cancels config sort keys' '
> +	test_config tag.sort "-refname" &&
> +
> +	# objecttype is identical for all of them, so sort falls back on
> +	# default (ascending refname)
> +	git tag -l \
> +		--no-sort \
> +		--sort="objecttype" \
> +		"foo*" >actual &&
> +	cat >expect <<-\EOF &&
> +	foo1.10
> +	foo1.3
> +	foo1.6
> +	EOF
> +	test_cmp expect actual
> +'

Same question here.

Patrick

> +test_expect_success '--no-sort cancels command line sort keys' '
> +	# objecttype is identical for all of them, so sort falls back on
> +	# default (ascending refname)
> +	git tag -l \
> +		--sort="-refname" \
> +		--no-sort \
> +		--sort="objecttype" \
> +		"foo*" >actual &&
> +	cat >expect <<-\EOF &&
> +	foo1.10
> +	foo1.3
> +	foo1.6
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '--no-sort without subsequent --sort prints expected tags' '
> +	# Sort the results with `sort` for a consistent comparison against
> +	# expected
> +	git tag -l --no-sort "foo*" | sort >actual &&
> +	cat >expect <<-\EOF &&
> +	foo1.10
> +	foo1.3
> +	foo1.6
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'invalid sort parameter on command line' '
>  	test_must_fail git tag -l --sort=notvalid "foo*" >actual
>  '
> -- 
> gitgitgadget
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]