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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2025, 15:29 UTC
Message-ID: <xmqqldq69phe.fsf@gitster.g>
In-Reply-To: <pull.1977.v5.git.git.1749104667618.gitgitgadget@gmail.com>

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

Show 6 quoted lines

> 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?

Show 7 quoted lines

> 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"?

Show 12 quoted lines

> 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.

Show 15 quoted lines

> 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.

Show 12 quoted lines

> +		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