Re: [PATCH 5/6] bitmap-commit-table: add tests for the bitmap lookup table

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 22, 2022, 16:54 UTC
Message-ID: <YrNJW00QuHbfi090@nand.local>
In-Reply-To: <a404779a30f767f7a05926fb99f354ea7958ab4b.1655728395.git.gitgitgadget@gmail.com>

On Mon, Jun 20, 2022 at 12:33:13PM +0000, Abhradeep Chakraborty via GitGitGadget wrote:

Show 24 quoted lines

> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
>
> Add tests to check the working of the newly implemented lookup table.
>
> Mentored-by: Taylor Blau <ttaylorr@github.com>
> Co-Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
> ---
>  t/t5310-pack-bitmaps.sh       | 14 ++++++++++++++
>  t/t5326-multi-pack-bitmaps.sh | 19 +++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index f775fc1ce69..f05d3e6ace7 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -43,6 +43,20 @@ test_expect_success 'full repack creates bitmaps' '
>
>  basic_bitmap_tests
>
> +test_expect_success 'using lookup table does not affect basic bitmap tests' '
> +	test_config pack.writeBitmapLookupTable true &&
> +	git repack -adb
> +'

Whether or not we end up making pack.writeBitmapLookupTable be "true" by default, I wonder if we should just set it to "true" whenever we write a bitmap in this file, and then adjust whether or not we *read* the lookup table with the GIT_TEST_ environment variable you introduced a few commits back.

Thinking on it more, though, I don't think it makes a huge practical difference for the code here in "t", since these repositories are tiny and repacking them or rewriting their bitmaps is cheap.

But in the performance tests it probably makes a bigger difference.

Show 28 quoted lines

> +basic_bitmap_tests
> +
> +test_expect_success 'using lookup table does not let each entries to be parsed one by one' '
> +	test_config pack.writeBitmapLookupTable true &&
> +	git repack -adb &&
> +	git rev-list --test-bitmap HEAD 2>out &&
> +	grep "Found bitmap for" out &&
> +	! grep "Bitmap v1 test "
> +'
> +
>  test_expect_success 'incremental repack fails when bitmaps are requested' '
>  	test_commit more-1 &&
>  	test_must_fail git repack -d 2>err &&
> diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
> index 4fe57414c13..85fbdf5e4bb 100755
> --- a/t/t5326-multi-pack-bitmaps.sh
> +++ b/t/t5326-multi-pack-bitmaps.sh
> @@ -306,5 +306,24 @@ test_expect_success 'graceful fallback when missing reverse index' '
>  		! grep "ignoring extra bitmap file" err
>  	)
>  '
> +test_expect_success 'multi-pack-index write --bitmap writes lookup table if enabled' '
> +	rm -fr repo &&
> +	git init repo &&
> +	test_when_finished "rm -fr repo" &&
> +	(
> +		cd repo &&
> +		test_commit_bulk 106 &&

Is there a reason we need to write this many commits? I think this is copied from a test further up which deals explicitly with a case where there are too many commits to write bitmaps for all of them (hence we need to write more commits than 100 or so).

But I think for our purposes here we just need a single commit, written into a single pack, which is covered with a MIDX.

So it should suffice to do something like:

    test_commit base &&
    git repack -ad &&
    git config pack.writeBitmapLookupTable true &&
    git multi-pack-index write --bitmap &&
    [...]

instead of what's written here.

Thanks, Taylor