Re: What's cooking in git.git (Oct 2021, #02; Wed, 6)

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Oct 06 2021, Jeff King wrote:

> On Wed, Oct 06, 2021 at 05:24:14PM -0700, Junio C Hamano wrote:
>
>> * tb/repack-write-midx (2021-10-01) 9 commits
>>   (merged to 'next' on 2021-10-06 at ccdd5aaf2a)
>>  + builtin/repack.c: pass `--refs-snapshot` when writing bitmaps
>>  + builtin/repack.c: make largest pack preferred
>>  + builtin/repack.c: support writing a MIDX while repacking
>>  + builtin/repack.c: extract showing progress to a variable
>>  + builtin/repack.c: rename variables that deal with non-kept packs
>>  + builtin/repack.c: keep track of existing packs unconditionally
>>  + midx: preliminary support for `--refs-snapshot`
>>  + builtin/multi-pack-index.c: support `--stdin-packs` mode
>>  + midx: expose `write_midx_file_only()` publicly
>> 
>>  "git repack" has been taught to generate multi-pack reachability
>>  bitmaps.
>> 
>>  Will merge to 'master'.
>
> Sorry not to catch this before it hit 'next', but there's a small leak
> in the test helper. This patch can go on top to fix it.
>
> -- >8 --
> Subject: [PATCH] test-read-midx: fix leak of bitmap_index struct
>
> In read_midx_preferred_pack(), we open the bitmap index but never free
> it. This isn't a big deal since this is just a test helper, and we exit
> immediately after, but since we're trying to keep our leak-checking tidy
> now, it's worth fixing.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/helper/test-read-midx.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
> index 0038559129..9d6fa7a377 100644
> --- a/t/helper/test-read-midx.c
> +++ b/t/helper/test-read-midx.c
> @@ -85,11 +85,15 @@ static int read_midx_preferred_pack(const char *object_dir)
>  		return 1;
>  
>  	bitmap = prepare_bitmap_git(the_repository);
> -	if (!(bitmap && bitmap_is_midx(bitmap)))
> +	if (!bitmap)
>  		return 1;
> -
> +	if (!bitmap_is_midx(bitmap)) {
> +		free_bitmap_index(bitmap);
> +		return 1;
> +	}
>  
>  	printf("%s\n", midx->pack_names[midx_preferred_pack(bitmap)]);
> +	free_bitmap_index(bitmap);
>  	return 0;
>  }

Thanks, I think it's no big deal, those tests seem to leak a lot
already. Here's a patch that might be generally applicable and makes a
few more of its tests pass.

The s/free/free_chunkfile/g seems like a good bug fix, and that "m =
NULL" pattern seems odd, don't we always want to free in that scenario?
It passes all tests...

This brings t5319-multi-pack-index.sh down from 64 to 57 failures in
"next", I didn't try in combination with your patch.

diff --git a/midx.c b/midx.c
index 7e06e859756..d24b1e6a9d4 100644
--- a/midx.c
+++ b/midx.c
@@ -179,12 +179,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 	trace2_data_intmax("midx", the_repository, "load/num_packs", m->num_packs);
 	trace2_data_intmax("midx", the_repository, "load/num_objects", m->num_objects);
 
+	free_chunkfile(cf);
 	return m;
 
 cleanup_fail:
 	free(m);
 	free(midx_name);
-	free(cf);
+	free_chunkfile(cf);
 	if (midx_map)
 		munmap(midx_map, midx_size);
 	if (0 <= fd)
@@ -1602,7 +1603,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 		 * Remaining tests assume that we have objects, so we can
 		 * return here.
 		 */
-		return verify_midx_error;
+		goto cleanup;
 	}
 
 	if (flags & MIDX_PROGRESS)
@@ -1679,8 +1680,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 		midx_display_sparse_progress(progress, i + 1);
 	}
 	stop_progress(&progress);
-
+cleanup:
 	free(pairs);
+	free(m);
 
 	return verify_midx_error;
 }
@@ -1927,11 +1929,9 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 	}
 
 	result = write_midx_internal(object_dir, NULL, NULL, NULL, NULL, flags);
-	m = NULL;
 
 cleanup:
-	if (m)
-		close_midx(m);
+	close_midx(m);
 	free(include_pack);
 	return result;
 }