{"thread":{"id":"48672","subject":"[PATCH 0/2] Object store refactoring: make bitmap_git not global","startedAt":"2018-06-07T19:04:26Z","lastAt":"2018-06-25T18:22:37Z","messageCount":9,"participants":["Jonathan Tan","Jeff King","Stefan Beller","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"349661","messageId":"cover.1528397984.git.jonathantanmy@google.com","threadId":"48672","inReplyTo":null,"subject":"[PATCH 0/2] Object store refactoring: make bitmap_git not global","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-07T19:04:12Z","receivedAt":"2018-06-07T19:04:26Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is a continuation of the object store refactoring effort.\n\nWe cannot truly free an object store without ensuring that any generated\nbitmaps are first freed, so here are patches to drastically reduce the\nlifetime of any bitmaps generated. As a bonus, the API is also improved,\nand global state reduced.\n\nJonathan Tan (2):\n  pack-bitmap: remove bitmap_git global variable\n  pack-bitmap: add free function\n\n builtin/pack-objects.c |   7 +-\n builtin/rev-list.c     |  13 +-\n pack-bitmap-write.c    |  10 +-\n pack-bitmap.c          | 344 ++++++++++++++++++++++++-----------------\n pack-bitmap.h          |  20 ++-\n 5 files changed, 234 insertions(+), 160 deletions(-)\n\n-- \n2.17.0.768.g1526ddbba1.dirty\n\n"},{"id":"349662","messageId":"b9be1d0371e5dc694e5e7ac88a900f985faabec8.1528397984.git.jonathantanmy@google.com","threadId":"48672","inReplyTo":"cover.1528397984.git.jonathantanmy@google.com","subject":"[PATCH 2/2] pack-bitmap: add free function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-07T19:04:14Z","receivedAt":"2018-06-07T19:04:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Add a function to free struct bitmap_index instances, and use it where\nneeded (except when rebuild_existing_bitmaps() is used, since it creates\nreferences to the bitmaps within the struct bitmap_index passed to it).\n\nNote that the hashes field in struct bitmap_index is not freed because\nit points to another field within the same struct. The documentation for\nthat field has been updated to clarify that.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/pack-objects.c |  1 +\n builtin/rev-list.c     |  2 ++\n pack-bitmap-write.c    |  4 ++++\n pack-bitmap.c          | 35 +++++++++++++++++++++++++++++------\n pack-bitmap.h          |  1 +\n 5 files changed, 37 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d064f944b..896e41300 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2945,6 +2945,7 @@ static int get_object_list_from_bitmap(struct rev_info *revs)\n \t}\n \n \ttraverse_bitmap_commit_list(bitmap_git, &add_object_entry_from_bitmap);\n+\tfree_bitmap_index(bitmap_git);\n \treturn 0;\n }\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex cce42ae1d..62776721f 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -521,6 +521,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\t\tif (max_count >= 0 && max_count < commit_count)\n \t\t\t\t\tcommit_count = max_count;\n \t\t\t\tprintf(\"%d\\n\", commit_count);\n+\t\t\t\tfree_bitmap_index(bitmap_git);\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t} else if (revs.max_count < 0 &&\n@@ -528,6 +529,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tstruct bitmap_index *bitmap_git;\n \t\t\tif ((bitmap_git = prepare_bitmap_walk(&revs))) {\n \t\t\t\ttraverse_bitmap_commit_list(bitmap_git, &show_object_fast);\n+\t\t\t\tfree_bitmap_index(bitmap_git);\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t}\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 03e122563..7896fedd3 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -367,6 +367,10 @@ void bitmap_writer_reuse_bitmaps(struct packing_data *to_pack)\n \twriter.reused = kh_init_sha1();\n \trebuild_existing_bitmaps(bitmap_git, to_pack, writer.reused,\n \t\t\t\t writer.show_progress);\n+\t/*\n+\t * NEEDSWORK: rebuild_existing_bitmaps() makes writer.reused reference\n+\t * some bitmaps in bitmap_git, so we can't free the latter.\n+\t */\n }\n \n static struct ewah_bitmap *find_reused_bitmap(const unsigned char *sha1)\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 7795444b0..c06b19f49 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -66,7 +66,7 @@ struct bitmap_index {\n \t/* Number of bitmapped commits */\n \tuint32_t entry_count;\n \n-\t/* Name-hash cache (or NULL if not present). */\n+\t/* If not NULL, this is a name-hash cache pointing into map. */\n \tuint32_t *hashes;\n \n \t/*\n@@ -350,6 +350,7 @@ struct bitmap_index *prepare_bitmap_git(void)\n \tif (!open_pack_bitmap(bitmap_git) && !load_pack_bitmap(bitmap_git))\n \t\treturn bitmap_git;\n \n+\tfree_bitmap_index(bitmap_git);\n \treturn NULL;\n }\n \n@@ -690,7 +691,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n \t/* try to open a bitmapped pack, but don't parse it yet\n \t * because we may not need to use it */\n \tif (open_pack_bitmap(bitmap_git) < 0)\n-\t\treturn NULL;\n+\t\tgoto cleanup;\n \n \tfor (i = 0; i < revs->pending.nr; ++i) {\n \t\tstruct object *object = revs->pending.objects[i].item;\n@@ -723,11 +724,11 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n \t * optimize here\n \t */\n \tif (haves && !in_bitmapped_pack(bitmap_git, haves))\n-\t\treturn NULL;\n+\t\tgoto cleanup;\n \n \t/* if we don't want anything, we're done here */\n \tif (!wants)\n-\t\treturn NULL;\n+\t\tgoto cleanup;\n \n \t/*\n \t * now we're going to use bitmaps, so load the actual bitmap entries\n@@ -735,7 +736,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n \t * becomes invalidated and we must perform the revwalk through bitmaps\n \t */\n \tif (!bitmap_git->loaded && load_pack_bitmap(bitmap_git) < 0)\n-\t\treturn NULL;\n+\t\tgoto cleanup;\n \n \tobject_array_clear(&revs->pending);\n \n@@ -761,6 +762,10 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n \n \tbitmap_free(haves_bitmap);\n \treturn bitmap_git;\n+\n+cleanup:\n+\tfree_bitmap_index(bitmap_git);\n+\treturn NULL;\n }\n \n int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n@@ -1001,7 +1006,7 @@ void test_bitmap_walk(struct rev_info *revs)\n \telse\n \t\tfprintf(stderr, \"Mismatch!\\n\");\n \n-\tbitmap_free(result);\n+\tfree_bitmap_index(bitmap_git);\n }\n \n static int rebuild_bitmap(uint32_t *reposition,\n@@ -1093,3 +1098,21 @@ int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,\n \tbitmap_free(rebuild);\n \treturn 0;\n }\n+\n+void free_bitmap_index(struct bitmap_index *b)\n+{\n+\tif (!b)\n+\t\treturn;\n+\n+\tif (b->map)\n+\t\tmunmap(b->map, b->map_size);\n+\tewah_pool_free(b->commits);\n+\tewah_pool_free(b->trees);\n+\tewah_pool_free(b->blobs);\n+\tewah_pool_free(b->tags);\n+\tkh_destroy_sha1(b->bitmaps);\n+\tfree(b->ext_index.objects);\n+\tfree(b->ext_index.hashes);\n+\tbitmap_free(b->result);\n+\tfree(b);\n+}\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 19f70043a..4555907de 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -48,6 +48,7 @@ int reuse_partial_packfile_from_bitmap(struct bitmap_index *,\n \t\t\t\t       uint32_t *entries, off_t *up_to);\n int rebuild_existing_bitmaps(struct bitmap_index *, struct packing_data *mapping,\n \t\t\t     khash_sha1 *reused_bitmaps, int show_progress);\n+void free_bitmap_index(struct bitmap_index *);\n \n void bitmap_writer_show_progress(int show);\n void bitmap_writer_set_checksum(unsigned char *sha1);\n-- \n2.17.0.768.g1526ddbba1.dirty\n\n"},{"id":"349663","messageId":"d4d4fec265c1c53e14da465904d93e559ff2c17d.1528397984.git.jonathantanmy@google.com","threadId":"48672","inReplyTo":"cover.1528397984.git.jonathantanmy@google.com","subject":"[PATCH 1/2] pack-bitmap: remove bitmap_git global variable","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-07T19:04:13Z","receivedAt":"2018-06-07T19:04:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Remove the bitmap_git global variable. Instead, generate on demand an\ninstance of struct bitmap_index for code that needs to access it.\n\nThis allows us significant control over the lifetime of instances of\nstruct bitmap_index. In particular, packs can now be closed without\nworrying if an unnecessarily long-lived \"pack\" field in struct\nbitmap_index still points to it.\n\nThe bitmap API is also clearer in that we need to first obtain a struct\nbitmap_index, then we use it.\n\nHelped-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/pack-objects.c |   6 +-\n builtin/rev-list.c     |  11 +-\n pack-bitmap-write.c    |   6 +-\n pack-bitmap.c          | 317 ++++++++++++++++++++++-------------------\n pack-bitmap.h          |  19 ++-\n 5 files changed, 201 insertions(+), 158 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 71056d829..d064f944b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2929,11 +2929,13 @@ static int pack_options_allow_reuse(void)\n \n static int get_object_list_from_bitmap(struct rev_info *revs)\n {\n-\tif (prepare_bitmap_walk(revs) < 0)\n+\tstruct bitmap_index *bitmap_git;\n+\tif (!(bitmap_git = prepare_bitmap_walk(revs)))\n \t\treturn -1;\n \n \tif (pack_options_allow_reuse() &&\n \t    !reuse_partial_packfile_from_bitmap(\n+\t\t\tbitmap_git,\n \t\t\t&reuse_packfile,\n \t\t\t&reuse_packfile_objects,\n \t\t\t&reuse_packfile_offset)) {\n@@ -2942,7 +2944,7 @@ static int get_object_list_from_bitmap(struct rev_info *revs)\n \t\tdisplay_progress(progress_state, nr_result);\n \t}\n \n-\ttraverse_bitmap_commit_list(&add_object_entry_from_bitmap);\n+\ttraverse_bitmap_commit_list(bitmap_git, &add_object_entry_from_bitmap);\n \treturn 0;\n }\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex fadd3ec14..cce42ae1d 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -16,6 +16,7 @@\n #include \"reflog-walk.h\"\n #include \"oidset.h\"\n #include \"packfile.h\"\n+#include \"object-store.h\"\n \n static const char rev_list_usage[] =\n \"git rev-list [OPTION] <commit-id>... [ -- paths... ]\\n\"\n@@ -514,8 +515,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\tif (revs.count && !revs.left_right && !revs.cherry_mark) {\n \t\t\tuint32_t commit_count;\n \t\t\tint max_count = revs.max_count;\n-\t\t\tif (!prepare_bitmap_walk(&revs)) {\n-\t\t\t\tcount_bitmap_commit_list(&commit_count, NULL, NULL, NULL);\n+\t\t\tstruct bitmap_index *bitmap_git;\n+\t\t\tif ((bitmap_git = prepare_bitmap_walk(&revs))) {\n+\t\t\t\tcount_bitmap_commit_list(bitmap_git, &commit_count, NULL, NULL, NULL);\n \t\t\t\tif (max_count >= 0 && max_count < commit_count)\n \t\t\t\t\tcommit_count = max_count;\n \t\t\t\tprintf(\"%d\\n\", commit_count);\n@@ -523,8 +525,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t} else if (revs.max_count < 0 &&\n \t\t\t   revs.tag_objects && revs.tree_objects && revs.blob_objects) {\n-\t\t\tif (!prepare_bitmap_walk(&revs)) {\n-\t\t\t\ttraverse_bitmap_commit_list(&show_object_fast);\n+\t\t\tstruct bitmap_index *bitmap_git;\n+\t\t\tif ((bitmap_git = prepare_bitmap_walk(&revs))) {\n+\t\t\t\ttraverse_bitmap_commit_list(bitmap_git, &show_object_fast);\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t}\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 7b2dc3e7d..03e122563 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -360,11 +360,13 @@ static int date_compare(const void *_a, const void *_b)\n \n void bitmap_writer_reuse_bitmaps(struct packing_data *to_pack)\n {\n-\tif (prepare_bitmap_git() < 0)\n+\tstruct bitmap_index *bitmap_git;\n+\tif (!(bitmap_git = prepare_bitmap_git()))\n \t\treturn;\n \n \twriter.reused = kh_init_sha1();\n-\trebuild_existing_bitmaps(to_pack, writer.reused, writer.show_progress);\n+\trebuild_existing_bitmaps(bitmap_git, to_pack, writer.reused,\n+\t\t\t\t writer.show_progress);\n }\n \n static struct ewah_bitmap *find_reused_bitmap(const unsigned char *sha1)\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 06771113f..7795444b0 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -25,14 +25,14 @@ struct stored_bitmap {\n };\n \n /*\n- * The currently active bitmap index. By design, repositories only have\n+ * The active bitmap index for a repository. By design, repositories only have\n  * a single bitmap index available (the index for the biggest packfile in\n  * the repository), since bitmap indexes need full closure.\n  *\n  * If there is more than one bitmap index available (e.g. because of alternates),\n  * the active bitmap index is the largest one.\n  */\n-static struct bitmap_index {\n+struct bitmap_index {\n \t/* Packfile to which this bitmap index belongs to */\n \tstruct packed_git *pack;\n \n@@ -90,8 +90,7 @@ static struct bitmap_index {\n \tunsigned int version;\n \n \tunsigned loaded : 1;\n-\n-} bitmap_git;\n+};\n \n static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n {\n@@ -259,7 +258,7 @@ static char *pack_bitmap_filename(struct packed_git *p)\n \treturn xstrfmt(\"%.*s.bitmap\", (int)len, p->pack_name);\n }\n \n-static int open_pack_bitmap_1(struct packed_git *packfile)\n+static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)\n {\n \tint fd;\n \tstruct stat st;\n@@ -280,117 +279,121 @@ static int open_pack_bitmap_1(struct packed_git *packfile)\n \t\treturn -1;\n \t}\n \n-\tif (bitmap_git.pack) {\n+\tif (bitmap_git->pack) {\n \t\twarning(\"ignoring extra bitmap file: %s\", packfile->pack_name);\n \t\tclose(fd);\n \t\treturn -1;\n \t}\n \n-\tbitmap_git.pack = packfile;\n-\tbitmap_git.map_size = xsize_t(st.st_size);\n-\tbitmap_git.map = xmmap(NULL, bitmap_git.map_size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tbitmap_git.map_pos = 0;\n+\tbitmap_git->pack = packfile;\n+\tbitmap_git->map_size = xsize_t(st.st_size);\n+\tbitmap_git->map = xmmap(NULL, bitmap_git->map_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tbitmap_git->map_pos = 0;\n \tclose(fd);\n \n-\tif (load_bitmap_header(&bitmap_git) < 0) {\n-\t\tmunmap(bitmap_git.map, bitmap_git.map_size);\n-\t\tbitmap_git.map = NULL;\n-\t\tbitmap_git.map_size = 0;\n+\tif (load_bitmap_header(bitmap_git) < 0) {\n+\t\tmunmap(bitmap_git->map, bitmap_git->map_size);\n+\t\tbitmap_git->map = NULL;\n+\t\tbitmap_git->map_size = 0;\n \t\treturn -1;\n \t}\n \n \treturn 0;\n }\n \n-static int load_pack_bitmap(void)\n+static int load_pack_bitmap(struct bitmap_index *bitmap_git)\n {\n-\tassert(bitmap_git.map && !bitmap_git.loaded);\n+\tassert(bitmap_git->map && !bitmap_git->loaded);\n \n-\tbitmap_git.bitmaps = kh_init_sha1();\n-\tbitmap_git.ext_index.positions = kh_init_sha1_pos();\n-\tload_pack_revindex(bitmap_git.pack);\n+\tbitmap_git->bitmaps = kh_init_sha1();\n+\tbitmap_git->ext_index.positions = kh_init_sha1_pos();\n+\tload_pack_revindex(bitmap_git->pack);\n \n-\tif (!(bitmap_git.commits = read_bitmap_1(&bitmap_git)) ||\n-\t\t!(bitmap_git.trees = read_bitmap_1(&bitmap_git)) ||\n-\t\t!(bitmap_git.blobs = read_bitmap_1(&bitmap_git)) ||\n-\t\t!(bitmap_git.tags = read_bitmap_1(&bitmap_git)))\n+\tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n+\t\t!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||\n+\t\t!(bitmap_git->blobs = read_bitmap_1(bitmap_git)) ||\n+\t\t!(bitmap_git->tags = read_bitmap_1(bitmap_git)))\n \t\tgoto failed;\n \n-\tif (load_bitmap_entries_v1(&bitmap_git) < 0)\n+\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n \t\tgoto failed;\n \n-\tbitmap_git.loaded = 1;\n+\tbitmap_git->loaded = 1;\n \treturn 0;\n \n failed:\n-\tmunmap(bitmap_git.map, bitmap_git.map_size);\n-\tbitmap_git.map = NULL;\n-\tbitmap_git.map_size = 0;\n+\tmunmap(bitmap_git->map, bitmap_git->map_size);\n+\tbitmap_git->map = NULL;\n+\tbitmap_git->map_size = 0;\n \treturn -1;\n }\n \n-static int open_pack_bitmap(void)\n+static int open_pack_bitmap(struct bitmap_index *bitmap_git)\n {\n \tstruct packed_git *p;\n \tint ret = -1;\n \n-\tassert(!bitmap_git.map && !bitmap_git.loaded);\n+\tassert(!bitmap_git->map && !bitmap_git->loaded);\n \n \tfor (p = get_packed_git(the_repository); p; p = p->next) {\n-\t\tif (open_pack_bitmap_1(p) == 0)\n+\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n \t\t\tret = 0;\n \t}\n \n \treturn ret;\n }\n \n-int prepare_bitmap_git(void)\n+struct bitmap_index *prepare_bitmap_git(void)\n {\n-\tif (bitmap_git.loaded)\n-\t\treturn 0;\n+\tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n \n-\tif (!open_pack_bitmap())\n-\t\treturn load_pack_bitmap();\n+\tif (!open_pack_bitmap(bitmap_git) && !load_pack_bitmap(bitmap_git))\n+\t\treturn bitmap_git;\n \n-\treturn -1;\n+\treturn NULL;\n }\n \n struct include_data {\n+\tstruct bitmap_index *bitmap_git;\n \tstruct bitmap *base;\n \tstruct bitmap *seen;\n };\n \n-static inline int bitmap_position_extended(const unsigned char *sha1)\n+static inline int bitmap_position_extended(struct bitmap_index *bitmap_git,\n+\t\t\t\t\t   const unsigned char *sha1)\n {\n-\tkhash_sha1_pos *positions = bitmap_git.ext_index.positions;\n+\tkhash_sha1_pos *positions = bitmap_git->ext_index.positions;\n \tkhiter_t pos = kh_get_sha1_pos(positions, sha1);\n \n \tif (pos < kh_end(positions)) {\n \t\tint bitmap_pos = kh_value(positions, pos);\n-\t\treturn bitmap_pos + bitmap_git.pack->num_objects;\n+\t\treturn bitmap_pos + bitmap_git->pack->num_objects;\n \t}\n \n \treturn -1;\n }\n \n-static inline int bitmap_position_packfile(const unsigned char *sha1)\n+static inline int bitmap_position_packfile(struct bitmap_index *bitmap_git,\n+\t\t\t\t\t   const unsigned char *sha1)\n {\n-\toff_t offset = find_pack_entry_one(sha1, bitmap_git.pack);\n+\toff_t offset = find_pack_entry_one(sha1, bitmap_git->pack);\n \tif (!offset)\n \t\treturn -1;\n \n-\treturn find_revindex_position(bitmap_git.pack, offset);\n+\treturn find_revindex_position(bitmap_git->pack, offset);\n }\n \n-static int bitmap_position(const unsigned char *sha1)\n+static int bitmap_position(struct bitmap_index *bitmap_git,\n+\t\t\t   const unsigned char *sha1)\n {\n-\tint pos = bitmap_position_packfile(sha1);\n-\treturn (pos >= 0) ? pos : bitmap_position_extended(sha1);\n+\tint pos = bitmap_position_packfile(bitmap_git, sha1);\n+\treturn (pos >= 0) ? pos : bitmap_position_extended(bitmap_git, sha1);\n }\n \n-static int ext_index_add_object(struct object *object, const char *name)\n+static int ext_index_add_object(struct bitmap_index *bitmap_git,\n+\t\t\t\tstruct object *object, const char *name)\n {\n-\tstruct eindex *eindex = &bitmap_git.ext_index;\n+\tstruct eindex *eindex = &bitmap_git->ext_index;\n \n \tkhiter_t hash_pos;\n \tint hash_ret;\n@@ -413,27 +416,34 @@ static int ext_index_add_object(struct object *object, const char *name)\n \t\tbitmap_pos = kh_value(eindex->positions, hash_pos);\n \t}\n \n-\treturn bitmap_pos + bitmap_git.pack->num_objects;\n+\treturn bitmap_pos + bitmap_git->pack->num_objects;\n }\n \n-static void show_object(struct object *object, const char *name, void *data)\n+struct bitmap_show_data {\n+\tstruct bitmap_index *bitmap_git;\n+\tstruct bitmap *base;\n+};\n+\n+static void show_object(struct object *object, const char *name, void *data_)\n {\n-\tstruct bitmap *base = data;\n+\tstruct bitmap_show_data *data = data_;\n \tint bitmap_pos;\n \n-\tbitmap_pos = bitmap_position(object->oid.hash);\n+\tbitmap_pos = bitmap_position(data->bitmap_git, object->oid.hash);\n \n \tif (bitmap_pos < 0)\n-\t\tbitmap_pos = ext_index_add_object(object, name);\n+\t\tbitmap_pos = ext_index_add_object(data->bitmap_git, object,\n+\t\t\t\t\t\t  name);\n \n-\tbitmap_set(base, bitmap_pos);\n+\tbitmap_set(data->base, bitmap_pos);\n }\n \n static void show_commit(struct commit *commit, void *data)\n {\n }\n \n-static int add_to_include_set(struct include_data *data,\n+static int add_to_include_set(struct bitmap_index *bitmap_git,\n+\t\t\t      struct include_data *data,\n \t\t\t      const unsigned char *sha1,\n \t\t\t      int bitmap_pos)\n {\n@@ -445,9 +455,9 @@ static int add_to_include_set(struct include_data *data,\n \tif (bitmap_get(data->base, bitmap_pos))\n \t\treturn 0;\n \n-\thash_pos = kh_get_sha1(bitmap_git.bitmaps, sha1);\n-\tif (hash_pos < kh_end(bitmap_git.bitmaps)) {\n-\t\tstruct stored_bitmap *st = kh_value(bitmap_git.bitmaps, hash_pos);\n+\thash_pos = kh_get_sha1(bitmap_git->bitmaps, sha1);\n+\tif (hash_pos < kh_end(bitmap_git->bitmaps)) {\n+\t\tstruct stored_bitmap *st = kh_value(bitmap_git->bitmaps, hash_pos);\n \t\tbitmap_or_ewah(data->base, lookup_stored_bitmap(st));\n \t\treturn 0;\n \t}\n@@ -461,11 +471,14 @@ static int should_include(struct commit *commit, void *_data)\n \tstruct include_data *data = _data;\n \tint bitmap_pos;\n \n-\tbitmap_pos = bitmap_position(commit->object.oid.hash);\n+\tbitmap_pos = bitmap_position(data->bitmap_git, commit->object.oid.hash);\n \tif (bitmap_pos < 0)\n-\t\tbitmap_pos = ext_index_add_object((struct object *)commit, NULL);\n+\t\tbitmap_pos = ext_index_add_object(data->bitmap_git,\n+\t\t\t\t\t\t  (struct object *)commit,\n+\t\t\t\t\t\t  NULL);\n \n-\tif (!add_to_include_set(data, commit->object.oid.hash, bitmap_pos)) {\n+\tif (!add_to_include_set(data->bitmap_git, data, commit->object.oid.hash,\n+\t\t\t\tbitmap_pos)) {\n \t\tstruct commit_list *parent = commit->parents;\n \n \t\twhile (parent) {\n@@ -479,7 +492,8 @@ static int should_include(struct commit *commit, void *_data)\n \treturn 1;\n }\n \n-static struct bitmap *find_objects(struct rev_info *revs,\n+static struct bitmap *find_objects(struct bitmap_index *bitmap_git,\n+\t\t\t\t   struct rev_info *revs,\n \t\t\t\t   struct object_list *roots,\n \t\t\t\t   struct bitmap *seen)\n {\n@@ -501,10 +515,10 @@ static struct bitmap *find_objects(struct rev_info *revs,\n \t\troots = roots->next;\n \n \t\tif (object->type == OBJ_COMMIT) {\n-\t\t\tkhiter_t pos = kh_get_sha1(bitmap_git.bitmaps, object->oid.hash);\n+\t\t\tkhiter_t pos = kh_get_sha1(bitmap_git->bitmaps, object->oid.hash);\n \n-\t\t\tif (pos < kh_end(bitmap_git.bitmaps)) {\n-\t\t\t\tstruct stored_bitmap *st = kh_value(bitmap_git.bitmaps, pos);\n+\t\t\tif (pos < kh_end(bitmap_git->bitmaps)) {\n+\t\t\t\tstruct stored_bitmap *st = kh_value(bitmap_git->bitmaps, pos);\n \t\t\t\tstruct ewah_bitmap *or_with = lookup_stored_bitmap(st);\n \n \t\t\t\tif (base == NULL)\n@@ -543,7 +557,7 @@ static struct bitmap *find_objects(struct rev_info *revs,\n \t\tint pos;\n \n \t\troots = roots->next;\n-\t\tpos = bitmap_position(object->oid.hash);\n+\t\tpos = bitmap_position(bitmap_git, object->oid.hash);\n \n \t\tif (pos < 0 || base == NULL || !bitmap_get(base, pos)) {\n \t\t\tobject->flags &= ~UNINTERESTING;\n@@ -556,10 +570,12 @@ static struct bitmap *find_objects(struct rev_info *revs,\n \n \tif (needs_walk) {\n \t\tstruct include_data incdata;\n+\t\tstruct bitmap_show_data show_data;\n \n \t\tif (base == NULL)\n \t\t\tbase = bitmap_new();\n \n+\t\tincdata.bitmap_git = bitmap_git;\n \t\tincdata.base = base;\n \t\tincdata.seen = seen;\n \n@@ -569,22 +585,27 @@ static struct bitmap *find_objects(struct rev_info *revs,\n \t\tif (prepare_revision_walk(revs))\n \t\t\tdie(\"revision walk setup failed\");\n \n-\t\ttraverse_commit_list(revs, show_commit, show_object, base);\n+\t\tshow_data.bitmap_git = bitmap_git;\n+\t\tshow_data.base = base;\n+\n+\t\ttraverse_commit_list(revs, show_commit, show_object,\n+\t\t\t\t     &show_data);\n \t}\n \n \treturn base;\n }\n \n-static void show_extended_objects(struct bitmap *objects,\n+static void show_extended_objects(struct bitmap_index *bitmap_git,\n \t\t\t\t  show_reachable_fn show_reach)\n {\n-\tstruct eindex *eindex = &bitmap_git.ext_index;\n+\tstruct bitmap *objects = bitmap_git->result;\n+\tstruct eindex *eindex = &bitmap_git->ext_index;\n \tuint32_t i;\n \n \tfor (i = 0; i < eindex->count; ++i) {\n \t\tstruct object *obj;\n \n-\t\tif (!bitmap_get(objects, bitmap_git.pack->num_objects + i))\n+\t\tif (!bitmap_get(objects, bitmap_git->pack->num_objects + i))\n \t\t\tcontinue;\n \n \t\tobj = eindex->objects[i];\n@@ -593,7 +614,7 @@ static void show_extended_objects(struct bitmap *objects,\n }\n \n static void show_objects_for_type(\n-\tstruct bitmap *objects,\n+\tstruct bitmap_index *bitmap_git,\n \tstruct ewah_bitmap *type_filter,\n \tenum object_type object_type,\n \tshow_reachable_fn show_reach)\n@@ -604,7 +625,9 @@ static void show_objects_for_type(\n \tstruct ewah_iterator it;\n \teword_t filter;\n \n-\tif (bitmap_git.reuse_objects == bitmap_git.pack->num_objects)\n+\tstruct bitmap *objects = bitmap_git->result;\n+\n+\tif (bitmap_git->reuse_objects == bitmap_git->pack->num_objects)\n \t\treturn;\n \n \tewah_iterator_init(&it, type_filter);\n@@ -622,16 +645,16 @@ static void show_objects_for_type(\n \n \t\t\toffset += ewah_bit_ctz64(word >> offset);\n \n-\t\t\tif (pos + offset < bitmap_git.reuse_objects)\n+\t\t\tif (pos + offset < bitmap_git->reuse_objects)\n \t\t\t\tcontinue;\n \n-\t\t\tentry = &bitmap_git.pack->revindex[pos + offset];\n-\t\t\tnth_packed_object_oid(&oid, bitmap_git.pack, entry->nr);\n+\t\t\tentry = &bitmap_git->pack->revindex[pos + offset];\n+\t\t\tnth_packed_object_oid(&oid, bitmap_git->pack, entry->nr);\n \n-\t\t\tif (bitmap_git.hashes)\n-\t\t\t\thash = get_be32(bitmap_git.hashes + entry->nr);\n+\t\t\tif (bitmap_git->hashes)\n+\t\t\t\thash = get_be32(bitmap_git->hashes + entry->nr);\n \n-\t\t\tshow_reach(&oid, object_type, 0, hash, bitmap_git.pack, entry->offset);\n+\t\t\tshow_reach(&oid, object_type, 0, hash, bitmap_git->pack, entry->offset);\n \t\t}\n \n \t\tpos += BITS_IN_EWORD;\n@@ -639,20 +662,21 @@ static void show_objects_for_type(\n \t}\n }\n \n-static int in_bitmapped_pack(struct object_list *roots)\n+static int in_bitmapped_pack(struct bitmap_index *bitmap_git,\n+\t\t\t     struct object_list *roots)\n {\n \twhile (roots) {\n \t\tstruct object *object = roots->item;\n \t\troots = roots->next;\n \n-\t\tif (find_pack_entry_one(object->oid.hash, bitmap_git.pack) > 0)\n+\t\tif (find_pack_entry_one(object->oid.hash, bitmap_git->pack) > 0)\n \t\t\treturn 1;\n \t}\n \n \treturn 0;\n }\n \n-int prepare_bitmap_walk(struct rev_info *revs)\n+struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n {\n \tunsigned int i;\n \n@@ -662,12 +686,11 @@ int prepare_bitmap_walk(struct rev_info *revs)\n \tstruct bitmap *wants_bitmap = NULL;\n \tstruct bitmap *haves_bitmap = NULL;\n \n-\tif (!bitmap_git.loaded) {\n-\t\t/* try to open a bitmapped pack, but don't parse it yet\n-\t\t * because we may not need to use it */\n-\t\tif (open_pack_bitmap() < 0)\n-\t\t\treturn -1;\n-\t}\n+\tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n+\t/* try to open a bitmapped pack, but don't parse it yet\n+\t * because we may not need to use it */\n+\tif (open_pack_bitmap(bitmap_git) < 0)\n+\t\treturn NULL;\n \n \tfor (i = 0; i < revs->pending.nr; ++i) {\n \t\tstruct object *object = revs->pending.objects[i].item;\n@@ -699,26 +722,26 @@ int prepare_bitmap_walk(struct rev_info *revs)\n \t * in the packfile that has a bitmap, we don't have anything to\n \t * optimize here\n \t */\n-\tif (haves && !in_bitmapped_pack(haves))\n-\t\treturn -1;\n+\tif (haves && !in_bitmapped_pack(bitmap_git, haves))\n+\t\treturn NULL;\n \n \t/* if we don't want anything, we're done here */\n \tif (!wants)\n-\t\treturn -1;\n+\t\treturn NULL;\n \n \t/*\n \t * now we're going to use bitmaps, so load the actual bitmap entries\n \t * from disk. this is the point of no return; after this the rev_list\n \t * becomes invalidated and we must perform the revwalk through bitmaps\n \t */\n-\tif (!bitmap_git.loaded && load_pack_bitmap() < 0)\n-\t\treturn -1;\n+\tif (!bitmap_git->loaded && load_pack_bitmap(bitmap_git) < 0)\n+\t\treturn NULL;\n \n \tobject_array_clear(&revs->pending);\n \n \tif (haves) {\n \t\trevs->ignore_missing_links = 1;\n-\t\thaves_bitmap = find_objects(revs, haves, NULL);\n+\t\thaves_bitmap = find_objects(bitmap_git, revs, haves, NULL);\n \t\treset_revision_walk();\n \t\trevs->ignore_missing_links = 0;\n \n@@ -726,7 +749,7 @@ int prepare_bitmap_walk(struct rev_info *revs)\n \t\t\tBUG(\"failed to perform bitmap walk\");\n \t}\n \n-\twants_bitmap = find_objects(revs, wants, haves_bitmap);\n+\twants_bitmap = find_objects(bitmap_git, revs, wants, haves_bitmap);\n \n \tif (!wants_bitmap)\n \t\tBUG(\"failed to perform bitmap walk\");\n@@ -734,13 +757,14 @@ int prepare_bitmap_walk(struct rev_info *revs)\n \tif (haves_bitmap)\n \t\tbitmap_and_not(wants_bitmap, haves_bitmap);\n \n-\tbitmap_git.result = wants_bitmap;\n+\tbitmap_git->result = wants_bitmap;\n \n \tbitmap_free(haves_bitmap);\n-\treturn 0;\n+\treturn bitmap_git;\n }\n \n-int reuse_partial_packfile_from_bitmap(struct packed_git **packfile,\n+int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n+\t\t\t\t       struct packed_git **packfile,\n \t\t\t\t       uint32_t *entries,\n \t\t\t\t       off_t *up_to)\n {\n@@ -750,7 +774,7 @@ int reuse_partial_packfile_from_bitmap(struct packed_git **packfile,\n \t */\n \tstatic const double REUSE_PERCENT = 0.9;\n \n-\tstruct bitmap *result = bitmap_git.result;\n+\tstruct bitmap *result = bitmap_git->result;\n \tuint32_t reuse_threshold;\n \tuint32_t i, reuse_objects = 0;\n \n@@ -770,8 +794,8 @@ int reuse_partial_packfile_from_bitmap(struct packed_git **packfile,\n \t\tconst unsigned char *sha1;\n \t\tstruct revindex_entry *entry;\n \n-\t\tentry = &bitmap_git.reverse_index->revindex[reuse_objects];\n-\t\tsha1 = nth_packed_object_sha1(bitmap_git.pack, entry->nr);\n+\t\tentry = &bitmap_git->reverse_index->revindex[reuse_objects];\n+\t\tsha1 = nth_packed_object_sha1(bitmap_git->pack, entry->nr);\n \n \t\tfprintf(stderr, \"Failed to reuse at %d (%016llx)\\n\",\n \t\t\treuse_objects, result->words[i]);\n@@ -782,48 +806,50 @@ int reuse_partial_packfile_from_bitmap(struct packed_git **packfile,\n \tif (!reuse_objects)\n \t\treturn -1;\n \n-\tif (reuse_objects >= bitmap_git.pack->num_objects) {\n-\t\tbitmap_git.reuse_objects = *entries = bitmap_git.pack->num_objects;\n+\tif (reuse_objects >= bitmap_git->pack->num_objects) {\n+\t\tbitmap_git->reuse_objects = *entries = bitmap_git->pack->num_objects;\n \t\t*up_to = -1; /* reuse the full pack */\n-\t\t*packfile = bitmap_git.pack;\n+\t\t*packfile = bitmap_git->pack;\n \t\treturn 0;\n \t}\n \n-\treuse_threshold = bitmap_popcount(bitmap_git.result) * REUSE_PERCENT;\n+\treuse_threshold = bitmap_popcount(bitmap_git->result) * REUSE_PERCENT;\n \n \tif (reuse_objects < reuse_threshold)\n \t\treturn -1;\n \n-\tbitmap_git.reuse_objects = *entries = reuse_objects;\n-\t*up_to = bitmap_git.pack->revindex[reuse_objects].offset;\n-\t*packfile = bitmap_git.pack;\n+\tbitmap_git->reuse_objects = *entries = reuse_objects;\n+\t*up_to = bitmap_git->pack->revindex[reuse_objects].offset;\n+\t*packfile = bitmap_git->pack;\n \n \treturn 0;\n }\n \n-void traverse_bitmap_commit_list(show_reachable_fn show_reachable)\n+void traverse_bitmap_commit_list(struct bitmap_index *bitmap_git,\n+\t\t\t\t show_reachable_fn show_reachable)\n {\n-\tassert(bitmap_git.result);\n+\tassert(bitmap_git->result);\n \n-\tshow_objects_for_type(bitmap_git.result, bitmap_git.commits,\n+\tshow_objects_for_type(bitmap_git, bitmap_git->commits,\n \t\tOBJ_COMMIT, show_reachable);\n-\tshow_objects_for_type(bitmap_git.result, bitmap_git.trees,\n+\tshow_objects_for_type(bitmap_git, bitmap_git->trees,\n \t\tOBJ_TREE, show_reachable);\n-\tshow_objects_for_type(bitmap_git.result, bitmap_git.blobs,\n+\tshow_objects_for_type(bitmap_git, bitmap_git->blobs,\n \t\tOBJ_BLOB, show_reachable);\n-\tshow_objects_for_type(bitmap_git.result, bitmap_git.tags,\n+\tshow_objects_for_type(bitmap_git, bitmap_git->tags,\n \t\tOBJ_TAG, show_reachable);\n \n-\tshow_extended_objects(bitmap_git.result, show_reachable);\n+\tshow_extended_objects(bitmap_git, show_reachable);\n \n-\tbitmap_free(bitmap_git.result);\n-\tbitmap_git.result = NULL;\n+\tbitmap_free(bitmap_git->result);\n+\tbitmap_git->result = NULL;\n }\n \n-static uint32_t count_object_type(struct bitmap *objects,\n+static uint32_t count_object_type(struct bitmap_index *bitmap_git,\n \t\t\t\t  enum object_type type)\n {\n-\tstruct eindex *eindex = &bitmap_git.ext_index;\n+\tstruct bitmap *objects = bitmap_git->result;\n+\tstruct eindex *eindex = &bitmap_git->ext_index;\n \n \tuint32_t i = 0, count = 0;\n \tstruct ewah_iterator it;\n@@ -831,19 +857,19 @@ static uint32_t count_object_type(struct bitmap *objects,\n \n \tswitch (type) {\n \tcase OBJ_COMMIT:\n-\t\tewah_iterator_init(&it, bitmap_git.commits);\n+\t\tewah_iterator_init(&it, bitmap_git->commits);\n \t\tbreak;\n \n \tcase OBJ_TREE:\n-\t\tewah_iterator_init(&it, bitmap_git.trees);\n+\t\tewah_iterator_init(&it, bitmap_git->trees);\n \t\tbreak;\n \n \tcase OBJ_BLOB:\n-\t\tewah_iterator_init(&it, bitmap_git.blobs);\n+\t\tewah_iterator_init(&it, bitmap_git->blobs);\n \t\tbreak;\n \n \tcase OBJ_TAG:\n-\t\tewah_iterator_init(&it, bitmap_git.tags);\n+\t\tewah_iterator_init(&it, bitmap_git->tags);\n \t\tbreak;\n \n \tdefault:\n@@ -857,32 +883,34 @@ static uint32_t count_object_type(struct bitmap *objects,\n \n \tfor (i = 0; i < eindex->count; ++i) {\n \t\tif (eindex->objects[i]->type == type &&\n-\t\t\tbitmap_get(objects, bitmap_git.pack->num_objects + i))\n+\t\t\tbitmap_get(objects, bitmap_git->pack->num_objects + i))\n \t\t\tcount++;\n \t}\n \n \treturn count;\n }\n \n-void count_bitmap_commit_list(uint32_t *commits, uint32_t *trees,\n+void count_bitmap_commit_list(struct bitmap_index *bitmap_git,\n+\t\t\t      uint32_t *commits, uint32_t *trees,\n \t\t\t      uint32_t *blobs, uint32_t *tags)\n {\n-\tassert(bitmap_git.result);\n+\tassert(bitmap_git->result);\n \n \tif (commits)\n-\t\t*commits = count_object_type(bitmap_git.result, OBJ_COMMIT);\n+\t\t*commits = count_object_type(bitmap_git, OBJ_COMMIT);\n \n \tif (trees)\n-\t\t*trees = count_object_type(bitmap_git.result, OBJ_TREE);\n+\t\t*trees = count_object_type(bitmap_git, OBJ_TREE);\n \n \tif (blobs)\n-\t\t*blobs = count_object_type(bitmap_git.result, OBJ_BLOB);\n+\t\t*blobs = count_object_type(bitmap_git, OBJ_BLOB);\n \n \tif (tags)\n-\t\t*tags = count_object_type(bitmap_git.result, OBJ_TAG);\n+\t\t*tags = count_object_type(bitmap_git, OBJ_TAG);\n }\n \n struct bitmap_test_data {\n+\tstruct bitmap_index *bitmap_git;\n \tstruct bitmap *base;\n \tstruct progress *prg;\n \tsize_t seen;\n@@ -894,7 +922,7 @@ static void test_show_object(struct object *object, const char *name,\n \tstruct bitmap_test_data *tdata = data;\n \tint bitmap_pos;\n \n-\tbitmap_pos = bitmap_position(object->oid.hash);\n+\tbitmap_pos = bitmap_position(tdata->bitmap_git, object->oid.hash);\n \tif (bitmap_pos < 0)\n \t\tdie(\"Object not in bitmap: %s\\n\", oid_to_hex(&object->oid));\n \n@@ -907,7 +935,8 @@ static void test_show_commit(struct commit *commit, void *data)\n \tstruct bitmap_test_data *tdata = data;\n \tint bitmap_pos;\n \n-\tbitmap_pos = bitmap_position(commit->object.oid.hash);\n+\tbitmap_pos = bitmap_position(tdata->bitmap_git,\n+\t\t\t\t     commit->object.oid.hash);\n \tif (bitmap_pos < 0)\n \t\tdie(\"Object not in bitmap: %s\\n\", oid_to_hex(&commit->object.oid));\n \n@@ -922,21 +951,22 @@ void test_bitmap_walk(struct rev_info *revs)\n \tkhiter_t pos;\n \tsize_t result_popcnt;\n \tstruct bitmap_test_data tdata;\n+\tstruct bitmap_index *bitmap_git;\n \n-\tif (prepare_bitmap_git())\n+\tif (!(bitmap_git = prepare_bitmap_git()))\n \t\tdie(\"failed to load bitmap indexes\");\n \n \tif (revs->pending.nr != 1)\n \t\tdie(\"you must specify exactly one commit to test\");\n \n \tfprintf(stderr, \"Bitmap v%d test (%d entries loaded)\\n\",\n-\t\tbitmap_git.version, bitmap_git.entry_count);\n+\t\tbitmap_git->version, bitmap_git->entry_count);\n \n \troot = revs->pending.objects[0].item;\n-\tpos = kh_get_sha1(bitmap_git.bitmaps, root->oid.hash);\n+\tpos = kh_get_sha1(bitmap_git->bitmaps, root->oid.hash);\n \n-\tif (pos < kh_end(bitmap_git.bitmaps)) {\n-\t\tstruct stored_bitmap *st = kh_value(bitmap_git.bitmaps, pos);\n+\tif (pos < kh_end(bitmap_git->bitmaps)) {\n+\t\tstruct stored_bitmap *st = kh_value(bitmap_git->bitmaps, pos);\n \t\tstruct ewah_bitmap *bm = lookup_stored_bitmap(st);\n \n \t\tfprintf(stderr, \"Found bitmap for %s. %d bits / %08x checksum\\n\",\n@@ -957,6 +987,7 @@ void test_bitmap_walk(struct rev_info *revs)\n \tif (prepare_revision_walk(revs))\n \t\tdie(\"revision walk setup failed\");\n \n+\ttdata.bitmap_git = bitmap_git;\n \ttdata.base = bitmap_new();\n \ttdata.prg = start_progress(\"Verifying bitmap entries\", result_popcnt);\n \ttdata.seen = 0;\n@@ -1004,7 +1035,8 @@ static int rebuild_bitmap(uint32_t *reposition,\n \treturn 0;\n }\n \n-int rebuild_existing_bitmaps(struct packing_data *mapping,\n+int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,\n+\t\t\t     struct packing_data *mapping,\n \t\t\t     khash_sha1 *reused_bitmaps,\n \t\t\t     int show_progress)\n {\n@@ -1017,10 +1049,7 @@ int rebuild_existing_bitmaps(struct packing_data *mapping,\n \tkhiter_t hash_pos;\n \tint hash_ret;\n \n-\tif (prepare_bitmap_git() < 0)\n-\t\treturn -1;\n-\n-\tnum_objects = bitmap_git.pack->num_objects;\n+\tnum_objects = bitmap_git->pack->num_objects;\n \treposition = xcalloc(num_objects, sizeof(uint32_t));\n \n \tfor (i = 0; i < num_objects; ++i) {\n@@ -1028,8 +1057,8 @@ int rebuild_existing_bitmaps(struct packing_data *mapping,\n \t\tstruct revindex_entry *entry;\n \t\tstruct object_entry *oe;\n \n-\t\tentry = &bitmap_git.pack->revindex[i];\n-\t\tsha1 = nth_packed_object_sha1(bitmap_git.pack, entry->nr);\n+\t\tentry = &bitmap_git->pack->revindex[i];\n+\t\tsha1 = nth_packed_object_sha1(bitmap_git->pack, entry->nr);\n \t\toe = packlist_find(mapping, sha1, NULL);\n \n \t\tif (oe)\n@@ -1042,7 +1071,7 @@ int rebuild_existing_bitmaps(struct packing_data *mapping,\n \tif (show_progress)\n \t\tprogress = start_progress(\"Reusing bitmaps\", 0);\n \n-\tkh_foreach_value(bitmap_git.bitmaps, stored, {\n+\tkh_foreach_value(bitmap_git->bitmaps, stored, {\n \t\tif (stored->flags & BITMAP_FLAG_REUSE) {\n \t\t\tif (!rebuild_bitmap(reposition,\n \t\t\t\t\t    lookup_stored_bitmap(stored),\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 5ded2f139..19f70043a 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -34,13 +34,20 @@ typedef int (*show_reachable_fn)(\n \tstruct packed_git *found_pack,\n \toff_t found_offset);\n \n-int prepare_bitmap_git(void);\n-void count_bitmap_commit_list(uint32_t *commits, uint32_t *trees, uint32_t *blobs, uint32_t *tags);\n-void traverse_bitmap_commit_list(show_reachable_fn show_reachable);\n+struct bitmap_index;\n+\n+struct bitmap_index *prepare_bitmap_git(void);\n+void count_bitmap_commit_list(struct bitmap_index *, uint32_t *commits,\n+\t\t\t      uint32_t *trees, uint32_t *blobs, uint32_t *tags);\n+void traverse_bitmap_commit_list(struct bitmap_index *,\n+\t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n-int prepare_bitmap_walk(struct rev_info *revs);\n-int reuse_partial_packfile_from_bitmap(struct packed_git **packfile, uint32_t *entries, off_t *up_to);\n-int rebuild_existing_bitmaps(struct packing_data *mapping, khash_sha1 *reused_bitmaps, int show_progress);\n+struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs);\n+int reuse_partial_packfile_from_bitmap(struct bitmap_index *,\n+\t\t\t\t       struct packed_git **packfile,\n+\t\t\t\t       uint32_t *entries, off_t *up_to);\n+int rebuild_existing_bitmaps(struct bitmap_index *, struct packing_data *mapping,\n+\t\t\t     khash_sha1 *reused_bitmaps, int show_progress);\n \n void bitmap_writer_show_progress(int show);\n void bitmap_writer_set_checksum(unsigned char *sha1);\n-- \n2.17.0.768.g1526ddbba1.dirty\n\n"},{"id":"349772","messageId":"20180609060437.GA30224@sigill.intra.peff.net","threadId":"48672","inReplyTo":"d4d4fec265c1c53e14da465904d93e559ff2c17d.1528397984.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] pack-bitmap: remove bitmap_git global variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T06:04:38Z","receivedAt":"2018-06-09T06:04:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 07, 2018 at 12:04:13PM -0700, Jonathan Tan wrote:\n\n> Remove the bitmap_git global variable. Instead, generate on demand an\n> instance of struct bitmap_index for code that needs to access it.\n> \n> This allows us significant control over the lifetime of instances of\n> struct bitmap_index. In particular, packs can now be closed without\n> worrying if an unnecessarily long-lived \"pack\" field in struct\n> bitmap_index still points to it.\n> \n> The bitmap API is also clearer in that we need to first obtain a struct\n> bitmap_index, then we use it.\n\nI think this is the right direction, and overall it looks pretty good.\nThere's one call that gave me pause:\n\n> -int prepare_bitmap_git(void)\n> +struct bitmap_index *prepare_bitmap_git(void)\n>  {\n> -\tif (bitmap_git.loaded)\n> -\t\treturn 0;\n> +\tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n\nThis function used to be idempotent, so any code which wanted to use the\nglobal bitmap_git could call it \"just in case\". After your patch, it's\nnot. I think it's probably OK, since such functions would generally now\ntake a bitmap_git argument and use that (e.g., rebuild_existing_bitmaps\nworks that way after your patch).\n\n> -\tif (!open_pack_bitmap())\n> -\t\treturn load_pack_bitmap();\n> +\tif (!open_pack_bitmap(bitmap_git) && !load_pack_bitmap(bitmap_git))\n> +\t\treturn bitmap_git;\n>  \n> -\treturn -1;\n> +\treturn NULL;\n>  }\n\nWe probably need to free(bitmap_git) before returning NULL here (this is\nstill in prepare_bitmap_git()).\n\n> @@ -662,12 +686,11 @@ int prepare_bitmap_walk(struct rev_info *revs)\n>  \tstruct bitmap *wants_bitmap = NULL;\n>  \tstruct bitmap *haves_bitmap = NULL;\n>  \n> -\tif (!bitmap_git.loaded) {\n> -\t\t/* try to open a bitmapped pack, but don't parse it yet\n> -\t\t * because we may not need to use it */\n> -\t\tif (open_pack_bitmap() < 0)\n> -\t\t\treturn -1;\n> -\t}\n> +\tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n> +\t/* try to open a bitmapped pack, but don't parse it yet\n> +\t * because we may not need to use it */\n> +\tif (open_pack_bitmap(bitmap_git) < 0)\n> +\t\treturn NULL;\n\nDitto here (and probably other error returns lower in the function, but\nI didn't go through it carefully).\n\n-Peff\n"},{"id":"349773","messageId":"20180609060621.GB30224@sigill.intra.peff.net","threadId":"48672","inReplyTo":"b9be1d0371e5dc694e5e7ac88a900f985faabec8.1528397984.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] pack-bitmap: add free function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T06:06:21Z","receivedAt":"2018-06-09T06:06:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 07, 2018 at 12:04:14PM -0700, Jonathan Tan wrote:\n\n> Add a function to free struct bitmap_index instances, and use it where\n> needed (except when rebuild_existing_bitmaps() is used, since it creates\n> references to the bitmaps within the struct bitmap_index passed to it).\n> \n> Note that the hashes field in struct bitmap_index is not freed because\n> it points to another field within the same struct. The documentation for\n> that field has been updated to clarify that.\n\nHrm, this one fixes the leaks I noticed in the previous patch. I'm OK\nwith doing it separately if it breaking it down this way makes the code\neasier to reason about. But we should probably say something in the\ncommit message about it.\n\n-Peff\n"},{"id":"349928","messageId":"20180611115046.d03f8093347dc6c0e9b116fc@google.com","threadId":"48672","inReplyTo":"20180609060437.GA30224@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] pack-bitmap: remove bitmap_git global variable","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-11T18:50:46Z","receivedAt":"2018-06-11T18:50:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Sat, 9 Jun 2018 02:04:38 -0400\nJeff King <peff@peff.net> wrote:\n\n> This function used to be idempotent, so any code which wanted to use the\n> global bitmap_git could call it \"just in case\". After your patch, it's\n> not. I think it's probably OK, since such functions would generally now\n> take a bitmap_git argument and use that (e.g., rebuild_existing_bitmaps\n> works that way after your patch).\n\nYes, and I've checked that the existing callers do not use bitmaps\ntwice. I've appended an extra paragraph for the commit message to this\ne-mail below that will hopefully clarify things.\n\n> We probably need to free(bitmap_git) before returning NULL here (this is\n> still in prepare_bitmap_git()).\n\n[snip]\n\n> Ditto here (and probably other error returns lower in the function, but\n> I didn't go through it carefully).\n\nThe contents of the struct also need to be handled, and I wanted to\nhandle it all at once in the subsequent patch. But you're right that the\ncommit message should explain that.\n\nHere's an paragraph to be added to the end of the commit message. I can\nsend a reroll with the exact same code but with the updated commit\nmessage if Junio requests it.\n\n[additional paragraph begin]\n\nThis patch raises two potential issues: (1) memory for the struct\nbitmap_index is allocated without being freed, and (2)\nprepare_bitmap_git() and prepare_bitmap_walk() can reuse a previously\nloaded bitmap. For (1), this will be dealt with in a subsequent patch in\nthis patch set that also deals with freeing the contents of the struct\nbitmap_index (which were not freed previously, because they have global\nscope). For (2), current bitmap users only load the bitmap once at most\n(note that pack-objects can use bitmaps or write bitmaps, but not both\nat the same time), so support for reuse has no effect - and future users\ncan pass around the struct bitmap_index * obtained if they need to do 2\nor more things with the same bitmap.\n\n[additional paragraph end]\n"},{"id":"349943","messageId":"20180611211033.GB26235@sigill.intra.peff.net","threadId":"48672","inReplyTo":"20180611115046.d03f8093347dc6c0e9b116fc@google.com","subject":"Re: [PATCH 1/2] pack-bitmap: remove bitmap_git global variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T21:10:34Z","receivedAt":"2018-06-11T21:10:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 11, 2018 at 11:50:46AM -0700, Jonathan Tan wrote:\n\n> Here's an paragraph to be added to the end of the commit message. I can\n> send a reroll with the exact same code but with the updated commit\n> message if Junio requests it.\n> \n> [additional paragraph begin]\n> \n> This patch raises two potential issues: (1) memory for the struct\n> bitmap_index is allocated without being freed, and (2)\n> prepare_bitmap_git() and prepare_bitmap_walk() can reuse a previously\n> loaded bitmap. For (1), this will be dealt with in a subsequent patch in\n> this patch set that also deals with freeing the contents of the struct\n> bitmap_index (which were not freed previously, because they have global\n> scope). For (2), current bitmap users only load the bitmap once at most\n> (note that pack-objects can use bitmaps or write bitmaps, but not both\n> at the same time), so support for reuse has no effect - and future users\n> can pass around the struct bitmap_index * obtained if they need to do 2\n> or more things with the same bitmap.\n> \n> [additional paragraph end]\n\nYeah, that sounds fine to me. Thanks.\n\n-Peff\n"},{"id":"349959","messageId":"CAGZ79kbeB_ppTY6cqvxSPsmFghfYaXypS+fjyDr9Ukxax5jz2Q@mail.gmail.com","threadId":"48672","inReplyTo":"cover.1528397984.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/2] Object store refactoring: make bitmap_git not global","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-12T00:00:47Z","receivedAt":"2018-06-12T00:01:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 7, 2018 at 12:04 PM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> This is a continuation of the object store refactoring effort.\n>\n> We cannot truly free an object store without ensuring that any generated\n> bitmaps are first freed, so here are patches to drastically reduce the\n> lifetime of any bitmaps generated. As a bonus, the API is also improved,\n> and global state reduced.\n>\n\nThanks for writing this series, I have no comments on it.\n\nThanks,\nStefan\n"},{"id":"350877","messageId":"20180625182231.GD19910@google.com","threadId":"48672","inReplyTo":"cover.1528397984.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/2] Object store refactoring: make bitmap_git not global","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-25T18:22:31Z","receivedAt":"2018-06-25T18:22:37Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/07, Jonathan Tan wrote:\n> This is a continuation of the object store refactoring effort.\n> \n> We cannot truly free an object store without ensuring that any generated\n> bitmaps are first freed, so here are patches to drastically reduce the\n> lifetime of any bitmaps generated. As a bonus, the API is also improved,\n> and global state reduced.\n\nI've reviewed this series and haven't found any issues.\n\nReviewed-by: Brandon Williams <bmwill@google.com>\n\n-- \nBrandon Williams\n"}]}