On 09/12/2017 07:32 PM, Jonathan Nieder wrote:
> From: Stefan Beller <sbeller@xxxxxxxxxx>
>
> The first argument of a ref_store_init_fn is documented to represent
> the $GIT_DIR, not the path to the packed-refs file. This brings the
> packed-refs store more in line with the usual ref store interface.
>
> Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> ---
> That's the end of the series. Thanks for reading.
>
> refs/files-backend.c | 4 ++--
> refs/packed-backend.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index fccbc24ac4..3b8e13a8b7 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -57,8 +57,8 @@ static struct ref_store *files_ref_store_create(const char *gitdir,
> refs->gitdir = xstrdup(gitdir);
> get_common_dir_noenv(&sb, gitdir);
> refs->gitcommondir = strbuf_detach(&sb, NULL);
> - strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
> - refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
> + refs->packed_ref_store =
> + packed_ref_store_create(refs->gitcommondir, flags);
> strbuf_release(&sb);
>
> return ref_store;
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 412c85034f..2c5db15279 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -78,7 +78,7 @@ struct packed_ref_store {
> struct tempfile tempfile;
> };
>
> -struct ref_store *packed_ref_store_create(const char *path,
> +struct ref_store *packed_ref_store_create(const char *common_dir,
> unsigned int store_flags)
> {
> struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
> @@ -87,7 +87,7 @@ struct ref_store *packed_ref_store_create(const char *path,
> base_ref_store_init(ref_store, &refs_be_packed);
> refs->store_flags = store_flags;
>
> - refs->path = xstrdup(path);
> + refs->path = xstrfmt("%s/packed-refs", common_dir);
> return ref_store;
> }
>
>
This little patch, perhaps surprisingly, brings up some deeper issues.
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.
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.)
Since a `packed_ref_store` is definitely a physical `ref_store`, the
function that we're talking about here is the second type, so I don't
see a need for it to take a gitdir as argument. OTOH, the function
certainly doesn't belong in the vtable slot for `init`, because a
`packed_ref_store` can't serve as a repository's logical reference store.
Did you have a particular reason for changing this now, aside from
"consistency"? If not, feel free to drop this patch and I'll implement
something more like what I have described above when I have time.
Michael
