Re: [PATCH 4/4] packed refs: pass .git dir instead of packed-refs path to init_fn
On 09/12/2017 07:32 PM, Jonathan Nieder wrote:
Show 53 quoted lines
> From: Stefan Beller <sbeller@google.com>
>
> 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@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 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
Jonathan Nieder
