Taylor BlauThis 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.cThanks 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
