| Message ID | 13a25a425bc0901ee38760095eb1bcf314c00da2.1666819953.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c695592850a3b4b7857fc11721cbc7d2b332f41e |
| Headers | show |
| Series | gc: let feature.experimental imply gc.cruftPacks | expand |
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.
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