Thread (9 messages) 9 messages, 3 authors, 2021-09-16

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

From: Jonathan Tan <hidden>
Date: 2021-08-26 22:24:45

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.
quoted
- 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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help