[PATCH 0/2] let feature.experimental imply gc.cruftPacks=true

[PATCH 0/2] let feature.experimental imply gc.cruftPacks=true

[Emily Shaffer]

Cruft packs seem to save us disk space during garbage collection. There
may still be some interesting races around mtime, but as I understand
it, those are not particularly worse or better with or without cruft
packs; so this setting primarily works to save us disk space. At least
at Google, we see concerns around loose object explosion during gc
fairly often, so this is a welcome change and we've turned it on for all
Googlers. Because we did so, I wonder if it's going to make sense to for
gc.cruftPacks to default to true in the future, with or without mtime
race fixes. With that in mind, I think it's a good idea to get any folks
who may be testing for us with feature.experimental to also try out
cruft packs and complain to us if there is an issue.

In Monday's standup there was some discussion around remaining concerns
with mtime and cruft packs. As I understood it at least, it seems there
can be a loss of information around mtimes when switching between cruft
repacks and no-cruft repacks, for example:

 ab/cdef is unreachable and has mtime <last week>
 'git gc --cruft'
 packs/<cruft>.pack contains abcdef
 packs/<cruft>.mtimes says abcdef has mtime <last week>
 'git gc' (no cruft)
 ab/cdef is unreachable and has mtime <now>

I could have misunderstood it. But it seems to be an issue primarily
around switching between crufty and non-crufty gc.

Read the full conversation around mtime races:
https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-01

As for the series, it's only a two-patch series because I noticed while
trying to add tests for feature.experimental => gc.cruftPacks=true that
there weren't any tests around gc.cruftPacks to begin with. It's
possible that there are too many tests in patch 1 - gc --cruft is tested
in t5329 'expiring cruft objects with git gc' - but it looked like the
content of the test was different, in that t5329's test checks to make
sure the cruft pack and associated metadata are deleted during
expiration, but the one I added checks that the cruft pack is generated
during a 'gc --cruft' which isn't ready to expire yet.

 - Emily

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

 Documentation/config/feature.txt |  2 +
 builtin/gc.c                     |  6 +++
 t/t6500-gc.sh                    | 71 ++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)

-- 
2.37.1.455.g008518b4e5-goog

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

[Emily Shaffer]

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>
---
 t/t6500-gc.sh | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index cd6c53360d..e4c2c3583d 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -202,6 +202,42 @@ 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
 '
 
+test_expect_success 'gc --cruft generates a cruft pack' '
+	git init crufts &&
+	test_when_finished "rm -fr crufts" &&
+	(
+		cd crufts &&
+		test_commit base &&
+
+		test_commit --no-tag foo &&
+		test_commit --no-tag bar &&
+		git reset HEAD^^ &&
+
+		git gc --cruft &&
+
+		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
+		test_path_is_file .git/objects/pack/$cruft.pack
+	)
+'
+
+test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
+	git init crufts &&
+	test_when_finished "rm -fr crufts" &&
+	(
+		cd crufts &&
+		test_commit base &&
+
+		test_commit --no-tag foo &&
+		test_commit --no-tag bar &&
+		git reset HEAD^^ &&
+
+		git -c gc.cruftPacks=true gc &&
+
+		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
+		test_path_is_file .git/objects/pack/$cruft.pack
+	)
+'
+
 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.37.1.455.g008518b4e5-goog

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

[Emily Shaffer]

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>
---
 Documentation/config/feature.txt |  2 ++
 builtin/gc.c                     |  6 ++++++
 t/t6500-gc.sh                    | 35 ++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt
