for-each-ref: delay parsing of --sort=<atom> options

The for-each-ref family of commands invoke parsers immediately when it sees each --sort=<atom> option, and die before even seeing the other options on the command line when the <atom> is unrecognised. Instead, accumulate them in a string list, and have them parsed into a ref_sorting structure after the command line parsing is done. As a consequence, "git branch --sort=bogus -h" used to fail to give the brief help, which arguably may have been a feature, now does so, which is more consistent with how other options work. The patch is smaller than the actual extent of the "damage" to the codebase, thanks to the fact that the original code consistently used OPT_REF_SORT() macro to handle command line options. We only needed to replace the variable used for the list, and implementation of the callback function used in the macro. The old rule was for the users of the API to: - Declare ref_sorting and ref_sorting_tail variables; - OPT_REF_SORT() macro will instantiate ref_sorting instance (which may barf and die) and append it to the tail; - Append to the tail each ref_sorting read from the configuration by parsing in the config callback (which may barf and die); - See if ref_sorting is null and use ref_sorting_default() instead. Now the rule is not all that different but is simpler: - Declare ref_sorting_options string list. - OPT_REF_SORT() macro will append it to the string list; - Append to the string list the sort key read from the configuration; - call ref_sorting_options() to turn the string list to ref_sorting structure (which also deals with the default value). As side effects, this change also cleans up a few issues: - 95be717c (parse_opt_ref_sorting: always use with NONEG flag, 2019-03-20) muses that "git for-each-ref --no-sort" should simply clear the sort keys accumulated so far; it now does. - The implementation detail of "struct ref_sorting" and the helper function parse_ref_sorting() can now be private to the ref-filter API implementation. - If you set branch.sort to a bogus value, the any "git branch" invocation, not only the listing mode, would abort with the original code; now it doesn't Signed-off-by: Junio C Hamano <gitster@pobox.com>

