{"thread":{"id":"63518","subject":"[PATCH] pack-bitmap: remove checks before bitmap_free","startedAt":"2025-05-25T05:09:46Z","lastAt":"2025-06-10T05:59:28Z","messageCount":28,"participants":["Lidong Yan via GitGitGadget","Patrick Steinhardt","lidongyan","Junio C Hamano","Eric Sunshine","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518855","messageId":"pull.1977.git.git.1748149783383.gitgitgadget@gmail.com","threadId":"63518","inReplyTo":null,"subject":"[PATCH] pack-bitmap: remove checks before bitmap_free","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-25T05:09:43Z","receivedAt":"2025-05-25T05:09:46Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn pack-bitmap.c:find_boundary_objects, we build a roots_bitmap and\ncascade it to cb.base. However, I’m wondering why we only free\nroots_bitmap when the cascade succeeds. It seems we could safely remove\nthis check and always free roots_bitmap afterward, which might provide\nsome performance benefits.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    pack-bitmap: remove checks before bitmap_free\n    \n    In pack-bitmap.c:find_boundary_objects, remove cascade success check and\n    always free roots_bitmap afterward to make static analysis tool works\n    better.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1977%2Fbrandb97%2Fremove-check-before-bitmap-free-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1977/brandb97/remove-check-before-bitmap-free-v1\nPull-Request: https://github.com/git/git/pull/1977\n\n pack-bitmap.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c..8727f316de9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n \t\t\tbitmap_set(roots_bitmap, pos);\n \t\t}\n \n-\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n-\t\t\tbitmap_free(roots_bitmap);\n+\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n+\t\tbitmap_free(roots_bitmap);\n \t}\n \n \t/*\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n-- \ngitgitgadget\n"},{"id":"518904","messageId":"aDQO4Vkj7POztMnC@pks.im","threadId":"63518","inReplyTo":"pull.1977.git.git.1748149783383.gitgitgadget@gmail.com","subject":"Re: [PATCH] pack-bitmap: remove checks before bitmap_free","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-26T06:49:05Z","receivedAt":"2025-05-26T06:49:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 25, 2025 at 05:09:43AM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> In pack-bitmap.c:find_boundary_objects, we build a roots_bitmap and\n> cascade it to cb.base. However, I’m wondering why we only free\n> roots_bitmap when the cascade succeeds. It seems we could safely remove\n> this check and always free roots_bitmap afterward, which might provide\n> some performance benefits.\n\nThis commit message isn't quite a convincing one. As author of a patch\nthe onus falls on you to explain why the change is sensible, but even\nmore importantly it also falls on you to explain why it is correct.\n\nIt is of course fine to ask for help and input, but in that case you\nshould probably mark the patch accordingly, for example with the RFC\ntag.\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index ac6d62b980c..8727f316de9 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n>  \t\t\tbitmap_set(roots_bitmap, pos);\n>  \t\t}\n>  \n> -\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n> -\t\t\tbitmap_free(roots_bitmap);\n> +\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n> +\t\tbitmap_free(roots_bitmap);\n>  \t}\n\nWe know that `roots_bitmap` is always allocated via `bitmap_new()`, so\nit won't ever be a `NULL` pointer and should in theory always be free'd.\nFurthermore, we know that the pointer never escapes the local scope,\neither.\n\nThe next question would thus be: what does `cascade_pseudo_merges_1()`\ndo with the bitmap? Are there situations where it does free it for us,\nor where it moves ownership of that bitmap? So let's go down the call\nchain:\n\n  - `cascade_pseudo_merges_1()` passes it on to\n    `cascade_pseudo_merges()`.\n\n  - `cascade_pseudo_merges()` passes it on to `apply_pseudo_merge()`.\n\n`apply_pseudo_merge()` itself then checks whether the pseudo-merge is a\nsubset of the `roots_bitmap` and, if not, ORs the pseudo-merge into it.\n\nNone of these operations move around ownership or free the bitmap, so\nthis looks like a true memory leak in case `cascade_pseudo_merges_1()`\nreturns non-zero. Which would raise another question: when exactly does\nit return non-zero, and can we trigger the memory leak via a test?\n\nInformation like this should ideally be part of the commit message\nitself. It helps reviewers to figure out _why_ a change is correct and,\nif anybody were to dig into history, would also help them to have enough\ncontext.\n\nThanks!\n\nPatrick\n"},{"id":"518949","messageId":"3F8C41C7-8DCF-408A-AB81-B77B46D20FC9@smail.nju.edu.cn","threadId":"63518","inReplyTo":"aDQO4Vkj7POztMnC@pks.im","subject":"Re: [PATCH] pack-bitmap: remove checks before bitmap_free","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-26T16:05:39Z","receivedAt":"2025-05-26T16:06:12Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年5月26日 14:49，Patrick Steinhardt <ps@pks.im> 写道：\n> \n> This commit message isn't quite a convincing one. As author of a patch\n> the onus falls on you to explain why the change is sensible, but even\n> more importantly it also falls on you to explain why it is correct.\n> \n> It is of course fine to ask for help and input, but in that case you\n> should probably mark the patch accordingly, for example with the RFC\n> tag.\n\nThank you for the suggestion. Please allow me to keep the patch in its\n current form this time. \n\n> We know that `roots_bitmap` is always allocated via `bitmap_new()`, so\n> it won't ever be a `NULL` pointer and should in theory always be free'd.\n> Furthermore, we know that the pointer never escapes the local scope,\n> either.\n> \n> The next question would thus be: what does `cascade_pseudo_merges_1()`\n> do with the bitmap? Are there situations where it does free it for us,\n> or where it moves ownership of that bitmap? So let's go down the call\n> chain:\n> \n>  - `cascade_pseudo_merges_1()` passes it on to\n>    `cascade_pseudo_merges()`.\n> \n>  - `cascade_pseudo_merges()` passes it on to `apply_pseudo_merge()`.\n> \n> `apply_pseudo_merge()` itself then checks whether the pseudo-merge is a\n> subset of the `roots_bitmap` and, if not, ORs the pseudo-merge into it.\n\nI would put this into commit message. I also noticed that `find_objects()` in\npack-bitmap.c has similar code but without `if (cascade_pseudo_merges_1)`.\n\n> \n> None of these operations move around ownership or free the bitmap, so\n> this looks like a true memory leak in case `cascade_pseudo_merges_1()`\n> returns non-zero. Which would raise another question: when exactly does\n> it return non-zero, and can we trigger the memory leak via a test?\n\nSeems we need to make `bitmap_git->pseudo_merges->v[I]` contains some\nobjects which doesn’t exist in `roots`. I’ll try to figure it out in the next patch. \n\nThanks\nLidong\n\n"},{"id":"519285","messageId":"pull.1977.v2.git.git.1748628846.gitgitgadget@gmail.com","threadId":"63518","inReplyTo":"pull.1977.git.git.1748149783383.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] pack-bitmap: remove checks before bitmap_free","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-30T18:14:04Z","receivedAt":"2025-05-30T18:14:10Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"In pack-bitmap.c:find_boundary_objects, remove cascade success check and\nalways free roots_bitmap afterward to make static analysis tool works\nbetter.\n\nLidong Yan (2):\n  pack-bitmap: remove checks before bitmap_free\n  t5333: test memory leak when use pseudo-merge in boundary traversal\n\n pack-bitmap.c                   |  4 ++--\n t/t5333-pseudo-merge-bitmaps.sh | 20 ++++++++++++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1977%2Fbrandb97%2Fremove-check-before-bitmap-free-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1977/brandb97/remove-check-before-bitmap-free-v2\nPull-Request: https://github.com/git/git/pull/1977\n\nRange-diff vs v1:\n\n 1:  19677bcbc3d ! 1:  d7b7a0e29ec pack-bitmap: remove checks before bitmap_free\n     @@ Commit message\n          pack-bitmap: remove checks before bitmap_free\n      \n          In pack-bitmap.c:find_boundary_objects, we build a roots_bitmap and\n     -    cascade it to cb.base. However, I’m wondering why we only free\n     -    roots_bitmap when the cascade succeeds. It seems we could safely remove\n     -    this check and always free roots_bitmap afterward, which might provide\n     -    some performance benefits.\n     +    cascade it to cb.base. Only when cascade failed, roots_bitmap is\n     +    freed otherwise it leaks. Since cascade_pseudo_merges_1() only use\n     +    roots_bitmap as a mutable reference not takes roots_bitmap's ownership\n     +    we'd better remove `if(cascade_pseudo_merges_1)` and frees roots_bitmap\n     +    anyway.\n      \n          Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n      \n -:  ----------- > 2:  56b24d681cb t5333: test memory leak when use pseudo-merge in boundary traversal\n\n-- \ngitgitgadget\n"},{"id":"519286","messageId":"56b24d681cbcedaf5c03c89eee582d554a0894b7.1748628847.git.gitgitgadget@gmail.com","threadId":"63518","inReplyTo":"pull.1977.v2.git.git.1748628846.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] t5333: test memory leak when use pseudo-merge in boundary traversal","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-30T18:14:06Z","receivedAt":"2025-05-30T18:14:11Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\nif cascade_pseudo_merges_1() fails. Otherwise, it leaks, leading to\na memory leak that currently lacks a dedicated test to detect it.\n\nTo trigger this leak, we need a pseudo-merge whose size is equal to\nor smaller than roots_bitmap (which corresponds to the set of \"haves\"\ncommits in prepare_bitmap_walk()). To do this, we can create two\ncommits: A and B. Add A to the pseudo-merge list and perform a traversal\nover the range A..B. In this scenario, the \"haves\" set will be {A},\nand cascade_pseudo_merges_1() will succeed — thereby exposing the leak\ndue to the missing roots_bitmap cleanup.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n t/t5333-pseudo-merge-bitmaps.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\nindex 56674db562f9..5e263fce50a7 100755\n--- a/t/t5333-pseudo-merge-bitmaps.sh\n+++ b/t/t5333-pseudo-merge-bitmaps.sh\n@@ -445,4 +445,24 @@ test_expect_success 'pseudo-merge closure' '\n \t)\n '\n \n+test_expect_success 'use pseudo-merge in boundary traversal' '\n+\tgit init pseudo-merge-boundary-traversal &&\n+\t(\n+\t\tcd pseudo-merge-boundary-traversal &&\n+\n+\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n+\t\tgit config bitmapPseudoMerge.test.threshold now &&\n+\t\tgit config bitmapPseudoMerge.test.stableThreshold now &&\n+\t\texport GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n+\n+\t\ttest_commit A &&\n+\t\tgit repack -adb &&\n+\t\ttest_commit B &&\n+\n+\t\techo '1' >expect &&\n+\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"519287","messageId":"d7b7a0e29ec0dc92e491401bc0dacfa15d4af2ad.1748628847.git.gitgitgadget@gmail.com","threadId":"63518","inReplyTo":"pull.1977.v2.git.git.1748628846.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] pack-bitmap: remove checks before bitmap_free","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-30T18:14:05Z","receivedAt":"2025-05-30T18:14:11Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn pack-bitmap.c:find_boundary_objects, we build a roots_bitmap and\ncascade it to cb.base. Only when cascade failed, roots_bitmap is\nfreed otherwise it leaks. Since cascade_pseudo_merges_1() only use\nroots_bitmap as a mutable reference not takes roots_bitmap's ownership\nwe'd better remove `if(cascade_pseudo_merges_1)` and frees roots_bitmap\nanyway.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c5..8727f316de92 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n \t\t\tbitmap_set(roots_bitmap, pos);\n \t\t}\n \n-\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n-\t\t\tbitmap_free(roots_bitmap);\n+\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n+\t\tbitmap_free(roots_bitmap);\n \t}\n \n \t/*\n-- \ngitgitgadget\n\n"},{"id":"519292","messageId":"xmqqjz5xerl4.fsf@gitster.g","threadId":"63518","inReplyTo":"pull.1977.v2.git.git.1748628846.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] pack-bitmap: remove checks before bitmap_free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-30T21:06:31Z","receivedAt":"2025-05-30T21:06:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> In pack-bitmap.c:find_boundary_objects, remove cascade success check and\n> always free roots_bitmap afterward to make static analysis tool works\n> better.\n>\n> Lidong Yan (2):\n>   pack-bitmap: remove checks before bitmap_free\n>   t5333: test memory leak when use pseudo-merge in boundary traversal\n\nHow would these two commits relate to each other?  If [2/2] is a\ntest that exposes existing breakage if [1/2] weren't there, we\nusually have them in the same commit.  If they are not related, of\ncourse, they can be applied and advanced independently.\n"},{"id":"519295","messageId":"xmqqa56tepx8.fsf@gitster.g","threadId":"63518","inReplyTo":"56b24d681cbcedaf5c03c89eee582d554a0894b7.1748628847.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] t5333: test memory leak when use pseudo-merge in boundary traversal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-30T21:42:27Z","receivedAt":"2025-05-30T21:42:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +\t\texport GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n\nThe test linter complains on this line for me, it seems.\n\nI've ran out of time for today's integration cycle, so this topic\nwill not be in what I'll push out later this afternoon.\n\nThanks.\n"},{"id":"519296","messageId":"CAPig+cSv8ADqERwZBZ_7OXnedGPR_iwRa0Z-NtEBHxS2Zc8EjQ@mail.gmail.com","threadId":"63518","inReplyTo":"xmqqa56tepx8.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] t5333: test memory leak when use pseudo-merge in boundary traversal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-05-30T21:50:20Z","receivedAt":"2025-05-30T21:50:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 30, 2025 at 5:42 PM Junio C Hamano <gitster@pobox.com> wrote:\n> \"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> > +             export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n>\n> The test linter complains on this line for me, it seems.\n\nTo provide a bit more context:\n\n    % (cd t && make test-lint-shell-syntax)\n\ntells you that `export FOO=bar` is not portable and that it should\ninstead be written as:\n\n    FOO=bar &&\n    export FOO &&\n"},{"id":"519302","messageId":"E2FF23EF-004D-45CC-85CD-5FDB1E375213@smail.nju.edu.cn","threadId":"63518","inReplyTo":"CAPig+cSv8ADqERwZBZ_7OXnedGPR_iwRa0Z-NtEBHxS2Zc8EjQ@mail.gmail.com","subject":"Re: [PATCH v2 2/2] t5333: test memory leak when use pseudo-merge in boundary traversal","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-31T03:18:07Z","receivedAt":"2025-05-31T03:18:47Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年5月31日 05:50，Eric Sunshine <sunshine@sunshineco.com> 写道：\n> \n> On Fri, May 30, 2025 at 5:42 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> \"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>> +             export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n>> \n>> The test linter complains on this line for me, it seems.\n> \n> To provide a bit more context:\n> \n>    % (cd t && make test-lint-shell-syntax)\n\nThanks, I should run this before submit.\n\n> \n> tells you that `export FOO=bar` is not portable and that it should\n> instead be written as:\n> \n>    FOO=bar &&\n>    export FOO &&\n> \n\n"},{"id":"519521","messageId":"pull.1977.v3.git.git.1748915181113.gitgitgadget@gmail.com","threadId":"63518","inReplyTo":"pull.1977.v2.git.git.1748628846.gitgitgadget@gmail.com","subject":"[PATCH v3] pack-bitmap: remove checks before bitmap_free","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-03T01:46:20Z","receivedAt":"2025-06-03T01:46:24Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\nif cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\nuse roots_bitmap as a mutable reference but not takes roots_bitmap's\nownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.\nAnd this leak currently lacks a dedicated test to detect it.\n\nTo fix this leak, remove if cascade_pseudo_merges_1() succeed check and\nalways calling bitmap_free(roots_bitmap);\n\nTo trigger this leak, we need a pseudo-merge whose size is equal to\nor smaller than roots_bitmap (which corresponds to the set of \"haves\"\ncommits in prepare_bitmap_walk()). To do this, we can create two\ncommits: A and B. Add A to the pseudo-merge list and perform a traversal\nover the range A..B. In this scenario, the \"haves\" set will be {A},\nand cascade_pseudo_merges_1() will succeed, thereby exposing the leak\ndue to the missing roots_bitmap cleanup.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    pack-bitmap: remove checks before bitmap_free\n    \n    In pack-bitmap.c:find_boundary_objects, remove cascade success check and\n    always free roots_bitmap afterward to make static analysis tool works\n    better.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1977%2Fbrandb97%2Fremove-check-before-bitmap-free-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1977/brandb97/remove-check-before-bitmap-free-v3\nPull-Request: https://github.com/git/git/pull/1977\n\nRange-diff vs v2:\n\n 1:  d7b7a0e29ec < -:  ----------- pack-bitmap: remove checks before bitmap_free\n 2:  56b24d681cb ! 1:  151a7f5dc70 t5333: test memory leak when use pseudo-merge in boundary traversal\n     @@ Metadata\n      Author: Lidong Yan <502024330056@smail.nju.edu.cn>\n      \n       ## Commit message ##\n     -    t5333: test memory leak when use pseudo-merge in boundary traversal\n     +    pack-bitmap: remove checks before bitmap_free\n      \n          In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\n     -    if cascade_pseudo_merges_1() fails. Otherwise, it leaks, leading to\n     -    a memory leak that currently lacks a dedicated test to detect it.\n     +    if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\n     +    use roots_bitmap as a mutable reference but not takes roots_bitmap's\n     +    ownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.\n     +    And this leak currently lacks a dedicated test to detect it.\n     +\n     +    To fix this leak, remove if cascade_pseudo_merges_1() succeed check and\n     +    always calling bitmap_free(roots_bitmap);\n      \n          To trigger this leak, we need a pseudo-merge whose size is equal to\n          or smaller than roots_bitmap (which corresponds to the set of \"haves\"\n          commits in prepare_bitmap_walk()). To do this, we can create two\n          commits: A and B. Add A to the pseudo-merge list and perform a traversal\n          over the range A..B. In this scenario, the \"haves\" set will be {A},\n     -    and cascade_pseudo_merges_1() will succeed — thereby exposing the leak\n     +    and cascade_pseudo_merges_1() will succeed, thereby exposing the leak\n          due to the missing roots_bitmap cleanup.\n      \n          Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n      \n     + ## pack-bitmap.c ##\n     +@@ pack-bitmap.c: static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n     + \t\t\tbitmap_set(roots_bitmap, pos);\n     + \t\t}\n     + \n     +-\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n     +-\t\t\tbitmap_free(roots_bitmap);\n     ++\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n     ++\t\tbitmap_free(roots_bitmap);\n     + \t}\n     + \n     + \t/*\n     +\n       ## t/t5333-pseudo-merge-bitmaps.sh ##\n      @@ t/t5333-pseudo-merge-bitmaps.sh: test_expect_success 'pseudo-merge closure' '\n       \t)\n     @@ t/t5333-pseudo-merge-bitmaps.sh: test_expect_success 'pseudo-merge closure' '\n      +\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n      +\t\tgit config bitmapPseudoMerge.test.threshold now &&\n      +\t\tgit config bitmapPseudoMerge.test.stableThreshold now &&\n     -+\t\texport GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n     ++\t\tGIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n      +\n      +\t\ttest_commit A &&\n      +\t\tgit repack -adb &&\n\n\n pack-bitmap.c                   |  4 ++--\n t/t5333-pseudo-merge-bitmaps.sh | 20 ++++++++++++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c..8727f316de9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n \t\t\tbitmap_set(roots_bitmap, pos);\n \t\t}\n \n-\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n-\t\t\tbitmap_free(roots_bitmap);\n+\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n+\t\tbitmap_free(roots_bitmap);\n \t}\n \n \t/*\ndiff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\nindex 56674db562f..454f8c7a817 100755\n--- a/t/t5333-pseudo-merge-bitmaps.sh\n+++ b/t/t5333-pseudo-merge-bitmaps.sh\n@@ -445,4 +445,24 @@ test_expect_success 'pseudo-merge closure' '\n \t)\n '\n \n+test_expect_success 'use pseudo-merge in boundary traversal' '\n+\tgit init pseudo-merge-boundary-traversal &&\n+\t(\n+\t\tcd pseudo-merge-boundary-traversal &&\n+\n+\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n+\t\tgit config bitmapPseudoMerge.test.threshold now &&\n+\t\tgit config bitmapPseudoMerge.test.stableThreshold now &&\n+\t\tGIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n+\n+\t\ttest_commit A &&\n+\t\tgit repack -adb &&\n+\t\ttest_commit B &&\n+\n+\t\techo '1' >expect &&\n+\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n-- \ngitgitgadget\n"},{"id":"519536","messageId":"xmqq1ps1s698.fsf@gitster.g","threadId":"63518","inReplyTo":"pull.1977.v3.git.git.1748915181113.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] pack-bitmap: remove checks before bitmap_free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-03T06:12:35Z","receivedAt":"2025-06-03T06:12:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +test_expect_success 'use pseudo-merge in boundary traversal' '\n> +\tgit init pseudo-merge-boundary-traversal &&\n> +\t(\n> +\t\tcd pseudo-merge-boundary-traversal &&\n> +\n> +\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n> +\t\tgit config bitmapPseudoMerge.test.threshold now &&\n> +\t\tgit config bitmapPseudoMerge.test.stableThreshold now &&\n\n\n> +\t\tGIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n\nEither before or after that line, don't you need to \n\n\t\texport GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL &&\n\nas well?\n\nAnd if the test passed without exporting the variable, is it really\ntesting what we want to test?\n\n> +\t\ttest_commit A &&\n> +\t\tgit repack -adb &&\n> +\t\ttest_commit B &&\n> +\n> +\t\techo '1' >expect &&\n> +\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n>  test_done\n>\n> base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n"},{"id":"519538","messageId":"pull.1977.v4.git.git.1748931650166.gitgitgadget@gmail.com","threadId":"63518","inReplyTo":"pull.1977.v3.git.git.1748915181113.gitgitgadget@gmail.com","subject":"[PATCH v4] pack-bitmap: remove checks before bitmap_free","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-03T06:20:49Z","receivedAt":"2025-06-03T06:20:53Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\nif cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\nuse roots_bitmap as a mutable reference but not takes roots_bitmap's\nownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.\nAnd this leak currently lacks a dedicated test to detect it.\n\nTo fix this leak, remove if cascade_pseudo_merges_1() succeed check and\nalways calling bitmap_free(roots_bitmap);\n\nTo trigger this leak, we need a pseudo-merge whose size is equal to\nor smaller than roots_bitmap (which corresponds to the set of \"haves\"\ncommits in prepare_bitmap_walk()). To do this, we can create two\ncommits: A and B. Add A to the pseudo-merge list and perform a traversal\nover the range A..B. In this scenario, the \"haves\" set will be {A},\nand cascade_pseudo_merges_1() will succeed, thereby exposing the leak\ndue to the missing roots_bitmap cleanup.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    pack-bitmap: remove checks before bitmap_free\n    \n    In pack-bitmap.c:find_boundary_objects, remove cascade success check and\n    always free roots_bitmap afterward to make static analysis tool works\n    better.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1977%2Fbrandb97%2Fremove-check-before-bitmap-free-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1977/brandb97/remove-check-before-bitmap-free-v4\nPull-Request: https://github.com/git/git/pull/1977\n\nRange-diff vs v3:\n\n 1:  151a7f5dc70 ! 1:  fa443065436 pack-bitmap: remove checks before bitmap_free\n     @@ t/t5333-pseudo-merge-bitmaps.sh: test_expect_success 'pseudo-merge closure' '\n      +\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n      +\t\tgit config bitmapPseudoMerge.test.threshold now &&\n      +\t\tgit config bitmapPseudoMerge.test.stableThreshold now &&\n     -+\t\tGIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n      +\n      +\t\ttest_commit A &&\n      +\t\tgit repack -adb &&\n      +\t\ttest_commit B &&\n      +\n      +\t\techo '1' >expect &&\n     -+\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n     ++\t\tGIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 \\\n     ++\t\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n      +\t\ttest_cmp expect actual\n      +\t)\n      +'\n\n\n pack-bitmap.c                   |  4 ++--\n t/t5333-pseudo-merge-bitmaps.sh | 20 ++++++++++++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c..8727f316de9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n \t\t\tbitmap_set(roots_bitmap, pos);\n \t\t}\n \n-\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n-\t\t\tbitmap_free(roots_bitmap);\n+\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n+\t\tbitmap_free(roots_bitmap);\n \t}\n \n \t/*\ndiff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\nindex 56674db562f..e665001a410 100755\n--- a/t/t5333-pseudo-merge-bitmaps.sh\n+++ b/t/t5333-pseudo-merge-bitmaps.sh\n@@ -445,4 +445,24 @@ test_expect_success 'pseudo-merge closure' '\n \t)\n '\n \n+test_expect_success 'use pseudo-merge in boundary traversal' '\n+\tgit init pseudo-merge-boundary-traversal &&\n+\t(\n+\t\tcd pseudo-merge-boundary-traversal &&\n+\n+\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n+\t\tgit config bitmapPseudoMerge.test.threshold now &&\n+\t\tgit config bitmapPseudoMerge.test.stableThreshold now &&\n+\n+\t\ttest_commit A &&\n+\t\tgit repack -adb &&\n+\t\ttest_commit B &&\n+\n+\t\techo '1' >expect &&\n+\t\tGIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 \\\n+\t\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n-- \ngitgitgadget\n"},{"id":"519539","messageId":"0BFD6581-2BB9-439B-9837-767FA98900C5@smail.nju.edu.cn","threadId":"63518","inReplyTo":"xmqq1ps1s698.fsf@gitster.g","subject":"Re: [PATCH v3] pack-bitmap: remove checks before bitmap_free","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-03T06:22:09Z","receivedAt":"2025-06-03T06:22:53Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年6月3日 14:12，Junio C Hamano <gitster@pobox.com> 写道：\n> \n> \"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> +test_expect_success 'use pseudo-merge in boundary traversal' '\n>> + git init pseudo-merge-boundary-traversal &&\n>> + (\n>> + cd pseudo-merge-boundary-traversal &&\n>> +\n>> + git config bitmapPseudoMerge.test.pattern refs/ &&\n>> + git config bitmapPseudoMerge.test.threshold now &&\n>> + git config bitmapPseudoMerge.test.stableThreshold now &&\n> \n> \n>> + GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 &&\n> \n> Either before or after that line, don't you need to \n> \n> export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL &&\n> \n> as well?\n> \n> And if the test passed without exporting the variable, is it really\n> testing what we want to test?\n> \n\nSorry about that. I should put GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL\nIn front of `git rev-list …` so that when traverse bitmap it enters `pack-bitmap:find_boundary_objects()`.\n\n>> + test_commit A &&\n>> + git repack -adb &&\n>> + test_commit B &&\n>> +\n>> + echo '1' >expect &&\n>> + git rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n>> + test_cmp expect actual\n>> + )\n>> +'\n>> +\n>> test_done\n>> \n>> base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n> \n\n"},{"id":"519575","messageId":"xmqqwm9sq2lq.fsf@gitster.g","threadId":"63518","inReplyTo":"0BFD6581-2BB9-439B-9837-767FA98900C5@smail.nju.edu.cn","subject":"Re: [PATCH v3] pack-bitmap: remove checks before bitmap_free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-03T15:14:25Z","receivedAt":"2025-06-03T15:14:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lidongyan <502024330056@smail.nju.edu.cn> writes:\n\n>> And if the test passed without exporting the variable, is it really\n>> testing what we want to test?\n>> \n>\n> Sorry about that. I should put GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL\n> In front of `git rev-list …` so that when traverse bitmap it\n> enters `pack-bitmap:find_boundary_objects()`.\n\nThat would work well.  By narrowing where the environment variable is\napplied, such an arrangement would also help readers.\n\nIt still is curious why this version did not fail for you, though.\nIf setting it without exporting it still made \"rev-list\" traverse\nand expected result, wouldn't that mean we are not really testing\nwhat we want to test?\n\n>>> + test_commit A &&\n>>> + git repack -adb &&\n>>> + test_commit B &&\n>>> +\n>>> + echo '1' >expect &&\n>>> + git rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n>>> + test_cmp expect actual\n>>> + )\n>>> +'\n>>> +\n>>> test_done\n>>> \n>>> base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n>> \n"},{"id":"519578","messageId":"B7032488-F47A-46B9-AF9C-D059AFC31FE8@smail.nju.edu.cn","threadId":"63518","inReplyTo":"xmqqwm9sq2lq.fsf@gitster.g","subject":"Re: [PATCH v3] pack-bitmap: remove checks before bitmap_free","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-03T15:32:23Z","receivedAt":"2025-06-03T15:33:24Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年6月3日 23:14，Junio C Hamano <gitster@pobox.com> 写道：\n> \n> It still is curious why this version did not fail for you, though.\n> If setting it without exporting it still made \"rev-list\" traverse\n> and expected result, wouldn't that mean we are not really testing\n> what we want to test?\n> \n>>>> + test_commit A &&\n>>>> + git repack -adb &&\n>>>> + test_commit B &&\n>>>> +\n>>>> + echo '1' >expect &&\n>>>> + git rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n>>>> + test_cmp expect actual\n>>>> + )\n>>>> +'\n>>>> +\n>>>> test_done\n>>>> \n>>>> base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n>>> \n> \n\nNo, this test case should only fail when ’SANITIZE_LEAK’ is set. I heard\nthat other developer call this type of test as prereq. So only when git is\ncompiled with `-fsanitize=address` and `export ASAN_OPTION=detect_leaks=1`\nand without changes as\n\n- if (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n- bitmap_free(roots_bitmap);\n+ cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n+ bitmap_free(roots_bitmap);\n\nThis test case would fail."},{"id":"519599","messageId":"aD9ylkFDWqapFjey@nand.local","threadId":"63518","inReplyTo":"pull.1977.v4.git.git.1748931650166.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] pack-bitmap: remove checks before bitmap_free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-06-03T22:09:26Z","receivedAt":"2025-06-03T22:09:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 03, 2025 at 06:20:49AM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>\n> In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\n> if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\n> use roots_bitmap as a mutable reference but not takes roots_bitmap's\n> ownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.\n> And this leak currently lacks a dedicated test to detect it.\n>\n> To fix this leak, remove if cascade_pseudo_merges_1() succeed check and\n> always calling bitmap_free(roots_bitmap);\n\nThis sentence might be more clear if it were written as:\n\n    To fix this leak, unconditionally free the roots_bitmap regardless\n    of whether or not cascade_pseudo_merges_1() succeeds.\n\n> To trigger this leak, we need a pseudo-merge whose size is equal to\n> or smaller than roots_bitmap (which corresponds to the set of \"haves\"\n> commits in prepare_bitmap_walk()). To do this, we can create two\n> commits: A and B. Add A to the pseudo-merge list and perform a traversal\n> over the range A..B. In this scenario, the \"haves\" set will be {A},\n> and cascade_pseudo_merges_1() will succeed, thereby exposing the leak\n> due to the missing roots_bitmap cleanup.\n\nI don't think this is quite right. Calling cascade_pseudo_merges_1()\nsucceeds (and returns a non-zero value) when one or more pseudo-merges\nare satisfied. A pseudo-merge is satisfied here when its parents bitmap\nis a *subset* of the roots_bitmap, not when it has a smaller size.\n\nThe precise definition of one bitmap being a subset of another can be\nfound in ewah/bitmap.c::ewah_bitamp_is_subset(). But in general one\nbitmap is a subset of the other if the set of bit positions with value\n\"1\" from one is a subset of the same set from the other bitmap.\n\nI think that's what you meant by \"smaller\", but I think it's worth\nclarifying here.\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index ac6d62b980c..8727f316de9 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n>  \t\t\tbitmap_set(roots_bitmap, pos);\n>  \t\t}\n>\n> -\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n> -\t\t\tbitmap_free(roots_bitmap);\n> +\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n> +\t\tbitmap_free(roots_bitmap);\n\nMakes sense.\n\n> diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\n> index 56674db562f..e665001a410 100755\n> --- a/t/t5333-pseudo-merge-bitmaps.sh\n> +++ b/t/t5333-pseudo-merge-bitmaps.sh\n> @@ -445,4 +445,24 @@ test_expect_success 'pseudo-merge closure' '\n>  \t)\n>  '\n>\n> +test_expect_success 'use pseudo-merge in boundary traversal' '\n> +\tgit init pseudo-merge-boundary-traversal &&\n> +\t(\n> +\t\tcd pseudo-merge-boundary-traversal &&\n> +\n> +\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n> +\t\tgit config bitmapPseudoMerge.test.threshold now &&\n\nSetting the unstable threshold here should be unnecessary, since the\nunstable portion of the group only includes matching commits beyond the\nthreshold that *don't* already have a bitmap. Since \"A\" is the only\ncommit at the time you write the bitmap below, it will always be\nselected, and thus never appear in the unstable portion of a\npseudo-merge group.\n\n> +\t\tgit config bitmapPseudoMerge.test.stableThreshold now &&\n\nThis one is technically unnecessary, but only because test_commit starts\nat the $test_tick value, which is very far in the past (beyond the\ndefault value of 1.month.ago).\n\n> +\t\ttest_commit A &&\n> +\t\tgit repack -adb &&\n> +\t\ttest_commit B &&\n> +\n> +\t\techo '1' >expect &&\n\nPlease do not use single-quotes in a test script. It happens to work in\nthis instance, but it is easy to break.\n\n> +\t\tGIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 \\\n> +\t\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n\nThis test needs to use the boundary-based bitmap traversal routines, but\nI'm unclear on why you're using the GIT_TEST_-environment variable to\nenable them.\n\nIs there a reason that we can't rely on the usual repository\nconfiguration here? I would have expected something like this (which\nshould apply cleanly on top of your patch):\n\n--- 8< ---\ndiff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\nindex e665001a41..491ef404ea 100755\n--- a/t/t5333-pseudo-merge-bitmaps.sh\n+++ b/t/t5333-pseudo-merge-bitmaps.sh\n@@ -453,14 +453,14 @@ test_expect_success 'use pseudo-merge in boundary traversal' '\n \t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n \t\tgit config bitmapPseudoMerge.test.threshold now &&\n \t\tgit config bitmapPseudoMerge.test.stableThreshold now &&\n+\t\tgit config pack.useBitmapBoundaryTraversal true &&\n\n \t\ttest_commit A &&\n \t\tgit repack -adb &&\n \t\ttest_commit B &&\n\n-\t\techo '1' >expect &&\n-\t\tGIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 \\\n-\t\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n+\t\techo 1 >expect &&\n+\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n--- >8 ---\n\n> +\t\ttest_cmp expect actual\n\nHmm. I suppose, although it feels a little clunky to me to write\nsomething like \"echo 1 >expect\". I would imagine that you'd do something\nlike:\n\n    test 1 -eq $(git rev-list --count --use-bitmap-index HEAD~1..HEAD)\n\ninstead. Or if you wanted to split them off into separate lines, you\ncould do:\n\n    nr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&\n    test 1 -eq \"$nr\"\n\nThanks,\nTaylor\n"},{"id":"519626","messageId":"04F52607-480F-41EC-ACBB-B335B238365F@smail.nju.edu.cn","threadId":"63518","inReplyTo":"aD9ylkFDWqapFjey@nand.local","subject":"Re: [PATCH v4] pack-bitmap: remove checks before bitmap_free","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-04T02:50:30Z","receivedAt":"2025-06-04T02:51:16Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"\n\n> 2025年6月4日 06:09，Taylor Blau <me@ttaylorr.com> 写道：\n> \n> On Tue, Jun 03, 2025 at 06:20:49AM +0000, Lidong Yan via GitGitGadget wrote:\n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> \n>> In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\n>> if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\n>> use roots_bitmap as a mutable reference but not takes roots_bitmap's\n>> ownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.\n>> And this leak currently lacks a dedicated test to detect it.\n>> \n>> To fix this leak, remove if cascade_pseudo_merges_1() succeed check and\n>> always calling bitmap_free(roots_bitmap);\n> \n> This sentence might be more clear if it were written as:\n> \n>    To fix this leak, unconditionally free the roots_bitmap regardless\n>    of whether or not cascade_pseudo_merges_1() succeeds.\n> \n>> To trigger this leak, we need a pseudo-merge whose size is equal to\n>> or smaller than roots_bitmap (which corresponds to the set of \"haves\"\n>> commits in prepare_bitmap_walk()). To do this, we can create two\n>> commits: A and B. Add A to the pseudo-merge list and perform a traversal\n>> over the range A..B. In this scenario, the \"haves\" set will be {A},\n>> and cascade_pseudo_merges_1() will succeed, thereby exposing the leak\n>> due to the missing roots_bitmap cleanup.\n> \n> I don't think this is quite right. Calling cascade_pseudo_merges_1()\n> succeeds (and returns a non-zero value) when one or more pseudo-merges\n> are satisfied. A pseudo-merge is satisfied here when its parents bitmap\n> is a *subset* of the roots_bitmap, not when it has a smaller size.\n> \n> The precise definition of one bitmap being a subset of another can be\n> found in ewah/bitmap.c::ewah_bitamp_is_subset(). But in general one\n> bitmap is a subset of the other if the set of bit positions with value\n> \"1\" from one is a subset of the same set from the other bitmap.\n> \n> I think that's what you meant by \"smaller\", but I think it's worth\n> clarifying here.\n\nYes, I want to say subset here, I will rewrite this part of comment.\n\n> \n>> diff --git a/pack-bitmap.c b/pack-bitmap.c\n>> index ac6d62b980c..8727f316de9 100644\n>> --- a/pack-bitmap.c\n>> +++ b/pack-bitmap.c\n>> @@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n>> bitmap_set(roots_bitmap, pos);\n>> }\n>> \n>> - if (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n>> - bitmap_free(roots_bitmap);\n>> + cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n>> + bitmap_free(roots_bitmap);\n> \n> Makes sense.\n> \n>> diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\n>> index 56674db562f..e665001a410 100755\n>> --- a/t/t5333-pseudo-merge-bitmaps.sh\n>> +++ b/t/t5333-pseudo-merge-bitmaps.sh\n>> @@ -445,4 +445,24 @@ test_expect_success 'pseudo-merge closure' '\n>> )\n>> '\n>> \n>> +test_expect_success 'use pseudo-merge in boundary traversal' '\n>> + git init pseudo-merge-boundary-traversal &&\n>> + (\n>> + cd pseudo-merge-boundary-traversal &&\n>> +\n>> + git config bitmapPseudoMerge.test.pattern refs/ &&\n>> + git config bitmapPseudoMerge.test.threshold now &&\n> \n> Setting the unstable threshold here should be unnecessary, since the\n> unstable portion of the group only includes matching commits beyond the\n> threshold that *don't* already have a bitmap. Since \"A\" is the only\n> commit at the time you write the bitmap below, it will always be\n> selected, and thus never appear in the unstable portion of a\n> pseudo-merge group.\n> \n>> + git config bitmapPseudoMerge.test.stableThreshold now &&\n> \n> This one is technically unnecessary, but only because test_commit starts\n> at the $test_tick value, which is very far in the past (beyond the\n> default value of 1.month.ago).\n\nMay be this is the time for me to re-read pseudo-merge documents.\n\n> \n>> + test_commit A &&\n>> + git repack -adb &&\n>> + test_commit B &&\n>> +\n>> + echo '1' >expect &&\n> \n> Please do not use single-quotes in a test script. It happens to work in\n> this instance, but it is easy to break.\n\nGot it.\n\n> \n>> + GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 \\\n>> + git rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n> \n> This test needs to use the boundary-based bitmap traversal routines, but\n> I'm unclear on why you're using the GIT_TEST_-environment variable to\n> enable them.\n\nI don’t have a special reason to choose GIT_TEST rather than `git config`.\nI just find in both way this test works so I use GIT_TEST. I will switch to `git config`.\n\n>  \n> Is there a reason that we can't rely on the usual repository\n> configuration here? I would have expected something like this (which\n> should apply cleanly on top of your patch):\n> \n> --- 8< ---\n> diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\n> index e665001a41..491ef404ea 100755\n> --- a/t/t5333-pseudo-merge-bitmaps.sh\n> +++ b/t/t5333-pseudo-merge-bitmaps.sh\n> @@ -453,14 +453,14 @@ test_expect_success 'use pseudo-merge in boundary traversal' '\n> git config bitmapPseudoMerge.test.pattern refs/ &&\n> git config bitmapPseudoMerge.test.threshold now &&\n> git config bitmapPseudoMerge.test.stableThreshold now &&\n> + git config pack.useBitmapBoundaryTraversal true &&\n> \n> test_commit A &&\n> git repack -adb &&\n> test_commit B &&\n> \n> - echo '1' >expect &&\n> - GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 \\\n> - git rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n> + echo 1 >expect &&\n> + git rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n> test_cmp expect actual\n> )\n> '\n> --- >8 ---\n> \n>> + test_cmp expect actual\n> \n> Hmm. I suppose, although it feels a little clunky to me to write\n> something like \"echo 1 >expect\". I would imagine that you'd do something\n> like:\n> \n>    test 1 -eq $(git rev-list --count --use-bitmap-index HEAD~1..HEAD)\n> \n> instead. Or if you wanted to split them off into separate lines, you\n> could do:\n> \n>    nr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&\n>    test 1 -eq \"$nr\"\n> \n\nI like the latter one, I will use it in the next series.\n\nThanks,\nLidong\n\n"},{"id":"519656","messageId":"xmqqcybjg00s.fsf@gitster.g","threadId":"63518","inReplyTo":"B7032488-F47A-46B9-AF9C-D059AFC31FE8@smail.nju.edu.cn","subject":"Re: [PATCH v3] pack-bitmap: remove checks before bitmap_free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-04T12:32:35Z","receivedAt":"2025-06-04T12:32:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lidongyan <502024330056@smail.nju.edu.cn> writes:\n\n> No, this test case should only fail when ’SANITIZE_LEAK’ is set. I heard\n> that other developer call this type of test as prereq. So only when git is\n> compiled with `-fsanitize=address` and `export ASAN_OPTION=detect_leaks=1`\n> and without changes as\n>\n> - if (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n> - bitmap_free(roots_bitmap);\n> + cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n> + bitmap_free(roots_bitmap);\n>\n> This test case would fail.\n\nIf the test tickles the code path that used to be broken (and\ncorrected by the patch), temporarily reverting only the code changes\nto pack-bitmap.c and then this test (under leak sanitizer, of\ncourse) should have failed.  And if the test passed with such an\nexperiment, you would have noticed that something is wrong.\n\nBut you didn't notice it and sent the patch, so I'd assume that you\nsaw such a test still failed.  IOW, with \"export\" forgotten in the\ntest, the original (unfixed) code still leaked, without using the\nbitmap traversal, right?\n\nWhich was where my question came from.\n\nOr perhaps you didn't do that \"is my test really tickling the bug I\nfixed and makes the original code without my fix fail?\" test?  Which\nalso explains why lack of \"export\" was not noticed.\n\n"},{"id":"519658","messageId":"60E19C19-2910-46E7-9409-58D26190722A@smail.nju.edu.cn","threadId":"63518","inReplyTo":"xmqqcybjg00s.fsf@gitster.g","subject":"Re: [PATCH v3] pack-bitmap: remove checks before bitmap_free","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-04T12:43:49Z","receivedAt":"2025-06-04T12:44:40Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年6月4日 20:32，Junio C Hamano <gitster@pobox.com> 写道：\n> \n> lidongyan <502024330056@smail.nju.edu.cn> writes:\n> \n>> No, this test case should only fail when ’SANITIZE_LEAK’ is set. I heard\n>> that other developer call this type of test as prereq. So only when git is\n>> compiled with `-fsanitize=address` and `export ASAN_OPTION=detect_leaks=1`\n>> and without changes as\n>> \n>> - if (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n>> - bitmap_free(roots_bitmap);\n>> + cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n>> + bitmap_free(roots_bitmap);\n>> \n>> This test case would fail.\n> \n> If the test tickles the code path that used to be broken (and\n> corrected by the patch), temporarily reverting only the code changes\n> to pack-bitmap.c and then this test (under leak sanitizer, of\n> course) should have failed.  And if the test passed with such an\n> experiment, you would have noticed that something is wrong.\n> \n> But you didn't notice it and sent the patch, so I'd assume that you\n> saw such a test still failed.  IOW, with \"export\" forgotten in the\n> test, the original (unfixed) code still leaked, without using the\n> bitmap traversal, right?\n> \n> Which was where my question came from.\n> \n> Or perhaps you didn't do that \"is my test really tickling the bug I\n> fixed and makes the original code without my fix fail?\" test?  Which\n> also explains why lack of \"export\" was not noticed.\n\nThe test case in v0 with “export” would fail, but test-lint in CI shouts. To make\nCI happy, I delete “export” and submit immediately. So I am sure now in\nv3 this test truly test what we want. But I make two mistakes that I haven’t\npass all CI test before submit. I apologize for the oversight. I'll double-check\nmy tests more carefully in the future to avoid similar issues.\n\nThanks,\nLidong"},{"id":"519674","messageId":"xmqqtt4vd0kh.fsf@gitster.g","threadId":"63518","inReplyTo":"60E19C19-2910-46E7-9409-58D26190722A@smail.nju.edu.cn","subject":"Re: [PATCH v3] pack-bitmap: remove checks before bitmap_free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-04T14:49:02Z","receivedAt":"2025-06-04T14:49:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lidongyan <502024330056@smail.nju.edu.cn> writes:\n\n>> Which was where my question came from.\n>> \n>> Or perhaps you didn't do that \"is my test really tickling the bug I\n>> fixed and makes the original code without my fix fail?\" test?  Which\n>> also explains why lack of \"export\" was not noticed.\n>\n> The test case in v0 with “export” would fail, but test-lint in CI shouts. To make\n> CI happy, I delete “export” and submit immediately. So I am sure now in\n> v3 this test truly test what we want. But I make two mistakes that I haven’t\n> pass all CI test before submit. I apologize for the oversight. I'll double-check\n> my tests more carefully in the future to avoid similar issues.\n\nAh, I understood what happened.  No need to apologize.  I just\nwanted to know how it was missed, so that we all can learn from\nthis episode to make sure that our tests do verify what we want\nthem to.\n\nThanks.\n"},{"id":"519710","messageId":"pull.1977.v5.git.git.1749104667618.gitgitgadget@gmail.com","threadId":"63518","inReplyTo":"pull.1977.v4.git.git.1748931650166.gitgitgadget@gmail.com","subject":"[PATCH v5] pack-bitmap: remove checks before bitmap_free","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-05T06:24:27Z","receivedAt":"2025-06-05T06:24:31Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\nif cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\nuse roots_bitmap as a mutable reference but not takes roots_bitmap's\nownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.\nAnd this leak currently lacks a dedicated test to detect it.\n\nTo fix this leak, remove if cascade_pseudo_merges_1() succeed check and\nalways calling bitmap_free(roots_bitmap);\n\nTo trigger this leak, we need roots_bitmap contains at least one pseudo\nmerge. So that we can use pseudo merge bitmap when we compute roots\nreachable bitmap. Here we create two commits: first A then B. Add A\nto the pseudo-merge and perform a traversal over the range A..B.\nIn this scenario, the \"haves\" set will be {A}, and cascade_pseudo_merges_1\nwill succeed, thereby exposing the leak due to the missing roots_bitmap\ncleanup.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    pack-bitmap: remove checks before bitmap_free\n    \n    In pack-bitmap.c:find_boundary_objects, remove cascade success check and\n    always free roots_bitmap afterward to make static analysis tool works\n    better.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1977%2Fbrandb97%2Fremove-check-before-bitmap-free-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1977/brandb97/remove-check-before-bitmap-free-v5\nPull-Request: https://github.com/git/git/pull/1977\n\nRange-diff vs v4:\n\n 1:  fa443065436 ! 1:  4bc90c83a40 pack-bitmap: remove checks before bitmap_free\n     @@ Commit message\n          To fix this leak, remove if cascade_pseudo_merges_1() succeed check and\n          always calling bitmap_free(roots_bitmap);\n      \n     -    To trigger this leak, we need a pseudo-merge whose size is equal to\n     -    or smaller than roots_bitmap (which corresponds to the set of \"haves\"\n     -    commits in prepare_bitmap_walk()). To do this, we can create two\n     -    commits: A and B. Add A to the pseudo-merge list and perform a traversal\n     -    over the range A..B. In this scenario, the \"haves\" set will be {A},\n     -    and cascade_pseudo_merges_1() will succeed, thereby exposing the leak\n     -    due to the missing roots_bitmap cleanup.\n     +    To trigger this leak, we need roots_bitmap contains at least one pseudo\n     +    merge. So that we can use pseudo merge bitmap when we compute roots\n     +    reachable bitmap. Here we create two commits: first A then B. Add A\n     +    to the pseudo-merge and perform a traversal over the range A..B.\n     +    In this scenario, the \"haves\" set will be {A}, and cascade_pseudo_merges_1\n     +    will succeed, thereby exposing the leak due to the missing roots_bitmap\n     +    cleanup.\n      \n          Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n      \n     @@ t/t5333-pseudo-merge-bitmaps.sh: test_expect_success 'pseudo-merge closure' '\n      +\t\tcd pseudo-merge-boundary-traversal &&\n      +\n      +\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n     -+\t\tgit config bitmapPseudoMerge.test.threshold now &&\n     -+\t\tgit config bitmapPseudoMerge.test.stableThreshold now &&\n     ++\t\tgit config pack.useBitmapBoundaryTraversal true &&\n      +\n      +\t\ttest_commit A &&\n      +\t\tgit repack -adb &&\n      +\t\ttest_commit B &&\n      +\n     -+\t\techo '1' >expect &&\n     -+\t\tGIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 \\\n     -+\t\t\tgit rev-list --count --use-bitmap-index HEAD~1..HEAD >actual &&\n     -+\t\ttest_cmp expect actual\n     ++\t\tnr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&\n     ++\t\ttest 1 -eq \"$nr\"\n      +\t)\n      +'\n      +\n\n\n pack-bitmap.c                   |  4 ++--\n t/t5333-pseudo-merge-bitmaps.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c..8727f316de9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n \t\t\tbitmap_set(roots_bitmap, pos);\n \t\t}\n \n-\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n-\t\t\tbitmap_free(roots_bitmap);\n+\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n+\t\tbitmap_free(roots_bitmap);\n \t}\n \n \t/*\ndiff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\nindex 56674db562f..ba5ae6a00c9 100755\n--- a/t/t5333-pseudo-merge-bitmaps.sh\n+++ b/t/t5333-pseudo-merge-bitmaps.sh\n@@ -445,4 +445,21 @@ test_expect_success 'pseudo-merge closure' '\n \t)\n '\n \n+test_expect_success 'use pseudo-merge in boundary traversal' '\n+\tgit init pseudo-merge-boundary-traversal &&\n+\t(\n+\t\tcd pseudo-merge-boundary-traversal &&\n+\n+\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n+\t\tgit config pack.useBitmapBoundaryTraversal true &&\n+\n+\t\ttest_commit A &&\n+\t\tgit repack -adb &&\n+\t\ttest_commit B &&\n+\n+\t\tnr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&\n+\t\ttest 1 -eq \"$nr\"\n+\t)\n+'\n+\n test_done\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n-- \ngitgitgadget\n"},{"id":"519784","messageId":"xmqqldq69phe.fsf@gitster.g","threadId":"63518","inReplyTo":"pull.1977.v5.git.git.1749104667618.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] pack-bitmap: remove checks before bitmap_free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-05T15:29:01Z","receivedAt":"2025-06-05T15:29:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>\n> In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\n> if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\n> use roots_bitmap as a mutable reference but not takes roots_bitmap's\n> ownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.\n\n\"Once cascade_pseudo_merges_1() succeeds\", perhaps?\n\n> And this leak currently lacks a dedicated test to detect it.\n>\n> To fix this leak, remove if cascade_pseudo_merges_1() succeed check and\n> always calling bitmap_free(roots_bitmap);\n>\n> To trigger this leak, we need roots_bitmap contains at least one pseudo\n> merge.\n\n\"contains\" -> \"that contains\"?\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index ac6d62b980c..8727f316de9 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n>  \t\t\tbitmap_set(roots_bitmap, pos);\n>  \t\t}\n>  \n> -\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n> -\t\t\tbitmap_free(roots_bitmap);\n> +\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n> +\t\tbitmap_free(roots_bitmap);\n\nThis makes it as if the original _wanted_ to leak it when the call\nfailed.  Readers may wonder how we got into this state in the first\nplace.  Was it a simple thinko when 11d45a6e (pack-bitmap.c: use\npseudo-merges during traversal, 2024-05-23) was written, I have to\nwonder.\n\n> diff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\n> index 56674db562f..ba5ae6a00c9 100755\n> --- a/t/t5333-pseudo-merge-bitmaps.sh\n> +++ b/t/t5333-pseudo-merge-bitmaps.sh\n> @@ -445,4 +445,21 @@ test_expect_success 'pseudo-merge closure' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'use pseudo-merge in boundary traversal' '\n> +\tgit init pseudo-merge-boundary-traversal &&\n> +\t(\n> +\t\tcd pseudo-merge-boundary-traversal &&\n> +\n> +\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n> +\t\tgit config pack.useBitmapBoundaryTraversal true &&\n\nYup, this is a temporary repository for only this test, so using\n\"git config\" there makes it the simplest and the easiest to\nunderstand.\n\n> +\t\ttest_commit A &&\n> +\t\tgit repack -adb &&\n> +\t\ttest_commit B &&\n> +\n> +\t\tnr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&\n> +\t\ttest 1 -eq \"$nr\"\n> +\t)\n> +'\n> +\n>  test_done\n>\n> base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n"},{"id":"519788","messageId":"pull.1977.v6.git.git.1749138820241.gitgitgadget@gmail.com","threadId":"63518","inReplyTo":"pull.1977.v5.git.git.1749104667618.gitgitgadget@gmail.com","subject":"[PATCH v6] pack-bitmap: remove checks before bitmap_free","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-05T15:53:39Z","receivedAt":"2025-06-05T15:53:43Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\nif cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\nuse roots_bitmap as a mutable reference but not takes roots_bitmap's\nownership. Once cascade_pseudo_merges_1() succeeds, roots_bitmap leaks.\nAnd this leak currently lacks a dedicated test to detect it.\n\nTo fix this leak, remove if cascade_pseudo_merges_1() succeed check and\nalways calling bitmap_free(roots_bitmap);\n\nTo trigger this leak, we need roots_bitmap that contains at least one\npseudo merge. So that we can use pseudo merge bitmap when we compute roots\nreachable bitmap. Here we create two commits: first A then B. Add A\nto the pseudo-merge and perform a traversal over the range A..B.\nIn this scenario, the \"haves\" set will be {A}, and cascade_pseudo_merges_1\nwill succeed, thereby exposing the leak due to the missing roots_bitmap\ncleanup.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    pack-bitmap: remove checks before bitmap_free\n    \n    In pack-bitmap.c:find_boundary_objects, remove cascade success check and\n    always free roots_bitmap afterward to make static analysis tool works\n    better.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1977%2Fbrandb97%2Fremove-check-before-bitmap-free-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1977/brandb97/remove-check-before-bitmap-free-v6\nPull-Request: https://github.com/git/git/pull/1977\n\nRange-diff vs v5:\n\n 1:  4bc90c83a40 ! 1:  43cdce190dc pack-bitmap: remove checks before bitmap_free\n     @@ Commit message\n          In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\n          if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\n          use roots_bitmap as a mutable reference but not takes roots_bitmap's\n     -    ownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.\n     +    ownership. Once cascade_pseudo_merges_1() succeeds, roots_bitmap leaks.\n          And this leak currently lacks a dedicated test to detect it.\n      \n          To fix this leak, remove if cascade_pseudo_merges_1() succeed check and\n          always calling bitmap_free(roots_bitmap);\n      \n     -    To trigger this leak, we need roots_bitmap contains at least one pseudo\n     -    merge. So that we can use pseudo merge bitmap when we compute roots\n     +    To trigger this leak, we need roots_bitmap that contains at least one\n     +    pseudo merge. So that we can use pseudo merge bitmap when we compute roots\n          reachable bitmap. Here we create two commits: first A then B. Add A\n          to the pseudo-merge and perform a traversal over the range A..B.\n          In this scenario, the \"haves\" set will be {A}, and cascade_pseudo_merges_1\n\n\n pack-bitmap.c                   |  4 ++--\n t/t5333-pseudo-merge-bitmaps.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c..8727f316de9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n \t\t\tbitmap_set(roots_bitmap, pos);\n \t\t}\n \n-\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n-\t\t\tbitmap_free(roots_bitmap);\n+\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n+\t\tbitmap_free(roots_bitmap);\n \t}\n \n \t/*\ndiff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\nindex 56674db562f..ba5ae6a00c9 100755\n--- a/t/t5333-pseudo-merge-bitmaps.sh\n+++ b/t/t5333-pseudo-merge-bitmaps.sh\n@@ -445,4 +445,21 @@ test_expect_success 'pseudo-merge closure' '\n \t)\n '\n \n+test_expect_success 'use pseudo-merge in boundary traversal' '\n+\tgit init pseudo-merge-boundary-traversal &&\n+\t(\n+\t\tcd pseudo-merge-boundary-traversal &&\n+\n+\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n+\t\tgit config pack.useBitmapBoundaryTraversal true &&\n+\n+\t\ttest_commit A &&\n+\t\tgit repack -adb &&\n+\t\ttest_commit B &&\n+\n+\t\tnr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&\n+\t\ttest 1 -eq \"$nr\"\n+\t)\n+'\n+\n test_done\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n-- \ngitgitgadget\n"},{"id":"519818","messageId":"xmqqplfh64lc.fsf@gitster.g","threadId":"63518","inReplyTo":"pull.1977.v6.git.git.1749138820241.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] pack-bitmap: remove checks before bitmap_free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-06T01:28:31Z","receivedAt":"2025-06-06T01:28:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>\n> In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\n> if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\n> use roots_bitmap as a mutable reference but not takes roots_bitmap's\n> ownership.\n\nSorry but I cannot parse the last sentence above.  I would have\nexpected that \"Since/Because X\" to be followed by comma and a\nsentence that describes the consequence of X.  Also \"but not takes\"\n-> \"but does not take\", probably.\n\n"},{"id":"519824","messageId":"E2C28248-2486-4E2A-846E-1C6233E7CE6A@smail.nju.edu.cn","threadId":"63518","inReplyTo":"xmqqplfh64lc.fsf@gitster.g","subject":"Re: [PATCH v6] pack-bitmap: remove checks before bitmap_free","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-06T05:49:23Z","receivedAt":"2025-06-06T05:50:14Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年6月6日 09:28，Junio C Hamano <gitster@pobox.com> 写道：\n> \n> \"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> \n>> In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\n>> if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\n>> use roots_bitmap as a mutable reference but not takes roots_bitmap's\n>> ownership.\n> \n> Sorry but I cannot parse the last sentence above.  I would have\n> expected that \"Since/Because X\" to be followed by comma and a\n> sentence that describes the consequence of X.  Also \"but not takes\"\n> -> \"but does not take\", probably.\n\n\nYou are right, I should use a grammar checker (chatgpt) on my log message.\nHow about\n“\nSince cascade_pseudo_merges_1() only\nuse roots_bitmap as a mutable reference but not takes roots_bitmap's\nownership. Once cascade_pseudo_merges_1() succeeds, roots_bitmap leaks.\n”\n->\n“\nHowever, cascade_pseudo_merges_1() uses roots_bitmap as a \nmutable reference without taking ownership of it. As a result, if \ncascade_pseudo_merges_1() succeeds, roots_bitmap is leaked.\n”"},{"id":"519966","messageId":"pull.1977.v7.git.git.1749457124804.gitgitgadget@gmail.com","threadId":"63518","inReplyTo":"pull.1977.v6.git.git.1749138820241.gitgitgadget@gmail.com","subject":"[PATCH v7] pack-bitmap: remove checks before bitmap_free","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-09T08:18:44Z","receivedAt":"2025-06-09T08:18:48Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nIn pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\nif cascade_pseudo_merges_1() fails. However, cascade_pseudo_merges_1()\nuses roots_bitmap as a mutable reference without taking ownership of it.\nAs a result, if cascade_pseudo_merges_1() succeeds, roots_bitmap is leaked.\nAnd this leak currently lacks a dedicated test to detect it.\n\nTo fix this leak, remove if cascade_pseudo_merges_1() succeed check and\nalways calling bitmap_free(roots_bitmap);\n\nTo trigger this leak, we need roots_bitmap that contains at least one\npseudo merge. So that we can use pseudo merge bitmap when we compute roots\nreachable bitmap. Here we create two commits: first A then B. Add A\nto the pseudo-merge and perform a traversal over the range A..B.\nIn this scenario, the \"haves\" set will be {A}, and cascade_pseudo_merges_1\nwill succeed, thereby exposing the leak due to the missing roots_bitmap\ncleanup.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    pack-bitmap: remove checks before bitmap_free\n    \n    In pack-bitmap.c:find_boundary_objects, remove cascade success check and\n    always free roots_bitmap afterward to make static analysis tool works\n    better.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1977%2Fbrandb97%2Fremove-check-before-bitmap-free-v7\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1977/brandb97/remove-check-before-bitmap-free-v7\nPull-Request: https://github.com/git/git/pull/1977\n\nRange-diff vs v6:\n\n 1:  43cdce190dc ! 1:  74c41eccfb0 pack-bitmap: remove checks before bitmap_free\n     @@ Commit message\n          pack-bitmap: remove checks before bitmap_free\n      \n          In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\n     -    if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\n     -    use roots_bitmap as a mutable reference but not takes roots_bitmap's\n     -    ownership. Once cascade_pseudo_merges_1() succeeds, roots_bitmap leaks.\n     +    if cascade_pseudo_merges_1() fails. However, cascade_pseudo_merges_1()\n     +    uses roots_bitmap as a mutable reference without taking ownership of it.\n     +    As a result, if cascade_pseudo_merges_1() succeeds, roots_bitmap is leaked.\n          And this leak currently lacks a dedicated test to detect it.\n      \n          To fix this leak, remove if cascade_pseudo_merges_1() succeed check and\n\n\n pack-bitmap.c                   |  4 ++--\n t/t5333-pseudo-merge-bitmaps.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c..8727f316de9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n \t\t\tbitmap_set(roots_bitmap, pos);\n \t\t}\n \n-\t\tif (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n-\t\t\tbitmap_free(roots_bitmap);\n+\t\tcascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n+\t\tbitmap_free(roots_bitmap);\n \t}\n \n \t/*\ndiff --git a/t/t5333-pseudo-merge-bitmaps.sh b/t/t5333-pseudo-merge-bitmaps.sh\nindex 56674db562f..ba5ae6a00c9 100755\n--- a/t/t5333-pseudo-merge-bitmaps.sh\n+++ b/t/t5333-pseudo-merge-bitmaps.sh\n@@ -445,4 +445,21 @@ test_expect_success 'pseudo-merge closure' '\n \t)\n '\n \n+test_expect_success 'use pseudo-merge in boundary traversal' '\n+\tgit init pseudo-merge-boundary-traversal &&\n+\t(\n+\t\tcd pseudo-merge-boundary-traversal &&\n+\n+\t\tgit config bitmapPseudoMerge.test.pattern refs/ &&\n+\t\tgit config pack.useBitmapBoundaryTraversal true &&\n+\n+\t\ttest_commit A &&\n+\t\tgit repack -adb &&\n+\t\ttest_commit B &&\n+\n+\t\tnr=$(git rev-list --count --use-bitmap-index HEAD~1..HEAD) &&\n+\t\ttest 1 -eq \"$nr\"\n+\t)\n+'\n+\n test_done\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\n-- \ngitgitgadget\n"},{"id":"520025","messageId":"5AA8E9CD-15C6-4707-9E3A-ACBE0C24184B@smail.nju.edu.cn","threadId":"63518","inReplyTo":"xmqqldq69phe.fsf@gitster.g","subject":"Re: [PATCH v5] pack-bitmap: remove checks before bitmap_free","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-06-10T05:58:39Z","receivedAt":"2025-06-10T05:59:28Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Junio C Hamano <gitster@pobox.com> 写道：\n> \n> \"Lidong Yan via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> \n>> In pack-bitmap.c:find_boundary_objects(), the roots_bitmap is only freed\n>> if cascade_pseudo_merges_1() fails. Since cascade_pseudo_merges_1() only\n>> use roots_bitmap as a mutable reference but not takes roots_bitmap's\n>> ownership. Once cascade_pseudo_merges_1 succeed(), roots_bitmap leaks.\n> \n> \"Once cascade_pseudo_merges_1() succeeds\", perhaps?\n> \n>> And this leak currently lacks a dedicated test to detect it.\n>> \n>> To fix this leak, remove if cascade_pseudo_merges_1() succeed check and\n>> always calling bitmap_free(roots_bitmap);\n>> \n>> To trigger this leak, we need roots_bitmap contains at least one pseudo\n>> merge.\n> \n> \"contains\" -> \"that contains\"?\n> \n>> diff --git a/pack-bitmap.c b/pack-bitmap.c\n>> index ac6d62b980c..8727f316de9 100644\n>> --- a/pack-bitmap.c\n>> +++ b/pack-bitmap.c\n>> @@ -1363,8 +1363,8 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n>> bitmap_set(roots_bitmap, pos);\n>> }\n>> \n>> - if (!cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap))\n>> - bitmap_free(roots_bitmap);\n>> + cascade_pseudo_merges_1(bitmap_git, cb.base, roots_bitmap);\n>> + bitmap_free(roots_bitmap);\n> \n> This makes it as if the original _wanted_ to leak it when the call\n> failed.  Readers may wonder how we got into this state in the first\n> place.  Was it a simple thinko when 11d45a6e (pack-bitmap.c: use\n> pseudo-merges during traversal, 2024-05-23) was written, I have to\n> wonder.\n\nI think this was a simple thinko, similar to \"we need to free resources if\nsomething fails. Since cascade_pseudo_merges_1() fails, we need to free roots_bitmap”.\nIn commit 55e563a (pseudo-merge: fix various memory leaks, 2024-09-30),\nPatrick fixed a similar leak in find_objects(). However, since t5333 doesn’t\ntest boundary traversal, the leak in find_boundary_objects() remains unresolved.\n\n"}]}