Re: [PATCH v2 2/2] config: let feature.experimental imply gc.cruftPacks=true

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 26, 2022, 21:32 UTC
Message-ID: <Y1mng9cqAyEqQIQM@nand.local>
In-Reply-To: <xmqqfsfaffs1.fsf@gitster.g>

On Wed, Oct 26, 2022 at 02:15:10PM -0700, Junio C Hamano wrote:

Show 38 quoted lines

> Taylor Blau <me@ttaylorr.com> 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().

Show 6 quoted lines

> > +refute_cruft_packs_exist () {
> > +	find .git/objects/pack -name "*.mtimes" >mtimes &&
> > +	test_must_be_empty mtimes
> > +}
>
> Hmph, not "assert_no_cruft_packs"?

Heh, sorry :-).

Show 9 quoted lines

> 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