Re: [PATCH 12/16] refs: store the main ref store inside the repository struct

[Stefan Beller]

Hi Michael,

On Tue, Apr 10, 2018 at 7:02 AM, Michael Haggerty  wrote:

> This also makes sense to me, as far as it goes. I have a few comments
> and questions:
>
> Why do you call the new member `main_ref_store`? Is there expected to be
> some other `ref_store` associated with a repository?

I'll rename it in a reroll.

>
> I think the origin of the name `main_ref_store` was to distinguish it
> from submodule ref stores. But presumably those will soon become the
> "main" ref stores for their respective submodule repository objects,
> right?

I hope so.

> So maybe calling things `repository.ref_store` and
> `get_ref_store(repository)` would be appropriate.

ok.

>
> There are some places in the reference code that only work with the main
> repository. The ones that I can think of are:
>
> * `ref_resolves_to_object()` depends on an object store.
>
> * `peel_object()` and `ref_iterator_peel()` also have to look up objects
> (in this case, tag objects).
>
> * Anything that calls `files_assert_main_repository()` or depends on
> `REF_STORE_MAIN` isn't implemented for other reference stores (usually,
> I think, these are functions that depend on the object store).
>
> Some of these things might be easy to generalize to non-main
> repositories, but I didn't bother because AFAIK currently only the main
> repository store is ever mutated.
>
> You can move a now-obsolete comment above the definition of `struct
> files_ref_store` if you haven't in some other patch (search for
> "libification").

ok, I'll have a look at that.

My plan was to remove the submodule accessors from the refs API, and
mandate the access via

repo_submodule_init(_repo, superproject_repo, path);
sub_ref_store = get_ref_store(submodule_repo);

instead of also having

sub_ref_store = get_submodule_ref_store(path);

as that would ease the refs API (and its internals potentially)
as well as avoiding errors with mixing up repositories. As the
construction of a submodule repository struct requires its
superproject repo, it helps avoiding pitfalls with nested
submodules IMO.

Thanks for the comments,
Stefan

>
> Hope that helps,
> Michael

Re: [PATCH 12/16] refs: store the main ref store inside the repository struct

[Michael Haggerty]

