Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 26, 2021, 22:24 UTC
Message-ID: <20210826222439.3915402-1-jonathantanmy@google.com>
In-Reply-To: <CAFQ2z_PKKZJY1kC1QJo8Zwq_yNh5QNGc3S5bq1jBfSfK3vQwRQ@mail.gmail.com>

> from a design perspective, it would be nice if the ref backend
> wouldn't need to know about the object store. Can't this be hidden in
> the layer in refs.c that calls into the backends?

Thanks for taking a look.

The answer requires additional context, so I'll answer this at the end of this email.

> If they have to know about the object store, have you considered
> passing the repository pointer
> in xxx_ref_store_create() ? Then there is no possibliity to mismatch
> the repository pointers and with the ref store.

I thought about that, but didn't want to make things worse - the effort in this patch set is, after all, to attempt to increase the dissociation between the ref stores and a certain object store (that is, the_repository's object store), and I thought that reintroducing an association (albeit to arbitrary object stores instead of a hardcoded object store) would be a step back.

But this may be the way to go - the ref stores already have a gitdir field that we could replace with a struct repository field.

Show 10 quoted lines

> > - Making all ref stores not access the object store during their
> >   _advance() callbacks, and making ref_iterator_advance() be responsible
> >   for checking the object store - thus, simplifying the code in that the
> >   logic of checking for the flag (current) or the pointer (after the
> >   equivalent of this commit) is only in one place instead of in every
> >   ref store's callback. However, the ref stores already make use of this
> >   flag for another reason - for determining if refs are resolvable when
> >   writing (search for "REF_STORE_ODB"). Thus, I decided to retain each
> 
> I looked, but I couldn't figure out how this flag is used.

I was thinking of files_ref_iterator_begin() setting a local variable required_flags. Somehow I thought that files_pack_refs() relied on files_ref_iterator_begin() setting that variable, but now I see that that's not true - both functions are independently checking that the underlying ref store supports ODB access, so I can remove ODB from files_ref_iterator_begin() if I want to.

To go back to the question at the top, now I agree that hiding the ODB access in _advance() in the layer in refs.c is possible. The last part still accessing the ODB is files_pack_refs(), I think. Refactoring that is possible, but I'll leave that to another patch set.

If you or anyone else has more questions or comments, please reply - and in the meantime, I'll update this patch set to move the ODB access in _advance() to the layer in refs.c.