| Message ID | 20190315062243.GA13466@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enable bitmap hash-cache by default | expand |
On Fri, Mar 15, 2019 at 02:22:44AM -0400, Jeff King wrote: > We use "jgit gc" to generate a pack bitmap file, and then make sure our > implementation can read it. To prepare the repo before running jgit, we > try to "rm -f" any existing bitmap files. But we got the path wrong; > we're in a bare repo, so looking in ".git/" finds nothing. Our "rm" > doesn't complain because of the "-f", and when we run "rev-list" there > are two bitmap files (ours and jgit's). Oh, indeed. > Our reader implementation will ignore one of the bitmap files, but it's > likely non-deterministic which one we will use. We'd prefer the one with > the more recent timestamp (just because of the way the packed_git list > is sorted), but in most test runs they'd have identical timestamps. > > So this was probably actually testing something useful about 50% of the > time, and other half just testing that we could read our own bitmaps > (which is covered elsewhere). At least it was testing the right thing up until 87a6bb701a (t5310-pack-bitmaps: make JGit tests work with GIT_TEST_SPLIT_INDEX, 2018-05-10) came along. > t/t5310-pack-bitmaps.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh > index 82d7f7f6a5..44a038881a 100755 > --- a/t/t5310-pack-bitmaps.sh > +++ b/t/t5310-pack-bitmaps.sh > @@ -269,7 +269,7 @@ test_expect_success JGIT 'we can read jgit bitmaps' ' > git clone --bare . compat-jgit.git && > ( > cd compat-jgit.git && > - rm -f .git/objects/pack/*.bitmap && > + rm -f objects/pack/*.bitmap && > jgit gc && > git rev-list --test-bitmap HEAD > ) > -- > 2.21.0.543.g90eed137f3 >
On Fri, Mar 15, 2019 at 02:25:45PM +0100, SZEDER Gábor wrote: > > Our reader implementation will ignore one of the bitmap files, but it's > > likely non-deterministic which one we will use. We'd prefer the one with > > the more recent timestamp (just because of the way the packed_git list > > is sorted), but in most test runs they'd have identical timestamps. > > > > So this was probably actually testing something useful about 50% of the > > time, and other half just testing that we could read our own bitmaps > > (which is covered elsewhere). > > At least it was testing the right thing up until 87a6bb701a > (t5310-pack-bitmaps: make JGit tests work with GIT_TEST_SPLIT_INDEX, > 2018-05-10) came along. Heh, indeed. I didn't even dig into the history, and just assumed I had botched it in 2013. :) -Peff
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index 82d7f7f6a5..44a038881a 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh @@ -269,7 +269,7 @@ test_expect_success JGIT 'we can read jgit bitmaps' ' git clone --bare . compat-jgit.git && ( cd compat-jgit.git && - rm -f .git/objects/pack/*.bitmap && + rm -f objects/pack/*.bitmap && jgit gc && git rev-list --test-bitmap HEAD )
We use "jgit gc" to generate a pack bitmap file, and then make sure our implementation can read it. To prepare the repo before running jgit, we try to "rm -f" any existing bitmap files. But we got the path wrong; we're in a bare repo, so looking in ".git/" finds nothing. Our "rm" doesn't complain because of the "-f", and when we run "rev-list" there are two bitmap files (ours and jgit's). Our reader implementation will ignore one of the bitmap files, but it's likely non-deterministic which one we will use. We'd prefer the one with the more recent timestamp (just because of the way the packed_git list is sorted), but in most test runs they'd have identical timestamps. So this was probably actually testing something useful about 50% of the time, and other half just testing that we could read our own bitmaps (which is covered elsewhere). Signed-off-by: Jeff King <peff@peff.net> --- Just a cleanup I noticed in the area; can be applied independently from patch 2. t/t5310-pack-bitmaps.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)