Junio C Hamano committed Oct 20, 2021 at 12:23 UTC 98e7ab6d42beba8b35fefb3856b07ac20e89d1ca
8 files changed +95 -52
builtin/branch.c
+6 -7
@@ -77,12 +77,11 @@ define_list_config_array(color_branch_slots);
77 static int git_branch_config(const char *var, const char *value, void *cb)
78 {
79 const char *slot_name;
80 - struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
80
81 if (!strcmp(var, "branch.sort")) {
82 if (!value)
83 return config_error_nonbool(var);
85 - parse_ref_sorting(sorting_tail, value);
84 + string_list_append(cb, value);
85 return 0;
86 }
87
@@ -625,7 +624,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
624 enum branch_track track;
625 struct ref_filter filter;
626 int icase = 0;
628 - static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
627 + static struct ref_sorting *sorting;
628 + struct string_list sorting_options = STRING_LIST_INIT_DUP;
629 struct ref_format format = REF_FORMAT_INIT;
630
631 struct option options[] = {
@@ -666,7 +666,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
666 OPT_MERGED(&filter, N_("print only branches that are merged")),
667 OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
668 OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
669 - OPT_REF_SORT(sorting_tail),
669 + OPT_REF_SORT(&sorting_options),
670 OPT_CALLBACK(0, "points-at", &filter.points_at, N_("object"),
671 N_("print only branches of the object"), parse_opt_object_name),
672 OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
@@ -683,7 +683,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
683 if (argc == 2 && !strcmp(argv[1], "-h"))
684 usage_with_options(builtin_branch_usage, options);
685
686 - git_config(git_branch_config, sorting_tail);
686 + git_config(git_branch_config, &sorting_options);
687
688 track = git_branch_track;
689
@@ -749,8 +749,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
749 * local branches 'refs/heads/...' and finally remote-tracking
750 * branches 'refs/remotes/...'.
751 */
752 - if (!sorting)
753 - sorting = ref_default_sorting();
752 + sorting = ref_sorting_options(&sorting_options);
753 ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
754 ref_sorting_set_sort_flags_all(
755 sorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);
builtin/for-each-ref.c
+4 -4
@@ -17,7 +17,8 @@ static char const * const for_each_ref_usage[] = {
17 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
18 {
19 int i;
20 - struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
20 + struct ref_sorting *sorting;
21 + struct string_list sorting_options = STRING_LIST_INIT_DUP;
22 int maxcount = 0, icase = 0;
23 struct ref_array array;
24 struct ref_filter filter;
@@ -39,7 +40,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
40 OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
41 OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")),
42 OPT__COLOR(&format.use_color, N_("respect format colors")),
42 - OPT_REF_SORT(sorting_tail),
43 + OPT_REF_SORT(&sorting_options),
44 OPT_CALLBACK(0, "points-at", &filter.points_at,
45 N_("object"), N_("print only refs which points at the given object"),
46 parse_opt_object_name),
@@ -70,8 +71,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
71 if (verify_ref_format(&format))
72 usage_with_options(for_each_ref_usage, opts);
73
73 - if (!sorting)
74 - sorting = ref_default_sorting();
74 + sorting = ref_sorting_options(&sorting_options);
75 ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
76 filter.ignore_case = icase;
77
builtin/ls-remote.c
+8 -5
@@ -54,7 +54,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
54 struct transport *transport;
55 const struct ref *ref;
56 struct ref_array ref_array;
57 - static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
57 + struct string_list sorting_options = STRING_LIST_INIT_DUP;
58
59 struct option options[] = {
60 OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -68,7 +68,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
68 OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
69 OPT_BOOL(0, "get-url", &get_url,
70 N_("take url.<base>.insteadOf into account")),
71 - OPT_REF_SORT(sorting_tail),
71 + OPT_REF_SORT(&sorting_options),
72 OPT_SET_INT_F(0, "exit-code", &status,
73 N_("exit with exit code 2 if no matching refs are found"),
74 2, PARSE_OPT_NOCOMPLETE),
@@ -86,8 +86,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
86
87 packet_trace_identity("ls-remote");
88
89 - UNLEAK(sorting);
90 -
89 if (argc > 1) {
90 int i;
91 CALLOC_ARRAY(pattern, argc);
@@ -139,8 +137,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
137 item->symref = xstrdup_or_null(ref->symref);
138 }
139
142 - if (sorting)
140 + if (sorting_options.nr) {
141 + struct ref_sorting *sorting;
142 +
143 + sorting = ref_sorting_options(&sorting_options);
144 ref_array_sort(sorting, &ref_array);
145 + ref_sorting_release(sorting);
146 + }
147
148 for (i = 0; i < ref_array.nr; i++) {
149 const struct ref_array_item *ref = ref_array.items[i];
builtin/tag.c
+6 -7
@@ -178,7 +178,6 @@ static const char tag_template_nocleanup[] =
178 static int git_tag_config(const char *var, const char *value, void *cb)
179 {
180 int status;
181 - struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
181
182 if (!strcmp(var, "tag.gpgsign")) {
183 config_sign_tag = git_config_bool(var, value);
@@ -188,7 +187,7 @@ static int git_tag_config(const char *var, const char *value, void *cb)
187 if (!strcmp(var, "tag.sort")) {
188 if (!value)
189 return config_error_nonbool(var);
191 - parse_ref_sorting(sorting_tail, value);
190 + string_list_append(cb, value);
191 return 0;
192 }
193
@@ -436,7 +435,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
435 struct ref_transaction *transaction;
436 struct strbuf err = STRBUF_INIT;
437 struct ref_filter filter;
439 - static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
438 + struct ref_sorting *sorting;
439 + struct string_list sorting_options = STRING_LIST_INIT_DUP;
440 struct ref_format format = REF_FORMAT_INIT;
441 int icase = 0;
442 int edit_flag = 0;
@@ -470,7 +470,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
470 OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
471 OPT_MERGED(&filter, N_("print only tags that are merged")),
472 OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
473 - OPT_REF_SORT(sorting_tail),
473 + OPT_REF_SORT(&sorting_options),
474 {
475 OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
476 N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT,
@@ -486,7 +486,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
486
487 setup_ref_filter_porcelain_msg();
488
489 - git_config(git_tag_config, sorting_tail);
489 + git_config(git_tag_config, &sorting_options);
490
491 memset(&opt, 0, sizeof(opt));
492 memset(&filter, 0, sizeof(filter));
@@ -525,8 +525,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
525 die(_("--column and -n are incompatible"));
526 colopts = 0;
527 }
528 - if (!sorting)
529 - sorting = ref_default_sorting();
528 + sorting = ref_sorting_options(&sorting_options);
529 ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
530 filter.ignore_case = icase;
531 if (cmdmode == 'l') {
ref-filter.c
+24 -10
@@ -2470,6 +2470,12 @@ static int memcasecmp(const void *vs1, const void *vs2, size_t n)
2470 return 0;
2471 }
2472
2473 +struct ref_sorting {
2474 + struct ref_sorting *next;
2475 + int atom; /* index into used_atom array (internal) */
2476 + enum ref_sorting_order sort_flags;
2477 +};
2478 +
2479 static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)
2480 {
2481 struct atom_value *va, *vb;
@@ -2663,7 +2669,7 @@ static int parse_sorting_atom(const char *atom)
2669 }
2670
2671 /* If no sorting option is given, use refname to sort as default */
2666 -struct ref_sorting *ref_default_sorting(void)
2672 +static struct ref_sorting *ref_default_sorting(void)
2673 {
2674 static const char cstr_name[] = "refname";
2675
@@ -2674,7 +2680,7 @@ struct ref_sorting *ref_default_sorting(void)
2680 return sorting;
2681 }
2682
2677 -void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
2683 +static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
2684 {
2685 struct ref_sorting *s;
2686
@@ -2692,17 +2698,25 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
2698 s->atom = parse_sorting_atom(arg);
2699 }
2700
2695 -int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
2701 +struct ref_sorting *ref_sorting_options(struct string_list *options)
2702 {
2703 + struct string_list_item *item;
2704 + struct ref_sorting *sorting = NULL, **tail = &sorting;
2705 +
2706 + if (!options->nr) {
2707 + sorting = ref_default_sorting();
2708 + } else {
2709 + for_each_string_list_item(item, options)
2710 + parse_ref_sorting(tail, item->string);
2711 + }
2712 +
2713 /*
2698 - * NEEDSWORK: We should probably clear the list in this case, but we've
2699 - * already munged the global used_atoms list, which would need to be
2700 - * undone.
2714 + * From here on, the ref_sorting list should be used to talk
2715 + * about the sort order used for the output. The caller
2716 + * should not touch the string form anymore.
2717 */
2702 - BUG_ON_OPT_NEG(unset);
2703 -
2704 - parse_ref_sorting(opt->value, arg);
2705 - return 0;
2718 + string_list_clear(options, 0);
2719 + return sorting;
2720 }
2721
2722 void ref_sorting_release(struct ref_sorting *sorting)
ref-filter.h
+10 -18
@@ -23,16 +23,13 @@
23 #define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
24
25 struct atom_value;
26 +struct ref_sorting;
27
27 -struct ref_sorting {
28 - struct ref_sorting *next;
29 - int atom; /* index into used_atom array (internal) */
30 - enum {
31 - REF_SORTING_REVERSE = 1<<0,
32 - REF_SORTING_ICASE = 1<<1,
33 - REF_SORTING_VERSION = 1<<2,
34 - REF_SORTING_DETACHED_HEAD_FIRST = 1<<3,
35 - } sort_flags;
28 +enum ref_sorting_order {
29 + REF_SORTING_REVERSE = 1<<0,
30 + REF_SORTING_ICASE = 1<<1,
31 + REF_SORTING_VERSION = 1<<2,
32 + REF_SORTING_DETACHED_HEAD_FIRST = 1<<3,
33 };
34
35 struct ref_array_item {
@@ -97,9 +94,8 @@ struct ref_format {
94 #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED("no-merged", f, h)
95
96 #define OPT_REF_SORT(var) \
100 - OPT_CALLBACK_F(0, "sort", (var), \
101 - N_("key"), N_("field name to sort on"), \
102 - PARSE_OPT_NONEG, parse_opt_ref_sorting)
97 + OPT_STRING_LIST(0, "sort", (var), \
98 + N_("key"), N_("field name to sort on"))
99
100 /*
101 * API for filtering a set of refs. Based on the type of refs the user
@@ -121,14 +117,10 @@ int format_ref_array_item(struct ref_array_item *info,
117 struct ref_format *format,
118 struct strbuf *final_buf,
119 struct strbuf *error_buf);
124 -/* Parse a single sort specifier and add it to the list */
125 -void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
126 -/* Callback function for parsing the sort option */
127 -int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
128 -/* Default sort option based on refname */
129 -struct ref_sorting *ref_default_sorting(void);
120 /* Release a "struct ref_sorting" */
121 void ref_sorting_release(struct ref_sorting *);
122 +/* Convert list of sort options into ref_sorting */
123 +struct ref_sorting *ref_sorting_options(struct string_list *);
124 /* Function to parse --merged and --no-merged options */
125 int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
126 /* Get the current HEAD's description */
t/t3200-branch.sh
+11 -1
@@ -1418,7 +1418,17 @@ test_expect_success 'invalid sort parameter in configuration' '
1418 (
1419 cd sort &&
1420 git config branch.sort "v:notvalid" &&
1421 - test_must_fail git branch
1421 +
1422 + # this works in the "listing" mode, so bad sort key
1423 + # is a dying offence.
1424 + test_must_fail git branch &&
1425 +
1426 + # these do not need to use sorting, and should all
1427 + # succeed
1428 + git branch newone main &&
1429 + git branch -c newone newerone &&
1430 + git branch -m newone newestone &&
1431 + git branch -d newerone newestone
1432 )
1433 '
1434
t/t6300-for-each-ref.sh
+26
@@ -419,6 +419,11 @@ test_expect_success 'Verify descending sort' '
419 test_cmp expected actual
420 '
421
422 +test_expect_success 'Give help even with invalid sort atoms' '
423 + test_expect_code 129 git for-each-ref --sort=bogus -h >actual 2>&1 &&
424 + grep "^usage: git for-each-ref" actual
425 +'
426 +
427 cat >expected <<\EOF
428 refs/tags/testtag
429 refs/tags/testtag-2
@@ -1019,6 +1024,27 @@ test_expect_success 'equivalent sorts fall back on refname' '
1024 test_cmp expected actual
1025 '
1026
1027 +test_expect_success '--no-sort cancels the previous sort keys' '
1028 + cat >expected <<-\EOF &&
1029 + 100000 <user1@example.com> refs/tags/multi-ref1-100000-user1
1030 + 100000 <user2@example.com> refs/tags/multi-ref1-100000-user2
1031 + 100000 <user1@example.com> refs/tags/multi-ref2-100000-user1
1032 + 100000 <user2@example.com> refs/tags/multi-ref2-100000-user2
1033 + 200000 <user1@example.com> refs/tags/multi-ref1-200000-user1
1034 + 200000 <user2@example.com> refs/tags/multi-ref1-200000-user2
1035 + 200000 <user1@example.com> refs/tags/multi-ref2-200000-user1
1036 + 200000 <user2@example.com> refs/tags/multi-ref2-200000-user2
1037 + EOF
1038 + git for-each-ref \
1039 + --format="%(taggerdate:unix) %(taggeremail) %(refname)" \
1040 + --sort=-refname \
1041 + --sort=taggeremail \
1042 + --no-sort \
1043 + --sort=taggerdate \
1044 + "refs/tags/multi-*" >actual &&
1045 + test_cmp expected actual
1046 +'
1047 +
1048 test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
1049 test_when_finished "git checkout main" &&
1050 git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ >actual &&