| Message ID | 2288571fbeb83d6bd98917f4788c3221aa973c81.1610129989.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-revindex: introduce on-disk '.rev' format | expand |
On Fri, Jan 08 2021, Taylor Blau wrote: > For now, sprinkle these locations with a 'grep -v "\.rev$"' to ignore > them. Once the pack.writeReverseIndex option has been thoroughly > tested, we will default it to 'true', removing GIT_TEST_WRITE_REV_INDEX, > and making it possible to revert this patch. Maybe some of it we can change/revert, but some of it just seems to be test warts we can fix: > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh > index 297de502a9..9696f88c2f 100755 > --- a/t/t5319-multi-pack-index.sh > +++ b/t/t5319-multi-pack-index.sh > @@ -710,7 +710,7 @@ test_expect_success 'expire respects .keep files' ' > PACKA=$(ls .git/objects/pack/a-pack*\.pack | sed s/\.pack\$//) && > touch $PACKA.keep && > git multi-pack-index expire && > - ls -S .git/objects/pack/a-pack* | grep $PACKA >a-pack-files && > + ls -S .git/objects/pack/a-pack* | grep $PACKA | grep -v "\.rev$" >a-pack-files && This seems to be testing "a *.keep file made my pack not expire". Can't it just check for *.{pack,idx,keep} or even just *.pack? > test_line_count = 3 a-pack-files && > test-tool read-midx .git/objects | grep idx >midx-list && > test_line_count = 2 midx-list > diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh > index 431699ade2..5a59cc71e4 100755 > --- a/t/t5325-reverse-index.sh > +++ b/t/t5325-reverse-index.sh > @@ -3,6 +3,10 @@ > test_description='on-disk reverse index' > . ./test-lib.sh > > +# The below tests want control over the 'pack.writeReverseIndex' setting > +# themselves to assert various combinations of it with other options. > +sane_unset GIT_TEST_WRITE_REV_INDEX > + > packdir=.git/objects/pack > > test_expect_success 'setup' ' > diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh > index 2f7be23044..d064426d03 100755 > --- a/t/t5604-clone-reference.sh > +++ b/t/t5604-clone-reference.sh > @@ -318,7 +318,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at obje > test_cmp T.objects T$option.objects && > ( > cd T$option/.git/objects && > - find . -type f | sort >../../../T$option.objects-files.raw && > + find . -type f | grep -v \.rev$ | sort >../../../T$option.objects-files.raw && > find . -type l | sort >../../../T$option.objects-symlinks.raw There's an existing loop just below that where we grep out /commit-graph/, /multi-pack-index/ etc. whith other test modes add to the objects directory with sed. Seems like this belongs there, not in the find above it. > ) > done && > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 7d5b17909b..9ebf045739 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -848,7 +848,7 @@ test_expect_success 'part of packfile response provided as URI' ' > test -f h2found && > > # Ensure that there are exactly 6 files (3 .pack and 3 .idx). > - ls http_child/.git/objects/pack/* >filelist && > + ls http_child/.git/objects/pack/* | grep -v \.rev$ >filelist && > test_line_count = 6 filelist > ' Maybe just check *.{pack,idx,keep}. I was looking at that code the other day and it's really just being overly specific. It really just cares about the *.pack files. > @@ -902,7 +902,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects' ' > clone "$HTTPD_URL/smart/http_parent" http_child && > > # Ensure that there are exactly 4 files (2 .pack and 2 .idx). > - ls http_child/.git/objects/pack/* >filelist && > + ls http_child/.git/objects/pack/* | grep -v \.rev$ >filelist && ditto. > test_line_count = 4 filelist > ' > > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh > index 4a3b8f48ac..d52f92f5a1 100755 > --- a/t/t6500-gc.sh > +++ b/t/t6500-gc.sh > @@ -106,13 +106,13 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre > test_commit "$(test_oid obj2)" && > # Our first gc will create a pack; our second will create a second pack > git gc --auto && > - ls .git/objects/pack | sort >existing_packs && > + ls .git/objects/pack | grep -v \.rev$ | sort >existing_packs && > test_commit "$(test_oid obj3)" && > test_commit "$(test_oid obj4)" && > > git gc --auto 2>err && > test_i18ngrep ! "^warning:" err && > - ls .git/objects/pack/ | sort >post_packs && > + ls .git/objects/pack/ | grep -v \.rev$ | sort >post_packs && > comm -1 -3 existing_packs post_packs >new && > comm -2 -3 existing_packs post_packs >del && > test_line_count = 0 del && # No packs are deleted This is all part of account where we later use comm/wc -l to check how many new packs we have,so just check *.pack? > diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh > index 308c1ef42c..100df52a71 100755 > --- a/t/t9300-fast-import.sh > +++ b/t/t9300-fast-import.sh > @@ -1629,7 +1629,7 @@ test_expect_success 'O: blank lines not necessary after other commands' ' > INPUT_END > > git fast-import <input && > - test 8 = $(find .git/objects/pack -type f | grep -v multi-pack-index | wc -l) && > + test 8 = $(find .git/objects/pack -type f \( -name "*.idx" -o -name "*.pack" \) | wc -l) && Yay, there the existing multi-pack-index case is amended in a future-proof way :)
Hi Ævar, Your suggestions are quite helpful, and I'm glad to apply them, especially if it means that this "clean up" patch can actually harden us from similar changes in the future. On Tue, Jan 12, 2021 at 06:11:00PM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Jan 08 2021, Taylor Blau wrote: > > > For now, sprinkle these locations with a 'grep -v "\.rev$"' to ignore > > them. Once the pack.writeReverseIndex option has been thoroughly > > tested, we will default it to 'true', removing GIT_TEST_WRITE_REV_INDEX, > > and making it possible to revert this patch. > > Maybe some of it we can change/revert, but some of it just seems to be > test warts we can fix: > > > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh > > index 297de502a9..9696f88c2f 100755 > > --- a/t/t5319-multi-pack-index.sh > > +++ b/t/t5319-multi-pack-index.sh > > @@ -710,7 +710,7 @@ test_expect_success 'expire respects .keep files' ' > > PACKA=$(ls .git/objects/pack/a-pack*\.pack | sed s/\.pack\$//) && > > touch $PACKA.keep && > > git multi-pack-index expire && > > - ls -S .git/objects/pack/a-pack* | grep $PACKA >a-pack-files && > > + ls -S .git/objects/pack/a-pack* | grep $PACKA | grep -v "\.rev$" >a-pack-files && > > This seems to be testing "a *.keep file made my pack not expire". Can't > it just check for *.{pack,idx,keep} or even just *.pack? Yeah, and I think the simplest thing to do here is just check that these files exist at all. Something like: diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 9696f88c2f..f5e50508c9 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -710,7 +710,9 @@ test_expect_success 'expire respects .keep files' ' PACKA=$(ls .git/objects/pack/a-pack*\.pack | sed s/\.pack\$//) && touch $PACKA.keep && git multi-pack-index expire && - ls -S .git/objects/pack/a-pack* | grep $PACKA | grep -v "\.rev$" >a-pack-files && + test_is_file .git/objects/pack/pack-a-$PACKA.idx && + test_is_file .git/objects/pack/pack-a-$PACKA.keep && + test_is_file .git/objects/pack/pack-a-$PACKA.pack && test_line_count = 3 a-pack-files && test-tool read-midx .git/objects | grep idx >midx-list && test_line_count = 2 midx-list > > diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh > > index 2f7be23044..d064426d03 100755 > > --- a/t/t5604-clone-reference.sh > > +++ b/t/t5604-clone-reference.sh > > @@ -318,7 +318,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at obje > > test_cmp T.objects T$option.objects && > > ( > > cd T$option/.git/objects && > > - find . -type f | sort >../../../T$option.objects-files.raw && > > + find . -type f | grep -v \.rev$ | sort >../../../T$option.objects-files.raw && > > find . -type l | sort >../../../T$option.objects-symlinks.raw > > There's an existing loop just below that where we grep out > /commit-graph/, /multi-pack-index/ etc. whith other test modes add to > the objects directory with sed. Seems like this belongs there, not in > the find above it. Much cleaner! Thank you. > > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > > index 7d5b17909b..9ebf045739 100755 > > --- a/t/t5702-protocol-v2.sh > > +++ b/t/t5702-protocol-v2.sh > > @@ -848,7 +848,7 @@ test_expect_success 'part of packfile response provided as URI' ' > > test -f h2found && > > > > # Ensure that there are exactly 6 files (3 .pack and 3 .idx). > > - ls http_child/.git/objects/pack/* >filelist && > > + ls http_child/.git/objects/pack/* | grep -v \.rev$ >filelist && > > test_line_count = 6 filelist > > ' > > Maybe just check *.{pack,idx,keep}. I was looking at that code the other > day and it's really just being overly specific. It really just cares > about the *.pack files. I think here and in t6500 as well as t9300 the easiest thing to do is just diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 9ebf045739..73cd9e3ff6 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -848,8 +848,10 @@ test_expect_success 'part of packfile response provided as URI' ' test -f h2found && # Ensure that there are exactly 6 files (3 .pack and 3 .idx). - ls http_child/.git/objects/pack/* | grep -v \.rev$ >filelist && - test_line_count = 6 filelist + ls http_child/.git/objects/pack/*.pack >packlist && + ls http_child/.git/objects/pack/*.idx >idxlist && + test_line_count = 3 idxlist && + test_line_count = 3 packlist ' > > diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh > > index 308c1ef42c..100df52a71 100755 > > --- a/t/t9300-fast-import.sh > > +++ b/t/t9300-fast-import.sh > > @@ -1629,7 +1629,7 @@ test_expect_success 'O: blank lines not necessary after other commands' ' > > INPUT_END > > > > git fast-import <input && > > - test 8 = $(find .git/objects/pack -type f | grep -v multi-pack-index | wc -l) && > > + test 8 = $(find .git/objects/pack -type f \( -name "*.idx" -o -name "*.pack" \) | wc -l) && > > Yay, there the existing multi-pack-index case is amended in a > future-proof way :) Here's actually a spot where I'm unhappy with the resulting complexity, and I think that it would be much cleaner if we did the same packlist+idxlist thing and then checked the line count of each. Thanks, Taylor
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 297de502a9..9696f88c2f 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -710,7 +710,7 @@ test_expect_success 'expire respects .keep files' ' PACKA=$(ls .git/objects/pack/a-pack*\.pack | sed s/\.pack\$//) && touch $PACKA.keep && git multi-pack-index expire && - ls -S .git/objects/pack/a-pack* | grep $PACKA >a-pack-files && + ls -S .git/objects/pack/a-pack* | grep $PACKA | grep -v "\.rev$" >a-pack-files && test_line_count = 3 a-pack-files && test-tool read-midx .git/objects | grep idx >midx-list && test_line_count = 2 midx-list diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh index 431699ade2..5a59cc71e4 100755 --- a/t/t5325-reverse-index.sh +++ b/t/t5325-reverse-index.sh @@ -3,6 +3,10 @@ test_description='on-disk reverse index' . ./test-lib.sh +# The below tests want control over the 'pack.writeReverseIndex' setting +# themselves to assert various combinations of it with other options. +sane_unset GIT_TEST_WRITE_REV_INDEX + packdir=.git/objects/pack test_expect_success 'setup' ' diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh index 2f7be23044..d064426d03 100755 --- a/t/t5604-clone-reference.sh +++ b/t/t5604-clone-reference.sh @@ -318,7 +318,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at obje test_cmp T.objects T$option.objects && ( cd T$option/.git/objects && - find . -type f | sort >../../../T$option.objects-files.raw && + find . -type f | grep -v \.rev$ | sort >../../../T$option.objects-files.raw && find . -type l | sort >../../../T$option.objects-symlinks.raw ) done && diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 7d5b17909b..9ebf045739 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -848,7 +848,7 @@ test_expect_success 'part of packfile response provided as URI' ' test -f h2found && # Ensure that there are exactly 6 files (3 .pack and 3 .idx). - ls http_child/.git/objects/pack/* >filelist && + ls http_child/.git/objects/pack/* | grep -v \.rev$ >filelist && test_line_count = 6 filelist ' @@ -902,7 +902,7 @@ test_expect_success 'packfile-uri with transfer.fsckobjects' ' clone "$HTTPD_URL/smart/http_parent" http_child && # Ensure that there are exactly 4 files (2 .pack and 2 .idx). - ls http_child/.git/objects/pack/* >filelist && + ls http_child/.git/objects/pack/* | grep -v \.rev$ >filelist && test_line_count = 4 filelist ' diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index 4a3b8f48ac..d52f92f5a1 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -106,13 +106,13 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre test_commit "$(test_oid obj2)" && # Our first gc will create a pack; our second will create a second pack git gc --auto && - ls .git/objects/pack | sort >existing_packs && + ls .git/objects/pack | grep -v \.rev$ | sort >existing_packs && test_commit "$(test_oid obj3)" && test_commit "$(test_oid obj4)" && git gc --auto 2>err && test_i18ngrep ! "^warning:" err && - ls .git/objects/pack/ | sort >post_packs && + ls .git/objects/pack/ | grep -v \.rev$ | sort >post_packs && comm -1 -3 existing_packs post_packs >new && comm -2 -3 existing_packs post_packs >del && test_line_count = 0 del && # No packs are deleted diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 308c1ef42c..100df52a71 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -1629,7 +1629,7 @@ test_expect_success 'O: blank lines not necessary after other commands' ' INPUT_END git fast-import <input && - test 8 = $(find .git/objects/pack -type f | grep -v multi-pack-index | wc -l) && + test 8 = $(find .git/objects/pack -type f \( -name "*.idx" -o -name "*.pack" \) | wc -l) && test $(git rev-parse refs/tags/O3-2nd) = $(git rev-parse O3^) && git log --reverse --pretty=oneline O3 | sed s/^.*z// >actual && test_cmp expect actual
In the next patch, we'll add support for unconditionally enabling the 'pack.writeReverseIndex' setting with a new GIT_TEST_WRITE_REV_INDEX environment variable. This causes a little bit of fallout with tests that, for example, compare the list of files in the pack directory being unprepared to see .rev files in its output. For now, sprinkle these locations with a 'grep -v "\.rev$"' to ignore them. Once the pack.writeReverseIndex option has been thoroughly tested, we will default it to 'true', removing GIT_TEST_WRITE_REV_INDEX, and making it possible to revert this patch. At that time, we'll have to adjust the expected output to contain the relevant .rev files, but for now this will do just fine. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t5319-multi-pack-index.sh | 2 +- t/t5325-reverse-index.sh | 4 ++++ t/t5604-clone-reference.sh | 2 +- t/t5702-protocol-v2.sh | 4 ++-- t/t6500-gc.sh | 4 ++-- t/t9300-fast-import.sh | 2 +- 6 files changed, 11 insertions(+), 7 deletions(-)