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

[Jonathan Tan <jonathantanmy@google.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.

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