Re: [PATCH 01/13] refs: introduce wrapper struct for `each_ref_fn`
On Tue, Oct 07, 2025 at 01:05:15PM -0500, Justin Tobler wrote:
Show 23 quoted lines
> On 25/10/07 12:58PM, Patrick Steinhardt wrote:
> > 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.
Fair, let me mention that.
[snip]
Show 9 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?