Re: [PATCH v2 3/8] refs/reftable: read references via `struct reftable_backend`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 12, 2024, 07:26 UTC
- Message-ID
- <xmqqed3g6hg1.fsf@gitster.g>
- In-Reply-To
- <1b50655202f311c6a6ded61d4d50b1f287761d84.1730792627.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 41 quoted lines
> 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.
Show 22 quoted lines
> + 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 ...
Show 21 quoted lines
> -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.
Show 10 quoted lines
> - } 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).
Show 25 quoted lines
> 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);
> +}