[PATCH v2 0/2] gc: let feature.experimental imply gc.cruftPacks

[PATCH v2 0/2] gc: let feature.experimental imply gc.cruftPacks

[Taylor Blau]

This is a moderately-sized reroll of Emily's series from [1], which I
saw while sifting through old mail in my inbox.

That series was sent when I was on vacation, which is a likely
explanation for why I most likely missed it.

I'm sending a reroll out on Emily's behalf, since the series hasn't
received any activity in a few months, and I would like to see the
topic pushed forward.

Changes since last time are included in a range-diff below, but the
major points are:

  - extracted common components of the test script into helper
    functions
  - reordered the repository creation/teardown so that
    test_when_finished "rm -fr ..." happens before "git init"
  - and centralized checking feature.experimental into
    repo-settings.c

Thanks in advance for your review.

[1]: https://lore.kernel.org/git/20220803205721.3686361-1-emilyshaffer@google.com/

Emily Shaffer (2):
  gc: add tests for --cruft and friends
  config: let feature.experimental imply gc.cruftPacks=true

 Documentation/config/feature.txt |  3 ++
 builtin/gc.c                     |  7 ++-
 repo-settings.c                  |  1 +
 repository.h                     |  1 +
 t/t6500-gc.sh                    | 84 ++++++++++++++++++++++++++++++++
 5 files changed, 94 insertions(+), 2 deletions(-)

Range-diff against v1:
1:  bf243d15c1 ! 1:  35d2c97715 gc: add tests for --cruft and friends
    @@ Commit message
         Add some tests to exercise these options to gc in the gc test suite.
     
         Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
    +    Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## t/t6500-gc.sh ##
     @@ t/t6500-gc.sh: test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e
      	grep -E "^trace: (built-in|exec|run_command): git reflog expire --" trace.out
      '
      
    ++prepare_cruft_history () {
    ++	test_commit base &&
    ++
    ++	test_commit --no-tag foo &&
    ++	test_commit --no-tag bar &&
    ++	git reset HEAD^^
    ++}
    ++
    ++assert_cruft_pack_exists () {
    ++	find .git/objects/pack -name "*.mtimes" >mtimes &&
    ++	sed -e 's/\.mtimes$/\.pack/g' mtimes >packs &&
    ++
    ++	test_file_not_empty packs &&
    ++	while read pack
    ++	do
    ++		test_path_is_file "$pack" || return 1
    ++	done <packs
    ++}
    ++
     +test_expect_success 'gc --cruft generates a cruft pack' '
    -+	git init crufts &&
     +	test_when_finished "rm -fr crufts" &&
    ++	git init crufts &&
     +	(
     +		cd crufts &&
    -+		test_commit base &&
    -+
    -+		test_commit --no-tag foo &&
    -+		test_commit --no-tag bar &&
    -+		git reset HEAD^^ &&
     +
    ++		prepare_cruft_history &&
     +		git gc --cruft &&
    -+
    -+		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
    -+		test_path_is_file .git/objects/pack/$cruft.pack
    ++		assert_cruft_pack_exists
     +	)
     +'
     +
     +test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
    -+	git init crufts &&
     +	test_when_finished "rm -fr crufts" &&
    ++	git init crufts &&
     +	(
     +		cd crufts &&
    -+		test_commit base &&
    -+
    -+		test_commit --no-tag foo &&
    -+		test_commit --no-tag bar &&
    -+		git reset HEAD^^ &&
     +
    ++		prepare_cruft_history &&
     +		git -c gc.cruftPacks=true gc &&
    -+
    -+		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
    -+		test_path_is_file .git/objects/pack/$cruft.pack
    ++		assert_cruft_pack_exists
     +	)
     +'
     +
