"Lidong Yan via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx> > > 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
