| Message ID | 38c8afabf25a7f5e144850938cf00b53e9cf25fd.1611617820.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pack-revindex: introduce on-disk '.rev' format | expand |
On Mon, Jan 25, 2021 at 06:37:51PM -0500, Taylor Blau wrote: > Right now, the test suite can be run with 'GIT_TEST_WRITE_REV_INDEX=1' > in the environment, which causes all operations which write a pack to > also write a .rev file. > > To prepare for when that eventually becomes the default, we should > continue to test the in-memory reverse index, too, in order to avoid > losing existing coverage. Unfortuantely, explicit existing coverage is > rather sparse, so only a basic test is added. > > The new test is parameterized over whether or not the .rev file should > be written, and is run in both modes by t5325 (without having to touch > GIT_TEST_WRITE_REV_INDEX). Thanks, I think this is worth having. > +revindex_tests () { > + on_disk="$1" > + > + test_expect_success "setup revindex tests (on_disk=$on_disk)" " > + test_oid_cache <<-EOF && > + disklen sha1:47 > + disklen sha256:59 > + EOF These sizes aren't really guaranteed to be stable. I think you've eliminated most variables by not having the opportunity for deltas, though we're still subject to the whims of zlib, for example. I think what we care about most is just that the on-disk and fallback cases generate the same results. If they do, we can assume those results are probably sane. If they're not, then a lot of _other_ stuff is going to be broken (like, say, pack-objects, which is writing out N bytes based on the sizes claimed by the revindex). What do you think about just doing this: ...setup some commits... && git rev-list --objects --no-object-names --all >objects && git -c pack.writeReverseIndex=false repack -ad && test_path_is_missing .git/objects/pack/*.rev && git cat-file --batch-check="%(objectsize:disk) %(objectname)" \ <objects >in-core && git -c pack.writeReverseIndex=true repack -ad && test_path_is_file .git/objects/pack/*.rev && git cat-file --batch-check="%(objectsize:disk) %(objectname)" \ <objects >on-disk && test_cmp on-disk in-core > + if test ztrue = \"z$on_disk\" > + then > + git config pack.writeReverseIndex true > + fi && Are we autotools now? :) I think we can assume that on_disk contains something sensible. Perhaps even: git config pack.writeReverseIndex $on_disk though if you take my advice above, all of this goes away. -Peff
On Mon, Jan 25, 2021 at 06:37:51PM -0500, Taylor Blau wrote: > Right now, the test suite can be run with 'GIT_TEST_WRITE_REV_INDEX=1' > in the environment, which causes all operations which write a pack to > also write a .rev file. > > To prepare for when that eventually becomes the default, we should > continue to test the in-memory reverse index, too, in order to avoid > losing existing coverage. Unfortuantely, explicit existing coverage is > rather sparse, so only a basic test is added. Oh, I forgot to mention: if re-rolling, s/fortuan/fortuna/. -Peff
On Thu, Jan 28, 2021 at 08:05:41PM -0500, Jeff King wrote: > Oh, I forgot to mention: if re-rolling, s/fortuan/fortuna/. I do like your suggestion quite a lot: it gets rid of some ugliness, while making the overall test structure simpler. Here's a replacement for the final series. Junio: when queuing, please apply this one instead of the original v3 10/10. Thanks, Taylor --- >8 --- Subject: [PATCH] t5325: check both on-disk and in-memory reverse index Right now, the test suite can be run with 'GIT_TEST_WRITE_REV_INDEX=1' in the environment, which causes all operations which write a pack to also write a .rev file. To prepare for when that eventually becomes the default, we should continue to test the in-memory reverse index, too, in order to avoid losing existing coverage. Unfortunately, explicit existing coverage is rather sparse, so only a basic test is added that compares the result of git rev-list --objects --no-object-names --all | git cat-file --batch-check='%(objectsize:disk) %(objectname)' with and without an on-disk reverse index. Suggested-by: Jeff King <peff@peff.net> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t5325-reverse-index.sh | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh index a344b18d7e..da453f68d6 100755 --- a/t/t5325-reverse-index.sh +++ b/t/t5325-reverse-index.sh @@ -94,4 +94,27 @@ test_expect_success 'reverse index is not generated when available on disk' ' --batch-check="%(objectsize:disk)" <tip ' +test_expect_success 'revindex in-memory vs on-disk' ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + test_commit commit && + + git rev-list --objects --no-object-names --all >objects && + + git -c pack.writeReverseIndex=false repack -ad && + test_path_is_missing $packdir/pack-*.rev && + git cat-file --batch-check="%(objectsize:disk) %(objectname)" \ + <objects >in-core && + + git -c pack.writeReverseIndex=true repack -ad && + test_path_is_file $packdir/pack-*.rev && + git cat-file --batch-check="%(objectsize:disk) %(objectname)" \ + <objects >on-disk && + + test_cmp on-disk in-core + ) +' test_done -- 2.30.0.138.g6d7191ea01
On Thu, Jan 28, 2021 at 08:32:02PM -0500, Taylor Blau wrote: > On Thu, Jan 28, 2021 at 08:05:41PM -0500, Jeff King wrote: > > Oh, I forgot to mention: if re-rolling, s/fortuan/fortuna/. > > I do like your suggestion quite a lot: it gets rid of some ugliness, > while making the overall test structure simpler. Here's a replacement > for the final series. > > Junio: when queuing, please apply this one instead of the original v3 > 10/10. Thanks, this looks good to me. -Peff
diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh index a344b18d7e..b1dd726d0e 100755 --- a/t/t5325-reverse-index.sh +++ b/t/t5325-reverse-index.sh @@ -94,4 +94,49 @@ test_expect_success 'reverse index is not generated when available on disk' ' --batch-check="%(objectsize:disk)" <tip ' +revindex_tests () { + on_disk="$1" + + test_expect_success "setup revindex tests (on_disk=$on_disk)" " + test_oid_cache <<-EOF && + disklen sha1:47 + disklen sha256:59 + EOF + + git init repo && + ( + cd repo && + + if test ztrue = \"z$on_disk\" + then + git config pack.writeReverseIndex true + fi && + + test_commit commit && + git repack -ad + ) + + " + + test_expect_success "check objectsize:disk (on_disk=$on_disk)" ' + ( + cd repo && + git rev-parse HEAD^{tree} >tree && + git cat-file --batch-check="%(objectsize:disk)" <tree >actual && + + git cat-file -p HEAD^{tree} && + + printf "%s\n" "$(test_oid disklen)" >expect && + test_cmp expect actual + ) + ' + + test_expect_success "cleanup revindex tests (on_disk=$on_disk)" ' + rm -fr repo + ' +} + +revindex_tests "true" +revindex_tests "false" + test_done
Right now, the test suite can be run with 'GIT_TEST_WRITE_REV_INDEX=1' in the environment, which causes all operations which write a pack to also write a .rev file. To prepare for when that eventually becomes the default, we should continue to test the in-memory reverse index, too, in order to avoid losing existing coverage. Unfortuantely, explicit existing coverage is rather sparse, so only a basic test is added. The new test is parameterized over whether or not the .rev file should be written, and is run in both modes by t5325 (without having to touch GIT_TEST_WRITE_REV_INDEX). Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t5325-reverse-index.sh | 45 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)