On 04/10/2018 12:45 AM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller 
> ---
>  refs.c   | 13 +
>  refs.h   |  4 +---
>  repository.h |  3 +++
>  3 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index f58b9fb7df..b5be754a97 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1608,9 +1608,6 @@ static struct ref_store_hash_entry 
> *alloc_ref_store_hash_entry(
>   return entry;
>  }
>  
> -/* A pointer to the ref_store for the main repository: */
> -static struct ref_store *main_ref_store;
> -
>  /* A hashmap of ref_stores, stored by submodule name: */
>  static struct hashmap submodule_ref_stores;
>  
> @@ -1652,13 +1649,13 @@ static struct ref_store *ref_store_init(const char 
> *gitdir,
>   return refs;
>  }
>  
> -struct ref_store *get_main_ref_store_the_repository(void)
> +struct ref_store *get_main_ref_store(struct repository *r)
>  {
> - if (main_ref_store)
> - return main_ref_store;
> + if (r->main_ref_store)
> + return r->main_ref_store;
>  
> - main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
> - return main_ref_store;
> + r->main_ref_store = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
> + return r->main_ref_store;
>  }
>  
>  /*
> diff --git a/refs.h b/refs.h
> index ab3d2bec2f..f5ab68c0ed 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -760,9 +760,7 @@ int reflog_expire(const char *refname, const struct 
> object_id *oid,
>  
>  int ref_storage_backend_exists(const char *name);
>  
> -#define get_main_ref_store(r) \
> - get_main_ref_store_##r()
> -struct ref_store *get_main_ref_store_the_repository(void);
> +struct ref_store *get_main_ref_store(struct repository *r);
>  /*
>   * Return the ref_store instance for the specified submodule. For the
>   * main repository, use submodule==NULL; such a call cannot fail. For
> diff --git a/repository.h b/repository.h
> index 09df94a472..7d0710b273 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -26,6 +26,9 @@ struct repository {
>*/
>   struct raw_object_store *objects;
>  
> + /* The store in which the refs are held. */
> + struct ref_store *main_ref_store;
> +
>   /*
>* Path to the repository's graft file.
>* Cannot be NULL after initialization.
> 

This also makes sense to me, as far as it goes. I have a few comments
and questions:

Why do you call the new member `main_ref_store`? Is there expected to be
some other `ref_store` associated with a repository?

I think the origin of the name `main_ref_store` was to distinguish it
from submodule ref stores. But presumably those will soon become the
"main" ref stores for their respective submodule repository objects,
right? So maybe calling things `repository.ref_store` and
`get_ref_store(repository)` would be appropriate.

There are some places in the reference code that only work with the main
repository. The ones that I can think of are:

* `ref_resolves_to_object()` depends on an object store.

* `peel_object()` and `ref_iterator_peel()` also have to look up objects
(in this case, tag objects).

* Anything that calls `files_assert_main_repository()` or depends on
`REF_STORE_MAIN` isn't implemented for other reference stores (usually,
I think, these are functions that depend on the object store).

Some of these things might be easy to generalize to non-main
repositories, but I didn't bother because AFAIK currently only the main
repository store is ever mutated.

You can move a now-obsolete comment above the definition of `struct
files_ref_store` if you haven't in some other patch (search for
"libification").

Hope that helps,
Michael

Re: [PATCH 12/16] refs: store the main ref store inside the repository struct

[Brandon Williams]

On 04/09, Stefan Beller wrote:
> Hi Brandon,
> 
> On Mon, Apr 9, 2018 at 4:24 PM, Brandon Williams  wrote:
> 
> >> - main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
> >> - return main_ref_store;
> >> + r->main_ref_store = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
> >> + return r->main_ref_store;
> >
> > I assume that since this takes in a git dir as a parameter
> > that the ref-store is in a good enough place to be embedded in a
> > repository struct (as in this would work with an arbitrary repo)?
> 
> That is my current understanding.
> 
> As the refs code can also take a path into a submodule and construct
> a submodule ref store for the caller, we'd want to resolve the tension
> between the ref store and the repository struct who is responsible for
> the submodule ref store eventually by removing the submodule
> functionality from the ref store and only relying on the ref stores created by
> the repository struct.

oh right, assuming it can already handle submodule ref-stores it should
work as-is then.

-- 
Brandon Williams

Re: [PATCH 12/16] refs: store the main ref store inside the repository struct

[Stefan Beller]

Hi Brandon,

On Mon, Apr 9, 2018 at 4:24 PM, Brandon Williams  wrote:

>> - main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
>> - return main_ref_store;
>> + r->main_ref_store = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
>> + return r->main_ref_store;
>
> I assume that since this takes in a git dir as a parameter
> that the ref-store is in a good enough place to be embedded in a
> repository struct (as in this would work with an arbitrary repo)?

That is my current understanding.

As the refs code can also take a path into a submodule and construct
a submodule ref store for the caller, we'd want to resolve the tension
between the ref store and the repository struct who is responsible for
the submodule ref store eventually by removing the submodule
functionality from the ref store and only relying on the ref stores created by
the repository struct.

Re: [PATCH 12/16] refs: store the main ref store inside the repository struct

[Brandon Williams]

On 04/09, Stefan Beller wrote:
> Signed-off-by: Stefan Beller 
> ---
>  refs.c   | 13 +
>  refs.h   |  4 +---
>  repository.h |  3 +++
>  3 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index f58b9fb7df..b5be754a97 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1608,9 +1608,6 @@ static struct ref_store_hash_entry 
> *alloc_ref_store_hash_entry(
>   return entry;
>  }
>  
> -/* A pointer to the ref_store for the main repository: */
> -static struct ref_store *main_ref_store;
> -
>  /* A hashmap of ref_stores, stored by submodule name: */
>  static struct hashmap submodule_ref_stores;
>  
> @@ -1652,13 +1649,13 @@ static struct ref_store *ref_store_init(const char 
> *gitdir,
>   return refs;
>  }
>  
> -struct ref_store *get_main_ref_store_the_repository(void)
> +struct ref_store *get_main_ref_store(struct repository *r)
>  {
> - if (main_ref_store)
> - return main_ref_store;
> + if (r->main_ref_store)
> + return r->main_ref_store;
>  
> - main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
> - return main_ref_store;
> + r->main_ref_store = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
> + return r->main_ref_store;

I assume that since this takes in a git dir as a parameter
that the ref-store is in a good enough place to be embedded in a
repository struct (as in this would work with an arbitrary repo)?

>  }
>  
>  /*
> diff --git a/refs.h b/refs.h
> index ab3d2bec2f..f5ab68c0ed 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -760,9 +760,7 @@ int reflog_expire(const char *refname, const struct 
> object_id *oid,
>  
>  int ref_storage_backend_exists(const char *name);
>  
> -#define get_main_ref_store(r) \
> - get_main_ref_store_##r()
> -struct ref_store *get_main_ref_store_the_repository(void);
> +struct ref_store *get_main_ref_store(struct repository *r);
>  /*
>   * Return the ref_store instance for the specified submodule. For the
>   * main repository, use submodule==NULL; such a call cannot fail. For
> diff --git a/repository.h b/repository.h
> index 09df94a472..7d0710b273 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -26,6 +26,9 @@ struct repository {
>*/
>   struct raw_object_store *objects;
>  
> + /* The store in which the refs are held. */
> + struct ref_store *main_ref_store;
> +
>   /*
>* Path to the repository's graft file.
>* Cannot be NULL after initialization.
> -- 
> 2.17.0.484.g0c8726318c-goog
> 

-- 
Brandon Williams

[PATCH 12/16] refs: store the main ref store inside the repository struct

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 refs.c   | 13 +
 refs.h   |  4 +---
 repository.h |  3 +++
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index f58b9fb7df..b5be754a97 100644
--- a/refs.c
+++ b/refs.c
@@ -1608,9 +1608,6 @@ static struct ref_store_hash_entry 
*alloc_ref_store_hash_entry(
return entry;
 }
 
-/* A pointer to the ref_store for the main repository: */
-static struct ref_store *main_ref_store;
-
 /* A hashmap of ref_stores, stored by submodule name: */
 static struct hashmap submodule_ref_stores;
 
