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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 29, 2021, 17:04 UTC
Message-ID: <20210929170415.1236650-1-jonathantanmy@google.com>
In-Reply-To: <xmqqlf3gib0p.fsf@gitster.g>

Show 8 quoted lines

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

Show 6 quoted lines

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

Show 11 quoted lines

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