Re: [PATCH 09/10] builtin/gc.c: make `gc.cruftPacks` enabled by default

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 18, 2023, 19:52 UTC
Message-ID: <ZD71Dxa+m7/p546d@nand.local>
In-Reply-To: <20230418110049.GB516715@coredump.intra.peff.net>

On Tue, Apr 18, 2023 at 07:00:49AM -0400, Jeff King wrote:

Show 15 quoted lines

> On Mon, Apr 17, 2023 at 04:54:39PM -0400, Taylor Blau wrote:
>
> >  for argv in \
> > +	"gc" \
> >  	"gc --cruft" \
> >  	"-c gc.cruftPacks=true gc" \
> > -	"-c gc.cruftPacks=false gc --cruft" \
> > -	"-c feature.experimental=true gc" \
> > -	"-c gc.cruftPacks=true -c feature.experimental=false gc"
> > +	"-c gc.cruftPacks=false gc --cruft"
> >  do
>
> Oh good. I was a little sad to see the increase in the size of this loop
> in the earlier patches, so now reducing the number of combinations is a
> welcome change.

Sorry for the roller-coaster of emotions ;-).

> The set you have here looks fine, though isn't "gc --cruft" redundant
> with "gc" now?

Yeah, these are redundant. I figured that it might be good to test all cases, but I think this matrix-style of testing only gets you so far, and can often come out wasteful.

"gc --cruft" isn't testing anything meaningful, so let's drop it.

Show 12 quoted lines

> > @@ -246,9 +245,7 @@ done
> >  for argv in \
> >  	"gc --no-cruft" \
> >  	"-c gc.cruftPacks=false gc" \
> > -	"-c gc.cruftPacks=true gc --no-cruft" \
> > -	"-c feature.expiremental=true -c gc.cruftPacks=false gc" \
> > -	"-c feature.experimental=false gc"
> > +	"-c gc.cruftPacks=true gc --no-cruft"
>
> Likewise here, "gc --no-cruft" would have been redundant with "gc"
> before this patch, but we did not even bother with it (so no need to
> remove it here!).

Yeah, this one we definitely want to keep. We probably *could* have dropped it in the previous patch and brought it back here, but I think that doing so would have been unnecessary churn for reviewers reading this series.

> The rest of the patch looks good, and I am quite on board with the
> overall goal. It's been a long time coming. :)

Thanks, I agree ;-).

Thanks, Taylor