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
