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

[Junio C Hamano <gitster@pobox.com>]

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.

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.

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

> 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"?

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

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

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