Re: [PATCH 6/6] pack-bitmap: introduce `bitmap_writer_free()`

From: Patrick Steinhardt <ps@pks.im>
Date: May 15, 2024, 09:05 UTC
Message-ID: <ZkR6zkUfW6Fosqyn@tanuki>
In-Reply-To: <bf65967764f34adc2ca00d4c8195840ad3e4e127.1715716605.git.me@ttaylorr.com>

On Tue, May 14, 2024 at 03:57:06PM -0400, Taylor Blau wrote:

> Now that there is clearer memory ownership around the bitmap_writer
> structure, introduce a bitmap_writer_free() function that callers may
> use to free any memory associated with their instance of the
> bitmap_writer structure.

Great. I wanted to ask about this in preceding commits already, good to see that you already thought of if.

Show 28 quoted lines

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  builtin/pack-objects.c |  3 ++-
>  midx-write.c           |  1 +
>  pack-bitmap-write.c    | 23 +++++++++++++++++++++++
>  pack-bitmap.h          |  1 +
>  4 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 10e69fdc8e..26a6d0d791 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1245,7 +1245,6 @@ static void write_pack_file(void)
>  	uint32_t nr_remaining = nr_result;
>  	time_t last_mtime = 0;
>  	struct object_entry **write_order;
> -	struct bitmap_writer bitmap_writer;
>  
>  	if (progress > pack_to_stdout)
>  		progress_state = start_progress(_("Writing objects"), nr_result);
> @@ -1315,6 +1314,7 @@ static void write_pack_file(void)
>  		if (!pack_to_stdout) {
>  			struct stat st;
>  			struct strbuf tmpname = STRBUF_INIT;
> +			struct bitmap_writer bitmap_writer;
>  			char *idx_tmp_name = NULL;
>  
>  			/*

Nit: we could have avoided moving the struct if it was introduced in
this spot in the preceding patch.

[snip]

Show 29 quoted lines

> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 420f17c2e0..6cae670412 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -32,6 +32,29 @@ void bitmap_writer_init(struct bitmap_writer *writer)
>  	memset(writer, 0, sizeof(struct bitmap_writer));
>  }
>  
> +void bitmap_writer_free(struct bitmap_writer *writer)
> +{
> +	uint32_t i;
> +
> +	if (!writer)
> +		return;
> +
> +	ewah_free(writer->commits);
> +	ewah_free(writer->trees);
> +	ewah_free(writer->blobs);
> +	ewah_free(writer->tags);
> +
> +	kh_destroy_oid_map(writer->bitmaps);
> +
> +	for (i = 0; i < writer->selected_nr; i++) {
> +		struct bitmapped_commit *bc = &writer->selected[i];
> +		if (bc->write_as != bc->bitmap)
> +			ewah_free(bc->write_as);
> +		ewah_free(bc->bitmap);
> +	}
> +	free(writer->selected);

I was wondering whether we also want to set all those fields to `NULL` at the end, or just `memset()` the whole structure. But there probably isn't much of a reason to do it currently, so I don't mind if your answer is "no".

Patrick