From: Karthik Nayak Date: Mon, 23 Feb 2026 08:14:00 GMT Subject: Re: [PATCH 06/17] refs: introduce `refs_for_each_ref_ext` Message-ID: In-Reply-To: <20260220-pks-refs-for-each-unification-v1-6-17170bd99de1@pks.im> Patrick Steinhardt writes: > In the refs subsystem we have a proliferation of functions that all > iterate through references. (Almost) all of these functions internally > call `do_for_each_ref()` and provide slightly different arguments so > that one can control different aspects of its behaviour. This approach > doesn't really scale: every time there is a slightly different use case > for iterating through refs we create another new function. > > This combinatorial explosion doesn't make a lot of sense: it leads to > confusing interfaces and heightens the maintenance burden. > > Refactor the code to become more composable by: > > - Exposing `do_for_each_ref()` as `refs_for_each_ref_ext()`. > > - Introducing an options structure that lets the caller control > individual options. > > This gives us a much better foundation to build on going forward. > > Signed-off-by: Patrick Steinhardt > --- > refs.c | 78 ++++++++++++++++++++++++++++++++++++++++-------------------------- > refs.h | 29 +++++++++++++++++++++++++ > 2 files changed, 77 insertions(+), 30 deletions(-) > > diff --git a/refs.c b/refs.c > index a45cc61211..ec9e466381 100644 > --- a/refs.c > +++ b/refs.c > @@ -1858,62 +1858,76 @@ struct ref_iterator *refs_ref_iterator_begin( > return iter; > } > > -static int do_for_each_ref(struct ref_store *refs, const char *prefix, > - const char **exclude_patterns, > - refs_for_each_cb fn, int trim, > - enum refs_for_each_flag flags, void *cb_data) > +int refs_for_each_ref_ext(struct ref_store *refs, > + refs_for_each_cb cb, void *cb_data, > + const struct refs_for_each_ref_options *opts) So instead of passing prefix, exclude_patterns and trim, we pass in a struct which contains the options. Ok. We also rename fn to cb, since that is more legible. > { > struct ref_iterator *iter; > > if (!refs) > return 0; > > - iter = refs_ref_iterator_begin(refs, prefix, exclude_patterns, trim, > - flags); > + iter = refs_ref_iterator_begin(refs, opts->prefix ? opts->prefix : "", > + opts->exclude_patterns, > + opts->trim_prefix, opts->flags); > > - return do_for_each_ref_iterator(iter, fn, cb_data); > + return do_for_each_ref_iterator(iter, cb, cb_data); > } > > -int refs_for_each_ref(struct ref_store *refs, refs_for_each_cb fn, void *cb_data) > +int refs_for_each_ref(struct ref_store *refs, refs_for_each_cb cb, void *cb_data) > { > - return do_for_each_ref(refs, "", NULL, fn, 0, 0, cb_data); > + struct refs_for_each_ref_options opts = { 0 }; > + return refs_for_each_ref_ext(refs, cb, cb_data, &opts); > } > > int refs_for_each_ref_in(struct ref_store *refs, const char *prefix, > - refs_for_each_cb fn, void *cb_data) > + refs_for_each_cb cb, void *cb_data) > { > - return do_for_each_ref(refs, prefix, NULL, fn, strlen(prefix), 0, cb_data); > + struct refs_for_each_ref_options opts = { > + .prefix = prefix, > + .trim_prefix = strlen(prefix), > + }; > + return refs_for_each_ref_ext(refs, cb, cb_data, &opts); > } > > int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix, > const char **exclude_patterns, > - refs_for_each_cb fn, void *cb_data) > + refs_for_each_cb cb, void *cb_data) > { > - return do_for_each_ref(refs, prefix, exclude_patterns, fn, 0, 0, cb_data); > + struct refs_for_each_ref_options opts = { > + .prefix = prefix, > + .exclude_patterns = exclude_patterns, > + }; > + return refs_for_each_ref_ext(refs, cb, cb_data, &opts); > } > > -int refs_for_each_replace_ref(struct ref_store *refs, refs_for_each_cb fn, void *cb_data) > +int refs_for_each_replace_ref(struct ref_store *refs, refs_for_each_cb cb, void *cb_data) > { > const char *git_replace_ref_base = ref_namespace[NAMESPACE_REPLACE].ref; > - return do_for_each_ref(refs, git_replace_ref_base, NULL, fn, > - strlen(git_replace_ref_base), > - REFS_FOR_EACH_INCLUDE_BROKEN, cb_data); > + struct refs_for_each_ref_options opts = { > + .prefix = git_replace_ref_base, > + .trim_prefix = strlen(git_replace_ref_base), > + .flags = REFS_FOR_EACH_INCLUDE_BROKEN, > + }; > + return refs_for_each_ref_ext(refs, cb, cb_data, &opts); > } > > int refs_for_each_namespaced_ref(struct ref_store *refs, > const char **exclude_patterns, > - refs_for_each_cb fn, void *cb_data) > + refs_for_each_cb cb, void *cb_data) > { > + struct refs_for_each_ref_options opts = { 0 }; > struct strvec namespaced_exclude_patterns = STRVEC_INIT; > struct strbuf prefix = STRBUF_INIT; > int ret; > > - exclude_patterns = get_namespaced_exclude_patterns(exclude_patterns, > - get_git_namespace(), > - &namespaced_exclude_patterns); > - > + opts.exclude_patterns = get_namespaced_exclude_patterns(exclude_patterns, > + get_git_namespace(), > + &namespaced_exclude_patterns); > strbuf_addf(&prefix, "%srefs/", get_git_namespace()); > - ret = do_for_each_ref(refs, prefix.buf, exclude_patterns, fn, 0, 0, cb_data); > + opts.prefix = prefix.buf; > + > + ret = refs_for_each_ref_ext(refs, cb, cb_data, &opts); > > strvec_clear(&namespaced_exclude_patterns); > strbuf_release(&prefix); > @@ -1926,10 +1940,13 @@ int refs_for_each_rawref(struct ref_store *refs, refs_for_each_cb fn, void *cb_d > } > > int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix, > - refs_for_each_cb fn, void *cb_data) > + refs_for_each_cb cb, void *cb_data) > { > - return do_for_each_ref(refs, prefix, NULL, fn, 0, > - REFS_FOR_EACH_INCLUDE_BROKEN, cb_data); > + struct refs_for_each_ref_options opts = { > + .prefix = prefix, > + .flags = REFS_FOR_EACH_INCLUDE_BROKEN, > + }; > + return refs_for_each_ref_ext(refs, cb, cb_data, &opts); > } > > static int qsort_strcmp(const void *va, const void *vb) > @@ -3187,6 +3204,9 @@ int repo_migrate_ref_storage_format(struct repository *repo, > struct strbuf *errbuf) > { > struct ref_store *old_refs = NULL, *new_refs = NULL; > + struct refs_for_each_ref_options for_each_ref_opts = { > + .flags = REFS_FOR_EACH_INCLUDE_ROOT_REFS | REFS_FOR_EACH_INCLUDE_BROKEN, > + }; > struct ref_transaction *transaction = NULL; > struct strbuf new_gitdir = STRBUF_INIT; > struct migration_data data = { > @@ -3270,7 +3290,7 @@ int repo_migrate_ref_storage_format(struct repository *repo, > data.errbuf = errbuf; > > /* > - * We need to use the internal `do_for_each_ref()` here so that we can > + * We need to use `refs_for_each_ref_ext()` here so that we can > * also include broken refs and symrefs. These would otherwise be > * skipped silently. > * > @@ -3280,9 +3300,7 @@ int repo_migrate_ref_storage_format(struct repository *repo, > * allow for a central lock due to its design. It's thus on the user to > * ensure that there are no concurrent writes. > */ > - ret = do_for_each_ref(old_refs, "", NULL, migrate_one_ref, 0, > - REFS_FOR_EACH_INCLUDE_ROOT_REFS | REFS_FOR_EACH_INCLUDE_BROKEN, > - &data); > + ret = refs_for_each_ref_ext(old_refs, migrate_one_ref, &data, &for_each_ref_opts); > if (ret < 0) > goto done; > Then we modify all the callees to use the new options struct instead of passing in the arguments individually. Seems good. > diff --git a/refs.h b/refs.h > index 5190e98b2c..bb9c64a51c 100644 > --- a/refs.h > +++ b/refs.h > @@ -453,8 +453,37 @@ int refs_head_ref(struct ref_store *refs, > int refs_head_ref_namespaced(struct ref_store *refs, > refs_for_each_cb fn, void *cb_data); > > + > +struct refs_for_each_ref_options { > + /* Only iterate over references that have this given prefix. */ > + const char *prefix; > + > + /* > + * Exclude any references that match any of these patterns on a > + * best-effort basis. The caller needs to be prepared for the exclude > + * patterns to be ignored. > + * > + * The array must be terminated with a NULL sentinel value. > + */ > + const char **exclude_patterns; > + > + /* > + * The number of bytes to trim from the refname. Note that the trimmed > + * bytes must not cause the reference to become empty. As such, this > + * field should typically only be set when one uses a `prefix` ending > + * in a slash. > + */ > + size_t trim_prefix; > + > + /* Flags that change which refs will be included. */ > + enum refs_for_each_flag flags; > +}; > + > int refs_for_each_ref(struct ref_store *refs, > refs_for_each_cb fn, void *cb_data); > +int refs_for_each_ref_ext(struct ref_store *refs, > + refs_for_each_cb cb, void *cb_data, > + const struct refs_for_each_ref_options *opts); > int refs_for_each_ref_in(struct ref_store *refs, const char *prefix, > refs_for_each_cb fn, void *cb_data); > int refs_for_each_tag_ref(struct ref_store *refs, > > -- > 2.53.0.414.gf7e9f6c205.dirty