Re: [PATCH 02/26] object-store: move alt_odb_list and alt_odb_tail to object store

[Stefan Beller]

On Tue, Feb 13, 2018 at 10:51 AM, Brandon Williams  wrote:

>> +#include "cache.h"
>> +
>
> Maybe we would want to move the definition of the alternate store to
> this header file?  That way we don't need to include cache.h here.
>

Sounds good. I'll take a look at that.

Stefan

Re: [PATCH 02/26] object-store: move alt_odb_list and alt_odb_tail to object store

[Brandon Williams]

On 02/12, Stefan Beller wrote:
> In a process with multiple repositories open, alternates should be
> associated to a single repository and not shared globally. Move
> alt_odb_list and alt_odb_tail into the_repository and adjust callers
> to reflect this.
> 
> Now that the alternative object data base is per repository, we're
> leaking its memory upon freeing a repository. The next patch plugs
> this hole.
> 
> No functional change intended.
> 
> Signed-off-by: Stefan Beller 
> Signed-off-by: Jonathan Nieder 
> ---
>  builtin/fsck.c |  4 +++-
>  cache.h|  4 ++--
>  object-store.h |  7 ++-
>  packfile.c |  3 ++-
>  sha1_file.c| 25 -
>  sha1_name.c|  3 ++-
>  6 files changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 04846d46f9..1048255da1 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -1,5 +1,6 @@
>  #include "builtin.h"
>  #include "cache.h"
> +#include "repository.h"
>  #include "config.h"
>  #include "commit.h"
>  #include "tree.h"
> @@ -694,7 +695,8 @@ int cmd_fsck(int argc, const char **argv, const char 
> *prefix)
>   fsck_object_dir(get_object_directory());
>  
>   prepare_alt_odb();
> - for (alt = alt_odb_list; alt; alt = alt->next)
> + for (alt = the_repository->objects.alt_odb_list;
> + alt; alt = alt->next)
>   fsck_object_dir(alt->path);
>  
>   if (check_full) {
> diff --git a/cache.h b/cache.h
> index d8b975a571..918c2f15b4 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1573,7 +1573,7 @@ extern int has_dirs_only_path(const char *name, int 
> len, int prefix_len);
>  extern void schedule_dir_for_removal(const char *name, int len);
>  extern void remove_scheduled_dirs(void);
>  
> -extern struct alternate_object_database {
> +struct alternate_object_database {
>   struct alternate_object_database *next;
>  
>   /* see alt_scratch_buf() */
> @@ -1591,7 +1591,7 @@ extern struct alternate_object_database {
>   struct oid_array loose_objects_cache;
>  
>   char path[FLEX_ARRAY];
> -} *alt_odb_list;
> +};
>  extern void prepare_alt_odb(void);
>  extern char *compute_alternate_path(const char *path, struct strbuf *err);
>  typedef int alt_odb_fn(struct alternate_object_database *, void *);
> diff --git a/object-store.h b/object-store.h
> index cf35760ceb..44b8f84753 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -1,14 +1,19 @@
>  #ifndef OBJECT_STORE_H
>  #define OBJECT_STORE_H
>  
> +#include "cache.h"
> +

Maybe we would want to move the definition of the alternate store to
this header file?  That way we don't need to include cache.h here.

>  struct raw_object_store {
>   /*
>* Path to the repository's object store.
>* Cannot be NULL after initialization.
>*/
>   char *objectdir;
> +
> + struct alternate_object_database *alt_odb_list;
> + struct alternate_object_database **alt_odb_tail;
>  };
> -#define RAW_OBJECT_STORE_INIT { NULL }
> +#define RAW_OBJECT_STORE_INIT { NULL, NULL, NULL }
>  
>  void raw_object_store_clear(struct raw_object_store *o);
>  
> diff --git a/packfile.c b/packfile.c
> index 4a5fe7ab18..d61076faaf 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1,6 +1,7 @@
>  #include "cache.h"
>  #include "mru.h"
>  #include "pack.h"
> +#include "repository.h"
>  #include "dir.h"
>  #include "mergesort.h"
>  #include "packfile.h"
> @@ -880,7 +881,7 @@ void prepare_packed_git(void)
>   return;
>   prepare_packed_git_one(get_object_directory(), 1);
>   prepare_alt_odb();
> - for (alt = alt_odb_list; alt; alt = alt->next)
> + for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next)
>   prepare_packed_git_one(alt->path, 0);
>   rearrange_packed_git();
>   prepare_packed_git_mru();
> diff --git a/sha1_file.c b/sha1_file.c
> index 3da70ac650..2826d5d6ed 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -22,6 +22,7 @@
>  #include "pack-revindex.h"
>  #include "sha1-lookup.h"
>  #include "bulk-checkin.h"
> +#include "repository.h"
>  #include "streaming.h"
>  #include "dir.h"
>  #include "mru.h"
> @@ -346,9 +347,6 @@ static const char *alt_sha1_path(struct 
> alternate_object_database *alt,
>   return buf->buf;
>  }
>  
> -struct alternate_object_database *alt_odb_list;
> -static struct alternate_object_database **alt_odb_tail;
> -
>  /*
>   * Return non-zero iff the path is usable as an alternate object database.
>   */
> @@ -368,7 +366,7 @@ static int alt_odb_usable(struct strbuf *path, const char 
> *normalized_objdir)
>* Prevent the common mistake of listing the same
>* thing twice, or object directory itself.
>*/
> - for (alt = alt_odb_list; alt; alt = alt->next) {
> + for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
>   if 

[PATCH 02/26] object-store: move alt_odb_list and alt_odb_tail to object store

[Stefan Beller]

In a process with multiple repositories open, alternates should be
associated to a single repository and not shared globally. Move
alt_odb_list and alt_odb_tail into the_repository and adjust callers
to reflect this.

Now that the alternative object data base is per repository, we're
leaking its memory upon freeing a repository. The next patch plugs
this hole.

No functional change intended.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 builtin/fsck.c |  4 +++-
 cache.h|  4 ++--
 object-store.h |  7 ++-
 packfile.c |  3 ++-
 sha1_file.c| 25 -
 sha1_name.c|  3 ++-
 6 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 04846d46f9..1048255da1 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "repository.h"
 #include "config.h"
 #include "commit.h"
 #include "tree.h"
@@ -694,7 +695,8 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
fsck_object_dir(get_object_directory());
 
prepare_alt_odb();
-   for (alt = alt_odb_list; alt; alt = alt->next)
+   for (alt = the_repository->objects.alt_odb_list;
+   alt; alt = alt->next)
fsck_object_dir(alt->path);
 
if (check_full) {
diff --git a/cache.h b/cache.h
index d8b975a571..918c2f15b4 100644
--- a/cache.h
+++ b/cache.h
@@ -1573,7 +1573,7 @@ extern int has_dirs_only_path(const char *name, int len, 
int prefix_len);
 extern void schedule_dir_for_removal(const char *name, int len);
 extern void remove_scheduled_dirs(void);
 
-extern struct alternate_object_database {
+struct alternate_object_database {
struct alternate_object_database *next;
 
/* see alt_scratch_buf() */
@@ -1591,7 +1591,7 @@ extern struct alternate_object_database {
struct oid_array loose_objects_cache;
 
char path[FLEX_ARRAY];
-} *alt_odb_list;
+};
 extern void prepare_alt_odb(void);
 extern char *compute_alternate_path(const char *path, struct strbuf *err);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
diff --git a/object-store.h b/object-store.h
index cf35760ceb..44b8f84753 100644
--- a/object-store.h
+++ b/object-store.h
@@ -1,14 +1,19 @@
 #ifndef OBJECT_STORE_H
 #define OBJECT_STORE_H
 
+#include "cache.h"
+
 struct raw_object_store {
/*
 * Path to the repository's object store.
 * Cannot be NULL after initialization.
 */
char *objectdir;
+
+   struct alternate_object_database *alt_odb_list;
+   struct alternate_object_database **alt_odb_tail;
 };
-#define RAW_OBJECT_STORE_INIT { NULL }
+#define RAW_OBJECT_STORE_INIT { NULL, NULL, NULL }
 
 void raw_object_store_clear(struct raw_object_store *o);
 
diff --git a/packfile.c b/packfile.c
index 4a5fe7ab18..d61076faaf 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "mru.h"
 #include "pack.h"
+#include "repository.h"
 #include "dir.h"
 #include "mergesort.h"
 #include "packfile.h"
@@ -880,7 +881,7 @@ void prepare_packed_git(void)
return;
prepare_packed_git_one(get_object_directory(), 1);
prepare_alt_odb();
-   for (alt = alt_odb_list; alt; alt = alt->next)
+   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next)
prepare_packed_git_one(alt->path, 0);
rearrange_packed_git();
prepare_packed_git_mru();
diff --git a/sha1_file.c b/sha1_file.c
index 3da70ac650..2826d5d6ed 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -22,6 +22,7 @@
 #include "pack-revindex.h"
 #include "sha1-lookup.h"
 #include "bulk-checkin.h"
+#include "repository.h"
 #include "streaming.h"
 #include "dir.h"
 #include "mru.h"
@@ -346,9 +347,6 @@ static const char *alt_sha1_path(struct 
alternate_object_database *alt,
return buf->buf;
 }
 
-struct alternate_object_database *alt_odb_list;
-static struct alternate_object_database **alt_odb_tail;
-
 /*
  * Return non-zero iff the path is usable as an alternate object database.
  */
@@ -368,7 +366,7 @@ static int alt_odb_usable(struct strbuf *path, const char 
*normalized_objdir)
 * Prevent the common mistake of listing the same
 * thing twice, or object directory itself.
 */
-   for (alt = alt_odb_list; alt; alt = alt->next) {
+   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
if (!fspathcmp(path->buf, alt->path))
return 0;
}
@@ -428,8 +426,8 @@ static int link_alt_odb_entry(const char *entry, const char 
*relative_base,
ent = alloc_alt_odb(pathbuf.buf);
 
/* add the alternate entry */
-   *alt_odb_tail = ent;
-   alt_odb_tail = &(ent->next);
+   *the_repository->objects.alt_odb_tail = ent;
+