2:  5a242e2370 ! 2:  eb151752b8 config: let feature.experimental imply gc.cruftPacks=true
    @@ Commit message
         config: let feature.experimental imply gc.cruftPacks=true
     
         We are interested in exploring whether gc.cruftPacks=true should become
    -    the default value; to determine whether it is safe to do so, let's
    -    encourage more users to try it out. Users who have set
    -    feature.experimental=true have already volunteered to try new and
    -    possibly-breaking config changes, so let's try this new default with
    -    that set of users.
    +    the default value.
    +
    +    To determine whether it is safe to do so, let's encourage more users to
    +    try it out.
    +
    +    Users who have set feature.experimental=true have already volunteered to
    +    try new and possibly-breaking config changes, so let's try this new
    +    default with that set of users.
     
         Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
    +    Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## Documentation/config/feature.txt ##
     @@ Documentation/config/feature.txt: feature.experimental::
      +
      * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by
      skipping more commits at a time, reducing the number of round trips.
    +++
     +* `gc.cruftPacks=true` reduces disk space used by unreachable objects during
    -+garbage collection.
    ++garbage collection, preventing loose object explosions.
      
      feature.manyFiles::
      	Enable config options that optimize for repos with many files in the
     
      ## builtin/gc.c ##
    -@@ builtin/gc.c: static int gc_config_is_timestamp_never(const char *var)
    - static void gc_config(void)
    - {
    - 	const char *value;
    -+	int experimental = 0;
    +@@ builtin/gc.c: static const char * const builtin_gc_usage[] = {
      
    - 	if (!git_config_get_value("gc.packrefs", &value)) {
    - 		if (value && !strcmp(value, "notbare"))
    -@@ builtin/gc.c: static void gc_config(void)
    - 	    gc_config_is_timestamp_never("gc.reflogexpireunreachable"))
    - 		prune_reflogs = 0;
    + 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;
    +@@ builtin/gc.c: 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);
      
    -+	/* feature.experimental implies gc.cruftPacks=true */
    -+	git_config_get_bool("feature.experimental", &experimental);
    -+	if (experimental)
    -+		cruft_packs = 1;
    ++	prepare_repo_settings(the_repository);
    ++	if (cruft_packs < 0)
    ++		cruft_packs = the_repository->settings.gc_cruft_packs;
     +
    - 	git_config_get_int("gc.aggressivewindow", &aggressive_window);
    - 	git_config_get_int("gc.aggressivedepth", &aggressive_depth);
    - 	git_config_get_int("gc.auto", &gc_auto_threshold);
    + 	if (aggressive) {
    + 		strvec_push(&repack, "-f");
    + 		if (aggressive_depth > 0)
    +@@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)
    + 		clean_pack_garbage();
    + 	}
    + 
    +-	prepare_repo_settings(the_repository);
    + 	if (the_repository->settings.gc_write_commit_graph == 1)
    + 		write_commit_graph_reachable(the_repository->objects->odb,
    + 					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
    +
    + ## repo-settings.c ##
    +@@ repo-settings.c: 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;
    + 	}
    + 	if (manyfiles) {
    + 		r->settings.index_version = 4;
    +
    + ## repository.h ##
    +@@ repository.h: struct repo_settings {
    + 	int commit_graph_generation_version;
    + 	int commit_graph_read_changed_paths;
    + 	int gc_write_commit_graph;
    ++	int gc_cruft_packs;
    + 	int fetch_write_commit_graph;
    + 	int command_requires_full_index;
    + 	int sparse_index;
     
      ## t/t6500-gc.sh ##
    +@@ t/t6500-gc.sh: assert_cruft_pack_exists () {
    + 	done <packs
    + }
    + 
    ++refute_cruft_packs_exist () {
    ++	find .git/objects/pack -name "*.mtimes" >mtimes &&
    ++	test_must_be_empty mtimes
    ++}
    ++
    + test_expect_success 'gc --cruft generates a cruft pack' '
    + 	test_when_finished "rm -fr crufts" &&
    + 	git init crufts &&
     @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
      	)
      '
    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
     +	test_when_finished "rm -fr crufts" &&
     +	(
     +		cd crufts &&
    -+		test_commit base &&
    -+
    -+		test_commit --no-tag foo &&
    -+		test_commit --no-tag bar &&
    -+		git reset HEAD^^ &&
     +
    ++		prepare_cruft_history &&
     +		git -c feature.experimental=true gc &&
    -+
    -+		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
    -+		test_path_is_file .git/objects/pack/$cruft.pack
    ++		assert_cruft_pack_exists
     +	)
     +'
     +
    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
     +	test_when_finished "rm -fr crufts" &&
     +	(
     +		cd crufts &&
    -+		test_commit base &&
    -+
    -+		test_commit --no-tag foo &&
    -+		test_commit --no-tag bar &&
    -+		git reset HEAD^^ &&
     +
    ++		prepare_cruft_history &&
     +		git -c gc.cruftPacks=true -c feature.experimental=false gc &&
    -+		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
    -+		test_path_is_file .git/objects/pack/$cruft.pack
    ++		assert_cruft_pack_exists
    ++	)
    ++'
    ++
    ++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
     +	)
     +'
     +
