On Tue, Nov 12, 2024 at 04:26:38PM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> 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@xxxxxx>
> > ---
> > 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
