{"thread":{"id":"59612","subject":"[PATCH 00/10] gc: enable cruft packs by default","startedAt":"2023-04-17T20:56:56Z","lastAt":"2023-04-22T11:23:37Z","messageCount":48,"participants":["Taylor Blau","Junio C Hamano","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"475547","messageId":"cover.1681764848.git.me@ttaylorr.com","threadId":"59612","inReplyTo":null,"subject":"[PATCH 00/10] gc: enable cruft packs by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T20:54:11Z","receivedAt":"2023-04-17T20:56:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series graduates `gc.cruftPacks` out of `feature.experimental` and\nenables it by default, meaning that a bare `git gc` will generate a\ncruft pack.\n\nThe main benefits are described in detail in the second to last patch,\nbut a brief summary is that:\n\n - cruft packs avoid an issue where repositories with many unreachable\n objects that were written too recently to be pruned causes an\n explosion of loose object files\n\n - cruft packs allow pairs of unreachable objects with similar content\n to delta with each other since they can be represented in a single\n pack instead of as individual loose objects.\n\nCruft packs have been in the tree for the vast majority of the past\ncalendar year, and have been in use at GitHub for even longer without\nissue.\n\nThe series is structured as follows:\n\n - The first two patches are cleanup.\n - The third patches fixes an oversight where the code for `git gc`'s\n `--keep-largest-pack` option would incorrectly consider cruft packs.\n - The next five patches are test preparation.\n - Then the substantive patch, to actually graduate `gc.cruftPacks` and\n enable it by default.\n - The final patch is some cleanup that can only take place towards the\n end of the series.\n\nCruft packs have been a success for us at GitHub, and I am really\nexcited to get it in the hands of more users by default.\n\nThanks in advance for your review.\n\n-Taylor\n\nTaylor Blau (10):\n pack-write.c: plug a leak in stage_tmp_packfiles()\n builtin/repack.c: fix incorrect reference to '-C'\n builtin/gc.c: ignore cruft packs with `--keep-largest-pack`\n t/t5304-prune.sh: prepare for `gc --cruft` by default\n t/t9300-fast-import.sh: prepare for `gc --cruft` by default\n t/t6500-gc.sh: refactor cruft pack tests\n t/t6500-gc.sh: add additional test cases\n t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default\n builtin/gc.c: make `gc.cruftPacks` enabled by default\n repository.h: drop unused `gc_cruft_packs`\n\n Documentation/config/feature.txt | 3 -\n Documentation/config/gc.txt | 12 +--\n Documentation/git-gc.txt | 12 +--\n Documentation/gitformat-pack.txt | 4 +-\n builtin/gc.c | 8 +-\n builtin/repack.c | 2 +-\n pack-write.c | 14 ++--\n repo-settings.c | 4 +-\n repository.h | 1 -\n t/t5304-prune.sh | 28 +++----\n t/t6500-gc.sh | 136 ++++++++++++++++---------------\n t/t6501-freshen-objects.sh | 10 +--\n t/t9300-fast-import.sh | 13 +--\n 13 files changed, 121 insertions(+), 126 deletions(-)\n\n-- \n2.38.1\n"},{"id":"475548","messageId":"65ac7ed9b843a83f7b4cc0acd7b4e70d98ca20aa.1681764848.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"[PATCH 01/10] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T20:54:15Z","receivedAt":"2023-04-17T20:56:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function `stage_tmp_packfiles()` generates a filename to use for\nstaging the contents of what will become the pack's \".mtimes\" file.\n\nThe name is generated in `write_mtimes_file()` and the result is\nreturned back to `stage_tmp_packfiles()` which uses it to rename the\ntemporary file into place via `rename_tmp_packfiles()`.\n\n`write_mtimes_file()` returns a `const char *`, indicating that callers\nare not expected to free its result (similar to, e.g., `oid_to_hex()`).\nBut callers are expected to free its result, so this return type is\nincorrect.\n\nChange the function's signature to return a non-const `char *`, and free\nit at the end of `stage_tmp_packfiles()`.\n\nSigned-off-by: Taylor Blau \n---\n pack-write.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex f171405495..4da0ccc5f5 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -312,13 +312,13 @@ static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash)\n \thashwrite(f, hash, the_hash_algo->rawsz);\n }\n \n-static const char *write_mtimes_file(struct packing_data *to_pack,\n-\t\t\t\t struct pack_idx_entry **objects,\n-\t\t\t\t uint32_t nr_objects,\n-\t\t\t\t const unsigned char *hash)\n+static char *write_mtimes_file(struct packing_data *to_pack,\n+\t\t\t struct pack_idx_entry **objects,\n+\t\t\t uint32_t nr_objects,\n+\t\t\t const unsigned char *hash)\n {\n \tstruct strbuf tmp_file = STRBUF_INIT;\n-\tconst char *mtimes_name;\n+\tchar *mtimes_name;\n \tstruct hashfile *f;\n \tint fd;\n \n@@ -544,7 +544,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\t\t char **idx_tmp_name)\n {\n \tconst char *rev_tmp_name = NULL;\n-\tconst char *mtimes_tmp_name = NULL;\n+\tchar *mtimes_tmp_name = NULL;\n \n \tif (adjust_shared_perm(pack_tmp_name))\n \t\tdie_errno(\"unable to make temporary pack file readable\");\n@@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\trename_tmp_packfile(name_buffer, rev_tmp_name, \"rev\");\n \tif (mtimes_tmp_name)\n \t\trename_tmp_packfile(name_buffer, mtimes_tmp_name, \"mtimes\");\n+\n+\tfree(mtimes_tmp_name);\n }\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n-- \n2.38.1\n\n"},{"id":"475549","messageId":"fbc8d15032e356cb742d1156629ad252a21e6d8c.1681764848.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"[PATCH 02/10] builtin/repack.c: fix incorrect reference to '-C'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T20:54:18Z","receivedAt":"2023-04-17T20:57:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When cruft packs were originally being developed, `-C` was designated as\nthe short-form for `--cruft` (as in `git repack -C`).\n\nThis was dropped due to confusion with Git's top-level `-C` option\nbefore submitting to the list. But the reference to it in\n`--cruft-expiration`'s help text was never updated. Fix that dangling\nreference in this patch.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex df4d8e0f0b..d9eee15c2f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -774,7 +774,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"same as -a, pack unreachable cruft objects separately\"),\n \t\t\t\t PACK_CRUFT),\n \t\tOPT_STRING(0, \"cruft-expiration\", &cruft_expiration, N_(\"approxidate\"),\n-\t\t\t\tN_(\"with -C, expire objects older than this\")),\n+\t\t\t\tN_(\"with --cruft, expire objects older than this\")),\n \t\tOPT_BOOL('d', NULL, &delete_redundant,\n \t\t\t\tN_(\"remove redundant packs, and run git-prune-packed\")),\n \t\tOPT_BOOL('f', NULL, &po_args.no_reuse_delta,\n-- \n2.38.1\n\n"},{"id":"475550","messageId":"796df920ad6af0ee9101a0f3f80edbc793987336.1681764848.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"[PATCH 03/10] builtin/gc.c: ignore cruft packs with `--keep-largest-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T20:54:21Z","receivedAt":"2023-04-17T20:57:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When cruft packs were implemented, we never adjusted the code for `git\ngc`'s `--keep-largest-pack` and `gc.bigPackThreshold` to ignore cruft\npacks. This option and configuration option share a common\nimplementation, but including cruft packs is wrong in both cases:\n\n - Running `git gc --keep-largest-pack` in a repository where the\n largest pack is the cruft pack itself will make it impossible for\n `git gc` to prune objects, since the cruft pack itself is kept.\n\n - The same is true for `gc.bigPackThreshold`, if the size of the cruft\n pack exceeds the limit set by the caller.\n\nIgnore cruft packs in the common implementation for both of these\noptions, and add a pair of tests to prevent any future regressions here.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/gc.txt | 10 ++++-----\n Documentation/git-gc.txt | 7 +++---\n builtin/gc.c | 2 +-\n t/t6500-gc.sh | 43 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 53 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt\nindex 38fea076a2..8d5353e9e0 100644\n--- a/Documentation/config/gc.txt\n+++ b/Documentation/config/gc.txt\n@@ -43,11 +43,11 @@ gc.autoDetach::\n \tif the system supports it. Default is true.\n \n gc.bigPackThreshold::\n-\tIf non-zero, all packs larger than this limit are kept when\n-\t`git gc` is run. This is very similar to `--keep-largest-pack`\n-\texcept that all packs that meet the threshold are kept, not\n-\tjust the largest pack. Defaults to zero. Common unit suffixes of\n-\t'k', 'm', or 'g' are supported.\n+\tIf non-zero, all non-cruft packs larger than this limit are kept\n+\twhen `git gc` is run. This is very similar to\n+\t`--keep-largest-pack` except that all non-cruft packs that meet\n+\tthe threshold are kept, not just the largest pack. Defaults to\n+\tzero. Common unit suffixes of 'k', 'm', or 'g' are supported.\n +\n Note that if the number of kept packs is more than gc.autoPackLimit,\n this configuration variable is ignored, all packs except the base pack\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex a65c9aa62d..2427478314 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -77,9 +77,10 @@ be performed as well.\n \tinstance running on this repository.\n \n --keep-largest-pack::\n-\tAll packs except the largest pack and those marked with a\n-\t`.keep` files are consolidated into a single pack. When this\n-\toption is used, `gc.bigPackThreshold` is ignored.\n+\tAll packs except the largest pack, any packs marked with a\n+\t`.keep` file, and any cruft pack(s) are consolidated into a\n+\tsingle pack. When this option is used, `gc.bigPackThreshold` is\n+\tignored.\n \n AGGRESSIVE\n ----------\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex edd98d35a5..53ef137e1d 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -219,7 +219,7 @@ static struct packed_git *find_base_packs(struct string_list *packs,\n \tstruct packed_git *p, *base = NULL;\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n-\t\tif (!p->pack_local)\n+\t\tif (!p->pack_local || p->is_cruft)\n \t\t\tcontinue;\n \t\tif (limit) {\n \t\t\tif (p->pack_size >= limit)\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex d9acb63951..df6f2e6e52 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -298,6 +298,49 @@ test_expect_success 'feature.experimental=false avoids cruft packs by default' '\n \t)\n '\n \n+test_expect_success '--keep-largest-pack ignores cruft packs' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Generate a pack for reachable objects (of which there\n+\t\t# are 3), and one for unreachable objects (of which\n+\t\t# there are 6).\n+\t\tprepare_cruft_history &&\n+\t\tgit gc --cruft &&\n+\n+\t\tmtimes=\"$(find .git/objects/pack -type f -name \"pack-*.mtimes\")\" &&\n+\t\tsz=\"$(test_file_size \"${mtimes%.mtimes}.pack\")\" &&\n+\n+\t\t# Ensure that the cruft pack gets removed (due to\n+\t\t# `--prune=now`) despite it being the largest pack.\n+\t\tgit -c gc.bigPackThreshold=$sz gc --cruft --prune=now &&\n+\n+\t\tassert_no_cruft_packs\n+\t)\n+'\n+\n+test_expect_success 'gc.bigPackThreshold ignores cruft packs' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Generate a pack for reachable objects (of which there\n+\t\t# are 3), and one for unreachable objects (of which\n+\t\t# there are 6).\n+\t\tprepare_cruft_history &&\n+\t\tgit gc --cruft &&\n+\n+\t\t# Ensure that the cruft pack gets removed (due to\n+\t\t# `--prune=now`) despite it being the largest pack.\n+\t\tgit gc --cruft --prune=now --keep-largest-pack &&\n+\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.1\n\n"},{"id":"475551","messageId":"44006da959eeeaef93a33d73071fb5edc45cd45b.1681764848.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"[PATCH 04/10] t/t5304-prune.sh: prepare for `gc --cruft` by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T20:54:24Z","receivedAt":"2023-04-17T20:57:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Many of the tests in t5304 run `git gc`, and rely on its behavior that\nunreachable-but-recent objects are written out loose. This is sensible,\nsince t5304 deals specifically with this kind of pruning.\n\nIf left unattended, however, this test would break when the default\nbehavior of a bare \"git gc\" is adjusted to generate a cruft pack by\ndefault.\n\nEnsure that these tests continue to work as-is (and continue to provide\ncoverage of loose object pruning) by passing `--no-cruft` explicitly.\n\nSigned-off-by: Taylor Blau \n---\n t/t5304-prune.sh | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 5500dd0842..662ae9b152 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -62,11 +62,11 @@ test_expect_success 'prune --expire' '\n test_expect_success 'gc: implicit prune --expire' '\n \tadd_blob &&\n \ttest-tool chmtime =-$((2*$week-30)) $BLOB_FILE &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \tverbose test $((1 + $before)) = $(git count-objects | sed \"s/ .*//\") &&\n \ttest_path_is_file $BLOB_FILE &&\n \ttest-tool chmtime =-$((2*$week+1)) $BLOB_FILE &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \tverbose test $before = $(git count-objects | sed \"s/ .*//\") &&\n \ttest_path_is_missing $BLOB_FILE\n '\n@@ -86,7 +86,7 @@ test_expect_success 'gc: refuse to start with invalid gc.pruneExpire' '\n \n test_expect_success 'gc: start with ok gc.pruneExpire' '\n \tgit config gc.pruneExpire 2.days.ago &&\n-\tgit gc\n+\tgit gc --no-cruft\n '\n \n test_expect_success 'prune: prune nonsense parameters' '\n@@ -137,44 +137,44 @@ test_expect_success 'gc --no-prune' '\n \tadd_blob &&\n \ttest-tool chmtime =-$((5001*$day)) $BLOB_FILE &&\n \tgit config gc.pruneExpire 2.days.ago &&\n-\tgit gc --no-prune &&\n+\tgit gc --no-prune --no-cruft &&\n \tverbose test 1 = $(git count-objects | sed \"s/ .*//\") &&\n \ttest_path_is_file $BLOB_FILE\n '\n \n test_expect_success 'gc respects gc.pruneExpire' '\n \tgit config gc.pruneExpire 5002.days.ago &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \ttest_path_is_file $BLOB_FILE &&\n \tgit config gc.pruneExpire 5000.days.ago &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \ttest_path_is_missing $BLOB_FILE\n '\n \n test_expect_success 'gc --prune=' '\n \tadd_blob &&\n \ttest-tool chmtime =-$((5001*$day)) $BLOB_FILE &&\n-\tgit gc --prune=5002.days.ago &&\n+\tgit gc --prune=5002.days.ago --no-cruft &&\n \ttest_path_is_file $BLOB_FILE &&\n-\tgit gc --prune=5000.days.ago &&\n+\tgit gc --prune=5000.days.ago --no-cruft &&\n \ttest_path_is_missing $BLOB_FILE\n '\n \n test_expect_success 'gc --prune=never' '\n \tadd_blob &&\n-\tgit gc --prune=never &&\n+\tgit gc --prune=never --no-cruft &&\n \ttest_path_is_file $BLOB_FILE &&\n-\tgit gc --prune=now &&\n+\tgit gc --prune=now --no-cruft &&\n \ttest_path_is_missing $BLOB_FILE\n '\n \n test_expect_success 'gc respects gc.pruneExpire=never' '\n \tgit config gc.pruneExpire never &&\n \tadd_blob &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \ttest_path_is_file $BLOB_FILE &&\n \tgit config gc.pruneExpire now &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \ttest_path_is_missing $BLOB_FILE\n '\n \n@@ -194,7 +194,7 @@ test_expect_success 'gc: prune old objects after local clone' '\n \t\tcd aclone &&\n \t\tverbose test 1 = $(git count-objects | sed \"s/ .*//\") &&\n \t\ttest_path_is_file $BLOB_FILE &&\n-\t\tgit gc --prune &&\n+\t\tgit gc --prune --no-cruft &&\n \t\tverbose test 0 = $(git count-objects | sed \"s/ .*//\") &&\n \t\ttest_path_is_missing $BLOB_FILE\n \t)\n@@ -237,7 +237,7 @@ test_expect_success 'clean pack garbage with gc' '\n \t>.git/objects/pack/fake2.keep &&\n \t>.git/objects/pack/fake2.idx &&\n \t>.git/objects/pack/fake3.keep &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n \tcat >expected <<\\EOF &&\n-- \n2.38.1\n\n"},{"id":"475552","messageId":"1b07eb83fe6ab9fcb65d5a4746dec50c0f2cbd9e.1681764848.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"[PATCH 05/10] t/t9300-fast-import.sh: prepare for `gc --cruft` by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T20:54:27Z","receivedAt":"2023-04-17T20:57:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar fashion as the previous commit, adjust the fast-import\ntests to prepare for \"git gc\" generating a cruft pack by default.\n\nThis adjustment is slightly different, however. Instead of relying on us\nwriting out the objects loose, and then calling `git prune` to remove\nthem, t9300 needs to be prepared to drop objects that would be moved\ninto cruft packs.\n\nTo do this, we can combine the `git gc` invocation with `git prune` into\none `git gc --prune`, which handles pruning both loose objects, and\nobjects that would otherwise be written to a cruft pack.\n\nSigned-off-by: Taylor Blau \n---\n t/t9300-fast-import.sh | 13 ++++---------\n 1 file changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex aa55b41b9a..ac237a1f90 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -388,9 +388,7 @@ test_expect_success 'B: accept branch name \"TEMP_TAG\"' '\n \n \tINPUT_END\n \n-\ttest_when_finished \"rm -f .git/TEMP_TAG\n-\t\tgit gc\n-\t\tgit prune\" &&\n+\ttest_when_finished \"rm -f .git/TEMP_TAG && git gc --prune=now\" &&\n \tgit fast-import \n---\n t/t6500-gc.sh | 126 ++++++++++++++++++--------------------------------\n 1 file changed, 44 insertions(+), 82 deletions(-)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex df6f2e6e52..a2f988c5c2 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -210,93 +210,55 @@ prepare_cruft_history () {\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 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(\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-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+for argv in \\\n+\t\"gc --cruft\" \\\n+\t\"-c gc.cruftPacks=true gc\" \\\n+\t\"-c feature.experimental=true gc\" \\\n+\t\"-c gc.cruftPacks=true -c feature.experimental=false gc\"\n+do\n+\ttest_expect_success \"git $argv generates a cruft pack\" '\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\tgit init repo &&\n+\t\t(\n+\t\t\tcd repo &&\n+\n+\t\t\tprepare_cruft_history &&\n+\t\t\tgit $argv &&\n+\n+\t\t\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n+\t\t\tsed -e 's/\\.mtimes$/\\.pack/g' mtimes >packs &&\n+\n+\t\t\ttest_file_not_empty packs &&\n+\t\t\twhile read pack\n+\t\t\tdo\n+\t\t\t\ttest_path_is_file \"$pack\" || return 1\n+\t\t\tdone \n---\n t/t6500-gc.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex a2f988c5c2..e7d3d1448f 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -218,6 +218,7 @@ assert_no_cruft_packs () {\n for argv in \\\n \t\"gc --cruft\" \\\n \t\"-c gc.cruftPacks=true gc\" \\\n+\t\"-c gc.cruftPacks=false gc --cruft\" \\\n \t\"-c feature.experimental=true gc\" \\\n \t\"-c gc.cruftPacks=true -c feature.experimental=false gc\"\n do\n@@ -243,6 +244,9 @@ do\n done\n \n for argv in \\\n+\t\"gc --no-cruft\" \\\n+\t\"-c gc.cruftPacks=false gc\" \\\n+\t\"-c gc.cruftPacks=true gc --no-cruft\" \\\n \t\"-c feature.expiremental=true -c gc.cruftPacks=false gc\" \\\n \t\"-c feature.experimental=false gc\"\n do\n-- \n2.38.1\n\n"},{"id":"475555","messageId":"4ccc525c39dd23db1136681bc0ffd572db0ed2f2.1681764848.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"[PATCH 08/10] t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T20:54:36Z","receivedAt":"2023-04-17T20:58:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, prepare for `gc --cruft`\nbecoming the default by ensuring that the tests in t6501 explicitly\ncover the case of freshening loose objects not using cruft packs.\n\nWe could run this test twice, once with `--cruft` and once with\n`--no-cruft`, but doing so is unnecessary, since the object rescuing and\nfreshening behavior is already extensively tested via t5329.\n\nSigned-off-by: Taylor Blau \n---\n t/t6501-freshen-objects.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh\nindex 3968b47ed5..dbfa8a4d4c 100755\n--- a/t/t6501-freshen-objects.sh\n+++ b/t/t6501-freshen-objects.sh\n@@ -101,7 +101,7 @@ do\n \t'\n \n \ttest_expect_success \"simultaneous gc ($title)\" '\n-\t\tgit gc --prune=12.hours.ago\n+\t\tgit gc --no-cruft --prune=12.hours.ago\n \t'\n \n \ttest_expect_success \"finish writing out commit ($title)\" '\n@@ -131,7 +131,7 @@ do\n \t'\n \n \ttest_expect_success \"simultaneous gc ($title)\" '\n-\t\tgit gc --prune=12.hours.ago\n+\t\tgit gc --no-cruft --prune=12.hours.ago\n \t'\n \n \t# tree should have been refreshed by write-tree\n@@ -151,7 +151,7 @@ test_expect_success 'do not complain about existing broken links (commit)' '\n \tsome message\n \tEOF\n \tcommit=$(git hash-object -t commit -w broken-commit) &&\n-\tgit gc -q 2>stderr &&\n+\tgit gc --no-cruft -q 2>stderr &&\n \tverbose git cat-file -e $commit &&\n \ttest_must_be_empty stderr\n '\n@@ -161,7 +161,7 @@ test_expect_success 'do not complain about existing broken links (tree)' '\n \t100644 blob $(test_oid 003)\tfoo\n \tEOF\n \ttree=$(git mktree --missing stderr &&\n+\tgit gc --no-cruft -q 2>stderr &&\n \tgit cat-file -e $tree &&\n \ttest_must_be_empty stderr\n '\n@@ -176,7 +176,7 @@ test_expect_success 'do not complain about existing broken links (tag)' '\n \tthis is a broken tag\n \tEOF\n \ttag=$(git hash-object -t tag -w broken-tag) &&\n-\tgit gc -q 2>stderr &&\n+\tgit gc --no-cruft -q 2>stderr &&\n \tgit cat-file -e $tag &&\n \ttest_must_be_empty stderr\n '\n-- \n2.38.1\n\n"},{"id":"475556","messageId":"bfda40a21ddb7abceb6a7eb9aa1f71aa5d856333.1681764848.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"[PATCH 09/10] builtin/gc.c: make `gc.cruftPacks` enabled by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T20:54:39Z","receivedAt":"2023-04-17T20:58:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Back in 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects\nvia loose, 2022-05-20), `git gc` learned the `--cruft` option and\n`gc.cruftPacks` configuration to opt-in to writing cruft packs when\ncollecting or pruning unreachable objects.\n\nCruft packs were introduced with the merge in a50036da1a (Merge branch\n'tb/cruft-packs', 2022-06-03). They address the problem of \"loose object\nexplosions\", where Git will write out many individual loose objects when\nthere is a large number of unreachable objects that have not yet aged\npast `--prune=`.\n\nInstead of keeping track of those unreachable yet recent objects via\ntheir loose object file's mtime, cruft packs collect all unreachable\nobjects into a single pack with a corresponding `*.mtimes` file that\nacts as a table to store the mtimes of all unreachable objects. This\nprevents the need to store unreachable objects as loose as they age out\nof the repository, and avoids the problem of loose object explosions.\n\nBeyond avoiding loose object explosions, cruft packs also act as a more\nefficient mechanism to store unreachable objects as they age out of a\nrepository. This is because pairs of similar unreachable objects serve\nas delta bases for one another.\n\nIn 5b92477f89, the feature was introduced as experimental. Since then,\nGitHub has been running these patches in every repository generating\nhundreds of millions of cruft packs along the way. The feature is\nbattle-tested, and avoids many pathological cases such as above. Users\nwho either run `git gc` manually, or via `git maintenance` can benefit\nfrom having cruft packs.\n\nAs such, enable cruft pack generation to take place by default (by\nmaking `gc.cruftPacks` have the default of \"true\" rather than \"false).\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/feature.txt | 3 ---\n Documentation/config/gc.txt | 2 +-\n Documentation/git-gc.txt | 5 +++--\n Documentation/gitformat-pack.txt | 4 ++--\n builtin/gc.c | 6 +-----\n t/t6500-gc.sh | 9 +++------\n 6 files changed, 10 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex e52bc6b858..17b4d39f89 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -14,9 +14,6 @@ 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/Documentation/config/gc.txt b/Documentation/config/gc.txt\nindex 8d5353e9e0..7f95c866e1 100644\n--- a/Documentation/config/gc.txt\n+++ b/Documentation/config/gc.txt\n@@ -84,7 +84,7 @@ gc.packRefs::\n gc.cruftPacks::\n \tStore unreachable objects in a cruft pack (see\n \tlinkgit:git-repack[1]) instead of as loose objects. The default\n-\tis `false`.\n+\tis `true`.\n \n gc.pruneExpire::\n \tWhen 'git gc' is run, it will call 'prune --expire 2.weeks.ago'\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex 2427478314..0b4e7ba882 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -54,9 +54,10 @@ other housekeeping tasks (e.g. rerere, working trees, reflog...) will\n be performed as well.\n \n \n---cruft::\n+--[no-]cruft::\n \tWhen expiring unreachable objects, pack them separately into a\n-\tcruft pack instead of storing them as loose objects.\n+\tcruft pack instead of storing them as loose objects. `--cruft`\n+\tis on by default.\n \n --prune=::\n \tPrune loose objects older than date (default is 2 weeks ago,\ndiff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\nindex e06af02f21..0c1be2dbe8 100644\n--- a/Documentation/gitformat-pack.txt\n+++ b/Documentation/gitformat-pack.txt\n@@ -611,8 +611,8 @@ result of repeatedly resetting the objects' mtimes to the present time.\n \n If you are GC-ing repositories in a mixed version environment, consider omitting\n the `--cruft` option when using linkgit:git-repack[1] and linkgit:git-gc[1], and\n-leaving the `gc.cruftPacks` configuration unset until all writers understand\n-cruft packs.\n+setting the `gc.cruftPacks` configuration to \"false\" until all writers\n+understand cruft packs.\n \n === Alternatives\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 53ef137e1d..ece01e966f 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -48,7 +48,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 = -1;\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@@ -608,10 +608,6 @@ 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)\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex e7d3d1448f..75760866b4 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -216,11 +216,10 @@ assert_no_cruft_packs () {\n }\n \n for argv in \\\n+\t\"gc\" \\\n \t\"gc --cruft\" \\\n \t\"-c gc.cruftPacks=true gc\" \\\n-\t\"-c gc.cruftPacks=false gc --cruft\" \\\n-\t\"-c feature.experimental=true gc\" \\\n-\t\"-c gc.cruftPacks=true -c feature.experimental=false gc\"\n+\t\"-c gc.cruftPacks=false gc --cruft\"\n do\n \ttest_expect_success \"git $argv generates a cruft pack\" '\n \t\ttest_when_finished \"rm -fr repo\" &&\n@@ -246,9 +245,7 @@ done\n for argv in \\\n \t\"gc --no-cruft\" \\\n \t\"-c gc.cruftPacks=false gc\" \\\n-\t\"-c gc.cruftPacks=true gc --no-cruft\" \\\n-\t\"-c feature.expiremental=true -c gc.cruftPacks=false gc\" \\\n-\t\"-c feature.experimental=false gc\"\n+\t\"-c gc.cruftPacks=true gc --no-cruft\"\n do\n \ttest_expect_success \"git $argv does not generate a cruft pack\" '\n \t\ttest_when_finished \"rm -fr repo\" &&\n-- \n2.38.1\n\n"},{"id":"475557","messageId":"fa15125454bb6596adbe68689555337a42cb80b7.1681764848.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"[PATCH 10/10] repository.h: drop unused `gc_cruft_packs`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T20:54:43Z","receivedAt":"2023-04-17T20:58:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of the previous commit, all callers that need to read the value of\n`gc.cruftPacks` do so outside without using the `repo_settings` struct,\nmaking its `gc_cruft_packs` unused. Drop it accordingly.\n\nSigned-off-by: Taylor Blau \n---\n repo-settings.c | 4 +---\n repository.h | 1 -\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0a6c0b381f..a8d0b98794 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -41,10 +41,8 @@ void prepare_repo_settings(struct repository *r)\n \trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n \n \t/* Defaults modified by feature.* */\n-\tif (experimental) {\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 \t\tr->settings.index_skip_hash = 1;\ndiff --git a/repository.h b/repository.h\nindex 15a8afc5fb..50eb0ce391 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -33,7 +33,6 @@ 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-- \n2.38.1\n"},{"id":"475569","messageId":"xmqqildui0gk.fsf@gitster.g","threadId":"59612","inReplyTo":"796df920ad6af0ee9101a0f3f80edbc793987336.1681764848.git.me@ttaylorr.com","subject":"Re: [PATCH 03/10] builtin/gc.c: ignore cruft packs with `--keep-largest-pack`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-17T22:54:35Z","receivedAt":"2023-04-17T22:54:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> When cruft packs were implemented, we never adjusted the code for `git\n> gc`'s `--keep-largest-pack` and `gc.bigPackThreshold` to ignore cruft\n> packs. This option and configuration option share a common\n> implementation, but including cruft packs is wrong in both cases:\n>\n> - Running `git gc --keep-largest-pack` in a repository where the\n> largest pack is the cruft pack itself will make it impossible for\n> `git gc` to prune objects, since the cruft pack itself is kept.\n\nMakes sense. We want to keep the largest pack that is actually in\nuse, and we want to consolidate other non-cruft packs into one.\n\n> - The same is true for `gc.bigPackThreshold`, if the size of the cruft\n> pack exceeds the limit set by the caller.\n\nThis is not as cut-and-dried clear as the previous one. \"This pack\nis so large that it is not worth rewriting it only to expunge a\nhandful of objects that are no longer reachable from it\" is the main\nmotivation to use this configuration, but doesn't some part of the\nsame reasoning apply equally to a large cruft pack? But let's\nassume that the configuration is totally irrelevant to cruft packs\nand read on.\n\n> --keep-largest-pack::\n> -\tAll packs except the largest pack and those marked with a\n> -\t`.keep` files are consolidated into a single pack. When this\n> -\toption is used, `gc.bigPackThreshold` is ignored.\n> +\tAll packs except the largest pack, any packs marked with a\n> +\t`.keep` file, and any cruft pack(s) are consolidated into a\n> +\tsingle pack. When this option is used, `gc.bigPackThreshold` is\n> +\tignored.\n\n\"except the largest pack\" -> \"except the largest, non-cruft pack\"\n\n"},{"id":"475570","messageId":"ZD3QLMs8/+DLKZM6@nand.local","threadId":"59612","inReplyTo":"xmqqildui0gk.fsf@gitster.g","subject":"Re: [PATCH 03/10] builtin/gc.c: ignore cruft packs with `--keep-largest-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T23:03:08Z","receivedAt":"2023-04-17T23:03:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 17, 2023 at 03:54:35PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > - The same is true for `gc.bigPackThreshold`, if the size of the cruft\n> > pack exceeds the limit set by the caller.\n>\n> This is not as cut-and-dried clear as the previous one. \"This pack\n> is so large that it is not worth rewriting it only to expunge a\n> handful of objects that are no longer reachable from it\" is the main\n> motivation to use this configuration, but doesn't some part of the\n> same reasoning apply equally to a large cruft pack? But let's\n> assume that the configuration is totally irrelevant to cruft packs\n> and read on.\n\nThis is an inherent design trade-off. I imagine that callers who want to\navoid rewriting their (large) cruft packs would prefer to generate a new\ncruft pack on top with just the recently accumulated unreachable\nobjects.\n\nThat kind of works, except if you need to prune objects that are packed\nin an earlier cruft pack. If you have `gc.bigPackThreshold`, there is no\nway to do this if you need to expire objects that are in cruft packs\nabove that threshold.\n\nA user may find themselves frustrated when trying to `git gc --prune`\nsome sensitive object(s) from their repository doesn't appear to work,\nonly to discover that `gc.bigPackThreshold` is set somewhere in their\nconfiguration.\n\nWriting (largely) the same cruft pack to expunge a few objects isn't\nideal, but it is better than the status quo. And if you have so many\nunreachable objects that this is a concern, it is probably time to prune\nanyway.\n\nIt is possible that in the future we could support writing multiple\ncruft packs (we already handle the presence of multiple cruft packs\nfine, just don't expose an easy way for the user to write >1 of them).\nAnd at that point we would be able to relax this patch a bit and allow\n`gc.bigPackThreshold` to cover cruft packs, too. But in the meantime,\nthe benefit of avoiding loose object explosions outweighs the possible\ndrawbacks here, IMHO.\n\n> > --keep-largest-pack::\n> > -\tAll packs except the largest pack and those marked with a\n> > -\t`.keep` files are consolidated into a single pack. When this\n> > -\toption is used, `gc.bigPackThreshold` is ignored.\n> > +\tAll packs except the largest pack, any packs marked with a\n> > +\t`.keep` file, and any cruft pack(s) are consolidated into a\n> > +\tsingle pack. When this option is used, `gc.bigPackThreshold` is\n> > +\tignored.\n>\n> \"except the largest pack\" -> \"except the largest, non-cruft pack\"\n\nIndeed, good eyes.\n\nThanks,\nTaylor\n"},{"id":"475605","messageId":"20230418103005.GC508219@coredump.intra.peff.net","threadId":"59612","inReplyTo":"65ac7ed9b843a83f7b4cc0acd7b4e70d98ca20aa.1681764848.git.me@ttaylorr.com","subject":"Re: [PATCH 01/10] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T10:30:05Z","receivedAt":"2023-04-18T10:30:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2023 at 04:54:15PM -0400, Taylor Blau wrote:\n\n> The function `stage_tmp_packfiles()` generates a filename to use for\n> staging the contents of what will become the pack's \".mtimes\" file.\n> \n> The name is generated in `write_mtimes_file()` and the result is\n> returned back to `stage_tmp_packfiles()` which uses it to rename the\n> temporary file into place via `rename_tmp_packfiles()`.\n> \n> `write_mtimes_file()` returns a `const char *`, indicating that callers\n> are not expected to free its result (similar to, e.g., `oid_to_hex()`).\n> But callers are expected to free its result, so this return type is\n> incorrect.\n\nMakes sense. I do think in the long run that it might make sense for all\nof these pack-write tmpfiles to tempfile.[ch] (either directly, or via\nregister_tempfile() after creating the file). And then it would be safe\nto pass around the tempfile struct itself, which contains the filename,\nwithout worrying so much about ownership issues.\n\nBut that's a much bigger change that shouldn't be part of your series.\n\n-Peff\n"},{"id":"475606","messageId":"20230418103909.GD508219@coredump.intra.peff.net","threadId":"59612","inReplyTo":"ZD3QLMs8/+DLKZM6@nand.local","subject":"Re: [PATCH 03/10] builtin/gc.c: ignore cruft packs with `--keep-largest-pack`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T10:39:09Z","receivedAt":"2023-04-18T10:39:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2023 at 07:03:08PM -0400, Taylor Blau wrote:\n\n> On Mon, Apr 17, 2023 at 03:54:35PM -0700, Junio C Hamano wrote:\n> > Taylor Blau writes:\n> >\n> > > - The same is true for `gc.bigPackThreshold`, if the size of the cruft\n> > > pack exceeds the limit set by the caller.\n> >\n> > This is not as cut-and-dried clear as the previous one. \"This pack\n> > is so large that it is not worth rewriting it only to expunge a\n> > handful of objects that are no longer reachable from it\" is the main\n> > motivation to use this configuration, but doesn't some part of the\n> > same reasoning apply equally to a large cruft pack? But let's\n> > assume that the configuration is totally irrelevant to cruft packs\n> > and read on.\n> \n> This is an inherent design trade-off. I imagine that callers who want to\n> avoid rewriting their (large) cruft packs would prefer to generate a new\n> cruft pack on top with just the recently accumulated unreachable\n> objects.\n> \n> That kind of works, except if you need to prune objects that are packed\n> in an earlier cruft pack. If you have `gc.bigPackThreshold`, there is no\n> way to do this if you need to expire objects that are in cruft packs\n> above that threshold.\n> \n> A user may find themselves frustrated when trying to `git gc --prune`\n> some sensitive object(s) from their repository doesn't appear to work,\n> only to discover that `gc.bigPackThreshold` is set somewhere in their\n> configuration.\n> \n> Writing (largely) the same cruft pack to expunge a few objects isn't\n> ideal, but it is better than the status quo. And if you have so many\n> unreachable objects that this is a concern, it is probably time to prune\n> anyway.\n\nYeah, what your patch does makes sense to me as a default behavior. In a\npre-cruft-pack world, those objects would all be left alone by\ngc.bigPackThreshol (because they're loose), and the essence of\ncruft-packs is creating a parallel universe where those ejected-to-loose\nobjects just happen to be stored in a more efficient format.\n\n> It is possible that in the future we could support writing multiple\n> cruft packs (we already handle the presence of multiple cruft packs\n> fine, just don't expose an easy way for the user to write >1 of them).\n> And at that point we would be able to relax this patch a bit and allow\n> `gc.bigPackThreshold` to cover cruft packs, too. But in the meantime,\n> the benefit of avoiding loose object explosions outweighs the possible\n> drawbacks here, IMHO.\n\nI wondered if that interface might be an option to say \"hey, I have a\ngigantic cruft file I want to carry forward, please leave it alone\".\n\nBut if you have a giant cruft pack that is making your \"git gc\" too\nslow, it will eventually age out on its own. And if you're impatient,\nthen \"git gc --prune=now\" is probably the right tool.\n\nAnd If you really did want to keep rolling it forward for some reason,\nthen I'd think marking it with \".keep\" would be the best thing (and\nmaybe even dropping the mtimes file? I'm not sure a how a kept-cruft\npack does or should behave).\n\n-Peff\n"},{"id":"475607","messageId":"20230418104331.GE508219@coredump.intra.peff.net","threadId":"59612","inReplyTo":"1b07eb83fe6ab9fcb65d5a4746dec50c0f2cbd9e.1681764848.git.me@ttaylorr.com","subject":"Re: [PATCH 05/10] t/t9300-fast-import.sh: prepare for `gc --cruft` by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T10:43:31Z","receivedAt":"2023-04-18T10:43:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2023 at 04:54:27PM -0400, Taylor Blau wrote:\n\n> In a similar fashion as the previous commit, adjust the fast-import\n> tests to prepare for \"git gc\" generating a cruft pack by default.\n> \n> This adjustment is slightly different, however. Instead of relying on us\n> writing out the objects loose, and then calling `git prune` to remove\n> them, t9300 needs to be prepared to drop objects that would be moved\n> into cruft packs.\n> \n> To do this, we can combine the `git gc` invocation with `git prune` into\n> one `git gc --prune`, which handles pruning both loose objects, and\n> objects that would otherwise be written to a cruft pack.\n\nGood. This is more efficient anyway. It probably does not matter for our\ntiny test repository, but it is always good for our tests to model the\nbest option when possible. :)\n\n> t/t9300-fast-import.sh | 13 ++++---------\n> 1 file changed, 4 insertions(+), 9 deletions(-)\n\nThe patch itself looks good to me.\n\n-Peff\n"},{"id":"475608","messageId":"20230418104813.GF508219@coredump.intra.peff.net","threadId":"59612","inReplyTo":"6957e54f51759ff1b6d2469bc40c9b966635595d.1681764848.git.me@ttaylorr.com","subject":"Re: [PATCH 07/10] t/t6500-gc.sh: add additional test cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T10:48:13Z","receivedAt":"2023-04-18T10:48:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2023 at 04:54:33PM -0400, Taylor Blau wrote:\n\n> In the last commit, we refactored some of the tests in t6500 to make\n> clearer when cruft packs will and won't be generated by `git gc`.\n> \n> Add the remaining cases not covered by the previous patch into this one,\n> which enumerates all possible combinations of arguments that will\n> produce (or not produce) a cruft pack.\n> \n> This prepares us for the following commit, which will change the default\n> of `gc.cruftPacks` by ensuring that we understand which invocations do\n> and do not change as a result.\n\nI think \"the following commit\" is really patch 9. Patch 8 adjusts t6501\n(and likewise says \"like the previous commits\"). It would probably make\nmore sense to put patch 8 before patches 6 and 7.\n\nProbably not worth a re-roll on its own, but if you're doing one anyway,\nit should be easy to do.\n\n-Peff\n"},{"id":"475609","messageId":"20230418105622.GA516715@coredump.intra.peff.net","threadId":"59612","inReplyTo":"4ccc525c39dd23db1136681bc0ffd572db0ed2f2.1681764848.git.me@ttaylorr.com","subject":"Re: [PATCH 08/10] t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T10:56:22Z","receivedAt":"2023-04-18T10:56:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2023 at 04:54:36PM -0400, Taylor Blau wrote:\n\n> We could run this test twice, once with `--cruft` and once with\n> `--no-cruft`, but doing so is unnecessary, since the object rescuing and\n> freshening behavior is already extensively tested via t5329.\n\nThat's doubtless true for the general case of freshening (after all,\nthat's the point of cruft packs). I do wonder about these \"broken links\"\ncases:\n\n> @@ -151,7 +151,7 @@ test_expect_success 'do not complain about existing broken links (commit)' '\n> \tsome message\n> \tEOF\n> \tcommit=$(git hash-object -t commit -w broken-commit) &&\n> -\tgit gc -q 2>stderr &&\n> +\tgit gc --no-cruft -q 2>stderr &&\n> \tverbose git cat-file -e $commit &&\n> \ttest_must_be_empty stderr\n> '\n\nThe idea is that we don't complain when repacking unreachable-but-broken\nsegments of history. Which could perhaps behave differently for objects\nthat are going into a cruft pack versus being turned loose. So maybe\nit's worth covering for the --cruft case, too. I dunno.\n\nCertainly your patch is not making the test coverage worse, but it might\nbe highlighting an existing blind-spot (and one that will become the\ndefault behavior in the next patch).\n\n-Peff\n"},{"id":"475610","messageId":"20230418110049.GB516715@coredump.intra.peff.net","threadId":"59612","inReplyTo":"bfda40a21ddb7abceb6a7eb9aa1f71aa5d856333.1681764848.git.me@ttaylorr.com","subject":"Re: [PATCH 09/10] builtin/gc.c: make `gc.cruftPacks` enabled by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T11:00:49Z","receivedAt":"2023-04-18T11:00:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2023 at 04:54:39PM -0400, Taylor Blau wrote:\n\n> for argv in \\\n> +\t\"gc\" \\\n> \t\"gc --cruft\" \\\n> \t\"-c gc.cruftPacks=true gc\" \\\n> -\t\"-c gc.cruftPacks=false gc --cruft\" \\\n> -\t\"-c feature.experimental=true gc\" \\\n> -\t\"-c gc.cruftPacks=true -c feature.experimental=false gc\"\n> +\t\"-c gc.cruftPacks=false gc --cruft\"\n> do\n\nOh good. I was a little sad to see the increase in the size of this loop\nin the earlier patches, so now reducing the number of combinations is a\nwelcome change.\n\nThe set you have here looks fine, though isn't \"gc --cruft\" redundant\nwith \"gc\" now?\n\n> @@ -246,9 +245,7 @@ done\n> for argv in \\\n> \t\"gc --no-cruft\" \\\n> \t\"-c gc.cruftPacks=false gc\" \\\n> -\t\"-c gc.cruftPacks=true gc --no-cruft\" \\\n> -\t\"-c feature.expiremental=true -c gc.cruftPacks=false gc\" \\\n> -\t\"-c feature.experimental=false gc\"\n> +\t\"-c gc.cruftPacks=true gc --no-cruft\"\n\nLikewise here, \"gc --no-cruft\" would have been redundant with \"gc\"\nbefore this patch, but we did not even bother with it (so no need to\nremove it here!).\n\nThe rest of the patch looks good, and I am quite on board with the\noverall goal. It's been a long time coming. :)\n\n-Peff\n"},{"id":"475611","messageId":"20230418110215.GC516715@coredump.intra.peff.net","threadId":"59612","inReplyTo":"fa15125454bb6596adbe68689555337a42cb80b7.1681764848.git.me@ttaylorr.com","subject":"Re: [PATCH 10/10] repository.h: drop unused `gc_cruft_packs`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T11:02:15Z","receivedAt":"2023-04-18T11:02:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2023 at 04:54:43PM -0400, Taylor Blau wrote:\n\n> As of the previous commit, all callers that need to read the value of\n> `gc.cruftPacks` do so outside without using the `repo_settings` struct,\n> making its `gc_cruft_packs` unused. Drop it accordingly.\n\nNice cleanup (I had to peek forward to understand the mystery\nrepo_settings hunk in the previous patch; it is a little weird to me\nthat this \"settings\" struct exists independent of the config, but that\nis not your doing).\n\n-Peff\n"},{"id":"475612","messageId":"20230418110424.GD516715@coredump.intra.peff.net","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"Re: [PATCH 00/10] gc: enable cruft packs by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T11:04:24Z","receivedAt":"2023-04-18T11:04:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2023 at 04:54:11PM -0400, Taylor Blau wrote:\n\n> The series is structured as follows:\n> \n> - The first two patches are cleanup.\n> - The third patches fixes an oversight where the code for `git gc`'s\n> `--keep-largest-pack` option would incorrectly consider cruft packs.\n> - The next five patches are test preparation.\n> - Then the substantive patch, to actually graduate `gc.cruftPacks` and\n> enable it by default.\n> - The final patch is some cleanup that can only take place towards the\n> end of the series.\n\nThanks, this was a pleasant read. Besides Junio's small fixup in the\ndocs, everything I pointed out was minor enough that it wouldn't be a\nbig deal to go in as-is.\n\n> Cruft packs have been a success for us at GitHub, and I am really\n> excited to get it in the hands of more users by default.\n\nMe too. :)\n\n-Peff\n"},{"id":"475621","messageId":"64e238fe-b69b-d670-6224-930b32bab9a5@github.com","threadId":"59612","inReplyTo":"20230418103909.GD508219@coredump.intra.peff.net","subject":"Re: [PATCH 03/10] builtin/gc.c: ignore cruft packs with `--keep-largest-pack`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-18T14:54:49Z","receivedAt":"2023-04-18T14:55:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/18/2023 6:39 AM, Jeff King wrote:\n> On Mon, Apr 17, 2023 at 07:03:08PM -0400, Taylor Blau wrote:\n\nI agree with the prior discussion that gc.bigPackThreshold is\ncurrently misbehaving and stopping it from caring about cruft packs\nis the best way to fix that behavior in this series.\n\n>> It is possible that in the future we could support writing multiple\n>> cruft packs (we already handle the presence of multiple cruft packs\n>> fine, just don't expose an easy way for the user to write >1 of them).\n>> And at that point we would be able to relax this patch a bit and allow\n>> `gc.bigPackThreshold` to cover cruft packs, too. But in the meantime,\n>> the benefit of avoiding loose object explosions outweighs the possible\n>> drawbacks here, IMHO.\n> \n> I wondered if that interface might be an option to say \"hey, I have a\n> gigantic cruft file I want to carry forward, please leave it alone\".\n> \n> But if you have a giant cruft pack that is making your \"git gc\" too\n> slow, it will eventually age out on its own. And if you're impatient,\n> then \"git gc --prune=now\" is probably the right tool.\n> \n> And If you really did want to keep rolling it forward for some reason,\n> then I'd think marking it with \".keep\" would be the best thing (and\n> maybe even dropping the mtimes file? I'm not sure a how a kept-cruft\n> pack does or should behave).\n\nGenerally, it's probably a good idea to (later) create a separate knob\nfor \"don't rewrite the objects in a 'big' cruft pack unless you need\nto\". For situations where cruft objects are being collected and not\nregularly pruned, this helps avoid repacking all unreachable objects\ninto a giant single pack, even though only a small number of objects\nwere discovered unreachable this time.\n\nThe important times where we'd want to consider a 'big' cruft pack\nfor rewrite are:\n\n 1. Some objects in the cruft pack are being pruned.\n 2. Some objects in the cruft pack need updated mtimes.\n\nHowever, in the typical case that we are adding new cruft objects\nand not changing the mtimes of existing unreachable objects, we could\ncreate a sensible limit on the size of a cruft pack to be rewritten\nduring normal maintenance.\n\nMy personal preference would be something between 2GB and 10GB, which\nseems like a decent balance between \"size of cruft pack\" and \"number of\ncruft packs\" for most repositories. Since none of the objects are\nreachable, we don't really care about them having good deltas for things\nlike fetches and clones. The benefit of reducing the time spent in 'git\nrepack --cruft' outweighs the slight disk space savings by having a\nsingle cruft pack, in my opinion.\n\nThanks,\n-Stolee\n"},{"id":"475654","messageId":"ZD7yOyC7axl6utEZ@nand.local","threadId":"59612","inReplyTo":"20230418103005.GC508219@coredump.intra.peff.net","subject":"Re: [PATCH 01/10] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:40:43Z","receivedAt":"2023-04-18T19:40:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 06:30:05AM -0400, Jeff King wrote:\n> On Mon, Apr 17, 2023 at 04:54:15PM -0400, Taylor Blau wrote:\n>\n> > The function `stage_tmp_packfiles()` generates a filename to use for\n> > staging the contents of what will become the pack's \".mtimes\" file.\n> >\n> > The name is generated in `write_mtimes_file()` and the result is\n> > returned back to `stage_tmp_packfiles()` which uses it to rename the\n> > temporary file into place via `rename_tmp_packfiles()`.\n> >\n> > `write_mtimes_file()` returns a `const char *`, indicating that callers\n> > are not expected to free its result (similar to, e.g., `oid_to_hex()`).\n> > But callers are expected to free its result, so this return type is\n> > incorrect.\n>\n> Makes sense. I do think in the long run that it might make sense for all\n> of these pack-write tmpfiles to tempfile.[ch] (either directly, or via\n> register_tempfile() after creating the file). And then it would be safe\n> to pass around the tempfile struct itself, which contains the filename,\n> without worrying so much about ownership issues.\n>\n> But that's a much bigger change that shouldn't be part of your series.\n\nAgreed on both fronts. I'll put investigating something like this on my\nlist of things to-do.\n\nThanks,\nTaylor\n"},{"id":"475655","messageId":"ZD7zKl3iDQc/TbWJ@nand.local","threadId":"59612","inReplyTo":"20230418104331.GE508219@coredump.intra.peff.net","subject":"Re: [PATCH 05/10] t/t9300-fast-import.sh: prepare for `gc --cruft` by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:44:42Z","receivedAt":"2023-04-18T19:44:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 06:43:31AM -0400, Jeff King wrote:\n> On Mon, Apr 17, 2023 at 04:54:27PM -0400, Taylor Blau wrote:\n>\n> > In a similar fashion as the previous commit, adjust the fast-import\n> > tests to prepare for \"git gc\" generating a cruft pack by default.\n> >\n> > This adjustment is slightly different, however. Instead of relying on us\n> > writing out the objects loose, and then calling `git prune` to remove\n> > them, t9300 needs to be prepared to drop objects that would be moved\n> > into cruft packs.\n> >\n> > To do this, we can combine the `git gc` invocation with `git prune` into\n> > one `git gc --prune`, which handles pruning both loose objects, and\n> > objects that would otherwise be written to a cruft pack.\n>\n> Good. This is more efficient anyway. It probably does not matter for our\n> tiny test repository, but it is always good for our tests to model the\n> best option when possible. :)\n\nI spent more time than I'd willingly admit second-guessing myself\nwondering if there was something I'm missing why we'd want to call `git\ngc` and `git prune` separately.\n\nAs best as I can tell, this pattern started all the way back in\n03db4525d3 (Support gitlinks in fast-import., 2008-07-19), which\nhappened after deprecating `git gc --prune` in 9e7d501990 (builtin-gc.c:\ndeprecate --prune, it now really has no effect, 2008-05-09).\n\nAfter `--prune` was un-deprecated in 58e9d9d472 (gc: make --prune useful\nagain by accepting an optional parameter, 2009-02-14), we got a handful\nof new uses in this script via 4cedb78cb5 (fast-import: add input format\ntests, 2011-08-11), which could have been `git gc --prune`, but weren't\n(likely taking after 03db4525d3).\n\nI don't know if any of that detail is interesting enough to rise to the\nlevel of needing to be in the patch itself, those were just my notes I\ntook while wandering through this part of the suite.\n\n> > t/t9300-fast-import.sh | 13 ++++---------\n> > 1 file changed, 4 insertions(+), 9 deletions(-)\n>\n> The patch itself looks good to me.\n\nThanks!\n\n-Taylor\n"},{"id":"475656","messageId":"ZD7z+rg2+fhq/AFP@nand.local","threadId":"59612","inReplyTo":"20230418104813.GF508219@coredump.intra.peff.net","subject":"Re: [PATCH 07/10] t/t6500-gc.sh: add additional test cases","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:48:10Z","receivedAt":"2023-04-18T19:48:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 06:48:13AM -0400, Jeff King wrote:\n> On Mon, Apr 17, 2023 at 04:54:33PM -0400, Taylor Blau wrote:\n>\n> > In the last commit, we refactored some of the tests in t6500 to make\n> > clearer when cruft packs will and won't be generated by `git gc`.\n> >\n> > Add the remaining cases not covered by the previous patch into this one,\n> > which enumerates all possible combinations of arguments that will\n> > produce (or not produce) a cruft pack.\n> >\n> > This prepares us for the following commit, which will change the default\n> > of `gc.cruftPacks` by ensuring that we understand which invocations do\n> > and do not change as a result.\n>\n> I think \"the following commit\" is really patch 9. Patch 8 adjusts t6501\n> (and likewise says \"like the previous commits\"). It would probably make\n> more sense to put patch 8 before patches 6 and 7.\n\nI'm glad you said something, because shortly after I sent this series I\nwas annoyed with myself for not organizing the preparatory patches in\norder of their test number.\n\nI reordered them like so:\n\n c02f80e1e2 t/t5304-prune.sh: prepare for `gc --cruft` by default\n 2e0fb1382b t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default\n c916e6d356 t/t6500-gc.sh: refactor cruft pack tests\n f9f7b2137b t/t6500-gc.sh: add additional test cases\n 5a74520b35 t/t9300-fast-import.sh: prepare for `gc --cruft` by default\n\nWhich feels much better to me.\n\n> Probably not worth a re-roll on its own, but if you're doing one anyway,\n> it should be easy to do.\n\nAgreed on both. I'm rerolling a few tiny things anyways, so I'll squash\nthis in, too.\n\nThanks,\nTaylor\n"},{"id":"475657","messageId":"ZD70iXDdAqeDIeXX@nand.local","threadId":"59612","inReplyTo":"20230418105622.GA516715@coredump.intra.peff.net","subject":"Re: [PATCH 08/10] t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:50:33Z","receivedAt":"2023-04-18T19:50:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 06:56:22AM -0400, Jeff King wrote:\n> On Mon, Apr 17, 2023 at 04:54:36PM -0400, Taylor Blau wrote:\n>\n> > We could run this test twice, once with `--cruft` and once with\n> > `--no-cruft`, but doing so is unnecessary, since the object rescuing and\n> > freshening behavior is already extensively tested via t5329.\n>\n> That's doubtless true for the general case of freshening (after all,\n> that's the point of cruft packs). I do wonder about these \"broken links\"\n> cases:\n>\n> > @@ -151,7 +151,7 @@ test_expect_success 'do not complain about existing broken links (commit)' '\n> > \tsome message\n> > \tEOF\n> > \tcommit=$(git hash-object -t commit -w broken-commit) &&\n> > -\tgit gc -q 2>stderr &&\n> > +\tgit gc --no-cruft -q 2>stderr &&\n> > \tverbose git cat-file -e $commit &&\n> > \ttest_must_be_empty stderr\n> > '\n>\n> The idea is that we don't complain when repacking unreachable-but-broken\n> segments of history. Which could perhaps behave differently for objects\n> that are going into a cruft pack versus being turned loose. So maybe\n> it's worth covering for the --cruft case, too. I dunno.\n\nI think we already have coverage of those cases in t5329, specifically\nin the tests:\n\n - cruft packs tolerate missing trees (expire $expire)\n - cruft packs tolerate missing blobs (expire $expire)\n\nwhich are tested for both the pruning and non-pruning implementations\n(by setting $expire to \"2.weeks.ago\", and \"never\", respectively).\n\nThanks,\nTaylor\n"},{"id":"475658","messageId":"ZD71Dxa+m7/p546d@nand.local","threadId":"59612","inReplyTo":"20230418110049.GB516715@coredump.intra.peff.net","subject":"Re: [PATCH 09/10] builtin/gc.c: make `gc.cruftPacks` enabled by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:52:47Z","receivedAt":"2023-04-18T19:52:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 07:00:49AM -0400, Jeff King wrote:\n> On Mon, Apr 17, 2023 at 04:54:39PM -0400, Taylor Blau wrote:\n>\n> > for argv in \\\n> > +\t\"gc\" \\\n> > \t\"gc --cruft\" \\\n> > \t\"-c gc.cruftPacks=true gc\" \\\n> > -\t\"-c gc.cruftPacks=false gc --cruft\" \\\n> > -\t\"-c feature.experimental=true gc\" \\\n> > -\t\"-c gc.cruftPacks=true -c feature.experimental=false gc\"\n> > +\t\"-c gc.cruftPacks=false gc --cruft\"\n> > do\n>\n> Oh good. I was a little sad to see the increase in the size of this loop\n> in the earlier patches, so now reducing the number of combinations is a\n> welcome change.\n\nSorry for the roller-coaster of emotions ;-).\n\n> The set you have here looks fine, though isn't \"gc --cruft\" redundant\n> with \"gc\" now?\n\nYeah, these are redundant. I figured that it might be good to test all\ncases, but I think this matrix-style of testing only gets you so far,\nand can often come out wasteful.\n\n\"gc --cruft\" isn't testing anything meaningful, so let's drop it.\n\n> > @@ -246,9 +245,7 @@ done\n> > for argv in \\\n> > \t\"gc --no-cruft\" \\\n> > \t\"-c gc.cruftPacks=false gc\" \\\n> > -\t\"-c gc.cruftPacks=true gc --no-cruft\" \\\n> > -\t\"-c feature.expiremental=true -c gc.cruftPacks=false gc\" \\\n> > -\t\"-c feature.experimental=false gc\"\n> > +\t\"-c gc.cruftPacks=true gc --no-cruft\"\n>\n> Likewise here, \"gc --no-cruft\" would have been redundant with \"gc\"\n> before this patch, but we did not even bother with it (so no need to\n> remove it here!).\n\nYeah, this one we definitely want to keep. We probably *could* have\ndropped it in the previous patch and brought it back here, but I think\nthat doing so would have been unnecessary churn for reviewers reading\nthis series.\n\n> The rest of the patch looks good, and I am quite on board with the\n> overall goal. It's been a long time coming. :)\n\nThanks, I agree ;-).\n\nThanks,\nTaylor\n"},{"id":"475659","messageId":"ZD71Sxh9vviM06T3@nand.local","threadId":"59612","inReplyTo":"20230418110424.GD516715@coredump.intra.peff.net","subject":"Re: [PATCH 00/10] gc: enable cruft packs by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T19:53:47Z","receivedAt":"2023-04-18T19:53:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 07:04:24AM -0400, Jeff King wrote:\n> On Mon, Apr 17, 2023 at 04:54:11PM -0400, Taylor Blau wrote:\n>\n> > The series is structured as follows:\n> >\n> > - The first two patches are cleanup.\n> > - The third patches fixes an oversight where the code for `git gc`'s\n> > `--keep-largest-pack` option would incorrectly consider cruft packs.\n> > - The next five patches are test preparation.\n> > - Then the substantive patch, to actually graduate `gc.cruftPacks` and\n> > enable it by default.\n> > - The final patch is some cleanup that can only take place towards the\n> > end of the series.\n>\n> Thanks, this was a pleasant read. Besides Junio's small fixup in the\n> docs, everything I pointed out was minor enough that it wouldn't be a\n> big deal to go in as-is.\n\nThanks, both, for the review. Most (all?) of the clean-up was all pretty\nminor, but I would have rerolled here regardless. New version incoming\njust as soon as CI says it's all good.\n\nThanks,\nTaylor\n"},{"id":"475660","messageId":"cover.1681850424.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681764848.git.me@ttaylorr.com","subject":"[PATCH v2 00/10] gc: enable cruft packs by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:40:29Z","receivedAt":"2023-04-18T20:40:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a very tiny reroll of my series to graduate `gc.cruftPacks` out\nof `feature.experimental` and enables it by default.\n\nA complete summary of the topic is available in the original cover\nletter[1], and the changes since last time are limited to test clean-up,\npatch reorganization, and some touch-ups on the patch messages\nthemselves.\n\nAs always, a range-diff is below for convenience.\n\nThanks for all of the review thus far, and in advance for any review on\nthis round, too.\n\n[1]: https://lore.kernel.org/git/cover.1681764848.git.me@ttaylorr.com/\n\nTaylor Blau (10):\n pack-write.c: plug a leak in stage_tmp_packfiles()\n builtin/repack.c: fix incorrect reference to '-C'\n builtin/gc.c: ignore cruft packs with `--keep-largest-pack`\n t/t5304-prune.sh: prepare for `gc --cruft` by default\n t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default\n t/t6500-gc.sh: refactor cruft pack tests\n t/t6500-gc.sh: add additional test cases\n t/t9300-fast-import.sh: prepare for `gc --cruft` by default\n builtin/gc.c: make `gc.cruftPacks` enabled by default\n repository.h: drop unused `gc_cruft_packs`\n\n Documentation/config/feature.txt | 3 -\n Documentation/config/gc.txt | 12 +--\n Documentation/git-gc.txt | 12 +--\n Documentation/gitformat-pack.txt | 4 +-\n builtin/gc.c | 8 +-\n builtin/repack.c | 2 +-\n pack-write.c | 14 ++--\n repo-settings.c | 4 +-\n repository.h | 1 -\n t/t5304-prune.sh | 28 +++----\n t/t6500-gc.sh | 135 ++++++++++++++++---------------\n t/t6501-freshen-objects.sh | 10 +--\n t/t9300-fast-import.sh | 13 +--\n 13 files changed, 120 insertions(+), 126 deletions(-)\n\nRange-diff against v1:\n 1: 65ac7ed9b8 = 1: c477b754e7 pack-write.c: plug a leak in stage_tmp_packfiles()\n 2: fbc8d15032 = 2: 52fb61fa9c builtin/repack.c: fix incorrect reference to '-C'\n 3: 796df920ad ! 3: 63b9ee8e2e builtin/gc.c: ignore cruft packs with `--keep-largest-pack`\n @@ Commit message\n - The same is true for `gc.bigPackThreshold`, if the size of the cruft\n pack exceeds the limit set by the caller.\n \n - Ignore cruft packs in the common implementation for both of these\n - options, and add a pair of tests to prevent any future regressions here.\n + In the future, it is possible that `gc.bigPackThreshold` could be used\n + to write a separate cruft pack containing any new unreachable objects\n + that entered the repository since the last time a cruft pack was\n + written.\n +\n + There are some complexities to doing so, mainly around handling\n + pruning objects that are in an existing cruft pack that is above the\n + threshold (which would either need to be rewritten, or else delay\n + pruning). Rewriting a substantially similar cruft pack isn't ideal, but\n + it is significantly better than the status-quo.\n +\n + If users have large cruft packs that they don't want to rewrite, they\n + can mark them as `*.keep` packs. But in general, if a repository has a\n + cruft pack that is so large it is slowing down GC's, it should probably\n + be pruned anyway.\n +\n + In the meantime, ignore cruft packs in the common implementation for\n + both of these options, and add a pair of tests to prevent any future\n + regressions here.\n \n Signed-off-by: Taylor Blau \n \n @@ Documentation/git-gc.txt: be performed as well.\n -\tAll packs except the largest pack and those marked with a\n -\t`.keep` files are consolidated into a single pack. When this\n -\toption is used, `gc.bigPackThreshold` is ignored.\n -+\tAll packs except the largest pack, any packs marked with a\n -+\t`.keep` file, and any cruft pack(s) are consolidated into a\n -+\tsingle pack. When this option is used, `gc.bigPackThreshold` is\n -+\tignored.\n ++\tAll packs except the largest non-cruft pack, any packs marked\n ++\twith a `.keep` file, and any cruft pack(s) are consolidated into\n ++\ta single pack. When this option is used, `gc.bigPackThreshold`\n ++\tis ignored.\n \n AGGRESSIVE\n ----------\n 4: 44006da959 = 4: 905eeb9027 t/t5304-prune.sh: prepare for `gc --cruft` by default\n 8: 4ccc525c39 ! 5: fa6eafb1fe t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default\n @@ Commit message\n cover the case of freshening loose objects not using cruft packs.\n \n We could run this test twice, once with `--cruft` and once with\n - `--no-cruft`, but doing so is unnecessary, since the object rescuing and\n - freshening behavior is already extensively tested via t5329.\n + `--no-cruft`, but doing so is unnecessary, since we already test object\n + rescuing, freshening, and dealing with corrupt parts of the unreachable\n + object graph extensively via t5329.\n \n Signed-off-by: Taylor Blau \n \n 6: 56a965e517 = 6: e6270d75fa t/t6500-gc.sh: refactor cruft pack tests\n 7: 6957e54f51 ! 7: 9db3fa9e36 t/t6500-gc.sh: add additional test cases\n @@ Commit message\n which enumerates all possible combinations of arguments that will\n produce (or not produce) a cruft pack.\n \n - This prepares us for the following commit, which will change the default\n + This prepares us for a future commit which will change the default value\n of `gc.cruftPacks` by ensuring that we understand which invocations do\n and do not change as a result.\n \n @@ t/t6500-gc.sh: do\n done\n \n for argv in \\\n -+\t\"gc --no-cruft\" \\\n ++\t\"gc\" \\\n +\t\"-c gc.cruftPacks=false gc\" \\\n +\t\"-c gc.cruftPacks=true gc --no-cruft\" \\\n \t\"-c feature.expiremental=true -c gc.cruftPacks=false gc\" \\\n 5: 1b07eb83fe ! 8: 894cf176ea t/t9300-fast-import.sh: prepare for `gc --cruft` by default\n @@ Metadata\n ## Commit message ##\n t/t9300-fast-import.sh: prepare for `gc --cruft` by default\n \n - In a similar fashion as the previous commit, adjust the fast-import\n - tests to prepare for \"git gc\" generating a cruft pack by default.\n + In a similar fashion as previous commits, adjust the fast-import tests\n + to prepare for \"git gc\" generating a cruft pack by default.\n \n This adjustment is slightly different, however. Instead of relying on us\n writing out the objects loose, and then calling `git prune` to remove\n @@ Commit message\n one `git gc --prune`, which handles pruning both loose objects, and\n objects that would otherwise be written to a cruft pack.\n \n + Likely this pattern of \"git gc && git prune\" started all the way back in\n + 03db4525d3 (Support gitlinks in fast-import., 2008-07-19), which\n + happened after deprecating `git gc --prune` in 9e7d501990 (builtin-gc.c:\n + deprecate --prune, it now really has no effect, 2008-05-09).\n +\n + After `--prune` was un-deprecated in 58e9d9d472 (gc: make --prune useful\n + again by accepting an optional parameter, 2009-02-14), this script got a\n + handful of new \"git gc && git prune\" instances via via 4cedb78cb5\n + (fast-import: add input format tests, 2011-08-11). These could have been\n + `git gc --prune`, but weren't (likely taking after 03db4525d3).\n +\n Signed-off-by: Taylor Blau \n \n ## t/t9300-fast-import.sh ##\n 9: bfda40a21d ! 9: b6784ddfe2 builtin/gc.c: make `gc.cruftPacks` enabled by default\n @@ t/t6500-gc.sh: assert_no_cruft_packs () {\n }\n \n for argv in \\\n +-\t\"gc --cruft\" \\\n +\t\"gc\" \\\n - \t\"gc --cruft\" \\\n \t\"-c gc.cruftPacks=true gc\" \\\n -\t\"-c gc.cruftPacks=false gc --cruft\" \\\n -\t\"-c feature.experimental=true gc\" \\\n @@ t/t6500-gc.sh: assert_no_cruft_packs () {\n do\n \ttest_expect_success \"git $argv generates a cruft pack\" '\n \t\ttest_when_finished \"rm -fr repo\" &&\n -@@ t/t6500-gc.sh: done\n +@@ t/t6500-gc.sh: do\n + done\n + \n for argv in \\\n - \t\"gc --no-cruft\" \\\n +-\t\"gc\" \\\n ++\t\"gc --no-cruft\" \\\n \t\"-c gc.cruftPacks=false gc\" \\\n -\t\"-c gc.cruftPacks=true gc --no-cruft\" \\\n -\t\"-c feature.expiremental=true -c gc.cruftPacks=false gc\" \\\n10: fa15125454 = 10: c67ee7c2ff repository.h: drop unused `gc_cruft_packs`\n-- \n2.40.0.362.gc67ee7c2ff\n"},{"id":"475661","messageId":"c477b754e7ddde0d6e696cfd4027ad88c18aeff3.1681850424.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681850424.git.me@ttaylorr.com","subject":"[PATCH v2 01/10] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:40:32Z","receivedAt":"2023-04-18T20:40:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function `stage_tmp_packfiles()` generates a filename to use for\nstaging the contents of what will become the pack's \".mtimes\" file.\n\nThe name is generated in `write_mtimes_file()` and the result is\nreturned back to `stage_tmp_packfiles()` which uses it to rename the\ntemporary file into place via `rename_tmp_packfiles()`.\n\n`write_mtimes_file()` returns a `const char *`, indicating that callers\nare not expected to free its result (similar to, e.g., `oid_to_hex()`).\nBut callers are expected to free its result, so this return type is\nincorrect.\n\nChange the function's signature to return a non-const `char *`, and free\nit at the end of `stage_tmp_packfiles()`.\n\nSigned-off-by: Taylor Blau \n---\n pack-write.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex f171405495..4da0ccc5f5 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -312,13 +312,13 @@ static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash)\n \thashwrite(f, hash, the_hash_algo->rawsz);\n }\n \n-static const char *write_mtimes_file(struct packing_data *to_pack,\n-\t\t\t\t struct pack_idx_entry **objects,\n-\t\t\t\t uint32_t nr_objects,\n-\t\t\t\t const unsigned char *hash)\n+static char *write_mtimes_file(struct packing_data *to_pack,\n+\t\t\t struct pack_idx_entry **objects,\n+\t\t\t uint32_t nr_objects,\n+\t\t\t const unsigned char *hash)\n {\n \tstruct strbuf tmp_file = STRBUF_INIT;\n-\tconst char *mtimes_name;\n+\tchar *mtimes_name;\n \tstruct hashfile *f;\n \tint fd;\n \n@@ -544,7 +544,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\t\t char **idx_tmp_name)\n {\n \tconst char *rev_tmp_name = NULL;\n-\tconst char *mtimes_tmp_name = NULL;\n+\tchar *mtimes_tmp_name = NULL;\n \n \tif (adjust_shared_perm(pack_tmp_name))\n \t\tdie_errno(\"unable to make temporary pack file readable\");\n@@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\trename_tmp_packfile(name_buffer, rev_tmp_name, \"rev\");\n \tif (mtimes_tmp_name)\n \t\trename_tmp_packfile(name_buffer, mtimes_tmp_name, \"mtimes\");\n+\n+\tfree(mtimes_tmp_name);\n }\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n-- \n2.40.0.362.gc67ee7c2ff\n\n"},{"id":"475662","messageId":"52fb61fa9c7398c7b412336b3ebe5724f5263108.1681850424.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681850424.git.me@ttaylorr.com","subject":"[PATCH v2 02/10] builtin/repack.c: fix incorrect reference to '-C'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:40:35Z","receivedAt":"2023-04-18T20:40:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When cruft packs were originally being developed, `-C` was designated as\nthe short-form for `--cruft` (as in `git repack -C`).\n\nThis was dropped due to confusion with Git's top-level `-C` option\nbefore submitting to the list. But the reference to it in\n`--cruft-expiration`'s help text was never updated. Fix that dangling\nreference in this patch.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex df4d8e0f0b..d9eee15c2f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -774,7 +774,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"same as -a, pack unreachable cruft objects separately\"),\n \t\t\t\t PACK_CRUFT),\n \t\tOPT_STRING(0, \"cruft-expiration\", &cruft_expiration, N_(\"approxidate\"),\n-\t\t\t\tN_(\"with -C, expire objects older than this\")),\n+\t\t\t\tN_(\"with --cruft, expire objects older than this\")),\n \t\tOPT_BOOL('d', NULL, &delete_redundant,\n \t\t\t\tN_(\"remove redundant packs, and run git-prune-packed\")),\n \t\tOPT_BOOL('f', NULL, &po_args.no_reuse_delta,\n-- \n2.40.0.362.gc67ee7c2ff\n\n"},{"id":"475663","messageId":"63b9ee8e2e398abb4a7cb435589b080dd26c266d.1681850424.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681850424.git.me@ttaylorr.com","subject":"[PATCH v2 03/10] builtin/gc.c: ignore cruft packs with `--keep-largest-pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:40:38Z","receivedAt":"2023-04-18T20:40:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When cruft packs were implemented, we never adjusted the code for `git\ngc`'s `--keep-largest-pack` and `gc.bigPackThreshold` to ignore cruft\npacks. This option and configuration option share a common\nimplementation, but including cruft packs is wrong in both cases:\n\n - Running `git gc --keep-largest-pack` in a repository where the\n largest pack is the cruft pack itself will make it impossible for\n `git gc` to prune objects, since the cruft pack itself is kept.\n\n - The same is true for `gc.bigPackThreshold`, if the size of the cruft\n pack exceeds the limit set by the caller.\n\nIn the future, it is possible that `gc.bigPackThreshold` could be used\nto write a separate cruft pack containing any new unreachable objects\nthat entered the repository since the last time a cruft pack was\nwritten.\n\nThere are some complexities to doing so, mainly around handling\npruning objects that are in an existing cruft pack that is above the\nthreshold (which would either need to be rewritten, or else delay\npruning). Rewriting a substantially similar cruft pack isn't ideal, but\nit is significantly better than the status-quo.\n\nIf users have large cruft packs that they don't want to rewrite, they\ncan mark them as `*.keep` packs. But in general, if a repository has a\ncruft pack that is so large it is slowing down GC's, it should probably\nbe pruned anyway.\n\nIn the meantime, ignore cruft packs in the common implementation for\nboth of these options, and add a pair of tests to prevent any future\nregressions here.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/gc.txt | 10 ++++-----\n Documentation/git-gc.txt | 7 +++---\n builtin/gc.c | 2 +-\n t/t6500-gc.sh | 43 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 53 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt\nindex 38fea076a2..8d5353e9e0 100644\n--- a/Documentation/config/gc.txt\n+++ b/Documentation/config/gc.txt\n@@ -43,11 +43,11 @@ gc.autoDetach::\n \tif the system supports it. Default is true.\n \n gc.bigPackThreshold::\n-\tIf non-zero, all packs larger than this limit are kept when\n-\t`git gc` is run. This is very similar to `--keep-largest-pack`\n-\texcept that all packs that meet the threshold are kept, not\n-\tjust the largest pack. Defaults to zero. Common unit suffixes of\n-\t'k', 'm', or 'g' are supported.\n+\tIf non-zero, all non-cruft packs larger than this limit are kept\n+\twhen `git gc` is run. This is very similar to\n+\t`--keep-largest-pack` except that all non-cruft packs that meet\n+\tthe threshold are kept, not just the largest pack. Defaults to\n+\tzero. Common unit suffixes of 'k', 'm', or 'g' are supported.\n +\n Note that if the number of kept packs is more than gc.autoPackLimit,\n this configuration variable is ignored, all packs except the base pack\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex a65c9aa62d..fef382a70f 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -77,9 +77,10 @@ be performed as well.\n \tinstance running on this repository.\n \n --keep-largest-pack::\n-\tAll packs except the largest pack and those marked with a\n-\t`.keep` files are consolidated into a single pack. When this\n-\toption is used, `gc.bigPackThreshold` is ignored.\n+\tAll packs except the largest non-cruft pack, any packs marked\n+\twith a `.keep` file, and any cruft pack(s) are consolidated into\n+\ta single pack. When this option is used, `gc.bigPackThreshold`\n+\tis ignored.\n \n AGGRESSIVE\n ----------\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex edd98d35a5..53ef137e1d 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -219,7 +219,7 @@ static struct packed_git *find_base_packs(struct string_list *packs,\n \tstruct packed_git *p, *base = NULL;\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n-\t\tif (!p->pack_local)\n+\t\tif (!p->pack_local || p->is_cruft)\n \t\t\tcontinue;\n \t\tif (limit) {\n \t\t\tif (p->pack_size >= limit)\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex d9acb63951..df6f2e6e52 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -298,6 +298,49 @@ test_expect_success 'feature.experimental=false avoids cruft packs by default' '\n \t)\n '\n \n+test_expect_success '--keep-largest-pack ignores cruft packs' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Generate a pack for reachable objects (of which there\n+\t\t# are 3), and one for unreachable objects (of which\n+\t\t# there are 6).\n+\t\tprepare_cruft_history &&\n+\t\tgit gc --cruft &&\n+\n+\t\tmtimes=\"$(find .git/objects/pack -type f -name \"pack-*.mtimes\")\" &&\n+\t\tsz=\"$(test_file_size \"${mtimes%.mtimes}.pack\")\" &&\n+\n+\t\t# Ensure that the cruft pack gets removed (due to\n+\t\t# `--prune=now`) despite it being the largest pack.\n+\t\tgit -c gc.bigPackThreshold=$sz gc --cruft --prune=now &&\n+\n+\t\tassert_no_cruft_packs\n+\t)\n+'\n+\n+test_expect_success 'gc.bigPackThreshold ignores cruft packs' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Generate a pack for reachable objects (of which there\n+\t\t# are 3), and one for unreachable objects (of which\n+\t\t# there are 6).\n+\t\tprepare_cruft_history &&\n+\t\tgit gc --cruft &&\n+\n+\t\t# Ensure that the cruft pack gets removed (due to\n+\t\t# `--prune=now`) despite it being the largest pack.\n+\t\tgit gc --cruft --prune=now --keep-largest-pack &&\n+\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.40.0.362.gc67ee7c2ff\n\n"},{"id":"475664","messageId":"905eeb90275178ac53adddb1abc17d149775dd60.1681850424.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681850424.git.me@ttaylorr.com","subject":"[PATCH v2 04/10] t/t5304-prune.sh: prepare for `gc --cruft` by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:40:41Z","receivedAt":"2023-04-18T20:40:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Many of the tests in t5304 run `git gc`, and rely on its behavior that\nunreachable-but-recent objects are written out loose. This is sensible,\nsince t5304 deals specifically with this kind of pruning.\n\nIf left unattended, however, this test would break when the default\nbehavior of a bare \"git gc\" is adjusted to generate a cruft pack by\ndefault.\n\nEnsure that these tests continue to work as-is (and continue to provide\ncoverage of loose object pruning) by passing `--no-cruft` explicitly.\n\nSigned-off-by: Taylor Blau \n---\n t/t5304-prune.sh | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 5500dd0842..662ae9b152 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -62,11 +62,11 @@ test_expect_success 'prune --expire' '\n test_expect_success 'gc: implicit prune --expire' '\n \tadd_blob &&\n \ttest-tool chmtime =-$((2*$week-30)) $BLOB_FILE &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \tverbose test $((1 + $before)) = $(git count-objects | sed \"s/ .*//\") &&\n \ttest_path_is_file $BLOB_FILE &&\n \ttest-tool chmtime =-$((2*$week+1)) $BLOB_FILE &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \tverbose test $before = $(git count-objects | sed \"s/ .*//\") &&\n \ttest_path_is_missing $BLOB_FILE\n '\n@@ -86,7 +86,7 @@ test_expect_success 'gc: refuse to start with invalid gc.pruneExpire' '\n \n test_expect_success 'gc: start with ok gc.pruneExpire' '\n \tgit config gc.pruneExpire 2.days.ago &&\n-\tgit gc\n+\tgit gc --no-cruft\n '\n \n test_expect_success 'prune: prune nonsense parameters' '\n@@ -137,44 +137,44 @@ test_expect_success 'gc --no-prune' '\n \tadd_blob &&\n \ttest-tool chmtime =-$((5001*$day)) $BLOB_FILE &&\n \tgit config gc.pruneExpire 2.days.ago &&\n-\tgit gc --no-prune &&\n+\tgit gc --no-prune --no-cruft &&\n \tverbose test 1 = $(git count-objects | sed \"s/ .*//\") &&\n \ttest_path_is_file $BLOB_FILE\n '\n \n test_expect_success 'gc respects gc.pruneExpire' '\n \tgit config gc.pruneExpire 5002.days.ago &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \ttest_path_is_file $BLOB_FILE &&\n \tgit config gc.pruneExpire 5000.days.ago &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \ttest_path_is_missing $BLOB_FILE\n '\n \n test_expect_success 'gc --prune=' '\n \tadd_blob &&\n \ttest-tool chmtime =-$((5001*$day)) $BLOB_FILE &&\n-\tgit gc --prune=5002.days.ago &&\n+\tgit gc --prune=5002.days.ago --no-cruft &&\n \ttest_path_is_file $BLOB_FILE &&\n-\tgit gc --prune=5000.days.ago &&\n+\tgit gc --prune=5000.days.ago --no-cruft &&\n \ttest_path_is_missing $BLOB_FILE\n '\n \n test_expect_success 'gc --prune=never' '\n \tadd_blob &&\n-\tgit gc --prune=never &&\n+\tgit gc --prune=never --no-cruft &&\n \ttest_path_is_file $BLOB_FILE &&\n-\tgit gc --prune=now &&\n+\tgit gc --prune=now --no-cruft &&\n \ttest_path_is_missing $BLOB_FILE\n '\n \n test_expect_success 'gc respects gc.pruneExpire=never' '\n \tgit config gc.pruneExpire never &&\n \tadd_blob &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \ttest_path_is_file $BLOB_FILE &&\n \tgit config gc.pruneExpire now &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \ttest_path_is_missing $BLOB_FILE\n '\n \n@@ -194,7 +194,7 @@ test_expect_success 'gc: prune old objects after local clone' '\n \t\tcd aclone &&\n \t\tverbose test 1 = $(git count-objects | sed \"s/ .*//\") &&\n \t\ttest_path_is_file $BLOB_FILE &&\n-\t\tgit gc --prune &&\n+\t\tgit gc --prune --no-cruft &&\n \t\tverbose test 0 = $(git count-objects | sed \"s/ .*//\") &&\n \t\ttest_path_is_missing $BLOB_FILE\n \t)\n@@ -237,7 +237,7 @@ test_expect_success 'clean pack garbage with gc' '\n \t>.git/objects/pack/fake2.keep &&\n \t>.git/objects/pack/fake2.idx &&\n \t>.git/objects/pack/fake3.keep &&\n-\tgit gc &&\n+\tgit gc --no-cruft &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n \tcat >expected <<\\EOF &&\n-- \n2.40.0.362.gc67ee7c2ff\n\n"},{"id":"475665","messageId":"fa6eafb1fecc0956f846487af0ad0bd243c0ff3f.1681850424.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681850424.git.me@ttaylorr.com","subject":"[PATCH v2 05/10] t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:40:43Z","receivedAt":"2023-04-18T20:40:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, prepare for `gc --cruft`\nbecoming the default by ensuring that the tests in t6501 explicitly\ncover the case of freshening loose objects not using cruft packs.\n\nWe could run this test twice, once with `--cruft` and once with\n`--no-cruft`, but doing so is unnecessary, since we already test object\nrescuing, freshening, and dealing with corrupt parts of the unreachable\nobject graph extensively via t5329.\n\nSigned-off-by: Taylor Blau \n---\n t/t6501-freshen-objects.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh\nindex 3968b47ed5..dbfa8a4d4c 100755\n--- a/t/t6501-freshen-objects.sh\n+++ b/t/t6501-freshen-objects.sh\n@@ -101,7 +101,7 @@ do\n \t'\n \n \ttest_expect_success \"simultaneous gc ($title)\" '\n-\t\tgit gc --prune=12.hours.ago\n+\t\tgit gc --no-cruft --prune=12.hours.ago\n \t'\n \n \ttest_expect_success \"finish writing out commit ($title)\" '\n@@ -131,7 +131,7 @@ do\n \t'\n \n \ttest_expect_success \"simultaneous gc ($title)\" '\n-\t\tgit gc --prune=12.hours.ago\n+\t\tgit gc --no-cruft --prune=12.hours.ago\n \t'\n \n \t# tree should have been refreshed by write-tree\n@@ -151,7 +151,7 @@ test_expect_success 'do not complain about existing broken links (commit)' '\n \tsome message\n \tEOF\n \tcommit=$(git hash-object -t commit -w broken-commit) &&\n-\tgit gc -q 2>stderr &&\n+\tgit gc --no-cruft -q 2>stderr &&\n \tverbose git cat-file -e $commit &&\n \ttest_must_be_empty stderr\n '\n@@ -161,7 +161,7 @@ test_expect_success 'do not complain about existing broken links (tree)' '\n \t100644 blob $(test_oid 003)\tfoo\n \tEOF\n \ttree=$(git mktree --missing stderr &&\n+\tgit gc --no-cruft -q 2>stderr &&\n \tgit cat-file -e $tree &&\n \ttest_must_be_empty stderr\n '\n@@ -176,7 +176,7 @@ test_expect_success 'do not complain about existing broken links (tag)' '\n \tthis is a broken tag\n \tEOF\n \ttag=$(git hash-object -t tag -w broken-tag) &&\n-\tgit gc -q 2>stderr &&\n+\tgit gc --no-cruft -q 2>stderr &&\n \tgit cat-file -e $tag &&\n \ttest_must_be_empty stderr\n '\n-- \n2.40.0.362.gc67ee7c2ff\n\n"},{"id":"475666","messageId":"e6270d75fa1112a7ed92a87f3d0cf24187292d10.1681850424.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681850424.git.me@ttaylorr.com","subject":"[PATCH v2 06/10] t/t6500-gc.sh: refactor cruft pack tests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:40:46Z","receivedAt":"2023-04-18T20:41:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 12253ab6d0 (gc: add tests for --cruft and friends, 2022-10-26), we\nadded a handful of tests to t6500 to ensure that `git gc` respected the\nvalue of `--cruft` and `gc.cruftPacks`.\n\nThen, in c695592850 (config: let feature.experimental imply\ngc.cruftPacks=true, 2022-10-26), another set of similar tests was added\nto ensure that `feature.experimental` correctly implied enabling cruft\npack generation (or not).\n\nThese tests are similar and could be consolidated. Do so in this patch\nto prepare for expanding the set of command-line invocations that enable\nor disable writing cruft packs. This makes it possible to easily test\nmore combinations of arguments without being overly repetitive.\n\nSigned-off-by: Taylor Blau \n---\n t/t6500-gc.sh | 126 ++++++++++++++++++--------------------------------\n 1 file changed, 44 insertions(+), 82 deletions(-)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex df6f2e6e52..a2f988c5c2 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -210,93 +210,55 @@ prepare_cruft_history () {\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 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(\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-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+for argv in \\\n+\t\"gc --cruft\" \\\n+\t\"-c gc.cruftPacks=true gc\" \\\n+\t\"-c feature.experimental=true gc\" \\\n+\t\"-c gc.cruftPacks=true -c feature.experimental=false gc\"\n+do\n+\ttest_expect_success \"git $argv generates a cruft pack\" '\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\tgit init repo &&\n+\t\t(\n+\t\t\tcd repo &&\n+\n+\t\t\tprepare_cruft_history &&\n+\t\t\tgit $argv &&\n+\n+\t\t\tfind .git/objects/pack -name \"*.mtimes\" >mtimes &&\n+\t\t\tsed -e 's/\\.mtimes$/\\.pack/g' mtimes >packs &&\n+\n+\t\t\ttest_file_not_empty packs &&\n+\t\t\twhile read pack\n+\t\t\tdo\n+\t\t\t\ttest_path_is_file \"$pack\" || return 1\n+\t\t\tdone \n---\n t/t6500-gc.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex a2f988c5c2..3ba2ae5140 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -218,6 +218,7 @@ assert_no_cruft_packs () {\n for argv in \\\n \t\"gc --cruft\" \\\n \t\"-c gc.cruftPacks=true gc\" \\\n+\t\"-c gc.cruftPacks=false gc --cruft\" \\\n \t\"-c feature.experimental=true gc\" \\\n \t\"-c gc.cruftPacks=true -c feature.experimental=false gc\"\n do\n@@ -243,6 +244,9 @@ do\n done\n \n for argv in \\\n+\t\"gc\" \\\n+\t\"-c gc.cruftPacks=false gc\" \\\n+\t\"-c gc.cruftPacks=true gc --no-cruft\" \\\n \t\"-c feature.expiremental=true -c gc.cruftPacks=false gc\" \\\n \t\"-c feature.experimental=false gc\"\n do\n-- \n2.40.0.362.gc67ee7c2ff\n\n"},{"id":"475668","messageId":"894cf176ea8911e615cb59857b93a2765ca6ba9d.1681850424.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681850424.git.me@ttaylorr.com","subject":"[PATCH v2 08/10] t/t9300-fast-import.sh: prepare for `gc --cruft` by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:40:52Z","receivedAt":"2023-04-18T20:41:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar fashion as previous commits, adjust the fast-import tests\nto prepare for \"git gc\" generating a cruft pack by default.\n\nThis adjustment is slightly different, however. Instead of relying on us\nwriting out the objects loose, and then calling `git prune` to remove\nthem, t9300 needs to be prepared to drop objects that would be moved\ninto cruft packs.\n\nTo do this, we can combine the `git gc` invocation with `git prune` into\none `git gc --prune`, which handles pruning both loose objects, and\nobjects that would otherwise be written to a cruft pack.\n\nLikely this pattern of \"git gc && git prune\" started all the way back in\n03db4525d3 (Support gitlinks in fast-import., 2008-07-19), which\nhappened after deprecating `git gc --prune` in 9e7d501990 (builtin-gc.c:\ndeprecate --prune, it now really has no effect, 2008-05-09).\n\nAfter `--prune` was un-deprecated in 58e9d9d472 (gc: make --prune useful\nagain by accepting an optional parameter, 2009-02-14), this script got a\nhandful of new \"git gc && git prune\" instances via via 4cedb78cb5\n(fast-import: add input format tests, 2011-08-11). These could have been\n`git gc --prune`, but weren't (likely taking after 03db4525d3).\n\nSigned-off-by: Taylor Blau \n---\n t/t9300-fast-import.sh | 13 ++++---------\n 1 file changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex aa55b41b9a..ac237a1f90 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -388,9 +388,7 @@ test_expect_success 'B: accept branch name \"TEMP_TAG\"' '\n \n \tINPUT_END\n \n-\ttest_when_finished \"rm -f .git/TEMP_TAG\n-\t\tgit gc\n-\t\tgit prune\" &&\n+\ttest_when_finished \"rm -f .git/TEMP_TAG && git gc --prune=now\" &&\n \tgit fast-import `.\n\nInstead of keeping track of those unreachable yet recent objects via\ntheir loose object file's mtime, cruft packs collect all unreachable\nobjects into a single pack with a corresponding `*.mtimes` file that\nacts as a table to store the mtimes of all unreachable objects. This\nprevents the need to store unreachable objects as loose as they age out\nof the repository, and avoids the problem of loose object explosions.\n\nBeyond avoiding loose object explosions, cruft packs also act as a more\nefficient mechanism to store unreachable objects as they age out of a\nrepository. This is because pairs of similar unreachable objects serve\nas delta bases for one another.\n\nIn 5b92477f89, the feature was introduced as experimental. Since then,\nGitHub has been running these patches in every repository generating\nhundreds of millions of cruft packs along the way. The feature is\nbattle-tested, and avoids many pathological cases such as above. Users\nwho either run `git gc` manually, or via `git maintenance` can benefit\nfrom having cruft packs.\n\nAs such, enable cruft pack generation to take place by default (by\nmaking `gc.cruftPacks` have the default of \"true\" rather than \"false).\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/feature.txt | 3 ---\n Documentation/config/gc.txt | 2 +-\n Documentation/git-gc.txt | 5 +++--\n Documentation/gitformat-pack.txt | 4 ++--\n builtin/gc.c | 6 +-----\n t/t6500-gc.sh | 12 ++++--------\n 6 files changed, 11 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex e52bc6b858..17b4d39f89 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -14,9 +14,6 @@ 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/Documentation/config/gc.txt b/Documentation/config/gc.txt\nindex 8d5353e9e0..7f95c866e1 100644\n--- a/Documentation/config/gc.txt\n+++ b/Documentation/config/gc.txt\n@@ -84,7 +84,7 @@ gc.packRefs::\n gc.cruftPacks::\n \tStore unreachable objects in a cruft pack (see\n \tlinkgit:git-repack[1]) instead of as loose objects. The default\n-\tis `false`.\n+\tis `true`.\n \n gc.pruneExpire::\n \tWhen 'git gc' is run, it will call 'prune --expire 2.weeks.ago'\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex fef382a70f..90806fd26a 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -54,9 +54,10 @@ other housekeeping tasks (e.g. rerere, working trees, reflog...) will\n be performed as well.\n \n \n---cruft::\n+--[no-]cruft::\n \tWhen expiring unreachable objects, pack them separately into a\n-\tcruft pack instead of storing them as loose objects.\n+\tcruft pack instead of storing them as loose objects. `--cruft`\n+\tis on by default.\n \n --prune=::\n \tPrune loose objects older than date (default is 2 weeks ago,\ndiff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\nindex e06af02f21..0c1be2dbe8 100644\n--- a/Documentation/gitformat-pack.txt\n+++ b/Documentation/gitformat-pack.txt\n@@ -611,8 +611,8 @@ result of repeatedly resetting the objects' mtimes to the present time.\n \n If you are GC-ing repositories in a mixed version environment, consider omitting\n the `--cruft` option when using linkgit:git-repack[1] and linkgit:git-gc[1], and\n-leaving the `gc.cruftPacks` configuration unset until all writers understand\n-cruft packs.\n+setting the `gc.cruftPacks` configuration to \"false\" until all writers\n+understand cruft packs.\n \n === Alternatives\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 53ef137e1d..ece01e966f 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -48,7 +48,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 = -1;\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@@ -608,10 +608,6 @@ 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)\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 3ba2ae5140..69509d0c11 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -216,11 +216,9 @@ assert_no_cruft_packs () {\n }\n \n for argv in \\\n-\t\"gc --cruft\" \\\n+\t\"gc\" \\\n \t\"-c gc.cruftPacks=true gc\" \\\n-\t\"-c gc.cruftPacks=false gc --cruft\" \\\n-\t\"-c feature.experimental=true gc\" \\\n-\t\"-c gc.cruftPacks=true -c feature.experimental=false gc\"\n+\t\"-c gc.cruftPacks=false gc --cruft\"\n do\n \ttest_expect_success \"git $argv generates a cruft pack\" '\n \t\ttest_when_finished \"rm -fr repo\" &&\n@@ -244,11 +242,9 @@ do\n done\n \n for argv in \\\n-\t\"gc\" \\\n+\t\"gc --no-cruft\" \\\n \t\"-c gc.cruftPacks=false gc\" \\\n-\t\"-c gc.cruftPacks=true gc --no-cruft\" \\\n-\t\"-c feature.expiremental=true -c gc.cruftPacks=false gc\" \\\n-\t\"-c feature.experimental=false gc\"\n+\t\"-c gc.cruftPacks=true gc --no-cruft\"\n do\n \ttest_expect_success \"git $argv does not generate a cruft pack\" '\n \t\ttest_when_finished \"rm -fr repo\" &&\n-- \n2.40.0.362.gc67ee7c2ff\n\n"},{"id":"475670","messageId":"c67ee7c2ff9456eda1c5468cf68ddb1b83d37c85.1681850424.git.me@ttaylorr.com","threadId":"59612","inReplyTo":"cover.1681850424.git.me@ttaylorr.com","subject":"[PATCH v2 10/10] repository.h: drop unused `gc_cruft_packs`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T20:41:00Z","receivedAt":"2023-04-18T20:41:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of the previous commit, all callers that need to read the value of\n`gc.cruftPacks` do so outside without using the `repo_settings` struct,\nmaking its `gc_cruft_packs` unused. Drop it accordingly.\n\nSigned-off-by: Taylor Blau \n---\n repo-settings.c | 4 +---\n repository.h | 1 -\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0a6c0b381f..a8d0b98794 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -41,10 +41,8 @@ void prepare_repo_settings(struct repository *r)\n \trepo_cfg_bool(r, \"feature.experimental\", &experimental, 0);\n \n \t/* Defaults modified by feature.* */\n-\tif (experimental) {\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 \t\tr->settings.index_skip_hash = 1;\ndiff --git a/repository.h b/repository.h\nindex 15a8afc5fb..50eb0ce391 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -33,7 +33,6 @@ 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-- \n2.40.0.362.gc67ee7c2ff\n"},{"id":"475716","messageId":"xmqqpm7z7crz.fsf@gitster.g","threadId":"59612","inReplyTo":"c477b754e7ddde0d6e696cfd4027ad88c18aeff3.1681850424.git.me@ttaylorr.com","subject":"Re: [PATCH v2 01/10] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-19T22:00:48Z","receivedAt":"2023-04-19T22:00:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> The function `stage_tmp_packfiles()` generates a filename to use for\n> staging the contents of what will become the pack's \".mtimes\" file.\n>\n> The name is generated in `write_mtimes_file()` and the result is\n> returned back to `stage_tmp_packfiles()` which uses it to rename the\n> temporary file into place via `rename_tmp_packfiles()`.\n>\n> `write_mtimes_file()` returns a `const char *`, indicating that callers\n> are not expected to free its result (similar to, e.g., `oid_to_hex()`).\n> But callers are expected to free its result, so this return type is\n> incorrect.\n\nIndeed the string that holds the name of the file returned by\nwrite_mtimes_file() is leaking. Does the same logic apply to the\nreturned filename from write_rev_file() and stored in rev_tmp_name\nthat is not freed in stage_tmp_packfiles() in another topic?\n\n> @@ -544,7 +544,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n> \t\t\t char **idx_tmp_name)\n> {\n> \tconst char *rev_tmp_name = NULL;\n> -\tconst char *mtimes_tmp_name = NULL;\n> +\tchar *mtimes_tmp_name = NULL;\n> \n> \tif (adjust_shared_perm(pack_tmp_name))\n> \t\tdie_errno(\"unable to make temporary pack file readable\");\n> @@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n> \t\trename_tmp_packfile(name_buffer, rev_tmp_name, \"rev\");\n> \tif (mtimes_tmp_name)\n> \t\trename_tmp_packfile(name_buffer, mtimes_tmp_name, \"mtimes\");\n> +\n> +\tfree(mtimes_tmp_name);\n> }\n> \n> void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n"},{"id":"475717","messageId":"xmqqh6tb7bsa.fsf@gitster.g","threadId":"59612","inReplyTo":"b6784ddfe2906f7c04b3050bd9ba63a884ddb047.1681850424.git.me@ttaylorr.com","subject":"Re: [PATCH v2 09/10] builtin/gc.c: make `gc.cruftPacks` enabled by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-19T22:22:13Z","receivedAt":"2023-04-19T22:25:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\n> index e52bc6b858..17b4d39f89 100644\n> --- a/Documentation/config/feature.txt\n> +++ b/Documentation/config/feature.txt\n> @@ -14,9 +14,6 @@ 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\nBeing listed here is a definite sign that a feature behind a\nconfiguration variable is considered experimental. Do we have (and\nif not, do we want to establish) a procedure to mark and announce a\nfeature that used to be experimental no longer is? If it is enough\nto mention it in the release notes, then I can take care of it, of\ncourse.\n"},{"id":"475718","messageId":"xmqqlein7bwt.fsf@gitster.g","threadId":"59612","inReplyTo":"c67ee7c2ff9456eda1c5468cf68ddb1b83d37c85.1681850424.git.me@ttaylorr.com","subject":"Re: [PATCH v2 10/10] repository.h: drop unused `gc_cruft_packs`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-19T22:19:30Z","receivedAt":"2023-04-19T22:28:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> As of the previous commit, all callers that need to read the value of\n> `gc.cruftPacks` do so outside without using the `repo_settings` struct,\n> making its `gc_cruft_packs` unused. Drop it accordingly.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> repo-settings.c | 4 +---\n> repository.h | 1 -\n> 2 files changed, 1 insertion(+), 4 deletions(-)\n\nThis is in line with the current design of repo-settings, and it is\na total tangent to this topic, but in retrospect, if we structured\nhow \"what's experimental\" is determined in a more distributed way,\nwe didn't have to have this patch. That is, instead of saying \"ok\nwe are in a repository with experimental setting turned on, and the\nset of features to be enabled are centrally described in this\nsettings structure\", each opt-in features (like the code that reads\n\"gc.cruftPacks\" by calling git_config_get_bool() to override its\ndefault value) can select their default/fallback values by seeing if\nthe experimental bit is set.\n\nIn any case, this patch looks good and we have one fewer such\ncentrally managed bit.\n"},{"id":"475741","messageId":"ZEFo4iGFTg1UWpL0@nand.local","threadId":"59612","inReplyTo":"xmqqpm7z7crz.fsf@gitster.g","subject":"Re: [PATCH v2 01/10] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-20T16:31:30Z","receivedAt":"2023-04-20T16:31:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 19, 2023 at 03:00:48PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > The function `stage_tmp_packfiles()` generates a filename to use for\n> > staging the contents of what will become the pack's \".mtimes\" file.\n> >\n> > The name is generated in `write_mtimes_file()` and the result is\n> > returned back to `stage_tmp_packfiles()` which uses it to rename the\n> > temporary file into place via `rename_tmp_packfiles()`.\n> >\n> > `write_mtimes_file()` returns a `const char *`, indicating that callers\n> > are not expected to free its result (similar to, e.g., `oid_to_hex()`).\n> > But callers are expected to free its result, so this return type is\n> > incorrect.\n>\n> Indeed the string that holds the name of the file returned by\n> write_mtimes_file() is leaking. Does the same logic apply to the\n> returned filename from write_rev_file() and stored in rev_tmp_name\n> that is not freed in stage_tmp_packfiles() in another topic?\n\nThey are similar, but unfortunately different.\n\nHere, our temporary name is generated by `write_mtimes_file()` which\n*always* comes up with a new name from scratch. So we know that it\nshould always be free()'d at the end of `stage_tmp_packfiles()`.\n\nBut in the case of `write_rev_file()`, it only *sometimes* generates a\nnew name from scratch. The first parameter can be NULL, in which case\n`write_rev_file()` will generate a new name. Or it can be non-NULL, in\nwhich case that name will be used instead.\n\nSo in that topic, it's less clear on what the ultimate right path\nforward is. But in this topic, changing `mtimes_tmp_name` and the return\ntype of `write_mtimes_file()` to be a non-const `char *` (and free()ing\nit, of course ;-)) is the right thing to do.\n\nThanks,\nTaylor\n"},{"id":"475743","messageId":"xmqqh6ta1og5.fsf@gitster.g","threadId":"59612","inReplyTo":"ZEFo4iGFTg1UWpL0@nand.local","subject":"Re: [PATCH v2 01/10] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T16:57:30Z","receivedAt":"2023-04-20T16:59:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> Indeed the string that holds the name of the file returned by\n>> write_mtimes_file() is leaking. Does the same logic apply to the\n>> returned filename from write_rev_file() and stored in rev_tmp_name\n>> that is not freed in stage_tmp_packfiles() in another topic?\n>\n> They are similar, but unfortunately different.\n\nI hoped \"fortunately different and there is no leak\", but it seems\nwhat you said below is a bit different X-<.\n\n> But in the case of `write_rev_file()`, it only *sometimes* generates a\n> new name from scratch. The first parameter can be NULL, in which case\n> `write_rev_file()` will generate a new name. Or it can be non-NULL, in\n> which case that name will be used instead.\n\nIOW, it is a mess.\n\nBut I think the topic to introduce that variable is being rerolled\nand has no interaction with the codepath we are discussing here, so\nwe are still in good shape wrt this series ;-)\n\nThanks.\n"},{"id":"475747","messageId":"ZEF1MSwtSl7KdjMi@nand.local","threadId":"59612","inReplyTo":"xmqqh6tb7bsa.fsf@gitster.g","subject":"Re: [PATCH v2 09/10] builtin/gc.c: make `gc.cruftPacks` enabled by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-20T17:24:01Z","receivedAt":"2023-04-20T17:24:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 19, 2023 at 03:22:13PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\n> > index e52bc6b858..17b4d39f89 100644\n> > --- a/Documentation/config/feature.txt\n> > +++ b/Documentation/config/feature.txt\n> > @@ -14,9 +14,6 @@ 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> Being listed here is a definite sign that a feature behind a\n> configuration variable is considered experimental. Do we have (and\n> if not, do we want to establish) a procedure to mark and announce a\n> feature that used to be experimental no longer is? If it is enough\n> to mention it in the release notes, then I can take care of it, of\n> course.\n\nI am not aware of such a procedure. But personally I think it would be\nfine to mention it in the release notes for the next release.\n\nThanks,\nTaylor\n"},{"id":"475749","messageId":"xmqq5y9q1mwb.fsf@gitster.g","threadId":"59612","inReplyTo":"ZEF1MSwtSl7KdjMi@nand.local","subject":"Re: [PATCH v2 09/10] builtin/gc.c: make `gc.cruftPacks` enabled by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T17:31:00Z","receivedAt":"2023-04-20T17:31:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> Being listed here is a definite sign that a feature behind a\n>> configuration variable is considered experimental. Do we have (and\n>> if not, do we want to establish) a procedure to mark and announce a\n>> feature that used to be experimental no longer is? If it is enough\n>> to mention it in the release notes, then I can take care of it, of\n>> course.\n>\n> I am not aware of such a procedure. But personally I think it would be\n> fine to mention it in the release notes for the next release.\n\nOK. I updated the entry for the topic in the draft \"What's cooking\"\nreport to read:\n\n * tb/enable-cruft-packs-by-default (2023-04-18) 10 commits\n - ...\n\n When \"gc\" needs to retain unreachable objects, packing them into\n cruft packs (instead of exploding them into loose object files) has\n been offered as a more efficient option for some time. Now the use\n of cruft packs has been made the default and no longer considered\n an experimental feature.\n\n Will merge to 'next'.\n source: \n\nand the per-topic description text is usually what is copied\nverbatim to the release notes, so even if I forget, the procedure\nwill remember it for us ;-)\n\nThanks.\n"},{"id":"475754","messageId":"ZEGQVcblWdmdN+9j@nand.local","threadId":"59612","inReplyTo":"xmqq5y9q1mwb.fsf@gitster.g","subject":"Re: [PATCH v2 09/10] builtin/gc.c: make `gc.cruftPacks` enabled by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-20T19:19:49Z","receivedAt":"2023-04-20T19:20:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 20, 2023 at 10:31:00AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> >> Being listed here is a definite sign that a feature behind a\n> >> configuration variable is considered experimental. Do we have (and\n> >> if not, do we want to establish) a procedure to mark and announce a\n> >> feature that used to be experimental no longer is? If it is enough\n> >> to mention it in the release notes, then I can take care of it, of\n> >> course.\n> >\n> > I am not aware of such a procedure. But personally I think it would be\n> > fine to mention it in the release notes for the next release.\n>\n> OK. I updated the entry for the topic in the draft \"What's cooking\"\n> report to read:\n>\n> * tb/enable-cruft-packs-by-default (2023-04-18) 10 commits\n> - ...\n>\n> When \"gc\" needs to retain unreachable objects, packing them into\n> cruft packs (instead of exploding them into loose object files) has\n> been offered as a more efficient option for some time. Now the use\n> of cruft packs has been made the default and no longer considered\n> an experimental feature.\n>\n> Will merge to 'next'.\n> source: \n\nThanks, I think that summarizes/announces the change well. I always\nappreciate your effort into concisely summarizing the topics being\nqueued.\n\n> and the per-topic description text is usually what is copied\n> verbatim to the release notes, so even if I forget, the procedure\n> will remember it for us ;-)\n\n;-).\n\nThanks,\nTaylor\n"},{"id":"475833","messageId":"20230422112333.GF2969939@coredump.intra.peff.net","threadId":"59612","inReplyTo":"ZD70iXDdAqeDIeXX@nand.local","subject":"Re: [PATCH 08/10] t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-22T11:23:33Z","receivedAt":"2023-04-22T11:23:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2023 at 03:50:33PM -0400, Taylor Blau wrote:\n\n> > > @@ -151,7 +151,7 @@ test_expect_success 'do not complain about existing broken links (commit)' '\n> > > \tsome message\n> > > \tEOF\n> > > \tcommit=$(git hash-object -t commit -w broken-commit) &&\n> > > -\tgit gc -q 2>stderr &&\n> > > +\tgit gc --no-cruft -q 2>stderr &&\n> > > \tverbose git cat-file -e $commit &&\n> > > \ttest_must_be_empty stderr\n> > > '\n> >\n> > The idea is that we don't complain when repacking unreachable-but-broken\n> > segments of history. Which could perhaps behave differently for objects\n> > that are going into a cruft pack versus being turned loose. So maybe\n> > it's worth covering for the --cruft case, too. I dunno.\n> \n> I think we already have coverage of those cases in t5329, specifically\n> in the tests:\n> \n> - cruft packs tolerate missing trees (expire $expire)\n> - cruft packs tolerate missing blobs (expire $expire)\n> \n> which are tested for both the pruning and non-pruning implementations\n> (by setting $expire to \"2.weeks.ago\", and \"never\", respectively).\n\nAh, perfect. I only gave a quick look over t5329, and didn't see those.\nSo yeah, I think it is fine to just have this whole script run in\nno-cruft mode.\n\n-Peff\n"}]}