Re: [PATCH] init-db: init the_repository->hash_algo early from GIT_DEFAULT_HASH

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Dec 7, 2020, 11:52 UTC
Message-ID: <CAFQ2z_OsyseuPWxpj9S4R_Pp_Koa0JuJeO2gte+TLxDh+oHPNg@mail.gmail.com>
In-Reply-To: <xmqqtut2fp5z.fsf@gitster.c.googlers.com>

On Fri, Dec 4, 2020 at 12:25 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> > If the_repository is only half-initialized at this point in init_db(),
> > then why are we passing it in refs_init_db() just a couple of lines
> > further? At what point the_repository considered initialized?
>
> I would have to say it probably depends on what callees expect.  The
> current implementation of refs_init_db() for files backend may not
> need anything other than the hash algorithm enum, but many other
> fields are missing, and they should ideally be populated, no?

I think so too, but it looks like a major refactoring by itself, which is only very tangentially related to the reftable effort, so I'd like to punt on it.

Show 19 quoted lines

> For example, I see files_ref_store_create() cheats by calling
> get_common_dir_noenv() to find out where the commondir is, instead
> of ever asking the repository the ref store belongs to. At least,
> get_main_ref_store() is told to get the ref store that belongs to
> the_repository, and it would be the right place to learn relevant
> pieces of information (for that matter, I am not sure why struct
> ref_store does not have a pointer to a repository structure; perhaps
> we are seeing the result of piecemeal evolution, not a designed
> structure?).
>
> > I'm a bit at a loss here; I never learned how to cleanly work with so
> > many global variables, so I'm happy to take your suggestion.
>
> I am only interested in giving a clear direction to future
> developers where to populate the_repository's members (and nothing
> else) if their enhancement needs members other than the hash
> algorithm to be populated, as if it were the_repository initialized
> in an already working repository (I am not talking about many global
> variables, whichever you are referring to).

You said earlier that maybe ref_store should hold a link to 'struct repository'. However, "struct repository" doesn't have a documented purpose (there is literally no comment documenting its purpose), so it's hard to tell upfront how to correctly configure the relation between "struct ref_store" and "struct repository". Currently, "struct repository" has a pointer to 'struct ref_store", which makes it suspect for there to be a pointer in the other direction as well.

The reason I bring up global variables is that without them, the code would enforce a much more clear relation between different entities.

Looking at the definition of repository, there seems to be some overlap in functionality between struct repository, struct repository_format, struct repo_settings, and config_set, and none of these structs have a clearly documented role. Before we change code, it's probably better to spell out what these structs are thought to be.

> One way to do so would probably be to do something like the
> attached.

Thanks, I've amended my patch.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado