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