Re: [PATCH 01/13] refs: introduce wrapper struct for `each_ref_fn`

From: Justin Tobler <jltobler@gmail.com>
Date: Oct 7, 2025, 18:05 UTC
Message-ID: <jrst5hft3o7ee72hrmswhrnz46rgvjihdxgfsougg5u5vs6os4@2prgx3uw6qp7>
In-Reply-To: <20251007-b4-pks-ref-filter-skip-parsing-objects-v1-1-916cc7c6886b@pks.im>

On 25/10/07 12:58PM, Patrick Steinhardt wrote:

Show 19 quoted lines

> The `each_ref_fn` callback function type is used across our code base
> for several different functions that iterate through reference. There's
> a bunch of callbacks implementing this type, which makes any changes to
> the callback signature extremely noisy. An example of the required churn
> is e8207717f1 (refs: add referent to each_ref_fn, 2024-08-09): adding a
> single argument required us to change 48 files.
> 
> It was already proposed back then [1] that we might want to introduce a
> wrapper structure to alleviate the pain going forward. While this of
> course requires the same kind of global refactoring as just introducing
> a new parameter, it at least allows us to more change the callback type
> afterwards by just extending the wrapper structure.
> 
> One counterargument to this refactoring is that it makes the structure
> more opaque. While it is obvious which callsites need to be fixed up
> when we change the function type, it's not obvious anymore once we use
> a structure. That being said, we only have a handful of sites that
> actually need to populate this wrapper structure: our ref backends and
> "refs/iterator.c".

It looks like we also populate `stuct reference` in a couple other spots where we invoke the callback explicitly.

Show 7 quoted lines

> Introduce this wrapper structure so that we can adapt the iterator
> interfaces more readily.
> 
> [1]: <ZmarVcF5JjsZx0dl@tanuki>
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

[snip]

Show 25 quoted lines

> diff --git a/refs.h b/refs.h
> index 4e6bd63aa86..2b24a3d9974 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -355,14 +355,32 @@ struct ref_transaction;
>   */
>  #define REF_BAD_NAME 0x08
>  
> +/* A reference passed to `for_each_ref()`-style callbacks. */
> +struct reference {
> +	/* The fully-qualified name of the reference. */
> +	const char *name;
> +
> +	/* The target of a symbolic ref. `NULL` for direct references. */
> +	const char *target;
> +
> +	/*
> +	 * The object ID of a reference. Either the direct object ID or the
> +	 * resolved object ID in the case of a symbolic ref. May be the zero
> +	 * object ID in case the symbolic ref cannot be resolved.
> +	 */
> +	const struct object_id *oid;
> +
> +	/* A bitfield of `REF_` flags. */
> +	int flags;

I was considering for a little while whether it would make sense for all the arguments to be moved here, or if ones such as flags should remain. Since all these fields directly relate to the reference though, I think it does make sense to relocate all of them.

> +};

Ok, so now all the explicit callback arguments are contained in `struct reference` here. Going forward this certainly would reduce churn if need need to add additional fields here. Overall, this seems sensible to me.

Show 7 quoted lines

> +
>  /*
>   * The signature for the callback function for the for_each_*()
>   * functions below.  The memory pointed to by the refname and oid
>   * arguments is only guaranteed to be valid for the duration of a
>   * single callback invocation.
>   */

Should we update this comment now that these fields are contained the wrapper struct?

[snip]

Show 15 quoted lines

> diff --git a/refs/iterator.c b/refs/iterator.c
> index 17ef841d8a3..7f2e718f1c9 100644
> --- a/refs/iterator.c
> +++ b/refs/iterator.c
> @@ -476,7 +476,14 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,
>  
>  	current_ref_iter = iter;
>  	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
> -		retval = fn(iter->refname, iter->referent, iter->oid, iter->flags, cb_data);
> +		struct reference ref = {
> +			.name = iter->refname,
> +			.target = iter->referent,
> +			.oid = iter->oid,
> +			.flags = iter->flags,
> +		};

Now we wire up the new wrapper struct instead of passing explicit arguments. Looks good.

Show 5 quoted lines

> +
> +		retval = fn(&ref, cb_data);
>  		if (retval)
>  			goto out;
>  	}

-Justin