On Wed, Sep 13, 2017 at 10:45:45AM +0200, Michael Haggerty wrote: > First of all there is a superficial naming inconsistency. This method is > called `init` in the vtable, but the functions implementing it are > called `*_ref_store_create()`. It actually initializes the data > structures for dealing with the reference store, as opposed to > initializing the reference store on disk (*that* virtual function is > called `init_db`). It should maybe be called `open` instead. > > But more fundamentally, what is a `ref_store`? Originally it always > represented a *logical* place to store all of the references for a > repository. In that sense, it made sense to have an "open" method that > takes a `gitdir` as argument. > > But its role is currently getting stretched. Now it sometimes represents > a *physical* place to store references, which might be combined with > other physical `ref_store`s to make a logical `ref_store`. There is not > necessarily a 1:1 correspondence between gitdirs and physical > `ref_store`s; more to the point you might want to initialize a physical > refstore that doesn't correspond to `$GITDIR`. So requiring every > `ref_store` to have a factory function with a gitdir argument is > probably incorrect. > > For example, a subtree has a single logical reference store, but it is > composed out of three physical reference stores: the loose references in > the subtree's gitdir plus loose and packed references in the common gitdir. One way to implement a namespacing backend would include a loose references backend of the subtree of refs for the namespace overlayed with a namespace translating backend backed onto packed references, so there's benefits to not being strict about it being a gitdir. > It seems to me that we need two separate concepts: > > 1. Create an object representing a gitdir's *logical* `ref_store`. This > "class method" could always have a single gitdir as parameter. This > would be the method by which the repository initialization code > instantiates its logical `ref_store`, for example by reading the type of > the reference store from the repo's config, then looking up the class by > its type, then calling its "open" method to create an instance. > > 2. Create an object representing a physical `ref_store`. Different > reference store classes might have different "constructor" arguments. > For example, if it represents a namespace within a larger reference tree > contained in a shared repository, its arguments might be > `(shared_gitdir, namespace)`. (CC to Richard Maw for this angle.) The distinction confused me while attempting to add the namespaced ref backend, since I was unfamiliar with how the ref stores were meant to be defined. In the end I just omitted the init function and create them separately. I'm not sure drawing the line at "logical ref stores init" and "physical ref stores are constructed differently" is the right division, since the namespaced ref store is more of a local ref store and its current interface is an existing ref store and a namespace. Perhaps a better distinction would be frontend vs implementation ref stores where frontend ones are registered to be found with find_ref_storage_backend, while implementation ref stores don't use that and hence don't need the next, name, init or init_db. Then the physical vs logical distinction is whether they access refs directly or only via another backend. It wouldn't be implausible for the namespaced ref store backend to be a frontend logical ref store that constructs the files ref store itself; I just didn't have enough of a feel of how things went together to spot that.