-- 
2.38.0.16.g393fd4c6db

[PATCH v2 1/2] gc: add tests for --cruft and friends

[Taylor Blau]

From: Emily Shaffer <emilyshaffer@google.com>

In 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via
loose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'.
'--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that
doesn't check whether a lone gc run generates these cruft packs.
'gc.cruftPacks' is never exercised.

Add some tests to exercise these options to gc in the gc test suite.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6500-gc.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index cd6c53360d..9110a39088 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -202,6 +202,49 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e
 	grep -E "^trace: (built-in|exec|run_command): git reflog expire --" trace.out
 '
 
+prepare_cruft_history () {
+	test_commit base &&
+
+	test_commit --no-tag foo &&
+	test_commit --no-tag bar &&
+	git reset HEAD^^
+}
+
+assert_cruft_pack_exists () {
+	find .git/objects/pack -name "*.mtimes" >mtimes &&
+	sed -e 's/\.mtimes$/\.pack/g' mtimes >packs &&
+
+	test_file_not_empty packs &&
+	while read pack
+	do
+		test_path_is_file "$pack" || return 1
+	done <packs
+}
+
+test_expect_success 'gc --cruft generates a cruft pack' '
+	test_when_finished "rm -fr crufts" &&
+	git init crufts &&
+	(
+		cd crufts &&
+
+		prepare_cruft_history &&
+		git gc --cruft &&
+		assert_cruft_pack_exists
+	)
+'
+
+test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
+	test_when_finished "rm -fr crufts" &&
+	git init crufts &&
+	(
+		cd crufts &&
+
+		prepare_cruft_history &&
+		git -c gc.cruftPacks=true gc &&
+		assert_cruft_pack_exists
+	)
+'
+
 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
-- 
2.38.0.16.g393fd4c6db

Re: [PATCH v2 1/2] gc: add tests for --cruft and friends

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> +prepare_cruft_history () {
> +	test_commit base &&
> +
> +	test_commit --no-tag foo &&
> +	test_commit --no-tag bar &&
> +	git reset HEAD^^
> +}

OK.  Three pearls on a single strand, then the tip-two gets rewound
away.

> +assert_cruft_pack_exists () {
> +	find .git/objects/pack -name "*.mtimes" >mtimes &&
> +	sed -e 's/\.mtimes$/\.pack/g' mtimes >packs &&

Somebody recently made comment that we already depend on the fact
that the default action for "find" is "-print".  I do not see the
need for the 'mtimes' intermediary file.  It does not hurt, but it
is not needed.

> +	test_file_not_empty packs &&
> +	while read pack
> +	do
> +		test_path_is_file "$pack" || return 1
> +	done <packs
> +}

OK.  We enumerate .mtimes files and ensure corresponding .pack
exists for each of them.  That might miss a case where we thought we
created a cruft pack (i.e. .pack exists) but somehow failed to
create the matching .mtimes file by mistake, but it is hard to tell
such a "broken cruft pack" file from a normal pack file, so I think
the above is the best we can do.

> +test_expect_success 'gc --cruft generates a cruft pack' '
> +	test_when_finished "rm -fr crufts" &&
> +	git init crufts &&
> +	(
> +		cd crufts &&
> +
> +		prepare_cruft_history &&
> +		git gc --cruft &&
> +		assert_cruft_pack_exists
> +	)
> +'
> +
> +test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
> +	test_when_finished "rm -fr crufts" &&
> +	git init crufts &&
> +	(
> +		cd crufts &&
> +
> +		prepare_cruft_history &&
> +		git -c gc.cruftPacks=true gc &&
> +		assert_cruft_pack_exists
> +	)
> +'

We could instead do away without when-finished clean-up and use
separate test subdirectory.  Consistently doing so may make CI
forensic easier.  I do not think it is necessary in the test history
used here that is too simple to be realisic.  If something breaks at
CI, it is simple enough to reproduce it manually, so I do not think
it is worth a reroll for that.

>  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

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

[Taylor Blau]

From: Emily Shaffer <emilyshaffer@google.com>

We are interested in exploring whether gc.cruftPacks=true should become
the default value.

To determine whether it is safe to do so, let's encourage more users to
try it out.

