Jeff King <peff@xxxxxxxx> writes: > For now there is no program that uses more than one ref-filter format. > But it seems like an obvious interface that would want to be lib-ified > eventually. We are not there yet because of the static global used_atoms > array. But the obvious path forward is to have a context struct > representing one ref-filter iteration. > > I think the intent was that ref_format would be that context struct, > though arguably it is a little funny since it forces the sorting and > formatting to be joined (OTOH, that is very much how the code works, > since it wants to share results between the two for efficiency). > > So one solution would be to make the use of that context struct more > explicit, and require ref_sorting callers to provide a format struct. > Like the patch below, which also passes your tests. > > I dunno. Your patch is deleting more code, which is nice. But I think in > the long run we'd end up replacing it. But maybe making a clean slate > now would make that easier? I could go either way. I agree with you that libification effort would want to move more static variables to members in a context structure. I initially suspected that would be more or less orthogonal, because it is not like we are adding a static or two to a code path that already holds everything else in a context structure, and would be a lot more work, but now you have a patch that makes the first step in that direction and it does not look too bad at all, so ... Thanks. > --- > builtin/branch.c | 2 +- > builtin/for-each-ref.c | 2 +- > builtin/ls-remote.c | 4 +++- > builtin/tag.c | 2 +- > ref-filter.c | 19 ++++++++----------- > ref-filter.h | 2 +- > 6 files changed, 15 insertions(+), 16 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index 6e7b0cfddb..0c3f35cd0a 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -875,7 +875,7 @@ int cmd_branch(int argc, > * local branches 'refs/heads/...' and finally remote-tracking > * branches 'refs/remotes/...'. > */ > - sorting = ref_sorting_options(&sorting_options); > + sorting = ref_sorting_options(&sorting_options, &format); > ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase); > ref_sorting_set_sort_flags_all( > sorting, REF_SORTING_DETACHED_HEAD_FIRST, 1); > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c > index 715745a262..4f247efe57 100644 > --- a/builtin/for-each-ref.c > +++ b/builtin/for-each-ref.c > @@ -80,7 +80,7 @@ int cmd_for_each_ref(int argc, > if (verify_ref_format(&format)) > usage_with_options(for_each_ref_usage, opts); > > - sorting = ref_sorting_options(&sorting_options); > + sorting = ref_sorting_options(&sorting_options, &format); > ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase); > filter.ignore_case = icase; > > diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c > index 42f34e1236..ed38b82346 100644 > --- a/builtin/ls-remote.c > +++ b/builtin/ls-remote.c > @@ -61,6 +61,7 @@ int cmd_ls_remote(int argc, > const struct ref *ref; > struct ref_array ref_array; > struct ref_sorting *sorting; > + struct ref_format format = REF_FORMAT_INIT; > struct string_list sorting_options = STRING_LIST_INIT_DUP; > > struct option options[] = { > @@ -155,7 +156,7 @@ int cmd_ls_remote(int argc, > item->symref = xstrdup_or_null(ref->symref); > } > > - sorting = ref_sorting_options(&sorting_options); > + sorting = ref_sorting_options(&sorting_options, &format); > ref_array_sort(sorting, &ref_array); > > for (i = 0; i < ref_array.nr; i++) { > @@ -173,6 +174,7 @@ int cmd_ls_remote(int argc, > status = 1; > transport_ls_refs_options_release(&transport_options); > > + ref_format_clear(&format); > strvec_clear(&pattern); > string_list_clear(&server_options, 0); > return status; > diff --git a/builtin/tag.c b/builtin/tag.c > index c4bd145831..a5240f66e2 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -574,7 +574,7 @@ int cmd_tag(int argc, > die(_("options '%s' and '%s' cannot be used together"), "--column", "-n"); > colopts = 0; > } > - sorting = ref_sorting_options(&sorting_options); > + sorting = ref_sorting_options(&sorting_options, &format); > ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase); > filter.ignore_case = icase; > if (cmdmode == 'l') { > diff --git a/ref-filter.c b/ref-filter.c > index 23054694c2..f5d0c448ed 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -3536,23 +3536,19 @@ void pretty_print_ref(const char *name, const struct object_id *oid, > free_array_item(ref_item); > } > > -static int parse_sorting_atom(const char *atom) > +static int parse_sorting_atom(struct ref_format *format, const char *atom) > { > - /* > - * This parses an atom using a dummy ref_format, since we don't > - * actually care about the formatting details. > - */ > - struct ref_format dummy = REF_FORMAT_INIT; > const char *end = atom + strlen(atom); > struct strbuf err = STRBUF_INIT; > - int res = parse_ref_filter_atom(&dummy, atom, end, &err); > + int res = parse_ref_filter_atom(format, atom, end, &err); > if (res < 0) > die("%s", err.buf); > strbuf_release(&err); > return res; > } > > -static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg) > +static void parse_ref_sorting(struct ref_format *format, > + struct ref_sorting **sorting_tail, const char *arg) > { > struct ref_sorting *s; > > @@ -3567,17 +3563,18 @@ static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg > if (skip_prefix(arg, "version:", &arg) || > skip_prefix(arg, "v:", &arg)) > s->sort_flags |= REF_SORTING_VERSION; > - s->atom = parse_sorting_atom(arg); > + s->atom = parse_sorting_atom(format, arg); > } > > -struct ref_sorting *ref_sorting_options(struct string_list *options) > +struct ref_sorting *ref_sorting_options(struct string_list *options, > + struct ref_format *format) > { > struct string_list_item *item; > struct ref_sorting *sorting = NULL, **tail = &sorting; > > if (options->nr) { > for_each_string_list_item(item, options) > - parse_ref_sorting(tail, item->string); > + parse_ref_sorting(format, tail, item->string); > } > > /* > diff --git a/ref-filter.h b/ref-filter.h > index 754038ab07..1531bf1762 100644 > --- a/ref-filter.h > +++ b/ref-filter.h > @@ -168,7 +168,7 @@ int format_ref_array_item(struct ref_array_item *info, > /* Release a "struct ref_sorting" */ > void ref_sorting_release(struct ref_sorting *); > /* Convert list of sort options into ref_sorting */ > -struct ref_sorting *ref_sorting_options(struct string_list *); > +struct ref_sorting *ref_sorting_options(struct string_list *, struct ref_format *); > /* Function to parse --merged and --no-merged options */ > int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset); > /* Get the current HEAD's description */