| Message ID | 3258ccfc1cc99038e43a37bd2d53c9d30a4f22ae.1627420428.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | multi-pack reachability bitmaps | expand |
On Tue, Jul 27, 2021 at 05:20:10PM -0400, Taylor Blau wrote: > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh > new file mode 100755 > index 0000000000..c1b7d633e2 > --- /dev/null > +++ b/t/t5326-multi-pack-bitmaps.sh > @@ -0,0 +1,277 @@ > +#!/bin/sh > + > +test_description='exercise basic multi-pack bitmap functionality' > +. ./test-lib.sh > +. "${TEST_DIRECTORY}/lib-bitmap.sh" > + > +# We'll be writing our own midx and bitmaps, so avoid getting confused by the > +# automatic ones. > +GIT_TEST_MULTI_PACK_INDEX=0 > +GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 This latter variable doesn't do anything at this point in the series. Probably not a big deal (it is simply a noop until then), but if it's not hard, it may make sense to bump the "respect ... WRITE_BITMAP" patch earlier in the series. > +test_expect_success 'create single-pack midx with bitmaps' ' > + git repack -ad && > + git multi-pack-index write --bitmap && > + test_path_is_file $midx && > + test_path_is_file $midx-$(midx_checksum $objdir).bitmap > +' > + > +basic_bitmap_tests We can't use a midx bitmap without a .rev file. The basic_bitmap_tests function covers that, but I wonder if we should also check: test_path_is_file $midx-$(midx_checksum $objdir).rev in that first test. > +test_expect_success 'create new additional packs' ' > + for i in $(test_seq 1 16) > + do > + test_commit "$i" && > + git repack -d > + done && This loop needs an "|| return 1" inside to catch &&-chain problems (not that we expect "repack -d" to fail, but just on principle). > + git checkout -b other2 HEAD~8 && > + for i in $(test_seq 1 8) > + do > + test_commit "side-$i" && > + git repack -d > + done && Ditto here. > +test_expect_success 'create multi-pack midx with bitmaps' ' > + git multi-pack-index write --bitmap && > + > + ls $objdir/pack/pack-*.pack >packs && > + test_line_count = 25 packs && > + > + test_path_is_file $midx && > + test_path_is_file $midx-$(midx_checksum $objdir).bitmap > +' Possible spot for checking the .rev file again (though really, it is belt-and-suspenders at this point). > +basic_bitmap_tests I love how the earlier refactoring made it easy to test the single- and multi-pack cases thoroughly. > +test_expect_success '--no-bitmap is respected when bitmaps exist' ' > + git multi-pack-index write --bitmap && > + > + test_commit respect--no-bitmap && > + GIT_TEST_MULTI_PACK_INDEX=0 git repack -d && Do we need to set this env variable? We've already set it to 0 at the top of the script. > + test_path_is_file $midx && > + test_path_is_file $midx-$(midx_checksum $objdir).bitmap && > + > + git multi-pack-index write --no-bitmap && > + > + test_path_is_file $midx && > + test_path_is_missing $midx-$(midx_checksum $objdir).bitmap > +' OK, so we expect "--no-bitmap" to drop the bitmap (just like it does for a regular pack bitmap). Makes sense. We probably should check: test_path_is_missing $midx-$(midx_checksum $objdir).rev here, too (unlike the other spots, it isn't redundant; we could leave a stale file around and likely nobody would notice). > +test_expect_success 'setup midx with base from later pack' ' > + # Write a and b so that "a" is a delta on top of base "b", since Git > + # prefers to delete contents out of a base rather than add to a shorter > + # object. > + test_seq 1 128 >a && > + test_seq 1 130 >b && > + > + git add a b && > + git commit -m "initial commit" && > + > + a=$(git rev-parse HEAD:a) && > + b=$(git rev-parse HEAD:b) && > + > + # In the first pack, "a" is stored as a delta to "b". > + p1=$(git pack-objects .git/objects/pack/pack <<-EOF > + $a > + $b > + EOF > + ) && This is brittle with respect to Git's delta heuristics, of course, but I don't think there's a better way to do it with pack-objects. And this is not the first test to make similar assumptions. I think you can construct a known set of deltas using lib-pack.sh. It may get a bit complicated. As an alternative, maybe it makes sense to confirm that the deltas are set up as expected? You can do it with cat-file --batch-check. > +test_expect_success 'removing a MIDX clears stale bitmaps' ' > + rm -fr repo && > + git init repo && > + test_when_finished "rm -fr repo" && > + ( > + cd repo && > + test_commit base && > + git repack && > + git multi-pack-index write --bitmap && > + > + # Write a MIDX and bitmap; remove the MIDX but leave the bitmap. > + stale_bitmap=$midx-$(midx_checksum $objdir).bitmap && > + rm $midx && > + > + # Then write a new MIDX. > + test_commit new && > + git repack && > + git multi-pack-index write --bitmap && > + > + test_path_is_file $midx && > + test_path_is_file $midx-$(midx_checksum $objdir).bitmap && > + test_path_is_missing $stale_bitmap > + ) Another spot where we might want to check that the stale .rev file has gone away (and optionally that the new one was written; I haven't noted all of those, though). > +test_expect_success 'pack.preferBitmapTips' ' > + git init repo && > + test_when_finished "rm -fr repo" && > + ( > + cd repo && > + > + test_commit_bulk --message="%s" 103 && > + > + git log --format="%H" >commits.raw && > + sort <commits.raw >commits && > + > + git log --format="create refs/tags/%s %H" HEAD >refs && > + git update-ref --stdin <refs && > + > + git multi-pack-index write --bitmap && > + test_path_is_file $midx && > + test_path_is_file $midx-$(midx_checksum $objdir).bitmap && > + > + test-tool bitmap list-commits | sort >bitmaps && > + comm -13 bitmaps commits >before && > + test_line_count = 1 before && > + > + perl -ne "printf(\"create refs/tags/include/%d \", $.); print" \ > + <before | git update-ref --stdin && > + > + rm -fr $midx-$(midx_checksum $objdir).bitmap && > + rm -fr $midx-$(midx_checksum $objdir).rev && > + rm -fr $midx && > + > + git -c pack.preferBitmapTips=refs/tags/include \ > + multi-pack-index write --bitmap && > + test-tool bitmap list-commits | sort >bitmaps && > + comm -13 bitmaps commits >after && > + > + ! test_cmp before after > + ) > +' OK, so we are not depending on any _specific_ commits to get bitmapped, but just confirming that we have some impact. That may be the best we can do given that we are subject to the bitmap code's heuristics (and anyway, this is exactly what the pack version does). Any other parts of the patch that I didn't quote looked very good to me. I'm happy to have such a thorough set of tests. -Peff
On Thu, Aug 12, 2021 at 05:02:26PM -0400, Jeff King wrote: > > +# We'll be writing our own midx and bitmaps, so avoid getting confused by the > > +# automatic ones. > > +GIT_TEST_MULTI_PACK_INDEX=0 > > +GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 > > This latter variable doesn't do anything at this point in the series. > Probably not a big deal (it is simply a noop until then), but if it's > not hard, it may make sense to bump the "respect ... WRITE_BITMAP" patch > earlier in the series. Reading the other patches, I guess you ordering was to "fix" each of the tests preemptively, and then add the knob at the end. That's OK by me. For an alternate test-mode like this, I usually wouldn't worry about bisectability, but it doesn't hurt. Somebody reading the commits later won't have any trouble finding the definition of the WRITE_BITMAP variable added in the subsequent patch. > > +test_expect_success '--no-bitmap is respected when bitmaps exist' ' > > + git multi-pack-index write --bitmap && > > + > > + test_commit respect--no-bitmap && > > + GIT_TEST_MULTI_PACK_INDEX=0 git repack -d && > > Do we need to set this env variable? We've already set it to 0 at the > top of the script. By the way, there were a few more of these later in the script that could be cleaned up, too. -Peff
On Thu, Aug 12, 2021 at 05:02:26PM -0400, Jeff King wrote: > On Tue, Jul 27, 2021 at 05:20:10PM -0400, Taylor Blau wrote: > > > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh > > new file mode 100755 > > index 0000000000..c1b7d633e2 > > --- /dev/null > > +++ b/t/t5326-multi-pack-bitmaps.sh > > @@ -0,0 +1,277 @@ > > +#!/bin/sh > > + > > +test_description='exercise basic multi-pack bitmap functionality' > > +. ./test-lib.sh > > +. "${TEST_DIRECTORY}/lib-bitmap.sh" > > + > > +# We'll be writing our own midx and bitmaps, so avoid getting confused by the > > +# automatic ones. > > +GIT_TEST_MULTI_PACK_INDEX=0 > > +GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 > > This latter variable doesn't do anything at this point in the series. > Probably not a big deal (it is simply a noop until then), but if it's > not hard, it may make sense to bump the "respect ... WRITE_BITMAP" patch > earlier in the series. If my memory serves me correctly, I think the very first version of this patch didn't have a GIT_TEST_MULTI_PACK_INDEX{,_WRITE_BITMAP}=0 at the top, and so individual invocations needed to set it in their own environment. Presumably at some point I added this, but forgot to clean up the redundant ones. I removed the ones you mentioned in your response, and a few others. > > +test_expect_success 'create single-pack midx with bitmaps' ' > > + git repack -ad && > > + git multi-pack-index write --bitmap && > > + test_path_is_file $midx && > > + test_path_is_file $midx-$(midx_checksum $objdir).bitmap > > +' > > + > > +basic_bitmap_tests > > We can't use a midx bitmap without a .rev file. The basic_bitmap_tests > function covers that, but I wonder if we should also check: > > test_path_is_file $midx-$(midx_checksum $objdir).rev > > in that first test. Good idea. These tests probably preceded the invention of .rev files, so a lot of them needed updating. I made sure to add them where appropriate. > > +test_expect_success 'create new additional packs' ' > > + for i in $(test_seq 1 16) > > + do > > + test_commit "$i" && > > + git repack -d > > + done && > > This loop needs an "|| return 1" inside to catch &&-chain problems (not > that we expect "repack -d" to fail, but just on principle). Nice catch, thanks. > I love how the earlier refactoring made it easy to test the single- and > multi-pack cases thoroughly. Likewise :-). > > +test_expect_success 'setup midx with base from later pack' ' > > + # Write a and b so that "a" is a delta on top of base "b", since Git > > + # prefers to delete contents out of a base rather than add to a shorter > > + # object. > > + test_seq 1 128 >a && > > + test_seq 1 130 >b && > > + > > + git add a b && > > + git commit -m "initial commit" && > > + > > + a=$(git rev-parse HEAD:a) && > > + b=$(git rev-parse HEAD:b) && > > + > > + # In the first pack, "a" is stored as a delta to "b". > > + p1=$(git pack-objects .git/objects/pack/pack <<-EOF > > + $a > > + $b > > + EOF > > + ) && > > This is brittle with respect to Git's delta heuristics, of course, but I > don't think there's a better way to do it with pack-objects. And this is > not the first test to make similar assumptions. I think you can > construct a known set of deltas using lib-pack.sh. It may get a bit > complicated. As an alternative, maybe it makes sense to confirm that the > deltas are set up as expected? You can do it with cat-file > --batch-check. Yeah, I definitely agree that this test is brittle. But it would fail if our assumptions about what gets delta'd with what changes, because we do check that 'a' is a delta on top of 'b' (see the call to have_delta towards the end of this test). That have_delta helper does use `--batch-check=%(deltabase)`, which is (I think) the cat-file invocation you're mentioning. Thanks, Taylor
On Thu, Aug 12, 2021 at 06:38:20PM -0400, Taylor Blau wrote: > > This is brittle with respect to Git's delta heuristics, of course, but I > > don't think there's a better way to do it with pack-objects. And this is > > not the first test to make similar assumptions. I think you can > > construct a known set of deltas using lib-pack.sh. It may get a bit > > complicated. As an alternative, maybe it makes sense to confirm that the > > deltas are set up as expected? You can do it with cat-file > > --batch-check. > > Yeah, I definitely agree that this test is brittle. But it would fail if > our assumptions about what gets delta'd with what changes, because we do > check that 'a' is a delta on top of 'b' (see the call to have_delta > towards the end of this test). That have_delta helper does use > `--batch-check=%(deltabase)`, which is (I think) the cat-file invocation > you're mentioning. Doh, I totally missed that. I was expecting to verify it earlier in the test as a pre-condition, but it works just fine where it is. So yeah, you are already doing the thing I was suggesting. -Peff
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh new file mode 100755 index 0000000000..c1b7d633e2 --- /dev/null +++ b/t/t5326-multi-pack-bitmaps.sh @@ -0,0 +1,277 @@ +#!/bin/sh + +test_description='exercise basic multi-pack bitmap functionality' +. ./test-lib.sh +. "${TEST_DIRECTORY}/lib-bitmap.sh" + +# We'll be writing our own midx and bitmaps, so avoid getting confused by the +# automatic ones. +GIT_TEST_MULTI_PACK_INDEX=0 +GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 + +objdir=.git/objects +midx=$objdir/pack/multi-pack-index + +# midx_pack_source <obj> +midx_pack_source () { + test-tool read-midx --show-objects .git/objects | grep "^$1 " | cut -f2 +} + +setup_bitmap_history + +test_expect_success 'enable core.multiPackIndex' ' + git config core.multiPackIndex true +' + +test_expect_success 'create single-pack midx with bitmaps' ' + git repack -ad && + git multi-pack-index write --bitmap && + test_path_is_file $midx && + test_path_is_file $midx-$(midx_checksum $objdir).bitmap +' + +basic_bitmap_tests + +test_expect_success 'create new additional packs' ' + for i in $(test_seq 1 16) + do + test_commit "$i" && + git repack -d + done && + + git checkout -b other2 HEAD~8 && + for i in $(test_seq 1 8) + do + test_commit "side-$i" && + git repack -d + done && + git checkout second +' + +test_expect_success 'create multi-pack midx with bitmaps' ' + git multi-pack-index write --bitmap && + + ls $objdir/pack/pack-*.pack >packs && + test_line_count = 25 packs && + + test_path_is_file $midx && + test_path_is_file $midx-$(midx_checksum $objdir).bitmap +' + +basic_bitmap_tests + +test_expect_success '--no-bitmap is respected when bitmaps exist' ' + git multi-pack-index write --bitmap && + + test_commit respect--no-bitmap && + GIT_TEST_MULTI_PACK_INDEX=0 git repack -d && + + test_path_is_file $midx && + test_path_is_file $midx-$(midx_checksum $objdir).bitmap && + + git multi-pack-index write --no-bitmap && + + test_path_is_file $midx && + test_path_is_missing $midx-$(midx_checksum $objdir).bitmap +' + +test_expect_success 'setup midx with base from later pack' ' + # Write a and b so that "a" is a delta on top of base "b", since Git + # prefers to delete contents out of a base rather than add to a shorter + # object. + test_seq 1 128 >a && + test_seq 1 130 >b && + + git add a b && + git commit -m "initial commit" && + + a=$(git rev-parse HEAD:a) && + b=$(git rev-parse HEAD:b) && + + # In the first pack, "a" is stored as a delta to "b". + p1=$(git pack-objects .git/objects/pack/pack <<-EOF + $a + $b + EOF + ) && + + # In the second pack, "a" is missing, and "b" is not a delta nor base to + # any other object. + p2=$(git pack-objects .git/objects/pack/pack <<-EOF + $b + $(git rev-parse HEAD) + $(git rev-parse HEAD^{tree}) + EOF + ) && + + git prune-packed && + # Use the second pack as the preferred source, so that "b" occurs + # earlier in the MIDX object order, rendering "a" unusable for pack + # reuse. + git multi-pack-index write --bitmap --preferred-pack=pack-$p2.idx && + + have_delta $a $b && + test $(midx_pack_source $a) != $(midx_pack_source $b) +' + +rev_list_tests 'full bitmap with backwards delta' + +test_expect_success 'clone with bitmaps enabled' ' + git clone --no-local --bare . clone-reverse-delta.git && + test_when_finished "rm -fr clone-reverse-delta.git" && + + git rev-parse HEAD >expect && + git --git-dir=clone-reverse-delta.git rev-parse HEAD >actual && + test_cmp expect actual +' + +bitmap_reuse_tests() { + from=$1 + to=$2 + + test_expect_success "setup pack reuse tests ($from -> $to)" ' + rm -fr repo && + git init repo && + ( + cd repo && + test_commit_bulk 16 && + git tag old-tip && + + git config core.multiPackIndex true && + if test "MIDX" = "$from" + then + GIT_TEST_MULTI_PACK_INDEX=0 git repack -Ad && + git multi-pack-index write --bitmap + else + GIT_TEST_MULTI_PACK_INDEX=0 git repack -Adb + fi + ) + ' + + test_expect_success "build bitmap from existing ($from -> $to)" ' + ( + cd repo && + test_commit_bulk --id=further 16 && + git tag new-tip && + + if test "MIDX" = "$to" + then + GIT_TEST_MULTI_PACK_INDEX=0 git repack -d && + git multi-pack-index write --bitmap + else + GIT_TEST_MULTI_PACK_INDEX=0 git repack -Adb + fi + ) + ' + + test_expect_success "verify resulting bitmaps ($from -> $to)" ' + ( + cd repo && + git for-each-ref && + git rev-list --test-bitmap refs/tags/old-tip && + git rev-list --test-bitmap refs/tags/new-tip + ) + ' +} + +bitmap_reuse_tests 'pack' 'MIDX' +bitmap_reuse_tests 'MIDX' 'pack' +bitmap_reuse_tests 'MIDX' 'MIDX' + +test_expect_success 'missing object closure fails gracefully' ' + rm -fr repo && + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + test_commit loose && + test_commit packed && + + # Do not pass "--revs"; we want a pack without the "loose" + # commit. + git pack-objects $objdir/pack/pack <<-EOF && + $(git rev-parse packed) + EOF + + test_must_fail git multi-pack-index write --bitmap 2>err && + grep "doesn.t have full closure" err && + test_path_is_missing $midx + ) +' + +test_expect_success 'setup partial bitmaps' ' + test_commit packed && + git repack && + test_commit loose && + git multi-pack-index write --bitmap 2>err && + test_path_is_file $midx && + test_path_is_file $midx-$(midx_checksum $objdir).bitmap +' + +basic_bitmap_tests HEAD~ + +test_expect_success 'removing a MIDX clears stale bitmaps' ' + rm -fr repo && + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit base && + git repack && + git multi-pack-index write --bitmap && + + # Write a MIDX and bitmap; remove the MIDX but leave the bitmap. + stale_bitmap=$midx-$(midx_checksum $objdir).bitmap && + rm $midx && + + # Then write a new MIDX. + test_commit new && + git repack && + git multi-pack-index write --bitmap && + + test_path_is_file $midx && + test_path_is_file $midx-$(midx_checksum $objdir).bitmap && + test_path_is_missing $stale_bitmap + ) +' + +test_expect_success 'pack.preferBitmapTips' ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + test_commit_bulk --message="%s" 103 && + + git log --format="%H" >commits.raw && + sort <commits.raw >commits && + + git log --format="create refs/tags/%s %H" HEAD >refs && + git update-ref --stdin <refs && + + git multi-pack-index write --bitmap && + test_path_is_file $midx && + test_path_is_file $midx-$(midx_checksum $objdir).bitmap && + + test-tool bitmap list-commits | sort >bitmaps && + comm -13 bitmaps commits >before && + test_line_count = 1 before && + + perl -ne "printf(\"create refs/tags/include/%d \", $.); print" \ + <before | git update-ref --stdin && + + rm -fr $midx-$(midx_checksum $objdir).bitmap && + rm -fr $midx-$(midx_checksum $objdir).rev && + rm -fr $midx && + + git -c pack.preferBitmapTips=refs/tags/include \ + multi-pack-index write --bitmap && + test-tool bitmap list-commits | sort >bitmaps && + comm -13 bitmaps commits >after && + + ! test_cmp before after + ) +' + +test_done
This patch introduces a new test, t5326, which tests the basic functionality of multi-pack bitmaps. Some trivial behavior is tested, such as: - Whether bitmaps can be generated with more than one pack. - Whether clones can be served with all objects in the bitmap. - Whether follow-up fetches can be served with some objects outside of the server's bitmap These use lib-bitmap's tests (which in turn were pulled from t5310), and we cover cases where the MIDX represents both a single pack and multiple packs. In addition, some non-trivial and MIDX-specific behavior is tested, too, including: - Whether multi-pack bitmaps behave correctly with respect to the pack-reuse machinery when the base for some object is selected from a different pack than the delta. - Whether multi-pack bitmaps correctly respect the pack.preferBitmapTips configuration. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t5326-multi-pack-bitmaps.sh | 277 ++++++++++++++++++++++++++++++++++ 1 file changed, 277 insertions(+) create mode 100755 t/t5326-multi-pack-bitmaps.sh