From: Karthik Nayak Date: Tue, 07 Oct 2025 14:52:43 GMT Subject: Re: [PATCH 04/13] refs: expose peeled object ID via the iterator Message-ID: In-Reply-To: <20251007-b4-pks-ref-filter-skip-parsing-objects-v1-4-916cc7c6886b@pks.im> Patrick Steinhardt writes: > Both the "files" and "reftable" backend are able to store peeled values > for tags in the respective formats. This allows for a more efficient > lookup of the target object of such a tag without having to manually > peel via the object database. > In the 'files' backend, I thought only packed-refs store peeled values? > The infrastructure to access these peeled object IDs is somewhat funky > though. When iterating through objects, we store a pointer reference to > the current iterator in a global variable. The callbacks invoked by that > iterator are then expected to call `peel_iterated_oid()`, which checks > whether the globally-stored iterator's current reference refers to the > one handed into that function. If so, we ask the iterator to peel the > object, otherwise we manually peel the object via the object database. > Depending on global state like this is somewhat weird and also quite > fragile. > > Introduce a new `struct reference::peeled_oid` field that can be > populated by the reference backends. This field can be accessed via a > new function `reference_get_peeled_oid()` that either uses that value, > if set, or alternatively peels via the ODB. With this change we don't > have to rely on global state anymore, but make the peeled object ID > available to the callback functions directly. > > Adjust trivial callers that already have a `struct reference` available. > Remaining callers will be adjusted in subsequent commits. [snip] > diff --git a/refs.c b/refs.c > index 15ad0ef7a8..5002e56435 100644 > --- a/refs.c > +++ b/refs.c > @@ -2333,6 +2333,18 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct > return peel_object(r, base, peeled) ? -1 : 0; > } > > +int reference_get_peeled_oid(struct repository *repo, > + const struct reference *ref, > + struct object_id *peeled_oid) > +{ > + if (ref->peeled_oid) { > + oidcpy(peeled_oid, ref->peeled_oid); > + return 0; > + } > + > + return peel_object(repo, ref->oid, peeled_oid) ? -1 : 0; > +} > + > So similar to `peel_iterated_oid()` but instead of relying on the reference backend to actually provide us the information, we simply rely on the value if present. This avoids the round-trip. Makes sense. The last resource is to look into the object database. Which should only happen with loose refs in the files backend. > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > index 7987acdc96..7922d63acc 100644 > --- a/refs/packed-backend.c > +++ b/refs/packed-backend.c > @@ -959,11 +959,14 @@ static int next_record(struct packed_ref_iterator *iter) > if ((iter->base.ref.flags & REF_ISBROKEN)) { > oidclr(&iter->peeled, iter->repo->hash_algo); > iter->base.ref.flags &= ~REF_KNOWS_PEELED; > + iter->base.ref.peeled_oid = NULL; > } else { > iter->base.ref.flags |= REF_KNOWS_PEELED; > + iter->base.ref.peeled_oid = &iter->peeled; > } > } else { > oidclr(&iter->peeled, iter->repo->hash_algo); > + iter->base.ref.peeled_oid = NULL; > So my comment on the previous commit holds. We have to manually ensure we reset the fields. > } > > return ITER_OK; > diff --git a/refs/ref-cache.c b/refs/ref-cache.c > index 97555fa118..2f46f650a6 100644 > --- a/refs/ref-cache.c > +++ b/refs/ref-cache.c > @@ -428,6 +428,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) > iter->base.ref.name = entry->name; > iter->base.ref.target = entry->u.value.referent; > iter->base.ref.oid = &entry->u.value.oid; > + iter->base.ref.peeled_oid = NULL; > iter->base.ref.flags = entry->flag; > return ITER_OK; > } > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > index 7fbc77492e..f93ab96358 100644 > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -546,6 +546,7 @@ struct reftable_ref_iterator { > struct reftable_iterator iter; > struct reftable_ref_record ref; > struct object_id oid; > + struct object_id peeled_oid; > > char *prefix; > size_t prefix_len; > @@ -670,6 +671,8 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator) > case REFTABLE_REF_VAL2: > oidread(&iter->oid, iter->ref.value.val2.value, > refs->base.repo->hash_algo); > + oidread(&iter->peeled_oid, iter->ref.value.val2.target_value, > + refs->base.repo->hash_algo); > break; > case REFTABLE_REF_SYMREF: > referent = refs_resolve_ref_unsafe(&iter->refs->base, > @@ -706,6 +709,10 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator) > iter->base.ref.name = iter->ref.refname; > iter->base.ref.target = referent; > iter->base.ref.oid = &iter->oid; > + if (iter->ref.value_type == REFTABLE_REF_VAL2) > + iter->base.ref.peeled_oid = &iter->peeled_oid; > + else > + iter->base.ref.peeled_oid = NULL; > iter->base.ref.flags = flags; > > break; > > -- > 2.51.0.764.g787ff6f08a.dirty All of this looks good and as expected!