Users who have set feature.experimental=true have already volunteered to
try new and possibly-breaking config changes, so let's try this new
default with that set of users.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/feature.txt |  3 +++
 builtin/gc.c                     |  7 ++++--
 repo-settings.c                  |  1 +
 repository.h                     |  1 +
 t/t6500-gc.sh                    | 41 ++++++++++++++++++++++++++++++++
 5 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt
index cdecd04e5b..95975e5091 100644
--- a/Documentation/config/feature.txt
+++ b/Documentation/config/feature.txt
@@ -14,6 +14,9 @@ feature.experimental::
 +
 * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by
 skipping more commits at a time, reducing the number of round trips.
++
+* `gc.cruftPacks=true` reduces disk space used by unreachable objects during
+garbage collection, preventing loose object explosions.
 
 feature.manyFiles::
 	Enable config options that optimize for repos with many files in the
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);
 	if (the_repository->settings.gc_write_commit_graph == 1)
 		write_commit_graph_reachable(the_repository->objects->odb,
 					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
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;
 	}
 	if (manyfiles) {
 		r->settings.index_version = 4;
diff --git a/repository.h b/repository.h
index 24316ac944..6c461c5b9d 100644
--- a/repository.h
+++ b/repository.h
@@ -34,6 +34,7 @@ struct repo_settings {
 	int commit_graph_generation_version;
 	int commit_graph_read_changed_paths;
 	int gc_write_commit_graph;
+	int gc_cruft_packs;
 	int fetch_write_commit_graph;
 	int command_requires_full_index;
 	int sparse_index;
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
+}
+
 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
+	)
+'
+
+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
+	)
+'
+
+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
+	)
+'
+
 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
-- 
2.38.0.16.g393fd4c6db

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

[Junio C Hamano]

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

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

[Taylor Blau]

On Wed, Oct 26, 2022 at 02:15:10PM -0700, Junio C Hamano wrote:
> 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().

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

Re: [PATCH v2 0/2] gc: let feature.experimental imply gc.cruftPacks

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> This is a moderately-sized reroll of Emily's series from [1], which I
> saw while sifting through old mail in my inbox.
>
> That series was sent when I was on vacation, which is a likely
> explanation for why I most likely missed it.
>
> I'm sending a reroll out on Emily's behalf, since the series hasn't
> received any activity in a few months, and I would like to see the
> topic pushed forward.

Yeah, it has been quite a while.  I'd prefer to at least see an Ack
by the original author before proceeding.

In my quick scan of the original round
https://lore.kernel.org/git/20220803205721.3686361-1-emilyshaffer@google.com/

the new round appears to address all the points raised.  It was so
long time ago that I am not sure if reviewers stopped after seeing
minor points that need to be addressed, or there were only minor
issues in the original series, though.

Thanks.

[PATCH v3 0/2] gc: let feature.experimental imply gc.cruftPacks

[Taylor Blau]

This is a small reroll of an adopted version of Emily's series[1], which
I saw while sifting through old mail in my inbox.

This is substantively unchanged from the previous round, save:

  - renaming the helper functions which assert and refute the existence
    of cruft packs, and

  - an additional test which ensures that we can opt out of cruft packs
    via config

A range-diff is included below for convenience.

Thanks in advance for your review.

[1]: https://lore.kernel.org/git/20220803205721.3686361-1-emilyshaffer@google.com/

Emily Shaffer (2):
  gc: add tests for --cruft and friends
  config: let feature.experimental imply gc.cruftPacks=true

 Documentation/config/feature.txt |  3 +
 builtin/gc.c                     |  7 ++-
 repo-settings.c                  |  1 +
 repository.h                     |  1 +
 t/t6500-gc.sh                    | 96 ++++++++++++++++++++++++++++++++
 5 files changed, 106 insertions(+), 2 deletions(-)

Range-diff against v2:
1:  35d2c97715 ! 1:  e8726f3de7 gc: add tests for --cruft and friends
    @@ t/t6500-gc.sh: test_expect_success 'one of gc.reflogExpire{Unreachable,}=never d
     +	git reset HEAD^^
     +}
     +
    -+assert_cruft_pack_exists () {
    ++assert_cruft_packs () {
     +	find .git/objects/pack -name "*.mtimes" >mtimes &&
     +	sed -e 's/\.mtimes$/\.pack/g' mtimes >packs &&
     +
    @@ t/t6500-gc.sh: test_expect_success 'one of gc.reflogExpire{Unreachable,}=never d
     +
     +		prepare_cruft_history &&
     +		git gc --cruft &&
    -+		assert_cruft_pack_exists
    ++		assert_cruft_packs
     +	)
     +'
     +
    @@ t/t6500-gc.sh: test_expect_success 'one of gc.reflogExpire{Unreachable,}=never d
     +
     +		prepare_cruft_history &&
     +		git -c gc.cruftPacks=true gc &&
    -+		assert_cruft_pack_exists
    ++		assert_cruft_packs
     +	)
     +'
     +
