On Thu, Jan 28, 2021 at 07:45:37PM -0500, Jeff King wrote: > On Mon, Jan 25, 2021 at 06:37:38PM -0500, Taylor Blau wrote: > > > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh > > index 297de502a9..2fc3aadbd1 100755 > > --- a/t/t5319-multi-pack-index.sh > > +++ b/t/t5319-multi-pack-index.sh > > @@ -710,8 +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 >a-pack-files && > > - test_line_count = 3 a-pack-files && > > + test_path_is_file $PACKA.idx && > > + test_path_is_file $PACKA.keep && > > + test_path_is_file $PACKA.pack && > > I find the post-image here more readable than the original. It probably > runs faster, too (zero processes instead of three). > > > --- 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 > > I think we had a discussion a while ago about sane_unset outside of an > &&-chain, where it does nothing that regular "unset" would not. But I > don't know if we reached any kind of conclusion. I actually argued that > it was fine in: > > https://lore.kernel.org/git/20200630185536.GA1888406@xxxxxxxxxxxxxxxxxxxxxxx/ > > So I guess I should probably stand by that. ;) I think I probably took this from the trace2 tests? Not sure. I'm glad that it's not wrong, strictly speaking. This is another instance that I'd be happy to send a follow-up patch to get rid of all of these at once, unless you feel strongly that it should be changed in this series before applying. > > --- a/t/t5604-clone-reference.sh > > +++ b/t/t5604-clone-reference.sh > > @@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at obje > > for raw in $(ls T*.raw) > > do > > sed -e "s!/../!/Y/!; s![0-9a-f]\{38,\}!Z!" -e "/commit-graph/d" \ > > - -e "/multi-pack-index/d" <$raw >$raw.de-sha-1 && > > + -e "/multi-pack-index/d" -e "/rev/d" <$raw >$raw.de-sha-1 && > > This one is less readable than before. :) I'm not sure how to really > improve that, though. Yeah, this follows from a suggestion that Ævar gave in: https://lore.kernel.org/git/878s8y5bdn.fsf@xxxxxxxxxxxxxxxxxxx/ FWIW, I found that his suggestion was *clearer* than what was in v2. But I get that others may feel differently, too. > > --- a/t/t5702-protocol-v2.sh > > +++ b/t/t5702-protocol-v2.sh > > @@ -851,8 +851,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/* >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 > > ' > > Hmm. Too bad we can't rely on shell brace expansion, as: > > ls http_child/.git/objects/pack/*.{pack,idx} > > would be more readable. You could still do it in a single "ls" by > writing out both arguments manually, but it's probably not that > important. Agreed on all of that. > This one is making the test a bit looser (it would miss a case where we > somehow failed to generate the .idx). That seems like an unlikely bug, > but I wonder if we can keep the original behavior. I guess: > > ls .git/objects/pack/*.pack \ > .git/objects/pack/*.idx | > sort >post_packs > > would work? Sure, I see what you're saying. To be honest, I'm skeptical that we'd have a bug which failed only this one test, so I'm hesitant to send a replacement/reroll for this alone. If you feel strongly, though, I'm happy to change it. (But, I'll err on the side of leaving it as-is, since you indicated in your response to v3's cover letter that you'd be OK if I discarded some or all of your suggestions). > > test_expect_success 'gc --no-quiet' ' > > diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh > > index 3d17e932a0..8f1caf8025 100755 > > --- a/t/t9300-fast-import.sh > > +++ b/t/t9300-fast-import.sh > > @@ -1632,7 +1632,10 @@ 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) && > > + ls -la .git/objects/pack/pack-*.pack >packlist && > > + ls -la .git/objects/pack/pack-*.pack >idxlist && > > + test_line_count = 4 idxlist && > > + test_line_count = 4 packlist && > > Another case where I don't think we're losing anything (actually, we are > gaining just slightly; if a bug somehow created 6 packs and 2 idx files, > we'd now notice!). Yay! > -Peff Thanks, Taylor
