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

[Junio C Hamano <gitster@pobox.com>]

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

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

> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
> index 54787f2ef5..ae14270ea7 100644
> --- a/reftable/reftable-stack.h
> +++ b/reftable/reftable-stack.h
> @@ -149,4 +149,7 @@ struct reftable_compaction_stats {
>  struct reftable_compaction_stats *
>  reftable_stack_compaction_stats(struct reftable_stack *st);
>  
> +/* Return the hash of the stack. */
> +enum reftable_hash reftable_stack_hash_id(struct reftable_stack *st);
> +
>  #endif
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 1fffd75630..d97b64a40d 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -1791,3 +1791,8 @@ int reftable_stack_clean(struct reftable_stack *st)
>  	reftable_addition_destroy(add);
>  	return err;
>  }
> +
> +enum reftable_hash reftable_stack_hash_id(struct reftable_stack *st)
> +{
> +	return reftable_merged_table_hash_id(st->merged);
> +}