2:  eb151752b8 ! 2:  13a25a425b config: let feature.experimental imply gc.cruftPacks=true
    @@ repository.h: struct repo_settings {
      	int sparse_index;
     
      ## t/t6500-gc.sh ##
    -@@ t/t6500-gc.sh: assert_cruft_pack_exists () {
    +@@ t/t6500-gc.sh: assert_cruft_packs () {
      	done <packs
      }
      
    -+refute_cruft_packs_exist () {
    ++assert_no_cruft_packs () {
     +	find .git/objects/pack -name "*.mtimes" >mtimes &&
     +	test_must_be_empty mtimes
     +}
    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
     +
     +		prepare_cruft_history &&
     +		git -c feature.experimental=true gc &&
    -+		assert_cruft_pack_exists
    ++		assert_cruft_packs
     +	)
     +'
     +
    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
     +
     +		prepare_cruft_history &&
     +		git -c gc.cruftPacks=true -c feature.experimental=false gc &&
    -+		assert_cruft_pack_exists
    ++		assert_cruft_packs
    ++	)
    ++'
    ++
    ++test_expect_success 'feature.experimental=true can be overridden' '
    ++	git init crufts &&
    ++	test_when_finished "rm -fr crufts" &&
    ++	(
    ++		cd crufts &&
    ++
    ++		prepare_cruft_history &&
    ++		git -c feature.expiremental=true -c gc.cruftPacks=false gc &&
    ++		assert_no_cruft_packs
     +	)
     +'
     +
    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
     +
     +		prepare_cruft_history &&
     +		git -c feature.experimental=false gc &&
    -+		refute_cruft_packs_exist
    ++		assert_no_cruft_packs
     +	)
     +'
     +
-- 
2.38.0.16.g393fd4c6db

[PATCH v3 1/2] gc: add tests for --cruft and friends

[Taylor Blau]

From: Emily Shaffer <emilyshaffer@google.com>

In 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via
loose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'.
'--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that
doesn't check whether a lone gc run generates these cruft packs.
'gc.cruftPacks' is never exercised.

Add some tests to exercise these options to gc in the gc test suite.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6500-gc.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index cd6c53360d..928a522194 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -202,6 +202,49 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e
 	grep -E "^trace: (built-in|exec|run_command): git reflog expire --" trace.out
 '
 
+prepare_cruft_history () {
+	test_commit base &&
+
+	test_commit --no-tag foo &&
+	test_commit --no-tag bar &&
+	git reset HEAD^^
+}
+
+assert_cruft_packs () {
+	find .git/objects/pack -name "*.mtimes" >mtimes &&
+	sed -e 's/\.mtimes$/\.pack/g' mtimes >packs &&
+
+	test_file_not_empty packs &&
+	while read pack
+	do
+		test_path_is_file "$pack" || return 1
+	done <packs
+}
+
+test_expect_success 'gc --cruft generates a cruft pack' '
+	test_when_finished "rm -fr crufts" &&
+	git init crufts &&
+	(
+		cd crufts &&
+
+		prepare_cruft_history &&
+		git gc --cruft &&
+		assert_cruft_packs
+	)
+'
+
+test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
+	test_when_finished "rm -fr crufts" &&
+	git init crufts &&
+	(
+		cd crufts &&
+
+		prepare_cruft_history &&
+		git -c gc.cruftPacks=true gc &&
+		assert_cruft_packs
+	)
+'
+
 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
-- 
2.38.0.16.g393fd4c6db

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

[Taylor Blau]

From: Emily Shaffer <emilyshaffer@google.com>

We are interested in exploring whether gc.cruftPacks=true should become
the default value.

To determine whether it is safe to do so, let's encourage more users to
try it out.

Users who have set feature.experimental=true have already volunteered to
try new and possibly-breaking config changes, so let's try this new
default with that set of users.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/feature.txt |  3 ++
 builtin/gc.c                     |  7 +++--
 repo-settings.c                  |  1 +
 repository.h                     |  1 +
 t/t6500-gc.sh                    | 53 ++++++++++++++++++++++++++++++++
 5 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt
