Re: [PATCH v2 3/8] refs/reftable: read references via `struct reftable_backend`

[Patrick Steinhardt <ps@pks.im>]

On Tue, Nov 12, 2024 at 04:26:38PM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Refactor `read_ref_without_reload()` to accept a `struct reftable_stack`
> > as input instead of accepting a `struct reftable_stack`. This allows us
> > to implement an additional caching layer when reading refs where we can
> > reuse reftable iterators.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  refs/reftable-backend.c   | 110 ++++++++++++++++++++------------------
> >  reftable/reftable-stack.h |   3 ++
> >  reftable/stack.c          |   5 ++
> >  3 files changed, 67 insertions(+), 51 deletions(-)
> >
> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> > index 4a28dc8a9d..230adb690d 100644
> > --- a/refs/reftable-backend.c
> > +++ b/refs/reftable-backend.c
> > @@ -51,6 +51,50 @@ static void reftable_backend_release(struct reftable_backend *be)
> >  	be->stack = NULL;
> >  }
> >  
> > +static int reftable_backend_read_ref(struct reftable_backend *be,
> > +				     const char *refname,
> > +				     struct object_id *oid,
> > +				     struct strbuf *referent,
> > +				     unsigned int *type)
> > +{
> > +	struct reftable_ref_record ref = {0};
> > +	int ret;
> > +
> > +	ret = reftable_stack_read_ref(be->stack, refname, &ref);
> > +	if (ret)
> > +		goto done;
> > +
> > +	if (ref.value_type == REFTABLE_REF_SYMREF) {
> > +		strbuf_reset(referent);
> > +		strbuf_addstr(referent, ref.value.symref);
> > +		*type |= REF_ISSYMREF;
> > +	} else if (reftable_ref_record_val1(&ref)) {
> > +		unsigned int hash_id;
> > +
> > +		switch (reftable_stack_hash_id(be->stack)) {
> 
> So, relative to the original, instead of relying on the repository
> and its knowledge of what hash function is used, we ask the stack
> what hash function is in use and use that instead.
> 
> > +		case REFTABLE_HASH_SHA1:
> > +			hash_id = GIT_HASH_SHA1;
> > +			break;
> > +		case REFTABLE_HASH_SHA256:
> > +			hash_id = GIT_HASH_SHA256;
> > +			break;
> > +		default:
> > +			BUG("unhandled hash ID %d", reftable_stack_hash_id(be->stack));
> > +		}
> > +
> > +		oidread(oid, reftable_ref_record_val1(&ref),
> > +			&hash_algos[hash_id]);
> > +	} else {
> > +		/* We got a tombstone, which should not happen. */
> > +		BUG("unhandled reference value type %d", ref.value_type);
> > +	}
> > +
> > +done:
> > +	assert(ret != REFTABLE_API_ERROR);
> > +	reftable_ref_record_release(&ref);
> > +	return ret;
> > +}
> 
> Here is the original that got replaced.  Since ...
> 
> > -static int read_ref_without_reload(struct reftable_ref_store *refs,
> > -				   struct reftable_stack *stack,
> > -				   const char *refname,
> > -				   struct object_id *oid,
> > -				   struct strbuf *referent,
> > -				   unsigned int *type)
> > -{
> > -	struct reftable_ref_record ref = {0};
> > -	int ret;
> > -
> > -	ret = reftable_stack_read_ref(stack, refname, &ref);
> > -	if (ret)
> > -		goto done;
> > -
> > -	if (ref.value_type == REFTABLE_REF_SYMREF) {
> > -		strbuf_reset(referent);
> > -		strbuf_addstr(referent, ref.value.symref);
> > -		*type |= REF_ISSYMREF;
> > -	} else if (reftable_ref_record_val1(&ref)) {
> > -		oidread(oid, reftable_ref_record_val1(&ref),
> > -			refs->base.repo->hash_algo);
> 
> ... we have access to "refs", which is a ref_store, that knows its
> repository, it was just a few pointer references away to get the
> hash id of the Git side.  But of course we use REFTABLE_HASH_*NAME*
> to identify the algorithm at this layer, so we need to translate it
> back to the ide on the Git side before asking oidread() to read it.
> 
> > -	} else {
> > -		/* We got a tombstone, which should not happen. */
> > -		BUG("unhandled reference value type %d", ref.value_type);
> > -	}
> > -
> > -done:
> > -	assert(ret != REFTABLE_API_ERROR);
> > -	reftable_ref_record_release(&ref);
> > -	return ret;
> > -}
> 
> There is one thing that is curious about this step.
> 
> It isn't like we teach stack what hash it uses in this step---the
> reftable_stack_hash_id() could have been implemented as early as
> 59343984 (reftable/system: stop depending on "hash.h", 2024-11-08).

In theory we could've implemented it even earlier than that: the commit
only introduces the reftable-specific hashes, and we had the
Git-specific hashes available before that. Like that we wouldn't even
have to translate between the different hashes in the first place.

> Other than that this step introduces the first caller of
> reftable_stack_hash_id() in the series, the remaining hunks of this
> patch do not have to be part of this patch, but could have been a
> separate step.  Not a suggestion to split it out, but merely an
> observation (to make sure I am reading the code correctly).

Yup, your understanding matches mine.

Patrick