From: Karthik Nayak Date: Mon, 23 Feb 2026 09:06:38 GMT Subject: Re: [PATCH 09/17] refs: generalize `refs_for_each_fullref_in_prefixes()` Message-ID: In-Reply-To: <20260220-pks-refs-for-each-unification-v1-9-17170bd99de1@pks.im> Patrick Steinhardt writes: > The function `refs_for_each_fullref_in_prefixes()` can be used to > iterate over all references part of any of the user-provided prefixes. > In contrast to the `prefix` parameter of `refs_for_each_ref_ext()` it > knows to handle the case well where multiple of the passed-in prefixes > start with a common prefix by computing longest common prefixes and then > iterating over those. > > While we could move this logic into `refs_for_each_ref_ext()`, this one > feels somewhat special as we perform multiple iterations. But what we > _can_ do is to generalize how this function works: instead of accepting > only a small handful of parameters, we can have it accept the full > options structure. > > One obvious exception is that the caller must not provide a prefix via > the options. But this case can be easily detected. > > Refactor the code accordingly. > I've attempted the refactoring that this series is doing a couple of times, I always got stuck up on how to integrate this function with the rest. This is a novel approach. > Signed-off-by: Patrick Steinhardt > --- > ls-refs.c | 11 +++++++---- > ref-filter.c | 11 +++++++---- > refs.c | 39 +++++++++++++++------------------------ > refs.h | 16 +++++----------- > 4 files changed, 34 insertions(+), 43 deletions(-) > > diff --git a/ls-refs.c b/ls-refs.c > index 8641281b86..9759826ca7 100644 > --- a/ls-refs.c > +++ b/ls-refs.c > @@ -160,6 +160,7 @@ static int ls_refs_config(const char *var, const char *value, > > int ls_refs(struct repository *r, struct packet_reader *request) > { > + struct refs_for_each_ref_options opts = { 0 }; > struct ls_refs_data data; > > memset(&data, 0, sizeof(data)); > @@ -201,10 +202,12 @@ int ls_refs(struct repository *r, struct packet_reader *request) > send_possibly_unborn_head(&data); > if (!data.prefixes.nr) > strvec_push(&data.prefixes, ""); > - refs_for_each_fullref_in_prefixes(get_main_ref_store(r), > - get_git_namespace(), data.prefixes.v, > - hidden_refs_to_excludes(&data.hidden_refs), > - send_ref, &data); > + > + opts.exclude_patterns = hidden_refs_to_excludes(&data.hidden_refs); > + opts.namespace = get_git_namespace(); > + > + refs_for_each_ref_in_prefixes(get_main_ref_store(r), data.prefixes.v, > + &opts, send_ref, &data); > packet_fflush(stdout); > strvec_clear(&data.prefixes); > strbuf_release(&data.buf); > diff --git a/ref-filter.c b/ref-filter.c > index 049e845a19..7c682e0a33 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -2807,6 +2807,10 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter, > refs_for_each_cb cb, > void *cb_data) > { > + struct refs_for_each_ref_options opts = { > + .exclude_patterns = filter->exclude.v, > + }; > + > if (filter->kind & FILTER_REFS_ROOT_REFS) { > /* In this case, we want to print all refs including root refs. */ > return for_each_fullref_with_seek(filter, cb, cb_data, > @@ -2836,10 +2840,9 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter, > return for_each_fullref_with_seek(filter, cb, cb_data, 0); > } > > - return refs_for_each_fullref_in_prefixes(get_main_ref_store(the_repository), > - NULL, filter->name_patterns, > - filter->exclude.v, > - cb, cb_data); > + return refs_for_each_ref_in_prefixes(get_main_ref_store(the_repository), > + filter->name_patterns, &opts, > + cb, cb_data); > } > > /* > diff --git a/refs.c b/refs.c > index 99994879d9..20d34faeb5 100644 > --- a/refs.c > +++ b/refs.c > @@ -2029,40 +2029,31 @@ static void find_longest_prefixes(struct string_list *out, > strbuf_release(&prefix); > } > > -int refs_for_each_fullref_in_prefixes(struct ref_store *ref_store, > - const char *namespace, > - const char **patterns, > - const char **exclude_patterns, > - refs_for_each_cb fn, void *cb_data) > +int refs_for_each_ref_in_prefixes(struct ref_store *ref_store, > + const char **prefixes, > + const struct refs_for_each_ref_options *opts, > + refs_for_each_cb cb, void *cb_data) > { > - struct strvec namespaced_exclude_patterns = STRVEC_INIT; > - struct string_list prefixes = STRING_LIST_INIT_DUP; > + struct string_list longest_prefixes = STRING_LIST_INIT_DUP; > struct string_list_item *prefix; > - struct strbuf buf = STRBUF_INIT; > - int ret = 0, namespace_len; > + int ret = 0; > > - find_longest_prefixes(&prefixes, patterns); > + if (opts->prefix) > + BUG("refs_for_each_ref_in_prefixes called with specific prefix"); > > - if (namespace) > - strbuf_addstr(&buf, namespace); > - namespace_len = buf.len; > + find_longest_prefixes(&longest_prefixes, prefixes); > > - exclude_patterns = get_namespaced_exclude_patterns(exclude_patterns, > - namespace, > - &namespaced_exclude_patterns); > + for_each_string_list_item(prefix, &longest_prefixes) { > + struct refs_for_each_ref_options prefix_opts = *opts; > + prefix_opts.prefix = prefix->string; > > - for_each_string_list_item(prefix, &prefixes) { > - strbuf_addstr(&buf, prefix->string); > - ret = refs_for_each_fullref_in(ref_store, buf.buf, > - exclude_patterns, fn, cb_data); > + ret = refs_for_each_ref_ext(ref_store, cb, cb_data, > + &prefix_opts); Okay so it still calls `refs_for_each_ref_ext()` on the longest prefix. Makes sense. [snip]