threads / patch / 58263

patch, 2 partslet feature.experimental imply gc.cruftPacks=true

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

## tl;dr

9 messages between Aug 3, 2022 and Aug 4, 2022. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Emily Shaffer· Aug 3, 2022, 20:57 UTC · lore

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
Emily Shaffer· Aug 3, 2022, 20:57 UTC · re: Emily Shaffer · lore

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

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(+)
Show changes to t/t6500-gc.sh +36 −0
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
Junio C Hamano· Aug 3, 2022, 21:56 UTC · re: Emily Shaffer · lore

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

Emily Shaffer <emilyshaffer@google.com> writes:
Show 35 quoted lines
> 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"
Ævar Arnfjörð Bjarmason· Aug 4, 2022, 07:48 UTC · re: Emily Shaffer · lore

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

On Wed, Aug 03 2022, Emily Shaffer wrote:
Show 24 quoted lines
> 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.

Show 14 quoted lines
> +	(
> +		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...
Show 18 quoted lines
> +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 ..."?

Junio C Hamano· Aug 4, 2022, 16:09 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 6 quoted lines
>> +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 ;-)

Show 41 quoted lines
>
>> +	(
>> +		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?
Emily Shaffer· Aug 3, 2022, 20:57 UTC · re: Emily Shaffer · lore

[PATCH 2/2] 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.

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(+)
Show changes to 3 files +43 −0

Documentation/config/feature.txt, builtin/gc.c, t/t6500-gc.sh

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
Junio C Hamano· Aug 3, 2022, 22:05 UTC · re: Emily Shaffer · lore

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

Emily Shaffer <emilyshaffer@google.com> writes:
> +* `gc.cruftPacks=true` reduces disk space used by unreachable objects during
> +garbage collection.
OK.
Show 21 quoted lines
> 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...

Show 38 quoted lines
> 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.

Derrick Stolee· Aug 4, 2022, 13:05 UTC · re: Emily Shaffer · lore

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

On 8/3/2022 4:57 PM, Emily Shaffer wrote:
Show 5 quoted lines
> +	/* 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

Junio C Hamano· Aug 4, 2022, 16:10 UTC · re: Derrick Stolee · lore

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

Derrick Stolee <derrickstolee@github.com> writes:
Show 10 quoted lines
> 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.

← back to recent threads