{"thread":{"id":"58698","subject":"[PATCH v2 0/2] gc: let feature.experimental imply gc.cruftPacks","startedAt":"2022-10-26T20:13:41Z","lastAt":"2022-10-31T10:46:35Z","messageCount":11,"participants":["Taylor Blau","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"465824","messageId":"cover.1666815209.git.me@ttaylorr.com","threadId":"58698","inReplyTo":null,"subject":"[PATCH v2 0/2] gc: let feature.experimental imply gc.cruftPacks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-26T20:13:34Z","receivedAt":"2022-10-26T20:13:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This is a moderately-sized reroll of Emily's series from [1], which I\nsaw while sifting through old mail in my inbox.\n\nThat series was sent when I was on vacation, which is a likely\nexplanation for why I most likely missed it.\n\nI'm sending a reroll out on Emily's behalf, since the series hasn't\nreceived any activity in a few months, and I would like to see the\ntopic pushed forward.\n\nChanges since last time are included in a range-diff below, but the\nmajor points are:\n\n  - extracted common components of the test script into helper\n    functions\n  - reordered the repository creation/teardown so that\n    test_when_finished \"rm -fr ...\" happens before \"git init\"\n  - and centralized checking feature.experimental into\n    repo-settings.c\n\nThanks in advance for your review.\n\n[1]: https://lore.kernel.org/git/20220803205721.3686361-1-emilyshaffer@google.com/\n\nEmily Shaffer (2):\n  gc: add tests for --cruft and friends\n  config: let feature.experimental imply gc.cruftPacks=true\n\n Documentation/config/feature.txt |  3 ++\n builtin/gc.c                     |  7 ++-\n repo-settings.c                  |  1 +\n repository.h                     |  1 +\n t/t6500-gc.sh                    | 84 ++++++++++++++++++++++++++++++++\n 5 files changed, 94 insertions(+), 2 deletions(-)\n\nRange-diff against v1:\n1:  bf243d15c1 ! 1:  35d2c97715 gc: add tests for --cruft and friends\n    @@ Commit message\n         Add some tests to exercise these options to gc in the gc test suite.\n     \n         Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n    +    Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n      ## t/t6500-gc.sh ##\n     @@ t/t6500-gc.sh: test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"e\n      \tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n      '\n      \n    ++prepare_cruft_history () {\n    ++\ttest_commit base &&\n    ++\n    ++\ttest_commit --no-tag foo &&\n    ++\ttest_commit --no-tag bar &&\n    ++\tgit reset HEAD^^\n    ++}\n    ++\n    ++assert_cruft_pack_exists () {\n    ++\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n    ++\tsed -e 's/\\.mtimes$/\\.pack/g' mtimes >packs &&\n    ++\n    ++\ttest_file_not_empty packs &&\n    ++\twhile read pack\n    ++\tdo\n    ++\t\ttest_path_is_file \"$pack\" || return 1\n    ++\tdone <packs\n    ++}\n    ++\n     +test_expect_success 'gc --cruft generates a cruft pack' '\n    -+\tgit init crufts &&\n     +\ttest_when_finished \"rm -fr crufts\" &&\n    ++\tgit init crufts &&\n     +\t(\n     +\t\tcd crufts &&\n    -+\t\ttest_commit base &&\n    -+\n    -+\t\ttest_commit --no-tag foo &&\n    -+\t\ttest_commit --no-tag bar &&\n    -+\t\tgit reset HEAD^^ &&\n     +\n    ++\t\tprepare_cruft_history &&\n     +\t\tgit gc --cruft &&\n    -+\n    -+\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n    -+\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n    ++\t\tassert_cruft_pack_exists\n     +\t)\n     +'\n     +\n     +test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n    -+\tgit init crufts &&\n     +\ttest_when_finished \"rm -fr crufts\" &&\n    ++\tgit init crufts &&\n     +\t(\n     +\t\tcd crufts &&\n    -+\t\ttest_commit base &&\n    -+\n    -+\t\ttest_commit --no-tag foo &&\n    -+\t\ttest_commit --no-tag bar &&\n    -+\t\tgit reset HEAD^^ &&\n     +\n    ++\t\tprepare_cruft_history &&\n     +\t\tgit -c gc.cruftPacks=true gc &&\n    -+\n    -+\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n    -+\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n    ++\t\tassert_cruft_pack_exists\n     +\t)\n     +'\n     +\n2:  5a242e2370 ! 2:  eb151752b8 config: let feature.experimental imply gc.cruftPacks=true\n    @@ Commit message\n         config: let feature.experimental imply gc.cruftPacks=true\n     \n         We are interested in exploring whether gc.cruftPacks=true should become\n    -    the default value; to determine whether it is safe to do so, let's\n    -    encourage more users to try it out. Users who have set\n    -    feature.experimental=true have already volunteered to try new and\n    -    possibly-breaking config changes, so let's try this new default with\n    -    that set of users.\n    +    the default value.\n    +\n    +    To determine whether it is safe to do so, let's encourage more users to\n    +    try it out.\n    +\n    +    Users who have set feature.experimental=true have already volunteered to\n    +    try new and possibly-breaking config changes, so let's try this new\n    +    default with that set of users.\n     \n         Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n    +    Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n      ## Documentation/config/feature.txt ##\n     @@ Documentation/config/feature.txt: feature.experimental::\n      +\n      * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by\n      skipping more commits at a time, reducing the number of round trips.\n    +++\n     +* `gc.cruftPacks=true` reduces disk space used by unreachable objects during\n    -+garbage collection.\n    ++garbage collection, preventing loose object explosions.\n      \n      feature.manyFiles::\n      \tEnable config options that optimize for repos with many files in the\n     \n      ## builtin/gc.c ##\n    -@@ builtin/gc.c: static int gc_config_is_timestamp_never(const char *var)\n    - static void gc_config(void)\n    - {\n    - \tconst char *value;\n    -+\tint experimental = 0;\n    +@@ builtin/gc.c: static const char * const builtin_gc_usage[] = {\n      \n    - \tif (!git_config_get_value(\"gc.packrefs\", &value)) {\n    - \t\tif (value && !strcmp(value, \"notbare\"))\n    -@@ builtin/gc.c: static void gc_config(void)\n    - \t    gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n    - \t\tprune_reflogs = 0;\n    + static int pack_refs = 1;\n    + static int prune_reflogs = 1;\n    +-static int cruft_packs = 0;\n    ++static int cruft_packs = -1;\n    + static int aggressive_depth = 50;\n    + static int aggressive_window = 250;\n    + static int gc_auto_threshold = 6700;\n    +@@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)\n    + \tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n    + \t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n      \n    -+\t/* feature.experimental implies gc.cruftPacks=true */\n    -+\tgit_config_get_bool(\"feature.experimental\", &experimental);\n    -+\tif (experimental)\n    -+\t\tcruft_packs = 1;\n    ++\tprepare_repo_settings(the_repository);\n    ++\tif (cruft_packs < 0)\n    ++\t\tcruft_packs = the_repository->settings.gc_cruft_packs;\n     +\n    - \tgit_config_get_int(\"gc.aggressivewindow\", &aggressive_window);\n    - \tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n    - \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\n    + \tif (aggressive) {\n    + \t\tstrvec_push(&repack, \"-f\");\n    + \t\tif (aggressive_depth > 0)\n    +@@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)\n    + \t\tclean_pack_garbage();\n    + \t}\n    + \n    +-\tprepare_repo_settings(the_repository);\n    + \tif (the_repository->settings.gc_write_commit_graph == 1)\n    + \t\twrite_commit_graph_reachable(the_repository->objects->odb,\n    + \t\t\t\t\t     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\n    +\n    + ## repo-settings.c ##\n    +@@ repo-settings.c: void prepare_repo_settings(struct repository *r)\n    + \t/* Defaults modified by feature.* */\n    + \tif (experimental) {\n    + \t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n    ++\t\tr->settings.gc_cruft_packs = 1;\n    + \t}\n    + \tif (manyfiles) {\n    + \t\tr->settings.index_version = 4;\n    +\n    + ## repository.h ##\n    +@@ repository.h: struct repo_settings {\n    + \tint commit_graph_generation_version;\n    + \tint commit_graph_read_changed_paths;\n    + \tint gc_write_commit_graph;\n    ++\tint gc_cruft_packs;\n    + \tint fetch_write_commit_graph;\n    + \tint command_requires_full_index;\n    + \tint sparse_index;\n     \n      ## t/t6500-gc.sh ##\n    +@@ t/t6500-gc.sh: assert_cruft_pack_exists () {\n    + \tdone <packs\n    + }\n    + \n    ++refute_cruft_packs_exist () {\n    ++\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n    ++\ttest_must_be_empty mtimes\n    ++}\n    ++\n    + test_expect_success 'gc --cruft generates a cruft pack' '\n    + \ttest_when_finished \"rm -fr crufts\" &&\n    + \tgit init crufts &&\n     @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n      \t)\n      '\n    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n     +\ttest_when_finished \"rm -fr crufts\" &&\n     +\t(\n     +\t\tcd crufts &&\n    -+\t\ttest_commit base &&\n    -+\n    -+\t\ttest_commit --no-tag foo &&\n    -+\t\ttest_commit --no-tag bar &&\n    -+\t\tgit reset HEAD^^ &&\n     +\n    ++\t\tprepare_cruft_history &&\n     +\t\tgit -c feature.experimental=true gc &&\n    -+\n    -+\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n    -+\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n    ++\t\tassert_cruft_pack_exists\n     +\t)\n     +'\n     +\n    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n     +\ttest_when_finished \"rm -fr crufts\" &&\n     +\t(\n     +\t\tcd crufts &&\n    -+\t\ttest_commit base &&\n    -+\n    -+\t\ttest_commit --no-tag foo &&\n    -+\t\ttest_commit --no-tag bar &&\n    -+\t\tgit reset HEAD^^ &&\n     +\n    ++\t\tprepare_cruft_history &&\n     +\t\tgit -c gc.cruftPacks=true -c feature.experimental=false gc &&\n    -+\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n    -+\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n    ++\t\tassert_cruft_pack_exists\n    ++\t)\n    ++'\n    ++\n    ++test_expect_success 'feature.experimental=false avoids cruft packs by default' '\n    ++\tgit init crufts &&\n    ++\ttest_when_finished \"rm -fr crufts\" &&\n    ++\t(\n    ++\t\tcd crufts &&\n    ++\n    ++\t\tprepare_cruft_history &&\n    ++\t\tgit -c feature.experimental=false gc &&\n    ++\t\trefute_cruft_packs_exist\n     +\t)\n     +'\n     +\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465825","messageId":"35d2c97715008db3957a69a8260e2046c0e5e738.1666815209.git.me@ttaylorr.com","threadId":"58698","inReplyTo":"cover.1666815209.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] gc: add tests for --cruft and friends","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-26T20:13:38Z","receivedAt":"2022-10-26T20:13:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Emily Shaffer <emilyshaffer@google.com>\n\nIn 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via\nloose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'.\n'--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that\ndoesn't check whether a lone gc run generates these cruft packs.\n'gc.cruftPacks' is never exercised.\n\nAdd some tests to exercise these options to gc in the gc test suite.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t6500-gc.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 43 insertions(+)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex cd6c53360d..9110a39088 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -202,6 +202,49 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"e\n \tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n '\n \n+prepare_cruft_history () {\n+\ttest_commit base &&\n+\n+\ttest_commit --no-tag foo &&\n+\ttest_commit --no-tag bar &&\n+\tgit reset HEAD^^\n+}\n+\n+assert_cruft_pack_exists () {\n+\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n+\tsed -e 's/\\.mtimes$/\\.pack/g' mtimes >packs &&\n+\n+\ttest_file_not_empty packs &&\n+\twhile read pack\n+\tdo\n+\t\ttest_path_is_file \"$pack\" || return 1\n+\tdone <packs\n+}\n+\n+test_expect_success 'gc --cruft generates a cruft pack' '\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\tgit init crufts &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit gc --cruft &&\n+\t\tassert_cruft_pack_exists\n+\t)\n+'\n+\n+test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\tgit init crufts &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit -c gc.cruftPacks=true gc &&\n+\t\tassert_cruft_pack_exists\n+\t)\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9.  Furthermore, the\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465826","messageId":"eb151752b8de355ac334507e57dc95aadc9ef2bf.1666815209.git.me@ttaylorr.com","threadId":"58698","inReplyTo":"cover.1666815209.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] config: let feature.experimental imply gc.cruftPacks=true","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-26T20:13:41Z","receivedAt":"2022-10-26T20:13:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Emily Shaffer <emilyshaffer@google.com>\n\nWe are interested in exploring whether gc.cruftPacks=true should become\nthe default value.\n\nTo determine whether it is safe to do so, let's encourage more users to\ntry it out.\n\nUsers who have set feature.experimental=true have already volunteered to\ntry new and possibly-breaking config changes, so let's try this new\ndefault with that set of users.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/feature.txt |  3 +++\n builtin/gc.c                     |  7 ++++--\n repo-settings.c                  |  1 +\n repository.h                     |  1 +\n t/t6500-gc.sh                    | 41 ++++++++++++++++++++++++++++++++\n 5 files changed, 51 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex cdecd04e5b..95975e5091 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -14,6 +14,9 @@ feature.experimental::\n +\n * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by\n skipping more commits at a time, reducing the number of round trips.\n++\n+* `gc.cruftPacks=true` reduces disk space used by unreachable objects during\n+garbage collection, preventing loose object explosions.\n \n feature.manyFiles::\n \tEnable config options that optimize for repos with many files in the\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 243ee85d28..5a84f791ef 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -42,7 +42,7 @@ static const char * const builtin_gc_usage[] = {\n \n static int pack_refs = 1;\n static int prune_reflogs = 1;\n-static int cruft_packs = 0;\n+static int cruft_packs = -1;\n static int aggressive_depth = 50;\n static int aggressive_window = 250;\n static int gc_auto_threshold = 6700;\n@@ -593,6 +593,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n \t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n \n+\tprepare_repo_settings(the_repository);\n+\tif (cruft_packs < 0)\n+\t\tcruft_packs = the_repository->settings.gc_cruft_packs;\n+\n \tif (aggressive) {\n \t\tstrvec_push(&repack, \"-f\");\n \t\tif (aggressive_depth > 0)\n@@ -704,7 +708,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tclean_pack_garbage();\n \t}\n \n-\tprepare_repo_settings(the_repository);\n \tif (the_repository->settings.gc_write_commit_graph == 1)\n \t\twrite_commit_graph_reachable(the_repository->objects->odb,\n \t\t\t\t\t     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\ndiff --git a/repo-settings.c b/repo-settings.c\nindex e8b58151bc..3021921c53 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -43,6 +43,7 @@ void prepare_repo_settings(struct repository *r)\n \t/* Defaults modified by feature.* */\n \tif (experimental) {\n \t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n+\t\tr->settings.gc_cruft_packs = 1;\n \t}\n \tif (manyfiles) {\n \t\tr->settings.index_version = 4;\ndiff --git a/repository.h b/repository.h\nindex 24316ac944..6c461c5b9d 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -34,6 +34,7 @@ struct repo_settings {\n \tint commit_graph_generation_version;\n \tint commit_graph_read_changed_paths;\n \tint gc_write_commit_graph;\n+\tint gc_cruft_packs;\n \tint fetch_write_commit_graph;\n \tint command_requires_full_index;\n \tint sparse_index;\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 9110a39088..628dfeb737 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -221,6 +221,11 @@ assert_cruft_pack_exists () {\n \tdone <packs\n }\n \n+refute_cruft_packs_exist () {\n+\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n+\ttest_must_be_empty mtimes\n+}\n+\n test_expect_success 'gc --cruft generates a cruft pack' '\n \ttest_when_finished \"rm -fr crufts\" &&\n \tgit init crufts &&\n@@ -245,6 +250,42 @@ test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n \t)\n '\n \n+test_expect_success 'feature.experimental=true generates a cruft pack' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit -c feature.experimental=true gc &&\n+\t\tassert_cruft_pack_exists\n+\t)\n+'\n+\n+test_expect_success 'feature.experimental=false allows explicit cruft packs' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit -c gc.cruftPacks=true -c feature.experimental=false gc &&\n+\t\tassert_cruft_pack_exists\n+\t)\n+'\n+\n+test_expect_success 'feature.experimental=false avoids cruft packs by default' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit -c feature.experimental=false gc &&\n+\t\trefute_cruft_packs_exist\n+\t)\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9.  Furthermore, the\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465831","messageId":"xmqqwn8mfgsj.fsf@gitster.g","threadId":"58698","inReplyTo":"cover.1666815209.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/2] gc: let feature.experimental imply gc.cruftPacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-26T20:53:16Z","receivedAt":"2022-10-26T20:53:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> This is a moderately-sized reroll of Emily's series from [1], which I\n> saw while sifting through old mail in my inbox.\n>\n> That series was sent when I was on vacation, which is a likely\n> explanation for why I most likely missed it.\n>\n> I'm sending a reroll out on Emily's behalf, since the series hasn't\n> received any activity in a few months, and I would like to see the\n> topic pushed forward.\n\nYeah, it has been quite a while.  I'd prefer to at least see an Ack\nby the original author before proceeding.\n\nIn my quick scan of the original round\nhttps://lore.kernel.org/git/20220803205721.3686361-1-emilyshaffer@google.com/\n\nthe new round appears to address all the points raised.  It was so\nlong time ago that I am not sure if reviewers stopped after seeing\nminor points that need to be addressed, or there were only minor\nissues in the original series, though.\n\nThanks.\n"},{"id":"465832","messageId":"xmqqo7tyfgbx.fsf@gitster.g","threadId":"58698","inReplyTo":"35d2c97715008db3957a69a8260e2046c0e5e738.1666815209.git.me@ttaylorr.com","subject":"Re: [PATCH v2 1/2] gc: add tests for --cruft and friends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-26T21:03:14Z","receivedAt":"2022-10-26T21:03:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> +prepare_cruft_history () {\n> +\ttest_commit base &&\n> +\n> +\ttest_commit --no-tag foo &&\n> +\ttest_commit --no-tag bar &&\n> +\tgit reset HEAD^^\n> +}\n\nOK.  Three pearls on a single strand, then the tip-two gets rewound\naway.\n\n> +assert_cruft_pack_exists () {\n> +\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n> +\tsed -e 's/\\.mtimes$/\\.pack/g' mtimes >packs &&\n\nSomebody recently made comment that we already depend on the fact\nthat the default action for \"find\" is \"-print\".  I do not see the\nneed for the 'mtimes' intermediary file.  It does not hurt, but it\nis not needed.\n\n> +\ttest_file_not_empty packs &&\n> +\twhile read pack\n> +\tdo\n> +\t\ttest_path_is_file \"$pack\" || return 1\n> +\tdone <packs\n> +}\n\nOK.  We enumerate .mtimes files and ensure corresponding .pack\nexists for each of them.  That might miss a case where we thought we\ncreated a cruft pack (i.e. .pack exists) but somehow failed to\ncreate the matching .mtimes file by mistake, but it is hard to tell\nsuch a \"broken cruft pack\" file from a normal pack file, so I think\nthe above is the best we can do.\n\n> +test_expect_success 'gc --cruft generates a cruft pack' '\n> +\ttest_when_finished \"rm -fr crufts\" &&\n> +\tgit init crufts &&\n> +\t(\n> +\t\tcd crufts &&\n> +\n> +\t\tprepare_cruft_history &&\n> +\t\tgit gc --cruft &&\n> +\t\tassert_cruft_pack_exists\n> +\t)\n> +'\n> +\n> +test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n> +\ttest_when_finished \"rm -fr crufts\" &&\n> +\tgit init crufts &&\n> +\t(\n> +\t\tcd crufts &&\n> +\n> +\t\tprepare_cruft_history &&\n> +\t\tgit -c gc.cruftPacks=true gc &&\n> +\t\tassert_cruft_pack_exists\n> +\t)\n> +'\n\nWe could instead do away without when-finished clean-up and use\nseparate test subdirectory.  Consistently doing so may make CI\nforensic easier.  I do not think it is necessary in the test history\nused here that is too simple to be realisic.  If something breaks at\nCI, it is simple enough to reproduce it manually, so I do not think\nit is worth a reroll for that.\n\n>  run_and_wait_for_auto_gc () {\n>  \t# We read stdout from gc for the side effect of waiting until the\n>  \t# background gc process exits, closing its fd 9.  Furthermore, the\n"},{"id":"465833","messageId":"xmqqfsfaffs1.fsf@gitster.g","threadId":"58698","inReplyTo":"eb151752b8de355ac334507e57dc95aadc9ef2bf.1666815209.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/2] config: let feature.experimental imply gc.cruftPacks=true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-26T21:15:10Z","receivedAt":"2022-10-26T21:15:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 243ee85d28..5a84f791ef 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -42,7 +42,7 @@ static const char * const builtin_gc_usage[] = {\n>  \n>  static int pack_refs = 1;\n>  static int prune_reflogs = 1;\n> -static int cruft_packs = 0;\n> +static int cruft_packs = -1;\n>  static int aggressive_depth = 50;\n>  static int aggressive_window = 250;\n>  static int gc_auto_threshold = 6700;\n> @@ -593,6 +593,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n>  \t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n>  \n> +\tprepare_repo_settings(the_repository);\n> +\tif (cruft_packs < 0)\n> +\t\tcruft_packs = the_repository->settings.gc_cruft_packs;\n> +\n>  \tif (aggressive) {\n>  \t\tstrvec_push(&repack, \"-f\");\n>  \t\tif (aggressive_depth > 0)\n> @@ -704,7 +708,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \t\tclean_pack_garbage();\n>  \t}\n>  \n> -\tprepare_repo_settings(the_repository);\n\nIt is curious why we had this call so late in the sequence in the\noriginal.  This is well past what can be reasonably called \"start-up\".\nWe have locked the repository, we may have daemonized ourselves, we\nmay already have packed loose refs and pruned reflogs.  Was that due\nto somewhat lazy thinking that the next line is the first one that\nrequires the repository's settings already prepared, I wonder.\n\nI do not offhand see anything after the location of the new call\nabove and before this location that may negatively get affected by\nmaking the call to prepare_repo_settings() too early, so this change\nshould be safe, I guess.\n\n> diff --git a/repo-settings.c b/repo-settings.c\n> index e8b58151bc..3021921c53 100644\n> --- a/repo-settings.c\n> +++ b/repo-settings.c\n> @@ -43,6 +43,7 @@ void prepare_repo_settings(struct repository *r)\n>  \t/* Defaults modified by feature.* */\n>  \tif (experimental) {\n>  \t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n> +\t\tr->settings.gc_cruft_packs = 1;\n>  \t}\n\nOK.\n\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 9110a39088..628dfeb737 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -221,6 +221,11 @@ assert_cruft_pack_exists () {\n>  \tdone <packs\n>  }\n>  \n> +refute_cruft_packs_exist () {\n> +\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n> +\ttest_must_be_empty mtimes\n> +}\n\nHmph, not \"assert_no_cruft_packs\"?\n\n>  test_expect_success 'gc --cruft generates a cruft pack' '\n>  \ttest_when_finished \"rm -fr crufts\" &&\n>  \tgit init crufts &&\n> @@ -245,6 +250,42 @@ test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'feature.experimental=true generates a cruft pack' '\n> +\tgit init crufts &&\n> +\ttest_when_finished \"rm -fr crufts\" &&\n> +\t(\n> +\t\tcd crufts &&\n> +\n> +\t\tprepare_cruft_history &&\n> +\t\tgit -c feature.experimental=true gc &&\n> +\t\tassert_cruft_pack_exists\n> +\t)\n> +'\n\nOK.\n\n> +test_expect_success 'feature.experimental=false allows explicit cruft packs' '\n> +\tgit init crufts &&\n> +\ttest_when_finished \"rm -fr crufts\" &&\n> +\t(\n> +\t\tcd crufts &&\n> +\n> +\t\tprepare_cruft_history &&\n> +\t\tgit -c gc.cruftPacks=true -c feature.experimental=false gc &&\n> +\t\tassert_cruft_pack_exists\n> +\t)\n> +'\n\nOK.\n\n> +test_expect_success 'feature.experimental=false avoids cruft packs by default' '\n> +\tgit init crufts &&\n> +\ttest_when_finished \"rm -fr crufts\" &&\n> +\t(\n> +\t\tcd crufts &&\n> +\n> +\t\tprepare_cruft_history &&\n> +\t\tgit -c feature.experimental=false gc &&\n> +\t\trefute_cruft_packs_exist\n> +\t)\n> +'\n\nOK.\n\nOne thing missing is a test for the escape hatch for those who opt\ninto the experimental world when gc.cruftPacks feature is broken.\nIOW,\n\n\tgit -c feature.experimental=true -c gc.cruftPacks=false\n\nshould allow them to opt out of gc.cruftPacks.\n\nOther than that, looking good.\n\nThanks.\n\n>  run_and_wait_for_auto_gc () {\n>  \t# We read stdout from gc for the side effect of waiting until the\n>  \t# background gc process exits, closing its fd 9.  Furthermore, the\n"},{"id":"465838","messageId":"cover.1666819953.git.me@ttaylorr.com","threadId":"58698","inReplyTo":"cover.1666815209.git.me@ttaylorr.com","subject":"[PATCH v3 0/2] gc: let feature.experimental imply gc.cruftPacks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-26T21:32:34Z","receivedAt":"2022-10-26T21:32:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This is a small reroll of an adopted version of Emily's series[1], which\nI saw while sifting through old mail in my inbox.\n\nThis is substantively unchanged from the previous round, save:\n\n  - renaming the helper functions which assert and refute the existence\n    of cruft packs, and\n\n  - an additional test which ensures that we can opt out of cruft packs\n    via config\n\nA range-diff is included below for convenience.\n\nThanks in advance for your review.\n\n[1]: https://lore.kernel.org/git/20220803205721.3686361-1-emilyshaffer@google.com/\n\nEmily Shaffer (2):\n  gc: add tests for --cruft and friends\n  config: let feature.experimental imply gc.cruftPacks=true\n\n Documentation/config/feature.txt |  3 +\n builtin/gc.c                     |  7 ++-\n repo-settings.c                  |  1 +\n repository.h                     |  1 +\n t/t6500-gc.sh                    | 96 ++++++++++++++++++++++++++++++++\n 5 files changed, 106 insertions(+), 2 deletions(-)\n\nRange-diff against v2:\n1:  35d2c97715 ! 1:  e8726f3de7 gc: add tests for --cruft and friends\n    @@ t/t6500-gc.sh: test_expect_success 'one of gc.reflogExpire{Unreachable,}=never d\n     +\tgit reset HEAD^^\n     +}\n     +\n    -+assert_cruft_pack_exists () {\n    ++assert_cruft_packs () {\n     +\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n     +\tsed -e 's/\\.mtimes$/\\.pack/g' mtimes >packs &&\n     +\n    @@ t/t6500-gc.sh: test_expect_success 'one of gc.reflogExpire{Unreachable,}=never d\n     +\n     +\t\tprepare_cruft_history &&\n     +\t\tgit gc --cruft &&\n    -+\t\tassert_cruft_pack_exists\n    ++\t\tassert_cruft_packs\n     +\t)\n     +'\n     +\n    @@ t/t6500-gc.sh: test_expect_success 'one of gc.reflogExpire{Unreachable,}=never d\n     +\n     +\t\tprepare_cruft_history &&\n     +\t\tgit -c gc.cruftPacks=true gc &&\n    -+\t\tassert_cruft_pack_exists\n    ++\t\tassert_cruft_packs\n     +\t)\n     +'\n     +\n2:  eb151752b8 ! 2:  13a25a425b config: let feature.experimental imply gc.cruftPacks=true\n    @@ repository.h: struct repo_settings {\n      \tint sparse_index;\n     \n      ## t/t6500-gc.sh ##\n    -@@ t/t6500-gc.sh: assert_cruft_pack_exists () {\n    +@@ t/t6500-gc.sh: assert_cruft_packs () {\n      \tdone <packs\n      }\n      \n    -+refute_cruft_packs_exist () {\n    ++assert_no_cruft_packs () {\n     +\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n     +\ttest_must_be_empty mtimes\n     +}\n    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n     +\n     +\t\tprepare_cruft_history &&\n     +\t\tgit -c feature.experimental=true gc &&\n    -+\t\tassert_cruft_pack_exists\n    ++\t\tassert_cruft_packs\n     +\t)\n     +'\n     +\n    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n     +\n     +\t\tprepare_cruft_history &&\n     +\t\tgit -c gc.cruftPacks=true -c feature.experimental=false gc &&\n    -+\t\tassert_cruft_pack_exists\n    ++\t\tassert_cruft_packs\n    ++\t)\n    ++'\n    ++\n    ++test_expect_success 'feature.experimental=true can be overridden' '\n    ++\tgit init crufts &&\n    ++\ttest_when_finished \"rm -fr crufts\" &&\n    ++\t(\n    ++\t\tcd crufts &&\n    ++\n    ++\t\tprepare_cruft_history &&\n    ++\t\tgit -c feature.expiremental=true -c gc.cruftPacks=false gc &&\n    ++\t\tassert_no_cruft_packs\n     +\t)\n     +'\n     +\n    @@ t/t6500-gc.sh: test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n     +\n     +\t\tprepare_cruft_history &&\n     +\t\tgit -c feature.experimental=false gc &&\n    -+\t\trefute_cruft_packs_exist\n    ++\t\tassert_no_cruft_packs\n     +\t)\n     +'\n     +\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465839","messageId":"e8726f3de7a7a5eb3375c4b44e205b1f77174dbb.1666819953.git.me@ttaylorr.com","threadId":"58698","inReplyTo":"cover.1666819953.git.me@ttaylorr.com","subject":"[PATCH v3 1/2] gc: add tests for --cruft and friends","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-26T21:32:43Z","receivedAt":"2022-10-26T21:32:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Emily Shaffer <emilyshaffer@google.com>\n\nIn 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via\nloose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'.\n'--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that\ndoesn't check whether a lone gc run generates these cruft packs.\n'gc.cruftPacks' is never exercised.\n\nAdd some tests to exercise these options to gc in the gc test suite.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t6500-gc.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 43 insertions(+)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex cd6c53360d..928a522194 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -202,6 +202,49 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"e\n \tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n '\n \n+prepare_cruft_history () {\n+\ttest_commit base &&\n+\n+\ttest_commit --no-tag foo &&\n+\ttest_commit --no-tag bar &&\n+\tgit reset HEAD^^\n+}\n+\n+assert_cruft_packs () {\n+\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n+\tsed -e 's/\\.mtimes$/\\.pack/g' mtimes >packs &&\n+\n+\ttest_file_not_empty packs &&\n+\twhile read pack\n+\tdo\n+\t\ttest_path_is_file \"$pack\" || return 1\n+\tdone <packs\n+}\n+\n+test_expect_success 'gc --cruft generates a cruft pack' '\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\tgit init crufts &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit gc --cruft &&\n+\t\tassert_cruft_packs\n+\t)\n+'\n+\n+test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\tgit init crufts &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit -c gc.cruftPacks=true gc &&\n+\t\tassert_cruft_packs\n+\t)\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9.  Furthermore, the\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465840","messageId":"13a25a425bc0901ee38760095eb1bcf314c00da2.1666819953.git.me@ttaylorr.com","threadId":"58698","inReplyTo":"cover.1666819953.git.me@ttaylorr.com","subject":"[PATCH v3 2/2] config: let feature.experimental imply gc.cruftPacks=true","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-26T21:32:45Z","receivedAt":"2022-10-26T21:32:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Emily Shaffer <emilyshaffer@google.com>\n\nWe are interested in exploring whether gc.cruftPacks=true should become\nthe default value.\n\nTo determine whether it is safe to do so, let's encourage more users to\ntry it out.\n\nUsers who have set feature.experimental=true have already volunteered to\ntry new and possibly-breaking config changes, so let's try this new\ndefault with that set of users.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/feature.txt |  3 ++\n builtin/gc.c                     |  7 +++--\n repo-settings.c                  |  1 +\n repository.h                     |  1 +\n t/t6500-gc.sh                    | 53 ++++++++++++++++++++++++++++++++\n 5 files changed, 63 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex cdecd04e5b..95975e5091 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -14,6 +14,9 @@ feature.experimental::\n +\n * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by\n skipping more commits at a time, reducing the number of round trips.\n++\n+* `gc.cruftPacks=true` reduces disk space used by unreachable objects during\n+garbage collection, preventing loose object explosions.\n \n feature.manyFiles::\n \tEnable config options that optimize for repos with many files in the\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 243ee85d28..5a84f791ef 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -42,7 +42,7 @@ static const char * const builtin_gc_usage[] = {\n \n static int pack_refs = 1;\n static int prune_reflogs = 1;\n-static int cruft_packs = 0;\n+static int cruft_packs = -1;\n static int aggressive_depth = 50;\n static int aggressive_window = 250;\n static int gc_auto_threshold = 6700;\n@@ -593,6 +593,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n \t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n \n+\tprepare_repo_settings(the_repository);\n+\tif (cruft_packs < 0)\n+\t\tcruft_packs = the_repository->settings.gc_cruft_packs;\n+\n \tif (aggressive) {\n \t\tstrvec_push(&repack, \"-f\");\n \t\tif (aggressive_depth > 0)\n@@ -704,7 +708,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tclean_pack_garbage();\n \t}\n \n-\tprepare_repo_settings(the_repository);\n \tif (the_repository->settings.gc_write_commit_graph == 1)\n \t\twrite_commit_graph_reachable(the_repository->objects->odb,\n \t\t\t\t\t     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\ndiff --git a/repo-settings.c b/repo-settings.c\nindex e8b58151bc..3021921c53 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -43,6 +43,7 @@ void prepare_repo_settings(struct repository *r)\n \t/* Defaults modified by feature.* */\n \tif (experimental) {\n \t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n+\t\tr->settings.gc_cruft_packs = 1;\n \t}\n \tif (manyfiles) {\n \t\tr->settings.index_version = 4;\ndiff --git a/repository.h b/repository.h\nindex 24316ac944..6c461c5b9d 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -34,6 +34,7 @@ struct repo_settings {\n \tint commit_graph_generation_version;\n \tint commit_graph_read_changed_paths;\n \tint gc_write_commit_graph;\n+\tint gc_cruft_packs;\n \tint fetch_write_commit_graph;\n \tint command_requires_full_index;\n \tint sparse_index;\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 928a522194..d9acb63951 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -221,6 +221,11 @@ assert_cruft_packs () {\n \tdone <packs\n }\n \n+assert_no_cruft_packs () {\n+\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n+\ttest_must_be_empty mtimes\n+}\n+\n test_expect_success 'gc --cruft generates a cruft pack' '\n \ttest_when_finished \"rm -fr crufts\" &&\n \tgit init crufts &&\n@@ -245,6 +250,54 @@ test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n \t)\n '\n \n+test_expect_success 'feature.experimental=true generates a cruft pack' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit -c feature.experimental=true gc &&\n+\t\tassert_cruft_packs\n+\t)\n+'\n+\n+test_expect_success 'feature.experimental=false allows explicit cruft packs' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit -c gc.cruftPacks=true -c feature.experimental=false gc &&\n+\t\tassert_cruft_packs\n+\t)\n+'\n+\n+test_expect_success 'feature.experimental=true can be overridden' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit -c feature.expiremental=true -c gc.cruftPacks=false gc &&\n+\t\tassert_no_cruft_packs\n+\t)\n+'\n+\n+test_expect_success 'feature.experimental=false avoids cruft packs by default' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\n+\t\tprepare_cruft_history &&\n+\t\tgit -c feature.experimental=false gc &&\n+\t\tassert_no_cruft_packs\n+\t)\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9.  Furthermore, the\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465841","messageId":"Y1mng9cqAyEqQIQM@nand.local","threadId":"58698","inReplyTo":"xmqqfsfaffs1.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] config: let feature.experimental imply gc.cruftPacks=true","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-26T21:32:51Z","receivedAt":"2022-10-26T21:33:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 26, 2022 at 02:15:10PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > index 243ee85d28..5a84f791ef 100644\n> > --- a/builtin/gc.c\n> > +++ b/builtin/gc.c\n> > @@ -42,7 +42,7 @@ static const char * const builtin_gc_usage[] = {\n> >\n> >  static int pack_refs = 1;\n> >  static int prune_reflogs = 1;\n> > -static int cruft_packs = 0;\n> > +static int cruft_packs = -1;\n> >  static int aggressive_depth = 50;\n> >  static int aggressive_window = 250;\n> >  static int gc_auto_threshold = 6700;\n> > @@ -593,6 +593,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> >  \tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n> >  \t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n> >\n> > +\tprepare_repo_settings(the_repository);\n> > +\tif (cruft_packs < 0)\n> > +\t\tcruft_packs = the_repository->settings.gc_cruft_packs;\n> > +\n> >  \tif (aggressive) {\n> >  \t\tstrvec_push(&repack, \"-f\");\n> >  \t\tif (aggressive_depth > 0)\n> > @@ -704,7 +708,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> >  \t\tclean_pack_garbage();\n> >  \t}\n> >\n> > -\tprepare_repo_settings(the_repository);\n>\n> It is curious why we had this call so late in the sequence in the\n> original.  This is well past what can be reasonably called \"start-up\".\n> We have locked the repository, we may have daemonized ourselves, we\n> may already have packed loose refs and pruned reflogs.  Was that due\n> to somewhat lazy thinking that the next line is the first one that\n> requires the repository's settings already prepared, I wonder.\n\nThe latter. The general practice I've observed with\nprepare_repo_settings() is that it is supposed to be called lazily, just\nbefore the first piece of code in a particular file that is going to\nread the repo settings.\n\nSince the function is a noop on any subsequent calls, we can afford to\nbe over-eager placing it around.\n\nSo it would have been equally OK to duplicate the call, but it's\nunnecessary since the first call is entered unconditionally from within\ncmd_gc().\n\n> > +refute_cruft_packs_exist () {\n> > +\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n> > +\ttest_must_be_empty mtimes\n> > +}\n>\n> Hmph, not \"assert_no_cruft_packs\"?\n\nHeh, sorry :-).\n\n> One thing missing is a test for the escape hatch for those who opt\n> into the experimental world when gc.cruftPacks feature is broken.\n> IOW,\n>\n> \tgit -c feature.experimental=true -c gc.cruftPacks=false\n>\n> should allow them to opt out of gc.cruftPacks.\n>\n> Other than that, looking good.\n\nIndeed, thanks for spotting it. I'll send a reroll with the additional\ntest.\n\nThanks,\nTaylor\n"},{"id":"466079","messageId":"fe5658f8-ac50-b328-f286-ef0971455046@github.com","threadId":"58698","inReplyTo":"13a25a425bc0901ee38760095eb1bcf314c00da2.1666819953.git.me@ttaylorr.com","subject":"Re: [PATCH v3 2/2] config: let feature.experimental imply gc.cruftPacks=true","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-10-31T10:46:30Z","receivedAt":"2022-10-31T10:46:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/26/22 5:32 PM, Taylor Blau wrote:\n> From: Emily Shaffer <emilyshaffer@google.com>\n> \n> We are interested in exploring whether gc.cruftPacks=true should become\n> the default value.\n> \n> To determine whether it is safe to do so, let's encourage more users to\n> try it out.\n> \n> Users who have set feature.experimental=true have already volunteered to\n> try new and possibly-breaking config changes, so let's try this new\n> default with that set of users.\n> \n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  Documentation/config/feature.txt |  3 ++\n>  builtin/gc.c                     |  7 +++--\n>  repo-settings.c                  |  1 +\n>  repository.h                     |  1 +\n>  t/t6500-gc.sh                    | 53 ++++++++++++++++++++++++++++++++\n>  5 files changed, 63 insertions(+), 2 deletions(-)\n> \n> diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\n> index cdecd04e5b..95975e5091 100644\n> --- a/Documentation/config/feat\n\n\n\n> +\tprepare_repo_settings(the_repository);\n> +\tif (cruft_packs < 0)\n> +\t\tcruft_packs = the_repository->settings.gc_cruft_packs;\n> +\n\nHere, we are checking if cruft_packs hasn't been set by the\ncommand-line arguments _or_ the gc.cruftPacks config option\n(as checked in gc_config()). However, we now have another\nparameter saying \"enable it by the experimental flag\".\n\nYou are applying things in the correct order here, but I\nwanted to point out...\n\n> @@ -43,6 +43,7 @@ void prepare_repo_settings(struct repository *r)\n>  \t/* Defaults modified by feature.* */\n>  \tif (experimental) {\n>  \t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n> +\t\tr->settings.gc_cruft_packs = 1;\n>  \t}\n>  \tif (manyfiles) {\n>  \t\tr->settings.index_version = 4;\n\nThat later in prepare_repo_settings(), we check the exact\nconfig values so that the repo_settings reflects the full\nimplications from config:\n\n\t/* Commit graph config or default, does not cascade (simple) */\n\trepo_cfg_bool(r, \"core.commitgraph\", &r->settings.core_commit_graph, 1);\n\trepo_cfg_int(r, \"commitgraph.generationversion\", &r->settings.commit_graph_generation_version, 2);\n\trepo_cfg_bool(r, \"commitgraph.readchangedpaths\", &r->settings.commit_graph_read_changed_paths, 1);\n\trepo_cfg_bool(r, \"gc.writecommitgraph\", &r->settings.gc_write_commit_graph, 1);\n\trepo_cfg_bool(r, \"fetch.writecommitgraph\", &r->settings.fetch_write_commit_graph, 0);\n\n\t/* Boolean config or default, does not cascade (simple)  */\n\trepo_cfg_bool(r, \"pack.usesparse\", &r->settings.pack_use_sparse, 1);\n\trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n\trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n\nThis allows us to use the_repository->settings.* members\nas placeholders for the full implications from config.\n\nSo this implementation has some slight differences from\nother repo_settings implementations. It's correct, and the\nuse of gc.cruftPacks is pretty isolated, but I wonder if\nwe should be sticklers about this pattern in repo_settings.\n\nThanks,\n-Stolee\n\nP.S. I found this draft open on my laptop when I opened it\nthis morning. Hopefully it's not too late.\n"}]}