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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 26, 2022, 21:15 UTC
Message-ID: <xmqqfsfaffs1.fsf@gitster.g>
In-Reply-To: <eb151752b8de355ac334507e57dc95aadc9ef2bf.1666815209.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 29 quoted lines

> 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.

I do not offhand see anything after the location of the new call above and before this location that may negatively get affected by making the call to prepare_repo_settings() too early, so this change should be safe, I guess.

Show 10 quoted lines

> diff --git a/repo-settings.c b/repo-settings.c
> index e8b58151bc..3021921c53 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -43,6 +43,7 @@ void prepare_repo_settings(struct repository *r)
>  	/* Defaults modified by feature.* */
>  	if (experimental) {
>  		r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
> +		r->settings.gc_cruft_packs = 1;
>  	}

OK.

Show 12 quoted lines

> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index 9110a39088..628dfeb737 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -221,6 +221,11 @@ assert_cruft_pack_exists () {
>  	done <packs
>  }
>  
> +refute_cruft_packs_exist () {
> +	find .git/objects/pack -name "*.mtimes" >mtimes &&
> +	test_must_be_empty mtimes
> +}

Hmph, not "assert_no_cruft_packs"?

Show 18 quoted lines

>  test_expect_success 'gc --cruft generates a cruft pack' '
>  	test_when_finished "rm -fr crufts" &&
>  	git init crufts &&
> @@ -245,6 +250,42 @@ test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
>  	)
>  '
>  
> +test_expect_success 'feature.experimental=true generates a cruft pack' '
> +	git init crufts &&
> +	test_when_finished "rm -fr crufts" &&
> +	(
> +		cd crufts &&
> +
> +		prepare_cruft_history &&
> +		git -c feature.experimental=true gc &&
> +		assert_cruft_pack_exists
> +	)
> +'

OK.

Show 11 quoted lines

> +test_expect_success 'feature.experimental=false allows explicit cruft packs' '
> +	git init crufts &&
> +	test_when_finished "rm -fr crufts" &&
> +	(
> +		cd crufts &&
> +
> +		prepare_cruft_history &&
> +		git -c gc.cruftPacks=true -c feature.experimental=false gc &&
> +		assert_cruft_pack_exists
> +	)
> +'

OK.

Show 11 quoted lines

> +test_expect_success 'feature.experimental=false avoids cruft packs by default' '
> +	git init crufts &&
> +	test_when_finished "rm -fr crufts" &&
> +	(
> +		cd crufts &&
> +
> +		prepare_cruft_history &&
> +		git -c feature.experimental=false gc &&
> +		refute_cruft_packs_exist
> +	)
> +'

OK.

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.

Thanks.

>  run_and_wait_for_auto_gc () {
>  	# We read stdout from gc for the side effect of waiting until the
>  	# background gc process exits, closing its fd 9.  Furthermore, the