@@ -1652,13 +1649,13 @@ static struct ref_store *ref_store_init(const char 
*gitdir,
return refs;
 }
 
-struct ref_store *get_main_ref_store_the_repository(void)
+struct ref_store *get_main_ref_store(struct repository *r)
 {
-   if (main_ref_store)
-   return main_ref_store;
+   if (r->main_ref_store)
+   return r->main_ref_store;
 
-   main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
-   return main_ref_store;
+   r->main_ref_store = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
+   return r->main_ref_store;
 }
 
 /*
diff --git a/refs.h b/refs.h
index ab3d2bec2f..f5ab68c0ed 100644
--- a/refs.h
+++ b/refs.h
@@ -760,9 +760,7 @@ int reflog_expire(const char *refname, const struct 
object_id *oid,
 
 int ref_storage_backend_exists(const char *name);
 
-#define get_main_ref_store(r) \
-   get_main_ref_store_##r()
-struct ref_store *get_main_ref_store_the_repository(void);
+struct ref_store *get_main_ref_store(struct repository *r);
 /*
  * Return the ref_store instance for the specified submodule. For the
  * main repository, use submodule==NULL; such a call cannot fail. For
diff --git a/repository.h b/repository.h
index 09df94a472..7d0710b273 100644
--- a/repository.h
+++ b/repository.h
@@ -26,6 +26,9 @@ struct repository {
 */
struct raw_object_store *objects;
 
+   /* The store in which the refs are held. */
+   struct ref_store *main_ref_store;
+
/*
 * Path to the repository's graft file.
 * Cannot be NULL after initialization.
-- 
2.17.0.484.g0c8726318c-goog