Re: [PATCH v2 00/14] refs: improvements and fixes for peeling tags

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 9, 2025, 07:24 UTC
Message-ID: <aOdjM8F6WvTEBIo_@pks.im>
In-Reply-To: <20251009063956.GA1622884@coredump.intra.peff.net>

On Thu, Oct 09, 2025 at 02:39:56AM -0400, Jeff King wrote:

Show 31 quoted lines

> On Thu, Oct 09, 2025 at 08:09:53AM +0200, Patrick Steinhardt wrote:
> 
> > > I do have one minor complaint, though: the name of that struct. I have a
> > > feeling that the name "struct reference" may cause confusion down the
> > > road because it's so generic, and because "references" and "refs" are so
> > > common in the code. From the names, when would I know when to use
> > > "struct reference" and when "struct ref"?
> > > 
> > > Could we give it a name that ties it to the iteration interface?
> > > Something like iterated_ref, each_ref_data, etc?
> > > 
> > > I know this is minor (and will be annoying to adjust your series), but
> > > I'd rather raise the point now than realize later that it's confusing
> > > and try to change it then.
> > 
> > It is puzzling indeed. I would claim that in this case it is not `struct
> > reference` that is misnamed: what it contains is as close as you get to
> > a representation of a reference. It's rather `struct ref` that is
> > misnamed, as it carries a lot of data that is only valid in the context
> > of a remote.
> > 
> > Another approach could thus be to rename `struct ref` to `struct
> > remote_ref`, which I would claim would be a clear win for better semantics.
> > It's used in lots of places though, which is a valid counter argument.
> 
> I am not so much arguing that "struct reference" is misnamed, as that it
> is sufficiently generic that people will reach for it when it is not the
> appropriate tool. It is for passing the ref data to the iterator
> callback, but it probably doesn't make sense in other contexts. Would we
> ever expect anybody to declare their own "struct reference" in a local
> function? I don't think so.

Hm. The thing is: if `struct reference` is established in our code base, and if it is a simple representation of a reference, then I think it might even be a good thing to having it. I could for example see that we gain more interfaces over time that use it.

For example, functions like `refs_read_ref()` could totally be adapted to use the same struct, and I would claim that this is a good thing. We'd basically have a single central structure that allows us to get a reference out of the "refs" subsystem with metadata.

This would for example allow us to make the peeling infrastructure available in contexts where we don't use an iterator, which is something that we cannot do right now. In the end, what this would enable is to have higher-level interfaces for references.

So yes, `struct reference` is rather generic. But if it's the ref system owning it I think it's sensible, because references are basically what this subsystem is all about. This is somewhat equivalent to `struct object`: the struct name is generic, but it's the basic building block for our object subsystem.

Show 6 quoted lines

> And yes, "struct ref" suffers somewhat from the same problem. It is
> mostly about using refs in one specific space, but the name does not
> really help clarify that. I wouldn't mind seeing that improved, but yes,
> it would be a noisy patch. I don't know if remote_ref is the right name,
> though (the "peer_ref" links mean we store both local and remote refs in
> it, IIRC).

If we decide to do it it should definitely be a standalone patch (or patch series). I'm also open for different naming suggestions, but for now this is the best I could come up with.

Thanks!

Patrick