index cdecd04e5b..95975e5091 100644
--- a/Documentation/config/feature.txt
+++ b/Documentation/config/feature.txt
@@ -14,6 +14,9 @@ feature.experimental::
 +
 * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by
 skipping more commits at a time, reducing the number of round trips.
++
+* `gc.cruftPacks=true` reduces disk space used by unreachable objects during
+garbage collection, preventing loose object explosions.
 
 feature.manyFiles::
 	Enable config options that optimize for repos with many files in the
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);
 	if (the_repository->settings.gc_write_commit_graph == 1)
 		write_commit_graph_reachable(the_repository->objects->odb,
 					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
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;
 	}
 	if (manyfiles) {
 		r->settings.index_version = 4;
diff --git a/repository.h b/repository.h
index 24316ac944..6c461c5b9d 100644
--- a/repository.h
+++ b/repository.h
@@ -34,6 +34,7 @@ struct repo_settings {
 	int commit_graph_generation_version;
 	int commit_graph_read_changed_paths;
 	int gc_write_commit_graph;
+	int gc_cruft_packs;
 	int fetch_write_commit_graph;
 	int command_requires_full_index;
 	int sparse_index;
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 928a522194..d9acb63951 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -221,6 +221,11 @@ assert_cruft_packs () {
 	done <packs
 }
 
+assert_no_cruft_packs () {
+	find .git/objects/pack -name "*.mtimes" >mtimes &&
+	test_must_be_empty mtimes
+}
+
 test_expect_success 'gc --cruft generates a cruft pack' '
 	test_when_finished "rm -fr crufts" &&
 	git init crufts &&
@@ -245,6 +250,54 @@ 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_packs
+	)
+'
+
+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_packs
+	)
+'
+
+test_expect_success 'feature.experimental=true can be overridden' '
+	git init crufts &&
+	test_when_finished "rm -fr crufts" &&
+	(
+		cd crufts &&
+
+		prepare_cruft_history &&
+		git -c feature.expiremental=true -c gc.cruftPacks=false gc &&
+		assert_no_cruft_packs
+	)
+'
+
+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 &&
+		assert_no_cruft_packs
+	)
+'
+
 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
-- 
2.38.0.16.g393fd4c6db

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

[Derrick Stolee]

On 10/26/22 5:32 PM, Taylor Blau wrote:
> From: Emily Shaffer <emilyshaffer@google.com>
> 
> We are interested in exploring whether gc.cruftPacks=true should become
> the default value.
> 
> To determine whether it is safe to do so, let's encourage more users to
> try it out.
> 
> Users who have set feature.experimental=true have already volunteered to
> try new and possibly-breaking config changes, so let's try this new
> default with that set of users.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  Documentation/config/feature.txt |  3 ++
>  builtin/gc.c                     |  7 +++--
>  repo-settings.c                  |  1 +
>  repository.h                     |  1 +
>  t/t6500-gc.sh                    | 53 ++++++++++++++++++++++++++++++++
>  5 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt
> index cdecd04e5b..95975e5091 100644
> --- a/Documentation/config/feat



> +	prepare_repo_settings(the_repository);
> +	if (cruft_packs < 0)
> +		cruft_packs = the_repository->settings.gc_cruft_packs;
> +

Here, we are checking if cruft_packs hasn't been set by the
command-line arguments _or_ the gc.cruftPacks config option
(as checked in gc_config()). However, we now have another
parameter saying "enable it by the experimental flag".

You are applying things in the correct order here, but I
wanted to point out...

> @@ -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;
>  	}
>  	if (manyfiles) {
>  		r->settings.index_version = 4;

That later in prepare_repo_settings(), we check the exact
config values so that the repo_settings reflects the full
implications from config:

	/* Commit graph config or default, does not cascade (simple) */
	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
	repo_cfg_int(r, "commitgraph.generationversion", &r->settings.commit_graph_generation_version, 2);
	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
	repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);

	/* Boolean config or default, does not cascade (simple)  */
	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);

This allows us to use the_repository->settings.* members
as placeholders for the full implications from config.

So this implementation has some slight differences from
other repo_settings implementations. It's correct, and the
use of gc.cruftPacks is pretty isolated, but I wonder if
we should be sticklers about this pattern in repo_settings.

Thanks,
-Stolee

P.S. I found this draft open on my laptop when I opened it
this morning. Hopefully it's not too late.