On Tue, Apr 18, 2023 at 07:00:49AM -0400, Jeff King wrote: > 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. > > @@ -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
