Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Sep 15, 2021, 07:35 UTC
Message-ID: <CAFQ2z_ML_g6DTiG92srq8UCCu_D8bi6z1mP_znt24TJagVfi2g@mail.gmail.com>
In-Reply-To: <YUElL3RI0VTnjE5C@chooglen-macbookpro.roam.corp.google.com>

On Wed, Sep 15, 2021 at 12:41 AM Glen Choo <chooglen@google.com> wrote:

Show 7 quoted lines

> In the current state of affairs, the files ref store and the packed ref
> store seem to behave as a single logical ref database. An example of
> this (that I care about in particular) is in refs/files-backend.c where
> the files backend validates oids using the_repository's odb.
> refs/packed-backend.c doesn't do any such validation, and presumably
> just relies on the correctness of refs/files-backend.c. I assume that
> this also explains why some functions in refs_be_packed are stubs.

The loose/packed storage is implemented in terms of files backend (the public entry point) that defers to a packed backend in some cases. The latter is implemented as a ref backend, but for no good reason.

Show 10 quoted lines

> The answer to whether or not a ref store should refer to a certain
> object store seems unresolved because a ref store is trying to do two
> separate things. Perhaps it is reasonable to associate a ref database
> with an object store (so that it can validate its refs), but we would
> prefer to dissociate the physical ref storage layer from the object
> store. (I'm paraphrasing Johnathan Nieder here, this isn't an original
> thought).
>
> Perhaps this is a question we want to resolve when considering reftable
> and other ref databases.

Work on reftable shows that there are more egregious breaks of abstraction boundaries. For example, there are still parts of the code that equate

  (file under .git/ == ref)

you can find a good part of them if you run GIT_TEST_REFTABLE=1 with the reftable support switched on. Another place where API contracts are unclear is resolving symrefs: on first sight, you'd think that a ref backend should just provide storage for a refname => {symref, commit SHA-1, tag + commit SHA-1} mapping. However, in some places it is currently necessary for the ref backend to resolve symrefs. You can find these places by grepping for refs_resolve_ref_unsafe() in the files backend.

I think Jonathan is right, but I also think that teasing apart the ref backend and the ODB is premature until the ref backend itself is a strongly enforced abstraction boundary.

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