Re: [PATCH 04/13] refs: expose peeled object ID via the iterator

From: Karthik Nayak <karthik.188@gmail.com>
Date: Oct 7, 2025, 14:52 UTC
Message-ID: <CAOLa=ZRQuLa_xD8GzynHNmNZuyoJeK9dCBOKbUfkCES4ejG0OA@mail.gmail.com>
In-Reply-To: <20251007-b4-pks-ref-filter-skip-parsing-objects-v1-4-916cc7c6886b@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 5 quoted lines

> 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?

Show 19 quoted lines

> 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]

Show 21 quoted lines

> 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.

Show 17 quoted lines

> 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.

Show 50 quoted lines

>  	}
>
>  	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!