Re: [PATCH v5] pack-bitmap: remove checks before bitmap_free

[Junio C Hamano <gitster@pobox.com>]

"Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed
> if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only
> use roots_bitmap as a mutable reference but not takes roots_bitmap's
> ownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.

"Once cascade_pseudo_merges_1() succeeds", perhaps?

> And this leak currently lacks a dedicated test to detect it.
>
> To fix this leak, remove if cascade_pseudo_merges_1() succeed check and
> always calling bitmap_free(roots_bitmap);
>
> To trigger this leak, we need roots_bitmap contains at least one pseudo
> merge.

"contains" -> "that contains"?

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index ac6d62b980c..8727f316de9 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,
>  			bitmap_set(roots_bitmap, pos);
>  		}
>  
> -		if (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))
> -			bitmap_free(roots_bitmap);
> +		cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);
> +		bitmap_free(roots_bitmap);

This makes it as if the original _wanted_ to leak it when the call
failed.  Readers may wonder how we got into this state in the first
place.  Was it a simple thinko when 11d45a6e (pack-bitmap.c: use
pseudo-merges during traversal, 2024-05-23) was written, I have to
wonder.

> diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh
> index 56674db562f..ba5ae6a00c9 100755
> --- a/t/t5333-pseudo-merge-bitmaps.sh
> +++ b/t/t5333-pseudo-merge-bitmaps.sh
> @@ -445,4 +445,21 @@ test_expect_success 'pseudo-merge closure' '
>  	)
>  '
>  
> +test_expect_success 'use pseudo-merge in boundary traversal' '
> +	git init pseudo-merge-boundary-traversal &&
> +	(
> +		cd pseudo-merge-boundary-traversal &&
> +
> +		git config bitmapPseudoMerge.test.pattern refs/ &&
> +		git config pack.useBitmapBoundaryTraversal true &&

Yup, this is a temporary repository for only this test, so using
"git config" there makes it the simplest and the easiest to
understand.

> +		test_commit A &&
> +		git repack -adb &&
> +		test_commit B &&
> +
> +		nr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&
> +		test 1 -eq "$nr"
> +	)
> +'
> +
>  test_done
>
> base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229