Re: [PATCH v3 07/10] t: prepare for GIT_TEST_WRITE_REV_INDEX

From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, dstolee@microsoft.com, gitster@pobox.com,
	jrnieder@gmail.com
Subject: Re: [PATCH v3 07/10] t: prepare for GIT_TEST_WRITE_REV_INDEX
Date: Thu, 28 Jan 2021 20:21:50 -0500	[thread overview]
Message-ID: <YBNjLqOe1NtpziV7@nand.local> (raw)
In-Reply-To: <YBNasXXZc1BEkMxH@coredump.intra.peff.net>

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@coredump.intra.peff.net/
>
> 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@evledraar.gmail.com/

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