Re: [PATCH 7/7] t: invert `GIT_TEST_WRITE_REV_INDEX`

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 11, 2023, 21:33 UTC
Message-ID: <ZDXSHal5IBynUTMX@nand.local>
In-Reply-To: <18a54c6e-7357-42ca-779f-2ba3e6653880@github.com>

On Tue, Apr 11, 2023 at 09:51:06AM -0400, Derrick Stolee wrote:

Show 10 quoted lines

> On 4/10/2023 6:53 PM, Taylor Blau wrote:
> > Instead of getting rid of the option, invert its meaning to instead
> > disable writing ".rev" files, thereby running the test suite in a mode
> > where the reverse index is generated from scratch.
> >
> > This ensures that we are still running and exercising Git's behavior
> > when forced to generate reverse indexes from scratch.
>
> I don't think this is true because you remove the environment
> variable from the following test.

This is intentional; I wrote that last sentence ("This ensures...") with an implied "[When set], this ensures..." at the beginning of it. IOW, if you wanted to run the test suite with primarily in-memory reverse indexes, you still could.

Show 14 quoted lines

> Replacing the line with GIT_TEST_NO_WRITE_REV_INDEX=1 would keep us
> testing the from-scratch case as a side-effect in other tests.
>
> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> > index b098e10f52..e9fbfb6345 100755
> > --- a/ci/run-build-and-tests.sh
> > +++ b/ci/run-build-and-tests.sh
> > @@ -27,7 +27,6 @@ linux-TEST-vars)
> >  	export GIT_TEST_MULTI_PACK_INDEX=1
> >  	export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
> >  	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
> > -	export GIT_TEST_WRITE_REV_INDEX=1
> >  	export GIT_TEST_CHECKOUT_WORKERS=2
> >  	;;

I wasn't quite sure what to do here. On one hand, sticking GIT_TEST_NO_WRITE_REV_INDEX=1 here would ensure that the linux-TEST-vars test is still exercising the old code.

Is that desirable? I dunno. On one hand it increases our coverage, but on the other hand I've always treated this suite as for experimental features, not older ones.

But I think all things being equal, I'd rather have more CI coverage than not, so I'll add it back in. Thanks for a sanity check on this one.

Thanks, Taylor