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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 4, 2025, 12:32 UTC
Message-ID: <xmqqcybjg00s.fsf@gitster.g>
In-Reply-To: <B7032488-F47A-46B9-AF9C-D059AFC31FE8@smail.nju.edu.cn>

lidongyan <502024330056@smail.nju.edu.cn> writes:

Show 11 quoted lines

> No, this test case should only fail when ’SANITIZE_LEAK’ is set. I heard
> that other developer call this type of test as prereq. So only when git is
> compiled with `-fsanitize=address` and `export ASAN_OPTION=detect_leaks=1`
> and without changes as
>
> - 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 test case would fail.

If the test tickles the code path that used to be broken (and corrected by the patch), temporarily reverting only the code changes to pack-bitmap.c and then this test (under leak sanitizer, of course) should have failed. And if the test passed with such an experiment, you would have noticed that something is wrong.

But you didn't notice it and sent the patch, so I'd assume that you saw such a test still failed. IOW, with "export" forgotten in the test, the original (unfixed) code still leaked, without using the bitmap traversal, right?

Which was where my question came from.

Or perhaps you didn't do that "is my test really tickling the bug I fixed and makes the original code without my fix fail?" test? Which also explains why lack of "export" was not noticed.