{"thread":{"id":"60206","subject":"[PATCH 0/2] repack: implement `--cruft-max-size`","startedAt":"2023-09-07T21:52:03Z","lastAt":"2023-10-09T17:28:25Z","messageCount":21,"participants":["Taylor Blau","Junio C Hamano","Eric Sunshine","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"481526","messageId":"cover.1694123506.git.me@ttaylorr.com","threadId":"60206","inReplyTo":null,"subject":"[PATCH 0/2] repack: implement `--cruft-max-size`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-07T21:51:58Z","receivedAt":"2023-09-07T21:52:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"(These patches should be applied on top of a merge with\ntb/repack-existing-packs-cleanup, and tb/multi-cruft-pack).\n\nThis series attempts to give users some more robust tools for managing\nrepositories with a large number of unreachable objects by storing them\nin separate cruft packs, via a new option `--cruft-max-size`, like so:\n\n $ git.compile repack -d --cruft --max-pack-size=10M\n [...]\n Enumerating cruft objects: 617483, done.\n Counting objects: 100% (83791/83791), done.\n Delta compression using up to 20 threads\n Compressing objects: 100% (59696/59696), done.\n Writing objects: 100% (83791/83791), done.\n Total 83791 (delta 19251), reused 82502 (delta 19148), pack-reused 0\n\n $ ls -la .git/objects/pack/pack-*.mtimes\n -r--r--r-- 1 ttaylorr ttaylorr 179144 Sep 7 17:46 .git/objects/pack/pack-1a95260d26f2897abfd2d54f1d58f535acb81d23.mtimes\n -r--r--r-- 1 ttaylorr ttaylorr 452 Sep 7 17:46 .git/objects/pack/pack-5fde8701ae0f2e5553f1fa33de05faf12f94c07f.mtimes\n -r--r--r-- 1 ttaylorr ttaylorr 155720 Sep 7 17:46 .git/objects/pack/pack-91f9e66921e0ebe1b5e35d34842551468cecdc28.mtimes\n -r--r--r-- 1 ttaylorr ttaylorr 56 Sep 7 17:46 .git/objects/pack/pack-95fe626743207b177b45f32b60fdc313e525ea60.mtimes\n\nThe details are explained in the second patch, but the gist is that we\nwill combine cruft packs up until they reach a certain threshold (as\nspecified by `--cruft-max-size`) and then begin a new \"generation\" of\ncruft packs. That younger generation will grow up until it reaches the\nconfigured threshold, at which point it will become \"frozen\" and then\nany new unreachable objects will be written into a new generation of\ncruft packs.\n\nThe goal of this series is to reduce I/O churn in repositories that\neither (a) have a large number of unreachable objects, (b) rarely prune\nthem, or (c) both.\n\nInstead of having to rewrite a cruft pack containing every unreachable\nobject in the repository, we only have to rewrite a cruft pack up until\nit reaches the given threshold, at which point it is effectively kept\n(i.e., it behaves as if the cruft pack had a \".keep\" file tied to it,\nprovided that the threshold is held constant).\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n t7700: split cruft-related tests to t7704\n builtin/repack.c: implement support for `--cruft-max-size`\n\n Documentation/config/gc.txt | 6 +\n Documentation/git-gc.txt | 7 +\n Documentation/git-repack.txt | 9 +\n builtin/gc.c | 8 +\n builtin/repack.c | 133 +++++++++++--\n t/t6500-gc.sh | 27 +++\n t/t7700-repack.sh | 121 -----------\n t/t7704-repack-cruft.sh | 375 +++++++++++++++++++++++++++++++++++\n 8 files changed, 553 insertions(+), 133 deletions(-)\n create mode 100755 t/t7704-repack-cruft.sh\n\n-- \n2.42.0.138.g7e4e42e1aa\n"},{"id":"481527","messageId":"de6c2a0d70c3e62bd70fdca8b5c6c2ae6eaa13f5.1694123506.git.me@ttaylorr.com","threadId":"60206","inReplyTo":"cover.1694123506.git.me@ttaylorr.com","subject":"[PATCH 1/2] t7700: split cruft-related tests to t7704","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-07T21:52:01Z","receivedAt":"2023-09-07T21:52:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"---\n t/t7700-repack.sh | 121 -------------------------------------\n t/t7704-repack-cruft.sh | 130 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 130 insertions(+), 121 deletions(-)\n create mode 100755 t/t7704-repack-cruft.sh\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 27b66807cd..55ee3eb8ae 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -633,125 +633,4 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n \ttest_server_info_missing\n '\n \n-test_expect_success '--expire-to stores pruned objects (now)' '\n-\tgit init expire-to-now &&\n-\t(\n-\t\tcd expire-to-now &&\n-\n-\t\tgit branch -M main &&\n-\n-\t\ttest_commit base &&\n-\n-\t\tgit checkout -b cruft &&\n-\t\ttest_commit --no-tag cruft &&\n-\n-\t\tgit rev-list --objects --no-object-names main..cruft >moved.raw &&\n-\t\tsort moved.raw >moved.want &&\n-\n-\t\tgit rev-list --all --objects --no-object-names >expect.raw &&\n-\t\tsort expect.raw >expect &&\n-\n-\t\tgit checkout main &&\n-\t\tgit branch -D cruft &&\n-\t\tgit reflog expire --all --expire=all &&\n-\n-\t\tgit init --bare expired.git &&\n-\t\tgit repack -d \\\n-\t\t\t--cruft --cruft-expiration=\"now\" \\\n-\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n-\n-\t\texpired=\"$(ls expired.git/objects/pack/pack-*.idx)\" &&\n-\t\ttest_path_is_file \"${expired%.idx}.mtimes\" &&\n-\n-\t\t# Since the `--cruft-expiration` is \"now\", the effective\n-\t\t# behavior is to move _all_ unreachable objects out to\n-\t\t# the location in `--expire-to`.\n-\t\tgit show-index <$expired >expired.raw &&\n-\t\tcut -d\" \" -f2 expired.raw | sort >expired.objects &&\n-\t\tgit rev-list --all --objects --no-object-names \\\n-\t\t\t>remaining.objects &&\n-\n-\t\t# ...in other words, the combined contents of this\n-\t\t# repository and expired.git should be the same as the\n-\t\t# set of objects we started with.\n-\t\tcat expired.objects remaining.objects | sort >actual &&\n-\t\ttest_cmp expect actual &&\n-\n-\t\t# The \"moved\" objects (i.e., those in expired.git)\n-\t\t# should be the same as the cruft objects which were\n-\t\t# expired in the previous step.\n-\t\ttest_cmp moved.want expired.objects\n-\t)\n-'\n-\n-test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' '\n-\tgit init expire-to-5.minutes.ago &&\n-\t(\n-\t\tcd expire-to-5.minutes.ago &&\n-\n-\t\tgit branch -M main &&\n-\n-\t\ttest_commit base &&\n-\n-\t\t# Create two classes of unreachable objects, one which\n-\t\t# is older than 5 minutes (stale), and another which is\n-\t\t# newer (recent).\n-\t\tfor kind in stale recent\n-\t\tdo\n-\t\t\tgit checkout -b $kind main &&\n-\t\t\ttest_commit --no-tag $kind || return 1\n-\t\tdone &&\n-\n-\t\tgit rev-list --objects --no-object-names main..stale >in &&\n-\t\tstale=\"$(git pack-objects $objdir/pack/pack expect.raw &&\n-\t\tsort expect.raw >expect &&\n-\n-\t\t# moved.want holds the set of objects we expect to find\n-\t\t# in expired.git\n-\t\tgit rev-list --objects --no-object-names main..stale >out &&\n-\t\tsort out >moved.want &&\n-\n-\t\tgit checkout main &&\n-\t\tgit branch -D stale recent &&\n-\t\tgit reflog expire --all --expire=all &&\n-\t\tgit prune-packed &&\n-\n-\t\tgit init --bare expired.git &&\n-\t\tgit repack -d \\\n-\t\t\t--cruft --cruft-expiration=5.minutes.ago \\\n-\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n-\n-\t\t# Some of the remaining objects in this repository are\n-\t\t# unreachable, so use `cat-file --batch-all-objects`\n-\t\t# instead of `rev-list` to get their names\n-\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" \\\n-\t\t\t>remaining.objects &&\n-\t\tsort remaining.objects >actual &&\n-\t\ttest_cmp expect actual &&\n-\n-\t\t(\n-\t\t\tcd expired.git &&\n-\n-\t\t\texpired=\"$(ls objects/pack/pack-*.mtimes)\" &&\n-\t\t\ttest-tool pack-mtimes $(basename $expired) >out &&\n-\t\t\tcut -d\" \" -f1 out | sort >../moved.got &&\n-\n-\t\t\t# Ensure that there are as many objects with the\n-\t\t\t# expected mtime as were moved to expired.git.\n-\t\t\t#\n-\t\t\t# In other words, ensure that the recorded\n-\t\t\t# mtimes of any moved objects was written\n-\t\t\t# correctly.\n-\t\t\tgrep \" $mtime$\" out >matching &&\n-\t\t\ttest_line_count = $(wc -l <../moved.want) matching\n-\t\t) &&\n-\t\ttest_cmp moved.want moved.got\n-\t)\n-'\n-\n test_done\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nnew file mode 100755\nindex 0000000000..d91fcf1af1\n--- /dev/null\n+++ b/t/t7704-repack-cruft.sh\n@@ -0,0 +1,130 @@\n+#!/bin/sh\n+\n+test_description='git repack works correctly'\n+\n+. ./test-lib.sh\n+\n+objdir=.git/objects\n+\n+test_expect_success '--expire-to stores pruned objects (now)' '\n+\tgit init expire-to-now &&\n+\t(\n+\t\tcd expire-to-now &&\n+\n+\t\tgit branch -M main &&\n+\n+\t\ttest_commit base &&\n+\n+\t\tgit checkout -b cruft &&\n+\t\ttest_commit --no-tag cruft &&\n+\n+\t\tgit rev-list --objects --no-object-names main..cruft >moved.raw &&\n+\t\tsort moved.raw >moved.want &&\n+\n+\t\tgit rev-list --all --objects --no-object-names >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D cruft &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\tgit init --bare expired.git &&\n+\t\tgit repack -d \\\n+\t\t\t--cruft --cruft-expiration=\"now\" \\\n+\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n+\n+\t\texpired=\"$(ls expired.git/objects/pack/pack-*.idx)\" &&\n+\t\ttest_path_is_file \"${expired%.idx}.mtimes\" &&\n+\n+\t\t# Since the `--cruft-expiration` is \"now\", the effective\n+\t\t# behavior is to move _all_ unreachable objects out to\n+\t\t# the location in `--expire-to`.\n+\t\tgit show-index <$expired >expired.raw &&\n+\t\tcut -d\" \" -f2 expired.raw | sort >expired.objects &&\n+\t\tgit rev-list --all --objects --no-object-names \\\n+\t\t\t>remaining.objects &&\n+\n+\t\t# ...in other words, the combined contents of this\n+\t\t# repository and expired.git should be the same as the\n+\t\t# set of objects we started with.\n+\t\tcat expired.objects remaining.objects | sort >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# The \"moved\" objects (i.e., those in expired.git)\n+\t\t# should be the same as the cruft objects which were\n+\t\t# expired in the previous step.\n+\t\ttest_cmp moved.want expired.objects\n+\t)\n+'\n+\n+test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' '\n+\tgit init expire-to-5.minutes.ago &&\n+\t(\n+\t\tcd expire-to-5.minutes.ago &&\n+\n+\t\tgit branch -M main &&\n+\n+\t\ttest_commit base &&\n+\n+\t\t# Create two classes of unreachable objects, one which\n+\t\t# is older than 5 minutes (stale), and another which is\n+\t\t# newer (recent).\n+\t\tfor kind in stale recent\n+\t\tdo\n+\t\t\tgit checkout -b $kind main &&\n+\t\t\ttest_commit --no-tag $kind || return 1\n+\t\tdone &&\n+\n+\t\tgit rev-list --objects --no-object-names main..stale >in &&\n+\t\tstale=\"$(git pack-objects $objdir/pack/pack expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\t# moved.want holds the set of objects we expect to find\n+\t\t# in expired.git\n+\t\tgit rev-list --objects --no-object-names main..stale >out &&\n+\t\tsort out >moved.want &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D stale recent &&\n+\t\tgit reflog expire --all --expire=all &&\n+\t\tgit prune-packed &&\n+\n+\t\tgit init --bare expired.git &&\n+\t\tgit repack -d \\\n+\t\t\t--cruft --cruft-expiration=5.minutes.ago \\\n+\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n+\n+\t\t# Some of the remaining objects in this repository are\n+\t\t# unreachable, so use `cat-file --batch-all-objects`\n+\t\t# instead of `rev-list` to get their names\n+\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" \\\n+\t\t\t>remaining.objects &&\n+\t\tsort remaining.objects >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t(\n+\t\t\tcd expired.git &&\n+\n+\t\t\texpired=\"$(ls objects/pack/pack-*.mtimes)\" &&\n+\t\t\ttest-tool pack-mtimes $(basename $expired) >out &&\n+\t\t\tcut -d\" \" -f1 out | sort >../moved.got &&\n+\n+\t\t\t# Ensure that there are as many objects with the\n+\t\t\t# expected mtime as were moved to expired.git.\n+\t\t\t#\n+\t\t\t# In other words, ensure that the recorded\n+\t\t\t# mtimes of any moved objects was written\n+\t\t\t# correctly.\n+\t\t\tgrep \" $mtime$\" out >matching &&\n+\t\t\ttest_line_count = $(wc -l <../moved.want) matching\n+\t\t) &&\n+\t\ttest_cmp moved.want moved.got\n+\t)\n+'\n+\n+test_done\n-- \n2.42.0.138.g7e4e42e1aa\n\n"},{"id":"481528","messageId":"7e4e42e1aacf2111f04a933c0725a8c81769f8d6.1694123506.git.me@ttaylorr.com","threadId":"60206","inReplyTo":"cover.1694123506.git.me@ttaylorr.com","subject":"[PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-07T21:52:04Z","receivedAt":"2023-09-07T21:52:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Cruft packs are an alternative mechanism for storing a collection of\nunreachable objects whose mtimes are recent enough to avoid being\npruned out of the repository.\n\nWhen cruft packs were first introduced back in b757353676\n(builtin/pack-objects.c: --cruft without expiration, 2022-05-20) and\na7d493833f (builtin/pack-objects.c: --cruft with expiration,\n2022-05-20), the recommended workflow consisted of:\n\n - Repacking periodically, either by packing anything loose in the\n repository (via `git repack -d`) or producing a geometric sequence\n of packs (via `git repack --geometric= -d`).\n\n - Every so often, splitting the repository into two packs, one cruft\n to store the unreachable objects, and another non-cruft pack to\n store the reachable objects.\n\nRepositories may (out of band with the above) choose periodically to\nprune out some unreachable objects which have aged out of the grace\nperiod by generating a pack with `--cruft-expiration=`.\n\nThis allowed repositories to maintain relatively few packs on average,\nand quarantine unreachable objects together in a cruft pack, avoiding\nthe pitfalls of holding unreachable objects as loose while they age out\n(for more, see some of the details in 3d89a8c118\n(Documentation/technical: add cruft-packs.txt, 2022-05-20)).\n\nThis all works, but can be costly from an I/O-perspective when a\nrepository has either (a) many unreachable objects, (b) prunes objects\nrelatively infrequently/never, or (c) both.\n\nSince there is at most one cruft pack in the above scheme, each time we\nupdate the cruft pack it must be rewritten from scratch. Because much of\nthe pack is reused, this is a relatively inexpensive operation from a\nCPU-perspective, but is very costly in terms of I/O since we end up\nrewriting basically the same pack (plus any new unreachable objects that\nhave entered the repository since the last time a cruft pack was\ngenerated).\n\nAt the time, we decided against implementing more robust support for\nmultiple cruft packs. This patch implements that support which we were\nlacking.\n\nIntroduce a new option `--cruft-max-size` which allows repositories to\naccumulate cruft packs up to a given size, after which point a new\ngeneration of cruft packs can accumulate until it reaches the maximum\nsize, and so on. To generate a new cruft pack, the process works like\nso:\n\n - Sort a list of any existing cruft packs in ascending order of pack\n size.\n\n - Starting from the beginning of the list, group cruft packs together\n while the accumulated size is smaller than the maximum specified\n pack size.\n\n - Combine the objects in these cruft packs together into a new cruft\n pack, along with any other unreachable objects which have since\n entered the repository.\n\nThis limits the I/O churn up to a quadratic function of the value\nspecified by the `--cruft-max-size` option, instead of behaving\nquadratically in the number of total unreachable objects.\n\nWhen pruning unreachable objects, we bypass the new paths which combine\nsmall cruft packs together, and instead start from scratch, passing in\nthe appropriate `--max-pack-size` down to `pack-objects`, putting it in\ncharge of keeping the resulting set of cruft packs sized correctly.\n\nThis may seem like further I/O churn, but in practice it isn't so bad.\nWe could prune old cruft packs for whom all or most objects are removed,\nand then generate a new cruft pack with just the remaining set of\nobjects. But this additional complexity buys us relatively little,\nbecause most objects end up being pruned anyway, so the I/O churn is\nwell contained.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/gc.txt | 6 +\n Documentation/git-gc.txt | 7 +\n Documentation/git-repack.txt | 9 ++\n builtin/gc.c | 8 ++\n builtin/repack.c | 133 +++++++++++++++++--\n t/t6500-gc.sh | 27 ++++\n t/t7704-repack-cruft.sh | 245 +++++++++++++++++++++++++++++++++++\n 7 files changed, 423 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt\nindex ca47eb2008..795fcfbb69 100644\n--- a/Documentation/config/gc.txt\n+++ b/Documentation/config/gc.txt\n@@ -86,6 +86,12 @@ gc.cruftPacks::\n \tlinkgit:git-repack[1]) instead of as loose objects. The default\n \tis `true`.\n \n+gc.cruftMaxSize::\n+\tLimit the size of new cruft packs when repacking. When\n+\tspecified in addition to `--cruft-max-size`, the command line\n+\toption takes priority. See the `--cruft-max-size` option of\n+\tlinkgit:git-repack[1].\n+\n gc.pruneExpire::\n \tWhen 'git gc' is run, it will call 'prune --expire 2.weeks.ago'\n \t(and 'repack --cruft --cruft-expiration 2.weeks.ago' if using\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex 90806fd26a..8a90d684a7 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -59,6 +59,13 @@ be performed as well.\n \tcruft pack instead of storing them as loose objects. `--cruft`\n \tis on by default.\n \n+--cruft-max-size=::\n+\tWhen packing unreachable objects into a cruft pack, limit the\n+\tsize of new cruft packs to be at most ``. Overrides any\n+\tvalue specified via the `gc.cruftMaxSize` configuration. See\n+\tthe `--cruft-max-size` option of linkgit:git-repack[1] for\n+\tmore.\n+\n --prune=::\n \tPrune loose objects older than date (default is 2 weeks ago,\n \toverridable by the config variable `gc.pruneExpire`).\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 4017157949..23fd203d79 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -74,6 +74,15 @@ to the new separate pack will be written.\n \timmediately instead of waiting for the next `git gc` invocation.\n \tOnly useful with `--cruft -d`.\n \n+--cruft-max-size=::\n+\tRepack cruft objects into packs as large as `` before\n+\tcreating new packs. As long as there are enough cruft packs\n+\tsmaller than ``, repacking will cause a new cruft pack to\n+\tbe created containing objects from any combined cruft packs,\n+\talong with any new unreachable objects. Cruft packs larger\n+\tthan `` will not be modified. Only useful with `--cruft\n+\t-d`.\n+\n --expire-to=::\n \tWrite a cruft pack containing pruned objects (if any) to the\n \tdirectory ``. This option is useful for keeping a copy of\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 1f53b66c7b..b6640abd35 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -52,6 +52,7 @@ static const char * const builtin_gc_usage[] = {\n static int pack_refs = 1;\n static int prune_reflogs = 1;\n static int cruft_packs = 1;\n+static char *cruft_max_size;\n static int aggressive_depth = 50;\n static int aggressive_window = 250;\n static int gc_auto_threshold = 6700;\n@@ -163,6 +164,7 @@ static void gc_config(void)\n \tgit_config_get_int(\"gc.autopacklimit\", &gc_auto_pack_limit);\n \tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n \tgit_config_get_bool(\"gc.cruftpacks\", &cruft_packs);\n+\tgit_config_get_string(\"gc.cruftmaxsize\", &cruft_max_size);\n \tgit_config_get_expiry(\"gc.pruneexpire\", &prune_expire);\n \tgit_config_get_expiry(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\n \tgit_config_get_expiry(\"gc.logexpiry\", &gc_log_expire);\n@@ -347,6 +349,9 @@ static void add_repack_all_option(struct string_list *keep_pack)\n \t\tstrvec_push(&repack, \"--cruft\");\n \t\tif (prune_expire)\n \t\t\tstrvec_pushf(&repack, \"--cruft-expiration=%s\", prune_expire);\n+\t\tif (cruft_max_size)\n+\t\t\tstrvec_pushf(&repack, \"--cruft-max-size=%s\",\n+\t\t\t\t cruft_max_size);\n \t} else {\n \t\tstrvec_push(&repack, \"-A\");\n \t\tif (prune_expire)\n@@ -575,6 +580,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"prune unreferenced objects\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },\n \t\tOPT_BOOL(0, \"cruft\", &cruft_packs, N_(\"pack unreferenced objects separately\")),\n+\t\tOPT_STRING(0, \"cruft-max-size\", &cruft_max_size,\n+\t\t\t N_(\"bytes\"),\n+\t\t\t N_(\"with --cruft, limit the size of new cruft packs\")),\n \t\tOPT_BOOL(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n \t\tOPT_BOOL_F(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\"),\n \t\t\t PARSE_OPT_NOCOMPLETE),\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 44cb261371..56e7f5f43d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -26,6 +26,9 @@\n #define LOOSEN_UNREACHABLE 2\n #define PACK_CRUFT 4\n \n+#define DELETE_PACK ((void*)(uintptr_t)1)\n+#define RETAIN_PACK ((uintptr_t)(1<<1))\n+\n static int pack_everything;\n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n@@ -113,6 +116,11 @@ static int has_existing_non_kept_packs(const struct existing_packs *existing)\n \treturn existing->non_kept_packs.nr || existing->cruft_packs.nr;\n }\n \n+static int pack_is_retained(struct string_list_item *item)\n+{\n+\treturn (uintptr_t)item->util & RETAIN_PACK;\n+}\n+\n static void mark_packs_for_deletion_1(struct string_list *names,\n \t\t\t\t struct string_list *list)\n {\n@@ -125,17 +133,39 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n \t\tif (len < hexsz)\n \t\t\tcontinue;\n \t\tsha1 = item->string + len - hexsz;\n-\t\t/*\n-\t\t * Mark this pack for deletion, which ensures that this\n-\t\t * pack won't be included in a MIDX (if `--write-midx`\n-\t\t * was given) and that we will actually delete this pack\n-\t\t * (if `-d` was given).\n-\t\t */\n-\t\tif (!string_list_has_string(names, sha1))\n-\t\t\titem->util = (void*)1;\n+\n+\t\tif (pack_is_retained(item)) {\n+\t\t\titem->util = NULL;\n+\t\t} else if (!string_list_has_string(names, sha1)) {\n+\t\t\t/*\n+\t\t\t * Mark this pack for deletion, which ensures\n+\t\t\t * that this pack won't be included in a MIDX\n+\t\t\t * (if `--write-midx` was given) and that we\n+\t\t\t * will actually delete this pack (if `-d` was\n+\t\t\t * given).\n+\t\t\t */\n+\t\t\titem->util = DELETE_PACK;\n+\t\t}\n \t}\n }\n \n+static void retain_cruft_pack(struct existing_packs *existing,\n+\t\t\t struct packed_git *cruft)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\n+\tstrbuf_addstr(&buf, pack_basename(cruft));\n+\tstrbuf_strip_suffix(&buf, \".pack\");\n+\n+\titem = string_list_lookup(&existing->cruft_packs, buf.buf);\n+\tif (!item)\n+\t\tBUG(\"could not find cruft pack '%s'\", pack_basename(cruft));\n+\n+\titem->util = (void*)RETAIN_PACK;\n+\tstrbuf_release(&buf);\n+}\n+\n static void mark_packs_for_deletion(struct existing_packs *existing,\n \t\t\t\t struct string_list *names)\n \n@@ -217,6 +247,8 @@ static void collect_pack_filenames(struct existing_packs *existing,\n \t}\n \n \tstring_list_sort(&existing->kept_packs);\n+\tstring_list_sort(&existing->non_kept_packs);\n+\tstring_list_sort(&existing->cruft_packs);\n \tstrbuf_release(&buf);\n }\n \n@@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include,\n \tstrbuf_release(&path);\n }\n \n+static int existing_cruft_pack_cmp(const void *va, const void *vb)\n+{\n+\tstruct packed_git *a = *(struct packed_git **)va;\n+\tstruct packed_git *b = *(struct packed_git **)vb;\n+\n+\tif (a->pack_size < b->pack_size)\n+\t\treturn -1;\n+\tif (a->pack_size > b->pack_size)\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static void collapse_small_cruft_packs(FILE *in, unsigned long max_size,\n+\t\t\t\t struct existing_packs *existing)\n+{\n+\tstruct packed_git **existing_cruft, *p;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tunsigned long total_size = 0;\n+\tsize_t existing_cruft_nr = 0;\n+\tsize_t i;\n+\n+\tALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);\n+\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\tif (!(p->is_cruft && p->pack_local))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addstr(&buf, pack_basename(p));\n+\t\tstrbuf_strip_suffix(&buf, \".pack\");\n+\n+\t\tif (!string_list_has_string(&existing->cruft_packs, buf.buf))\n+\t\t\tcontinue;\n+\n+\t\tif (existing_cruft_nr >= existing->cruft_packs.nr)\n+\t\t\tBUG(\"too many cruft packs (found %\"PRIuMAX\", but knew \"\n+\t\t\t \"of %\"PRIuMAX\")\",\n+\t\t\t (uintmax_t)existing_cruft_nr + 1,\n+\t\t\t (uintmax_t)existing->cruft_packs.nr);\n+\t\texisting_cruft[existing_cruft_nr++] = p;\n+\t}\n+\n+\tQSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);\n+\n+\tfor (i = 0; i < existing_cruft_nr; i++) {\n+\t\toff_t proposed;\n+\n+\t\tp = existing_cruft[i];\n+\t\tproposed = st_add(total_size, p->pack_size);\n+\n+\t\tif (proposed <= max_size) {\n+\t\t\ttotal_size = proposed;\n+\t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n+\t\t} else {\n+\t\t\tretain_cruft_pack(existing, p);\n+\t\t\tfprintf(in, \"%s\\n\", pack_basename(p));\n+\t\t}\n+\t}\n+\n+\tfor (i = 0; i < existing->non_kept_packs.nr; i++)\n+\t\tfprintf(in, \"-%s.pack\\n\",\n+\t\t\texisting->non_kept_packs.items[i].string);\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static int write_cruft_pack(const struct pack_objects_args *args,\n \t\t\t const char *destination,\n \t\t\t const char *pack_prefix,\n@@ -846,10 +944,18 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \tin = xfdopen(cmd.in, \"w\");\n \tfor_each_string_list_item(item, names)\n \t\tfprintf(in, \"%s-%s.pack\\n\", pack_prefix, item->string);\n-\tfor_each_string_list_item(item, &existing->non_kept_packs)\n-\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n-\tfor_each_string_list_item(item, &existing->cruft_packs)\n-\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n+\tif (args->max_pack_size && !cruft_expiration) {\n+\t\tunsigned long max_pack_size;\n+\t\tif (!git_parse_ulong(args->max_pack_size, &max_pack_size))\n+\t\t\treturn error(_(\"could not parse --cruft-max-size: '%s'\"),\n+\t\t\t\t args->max_pack_size);\n+\t\tcollapse_small_cruft_packs(in, max_pack_size, existing);\n+\t} else {\n+\t\tfor_each_string_list_item(item, &existing->non_kept_packs)\n+\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n+\t\tfor_each_string_list_item(item, &existing->cruft_packs)\n+\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n+\t}\n \tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tfprintf(in, \"%s.pack\\n\", item->string);\n \tfclose(in);\n@@ -912,6 +1018,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t PACK_CRUFT),\n \t\tOPT_STRING(0, \"cruft-expiration\", &cruft_expiration, N_(\"approxidate\"),\n \t\t\t\tN_(\"with --cruft, expire objects older than this\")),\n+\t\tOPT_STRING(0, \"cruft-max-size\", &cruft_po_args.max_pack_size,\n+\t\t\t\tN_(\"bytes\"),\n+\t\t\t\tN_(\"with --cruft, limit the size of new cruft packs\")),\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,\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 69509d0c11..5e737c47be 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -303,6 +303,33 @@ test_expect_success 'gc.bigPackThreshold ignores cruft packs' '\n \t)\n '\n \n+cruft_max_size_opts=\"git repack -d -l --cruft --cruft-expiration=2.weeks.ago\"\n+\n+test_expect_success 'setup for --cruft-max-size tests' '\n+\tgit init cruft--max-size &&\n+\t(\n+\t\tcd cruft--max-size &&\n+\t\tprepare_cruft_history\n+\t)\n+'\n+\n+test_expect_success '--cruft-max-size sets appropriate repack options' '\n+\tGIT_TRACE2_EVENT=$(pwd)/trace2.txt git -C cruft--max-size \\\n+\t\tgc --cruft --cruft-max-size=1M &&\n+\ttest_subcommand $cruft_max_size_opts --cruft-max-size=1M blob &&\n+\tgit hash-object -w -t blob blob &&\n+\trm blob\n+}\n+\n+pack_random_blob () {\n+\tgenerate_random_blob \"$@\" &&\n+\tgit repack -d -q >/dev/null\n+}\n+\n+generate_cruft_pack () {\n+\tpack_random_blob \"$@\" >/dev/null &&\n+\n+\tls $packdir/pack-*.pack | xargs -n 1 basename >in &&\n+\tpack=\"$(git pack-objects --cruft $packdir/pack foo.objects &&\n+\t\ttest-tool pack-mtimes $(basename \"$cruft_bar\") >bar.objects &&\n+\n+\t\tgrep \"^$foo\" foo.objects &&\n+\t\ttest_line_count = 1 foo.objects &&\n+\t\tgrep \"^$bar\" bar.objects &&\n+\t\ttest_line_count = 1 bar.objects\n+\t)\n+'\n+\n+test_expect_success '--cruft-max-size combines existing packs when below threshold' '\n+\tgit init cruft-max-size-small &&\n+\t(\n+\t\tcd cruft-max-size-small &&\n+\t\ttest_commit base &&\n+\n+\t\tfoo=\"$(pack_random_blob foo $((1*1024*1024)))\" &&\n+\t\tgit repack --cruft -d &&\n+\n+\t\tbar=\"$(pack_random_blob bar $((1*1024*1024)))\" &&\n+\t\tgit repack --cruft -d --cruft-max-size=10M &&\n+\n+\t\tcruft=$(ls $packdir/pack-*.mtimes) &&\n+\t\ttest-tool pack-mtimes $(basename \"$cruft\") >cruft.objects &&\n+\n+\t\tgrep \"^$foo\" cruft.objects &&\n+\t\tgrep \"^$bar\" cruft.objects &&\n+\t\ttest_line_count = 2 cruft.objects\n+\t)\n+'\n+\n+test_expect_success '--cruft-max-size combines smaller packs first' '\n+\tgit init cruft-max-size-consume-small &&\n+\t(\n+\t\tcd cruft-max-size-consume-small &&\n+\n+\t\ttest_commit base &&\n+\t\tgit repack -ad &&\n+\n+\t\tcruft_foo=\"$(generate_cruft_pack foo 524288)\" && # 0.5 MiB\n+\t\tcruft_bar=\"$(generate_cruft_pack bar 524288)\" && # 0.5 MiB\n+\t\tcruft_baz=\"$(generate_cruft_pack baz 1048576)\" && # 1.0 MiB\n+\t\tcruft_quux=\"$(generate_cruft_pack quux 1572864)\" && # 1.5 MiB\n+\n+\t\ttest-tool pack-mtimes \"$(basename $cruft_foo)\" >expect.raw &&\n+\t\ttest-tool pack-mtimes \"$(basename $cruft_bar)\" >>expect.raw &&\n+\t\tsort expect.raw >expect.objects &&\n+\n+\t\t# repacking with `--cruft-max-size=2M` should combine\n+\t\t# both 0.5 MiB packs together, instead of, say, one of\n+\t\t# the 0.5 MiB packs with the 1.0 MiB pack\n+\t\tls $packdir/pack-*.mtimes | sort >cruft.before &&\n+\t\tgit repack -d --cruft --cruft-max-size=2M &&\n+\t\tls $packdir/pack-*.mtimes | sort >cruft.after &&\n+\n+\t\tcomm -13 cruft.before cruft.after >cruft.new &&\n+\t\tcomm -23 cruft.before cruft.after >cruft.removed &&\n+\n+\t\ttest_line_count = 1 cruft.new &&\n+\t\ttest_line_count = 2 cruft.removed &&\n+\n+\t\t# the two smaller packs should be rolled up first\n+\t\tprintf \"%s\\n\" $cruft_foo $cruft_bar | sort >expect.removed &&\n+\t\ttest_cmp expect.removed cruft.removed &&\n+\n+\t\t# ...and contain the set of objects rolled up\n+\t\ttest-tool pack-mtimes \"$(basename $(cat cruft.new))\" >actual.raw &&\n+\t\tsort actual.raw >actual.objects &&\n+\n+\t\ttest_cmp expect.objects actual.objects\n+\t)\n+'\n+\n+test_expect_success 'setup --cruft-max-size with freshened objects' '\n+\tgit init cruft-max-size-freshen &&\n+\t(\n+\t\tcd cruft-max-size-freshen &&\n+\n+\t\ttest_commit base &&\n+\t\tgit repack -ad &&\n+\n+\t\tfoo=\"$(generate_random_blob foo 64)\" &&\n+\t\ttest-tool chmtime --get -10000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$foo\")\" >foo.mtime &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\tcruft=\"$(ls $packdir/pack-*.mtimes)\" &&\n+\t\ttest-tool pack-mtimes \"$(basename $cruft)\" >actual &&\n+\t\techo \"$foo $(cat foo.mtime)\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--cruft-max-size with freshened objects (loose)' '\n+\t(\n+\t\tcd cruft-max-size-freshen &&\n+\n+\t\t# regenerate the object, setting its mtime to be more recent\n+\t\tfoo=\"$(generate_random_blob foo 64)\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$foo\")\" >foo.mtime &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\tcruft=\"$(ls $packdir/pack-*.mtimes)\" &&\n+\t\ttest-tool pack-mtimes \"$(basename $cruft)\" >actual &&\n+\t\techo \"$foo $(cat foo.mtime)\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--cruft-max-size with freshened objects (packed)' '\n+\t(\n+\t\tcd cruft-max-size-freshen &&\n+\n+\t\t# regenerate the object and store it in a packfile,\n+\t\t# setting its mtime to be more recent\n+\t\t#\n+\t\t# store it alongside another cruft object so that we\n+\t\t# do not create an identical copy of the existing\n+\t\t# cruft pack (which contains $foo).\n+\t\tfoo=\"$(generate_random_blob foo 64)\" &&\n+\t\tbar=\"$(generate_random_blob bar 64)\" &&\n+\t\tfoo_pack=\"$(printf \"%s\\n\" $foo $bar | git pack-objects $packdir/pack)\" &&\n+\t\tgit prune-packed &&\n+\n+\t\ttest-tool chmtime --get -10 \\\n+\t\t\t\"$packdir/pack-$foo_pack.pack\" >foo.mtime &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\tcruft=\"$(ls $packdir/pack-*.mtimes)\" &&\n+\t\ttest-tool pack-mtimes \"$(basename $cruft)\" >actual &&\n+\t\techo \"$foo $(cat foo.mtime)\" >expect.raw &&\n+\t\techo \"$bar $(cat foo.mtime)\" >>expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--cruft-max-size with pruning' '\n+\tgit init cruft-max-size-prune &&\n+\t(\n+\t\tcd cruft-max-size-prune &&\n+\n+\t\ttest_commit base &&\n+\t\tfoo=\"$(generate_random_blob foo $((1024*1024)))\" &&\n+\t\tbar=\"$(generate_random_blob bar $((1024*1024)))\" &&\n+\t\tbaz=\"$(generate_random_blob baz $((1024*1024)))\" &&\n+\n+\t\ttest-tool chmtime -10000 \"$objdir/$(test_oid_to_path \"$foo\")\" &&\n+\n+\t\tgit repack -d --cruft --cruft-max-size=1M &&\n+\n+\t\t# backdate the mtimes of all cruft packs to validate\n+\t\t# that they were rewritten as a result of pruning\n+\t\tls $packdir/pack-*.mtimes | sort >cruft.before &&\n+\t\tfor cruft in $(cat cruft.before)\n+\t\tdo\n+\t\t\tmtime=\"$(test-tool chmtime --get -10000 \"$cruft\")\" &&\n+\t\t\techo $cruft $mtime >>mtimes || return 1\n+\t\tdone &&\n+\n+\t\t# repack (and prune) with a --cruft-max-size to ensure\n+\t\t# that we appropriately split the resulting set of packs\n+\t\tgit repack -d --cruft --cruft-max-size=1M \\\n+\t\t\t--cruft-expiration=10.seconds.ago &&\n+\t\tls $packdir/pack-*.mtimes | sort >cruft.after &&\n+\n+\t\tfor cruft in $(cat cruft.after)\n+\t\tdo\n+\t\t\told_mtime=\"$(grep $cruft mtimes | cut -d\" \" -f2)\" &&\n+\t\t\tnew_mtime=\"$(test-tool chmtime --get $cruft)\" &&\n+\t\t\ttest $old_mtime -lt $new_mtime || return 1\n+\t\tdone &&\n+\n+\t\ttest_line_count = 3 cruft.before &&\n+\t\ttest_line_count = 2 cruft.after &&\n+\t\ttest_must_fail git cat-file -e $foo &&\n+\t\tgit cat-file -e $bar &&\n+\t\tgit cat-file -e $baz\n+\t)\n+'\n+\n+test_expect_success '--cruft-max-size ignores non-local packs' '\n+\trepo=\"cruft-max-size-non-local\" &&\n+\tgit init $repo &&\n+\t(\n+\t\tcd $repo &&\n+\t\ttest_commit base &&\n+\t\tgenerate_random_blob foo 64 &&\n+\t\tgit repack --cruft -d\n+\t) &&\n+\n+\tgit clone --reference=$repo $repo $repo-alt &&\n+\t(\n+\t\tcd $repo-alt &&\n+\n+\t\ttest_commit other &&\n+\t\tgenerate_random_blob bar 64 &&\n+\n+\t\t# ensure that we do not attempt to pick up packs from\n+\t\t# the non-alternated repository, which would result in a\n+\t\t# crash\n+\t\tgit repack --cruft --cruft-max-size=1M -d\n+\t)\n+'\n+\n test_done\n-- \n2.42.0.138.g7e4e42e1aa\n"},{"id":"481545","messageId":"xmqqtts5wnwn.fsf@gitster.g","threadId":"60206","inReplyTo":"7e4e42e1aacf2111f04a933c0725a8c81769f8d6.1694123506.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-07T23:42:48Z","receivedAt":"2023-09-07T23:43:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> This all works, but can be costly from an I/O-perspective when a\n> repository has either (a) many unreachable objects, (b) prunes objects\n> relatively infrequently/never, or (c) both.\n\nI can certainly understand (a). If we need to write a lot of\nobjects into the craft pack, among which many of them are already in\nthe previous generation of craft pack, we would be copying bits from\nthe old to the new craft pack, and having to do so for many objects\nwould involve expensive I/O. But not (b). Whether we prune objects\ninfrequently or we do so very often, as long as the number and\non-disk size of objects that has to go into craft packs are the\nsame, wouldn't the cost of I/O pretty much the same? IOW, (b) does\nnot have much to do with how repacking is costly I/O wise, except\nthat it is a contributing factor to make (a) worse, which is the\nreal cause of the I/O cost.\n\n> At the time, we decided against implementing more robust support for\n> multiple cruft packs. This patch implements that support which we were\n> lacking.\n>\n> Introduce a new option `--cruft-max-size` which allows repositories to\n> accumulate cruft packs up to a given size, after which point a new\n> generation of cruft packs can accumulate until it reaches the maximum\n> size, and so on. To generate a new cruft pack, the process works like\n> so:\n>\n> - Sort a list of any existing cruft packs in ascending order of pack\n> size.\n>\n> - Starting from the beginning of the list, group cruft packs together\n> while the accumulated size is smaller than the maximum specified\n> pack size.\n>\n> - Combine the objects in these cruft packs together into a new cruft\n> pack, along with any other unreachable objects which have since\n> entered the repository.\n\nThe above three steps guarantees that, before the amount of the\naccumulated cruft objects reaches the \"max-size\", we would be moving\nbits from old to new cruft packs and incurring the same I/O cost as\nin the current system. I suspect (I haven't read what the new code\ndoes) the untold fourth point is what improves the new system over\nthe current one, which would be \"Do not touch the existing cruft\npack(s) that are already large enough, and send enumerated cruft\nobjects that do not appear in these existing cruft pack(s) to a new\ncruft pack. Do not rewrite these existing cruft pack(s), even if\nthere are some cruft objects that has aged enough, because rewriting\nthese huge cruft packs only to expunge a few objects from them is\ncostly in I/O\" or something? In any case, I do not think with only\nthe above three ...\n\n> This limits the I/O churn up to a quadratic function of the value\n> specified by the `--cruft-max-size` option, instead of behaving\n> quadratically in the number of total unreachable objects.\n\n... I do not quite see how you would limit the I/O churn.\n\n> When pruning unreachable objects, we bypass the new paths which combine\n\n\"paths\" here refers to...? code paths, I guess?\n\n> +gc.cruftMaxSize::\n> +\tLimit the size of new cruft packs when repacking. When\n> +\tspecified in addition to `--cruft-max-size`, the command line\n> +\toption takes priority. See the `--cruft-max-size` option of\n> +\tlinkgit:git-repack[1].\n\nHmph.\n\nI am reasonably sure that I will mix the name up and call it\ngc.maxCruftSize in my configuration file and scratch my head\nwondering why it is not working.\n\n> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\n> index 90806fd26a..8a90d684a7 100644\n> --- a/Documentation/git-gc.txt\n> +++ b/Documentation/git-gc.txt\n> @@ -59,6 +59,13 @@ be performed as well.\n> \tcruft pack instead of storing them as loose objects. `--cruft`\n> \tis on by default.\n> \n> +--cruft-max-size=::\n> +\tWhen packing unreachable objects into a cruft pack, limit the\n> +\tsize of new cruft packs to be at most ``. Overrides any\n> +\tvalue specified via the `gc.cruftMaxSize` configuration. See\n> +\tthe `--cruft-max-size` option of linkgit:git-repack[1] for\n> +\tmore.\n\nAt least this side giving --max-cruft-size= (which I think is a\nlot more natural word order) would cause parse-options to give an\nerror, so it won't risk mistakes go silently unnoticed.\n\n> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\n> index 4017157949..23fd203d79 100644\n> --- a/Documentation/git-repack.txt\n> +++ b/Documentation/git-repack.txt\n> @@ -74,6 +74,15 @@ to the new separate pack will be written.\n> \timmediately instead of waiting for the next `git gc` invocation.\n> \tOnly useful with `--cruft -d`.\n> \n> +--cruft-max-size=::\n> +\tRepack cruft objects into packs as large as `` before\n> +\tcreating new packs. As long as there are enough cruft packs\n> +\tsmaller than ``, repacking will cause a new cruft pack to\n> +\tbe created containing objects from any combined cruft packs,\n> +\talong with any new unreachable objects. Cruft packs larger\n> +\tthan `` will not be modified. Only useful with `--cruft\n> +\t-d`.\n\nHere, the missing fourth point I pointed out above is mentioned,\nwhich is good.\n\nDescribe the unit for (I am assuming that is counted in bytes,\nhonoring the human-friendly suffix, like 100M).\n\nThere may be some \"interesting\" behaviour around the size boundary,\nno? If you pack too many objects, your resulting size may slightly\nbust and you will get a complaint, but by fixing that \"bug\", you\nwill always stop short of filling the whole bytes in the\nproduced packfiles, and they will not be excempt from rewriting\n(becuase they are not \"larger than \"), which defeats the point of\nthis patch.\n\nDescribe that is a threshold that we stop soon after passing to\nexplicitly allow us to go beyond it would solve the above problem, I\nwould presume.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 1f53b66c7b..b6640abd35 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -52,6 +52,7 @@ static const char * const builtin_gc_usage[] = {\n> static int pack_refs = 1;\n> static int prune_reflogs = 1;\n> static int cruft_packs = 1;\n> +static char *cruft_max_size;\n\nI do not think this type is a good idea.\n\n> static int aggressive_depth = 50;\n> static int aggressive_window = 250;\n> static int gc_auto_threshold = 6700;\n> @@ -163,6 +164,7 @@ static void gc_config(void)\n> \tgit_config_get_int(\"gc.autopacklimit\", &gc_auto_pack_limit);\n> \tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n> \tgit_config_get_bool(\"gc.cruftpacks\", &cruft_packs);\n> +\tgit_config_get_string(\"gc.cruftmaxsize\", &cruft_max_size);\n> \tgit_config_get_expiry(\"gc.pruneexpire\", &prune_expire);\n> \tgit_config_get_expiry(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\n> \tgit_config_get_expiry(\"gc.logexpiry\", &gc_log_expire);\n> @@ -347,6 +349,9 @@ static void add_repack_all_option(struct string_list *keep_pack)\n> \t\tstrvec_push(&repack, \"--cruft\");\n> \t\tif (prune_expire)\n> \t\t\tstrvec_pushf(&repack, \"--cruft-expiration=%s\", prune_expire);\n> +\t\tif (cruft_max_size)\n> +\t\t\tstrvec_pushf(&repack, \"--cruft-max-size=%s\",\n> +\t\t\t\t cruft_max_size);\n\nEven if you are going to pass it intact, as an opaque token, to\nanother program. Rather, you should parse and make sure it is a\nvalid non-negative byte count before calling out the other program\n(passing the original string down, after parsing it only for error\nchecking the value, and having \"repack\" do its own parsing, is\nperfectly fine).\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 44cb261371..56e7f5f43d 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -26,6 +26,9 @@\n> #define LOOSEN_UNREACHABLE 2\n> #define PACK_CRUFT 4\n> \n> +#define DELETE_PACK ((void*)(uintptr_t)1)\n> +#define RETAIN_PACK ((uintptr_t)(1<<1))\n\nShouldn't these look more similar? That is\n\n\t((void *)(uintptr_t)(1<<0))\n\t((void *)(uintptr_t)(1<<1))\n\n> +\t\tif (pack_is_retained(item)) {\n> +\t\t\titem->util = NULL;\n> +\t\t} else if (!string_list_has_string(names, sha1)) {\n\nWithout the pack_marked_for_deletion(item) macro Patrick suggested,\nthe logic around here looks very uneven.\n\n> +\t\t\t/*\n> +\t\t\t * Mark this pack for deletion, which ensures\n> +\t\t\t * that this pack won't be included in a MIDX\n> +\t\t\t * (if `--write-midx` was given) and that we\n> +\t\t\t * will actually delete this pack (if `-d` was\n> +\t\t\t * given).\n> +\t\t\t */\n> +\t\t\titem->util = DELETE_PACK;\n> +\t\t}\n> \t}\n> }\n> \n> +static void retain_cruft_pack(struct existing_packs *existing,\n> +\t\t\t struct packed_git *cruft)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct string_list_item *item;\n> +\n> +\tstrbuf_addstr(&buf, pack_basename(cruft));\n> +\tstrbuf_strip_suffix(&buf, \".pack\");\n> +\n> +\titem = string_list_lookup(&existing->cruft_packs, buf.buf);\n> +\tif (!item)\n> +\t\tBUG(\"could not find cruft pack '%s'\", pack_basename(cruft));\n> +\n> +\titem->util = (void*)RETAIN_PACK;\n> +\tstrbuf_release(&buf);\n> +}\n> +\n> static void mark_packs_for_deletion(struct existing_packs *existing,\n> \t\t\t\t struct string_list *names)\n> \n> @@ -217,6 +247,8 @@ static void collect_pack_filenames(struct existing_packs *existing,\n> \t}\n> \n> \tstring_list_sort(&existing->kept_packs);\n> +\tstring_list_sort(&existing->non_kept_packs);\n> +\tstring_list_sort(&existing->cruft_packs);\n> \tstrbuf_release(&buf);\n> }\n> \n> @@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include,\n> \tstrbuf_release(&path);\n> }\n> \n> +static int existing_cruft_pack_cmp(const void *va, const void *vb)\n> +{\n> +\tstruct packed_git *a = *(struct packed_git **)va;\n> +\tstruct packed_git *b = *(struct packed_git **)vb;\n> +\n> +\tif (a->pack_size < b->pack_size)\n> +\t\treturn -1;\n> +\tif (a->pack_size > b->pack_size)\n> +\t\treturn 1;\n> +\treturn 0;\n> +}\n> +\n> +static void collapse_small_cruft_packs(FILE *in, unsigned long max_size,\n> +\t\t\t\t struct existing_packs *existing)\n> +{\n> +\tstruct packed_git **existing_cruft, *p;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tunsigned long total_size = 0;\n> +\tsize_t existing_cruft_nr = 0;\n> +\tsize_t i;\n> +\n> +\tALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);\n> +\n> +\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> +\t\tif (!(p->is_cruft && p->pack_local))\n> +\t\t\tcontinue;\n> +\n> +\t\tstrbuf_reset(&buf);\n> +\t\tstrbuf_addstr(&buf, pack_basename(p));\n> +\t\tstrbuf_strip_suffix(&buf, \".pack\");\n> +\n> +\t\tif (!string_list_has_string(&existing->cruft_packs, buf.buf))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (existing_cruft_nr >= existing->cruft_packs.nr)\n> +\t\t\tBUG(\"too many cruft packs (found %\"PRIuMAX\", but knew \"\n> +\t\t\t \"of %\"PRIuMAX\")\",\n\nIs that a BUG() that somehow miscounted the packs, or can it be a\nruntime error that may happen when a \"git push\" is pushing new\nobjects into the repository, creating a new pack we did not know\nabout? Something like the latter should not be marked a BUG(), but \n\n> +\t\t\t (uintmax_t)existing_cruft_nr + 1,\n> +\t\t\t (uintmax_t)existing->cruft_packs.nr);\n> +\t\texisting_cruft[existing_cruft_nr++] = p;\n> +\t}\n> +\n> +\tQSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);\n\nWe use the simplest \"from smaller ones to larger ones, combine one\nby one together until the result gets large enough\", which would not\ngive us the best packing, but it is OK because it is not our goal to\nsolve knapsack problem here, I presume?\n\n> +\tfor (i = 0; i < existing_cruft_nr; i++) {\n> +\t\toff_t proposed;\n> +\n> +\t\tp = existing_cruft[i];\n> +\t\tproposed = st_add(total_size, p->pack_size);\n> +\n> +\t\tif (proposed <= max_size) {\n> +\t\t\ttotal_size = proposed;\n> +\t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n> +\t\t} else {\n> +\t\t\tretain_cruft_pack(existing, p);\n> +\t\t\tfprintf(in, \"%s\\n\", pack_basename(p));\n> +\t\t}\n> +\t}\n\nThis is exactly what I talked about the possibly funny behaviour\naround the boundary earlier, but it may be even worse. This time,\nwe may decide that a pack with size is to be retained,\nonly because the pack that came next in the existing list happened\nto be larger than epsilon, but next time around, it may not be the\ncase (i.e. our pack may be smaller than epsilon, the next one in the\ncurrent round may be larger than epsilon, but before we repack the\nnext time, a new pack that is slightly smaller than epsilon that is\nlarger than our pack may have been created---now our pack will be\ncombined with it), so the algorithm to choose which ones are kept\ndoes not depend on the pack itself alone but also depends on its\nsurroundings.\n\n"},{"id":"481546","messageId":"CAPig+cR8pYr6Z2Jojw-QO4n5qFkEpMgQh+ApFK352-r9da4KTg@mail.gmail.com","threadId":"60206","inReplyTo":"de6c2a0d70c3e62bd70fdca8b5c6c2ae6eaa13f5.1694123506.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] t7700: split cruft-related tests to t7704","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-09-08T00:01:35Z","receivedAt":"2023-09-08T00:01:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 7, 2023 at 5:52 PM Taylor Blau wrote:\n> t7700: split cruft-related tests to t7704\n> ---\n\nMissing sign-off.\n"},{"id":"481551","messageId":"ZPsDyKOa76Mxb9u-@tanuki","threadId":"60206","inReplyTo":"7e4e42e1aacf2111f04a933c0725a8c81769f8d6.1694123506.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-09-08T11:21:44Z","receivedAt":"2023-09-08T11:21:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 07, 2023 at 05:52:04PM -0400, Taylor Blau wrote:\n[snip]\n> @@ -125,17 +133,39 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n> \t\tif (len < hexsz)\n> \t\t\tcontinue;\n> \t\tsha1 = item->string + len - hexsz;\n> -\t\t/*\n> -\t\t * Mark this pack for deletion, which ensures that this\n> -\t\t * pack won't be included in a MIDX (if `--write-midx`\n> -\t\t * was given) and that we will actually delete this pack\n> -\t\t * (if `-d` was given).\n> -\t\t */\n> -\t\tif (!string_list_has_string(names, sha1))\n> -\t\t\titem->util = (void*)1;\n> +\n> +\t\tif (pack_is_retained(item)) {\n> +\t\t\titem->util = NULL;\n> +\t\t} else if (!string_list_has_string(names, sha1)) {\n> +\t\t\t/*\n> +\t\t\t * Mark this pack for deletion, which ensures\n> +\t\t\t * that this pack won't be included in a MIDX\n> +\t\t\t * (if `--write-midx` was given) and that we\n> +\t\t\t * will actually delete this pack (if `-d` was\n> +\t\t\t * given).\n> +\t\t\t */\n> +\t\t\titem->util = DELETE_PACK;\n> +\t\t}\n\nI find the behaviour of this function a tad surprising as it doesn't\nonly mark a pack for deletion, but it also marks a pack as not being\nretained anymore. Shouldn't we rather:\n\n if (pack_is_retained(item)) {\n // Theoretically speaking we shouldn't even do this bit here as\n // we _un_mark the pack for deletion. But at least we shouldn't\n // be removing the `RETAIN_PACK` bit, I'd think.\n item->util &= ~DELETE_PACK; \n } else if (!string_list_has_string(names, sha1)) {\n // And here we shouldn't discard the `RETAIN_PACK` bit either.\n item->util |= DELETE_PACK;\n }\n\n> \t}\n> }\n> \n> +static void retain_cruft_pack(struct existing_packs *existing,\n> +\t\t\t struct packed_git *cruft)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct string_list_item *item;\n> +\n> +\tstrbuf_addstr(&buf, pack_basename(cruft));\n> +\tstrbuf_strip_suffix(&buf, \".pack\");\n> +\n> +\titem = string_list_lookup(&existing->cruft_packs, buf.buf);\n> +\tif (!item)\n> +\t\tBUG(\"could not find cruft pack '%s'\", pack_basename(cruft));\n> +\n> +\titem->util = (void*)RETAIN_PACK;\n> +\tstrbuf_release(&buf);\n> +}\n> +\n\nSimilarly, should we handle potentially pre-existing bits gracefully and\n`item->util |= RETAIN_PACK`?\n\n> static void mark_packs_for_deletion(struct existing_packs *existing,\n> \t\t\t\t struct string_list *names)\n> \n> @@ -217,6 +247,8 @@ static void collect_pack_filenames(struct existing_packs *existing,\n> \t}\n> \n> \tstring_list_sort(&existing->kept_packs);\n> +\tstring_list_sort(&existing->non_kept_packs);\n> +\tstring_list_sort(&existing->cruft_packs);\n> \tstrbuf_release(&buf);\n> }\n> \n> @@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include,\n> \tstrbuf_release(&path);\n> }\n> \n> +static int existing_cruft_pack_cmp(const void *va, const void *vb)\n> +{\n> +\tstruct packed_git *a = *(struct packed_git **)va;\n> +\tstruct packed_git *b = *(struct packed_git **)vb;\n> +\n> +\tif (a->pack_size < b->pack_size)\n> +\t\treturn -1;\n> +\tif (a->pack_size > b->pack_size)\n> +\t\treturn 1;\n> +\treturn 0;\n> +}\n> +\n> +static void collapse_small_cruft_packs(FILE *in, unsigned long max_size,\n\nWe might want to use `size_t` to denote file sizes instead of `unsigned\nlong`.\n\n> +\t\t\t\t struct existing_packs *existing)\n> +{\n> +\tstruct packed_git **existing_cruft, *p;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tunsigned long total_size = 0;\n\nHere, as well.\n\n> +\tsize_t existing_cruft_nr = 0;\n> +\tsize_t i;\n> +\n> +\tALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);\n> +\n> +\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> +\t\tif (!(p->is_cruft && p->pack_local))\n> +\t\t\tcontinue;\n> +\n> +\t\tstrbuf_reset(&buf);\n> +\t\tstrbuf_addstr(&buf, pack_basename(p));\n> +\t\tstrbuf_strip_suffix(&buf, \".pack\");\n> +\n> +\t\tif (!string_list_has_string(&existing->cruft_packs, buf.buf))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (existing_cruft_nr >= existing->cruft_packs.nr)\n> +\t\t\tBUG(\"too many cruft packs (found %\"PRIuMAX\", but knew \"\n> +\t\t\t \"of %\"PRIuMAX\")\",\n> +\t\t\t (uintmax_t)existing_cruft_nr + 1,\n> +\t\t\t (uintmax_t)existing->cruft_packs.nr);\n> +\t\texisting_cruft[existing_cruft_nr++] = p;\n> +\t}\n> +\n> +\tQSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);\n> +\n> +\tfor (i = 0; i < existing_cruft_nr; i++) {\n> +\t\toff_t proposed;\n\nThis should also be `size_t` given that `st_add` returns `size_t` and\nnot `off_t`.\n\n> +\t\tp = existing_cruft[i];\n> +\t\tproposed = st_add(total_size, p->pack_size);\n> +\n> +\t\tif (proposed <= max_size) {\n> +\t\t\ttotal_size = proposed;\n> +\t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n> +\t\t} else {\n> +\t\t\tretain_cruft_pack(existing, p);\n> +\t\t\tfprintf(in, \"%s\\n\", pack_basename(p));\n> +\t\t}\n\nIt's a bit funny that we re-check whether we have exceeded the maximum\nsize in subsequente iterations once we hit the limit, but it arguably\nmakes the logic a bit simpler.\n\n> +\t}\n> +\n> +\tfor (i = 0; i < existing->non_kept_packs.nr; i++)\n> +\t\tfprintf(in, \"-%s.pack\\n\",\n> +\t\t\texisting->non_kept_packs.items[i].string);\n> +\n> +\tstrbuf_release(&buf);\n> +}\n> +\n> static int write_cruft_pack(const struct pack_objects_args *args,\n> \t\t\t const char *destination,\n> \t\t\t const char *pack_prefix,\n> @@ -846,10 +944,18 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n> \tin = xfdopen(cmd.in, \"w\");\n> \tfor_each_string_list_item(item, names)\n> \t\tfprintf(in, \"%s-%s.pack\\n\", pack_prefix, item->string);\n> -\tfor_each_string_list_item(item, &existing->non_kept_packs)\n> -\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n> -\tfor_each_string_list_item(item, &existing->cruft_packs)\n> -\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n> +\tif (args->max_pack_size && !cruft_expiration) {\n> +\t\tunsigned long max_pack_size;\n> +\t\tif (!git_parse_ulong(args->max_pack_size, &max_pack_size))\n> +\t\t\treturn error(_(\"could not parse --cruft-max-size: '%s'\"),\n> +\t\t\t\t args->max_pack_size);\n> +\t\tcollapse_small_cruft_packs(in, max_pack_size, existing);\n> +\t} else {\n> +\t\tfor_each_string_list_item(item, &existing->non_kept_packs)\n> +\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n> +\t\tfor_each_string_list_item(item, &existing->cruft_packs)\n> +\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n> +\t}\n\nIf I understand correctly, we only collapse small cruft packs in case\nwe're not expiring any objects at the same time. Is there an inherent\nreason why? I would imagine that it can indeed be useful to expire\nobjects contained in cruft packs and then have git-repack(1) recombine\nwhatever is left into larger packs.\n\nIf the reason is basically \"it's complicated\" then that is fine with me,\nwe can still implement the functionality at a later point in time. But\nuntil then I think that we should let callers know that the two options\nare incompatible with each other by producing an error when both are\npassed.\n\nPatrick\n"},{"id":"482294","messageId":"ZRHK7+2Ta6U4CXuL@nand.local","threadId":"60206","inReplyTo":"xmqqtts5wnwn.fsf@gitster.g","subject":"Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-25T18:01:19Z","receivedAt":"2023-09-25T18:01:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 07, 2023 at 04:42:48PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > This all works, but can be costly from an I/O-perspective when a\n> > repository has either (a) many unreachable objects, (b) prunes objects\n> > relatively infrequently/never, or (c) both.\n>\n> I can certainly understand (a). If we need to write a lot of\n> objects into the craft pack, among which many of them are already in\n> the previous generation of craft pack, we would be copying bits from\n> the old to the new craft pack, and having to do so for many objects\n> would involve expensive I/O. But not (b). Whether we prune objects\n> infrequently or we do so very often, as long as the number and\n> on-disk size of objects that has to go into craft packs are the\n> same, wouldn't the cost of I/O pretty much the same? IOW, (b) does\n> not have much to do with how repacking is costly I/O wise, except\n> that it is a contributing factor to make (a) worse, which is the\n> real cause of the I/O cost.\n\nYeah, (b) on its own isn't enough to cost us a significant amount of I/O\noverhead. (b) definitely exacerbates (a) if we are repacking relatively\nfrequently. I'll clarify in the patch notes.\n\n> > This limits the I/O churn up to a quadratic function of the value\n> > specified by the `--cruft-max-size` option, instead of behaving\n> > quadratically in the number of total unreachable objects.\n>\n> ... I do not quite see how you would limit the I/O churn.\n\nIn the above (elided here for brevity) response, you are correct: the\nidea is that once a cruft pack has grown to whatever threshold you set\nvia `--cruft-max-size`, it is effectively frozen, preventing you from\nincurring the same I/O churn from it in the future.\n\n> > When pruning unreachable objects, we bypass the new paths which combine\n>\n> \"paths\" here refers to...? code paths, I guess?\n\nYep.\n\n> > +gc.cruftMaxSize::\n> > +\tLimit the size of new cruft packs when repacking. When\n> > +\tspecified in addition to `--cruft-max-size`, the command line\n> > +\toption takes priority. See the `--cruft-max-size` option of\n> > +\tlinkgit:git-repack[1].\n>\n> Hmph.\n>\n> I am reasonably sure that I will mix the name up and call it\n> gc.maxCruftSize in my configuration file and scratch my head\n> wondering why it is not working.\n\nI have no strong preference for either \"gc.cruftMaxSize\" or\n\"gc.maxCruftSize\" (or anything else, really), so long as we are\nconsistent with the command-line option.\n\n> > diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\n> > index 90806fd26a..8a90d684a7 100644\n> > --- a/Documentation/git-gc.txt\n> > +++ b/Documentation/git-gc.txt\n> > @@ -59,6 +59,13 @@ be performed as well.\n> > \tcruft pack instead of storing them as loose objects. `--cruft`\n> > \tis on by default.\n> >\n> > +--cruft-max-size=::\n> > +\tWhen packing unreachable objects into a cruft pack, limit the\n> > +\tsize of new cruft packs to be at most ``. Overrides any\n> > +\tvalue specified via the `gc.cruftMaxSize` configuration. See\n> > +\tthe `--cruft-max-size` option of linkgit:git-repack[1] for\n> > +\tmore.\n>\n> At least this side giving --max-cruft-size= (which I think is a\n> lot more natural word order) would cause parse-options to give an\n> error, so it won't risk mistakes go silently unnoticed.\n\nYeah, that's compelling. I'm convinced ;-).\n\n> > diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\n> > index 4017157949..23fd203d79 100644\n> > --- a/Documentation/git-repack.txt\n> > +++ b/Documentation/git-repack.txt\n> > @@ -74,6 +74,15 @@ to the new separate pack will be written.\n> > \timmediately instead of waiting for the next `git gc` invocation.\n> > \tOnly useful with `--cruft -d`.\n> >\n> > +--cruft-max-size=::\n> > +\tRepack cruft objects into packs as large as `` before\n> > +\tcreating new packs. As long as there are enough cruft packs\n> > +\tsmaller than ``, repacking will cause a new cruft pack to\n> > +\tbe created containing objects from any combined cruft packs,\n> > +\talong with any new unreachable objects. Cruft packs larger\n> > +\tthan `` will not be modified. Only useful with `--cruft\n> > +\t-d`.\n>\n> Here, the missing fourth point I pointed out above is mentioned,\n> which is good.\n>\n> Describe the unit for (I am assuming that is counted in bytes,\n> honoring the human-friendly suffix, like 100M).\n\nYep, thanks.\n\n> There may be some \"interesting\" behaviour around the size boundary,\n> no? If you pack too many objects, your resulting size may slightly\n> bust and you will get a complaint, but by fixing that \"bug\", you\n> will always stop short of filling the whole bytes in the\n> produced packfiles, and they will not be excempt from rewriting\n> (becuase they are not \"larger than \"), which defeats the point of\n> this patch.\n\nYeah, the boundary conditions are definitely the most interesting part\nof this patch IMHO. When packing with `--max-cruft-size`, we still do\ncap the pack size of the resulting pack, so any excess spills over into\nthe next pack.\n\n> Describe that is a threshold that we stop soon after passing to\n> explicitly allow us to go beyond it would solve the above problem, I\n> would presume.\n\nI am not sure I understand what you're getting at here.\n\n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > index 1f53b66c7b..b6640abd35 100644\n> > --- a/builtin/gc.c\n> > +++ b/builtin/gc.c\n> > @@ -52,6 +52,7 @@ static const char * const builtin_gc_usage[] = {\n> > static int pack_refs = 1;\n> > static int prune_reflogs = 1;\n> > static int cruft_packs = 1;\n> > +static char *cruft_max_size;\n>\n> I do not think this type is a good idea.\n\nYeah, I marked this as a string because we don't ourselves do anything\nwith it in 'gc', and instead just immediately pass it down to 'repack'.\nWe could parse it ourselves and catch any malformed arguments earlier,\nthough, which sounds worthwhile to me.\n\n> > diff --git a/builtin/repack.c b/builtin/repack.c\n> > index 44cb261371..56e7f5f43d 100644\n> > --- a/builtin/repack.c\n> > +++ b/builtin/repack.c\n> > @@ -26,6 +26,9 @@\n> > #define LOOSEN_UNREACHABLE 2\n> > #define PACK_CRUFT 4\n> >\n> > +#define DELETE_PACK ((void*)(uintptr_t)1)\n> > +#define RETAIN_PACK ((uintptr_t)(1<<1))\n>\n> Shouldn't these look more similar? That is\n>\n> \t((void *)(uintptr_t)(1<<0))\n> \t((void *)(uintptr_t)(1<<1))\n\nYeah, these have been shored up from some helpful review on the repack\ninternals cleanup series I posted earlier.\n\n> > +\t\tif (existing_cruft_nr >= existing->cruft_packs.nr)\n> > +\t\t\tBUG(\"too many cruft packs (found %\"PRIuMAX\", but knew \"\n> > +\t\t\t \"of %\"PRIuMAX\")\",\n>\n> Is that a BUG() that somehow miscounted the packs, or can it be a\n> runtime error that may happen when a \"git push\" is pushing new\n> objects into the repository, creating a new pack we did not know\n> about? Something like the latter should not be marked a BUG(), but\n\nThis would be the former. We load the set of packs at the beginning of a\nrepack operation from collect_pack_filenames() via a call to\nget_all_packs(). So the set won't change between our view of it in\ncollect_pack_filenames() and collapse_small_cruft_packs(). IOW, we\nshould expect to see as many cruft packs as we saw in\ncollect_pack_filenames(), and any difference there would indeed be a\nBUG().\n\n> > +\t\t\t (uintmax_t)existing_cruft_nr + 1,\n> > +\t\t\t (uintmax_t)existing->cruft_packs.nr);\n> > +\t\texisting_cruft[existing_cruft_nr++] = p;\n> > +\t}\n> > +\n> > +\tQSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);\n>\n> We use the simplest \"from smaller ones to larger ones, combine one\n> by one together until the result gets large enough\", which would not\n> give us the best packing, but it is OK because it is not our goal to\n> solve knapsack problem here, I presume?\n\nExactly. Whatever spill-over we generate (if any) will be the seed of\nthe next \"generation\" of cruft packs, which are allowed to grow and\naccumulate until they themselves reach the size threshold, at which\npoint the process starts itself over again.\n\n> > +\tfor (i = 0; i < existing_cruft_nr; i++) {\n> > +\t\toff_t proposed;\n> > +\n> > +\t\tp = existing_cruft[i];\n> > +\t\tproposed = st_add(total_size, p->pack_size);\n> > +\n> > +\t\tif (proposed <= max_size) {\n> > +\t\t\ttotal_size = proposed;\n> > +\t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n> > +\t\t} else {\n> > +\t\t\tretain_cruft_pack(existing, p);\n> > +\t\t\tfprintf(in, \"%s\\n\", pack_basename(p));\n> > +\t\t}\n> > +\t}\n>\n> This is exactly what I talked about the possibly funny behaviour\n> around the boundary earlier, but it may be even worse. This time,\n> we may decide that a pack with size is to be retained,\n> only because the pack that came next in the existing list happened\n> to be larger than epsilon, but next time around, it may not be the\n> case (i.e. our pack may be smaller than epsilon, the next one in the\n> current round may be larger than epsilon, but before we repack the\n> next time, a new pack that is slightly smaller than epsilon that is\n> larger than our pack may have been created---now our pack will be\n> combined with it), so the algorithm to choose which ones are kept\n> does not depend on the pack itself alone but also depends on its\n> surroundings.\n\nIf I understand your comment correctly, that behavior is as-designed. We\ntry to grow cruft packs by combining other cruft packs that are not yet\nfrozen, and I think that is going to be dependent on the entire set of\npacks rather than the characteristic of any one pack.\n\nAn alternative approach might be to combine *all* cruft packs smaller\nthan some threshold, and let any spill-over get handled by pack-objects\nwith its --max-pack-size option. I do not have a strong preference\nbetween the two.\n\nThanks,\nTaylor\n"},{"id":"482545","messageId":"ZRsogqtbd6klqypL@nand.local","threadId":"60206","inReplyTo":"ZPsDyKOa76Mxb9u-@tanuki","subject":"Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-02T20:30:58Z","receivedAt":"2023-10-02T20:31:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 08, 2023 at 01:21:44PM +0200, Patrick Steinhardt wrote:\n> On Thu, Sep 07, 2023 at 05:52:04PM -0400, Taylor Blau wrote:\n> [snip]\n> > @@ -125,17 +133,39 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n> > \t\tif (len < hexsz)\n> > \t\t\tcontinue;\n> > \t\tsha1 = item->string + len - hexsz;\n> > -\t\t/*\n> > -\t\t * Mark this pack for deletion, which ensures that this\n> > -\t\t * pack won't be included in a MIDX (if `--write-midx`\n> > -\t\t * was given) and that we will actually delete this pack\n> > -\t\t * (if `-d` was given).\n> > -\t\t */\n> > -\t\tif (!string_list_has_string(names, sha1))\n> > -\t\t\titem->util = (void*)1;\n> > +\n> > +\t\tif (pack_is_retained(item)) {\n> > +\t\t\titem->util = NULL;\n> > +\t\t} else if (!string_list_has_string(names, sha1)) {\n> > +\t\t\t/*\n> > +\t\t\t * Mark this pack for deletion, which ensures\n> > +\t\t\t * that this pack won't be included in a MIDX\n> > +\t\t\t * (if `--write-midx` was given) and that we\n> > +\t\t\t * will actually delete this pack (if `-d` was\n> > +\t\t\t * given).\n> > +\t\t\t */\n> > +\t\t\titem->util = DELETE_PACK;\n> > +\t\t}\n>\n> I find the behaviour of this function a tad surprising as it doesn't\n> only mark a pack for deletion, but it also marks a pack as not being\n> retained anymore. Shouldn't we rather:\n>\n> if (pack_is_retained(item)) {\n> // Theoretically speaking we shouldn't even do this bit here as\n> // we _un_mark the pack for deletion. But at least we shouldn't\n> // be removing the `RETAIN_PACK` bit, I'd think.\n> item->util &= ~DELETE_PACK;\n> } else if (!string_list_has_string(names, sha1)) {\n> // And here we shouldn't discard the `RETAIN_PACK` bit either.\n> item->util |= DELETE_PACK;\n> }\n\nI think the new version should address these issues. But yeah, I\ndefinitely understand your confusion here. I think what's written in\nthis patch is OK, because we check only whether the `->util` field is\nnon-NULL before deleting, which is why we have to remove the RETAINED\nbit.\n\nBut the new version looks like this instead:\n\n if (pack_is_retained(item))\n pack_unmark_for_deletion(item);\n else if (!string_list_has_string(names, sha1))\n pack_mark_for_deletion(item);\n\nthe RETAINED bits still stick around (pack_unmark_for_deletion() just\ndoes `item->util &= ~DELETE_PACK`), but we don't consult them after\nmark_packs_for_deletion_1() has finished executing. Instead we just\ncheck for the existence of the DELETE_PACK bit, rather than whether or\nnot the whole util field is NULL.\n\n> > @@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include,\n> > \tstrbuf_release(&path);\n> > }\n> >\n> > +static int existing_cruft_pack_cmp(const void *va, const void *vb)\n> > +{\n> > +\tstruct packed_git *a = *(struct packed_git **)va;\n> > +\tstruct packed_git *b = *(struct packed_git **)vb;\n> > +\n> > +\tif (a->pack_size < b->pack_size)\n> > +\t\treturn -1;\n> > +\tif (a->pack_size > b->pack_size)\n> > +\t\treturn 1;\n> > +\treturn 0;\n> > +}\n> > +\n> > +static void collapse_small_cruft_packs(FILE *in, unsigned long max_size,\n>\n> We might want to use `size_t` to denote file sizes instead of `unsigned\n> long`.\n\nWe can safely change these to use size_t, but let's leave OPT_MAGNITUDE\nalone (and treat that portion as #leftoverbits).\n\n> > +\t\tp = existing_cruft[i];\n> > +\t\tproposed = st_add(total_size, p->pack_size);\n> > +\n> > +\t\tif (proposed <= max_size) {\n> > +\t\t\ttotal_size = proposed;\n> > +\t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n> > +\t\t} else {\n> > +\t\t\tretain_cruft_pack(existing, p);\n> > +\t\t\tfprintf(in, \"%s\\n\", pack_basename(p));\n> > +\t\t}\n>\n> It's a bit funny that we re-check whether we have exceeded the maximum\n> size in subsequente iterations once we hit the limit, but it arguably\n> makes the logic a bit simpler.\n\nYeah. Those checks are all noops (IOW, once we end up in the else\nbranch, we'll stay there for the rest of the loop). But we don't want to\nbreak early, because we have to call retain_cruft_pack() on everything.\nIn theory you could do something like:\n\n for (i = 0; i < existing_cruft_nr; i++) {\n size_t proposed;\n p = existing_cruft[i];\n proposed = st_add(total_size, p->pack_size);\n\n if (proposed <= max_size) {\n total_size = proposed;\n fprintf(in, \"-%s\\n\", pack_basename(p));\n } else {\n break;\n }\n }\n\n for (; i < existing_cruft_nr; i++) {\n retain_cruft_pack(existing, existing_cruft[i]);\n fprintf(in, \"%s\\n\", pack_basename(existing_cruft[i]));\n }\n\nBut I think that the above is slightly more error-prone than what is\nwritten in the original patch. I have only the vaguest of preferences\ntowards the former, but I'm happy to change it around if you feel\nstrongly.\n\n> If I understand correctly, we only collapse small cruft packs in case\n> we're not expiring any objects at the same time. Is there an inherent\n> reason why? I would imagine that it can indeed be useful to expire\n> objects contained in cruft packs and then have git-repack(1) recombine\n> whatever is left into larger packs.\n>\n> If the reason is basically \"it's complicated\" then that is fine with me,\n> we can still implement the functionality at a later point in time. But\n> until then I think that we should let callers know that the two options\n> are incompatible with each other by producing an error when both are\n> passed.\n\nYour understanding is correct. We could try to leave existing cruft\npacks alone when none of their objects are removed as a result of\npruning, but that case should be relatively rare. Another thing you\ncould do is handle cruft packs which have only part of their objects\nbeing pruned by combining the non-pruned parts into a new pack.\n\nThe latter should be mostly straightforward to implement, but since\nwe're often ending up with very few cruft objects post-pruning, it\nlikely wouldn't help much.\n\nThanks,\nTaylor\n"},{"id":"482548","messageId":"cover.1696293862.git.me@ttaylorr.com","threadId":"60206","inReplyTo":"cover.1694123506.git.me@ttaylorr.com","subject":"[PATCH v2 0/3] repack: implement `--cruft-max-size`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-03T00:44:23Z","receivedAt":"2023-10-03T00:44:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"(The earlier round of these patches depended on a couple of in-flight\ntopics, but this series has been rebased onto the tip of 'master').\n\nThis is a reroll of my series that introduces more ergonomic options for\ncreating and dealing with multiple cruft packs.\n\nMuch is unchanged since last time, but some notable changes that have\ntaken place include:\n\n - adding a missing commit message / s-o-b to the first patch to split\n cruft-related tests out of t7700 into t7704.\n\n - renamed the option to `--max-cruft-size` (instead of\n `--cruft-max-size`), and made corresponding changes to the\n documentation, configuration options, tests, etc.\n\n - cleaned up our handling of how we mark packs to be deleted and\n retained\n\n - parsed `--max-cruft-size` as an unsigned long up front instead of\n blindly passing a char * down to pack-objects to parse it for us.\n\nThe last of those also affects `--max-pack-size`, which caused this\nseries to grow a new second patch which cleans up the existing code\nwhich suffers from the same issue.\n\nAs usual, a range-diff is available below. Thanks for all of the helpful\nreview on the earlier round, and thanks in advance for your review on\nthis one!\n\nTaylor Blau (3):\n t7700: split cruft-related tests to t7704\n builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE\n builtin/repack.c: implement support for `--max-cruft-size`\n\n Documentation/config/gc.txt | 6 +\n Documentation/git-gc.txt | 7 +\n Documentation/git-repack.txt | 11 +\n builtin/gc.c | 7 +\n builtin/repack.c | 140 +++++++++++--\n t/t6500-gc.sh | 27 +++\n t/t7700-repack.sh | 121 -----------\n t/t7704-repack-cruft.sh | 375 +++++++++++++++++++++++++++++++++++\n 8 files changed, 559 insertions(+), 135 deletions(-)\n create mode 100755 t/t7704-repack-cruft.sh\n\nRange-diff against v1:\n 1: 103e19c75a < -: ---------- builtin/repack.c: extract structure to store existing packs\n 2: 45d5f15308 < -: ---------- builtin/repack.c: extract marking packs for deletion\n 3: 30eccbfdcb < -: ---------- builtin/repack.c: extract redundant pack cleanup for --geometric\n 4: 57e322301d < -: ---------- builtin/repack.c: extract redundant pack cleanup for existing packs\n 5: 4df4a2e51e < -: ---------- builtin/repack.c: extract `has_existing_non_kept_packs()`\n 6: 360be582b4 < -: ---------- builtin/repack.c: store existing cruft packs separately\n 7: ef9c8d775b < -: ---------- builtin/repack.c: drop `DELETE_PACK` macro\n 8: 873bc16abb < -: ---------- builtin/repack.c: extract common cruft pack loop\n 9: de6c2a0d70 ! 1: 3ed4ab61f6 t7700: split cruft-related tests to t7704\n @@ Metadata\n ## Commit message ##\n t7700: split cruft-related tests to t7704\n \n + A small handful of the tests in t7700 (the main script for testing\n + functionality of 'git repack') are specifically related to cruft pack\n + operations.\n +\n + Prepare for adding new cruft pack-related tests by moving the existing\n + set into a new test script.\n +\n + Signed-off-by: Taylor Blau \n +\n ## t/t7700-repack.sh ##\n @@ t/t7700-repack.sh: test_expect_success '-n overrides repack.updateServerInfo=true' '\n \ttest_server_info_missing\n -: ---------- > 2: 9ec999882d builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE\n10: 7e4e42e1aa ! 3: e7beb2060d builtin/repack.c: implement support for `--cruft-max-size`\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - builtin/repack.c: implement support for `--cruft-max-size`\n + builtin/repack.c: implement support for `--max-cruft-size`\n \n Cruft packs are an alternative mechanism for storing a collection of\n unreachable objects whose mtimes are recent enough to avoid being\n @@ Commit message\n (for more, see some of the details in 3d89a8c118\n (Documentation/technical: add cruft-packs.txt, 2022-05-20)).\n \n - This all works, but can be costly from an I/O-perspective when a\n - repository has either (a) many unreachable objects, (b) prunes objects\n - relatively infrequently/never, or (c) both.\n + This all works, but can be costly from an I/O-perspective when\n + frequently repacking a repository that has many unreachable objects.\n + This problem is exacerbated when those unreachable objects are rarely\n + (if every) pruned.\n \n Since there is at most one cruft pack in the above scheme, each time we\n update the cruft pack it must be rewritten from scratch. Because much of\n @@ Commit message\n multiple cruft packs. This patch implements that support which we were\n lacking.\n \n - Introduce a new option `--cruft-max-size` which allows repositories to\n + Introduce a new option `--max-cruft-size` which allows repositories to\n accumulate cruft packs up to a given size, after which point a new\n generation of cruft packs can accumulate until it reaches the maximum\n size, and so on. To generate a new cruft pack, the process works like\n @@ Commit message\n pack, along with any other unreachable objects which have since\n entered the repository.\n \n - This limits the I/O churn up to a quadratic function of the value\n - specified by the `--cruft-max-size` option, instead of behaving\n - quadratically in the number of total unreachable objects.\n + Once a cruft pack grows beyond the size specified via `--max-cruft-size`\n + the pack is effectively frozen. This limits the I/O churn up to a\n + quadratic function of the value specified by the `--max-cruft-size`\n + option, instead of behaving quadratically in the number of total\n + unreachable objects.\n \n - When pruning unreachable objects, we bypass the new paths which combine\n - small cruft packs together, and instead start from scratch, passing in\n - the appropriate `--max-pack-size` down to `pack-objects`, putting it in\n - charge of keeping the resulting set of cruft packs sized correctly.\n + When pruning unreachable objects, we bypass the new code paths which\n + combine small cruft packs together, and instead start from scratch,\n + passing in the appropriate `--max-pack-size` down to `pack-objects`,\n + putting it in charge of keeping the resulting set of cruft packs sized\n + correctly.\n \n This may seem like further I/O churn, but in practice it isn't so bad.\n We could prune old cruft packs for whom all or most objects are removed,\n @@ Documentation/config/gc.txt: gc.cruftPacks::\n \tlinkgit:git-repack[1]) instead of as loose objects. The default\n \tis `true`.\n \n -+gc.cruftMaxSize::\n ++gc.maxCruftSize::\n +\tLimit the size of new cruft packs when repacking. When\n -+\tspecified in addition to `--cruft-max-size`, the command line\n -+\toption takes priority. See the `--cruft-max-size` option of\n ++\tspecified in addition to `--max-cruft-size`, the command line\n ++\toption takes priority. See the `--max-cruft-size` option of\n +\tlinkgit:git-repack[1].\n +\n gc.pruneExpire::\n @@ Documentation/git-gc.txt: be performed as well.\n \tcruft pack instead of storing them as loose objects. `--cruft`\n \tis on by default.\n \n -+--cruft-max-size=::\n ++--max-cruft-size=::\n +\tWhen packing unreachable objects into a cruft pack, limit the\n +\tsize of new cruft packs to be at most ``. Overrides any\n -+\tvalue specified via the `gc.cruftMaxSize` configuration. See\n -+\tthe `--cruft-max-size` option of linkgit:git-repack[1] for\n ++\tvalue specified via the `gc.maxCruftSize` configuration. See\n ++\tthe `--max-cruft-size` option of linkgit:git-repack[1] for\n +\tmore.\n +\n --prune=::\n @@ Documentation/git-repack.txt: to the new separate pack will be written.\n \timmediately instead of waiting for the next `git gc` invocation.\n \tOnly useful with `--cruft -d`.\n \n -+--cruft-max-size=::\n -+\tRepack cruft objects into packs as large as `` before\n ++--max-cruft-size=::\n ++\tRepack cruft objects into packs as large as `` bytes before\n +\tcreating new packs. As long as there are enough cruft packs\n +\tsmaller than ``, repacking will cause a new cruft pack to\n +\tbe created containing objects from any combined cruft packs,\n -+\talong with any new unreachable objects. Cruft packs larger\n -+\tthan `` will not be modified. Only useful with `--cruft\n -+\t-d`.\n ++\talong with any new unreachable objects. Cruft packs larger than\n ++\t`` will not be modified. When the new cruft pack is larger\n ++\tthan `` bytes, it will be split into multiple packs, all of\n ++\twhich are guaranteed to be at most `` bytes in size. Only\n ++\tuseful with `--cruft -d`.\n +\n --expire-to=::\n \tWrite a cruft pack containing pruned objects (if any) to the\n @@ builtin/gc.c: static const char * const builtin_gc_usage[] = {\n static int pack_refs = 1;\n static int prune_reflogs = 1;\n static int cruft_packs = 1;\n -+static char *cruft_max_size;\n ++static unsigned long max_cruft_size;\n static int aggressive_depth = 50;\n static int aggressive_window = 250;\n static int gc_auto_threshold = 6700;\n @@ builtin/gc.c: static void gc_config(void)\n \tgit_config_get_int(\"gc.autopacklimit\", &gc_auto_pack_limit);\n \tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n \tgit_config_get_bool(\"gc.cruftpacks\", &cruft_packs);\n -+\tgit_config_get_string(\"gc.cruftmaxsize\", &cruft_max_size);\n ++\tgit_config_get_ulong(\"gc.maxcruftsize\", &max_cruft_size);\n \tgit_config_get_expiry(\"gc.pruneexpire\", &prune_expire);\n \tgit_config_get_expiry(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\n \tgit_config_get_expiry(\"gc.logexpiry\", &gc_log_expire);\n @@ builtin/gc.c: static void add_repack_all_option(struct string_list *keep_pack)\n \t\tstrvec_push(&repack, \"--cruft\");\n \t\tif (prune_expire)\n \t\t\tstrvec_pushf(&repack, \"--cruft-expiration=%s\", prune_expire);\n -+\t\tif (cruft_max_size)\n -+\t\t\tstrvec_pushf(&repack, \"--cruft-max-size=%s\",\n -+\t\t\t\t cruft_max_size);\n ++\t\tif (max_cruft_size)\n ++\t\t\tstrvec_pushf(&repack, \"--max-cruft-size=%lu\",\n ++\t\t\t\t max_cruft_size);\n \t} else {\n \t\tstrvec_push(&repack, \"-A\");\n \t\tif (prune_expire)\n @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"prune unreferenced objects\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },\n \t\tOPT_BOOL(0, \"cruft\", &cruft_packs, N_(\"pack unreferenced objects separately\")),\n -+\t\tOPT_STRING(0, \"cruft-max-size\", &cruft_max_size,\n -+\t\t\t N_(\"bytes\"),\n -+\t\t\t N_(\"with --cruft, limit the size of new cruft packs\")),\n ++\t\tOPT_MAGNITUDE(0, \"max-cruft-size\", &max_cruft_size,\n ++\t\t\t N_(\"with --cruft, limit the size of new cruft packs\")),\n \t\tOPT_BOOL(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n \t\tOPT_BOOL_F(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\"),\n \t\t\t PARSE_OPT_NOCOMPLETE),\n \n ## builtin/repack.c ##\n @@\n - #define LOOSEN_UNREACHABLE 2\n #define PACK_CRUFT 4\n \n -+#define DELETE_PACK ((void*)(uintptr_t)1)\n -+#define RETAIN_PACK ((uintptr_t)(1<<1))\n -+\n + #define DELETE_PACK 1\n ++#define RETAIN_PACK 2\n + \n static int pack_everything;\n static int delta_base_offset = 1;\n - static int pack_kept_objects = -1;\n -@@ builtin/repack.c: static int has_existing_non_kept_packs(const struct existing_packs *existing)\n - \treturn existing->non_kept_packs.nr || existing->cruft_packs.nr;\n +@@ builtin/repack.c: static void pack_mark_for_deletion(struct string_list_item *item)\n + \titem->util = (void*)((uintptr_t)item->util | DELETE_PACK);\n }\n \n ++static void pack_unmark_for_deletion(struct string_list_item *item)\n ++{\n ++\titem->util = (void*)((uintptr_t)item->util & ~DELETE_PACK);\n ++}\n ++\n + static int pack_is_marked_for_deletion(struct string_list_item *item)\n + {\n + \treturn (uintptr_t)item->util & DELETE_PACK;\n + }\n + \n ++static void pack_mark_retained(struct string_list_item *item)\n ++{\n ++\titem->util = (void*)((uintptr_t)item->util | RETAIN_PACK);\n ++}\n ++\n +static int pack_is_retained(struct string_list_item *item)\n +{\n +\treturn (uintptr_t)item->util & RETAIN_PACK;\n @@ builtin/repack.c: static void mark_packs_for_deletion_1(struct string_list *name\n -\t\t * (if `-d` was given).\n -\t\t */\n -\t\tif (!string_list_has_string(names, sha1))\n --\t\t\titem->util = (void*)1;\n +\n +\t\tif (pack_is_retained(item)) {\n -+\t\t\titem->util = NULL;\n ++\t\t\tpack_unmark_for_deletion(item);\n +\t\t} else if (!string_list_has_string(names, sha1)) {\n +\t\t\t/*\n +\t\t\t * Mark this pack for deletion, which ensures\n @@ builtin/repack.c: static void mark_packs_for_deletion_1(struct string_list *name\n +\t\t\t * will actually delete this pack (if `-d` was\n +\t\t\t * given).\n +\t\t\t */\n -+\t\t\titem->util = DELETE_PACK;\n + \t\t\tpack_mark_for_deletion(item);\n +\t\t}\n \t}\n }\n @@ builtin/repack.c: static void mark_packs_for_deletion_1(struct string_list *name\n +\tif (!item)\n +\t\tBUG(\"could not find cruft pack '%s'\", pack_basename(cruft));\n +\n -+\titem->util = (void*)RETAIN_PACK;\n ++\tpack_mark_retained(item);\n +\tstrbuf_release(&buf);\n +}\n +\n @@ builtin/repack.c: static void remove_redundant_bitmaps(struct string_list *inclu\n +\treturn 0;\n +}\n +\n -+static void collapse_small_cruft_packs(FILE *in, unsigned long max_size,\n ++static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n +\t\t\t\t struct existing_packs *existing)\n +{\n +\tstruct packed_git **existing_cruft, *p;\n +\tstruct strbuf buf = STRBUF_INIT;\n -+\tunsigned long total_size = 0;\n ++\tsize_t total_size = 0;\n +\tsize_t existing_cruft_nr = 0;\n +\tsize_t i;\n +\n @@ builtin/repack.c: static void remove_redundant_bitmaps(struct string_list *inclu\n +\tQSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);\n +\n +\tfor (i = 0; i < existing_cruft_nr; i++) {\n -+\t\toff_t proposed;\n ++\t\tsize_t proposed;\n +\n +\t\tp = existing_cruft[i];\n +\t\tproposed = st_add(total_size, p->pack_size);\n @@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *ar\n -\tfor_each_string_list_item(item, &existing->cruft_packs)\n -\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n +\tif (args->max_pack_size && !cruft_expiration) {\n -+\t\tunsigned long max_pack_size;\n -+\t\tif (!git_parse_ulong(args->max_pack_size, &max_pack_size))\n -+\t\t\treturn error(_(\"could not parse --cruft-max-size: '%s'\"),\n -+\t\t\t\t args->max_pack_size);\n -+\t\tcollapse_small_cruft_packs(in, max_pack_size, existing);\n ++\t\tcollapse_small_cruft_packs(in, args->max_pack_size, existing);\n +\t} else {\n +\t\tfor_each_string_list_item(item, &existing->non_kept_packs)\n +\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n \t\t\t\t PACK_CRUFT),\n \t\tOPT_STRING(0, \"cruft-expiration\", &cruft_expiration, N_(\"approxidate\"),\n \t\t\t\tN_(\"with --cruft, expire objects older than this\")),\n -+\t\tOPT_STRING(0, \"cruft-max-size\", &cruft_po_args.max_pack_size,\n -+\t\t\t\tN_(\"bytes\"),\n ++\t\tOPT_MAGNITUDE(0, \"max-cruft-size\", &cruft_po_args.max_pack_size,\n +\t\t\t\tN_(\"with --cruft, limit the size of new cruft packs\")),\n \t\tOPT_BOOL('d', NULL, &delete_redundant,\n \t\t\t\tN_(\"remove redundant packs, and run git-prune-packed\")),\n @@ t/t6500-gc.sh: test_expect_success 'gc.bigPackThreshold ignores cruft packs' '\n \n +cruft_max_size_opts=\"git repack -d -l --cruft --cruft-expiration=2.weeks.ago\"\n +\n -+test_expect_success 'setup for --cruft-max-size tests' '\n ++test_expect_success 'setup for --max-cruft-size tests' '\n +\tgit init cruft--max-size &&\n +\t(\n +\t\tcd cruft--max-size &&\n @@ t/t6500-gc.sh: test_expect_success 'gc.bigPackThreshold ignores cruft packs' '\n +\t)\n +'\n +\n -+test_expect_success '--cruft-max-size sets appropriate repack options' '\n ++test_expect_success '--max-cruft-size sets appropriate repack options' '\n +\tGIT_TRACE2_EVENT=$(pwd)/trace2.txt git -C cruft--max-size \\\n -+\t\tgc --cruft --cruft-max-size=1M &&\n -+\ttest_subcommand $cruft_max_size_opts --cruft-max-size=1M foo.objects &&\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t)\n +'\n +\n -+test_expect_success '--cruft-max-size combines existing packs when below threshold' '\n -+\tgit init cruft-max-size-small &&\n ++test_expect_success '--max-cruft-size combines existing packs when below threshold' '\n ++\tgit init max-cruft-size-small &&\n +\t(\n -+\t\tcd cruft-max-size-small &&\n ++\t\tcd max-cruft-size-small &&\n +\t\ttest_commit base &&\n +\n +\t\tfoo=\"$(pack_random_blob foo $((1*1024*1024)))\" &&\n +\t\tgit repack --cruft -d &&\n +\n +\t\tbar=\"$(pack_random_blob bar $((1*1024*1024)))\" &&\n -+\t\tgit repack --cruft -d --cruft-max-size=10M &&\n ++\t\tgit repack --cruft -d --max-cruft-size=10M &&\n +\n +\t\tcruft=$(ls $packdir/pack-*.mtimes) &&\n +\t\ttest-tool pack-mtimes $(basename \"$cruft\") >cruft.objects &&\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t)\n +'\n +\n -+test_expect_success '--cruft-max-size combines smaller packs first' '\n -+\tgit init cruft-max-size-consume-small &&\n ++test_expect_success '--max-cruft-size combines smaller packs first' '\n ++\tgit init max-cruft-size-consume-small &&\n +\t(\n -+\t\tcd cruft-max-size-consume-small &&\n ++\t\tcd max-cruft-size-consume-small &&\n +\n +\t\ttest_commit base &&\n +\t\tgit repack -ad &&\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t\ttest-tool pack-mtimes \"$(basename $cruft_bar)\" >>expect.raw &&\n +\t\tsort expect.raw >expect.objects &&\n +\n -+\t\t# repacking with `--cruft-max-size=2M` should combine\n ++\t\t# repacking with `--max-cruft-size=2M` should combine\n +\t\t# both 0.5 MiB packs together, instead of, say, one of\n +\t\t# the 0.5 MiB packs with the 1.0 MiB pack\n +\t\tls $packdir/pack-*.mtimes | sort >cruft.before &&\n -+\t\tgit repack -d --cruft --cruft-max-size=2M &&\n ++\t\tgit repack -d --cruft --max-cruft-size=2M &&\n +\t\tls $packdir/pack-*.mtimes | sort >cruft.after &&\n +\n +\t\tcomm -13 cruft.before cruft.after >cruft.new &&\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t)\n +'\n +\n -+test_expect_success 'setup --cruft-max-size with freshened objects' '\n -+\tgit init cruft-max-size-freshen &&\n ++test_expect_success 'setup --max-cruft-size with freshened objects' '\n ++\tgit init max-cruft-size-freshen &&\n +\t(\n -+\t\tcd cruft-max-size-freshen &&\n ++\t\tcd max-cruft-size-freshen &&\n +\n +\t\ttest_commit base &&\n +\t\tgit repack -ad &&\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t)\n +'\n +\n -+test_expect_success '--cruft-max-size with freshened objects (loose)' '\n ++test_expect_success '--max-cruft-size with freshened objects (loose)' '\n +\t(\n -+\t\tcd cruft-max-size-freshen &&\n ++\t\tcd max-cruft-size-freshen &&\n +\n +\t\t# regenerate the object, setting its mtime to be more recent\n +\t\tfoo=\"$(generate_random_blob foo 64)\" &&\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t)\n +'\n +\n -+test_expect_success '--cruft-max-size with freshened objects (packed)' '\n ++test_expect_success '--max-cruft-size with freshened objects (packed)' '\n +\t(\n -+\t\tcd cruft-max-size-freshen &&\n ++\t\tcd max-cruft-size-freshen &&\n +\n +\t\t# regenerate the object and store it in a packfile,\n +\t\t# setting its mtime to be more recent\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t)\n +'\n +\n -+test_expect_success '--cruft-max-size with pruning' '\n -+\tgit init cruft-max-size-prune &&\n ++test_expect_success '--max-cruft-size with pruning' '\n ++\tgit init max-cruft-size-prune &&\n +\t(\n -+\t\tcd cruft-max-size-prune &&\n ++\t\tcd max-cruft-size-prune &&\n +\n +\t\ttest_commit base &&\n +\t\tfoo=\"$(generate_random_blob foo $((1024*1024)))\" &&\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\n +\t\ttest-tool chmtime -10000 \"$objdir/$(test_oid_to_path \"$foo\")\" &&\n +\n -+\t\tgit repack -d --cruft --cruft-max-size=1M &&\n ++\t\tgit repack -d --cruft --max-cruft-size=1M &&\n +\n +\t\t# backdate the mtimes of all cruft packs to validate\n +\t\t# that they were rewritten as a result of pruning\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t\t\techo $cruft $mtime >>mtimes || return 1\n +\t\tdone &&\n +\n -+\t\t# repack (and prune) with a --cruft-max-size to ensure\n ++\t\t# repack (and prune) with a --max-cruft-size to ensure\n +\t\t# that we appropriately split the resulting set of packs\n -+\t\tgit repack -d --cruft --cruft-max-size=1M \\\n ++\t\tgit repack -d --cruft --max-cruft-size=1M \\\n +\t\t\t--cruft-expiration=10.seconds.ago &&\n +\t\tls $packdir/pack-*.mtimes | sort >cruft.after &&\n +\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t)\n +'\n +\n -+test_expect_success '--cruft-max-size ignores non-local packs' '\n -+\trepo=\"cruft-max-size-non-local\" &&\n ++test_expect_success '--max-cruft-size ignores non-local packs' '\n ++\trepo=\"max-cruft-size-non-local\" &&\n +\tgit init $repo &&\n +\t(\n +\t\tcd $repo &&\n @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects\n +\t\t# ensure that we do not attempt to pick up packs from\n +\t\t# the non-alternated repository, which would result in a\n +\t\t# crash\n -+\t\tgit repack --cruft --cruft-max-size=1M -d\n ++\t\tgit repack --cruft --max-cruft-size=1M -d\n +\t)\n +'\n +\n-- \n2.42.0.310.g9604b54f73.dirty\n"},{"id":"482549","messageId":"3ed4ab61f6aa260fe96f624f62d0d917c0a30761.1696293862.git.me@ttaylorr.com","threadId":"60206","inReplyTo":"cover.1696293862.git.me@ttaylorr.com","subject":"[PATCH v2 1/3] t7700: split cruft-related tests to t7704","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-03T00:44:26Z","receivedAt":"2023-10-03T00:44:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"A small handful of the tests in t7700 (the main script for testing\nfunctionality of 'git repack') are specifically related to cruft pack\noperations.\n\nPrepare for adding new cruft pack-related tests by moving the existing\nset into a new test script.\n\nSigned-off-by: Taylor Blau \n---\n t/t7700-repack.sh | 121 -------------------------------------\n t/t7704-repack-cruft.sh | 130 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 130 insertions(+), 121 deletions(-)\n create mode 100755 t/t7704-repack-cruft.sh\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 27b66807cd..55ee3eb8ae 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -633,125 +633,4 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n \ttest_server_info_missing\n '\n \n-test_expect_success '--expire-to stores pruned objects (now)' '\n-\tgit init expire-to-now &&\n-\t(\n-\t\tcd expire-to-now &&\n-\n-\t\tgit branch -M main &&\n-\n-\t\ttest_commit base &&\n-\n-\t\tgit checkout -b cruft &&\n-\t\ttest_commit --no-tag cruft &&\n-\n-\t\tgit rev-list --objects --no-object-names main..cruft >moved.raw &&\n-\t\tsort moved.raw >moved.want &&\n-\n-\t\tgit rev-list --all --objects --no-object-names >expect.raw &&\n-\t\tsort expect.raw >expect &&\n-\n-\t\tgit checkout main &&\n-\t\tgit branch -D cruft &&\n-\t\tgit reflog expire --all --expire=all &&\n-\n-\t\tgit init --bare expired.git &&\n-\t\tgit repack -d \\\n-\t\t\t--cruft --cruft-expiration=\"now\" \\\n-\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n-\n-\t\texpired=\"$(ls expired.git/objects/pack/pack-*.idx)\" &&\n-\t\ttest_path_is_file \"${expired%.idx}.mtimes\" &&\n-\n-\t\t# Since the `--cruft-expiration` is \"now\", the effective\n-\t\t# behavior is to move _all_ unreachable objects out to\n-\t\t# the location in `--expire-to`.\n-\t\tgit show-index <$expired >expired.raw &&\n-\t\tcut -d\" \" -f2 expired.raw | sort >expired.objects &&\n-\t\tgit rev-list --all --objects --no-object-names \\\n-\t\t\t>remaining.objects &&\n-\n-\t\t# ...in other words, the combined contents of this\n-\t\t# repository and expired.git should be the same as the\n-\t\t# set of objects we started with.\n-\t\tcat expired.objects remaining.objects | sort >actual &&\n-\t\ttest_cmp expect actual &&\n-\n-\t\t# The \"moved\" objects (i.e., those in expired.git)\n-\t\t# should be the same as the cruft objects which were\n-\t\t# expired in the previous step.\n-\t\ttest_cmp moved.want expired.objects\n-\t)\n-'\n-\n-test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' '\n-\tgit init expire-to-5.minutes.ago &&\n-\t(\n-\t\tcd expire-to-5.minutes.ago &&\n-\n-\t\tgit branch -M main &&\n-\n-\t\ttest_commit base &&\n-\n-\t\t# Create two classes of unreachable objects, one which\n-\t\t# is older than 5 minutes (stale), and another which is\n-\t\t# newer (recent).\n-\t\tfor kind in stale recent\n-\t\tdo\n-\t\t\tgit checkout -b $kind main &&\n-\t\t\ttest_commit --no-tag $kind || return 1\n-\t\tdone &&\n-\n-\t\tgit rev-list --objects --no-object-names main..stale >in &&\n-\t\tstale=\"$(git pack-objects $objdir/pack/pack expect.raw &&\n-\t\tsort expect.raw >expect &&\n-\n-\t\t# moved.want holds the set of objects we expect to find\n-\t\t# in expired.git\n-\t\tgit rev-list --objects --no-object-names main..stale >out &&\n-\t\tsort out >moved.want &&\n-\n-\t\tgit checkout main &&\n-\t\tgit branch -D stale recent &&\n-\t\tgit reflog expire --all --expire=all &&\n-\t\tgit prune-packed &&\n-\n-\t\tgit init --bare expired.git &&\n-\t\tgit repack -d \\\n-\t\t\t--cruft --cruft-expiration=5.minutes.ago \\\n-\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n-\n-\t\t# Some of the remaining objects in this repository are\n-\t\t# unreachable, so use `cat-file --batch-all-objects`\n-\t\t# instead of `rev-list` to get their names\n-\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" \\\n-\t\t\t>remaining.objects &&\n-\t\tsort remaining.objects >actual &&\n-\t\ttest_cmp expect actual &&\n-\n-\t\t(\n-\t\t\tcd expired.git &&\n-\n-\t\t\texpired=\"$(ls objects/pack/pack-*.mtimes)\" &&\n-\t\t\ttest-tool pack-mtimes $(basename $expired) >out &&\n-\t\t\tcut -d\" \" -f1 out | sort >../moved.got &&\n-\n-\t\t\t# Ensure that there are as many objects with the\n-\t\t\t# expected mtime as were moved to expired.git.\n-\t\t\t#\n-\t\t\t# In other words, ensure that the recorded\n-\t\t\t# mtimes of any moved objects was written\n-\t\t\t# correctly.\n-\t\t\tgrep \" $mtime$\" out >matching &&\n-\t\t\ttest_line_count = $(wc -l <../moved.want) matching\n-\t\t) &&\n-\t\ttest_cmp moved.want moved.got\n-\t)\n-'\n-\n test_done\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nnew file mode 100755\nindex 0000000000..d91fcf1af1\n--- /dev/null\n+++ b/t/t7704-repack-cruft.sh\n@@ -0,0 +1,130 @@\n+#!/bin/sh\n+\n+test_description='git repack works correctly'\n+\n+. ./test-lib.sh\n+\n+objdir=.git/objects\n+\n+test_expect_success '--expire-to stores pruned objects (now)' '\n+\tgit init expire-to-now &&\n+\t(\n+\t\tcd expire-to-now &&\n+\n+\t\tgit branch -M main &&\n+\n+\t\ttest_commit base &&\n+\n+\t\tgit checkout -b cruft &&\n+\t\ttest_commit --no-tag cruft &&\n+\n+\t\tgit rev-list --objects --no-object-names main..cruft >moved.raw &&\n+\t\tsort moved.raw >moved.want &&\n+\n+\t\tgit rev-list --all --objects --no-object-names >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D cruft &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\tgit init --bare expired.git &&\n+\t\tgit repack -d \\\n+\t\t\t--cruft --cruft-expiration=\"now\" \\\n+\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n+\n+\t\texpired=\"$(ls expired.git/objects/pack/pack-*.idx)\" &&\n+\t\ttest_path_is_file \"${expired%.idx}.mtimes\" &&\n+\n+\t\t# Since the `--cruft-expiration` is \"now\", the effective\n+\t\t# behavior is to move _all_ unreachable objects out to\n+\t\t# the location in `--expire-to`.\n+\t\tgit show-index <$expired >expired.raw &&\n+\t\tcut -d\" \" -f2 expired.raw | sort >expired.objects &&\n+\t\tgit rev-list --all --objects --no-object-names \\\n+\t\t\t>remaining.objects &&\n+\n+\t\t# ...in other words, the combined contents of this\n+\t\t# repository and expired.git should be the same as the\n+\t\t# set of objects we started with.\n+\t\tcat expired.objects remaining.objects | sort >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# The \"moved\" objects (i.e., those in expired.git)\n+\t\t# should be the same as the cruft objects which were\n+\t\t# expired in the previous step.\n+\t\ttest_cmp moved.want expired.objects\n+\t)\n+'\n+\n+test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' '\n+\tgit init expire-to-5.minutes.ago &&\n+\t(\n+\t\tcd expire-to-5.minutes.ago &&\n+\n+\t\tgit branch -M main &&\n+\n+\t\ttest_commit base &&\n+\n+\t\t# Create two classes of unreachable objects, one which\n+\t\t# is older than 5 minutes (stale), and another which is\n+\t\t# newer (recent).\n+\t\tfor kind in stale recent\n+\t\tdo\n+\t\t\tgit checkout -b $kind main &&\n+\t\t\ttest_commit --no-tag $kind || return 1\n+\t\tdone &&\n+\n+\t\tgit rev-list --objects --no-object-names main..stale >in &&\n+\t\tstale=\"$(git pack-objects $objdir/pack/pack expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\t# moved.want holds the set of objects we expect to find\n+\t\t# in expired.git\n+\t\tgit rev-list --objects --no-object-names main..stale >out &&\n+\t\tsort out >moved.want &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D stale recent &&\n+\t\tgit reflog expire --all --expire=all &&\n+\t\tgit prune-packed &&\n+\n+\t\tgit init --bare expired.git &&\n+\t\tgit repack -d \\\n+\t\t\t--cruft --cruft-expiration=5.minutes.ago \\\n+\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n+\n+\t\t# Some of the remaining objects in this repository are\n+\t\t# unreachable, so use `cat-file --batch-all-objects`\n+\t\t# instead of `rev-list` to get their names\n+\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" \\\n+\t\t\t>remaining.objects &&\n+\t\tsort remaining.objects >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t(\n+\t\t\tcd expired.git &&\n+\n+\t\t\texpired=\"$(ls objects/pack/pack-*.mtimes)\" &&\n+\t\t\ttest-tool pack-mtimes $(basename $expired) >out &&\n+\t\t\tcut -d\" \" -f1 out | sort >../moved.got &&\n+\n+\t\t\t# Ensure that there are as many objects with the\n+\t\t\t# expected mtime as were moved to expired.git.\n+\t\t\t#\n+\t\t\t# In other words, ensure that the recorded\n+\t\t\t# mtimes of any moved objects was written\n+\t\t\t# correctly.\n+\t\t\tgrep \" $mtime$\" out >matching &&\n+\t\t\ttest_line_count = $(wc -l <../moved.want) matching\n+\t\t) &&\n+\t\ttest_cmp moved.want moved.got\n+\t)\n+'\n+\n+test_done\n-- \n2.42.0.310.g9604b54f73.dirty\n\n"},{"id":"482550","messageId":"9ec999882d790aa770aba8c0916b9260661af9be.1696293862.git.me@ttaylorr.com","threadId":"60206","inReplyTo":"cover.1696293862.git.me@ttaylorr.com","subject":"[PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-03T00:44:29Z","receivedAt":"2023-10-03T00:44:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The repack builtin takes a `--max-pack-size` command-line argument which\nit uses to feed into any of the pack-objects children that it may spawn\nwhen generating a new pack.\n\nThis option is parsed with OPT_STRING, meaning that we'll accept\nanything as input, punting on more fine-grained validation until we get\ndown into pack-objects.\n\nThis is fine, but it's wasteful to spend an entire sub-process just to\nfigure out that one of its option is bogus. Instead, parse the value of\n`--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the\nknonw-good result down to pack-objects.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 529e13120d..8a5bbb9cba 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -51,7 +51,7 @@ struct pack_objects_args {\n \tconst char *window_memory;\n \tconst char *depth;\n \tconst char *threads;\n-\tconst char *max_pack_size;\n+\tunsigned long max_pack_size;\n \tint no_reuse_delta;\n \tint no_reuse_object;\n \tint quiet;\n@@ -242,7 +242,7 @@ static void prepare_pack_objects(struct child_process *cmd,\n \tif (args->threads)\n \t\tstrvec_pushf(&cmd->args, \"--threads=%s\", args->threads);\n \tif (args->max_pack_size)\n-\t\tstrvec_pushf(&cmd->args, \"--max-pack-size=%s\", args->max_pack_size);\n+\t\tstrvec_pushf(&cmd->args, \"--max-pack-size=%lu\", args->max_pack_size);\n \tif (args->no_reuse_delta)\n \t\tstrvec_pushf(&cmd->args, \"--no-reuse-delta\");\n \tif (args->no_reuse_object)\n@@ -946,7 +946,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n \t\tOPT_STRING(0, \"threads\", &po_args.threads, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum number of threads\")),\n-\t\tOPT_STRING(0, \"max-pack-size\", &po_args.max_pack_size, N_(\"bytes\"),\n+\t\tOPT_MAGNITUDE(0, \"max-pack-size\", &po_args.max_pack_size,\n \t\t\t\tN_(\"maximum size of each packfile\")),\n \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n-- \n2.42.0.310.g9604b54f73.dirty\n\n"},{"id":"482551","messageId":"e7beb2060dad648ec5c3fa8984e432ee243ae012.1696293862.git.me@ttaylorr.com","threadId":"60206","inReplyTo":"cover.1696293862.git.me@ttaylorr.com","subject":"[PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-03T00:44:32Z","receivedAt":"2023-10-03T00:44:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Cruft packs are an alternative mechanism for storing a collection of\nunreachable objects whose mtimes are recent enough to avoid being\npruned out of the repository.\n\nWhen cruft packs were first introduced back in b757353676\n(builtin/pack-objects.c: --cruft without expiration, 2022-05-20) and\na7d493833f (builtin/pack-objects.c: --cruft with expiration,\n2022-05-20), the recommended workflow consisted of:\n\n - Repacking periodically, either by packing anything loose in the\n repository (via `git repack -d`) or producing a geometric sequence\n of packs (via `git repack --geometric= -d`).\n\n - Every so often, splitting the repository into two packs, one cruft\n to store the unreachable objects, and another non-cruft pack to\n store the reachable objects.\n\nRepositories may (out of band with the above) choose periodically to\nprune out some unreachable objects which have aged out of the grace\nperiod by generating a pack with `--cruft-expiration=`.\n\nThis allowed repositories to maintain relatively few packs on average,\nand quarantine unreachable objects together in a cruft pack, avoiding\nthe pitfalls of holding unreachable objects as loose while they age out\n(for more, see some of the details in 3d89a8c118\n(Documentation/technical: add cruft-packs.txt, 2022-05-20)).\n\nThis all works, but can be costly from an I/O-perspective when\nfrequently repacking a repository that has many unreachable objects.\nThis problem is exacerbated when those unreachable objects are rarely\n(if every) pruned.\n\nSince there is at most one cruft pack in the above scheme, each time we\nupdate the cruft pack it must be rewritten from scratch. Because much of\nthe pack is reused, this is a relatively inexpensive operation from a\nCPU-perspective, but is very costly in terms of I/O since we end up\nrewriting basically the same pack (plus any new unreachable objects that\nhave entered the repository since the last time a cruft pack was\ngenerated).\n\nAt the time, we decided against implementing more robust support for\nmultiple cruft packs. This patch implements that support which we were\nlacking.\n\nIntroduce a new option `--max-cruft-size` which allows repositories to\naccumulate cruft packs up to a given size, after which point a new\ngeneration of cruft packs can accumulate until it reaches the maximum\nsize, and so on. To generate a new cruft pack, the process works like\nso:\n\n - Sort a list of any existing cruft packs in ascending order of pack\n size.\n\n - Starting from the beginning of the list, group cruft packs together\n while the accumulated size is smaller than the maximum specified\n pack size.\n\n - Combine the objects in these cruft packs together into a new cruft\n pack, along with any other unreachable objects which have since\n entered the repository.\n\nOnce a cruft pack grows beyond the size specified via `--max-cruft-size`\nthe pack is effectively frozen. This limits the I/O churn up to a\nquadratic function of the value specified by the `--max-cruft-size`\noption, instead of behaving quadratically in the number of total\nunreachable objects.\n\nWhen pruning unreachable objects, we bypass the new code paths which\ncombine small cruft packs together, and instead start from scratch,\npassing in the appropriate `--max-pack-size` down to `pack-objects`,\nputting it in charge of keeping the resulting set of cruft packs sized\ncorrectly.\n\nThis may seem like further I/O churn, but in practice it isn't so bad.\nWe could prune old cruft packs for whom all or most objects are removed,\nand then generate a new cruft pack with just the remaining set of\nobjects. But this additional complexity buys us relatively little,\nbecause most objects end up being pruned anyway, so the I/O churn is\nwell contained.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/gc.txt | 6 +\n Documentation/git-gc.txt | 7 +\n Documentation/git-repack.txt | 11 ++\n builtin/gc.c | 7 +\n builtin/repack.c | 134 +++++++++++++++++--\n t/t6500-gc.sh | 27 ++++\n t/t7704-repack-cruft.sh | 245 +++++++++++++++++++++++++++++++++++\n 7 files changed, 426 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt\nindex ca47eb2008..83ebc2de55 100644\n--- a/Documentation/config/gc.txt\n+++ b/Documentation/config/gc.txt\n@@ -86,6 +86,12 @@ gc.cruftPacks::\n \tlinkgit:git-repack[1]) instead of as loose objects. The default\n \tis `true`.\n \n+gc.maxCruftSize::\n+\tLimit the size of new cruft packs when repacking. When\n+\tspecified in addition to `--max-cruft-size`, the command line\n+\toption takes priority. See the `--max-cruft-size` option of\n+\tlinkgit:git-repack[1].\n+\n gc.pruneExpire::\n \tWhen 'git gc' is run, it will call 'prune --expire 2.weeks.ago'\n \t(and 'repack --cruft --cruft-expiration 2.weeks.ago' if using\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex 90806fd26a..fa0541b416 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -59,6 +59,13 @@ be performed as well.\n \tcruft pack instead of storing them as loose objects. `--cruft`\n \tis on by default.\n \n+--max-cruft-size=::\n+\tWhen packing unreachable objects into a cruft pack, limit the\n+\tsize of new cruft packs to be at most ``. Overrides any\n+\tvalue specified via the `gc.maxCruftSize` configuration. See\n+\tthe `--max-cruft-size` option of linkgit:git-repack[1] for\n+\tmore.\n+\n --prune=::\n \tPrune loose objects older than date (default is 2 weeks ago,\n \toverridable by the config variable `gc.pruneExpire`).\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 4017157949..fbfc72e1b2 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -74,6 +74,17 @@ to the new separate pack will be written.\n \timmediately instead of waiting for the next `git gc` invocation.\n \tOnly useful with `--cruft -d`.\n \n+--max-cruft-size=::\n+\tRepack cruft objects into packs as large as `` bytes before\n+\tcreating new packs. As long as there are enough cruft packs\n+\tsmaller than ``, repacking will cause a new cruft pack to\n+\tbe created containing objects from any combined cruft packs,\n+\talong with any new unreachable objects. Cruft packs larger than\n+\t`` will not be modified. When the new cruft pack is larger\n+\tthan `` bytes, it will be split into multiple packs, all of\n+\twhich are guaranteed to be at most `` bytes in size. Only\n+\tuseful with `--cruft -d`.\n+\n --expire-to=::\n \tWrite a cruft pack containing pruned objects (if any) to the\n \tdirectory ``. This option is useful for keeping a copy of\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 00192ae5d3..c97c9fb046 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -52,6 +52,7 @@ static const char * const builtin_gc_usage[] = {\n static int pack_refs = 1;\n static int prune_reflogs = 1;\n static int cruft_packs = 1;\n+static unsigned long max_cruft_size;\n static int aggressive_depth = 50;\n static int aggressive_window = 250;\n static int gc_auto_threshold = 6700;\n@@ -163,6 +164,7 @@ static void gc_config(void)\n \tgit_config_get_int(\"gc.autopacklimit\", &gc_auto_pack_limit);\n \tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n \tgit_config_get_bool(\"gc.cruftpacks\", &cruft_packs);\n+\tgit_config_get_ulong(\"gc.maxcruftsize\", &max_cruft_size);\n \tgit_config_get_expiry(\"gc.pruneexpire\", &prune_expire);\n \tgit_config_get_expiry(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\n \tgit_config_get_expiry(\"gc.logexpiry\", &gc_log_expire);\n@@ -347,6 +349,9 @@ static void add_repack_all_option(struct string_list *keep_pack)\n \t\tstrvec_push(&repack, \"--cruft\");\n \t\tif (prune_expire)\n \t\t\tstrvec_pushf(&repack, \"--cruft-expiration=%s\", prune_expire);\n+\t\tif (max_cruft_size)\n+\t\t\tstrvec_pushf(&repack, \"--max-cruft-size=%lu\",\n+\t\t\t\t max_cruft_size);\n \t} else {\n \t\tstrvec_push(&repack, \"-A\");\n \t\tif (prune_expire)\n@@ -575,6 +580,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"prune unreferenced objects\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },\n \t\tOPT_BOOL(0, \"cruft\", &cruft_packs, N_(\"pack unreferenced objects separately\")),\n+\t\tOPT_MAGNITUDE(0, \"max-cruft-size\", &max_cruft_size,\n+\t\t\t N_(\"with --cruft, limit the size of new cruft packs\")),\n \t\tOPT_BOOL(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n \t\tOPT_BOOL_F(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\"),\n \t\t\t PARSE_OPT_NOCOMPLETE),\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 8a5bbb9cba..04770b15fe 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -27,6 +27,7 @@\n #define PACK_CRUFT 4\n \n #define DELETE_PACK 1\n+#define RETAIN_PACK 2\n \n static int pack_everything;\n static int delta_base_offset = 1;\n@@ -116,11 +117,26 @@ static void pack_mark_for_deletion(struct string_list_item *item)\n \titem->util = (void*)((uintptr_t)item->util | DELETE_PACK);\n }\n \n+static void pack_unmark_for_deletion(struct string_list_item *item)\n+{\n+\titem->util = (void*)((uintptr_t)item->util & ~DELETE_PACK);\n+}\n+\n static int pack_is_marked_for_deletion(struct string_list_item *item)\n {\n \treturn (uintptr_t)item->util & DELETE_PACK;\n }\n \n+static void pack_mark_retained(struct string_list_item *item)\n+{\n+\titem->util = (void*)((uintptr_t)item->util | RETAIN_PACK);\n+}\n+\n+static int pack_is_retained(struct string_list_item *item)\n+{\n+\treturn (uintptr_t)item->util & RETAIN_PACK;\n+}\n+\n static void mark_packs_for_deletion_1(struct string_list *names,\n \t\t\t\t struct string_list *list)\n {\n@@ -133,17 +149,39 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n \t\tif (len < hexsz)\n \t\t\tcontinue;\n \t\tsha1 = item->string + len - hexsz;\n-\t\t/*\n-\t\t * Mark this pack for deletion, which ensures that this\n-\t\t * pack won't be included in a MIDX (if `--write-midx`\n-\t\t * was given) and that we will actually delete this pack\n-\t\t * (if `-d` was given).\n-\t\t */\n-\t\tif (!string_list_has_string(names, sha1))\n+\n+\t\tif (pack_is_retained(item)) {\n+\t\t\tpack_unmark_for_deletion(item);\n+\t\t} else if (!string_list_has_string(names, sha1)) {\n+\t\t\t/*\n+\t\t\t * Mark this pack for deletion, which ensures\n+\t\t\t * that this pack won't be included in a MIDX\n+\t\t\t * (if `--write-midx` was given) and that we\n+\t\t\t * will actually delete this pack (if `-d` was\n+\t\t\t * given).\n+\t\t\t */\n \t\t\tpack_mark_for_deletion(item);\n+\t\t}\n \t}\n }\n \n+static void retain_cruft_pack(struct existing_packs *existing,\n+\t\t\t struct packed_git *cruft)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\n+\tstrbuf_addstr(&buf, pack_basename(cruft));\n+\tstrbuf_strip_suffix(&buf, \".pack\");\n+\n+\titem = string_list_lookup(&existing->cruft_packs, buf.buf);\n+\tif (!item)\n+\t\tBUG(\"could not find cruft pack '%s'\", pack_basename(cruft));\n+\n+\tpack_mark_retained(item);\n+\tstrbuf_release(&buf);\n+}\n+\n static void mark_packs_for_deletion(struct existing_packs *existing,\n \t\t\t\t struct string_list *names)\n \n@@ -225,6 +263,8 @@ static void collect_pack_filenames(struct existing_packs *existing,\n \t}\n \n \tstring_list_sort(&existing->kept_packs);\n+\tstring_list_sort(&existing->non_kept_packs);\n+\tstring_list_sort(&existing->cruft_packs);\n \tstrbuf_release(&buf);\n }\n \n@@ -806,6 +846,72 @@ static void remove_redundant_bitmaps(struct string_list *include,\n \tstrbuf_release(&path);\n }\n \n+static int existing_cruft_pack_cmp(const void *va, const void *vb)\n+{\n+\tstruct packed_git *a = *(struct packed_git **)va;\n+\tstruct packed_git *b = *(struct packed_git **)vb;\n+\n+\tif (a->pack_size < b->pack_size)\n+\t\treturn -1;\n+\tif (a->pack_size > b->pack_size)\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n+\t\t\t\t struct existing_packs *existing)\n+{\n+\tstruct packed_git **existing_cruft, *p;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tsize_t total_size = 0;\n+\tsize_t existing_cruft_nr = 0;\n+\tsize_t i;\n+\n+\tALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);\n+\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\tif (!(p->is_cruft && p->pack_local))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addstr(&buf, pack_basename(p));\n+\t\tstrbuf_strip_suffix(&buf, \".pack\");\n+\n+\t\tif (!string_list_has_string(&existing->cruft_packs, buf.buf))\n+\t\t\tcontinue;\n+\n+\t\tif (existing_cruft_nr >= existing->cruft_packs.nr)\n+\t\t\tBUG(\"too many cruft packs (found %\"PRIuMAX\", but knew \"\n+\t\t\t \"of %\"PRIuMAX\")\",\n+\t\t\t (uintmax_t)existing_cruft_nr + 1,\n+\t\t\t (uintmax_t)existing->cruft_packs.nr);\n+\t\texisting_cruft[existing_cruft_nr++] = p;\n+\t}\n+\n+\tQSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);\n+\n+\tfor (i = 0; i < existing_cruft_nr; i++) {\n+\t\tsize_t proposed;\n+\n+\t\tp = existing_cruft[i];\n+\t\tproposed = st_add(total_size, p->pack_size);\n+\n+\t\tif (proposed <= max_size) {\n+\t\t\ttotal_size = proposed;\n+\t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n+\t\t} else {\n+\t\t\tretain_cruft_pack(existing, p);\n+\t\t\tfprintf(in, \"%s\\n\", pack_basename(p));\n+\t\t}\n+\t}\n+\n+\tfor (i = 0; i < existing->non_kept_packs.nr; i++)\n+\t\tfprintf(in, \"-%s.pack\\n\",\n+\t\t\texisting->non_kept_packs.items[i].string);\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static int write_cruft_pack(const struct pack_objects_args *args,\n \t\t\t const char *destination,\n \t\t\t const char *pack_prefix,\n@@ -853,10 +959,14 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \tin = xfdopen(cmd.in, \"w\");\n \tfor_each_string_list_item(item, names)\n \t\tfprintf(in, \"%s-%s.pack\\n\", pack_prefix, item->string);\n-\tfor_each_string_list_item(item, &existing->non_kept_packs)\n-\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n-\tfor_each_string_list_item(item, &existing->cruft_packs)\n-\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n+\tif (args->max_pack_size && !cruft_expiration) {\n+\t\tcollapse_small_cruft_packs(in, args->max_pack_size, existing);\n+\t} else {\n+\t\tfor_each_string_list_item(item, &existing->non_kept_packs)\n+\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n+\t\tfor_each_string_list_item(item, &existing->cruft_packs)\n+\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n+\t}\n \tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tfprintf(in, \"%s.pack\\n\", item->string);\n \tfclose(in);\n@@ -919,6 +1029,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t PACK_CRUFT),\n \t\tOPT_STRING(0, \"cruft-expiration\", &cruft_expiration, N_(\"approxidate\"),\n \t\t\t\tN_(\"with --cruft, expire objects older than this\")),\n+\t\tOPT_MAGNITUDE(0, \"max-cruft-size\", &cruft_po_args.max_pack_size,\n+\t\t\t\tN_(\"with --cruft, limit the size of new cruft packs\")),\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,\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 69509d0c11..0d6b5c3b27 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -303,6 +303,33 @@ test_expect_success 'gc.bigPackThreshold ignores cruft packs' '\n \t)\n '\n \n+cruft_max_size_opts=\"git repack -d -l --cruft --cruft-expiration=2.weeks.ago\"\n+\n+test_expect_success 'setup for --max-cruft-size tests' '\n+\tgit init cruft--max-size &&\n+\t(\n+\t\tcd cruft--max-size &&\n+\t\tprepare_cruft_history\n+\t)\n+'\n+\n+test_expect_success '--max-cruft-size sets appropriate repack options' '\n+\tGIT_TRACE2_EVENT=$(pwd)/trace2.txt git -C cruft--max-size \\\n+\t\tgc --cruft --max-cruft-size=1M &&\n+\ttest_subcommand $cruft_max_size_opts --max-cruft-size=1048576 blob &&\n+\tgit hash-object -w -t blob blob &&\n+\trm blob\n+}\n+\n+pack_random_blob () {\n+\tgenerate_random_blob \"$@\" &&\n+\tgit repack -d -q >/dev/null\n+}\n+\n+generate_cruft_pack () {\n+\tpack_random_blob \"$@\" >/dev/null &&\n+\n+\tls $packdir/pack-*.pack | xargs -n 1 basename >in &&\n+\tpack=\"$(git pack-objects --cruft $packdir/pack foo.objects &&\n+\t\ttest-tool pack-mtimes $(basename \"$cruft_bar\") >bar.objects &&\n+\n+\t\tgrep \"^$foo\" foo.objects &&\n+\t\ttest_line_count = 1 foo.objects &&\n+\t\tgrep \"^$bar\" bar.objects &&\n+\t\ttest_line_count = 1 bar.objects\n+\t)\n+'\n+\n+test_expect_success '--max-cruft-size combines existing packs when below threshold' '\n+\tgit init max-cruft-size-small &&\n+\t(\n+\t\tcd max-cruft-size-small &&\n+\t\ttest_commit base &&\n+\n+\t\tfoo=\"$(pack_random_blob foo $((1*1024*1024)))\" &&\n+\t\tgit repack --cruft -d &&\n+\n+\t\tbar=\"$(pack_random_blob bar $((1*1024*1024)))\" &&\n+\t\tgit repack --cruft -d --max-cruft-size=10M &&\n+\n+\t\tcruft=$(ls $packdir/pack-*.mtimes) &&\n+\t\ttest-tool pack-mtimes $(basename \"$cruft\") >cruft.objects &&\n+\n+\t\tgrep \"^$foo\" cruft.objects &&\n+\t\tgrep \"^$bar\" cruft.objects &&\n+\t\ttest_line_count = 2 cruft.objects\n+\t)\n+'\n+\n+test_expect_success '--max-cruft-size combines smaller packs first' '\n+\tgit init max-cruft-size-consume-small &&\n+\t(\n+\t\tcd max-cruft-size-consume-small &&\n+\n+\t\ttest_commit base &&\n+\t\tgit repack -ad &&\n+\n+\t\tcruft_foo=\"$(generate_cruft_pack foo 524288)\" && # 0.5 MiB\n+\t\tcruft_bar=\"$(generate_cruft_pack bar 524288)\" && # 0.5 MiB\n+\t\tcruft_baz=\"$(generate_cruft_pack baz 1048576)\" && # 1.0 MiB\n+\t\tcruft_quux=\"$(generate_cruft_pack quux 1572864)\" && # 1.5 MiB\n+\n+\t\ttest-tool pack-mtimes \"$(basename $cruft_foo)\" >expect.raw &&\n+\t\ttest-tool pack-mtimes \"$(basename $cruft_bar)\" >>expect.raw &&\n+\t\tsort expect.raw >expect.objects &&\n+\n+\t\t# repacking with `--max-cruft-size=2M` should combine\n+\t\t# both 0.5 MiB packs together, instead of, say, one of\n+\t\t# the 0.5 MiB packs with the 1.0 MiB pack\n+\t\tls $packdir/pack-*.mtimes | sort >cruft.before &&\n+\t\tgit repack -d --cruft --max-cruft-size=2M &&\n+\t\tls $packdir/pack-*.mtimes | sort >cruft.after &&\n+\n+\t\tcomm -13 cruft.before cruft.after >cruft.new &&\n+\t\tcomm -23 cruft.before cruft.after >cruft.removed &&\n+\n+\t\ttest_line_count = 1 cruft.new &&\n+\t\ttest_line_count = 2 cruft.removed &&\n+\n+\t\t# the two smaller packs should be rolled up first\n+\t\tprintf \"%s\\n\" $cruft_foo $cruft_bar | sort >expect.removed &&\n+\t\ttest_cmp expect.removed cruft.removed &&\n+\n+\t\t# ...and contain the set of objects rolled up\n+\t\ttest-tool pack-mtimes \"$(basename $(cat cruft.new))\" >actual.raw &&\n+\t\tsort actual.raw >actual.objects &&\n+\n+\t\ttest_cmp expect.objects actual.objects\n+\t)\n+'\n+\n+test_expect_success 'setup --max-cruft-size with freshened objects' '\n+\tgit init max-cruft-size-freshen &&\n+\t(\n+\t\tcd max-cruft-size-freshen &&\n+\n+\t\ttest_commit base &&\n+\t\tgit repack -ad &&\n+\n+\t\tfoo=\"$(generate_random_blob foo 64)\" &&\n+\t\ttest-tool chmtime --get -10000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$foo\")\" >foo.mtime &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\tcruft=\"$(ls $packdir/pack-*.mtimes)\" &&\n+\t\ttest-tool pack-mtimes \"$(basename $cruft)\" >actual &&\n+\t\techo \"$foo $(cat foo.mtime)\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--max-cruft-size with freshened objects (loose)' '\n+\t(\n+\t\tcd max-cruft-size-freshen &&\n+\n+\t\t# regenerate the object, setting its mtime to be more recent\n+\t\tfoo=\"$(generate_random_blob foo 64)\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$foo\")\" >foo.mtime &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\tcruft=\"$(ls $packdir/pack-*.mtimes)\" &&\n+\t\ttest-tool pack-mtimes \"$(basename $cruft)\" >actual &&\n+\t\techo \"$foo $(cat foo.mtime)\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--max-cruft-size with freshened objects (packed)' '\n+\t(\n+\t\tcd max-cruft-size-freshen &&\n+\n+\t\t# regenerate the object and store it in a packfile,\n+\t\t# setting its mtime to be more recent\n+\t\t#\n+\t\t# store it alongside another cruft object so that we\n+\t\t# do not create an identical copy of the existing\n+\t\t# cruft pack (which contains $foo).\n+\t\tfoo=\"$(generate_random_blob foo 64)\" &&\n+\t\tbar=\"$(generate_random_blob bar 64)\" &&\n+\t\tfoo_pack=\"$(printf \"%s\\n\" $foo $bar | git pack-objects $packdir/pack)\" &&\n+\t\tgit prune-packed &&\n+\n+\t\ttest-tool chmtime --get -10 \\\n+\t\t\t\"$packdir/pack-$foo_pack.pack\" >foo.mtime &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\tcruft=\"$(ls $packdir/pack-*.mtimes)\" &&\n+\t\ttest-tool pack-mtimes \"$(basename $cruft)\" >actual &&\n+\t\techo \"$foo $(cat foo.mtime)\" >expect.raw &&\n+\t\techo \"$bar $(cat foo.mtime)\" >>expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--max-cruft-size with pruning' '\n+\tgit init max-cruft-size-prune &&\n+\t(\n+\t\tcd max-cruft-size-prune &&\n+\n+\t\ttest_commit base &&\n+\t\tfoo=\"$(generate_random_blob foo $((1024*1024)))\" &&\n+\t\tbar=\"$(generate_random_blob bar $((1024*1024)))\" &&\n+\t\tbaz=\"$(generate_random_blob baz $((1024*1024)))\" &&\n+\n+\t\ttest-tool chmtime -10000 \"$objdir/$(test_oid_to_path \"$foo\")\" &&\n+\n+\t\tgit repack -d --cruft --max-cruft-size=1M &&\n+\n+\t\t# backdate the mtimes of all cruft packs to validate\n+\t\t# that they were rewritten as a result of pruning\n+\t\tls $packdir/pack-*.mtimes | sort >cruft.before &&\n+\t\tfor cruft in $(cat cruft.before)\n+\t\tdo\n+\t\t\tmtime=\"$(test-tool chmtime --get -10000 \"$cruft\")\" &&\n+\t\t\techo $cruft $mtime >>mtimes || return 1\n+\t\tdone &&\n+\n+\t\t# repack (and prune) with a --max-cruft-size to ensure\n+\t\t# that we appropriately split the resulting set of packs\n+\t\tgit repack -d --cruft --max-cruft-size=1M \\\n+\t\t\t--cruft-expiration=10.seconds.ago &&\n+\t\tls $packdir/pack-*.mtimes | sort >cruft.after &&\n+\n+\t\tfor cruft in $(cat cruft.after)\n+\t\tdo\n+\t\t\told_mtime=\"$(grep $cruft mtimes | cut -d\" \" -f2)\" &&\n+\t\t\tnew_mtime=\"$(test-tool chmtime --get $cruft)\" &&\n+\t\t\ttest $old_mtime -lt $new_mtime || return 1\n+\t\tdone &&\n+\n+\t\ttest_line_count = 3 cruft.before &&\n+\t\ttest_line_count = 2 cruft.after &&\n+\t\ttest_must_fail git cat-file -e $foo &&\n+\t\tgit cat-file -e $bar &&\n+\t\tgit cat-file -e $baz\n+\t)\n+'\n+\n+test_expect_success '--max-cruft-size ignores non-local packs' '\n+\trepo=\"max-cruft-size-non-local\" &&\n+\tgit init $repo &&\n+\t(\n+\t\tcd $repo &&\n+\t\ttest_commit base &&\n+\t\tgenerate_random_blob foo 64 &&\n+\t\tgit repack --cruft -d\n+\t) &&\n+\n+\tgit clone --reference=$repo $repo $repo-alt &&\n+\t(\n+\t\tcd $repo-alt &&\n+\n+\t\ttest_commit other &&\n+\t\tgenerate_random_blob bar 64 &&\n+\n+\t\t# ensure that we do not attempt to pick up packs from\n+\t\t# the non-alternated repository, which would result in a\n+\t\t# crash\n+\t\tgit repack --cruft --max-cruft-size=1M -d\n+\t)\n+'\n+\n test_done\n-- \n2.42.0.310.g9604b54f73.dirty\n"},{"id":"482681","messageId":"ZR6nKzflu_18JnoG@tanuki","threadId":"60206","inReplyTo":"e7beb2060dad648ec5c3fa8984e432ee243ae012.1696293862.git.me@ttaylorr.com","subject":"Re: [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-05T12:08:11Z","receivedAt":"2023-10-05T14:23:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:\n[snip]\n> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\n> index 90806fd26a..fa0541b416 100644\n> --- a/Documentation/git-gc.txt\n> +++ b/Documentation/git-gc.txt\n> @@ -59,6 +59,13 @@ be performed as well.\n> \tcruft pack instead of storing them as loose objects. `--cruft`\n> \tis on by default.\n> \n> +--max-cruft-size=::\n> +\tWhen packing unreachable objects into a cruft pack, limit the\n> +\tsize of new cruft packs to be at most ``. Overrides any\n\nWe should probably mention the unit here, which is bytes.\n\n[snip]\n> @@ -806,6 +846,72 @@ static void remove_redundant_bitmaps(struct string_list *include,\n> \tstrbuf_release(&path);\n> }\n> \n> +static int existing_cruft_pack_cmp(const void *va, const void *vb)\n> +{\n> +\tstruct packed_git *a = *(struct packed_git **)va;\n> +\tstruct packed_git *b = *(struct packed_git **)vb;\n> +\n> +\tif (a->pack_size < b->pack_size)\n> +\t\treturn -1;\n> +\tif (a->pack_size > b->pack_size)\n> +\t\treturn 1;\n> +\treturn 0;\n> +}\n> +\n> +static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n> +\t\t\t\t struct existing_packs *existing)\n> +{\n> +\tstruct packed_git **existing_cruft, *p;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tsize_t total_size = 0;\n> +\tsize_t existing_cruft_nr = 0;\n> +\tsize_t i;\n> +\n> +\tALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);\n> +\n> +\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> +\t\tif (!(p->is_cruft && p->pack_local))\n> +\t\t\tcontinue;\n> +\n> +\t\tstrbuf_reset(&buf);\n> +\t\tstrbuf_addstr(&buf, pack_basename(p));\n> +\t\tstrbuf_strip_suffix(&buf, \".pack\");\n> +\n> +\t\tif (!string_list_has_string(&existing->cruft_packs, buf.buf))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (existing_cruft_nr >= existing->cruft_packs.nr)\n> +\t\t\tBUG(\"too many cruft packs (found %\"PRIuMAX\", but knew \"\n> +\t\t\t \"of %\"PRIuMAX\")\",\n> +\t\t\t (uintmax_t)existing_cruft_nr + 1,\n> +\t\t\t (uintmax_t)existing->cruft_packs.nr);\n> +\t\texisting_cruft[existing_cruft_nr++] = p;\n> +\t}\n> +\n> +\tQSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);\n> +\n> +\tfor (i = 0; i < existing_cruft_nr; i++) {\n> +\t\tsize_t proposed;\n> +\n> +\t\tp = existing_cruft[i];\n> +\t\tproposed = st_add(total_size, p->pack_size);\n> +\n> +\t\tif (proposed <= max_size) {\n> +\t\t\ttotal_size = proposed;\n> +\t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n> +\t\t} else {\n> +\t\t\tretain_cruft_pack(existing, p);\n> +\t\t\tfprintf(in, \"%s\\n\", pack_basename(p));\n> +\t\t}\n> +\t}\n> +\n> +\tfor (i = 0; i < existing->non_kept_packs.nr; i++)\n> +\t\tfprintf(in, \"-%s.pack\\n\",\n> +\t\t\texisting->non_kept_packs.items[i].string);\n\nAs far as I can see, the non-kept packs are passed to\ngit-pack-objects(1) both in the cases where we do collapse small cruft\npacks and where we don't. Is there any particular reason why we handle\nthose in both code paths separately instead of merging that logic? Is\nthe ordering of packfiles important here?\n\n> +\tstrbuf_release(&buf);\n> +}\n> +\n> static int write_cruft_pack(const struct pack_objects_args *args,\n> \t\t\t const char *destination,\n> \t\t\t const char *pack_prefix,\n> @@ -853,10 +959,14 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n> \tin = xfdopen(cmd.in, \"w\");\n> \tfor_each_string_list_item(item, names)\n> \t\tfprintf(in, \"%s-%s.pack\\n\", pack_prefix, item->string);\n> -\tfor_each_string_list_item(item, &existing->non_kept_packs)\n> -\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n> -\tfor_each_string_list_item(item, &existing->cruft_packs)\n> -\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n> +\tif (args->max_pack_size && !cruft_expiration) {\n> +\t\tcollapse_small_cruft_packs(in, args->max_pack_size, existing);\n> +\t} else {\n> +\t\tfor_each_string_list_item(item, &existing->non_kept_packs)\n> +\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n> +\t\tfor_each_string_list_item(item, &existing->cruft_packs)\n> +\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n> +\t}\n> \tfor_each_string_list_item(item, &existing->kept_packs)\n> \t\tfprintf(in, \"%s.pack\\n\", item->string);\n> \tfclose(in);\n\nPatrick\n"},{"id":"482687","messageId":"ZR6ednOcuK6qizch@tanuki","threadId":"60206","inReplyTo":"9ec999882d790aa770aba8c0916b9260661af9be.1696293862.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-05T11:31:02Z","receivedAt":"2023-10-05T14:43:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 02, 2023 at 08:44:29PM -0400, Taylor Blau wrote:\n> The repack builtin takes a `--max-pack-size` command-line argument which\n> it uses to feed into any of the pack-objects children that it may spawn\n> when generating a new pack.\n> \n> This option is parsed with OPT_STRING, meaning that we'll accept\n> anything as input, punting on more fine-grained validation until we get\n> down into pack-objects.\n> \n> This is fine, but it's wasteful to spend an entire sub-process just to\n> figure out that one of its option is bogus. Instead, parse the value of\n> `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the\n> knonw-good result down to pack-objects.\n\nTiny nit: s/knonw/known.\n\nOther than that this patch looks good to me.\n\nPatrick\n\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Taylor Blau \n> ---\n> builtin/repack.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 529e13120d..8a5bbb9cba 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -51,7 +51,7 @@ struct pack_objects_args {\n> \tconst char *window_memory;\n> \tconst char *depth;\n> \tconst char *threads;\n> -\tconst char *max_pack_size;\n> +\tunsigned long max_pack_size;\n> \tint no_reuse_delta;\n> \tint no_reuse_object;\n> \tint quiet;\n> @@ -242,7 +242,7 @@ static void prepare_pack_objects(struct child_process *cmd,\n> \tif (args->threads)\n> \t\tstrvec_pushf(&cmd->args, \"--threads=%s\", args->threads);\n> \tif (args->max_pack_size)\n> -\t\tstrvec_pushf(&cmd->args, \"--max-pack-size=%s\", args->max_pack_size);\n> +\t\tstrvec_pushf(&cmd->args, \"--max-pack-size=%lu\", args->max_pack_size);\n> \tif (args->no_reuse_delta)\n> \t\tstrvec_pushf(&cmd->args, \"--no-reuse-delta\");\n> \tif (args->no_reuse_object)\n> @@ -946,7 +946,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\t\t\tN_(\"limits the maximum delta depth\")),\n> \t\tOPT_STRING(0, \"threads\", &po_args.threads, N_(\"n\"),\n> \t\t\t\tN_(\"limits the maximum number of threads\")),\n> -\t\tOPT_STRING(0, \"max-pack-size\", &po_args.max_pack_size, N_(\"bytes\"),\n> +\t\tOPT_MAGNITUDE(0, \"max-pack-size\", &po_args.max_pack_size,\n> \t\t\t\tN_(\"maximum size of each packfile\")),\n> \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n> \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n> -- \n> 2.42.0.310.g9604b54f73.dirty\n> \n"},{"id":"482695","messageId":"ZR7yOgvk3PCtYIR2@nand.local","threadId":"60206","inReplyTo":"ZR6ednOcuK6qizch@tanuki","subject":"Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-05T17:28:26Z","receivedAt":"2023-10-05T17:33:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 05, 2023 at 01:31:02PM +0200, Patrick Steinhardt wrote:\n> On Mon, Oct 02, 2023 at 08:44:29PM -0400, Taylor Blau wrote:\n> > The repack builtin takes a `--max-pack-size` command-line argument which\n> > it uses to feed into any of the pack-objects children that it may spawn\n> > when generating a new pack.\n> >\n> > This option is parsed with OPT_STRING, meaning that we'll accept\n> > anything as input, punting on more fine-grained validation until we get\n> > down into pack-objects.\n> >\n> > This is fine, but it's wasteful to spend an entire sub-process just to\n> > figure out that one of its option is bogus. Instead, parse the value of\n> > `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the\n> > knonw-good result down to pack-objects.\n>\n> Tiny nit: s/knonw/known.\n>\n> Other than that this patch looks good to me.\n\nOops, good eyes. Perhaps Junio can tweak this when queuing, but if he\ndoesn't, I don't think it'd be the end of the world...\n\nThanks,\nTaylor\n"},{"id":"482699","messageId":"ZR7z0/tStMSjBkFR@nand.local","threadId":"60206","inReplyTo":"ZR6nKzflu_18JnoG@tanuki","subject":"Re: [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-05T17:35:15Z","receivedAt":"2023-10-05T17:43:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 05, 2023 at 02:08:11PM +0200, Patrick Steinhardt wrote:\n> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:\n> [snip]\n> > diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\n> > index 90806fd26a..fa0541b416 100644\n> > --- a/Documentation/git-gc.txt\n> > +++ b/Documentation/git-gc.txt\n> > @@ -59,6 +59,13 @@ be performed as well.\n> > \tcruft pack instead of storing them as loose objects. `--cruft`\n> > \tis on by default.\n> >\n> > +--max-cruft-size=::\n> > +\tWhen packing unreachable objects into a cruft pack, limit the\n> > +\tsize of new cruft packs to be at most ``. Overrides any\n>\n> We should probably mention the unit here, which is bytes.\n\nPerhaps, though I'm OK with omitting it in the name of brevity, but only\nsince we link off to the relevant section in the git-repack(1)\ndocumentation (which does include the units there).\n\n> > +static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n> > +\t\t\t\t struct existing_packs *existing)\n> > +{\n> > +\tstruct packed_git **existing_cruft, *p;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tsize_t total_size = 0;\n> > +\tsize_t existing_cruft_nr = 0;\n> > +\tsize_t i;\n> > +\n> > +\tALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);\n> > +\n> > +\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> > +\t\tif (!(p->is_cruft && p->pack_local))\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tstrbuf_reset(&buf);\n> > +\t\tstrbuf_addstr(&buf, pack_basename(p));\n> > +\t\tstrbuf_strip_suffix(&buf, \".pack\");\n> > +\n> > +\t\tif (!string_list_has_string(&existing->cruft_packs, buf.buf))\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif (existing_cruft_nr >= existing->cruft_packs.nr)\n> > +\t\t\tBUG(\"too many cruft packs (found %\"PRIuMAX\", but knew \"\n> > +\t\t\t \"of %\"PRIuMAX\")\",\n> > +\t\t\t (uintmax_t)existing_cruft_nr + 1,\n> > +\t\t\t (uintmax_t)existing->cruft_packs.nr);\n> > +\t\texisting_cruft[existing_cruft_nr++] = p;\n> > +\t}\n> > +\n> > +\tQSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);\n> > +\n> > +\tfor (i = 0; i < existing_cruft_nr; i++) {\n> > +\t\tsize_t proposed;\n> > +\n> > +\t\tp = existing_cruft[i];\n> > +\t\tproposed = st_add(total_size, p->pack_size);\n> > +\n> > +\t\tif (proposed <= max_size) {\n> > +\t\t\ttotal_size = proposed;\n> > +\t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n> > +\t\t} else {\n> > +\t\t\tretain_cruft_pack(existing, p);\n> > +\t\t\tfprintf(in, \"%s\\n\", pack_basename(p));\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tfor (i = 0; i < existing->non_kept_packs.nr; i++)\n> > +\t\tfprintf(in, \"-%s.pack\\n\",\n> > +\t\t\texisting->non_kept_packs.items[i].string);\n>\n> As far as I can see, the non-kept packs are passed to\n> git-pack-objects(1) both in the cases where we do collapse small cruft\n> packs and where we don't. Is there any particular reason why we handle\n> those in both code paths separately instead of merging that logic? Is\n> the ordering of packfiles important here?\n\nNo particularly good reason. The ordering isn't important, and you could do something like this:\n\n--- 8< ---\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 04770b15fe..6e17fc3f51 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -905,10 +905,6 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n \t\t}\n \t}\n\n-\tfor (i = 0; i < existing->non_kept_packs.nr; i++)\n-\t\tfprintf(in, \"-%s.pack\\n\",\n-\t\t\texisting->non_kept_packs.items[i].string);\n-\n \tstrbuf_release(&buf);\n }\n\n@@ -959,14 +955,13 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \tin = xfdopen(cmd.in, \"w\");\n \tfor_each_string_list_item(item, names)\n \t\tfprintf(in, \"%s-%s.pack\\n\", pack_prefix, item->string);\n-\tif (args->max_pack_size && !cruft_expiration) {\n+\tif (args->max_pack_size && !cruft_expiration)\n \t\tcollapse_small_cruft_packs(in, args->max_pack_size, existing);\n-\t} else {\n-\t\tfor_each_string_list_item(item, &existing->non_kept_packs)\n-\t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n+\telse\n \t\tfor_each_string_list_item(item, &existing->cruft_packs)\n \t\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n-\t}\n+\tfor_each_string_list_item(item, &existing->non_kept_packs)\n+\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n \tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tfprintf(in, \"%s.pack\\n\", item->string);\n \tfclose(in);\n--- >8 ---\n\nBut I think that having a small amount of duplication is a fair price to\npay for being able to see the whole input given to pack-objects outlined\nin a single function.\n\nThanks,\nTaylor\n"},{"id":"482709","messageId":"xmqq8r8giztv.fsf@gitster.g","threadId":"60206","inReplyTo":"ZR7yOgvk3PCtYIR2@nand.local","subject":"Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-05T20:22:52Z","receivedAt":"2023-10-05T20:22:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Thu, Oct 05, 2023 at 01:31:02PM +0200, Patrick Steinhardt wrote:\n>> On Mon, Oct 02, 2023 at 08:44:29PM -0400, Taylor Blau wrote:\n>> > The repack builtin takes a `--max-pack-size` command-line argument which\n>> > it uses to feed into any of the pack-objects children that it may spawn\n>> > when generating a new pack.\n>> >\n>> > This option is parsed with OPT_STRING, meaning that we'll accept\n>> > anything as input, punting on more fine-grained validation until we get\n>> > down into pack-objects.\n>> >\n>> > This is fine, but it's wasteful to spend an entire sub-process just to\n>> > figure out that one of its option is bogus. Instead, parse the value of\n>> > `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the\n>> > knonw-good result down to pack-objects.\n>>\n>> Tiny nit: s/knonw/known.\n>>\n>> Other than that this patch looks good to me.\n>\n> Oops, good eyes. Perhaps Junio can tweak this when queuing, but if he\n> doesn't, I don't think it'd be the end of the world...\n\nI cannot do so \"when queuing\" anymore, but let me see if it is still\noutside 'next' and then run \"rebase -i\" on it.\n\nThis is a bit different from what I would have written (I would have\npassed the original string to underlying subprocess for it to sanity\ncheck the value to its own liking, after we checked if it makes\nsense when interpreted as magnitude, so that the underlying\nsubprocess, if it has stricter limit than we know of, can complain\nwith the string the end-user gave it, not the result of us turning\ninto a large integer string. But that is minor.\n\nThanks, both.\n"},{"id":"482710","messageId":"xmqq4jj4izpj.fsf@gitster.g","threadId":"60206","inReplyTo":"ZR6nKzflu_18JnoG@tanuki","subject":"Re: [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-05T20:25:28Z","receivedAt":"2023-10-05T20:25:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:\n> [snip]\n>> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\n>> index 90806fd26a..fa0541b416 100644\n>> --- a/Documentation/git-gc.txt\n>> +++ b/Documentation/git-gc.txt\n>> @@ -59,6 +59,13 @@ be performed as well.\n>> \tcruft pack instead of storing them as loose objects. `--cruft`\n>> \tis on by default.\n>> \n>> +--max-cruft-size=::\n>> +\tWhen packing unreachable objects into a cruft pack, limit the\n>> +\tsize of new cruft packs to be at most ``. Overrides any\n>\n> We should probably mention the unit here, which is bytes.\n\nGood suggestion. I'll tweak \"at most ``\" to \"& bytes\" or\nsomething.\n"},{"id":"482791","messageId":"20231007172031.GA1524950@coredump.intra.peff.net","threadId":"60206","inReplyTo":"e7beb2060dad648ec5c3fa8984e432ee243ae012.1696293862.git.me@ttaylorr.com","subject":"[PATCH] repack: free existing_cruft array after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-07T17:20:31Z","receivedAt":"2023-10-07T17:20:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:\n\n> +static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n> +\t\t\t\t struct existing_packs *existing)\n> +{\n> +\tstruct packed_git **existing_cruft, *p;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> [...]\n> +\n> +\tstrbuf_release(&buf);\n> +}\n\nCoverity (using the just-merged-to-next version of the workflow file!)\nflagged a leak here. Since the topic (tb/repack-max-cruft-size) is in\n'next', I think we'd want this on top:\n\n-- >8 --\nSubject: [PATCH] repack: free existing_cruft array after use\n\nWe allocate an array of packed_git pointers so that we can sort the list\nof cruft packs, but we never free the array, causing a small leak. Note\nthat we don't need to free the packed_git structs themselves; they're\nowned by the repository object.\n\nSigned-off-by: Jeff King \n---\n builtin/repack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a1a893d952..69e8f302c0 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -955,6 +955,7 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n \t\t\texisting->non_kept_packs.items[i].string);\n \n \tstrbuf_release(&buf);\n+\tfree(existing_cruft);\n }\n \n static int write_cruft_pack(const struct pack_objects_args *args,\n-- \n2.42.0.884.gc318dcfe19\n\n"},{"id":"482838","messageId":"ZSNWZ9+Q2eOpy91A@nand.local","threadId":"60206","inReplyTo":"20231007172031.GA1524950@coredump.intra.peff.net","subject":"Re: [PATCH] repack: free existing_cruft array after use","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-09T01:24:55Z","receivedAt":"2023-10-09T01:25:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Oct 07, 2023 at 01:20:31PM -0400, Jeff King wrote:\n> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:\n>\n> > +static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n> > +\t\t\t\t struct existing_packs *existing)\n> > +{\n> > +\tstruct packed_git **existing_cruft, *p;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > [...]\n> > +\n> > +\tstrbuf_release(&buf);\n> > +}\n>\n> Coverity (using the just-merged-to-next version of the workflow file!)\n> flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in\n> 'next', I think we'd want this on top:\n\nWoohoo! I'm glad that this is already paying dividends.\n\n> -- >8 --\n> Subject: [PATCH] repack: free existing_cruft array after use\n>\n> We allocate an array of packed_git pointers so that we can sort the list\n> of cruft packs, but we never free the array, causing a small leak. Note\n> that we don't need to free the packed_git structs themselves; they're\n> owned by the repository object.\n\nThanks, I can't believe I missed this when writing this function. The\nfix looks obviously correct to me, so this has my:\n\n Acked-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"482882","messageId":"xmqqttqz1z9u.fsf@gitster.g","threadId":"60206","inReplyTo":"ZSNWZ9+Q2eOpy91A@nand.local","subject":"Re: [PATCH] repack: free existing_cruft array after use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-09T17:28:13Z","receivedAt":"2023-10-09T17:28:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Sat, Oct 07, 2023 at 01:20:31PM -0400, Jeff King wrote:\n>> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:\n>> ...\n>> Coverity (using the just-merged-to-next version of the workflow file!)\n>> flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in\n>> 'next', I think we'd want this on top:\n>\n> Woohoo! I'm glad that this is already paying dividends.\n> ...\n> Thanks, I can't believe I missed this when writing this function. The\n> fix looks obviously correct to me, so this has my:\n>\n> Acked-by: Taylor Blau \n>\n> Thanks,\n\nWill queue. Thanks.\n\n\n"}]}