On Tue, Nov 07, 2023 at 01:25:53AM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
