Re: [PATCH 12/16] refs: store the main ref store inside the repository struct

From: Stefan Beller <sbeller@google.com>
Date: Apr 10, 2018, 18:38 UTC
Message-ID: <CAGZ79kZEOzAOqYePMxHMZChuoO3ryy8xY41KCPTp=HT-3TRVPw@mail.gmail.com>
In-Reply-To: <2c6214de-36b0-c67b-9a40-d43d28e780c4@alum.mit.edu>

Hi Michael,

On Tue, Apr 10, 2018 at 7:02 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Show 5 quoted lines

> This also makes sense to me, as far as it goes. I have a few comments
> and questions:
>
> Why do you call the new member `main_ref_store`? Is there expected to be
> some other `ref_store` associated with a repository?

I'll rename it in a reroll.

Show 5 quoted lines

>
> I think the origin of the name `main_ref_store` was to distinguish it
> from submodule ref stores. But presumably those will soon become the
> "main" ref stores for their respective submodule repository objects,
> right?

I hope so.

> So maybe calling things `repository.ref_store` and
> `get_ref_store(repository)` would be appropriate.

ok.

Show 20 quoted lines

>
> There are some places in the reference code that only work with the main
> repository. The ones that I can think of are:
>
> * `ref_resolves_to_object()` depends on an object store.
>
> * `peel_object()` and `ref_iterator_peel()` also have to look up objects
> (in this case, tag objects).
>
> * Anything that calls `files_assert_main_repository()` or depends on
> `REF_STORE_MAIN` isn't implemented for other reference stores (usually,
> I think, these are functions that depend on the object store).
>
> Some of these things might be easy to generalize to non-main
> repositories, but I didn't bother because AFAIK currently only the main
> repository store is ever mutated.
>
> You can move a now-obsolete comment above the definition of `struct
> files_ref_store` if you haven't in some other patch (search for
> "libification").

ok, I'll have a look at that.

My plan was to remove the submodule accessors from the refs API, and mandate the access via

    repo_submodule_init(&submodule_repo, superproject_repo, path);
    sub_ref_store = get_ref_store(submodule_repo);

instead of also having

    sub_ref_store = get_submodule_ref_store(path);

as that would ease the refs API (and its internals potentially) as well as avoiding errors with mixing up repositories. As the construction of a submodule repository struct requires its superproject repo, it helps avoiding pitfalls with nested submodules IMO.

Thanks for the comments, Stefan

>
> Hope that helps,
> Michael