Re: [PATCH v2 2/9] refs: teach arbitrary repo support to iterators

[Jonathan Tan <jonathantanmy@google.com>]

> The two steps so far seems to give the necessary information to code
> paths that want them, so it is not wrong per-se, but this makes me
> wonder a few things.
> 
>  - There may be multiple ref backends and iterators corresponding to
>    them.  Is it reasonable to assume that there are backends that do
>    not need "repo"?  Otherwise, shouldn't this be added to the base
>    class "struct ref_iterator base"?

All backends need repos, but not all iterators need backends - there is
a merge_ref_iterator and a prefix_ref_iterator, for example.

>  - The iterator_begin() and other functions have been taught to take
>    the repository in addition to the ref_store in the previous step,
>    but
> 
>    . Doesn't iterator iterate over a single ref_store?  Shouldn't it
>      have a pointer to the ref_store it is iterating over?

No - as above, merge_ref_iterator, for example, does not iterate over a
ref_store but combines the results of 2 other iterators.

>    . Doesn't a ref_store belong to a single repository?  Shouldn't
>      it have a pointer to the repository it is part of?
> 
>    If the answers to both are 'yes', then we wouldn't need to add a
>    repository pointer as a new parameter to functions that already
>    took a ref store.
> 
> In other words, I am wondering if the right pieces of information
> are stored in the right structure.
> 
> Thanks.

A ref_store does belong to a single repository. The reason why it
doesn't have a pointer to that repository is probably because struct
ref_store (00eebe351c ("refs: create a base class "ref_store" for
files_ref_store", 2016-09-09)) predates struct repository (359efeffc1
("repository: introduce the repository object", 2017-06-23)). I've been
avoiding introducing a pointer to the repository in struct ref_store to
avoid unnecessary coupling, but it is looking more and more necessary,
as you mention in your reply [1] to another patch about how this would
eliminate certain other "user" codepaths needing to know about the repo.
I'll take a look at introducing a pointer to the repo in struct
ref_store and report back with my findings.

[1] https://lore.kernel.org/git/xmqqh7e4iacw.fsf@gitster.g/