index cdecd04e5b..f029c422be 100644
--- a/Documentation/config/feature.txt
+++ b/Documentation/config/feature.txt
@@ -14,6 +14,8 @@ 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.
 
 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 eeff2b760e..919cc508c5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -136,6 +136,7 @@ static int gc_config_is_timestamp_never(const char *var)
 static void gc_config(void)
 {
 	const char *value;
+	int experimental = 0;
 
 	if (!git_config_get_value("gc.packrefs", &value)) {
 		if (value && !strcmp(value, "notbare"))
@@ -148,6 +149,11 @@ static void gc_config(void)
 	    gc_config_is_timestamp_never("gc.reflogexpireunreachable"))
 		prune_reflogs = 0;
 
+	/* feature.experimental implies gc.cruftPacks=true */
+	git_config_get_bool("feature.experimental", &experimental);
+	if (experimental)
+		cruft_packs = 1;
+
 	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);
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index e4c2c3583d..4ab6750111 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -238,6 +238,41 @@ 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 &&
+		test_commit base &&
+
+		test_commit --no-tag foo &&
+		test_commit --no-tag bar &&
+		git reset HEAD^^ &&
+
+		git -c feature.experimental=true gc &&
+
+		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
+		test_path_is_file .git/objects/pack/$cruft.pack
+	)
+'
+
+test_expect_success 'feature.experimental=false allows explicit cruft packs' '
+	git init crufts &&
+	test_when_finished "rm -fr crufts" &&
+	(
+		cd crufts &&
+		test_commit base &&
+
+		test_commit --no-tag foo &&
+		test_commit --no-tag bar &&
+		git reset HEAD^^ &&
+
+		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
+	)
+'
+
 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.37.1.455.g008518b4e5-goog

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

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> 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>
> ---
>  t/t6500-gc.sh | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index cd6c53360d..e4c2c3583d 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -202,6 +202,42 @@ 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
>  '
>  
> +test_expect_success 'gc --cruft generates a cruft pack' '
> +	git init crufts &&
> +	test_when_finished "rm -fr crufts" &&
> +	(
> +		cd crufts &&
> +		test_commit base &&
> +
> +		test_commit --no-tag foo &&
> +		test_commit --no-tag bar &&
> +		git reset HEAD^^ &&
> +
> +		git gc --cruft &&
> +
> +		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&

What guarantees that we will have one pack-*.mtimes?  

I do not mind if we reliably diagnosed it as an error when "git gc
--cruft" created two cruft packs, but I do mind if this call to
basename receives two files plus .mtimes suffix and misbehaves.

Is the fact that it is accompanied by a .mtimes file the only clue
that a pack is a "cruft" pack?  Given that the usefulness of mtimes
based expiration approach is doubted, do we want to rely on it (and
having to redesign the test)?

I think the right test would be to

 * make a list of all "in use" objects;

 * see if there is one (or more) packfile that does not contain any
   "in use" objects (look at their .idx file).

If all packfiles are packs with objects that are still in use, then
we did not create a cruft pack.

> +		test_path_is_file .git/objects/pack/$cruft.pack

DQuote the whole thing, i.e.

		test_path_is_file ".git/objects/pack/$cruft.pack"

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

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> +* `gc.cruftPacks=true` reduces disk space used by unreachable objects during
> +garbage collection.

OK.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index eeff2b760e..919cc508c5 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -136,6 +136,7 @@ static int gc_config_is_timestamp_never(const char *var)
>  static void gc_config(void)
>  {
>  	const char *value;
> +	int experimental = 0;
>  
>  	if (!git_config_get_value("gc.packrefs", &value)) {
>  		if (value && !strcmp(value, "notbare"))
> @@ -148,6 +149,11 @@ static void gc_config(void)
>  	    gc_config_is_timestamp_never("gc.reflogexpireunreachable"))
>  		prune_reflogs = 0;
>  
> +	/* feature.experimental implies gc.cruftPacks=true */
> +	git_config_get_bool("feature.experimental", &experimental);
> +	if (experimental)
> +		cruft_packs = 1;
> +

I suspect the whole thing can just be:

	git_config_get_bool("feature.experimental", &cruft_packs);

If there is no feature.experimental configuration, the call returns
non-zero (we do not check, though) without touching &cruft_packs, if
there is feature.experimental configuration, the call returns zero
(we do not check, though) and cruft_packs is set to either true
(when experimental) or false (otherwise).

And this whole thing happens before we inspect what the more
specific configuration gc.cruftPacks says, so...

> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index e4c2c3583d..4ab6750111 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -238,6 +238,41 @@ 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 &&
> +		test_commit base &&
> +
> +		test_commit --no-tag foo &&
> +		test_commit --no-tag bar &&
> +		git reset HEAD^^ &&
> +
> +		git -c feature.experimental=true gc &&
> +
> +		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
> +		test_path_is_file .git/objects/pack/$cruft.pack
> +	)
> +'
> +
> +test_expect_success 'feature.experimental=false allows explicit cruft packs' '
> +	git init crufts &&
> +	test_when_finished "rm -fr crufts" &&
> +	(
> +		cd crufts &&
> +		test_commit base &&
> +
> +		test_commit --no-tag foo &&
> +		test_commit --no-tag bar &&
> +		git reset HEAD^^ &&
> +
> +		git -c gc.cruftPacks=true -c feature.experimental=false gc &&

OK.  

What is not tested is setting feature.experimental explicitly to
false without touching gc.cruftPacks does not use the cruft pack.

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

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 03 2022, Emily Shaffer wrote:

> 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>
> ---
>  t/t6500-gc.sh | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index cd6c53360d..e4c2c3583d 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -202,6 +202,42 @@ 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
>  '
>  
> +test_expect_success 'gc --cruft generates a cruft pack' '
> +	git init crufts &&
> +	test_when_finished "rm -fr crufts" &&

Let's "test_when_finished" first, then "git init", the point is to clean
up the directory if we fail.

> +	(
> +		cd crufts &&
> +		test_commit base &&
> +
> +		test_commit --no-tag foo &&
> +		test_commit --no-tag bar &&
> +		git reset HEAD^^ &&
> +
> +		git gc --cruft &&
> +
> +		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
> +		test_path_is_file .git/objects/pack/$cruft.pack
> +	)
> +'

...this...

> +test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
> +	git init crufts &&
> +	test_when_finished "rm -fr crufts" &&
> +	(
> +		cd crufts &&
> +		test_commit base &&
> +
> +		test_commit --no-tag foo &&
> +		test_commit --no-tag bar &&
> +		git reset HEAD^^ &&
> +
> +		git -c gc.cruftPacks=true gc &&
> +
> +		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
> +		test_path_is_file .git/objects/pack/$cruft.pack
> +	)
> +'
> +

...whole thing seems to be copy/pasted aside from the git options.

If so let's factor this into a trivial helper that invokes git "$@",
then call it with "gc --cruft" and "-c ..."?

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

[Derrick Stolee]

On 8/3/2022 4:57 PM, Emily Shaffer wrote:

> +	/* feature.experimental implies gc.cruftPacks=true */
> +	git_config_get_bool("feature.experimental", &experimental);
> +	if (experimental)
> +		cruft_packs = 1;
> +

This should be grouped into prepare_repo_settings() in repo-settings.c
so we have a single place to see what is updated by feature.experimental.

Thanks,
-Stolee

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> +test_expect_success 'gc --cruft generates a cruft pack' '
>> +	git init crufts &&
>> +	test_when_finished "rm -fr crufts" &&
>
> Let's "test_when_finished" first, then "git init", the point is to clean
> up the directory if we fail.

Good advice.

We say "rm -fr" not "rm -r" there because we do not want to see a
failure to remove if "git init" failed before it manages to create
the directory ;-)

>
>> +	(
>> +		cd crufts &&
>> +		test_commit base &&
>> +
>> +		test_commit --no-tag foo &&
>> +		test_commit --no-tag bar &&
>> +		git reset HEAD^^ &&
>> +
>> +		git gc --cruft &&
>> +
>> +		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
>> +		test_path_is_file .git/objects/pack/$cruft.pack
>> +	)
>> +'
>
> ...this...
>
>> +test_expect_success 'gc.cruftPacks=true generates a cruft pack' '
>> +	git init crufts &&
>> +	test_when_finished "rm -fr crufts" &&
>> +	(
>> +		cd crufts &&
>> +		test_commit base &&
>> +
>> +		test_commit --no-tag foo &&
>> +		test_commit --no-tag bar &&
>> +		git reset HEAD^^ &&
>> +
>> +		git -c gc.cruftPacks=true gc &&
>> +
>> +		cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&
>> +		test_path_is_file .git/objects/pack/$cruft.pack
>> +	)
>> +'
>> +
>
> ...whole thing seems to be copy/pasted aside from the git options.
>
> If so let's factor this into a trivial helper that invokes git "$@",
> then call it with "gc --cruft" and "-c ..."?

With shell, passing "here is a series of commands to be run in the
middle of a boilerplate sequence" is indeed easy to write, but it
gets harder to follow and quote correctly, which is why I'd rather
not see that pattern overused.

A pair of helper functions, one of which prepares a sample history
to be used, and the other checks if we created one (or more) cruft
packs, may achieve the same conciseness while remaining to be more
readable.  I.e.

    test_when_finished "rm -fr crufts" &&
    git init crufts &&
    (
	cd crufts &&
	prepare_history &&

	git -c gc.cruftPacks=true gc &&

	cruft_packs_exist
    )

perhaps?

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

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 8/3/2022 4:57 PM, Emily Shaffer wrote:
>
>> +	/* feature.experimental implies gc.cruftPacks=true */
>> +	git_config_get_bool("feature.experimental", &experimental);
>> +	if (experimental)
>> +		cruft_packs = 1;
>> +
>
> This should be grouped into prepare_repo_settings() in repo-settings.c
> so we have a single place to see what is updated by feature.experimental.

Excellent.  I forgot about that.  Thanks.