Re: [PATCH 2/2] pack-bitmap: add free function

[Jeff King]

On Thu, Jun 07, 2018 at 12:04:14PM -0700, Jonathan Tan wrote:

> Add a function to free struct bitmap_index instances, and use it where
> needed (except when rebuild_existing_bitmaps() is used, since it creates
> references to the bitmaps within the struct bitmap_index passed to it).
> 
> Note that the hashes field in struct bitmap_index is not freed because
> it points to another field within the same struct. The documentation for
> that field has been updated to clarify that.

Hrm, this one fixes the leaks I noticed in the previous patch. I'm OK
with doing it separately if it breaking it down this way makes the code
easier to reason about. But we should probably say something in the
commit message about it.

-Peff

[PATCH 2/2] pack-bitmap: add free function

[Jonathan Tan]

Add a function to free struct bitmap_index instances, and use it where
needed (except when rebuild_existing_bitmaps() is used, since it creates
references to the bitmaps within the struct bitmap_index passed to it).

Note that the hashes field in struct bitmap_index is not freed because
it points to another field within the same struct. The documentation for
that field has been updated to clarify that.

Signed-off-by: Jonathan Tan 
---
 builtin/pack-objects.c |  1 +
 builtin/rev-list.c |  2 ++
 pack-bitmap-write.c|  4 
 pack-bitmap.c  | 35 +--
 pack-bitmap.h  |  1 +
 5 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d064f944b..896e41300 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2945,6 +2945,7 @@ static int get_object_list_from_bitmap(struct rev_info 
*revs)
}
 
traverse_bitmap_commit_list(bitmap_git, _object_entry_from_bitmap);
+   free_bitmap_index(bitmap_git);
return 0;
 }
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index cce42ae1d..62776721f 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -521,6 +521,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
if (max_count >= 0 && max_count < commit_count)
commit_count = max_count;
printf("%d\n", commit_count);
+   free_bitmap_index(bitmap_git);
return 0;
}
} else if (revs.max_count < 0 &&
@@ -528,6 +529,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
struct bitmap_index *bitmap_git;
if ((bitmap_git = prepare_bitmap_walk())) {
traverse_bitmap_commit_list(bitmap_git, 
_object_fast);
+   free_bitmap_index(bitmap_git);
return 0;
}
}
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 03e122563..7896fedd3 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -367,6 +367,10 @@ void bitmap_writer_reuse_bitmaps(struct packing_data 
*to_pack)
writer.reused = kh_init_sha1();
rebuild_existing_bitmaps(bitmap_git, to_pack, writer.reused,
 writer.show_progress);
+   /*
+* NEEDSWORK: rebuild_existing_bitmaps() makes writer.reused reference
+* some bitmaps in bitmap_git, so we can't free the latter.
+*/
 }
 
 static struct ewah_bitmap *find_reused_bitmap(const unsigned char *sha1)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 7795444b0..c06b19f49 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -66,7 +66,7 @@ struct bitmap_index {
/* Number of bitmapped commits */
uint32_t entry_count;
 
-   /* Name-hash cache (or NULL if not present). */
+   /* If not NULL, this is a name-hash cache pointing into map. */
uint32_t *hashes;
 
/*
@@ -350,6 +350,7 @@ struct bitmap_index *prepare_bitmap_git(void)
if (!open_pack_bitmap(bitmap_git) && !load_pack_bitmap(bitmap_git))
return bitmap_git;
 
+   free_bitmap_index(bitmap_git);
return NULL;
 }
 
@@ -690,7 +691,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info 
*revs)
/* try to open a bitmapped pack, but don't parse it yet
 * because we may not need to use it */
if (open_pack_bitmap(bitmap_git) < 0)
-   return NULL;
+   goto cleanup;
 
for (i = 0; i < revs->pending.nr; ++i) {
struct object *object = revs->pending.objects[i].item;
@@ -723,11 +724,11 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info 
*revs)
 * optimize here
 */
if (haves && !in_bitmapped_pack(bitmap_git, haves))
-   return NULL;
+   goto cleanup;
 
/* if we don't want anything, we're done here */
if (!wants)
-   return NULL;
+   goto cleanup;
 
/*
 * now we're going to use bitmaps, so load the actual bitmap entries
@@ -735,7 +736,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info 
*revs)
 * becomes invalidated and we must perform the revwalk through bitmaps
 */
if (!bitmap_git->loaded && load_pack_bitmap(bitmap_git) < 0)
-   return NULL;
+   goto cleanup;
 
object_array_clear(>pending);
 
@@ -761,6 +762,10 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info 
*revs)
 
bitmap_free(haves_bitmap);
return bitmap_git;
+
+cleanup:
+   free_bitmap_index(bitmap_git);
+   return NULL;
 }
 
 int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
@@ -1001,7 +1006,7 @@ void