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

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Dec 3, 2020, 20:23 UTC
Message-ID: <CAFQ2z_M3OO_nR6dhw6zzE0orYxcawP1DaJ_EOL5=+RUiZgCo8w@mail.gmail.com>
In-Reply-To: <xmqq5z5mruia.fsf@gitster.c.googlers.com>

On Tue, Dec 1, 2020 at 6:00 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > The reftable backend needs to know the hash algorithm for writing the
> > initialization hash table.
> >
> > The initial reftable contains a symref HEAD => "main" (or "master"), which is
> > agnostic to the size of hash value, but this is an exceptional circumstance, and
> > the reftable library does not cater for this exception. It insists that all
> > tables in the stack have a consistent format ID for the hash algorithm.
> >
> > Call set_repo_hash_algo directly after reading out GIT_DEFAULT_HASH.
>
> Seeing that there is no mention of the_repository in the entire
> init-db.c file,

it's not because it's hidden in the call to refs_init_db(). refs_init_db accesses the_repository global variable, because get_main_ref_store() takes a repository (and not repository_format) argument.

> Isn't it the matter of passing the hash
> taken from repo_fmt to the refs API to initialize that part of the
> repository, instead of relying on half-initialized state in
> the_repository?

The refstore is created in get_main_ref_store(). I can't add a repository_format argument there, because it has a lot of call sites where the repository_format isn't available.

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?

Show 7 quoted lines

> It's not like only the hash_algo member is yet to
> be prepared in the_repository instance at that point in the code.
> Most of the members are, except for a very few fields initialized by
> initialize_the_repository(), not filled in the codepath, no?
>
> So, this might have been the most convenient way to pass hash_algo
> down but the patch does not convince me that it is the best way.

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.

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