ref-filter.c: really don't sort when using --no-sort

When '--no-sort' is passed to 'for-each-ref', 'tag', and 'branch', the
printed refs are still sorted by ascending refname. Change the handling of
sort options in these commands so that '--no-sort' to truly disables
sorting.

'--no-sort' does not disable sorting in these commands is because their
option parsing does not distinguish between "the absence of '--sort'"
(and/or values for tag.sort & branch.sort) and '--no-sort'. Both result in
an empty 'sorting_options' string list, which is parsed by
'ref_sorting_options()' to create the 'struct ref_sorting *' for the
command. If the string list is empty, 'ref_sorting_options()' interprets
that as "the absence of '--sort'" and returns the default ref sorting
structure (equivalent to "refname" sort).

To handle '--no-sort' properly while preserving the "refname" sort in the
"absence of --sort'" case, first explicitly add "refname" to the string list
*before* parsing options. This alone 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.

Now that the string list is populated by default, '--no-sort' is the only
way to empty the 'sorting_options' string list. Update
'ref_sorting_options()' to return a NULL 'struct ref_sorting *' if the
string list is empty, and add a condition to 'ref_array_sort()' to skip the
sort altogether if the sort structure is NULL. Note that other functions
using 'struct ref_sorting *' do not need any changes because they already
ignore NULL values.

Finally, remove the condition around sorting in 'ls-remote', since it's no
longer necessary. Unlike 'for-each-ref' et. al., it does *not* do any
sorting by default. This default is preserved by simply leaving its sort key
string list empty before parsing options; if no additional sort keys are
set, 'struct ref_sorting *' is NULL and sorting is skipped.

Signed-off-by: Victoria Dye <vdye@github.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/branch.c b/builtin/branch.c
index 08da650516037e2e18b2e99831562db533111254..2da43f17e4a31d0e034fcc2f2ccd622f0cd81eb2 100644 (file)
--- 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 350bfa6e811b8c53d94ba33083a03bf9d93437dc..93b370f550b54d81868b80793a72ee9ed6cccaa0 100644 (file)
--- 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 fc765754305ed7a3f8a2efdcd604dd29c8b3b502..b416602b4d3cf931869509e980b6c8015bd6121a 100644 (file)
--- 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);
 
        for (i = 0; i < ref_array.nr; i++) {
                const struct ref_array_item *ref = ref_array.items[i];
@@ -157,6 +153,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
                status = 0; /* we found something */
        }
 
+       ref_sorting_release(sorting);
        ref_array_clear(&ref_array);
        if (transport_disconnect(transport))
                status = 1;

diff --git a/builtin/tag.c b/builtin/tag.c
index 3918eacbb57bd9e0d2b90ecbbf6cc07c16fad73b..64f3196cd4caec921ffb94b74c7252e7a37660d6 100644 (file)
--- 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 9dbc4f71bd272a25423f131bbbbb23152a248be8..f65551dc684996cf3d7f4dd9486a552a470faff3 100644 (file)
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -3058,7 +3058,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)
@@ -3164,18 +3165,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;
@@ -3199,9 +3188,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 daf1666df7adb9594c50445764440ad339f43645..470adfeb903c5f721f5d5b855d42bc02bd3011f4 100755 (executable)
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1558,9 +1558,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 &&
@@ -1580,9 +1581,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
@@ -1594,10 +1596,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 &&
+               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 7b943fd34cdece8c4fca0b55e4fab8510ac4b7b1..119720495ea9d3fef5e17f1d49ba26427d82e003 100755 (executable)
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1224,6 +1224,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 e689db429292e60162d4f8f89905cc8489d7a3e0..b41a47eb943a03b1588bdc87802b0645944ce2ec 100755 (executable)
--- 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
+'
+
+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
 '