Re: [PATCH 059/194] refs: store the main ref store inside the repository struct

[Stefan Beller]

On Mon, Feb 5, 2018 at 8:27 PM, Eric Sunshine  wrote:
> On Mon, Feb 5, 2018 at 6:55 PM, Stefan Beller  wrote:
>> diff --git a/refs.c b/refs.c
>> @@ -1609,9 +1609,6 @@ static struct ref_store_hash_entry 
>> *alloc_ref_store_hash_entry(
>> -/* A pointer to the ref_store for the main repository: */
>> -static struct ref_store *main_ref_store;
>> diff --git a/repository.h b/repository.h
>> @@ -33,6 +33,11 @@ struct repository {
>>  */
>> struct object_store objects;
>>
>> +   /*
>> +* The store in which the refs are hold.
>> +*/
>> +   struct ref_store *main_ref_store;
>
> Do items moved to the 'repository' structure need to be freed when the
> 'repository' itself is freed? Is that being done by a different patch?
> If so, it would ease review burden for the freeing to happen in the
> same patch in which the item is moved to the 'repository'.

There are two cases:

* the_repository
  In the_repository we'll mostly have the globals as they exist
  (the_index, the_hash_algo, the_ref_store) and just as before
  these globals will not be freed at the end of the program.
* arbitrary repos:
  For arbitrary repos, we usually need to allocate memory in repo_init
  and clear it out in repo_free/clear

This patch is incomplete and is leaking the main ref store for arbitrary repos.

Thanks for spotting the mem leak!
Stefan

Re: [PATCH 059/194] refs: store the main ref store inside the repository struct

[Eric Sunshine]

On Mon, Feb 5, 2018 at 6:55 PM, Stefan Beller  wrote:
> diff --git a/refs.c b/refs.c
> @@ -1609,9 +1609,6 @@ static struct ref_store_hash_entry 
> *alloc_ref_store_hash_entry(
> -/* A pointer to the ref_store for the main repository: */
> -static struct ref_store *main_ref_store;
> diff --git a/repository.h b/repository.h
> @@ -33,6 +33,11 @@ struct repository {
>  */
> struct object_store objects;
>
> +   /*
> +* The store in which the refs are hold.
> +*/
> +   struct ref_store *main_ref_store;

Do items moved to the 'repository' structure need to be freed when the
'repository' itself is freed? Is that being done by a different patch?
If so, it would ease review burden for the freeing to happen in the
same patch in which the item is moved to the 'repository'.

[PATCH 059/194] refs: store the main ref store inside the repository struct

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 refs.c   | 13 +
 refs.h   |  4 +---
 repository.c |  2 +-
 repository.h |  5 +
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index c2dee7a69a..f079d65030 100644
--- a/refs.c
+++ b/refs.c
@@ -1609,9 +1609,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;
 
@@ -1653,13 +1650,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.c b/repository.c
index 0ec9648f53..6f85fade82 100644
--- a/repository.c
+++ b/repository.c
@@ -6,7 +6,7 @@
 
 /* The main repository */
 static struct repository the_repo = {
-   NULL, NULL, NULL, OBJECT_STORE_INIT,
+   NULL, NULL, NULL, OBJECT_STORE_INIT, NULL,
NULL, NULL, NULL, NULL, NULL, NULL, _index, NULL, 0, 0
 };
 struct repository *the_repository = _repo;
diff --git a/repository.h b/repository.h
index ba7b3b7cb9..727ddcea5b 100644
--- a/repository.h
+++ b/repository.h
@@ -33,6 +33,11 @@ struct repository {
 */
struct object_store objects;
 
+   /*
+* The store in which the refs are hold.
+*/
+   struct ref_store *main_ref_store;
+
/*
 * Path to the repository's graft file.
 * Cannot be NULL after initialization.
-- 
2.15.1.433.g936d1b9894.dirty