On Wed, Oct 26, 2022 at 02:15:10PM -0700, Junio C Hamano wrote: > Taylor Blau <me@xxxxxxxxxxxx> writes: > > > diff --git a/builtin/gc.c b/builtin/gc.c > > index 243ee85d28..5a84f791ef 100644 > > --- a/builtin/gc.c > > +++ b/builtin/gc.c > > @@ -42,7 +42,7 @@ static const char * const builtin_gc_usage[] = { > > > > static int pack_refs = 1; > > static int prune_reflogs = 1; > > -static int cruft_packs = 0; > > +static int cruft_packs = -1; > > static int aggressive_depth = 50; > > static int aggressive_window = 250; > > static int gc_auto_threshold = 6700; > > @@ -593,6 +593,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix) > > if (prune_expire && parse_expiry_date(prune_expire, &dummy)) > > die(_("failed to parse prune expiry value %s"), prune_expire); > > > > + prepare_repo_settings(the_repository); > > + if (cruft_packs < 0) > > + cruft_packs = the_repository->settings.gc_cruft_packs; > > + > > if (aggressive) { > > strvec_push(&repack, "-f"); > > if (aggressive_depth > 0) > > @@ -704,7 +708,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix) > > clean_pack_garbage(); > > } > > > > - prepare_repo_settings(the_repository); > > It is curious why we had this call so late in the sequence in the > original. This is well past what can be reasonably called "start-up". > We have locked the repository, we may have daemonized ourselves, we > may already have packed loose refs and pruned reflogs. Was that due > to somewhat lazy thinking that the next line is the first one that > requires the repository's settings already prepared, I wonder. The latter. The general practice I've observed with prepare_repo_settings() is that it is supposed to be called lazily, just before the first piece of code in a particular file that is going to read the repo settings. Since the function is a noop on any subsequent calls, we can afford to be over-eager placing it around. So it would have been equally OK to duplicate the call, but it's unnecessary since the first call is entered unconditionally from within cmd_gc(). > > +refute_cruft_packs_exist () { > > + find .git/objects/pack -name "*.mtimes" >mtimes && > > + test_must_be_empty mtimes > > +} > > Hmph, not "assert_no_cruft_packs"? Heh, sorry :-). > One thing missing is a test for the escape hatch for those who opt > into the experimental world when gc.cruftPacks feature is broken. > IOW, > > git -c feature.experimental=true -c gc.cruftPacks=false > > should allow them to opt out of gc.cruftPacks. > > Other than that, looking good. Indeed, thanks for spotting it. I'll send a reroll with the additional test. Thanks, Taylor