{"thread":{"id":"63018","subject":"[PATCH 0/2] pack-objects: freshen objects with multi-cruft packs","startedAt":"2025-02-27T18:29:27Z","lastAt":"2025-03-13T18:41:11Z","messageCount":54,"participants":["Taylor Blau","Elijah Newren","Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"513174","messageId":"cover.1740680964.git.me@ttaylorr.com","threadId":"63018","inReplyTo":null,"subject":"[PATCH 0/2] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-27T18:29:24Z","receivedAt":"2025-02-27T18:29:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short series contains a fix for a bug I noticed while rolling out\nmulti-cruft packs (via 'git repack --max-cruft-size') within GitHub's\ninfrastructure.\n\nThe series is structured as follows:\n\n - The first patch simplifies how 'repack' aggregates cruft packs\n together when their size is below the '--max-cruft-size' or\n '--max-pack-size' threshold. This simplification changes behavior\n slightly, but not in a meaningful way. It occurred to me while\n writing the second patch.\n\n - The second patch describes and fixes the main bug. The gist here is\n that objects which are (a) unreachable, (b) exist in a cruft pack\n being retained, and (c) were freshened to have a more recent mtime\n than any existing cruft copy are unable to be freshened.\n\nThe fix pursued in the second patch changes the rules around when we\nwant to retain an object via builtin/pack-objects.c::want_found_object()\nwhen at least one cruft pack will survive the repack.\n\nPreviously the rule was to discard any object which appears in any\nsurviving pack, regardless of mtime. The rule now is to only discard an\nobject if it appears in either (a) a non-cruft pack which will survive\nthe repack, or (b) a cruft pack whose mtime for that object is older\nthan the one we are trying to pack.\n\nI think that this is the right behavior, but admittedly putting this\nseries together hurt my brain trying to think through all of the cases.\nI'm fairly confident in the testing here as I remember it being fairly\nexhaustive of all interesting cases. But I'd appreciate a sanity check\nfrom others that they too are convinced this is the right approach.\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n builtin/repack.c: simplify cruft pack aggregation\n builtin/pack-objects.c: freshen objects from existing cruft packs\n\n builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n builtin/repack.c | 38 +------------\n packfile.c | 3 +-\n packfile.h | 2 +\n t/t7704-repack-cruft.sh | 106 ++++++++++++++++++++++--------------\n 5 files changed, 171 insertions(+), 96 deletions(-)\n\n\nbase-commit: 08bdfd453584e489d5a551aecbdcb77584e1b958\n-- \n2.49.0.rc0.2.gc0c926adde2\n"},{"id":"513175","messageId":"8564f98259727225391edcb5ab3b47dd53f00e48.1740680964.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1740680964.git.me@ttaylorr.com","subject":"[PATCH 1/2] builtin/repack.c: simplify cruft pack aggregation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-27T18:29:28Z","receivedAt":"2025-02-27T18:29:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 37dc6d8104 (builtin/repack.c: implement support for\n`--max-cruft-size`, 2023-10-02), 'git repack' built on support for\nmultiple cruft packs in Git by instructing 'git pack-objects --cruft'\nhow to aggregate smaller cruft packs up to the provided threshold.\n\nThe implementation in 37dc6d8104 worked something like the following\npseudo-code:\n\n total_size = 0;\n\n for (p in cruft packs) {\n if (p->pack_size + total_size < max_size) {\n total_size += p->pack_size;\n collapse(p)\n } else {\n retain(p);\n }\n }\n\nThe original idea behind this approach was that smaller cruft packs\nwould get combined together until the sum of their sizes was no larger\nthan the given max pack size.\n\nThere is a much simpler way to achieve this, however, which is to simply\ncombine *all* cruft packs which are smaller than the threshold,\nregardless of what their sum is. With '--max-pack-size', 'pack-objects'\nwill split out the resulting pack into individual pack(s) if necessary\nto ensure that the written pack(s) are each no larger than the provided\nthreshold.\n\nThis yields a slight behavior change, which is reflected in the removed\ntest. Previous to this change, we would aggregate smaller cruft packs\nfirst, whereas now we will opportunistically combine as many cruft packs\nas possible. As as result, that test is no longer relevant, and can be\ndeleted.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 38 ++-----------------------------------\n t/t7704-repack-cruft.sh | 42 -----------------------------------------\n 2 files changed, 2 insertions(+), 78 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 75e3752353a..4d83d40f39f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1022,29 +1022,13 @@ static int write_filtered_pack(const struct pack_objects_args *args,\n \treturn finish_pack_objects_cmd(&cmd, names, local);\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 packed_git *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@@ -1056,24 +1040,7 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size,\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\tif (p->pack_size < max_size) {\n \t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n \t\t} else {\n \t\t\tretain_cruft_pack(existing, p);\n@@ -1086,7 +1053,6 @@ 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,\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 959e6e26488..5a76b541ddd 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -194,48 +194,6 @@ test_expect_success '--max-cruft-size combines existing packs when below thresho\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-- \n2.49.0.rc0.2.gc0c926adde2\n\n"},{"id":"513176","messageId":"c0c926adde2b7c8f4b53b7a274d5b8c040f77e62.1740680964.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1740680964.git.me@ttaylorr.com","subject":"[PATCH 2/2] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-27T18:29:31Z","receivedAt":"2025-02-27T18:29:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Once an object is written into a cruft pack, we can only freshen it by\nwriting a new loose or packed copy of that object with a more recent\nmtime.\n\nPrior to 61568efa95 (builtin/pack-objects.c: support `--max-pack-size`\nwith `--cruft`, 2023-08-28), we typically had at most one cruft pack in\na repository at any given time. So freshening unreachable objects was\nstraightforward when already rewriting the cruft pack (and its *.mtimes\nfile).\n\nBut 61568efa95 changes things: 'pack-objects' now supports writing\nmultiple cruft packs when invoked with `--cruft` and the\n`--max-pack-size` flag. Cruft packs are rewritten until they reach some\nsize threshold, at which point they are considered \"frozen\", and will\nonly be modified in a pruning GC, or if the threshold itself is\nadjusted.\n\nHowever, this process breaks down when we attempt to freshen an object\npacked in an earlier cruft pack that is larger than the threshold and\nthus will survive the repack.\n\nWhen this is the case, it is impossible to freshen objects in cruft\npack(s) which are larger than the threshold. This is because we avoid\nwriting them in the new cruft pack entirely, for a couple of reasons.\n\n 1. When enumerating packed objects via 'add_objects_in_unpacked_packs()'\n we pass the SKIP_IN_CORE_KEPT_PACKS, which is used to avoid looping\n over the packs we're going to retain (which are marked as kept\n in-core by 'read_cruft_objects()').\n\n This means that we will avoid enumerating additional packed copies\n of objects found in any cruft packs which are larger than the given\n size threshold. Thus there is no opportunity to call\n 'create_object_entry()' whatsoever.\n\n 2. We likewise will discard the loose copy (if one exists) of any\n unreachable object packed in a cruft pack that is larger than the\n threshold. Here our call path is 'add_unreachable_loose_objects()',\n which uses the 'add_loose_object()' callback.\n\n That function will eventually land us in 'want_object_in_pack()'\n (via 'add_cruft_object_entry()'), and we'll discard the object as it\n appears in one of the packs which we marked as kept in-core.\n\nThis means in effect that it is impossible to freshen an unreachable\nobject once it appears in a cruft pack larger than the given threshold.\n\nInstead, we should pack an additional copy of an unreachable object we\nwant to freshen even if it appears in a cruft pack, provided that the\ncruft copy has an mtime which is before the mtime of the copy we are\ntrying to pack/freshen. This is sub-optimal in the sense that it\nrequires keeping an additional copy of unreachable objects upon\nfreshening, but we don't have a better alternative without the ability\nto make in-place modifications to existing *.mtimes files.\n\nIn order to implement this, we have to adjust the behavior of\n'want_found_object()'. When 'pack-objects' is told that we're *not*\ngoing to retain any cruft packs (i.e. the set of packs marked as kept\nin-core does not contain a cruft pack), the behavior is unchanged.\n\nBut when there *is* at least one cruft pack that we're holding onto, it\nis no longer sufficient to reject a copy of an object found in that\ncruft pack for that reason alone. In this case, we only want to reject a\ncandidate object when copies of that object either:\n\n - exists in a non-cruft pack that we are retaining, regardless of that\n pack's mtime, or\n\n - exists in a cruft pack with an mtime more recent than the copy we are\n debating whether or not to pack, in which case freshening would be\n redundant.\n\nTo do this, keep track of whether or not we have any cruft packs in our\nin-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\nflag. When we end up in this new special case, we replace a call to\n'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only\nreject objects when we have a copy in an existing cruft pack with a more\nrecent mtime (in which case \"freshening\" would be redundant).\n\nSigned-off-by: Taylor Blau \n---\n builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n packfile.c | 3 +-\n packfile.h | 2 +\n t/t7704-repack-cruft.sh | 64 ++++++++++++++++++++++\n 4 files changed, 169 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 58a9b161262..79e1e6fb52b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -206,6 +206,7 @@ static int have_non_local_packs;\n static int incremental;\n static int ignore_packed_keep_on_disk;\n static int ignore_packed_keep_in_core;\n+static int ignore_packed_keep_in_core_has_cruft;\n static int allow_ofs_delta;\n static struct pack_idx_option pack_idx_opts;\n static const char *base_name;\n@@ -1502,8 +1503,60 @@ static int have_duplicate_entry(const struct object_id *oid,\n \treturn 1;\n }\n \n+static int want_cruft_object_mtime(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t unsigned flags, uint32_t mtime)\n+{\n+\tstruct packed_git **cache;\n+\n+\tfor (cache = kept_pack_cache(r, flags); *cache; cache++) {\n+\t\tstruct packed_git *p = *cache;\n+\t\toff_t ofs;\n+\t\tuint32_t candidate_mtime;\n+\n+\t\tofs = find_pack_entry_one(oid, p);\n+\t\tif (!ofs)\n+\t\t\tcontinue;\n+\n+\t\t/*\n+\t\t * We have a copy of the object 'oid' in a non-cruft\n+\t\t * pack. We can avoid packing an additional copy\n+\t\t * regardless of what the existing copy's mtime is since\n+\t\t * it is outside of a cruft pack.\n+\t\t */\n+\t\tif (!p->is_cruft)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * If we have a copy of the object 'oid' in a cruft\n+\t\t * pack, then either read the cruft pack's mtime for\n+\t\t * that object, or, if that can't be loaded, assume the\n+\t\t * pack's mtime itself.\n+\t\t */\n+\t\tif (!load_pack_mtimes(p)) {\n+\t\t\tuint32_t pos;\n+\t\t\tif (offset_to_pack_pos(p, ofs, &pos) < 0)\n+\t\t\t\tcontinue;\n+\t\t\tcandidate_mtime = nth_packed_mtime(p, pos);\n+\t\t} else {\n+\t\t\tcandidate_mtime = p->mtime;\n+\t\t}\n+\n+\t\t/*\n+\t\t * We have a surviving copy of the object in a cruft\n+\t\t * pack whose mtime is greater than or equal to the one\n+\t\t * we are considering. We can thus avoid packing an\n+\t\t * additional copy of that object.\n+\t\t */\n+\t\tif (mtime <= candidate_mtime)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n static int want_found_object(const struct object_id *oid, int exclude,\n-\t\t\t struct packed_git *p)\n+\t\t\t struct packed_git *p, uint32_t mtime)\n {\n \tif (exclude)\n \t\treturn 1;\n@@ -1553,12 +1606,29 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\tif (ignore_packed_keep_in_core)\n \t\t\tflags |= IN_CORE_KEEP_PACKS;\n \n-\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n-\t\t\treturn 0;\n-\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n-\t\t\treturn 0;\n-\t\tif (has_object_kept_pack(p->repo, oid, flags))\n-\t\t\treturn 0;\n+\t\t/*\n+\t\t * If the object is in a pack that we want to ignore, *and* we\n+\t\t * don't have any cruft packs that are being retained, we can\n+\t\t * abort quickly.\n+\t\t */\n+\t\tif (!ignore_packed_keep_in_core_has_cruft) {\n+\t\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n+\t\t\t\treturn 0;\n+\t\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n+\t\t\t\treturn 0;\n+\t\t\tif (has_object_kept_pack(p->repo, oid, flags))\n+\t\t\t\treturn 0;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * But if there is at least one cruft pack which\n+\t\t\t * is being kept, we only want to include the\n+\t\t\t * provided object if it has a strictly greater\n+\t\t\t * mtime than any existing cruft copy.\n+\t\t\t */\n+\t\t\tif (!want_cruft_object_mtime(p->repo, oid, flags,\n+\t\t\t\t\t\t mtime))\n+\t\t\t\treturn 0;\n+\t\t}\n \t}\n \n \t/*\n@@ -1577,7 +1647,8 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t int exclude,\n \t\t\t\t struct packed_git **found_pack,\n-\t\t\t\t off_t *found_offset)\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \toff_t offset;\n \n@@ -1593,7 +1664,7 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t*found_offset = offset;\n \t\t\t*found_pack = p;\n \t\t}\n-\t\treturn want_found_object(oid, exclude, p);\n+\t\treturn want_found_object(oid, exclude, p, found_mtime);\n \t}\n \treturn -1;\n }\n@@ -1607,10 +1678,11 @@ static int want_object_in_pack_one(struct packed_git *p,\n * function finds if there is any pack that has the object and returns the pack\n * and its offset in these variables.\n */\n-static int want_object_in_pack(const struct object_id *oid,\n-\t\t\t int exclude,\n-\t\t\t struct packed_git **found_pack,\n-\t\t\t off_t *found_offset)\n+static int want_object_in_pack_mtime(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \tint want;\n \tstruct list_head *pos;\n@@ -1625,7 +1697,8 @@ static int want_object_in_pack(const struct object_id *oid,\n \t * are present we will determine the answer right now.\n \t */\n \tif (*found_pack) {\n-\t\twant = want_found_object(oid, exclude, *found_pack);\n+\t\twant = want_found_object(oid, exclude, *found_pack,\n+\t\t\t\t\t found_mtime);\n \t\tif (want != -1)\n \t\t\treturn want;\n \n@@ -1636,7 +1709,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \tfor (m = get_multi_pack_index(the_repository); m; m = m->next) {\n \t\tstruct pack_entry e;\n \t\tif (fill_midx_entry(the_repository, oid, &e, m)) {\n-\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset);\n+\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\n \t\t}\n@@ -1644,7 +1717,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \n \tlist_for_each(pos, get_packed_git_mru(the_repository)) {\n \t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n-\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset);\n+\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\tif (!exclude && want > 0)\n \t\t\tlist_move(&p->mru,\n \t\t\t\t get_packed_git_mru(the_repository));\n@@ -1674,6 +1747,15 @@ static int want_object_in_pack(const struct object_id *oid,\n \treturn 1;\n }\n \n+static inline int want_object_in_pack(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset)\n+{\n+\treturn want_object_in_pack_mtime(oid, exclude, found_pack, found_offset,\n+\t\t\t\t\t 0);\n+}\n+\n static struct object_entry *create_object_entry(const struct object_id *oid,\n \t\t\t\t\t\tenum object_type type,\n \t\t\t\t\t\tuint32_t hash,\n@@ -3606,7 +3688,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type\n \t\t\tentry->no_try_delta = no_try_delta(name);\n \t\t}\n \t} else {\n-\t\tif (!want_object_in_pack(oid, 0, &pack, &offset))\n+\t\tif (!want_object_in_pack_mtime(oid, 0, &pack, &offset, mtime))\n \t\t\treturn;\n \t\tif (!pack && type == OBJ_BLOB && !has_loose_object(oid)) {\n \t\t\t/*\n@@ -3680,6 +3762,8 @@ static void mark_pack_kept_in_core(struct string_list *packs, unsigned keep)\n \t\tstruct packed_git *p = item->util;\n \t\tif (!p)\n \t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n+\t\tif (p->is_cruft && keep)\n+\t\t\tignore_packed_keep_in_core_has_cruft = 1;\n \t\tp->pack_keep_in_core = keep;\n \t}\n }\ndiff --git a/packfile.c b/packfile.c\nindex 2d80d80cb38..9d09f8bc726 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -24,6 +24,7 @@\n #include \"commit-graph.h\"\n #include \"pack-revindex.h\"\n #include \"promisor-remote.h\"\n+#include \"pack-mtimes.h\"\n \n char *odb_pack_name(struct repository *r, struct strbuf *buf,\n \t\t const unsigned char *hash, const char *ext)\n@@ -2107,7 +2108,7 @@ static void maybe_invalidate_kept_pack_cache(struct repository *r,\n \tr->objects->kept_pack_cache.flags = 0;\n }\n \n-static struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n {\n \tmaybe_invalidate_kept_pack_cache(r, flags);\n \ndiff --git a/packfile.h b/packfile.h\nindex 00ada7a938f..25097213d06 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -197,6 +197,8 @@ int has_object_pack(struct repository *r, const struct object_id *oid);\n int has_object_kept_pack(struct repository *r, const struct object_id *oid,\n \t\t\t unsigned flags);\n \n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags);\n+\n /*\n * Return 1 if an object in a promisor packfile is or refers to the given\n * object, 0 otherwise.\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 5a76b541ddd..2b20d66333c 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -262,6 +262,70 @@ test_expect_success '--max-cruft-size with freshened objects (packed)' '\n \t)\n '\n \n+test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n+\tgit init max-cruft-size-threshold &&\n+\t(\n+\t\tcd max-cruft-size-threshold &&\n+\n+\t\ttest_commit base &&\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\tbaz=\"$(generate_random_blob baz $((2*1024*1024)))\" &&\n+\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$foo\")\" >foo.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$baz\")\" >baz.old &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\t# Make a packed copy of object $foo with a more recent\n+\t\t# mtime.\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tfoo_pack=\"$(echo \"$foo\" | git pack-objects $packdir/pack)\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$packdir/pack-$foo_pack.pack\" >foo.new &&\n+\t\tgit prune-packed &&\n+\n+\t\t# Make a loose copy of object $bar with a more recent\n+\t\t# mtime.\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.new &&\n+\n+\t\t# Make a new cruft object $quux to ensure we do not\n+\t\t# generate an identical pack to the existing cruft\n+\t\t# pack.\n+\t\tquux=\"$(generate_random_blob quux $((1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$quux\")\" >quux.new &&\n+\n+\t\tgit repack --cruft --max-cruft-size=3M -d &&\n+\n+\t\tfor p in $packdir/pack-*.mtimes\n+\t\tdo\n+\t\t\ttest-tool pack-mtimes \"$(basename \"$p\")\" || return 1\n+\t\tdone >actual.raw &&\n+\t\tsort actual.raw >actual &&\n+\n+\t\t# Among the set of all cruft packs, we should see both\n+\t\t# mtimes for object $foo and $bar, as well as the\n+\t\t# single new copy of $baz.\n+\t\tsort >expect <<-EOF &&\n+\t\t$foo $(cat foo.old)\n+\t\t$foo $(cat foo.new)\n+\t\t$bar $(cat bar.old)\n+\t\t$bar $(cat bar.new)\n+\t\t$baz $(cat baz.old)\n+\t\t$quux $(cat quux.new)\n+\t\tEOF\n+\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-- \n2.49.0.rc0.2.gc0c926adde2\n"},{"id":"513182","messageId":"CABPp-BE9=WUv1typWB7JB4HvP04Q_Bh6h23D=LzcpsAsggQ+7A@mail.gmail.com","threadId":"63018","inReplyTo":"8564f98259727225391edcb5ab3b47dd53f00e48.1740680964.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] builtin/repack.c: simplify cruft pack aggregation","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-02-27T19:23:02Z","receivedAt":"2025-02-27T19:23:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Feb 27, 2025 at 10:29 AM Taylor Blau wrote:\n>\n> In 37dc6d8104 (builtin/repack.c: implement support for\n> `--max-cruft-size`, 2023-10-02), 'git repack' built on support for\n> multiple cruft packs in Git by instructing 'git pack-objects --cruft'\n> how to aggregate smaller cruft packs up to the provided threshold.\n>\n> The implementation in 37dc6d8104 worked something like the following\n> pseudo-code:\n>\n> total_size = 0;\n>\n> for (p in cruft packs) {\n> if (p->pack_size + total_size < max_size) {\n> total_size += p->pack_size;\n> collapse(p)\n> } else {\n> retain(p);\n> }\n> }\n>\n> The original idea behind this approach was that smaller cruft packs\n> would get combined together until the sum of their sizes was no larger\n> than the given max pack size.\n>\n> There is a much simpler way to achieve this, however, which is to simply\n> combine *all* cruft packs which are smaller than the threshold,\n> regardless of what their sum is. With '--max-pack-size', 'pack-objects'\n> will split out the resulting pack into individual pack(s) if necessary\n> to ensure that the written pack(s) are each no larger than the provided\n> threshold.\n\nThat doesn't really \"achieve this\" though, unless the antecedent of\n\"this\" isn't what was described in the previous paragraph but\nsomething elsewhere. I suspect your actual meaning was something\nalong the lines of \"There is a much simpler way to combine cruft\npacks, however, which...\" ?\n\n> This yields a slight behavior change, which is reflected in the removed\n> test. Previous to this change, we would aggregate smaller cruft packs\n> first, whereas now we will opportunistically combine as many cruft packs\n> as possible. As as result, that test is no longer relevant, and can be\n> deleted.\n\nI like the idea, since it sounds like it should be simpler...\n\n> Signed-off-by: Taylor Blau \n> ---\n> builtin/repack.c | 38 ++-----------------------------------\n> t/t7704-repack-cruft.sh | 42 -----------------------------------------\n> 2 files changed, 2 insertions(+), 78 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 75e3752353a..4d83d40f39f 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -1022,29 +1022,13 @@ static int write_filtered_pack(const struct pack_objects_args *args,\n> return finish_pack_objects_cmd(&cmd, names, local);\n> }\n>\n> -static int existing_cruft_pack_cmp(const void *va, const void *vb)\n> -{\n> - struct packed_git *a = *(struct packed_git **)va;\n> - struct packed_git *b = *(struct packed_git **)vb;\n> -\n> - if (a->pack_size < b->pack_size)\n> - return -1;\n> - if (a->pack_size > b->pack_size)\n> - return 1;\n> - return 0;\n> -}\n> -\n> static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n> struct existing_packs *existing)\n> {\n> - struct packed_git **existing_cruft, *p;\n> + struct packed_git *p;\n> struct strbuf buf = STRBUF_INIT;\n> - size_t total_size = 0;\n> - size_t existing_cruft_nr = 0;\n> size_t i;\n>\n> - ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);\n> -\n> for (p = get_all_packs(the_repository); p; p = p->next) {\n> if (!(p->is_cruft && p->pack_local))\n> continue;\n> @@ -1056,24 +1040,7 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n> if (!string_list_has_string(&existing->cruft_packs, buf.buf))\n> continue;\n>\n> - if (existing_cruft_nr >= existing->cruft_packs.nr)\n> - BUG(\"too many cruft packs (found %\"PRIuMAX\", but knew \"\n> - \"of %\"PRIuMAX\")\",\n> - (uintmax_t)existing_cruft_nr + 1,\n> - (uintmax_t)existing->cruft_packs.nr);\n> - existing_cruft[existing_cruft_nr++] = p;\n> - }\n> -\n> - QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);\n> -\n> - for (i = 0; i < existing_cruft_nr; i++) {\n> - size_t proposed;\n> -\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> + if (p->pack_size < max_size) {\n\nLook at all that deleted code. Always nice to see a simplification in\naction. :-)\n\n> fprintf(in, \"-%s\\n\", pack_basename(p));\n> } else {\n> retain_cruft_pack(existing, p);\n> @@ -1086,7 +1053,6 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size,\n> existing->non_kept_packs.items[i].string);\n>\n> strbuf_release(&buf);\n> - free(existing_cruft);\n> }\n>\n> static int write_cruft_pack(const struct pack_objects_args *args,\n> diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\n> index 959e6e26488..5a76b541ddd 100755\n> --- a/t/t7704-repack-cruft.sh\n> +++ b/t/t7704-repack-cruft.sh\n> @@ -194,48 +194,6 @@ test_expect_success '--max-cruft-size combines existing packs when below thresho\n> )\n> '\n>\n> -test_expect_success '--max-cruft-size combines smaller packs first' '\n> - git init max-cruft-size-consume-small &&\n> - (\n> - cd max-cruft-size-consume-small &&\n> -\n> - test_commit base &&\n> - git repack -ad &&\n> -\n> - cruft_foo=\"$(generate_cruft_pack foo 524288)\" && # 0.5 MiB\n> - cruft_bar=\"$(generate_cruft_pack bar 524288)\" && # 0.5 MiB\n> - cruft_baz=\"$(generate_cruft_pack baz 1048576)\" && # 1.0 MiB\n> - cruft_quux=\"$(generate_cruft_pack quux 1572864)\" && # 1.5 MiB\n> -\n> - test-tool pack-mtimes \"$(basename $cruft_foo)\" >expect.raw &&\n> - test-tool pack-mtimes \"$(basename $cruft_bar)\" >>expect.raw &&\n> - sort expect.raw >expect.objects &&\n> -\n> - # repacking with `--max-cruft-size=2M` should combine\n> - # both 0.5 MiB packs together, instead of, say, one of\n> - # the 0.5 MiB packs with the 1.0 MiB pack\n> - ls $packdir/pack-*.mtimes | sort >cruft.before &&\n> - git repack -d --cruft --max-cruft-size=2M &&\n> - ls $packdir/pack-*.mtimes | sort >cruft.after &&\n> -\n> - comm -13 cruft.before cruft.after >cruft.new &&\n> - comm -23 cruft.before cruft.after >cruft.removed &&\n> -\n> - test_line_count = 1 cruft.new &&\n> - test_line_count = 2 cruft.removed &&\n> -\n> - # the two smaller packs should be rolled up first\n> - printf \"%s\\n\" $cruft_foo $cruft_bar | sort >expect.removed &&\n> - test_cmp expect.removed cruft.removed &&\n> -\n> - # ...and contain the set of objects rolled up\n> - test-tool pack-mtimes \"$(basename $(cat cruft.new))\" >actual.raw &&\n> - sort actual.raw >actual.objects &&\n> -\n> - test_cmp expect.objects actual.objects\n> - )\n> -'\n> -\n> test_expect_success 'setup --max-cruft-size with freshened objects' '\n> git init max-cruft-size-freshen &&\n> (\n> --\n> 2.49.0.rc0.2.gc0c926adde2\n\nLooks good to me, other than the misleading/ambiguous wording in that\none paragraph of the commit message.\n"},{"id":"513183","messageId":"CABPp-BGv6J307nTo1sUgAnE+7ZnueSPm4CJLb10wBGQGaPPDWA@mail.gmail.com","threadId":"63018","inReplyTo":"c0c926adde2b7c8f4b53b7a274d5b8c040f77e62.1740680964.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-02-27T19:26:32Z","receivedAt":"2025-02-27T19:26:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Feb 27, 2025 at 10:29 AM Taylor Blau wrote:\n>\n> Once an object is written into a cruft pack, we can only freshen it by\n> writing a new loose or packed copy of that object with a more recent\n> mtime.\n\nThanks for adding this. (Side note for others reading: I reviewed an\nearlier round before it was posted to the list).\n\n> Prior to 61568efa95 (builtin/pack-objects.c: support `--max-pack-size`\n> with `--cruft`, 2023-08-28), we typically had at most one cruft pack in\n> a repository at any given time. So freshening unreachable objects was\n> straightforward when already rewriting the cruft pack (and its *.mtimes\n> file).\n>\n> But 61568efa95 changes things: 'pack-objects' now supports writing\n> multiple cruft packs when invoked with `--cruft` and the\n> `--max-pack-size` flag. Cruft packs are rewritten until they reach some\n> size threshold, at which point they are considered \"frozen\", and will\n> only be modified in a pruning GC, or if the threshold itself is\n> adjusted.\n>\n> However, this process breaks down when we attempt to freshen an object\n> packed in an earlier cruft pack that is larger than the threshold and\n> thus will survive the repack.\n\n...packed in an earlier cruft pack, and that cruft pack is larger than\nthe threshold...\n\n(Otherwise, it's unclear whether you are talking about the object or\nthe cruft pack it is in being larger than the threshold.)\n\n> When this is the case, it is impossible to freshen objects in cruft\n> pack(s) which are larger than the threshold. This is because we avoid\n> writing them in the new cruft pack entirely, for a couple of reasons.\n\n...freshen objects in cruft packs when those cruft packs are larger\nthan the threshold...\n\nAgain, just to clarify what thing is \"larger\".\n\nAlso, this paragraph while fine on its own is slightly unclear whether\nyou are discussing pre-patch or post-patch state, which when reading\nthe next two items causes some double takes. Perhaps just spell it\nout slightly clearer here that for the next two enumerated items you\nare discussing the existing state previous to your changes?\n\n> 1. When enumerating packed objects via 'add_objects_in_unpacked_packs()'\n> we pass the SKIP_IN_CORE_KEPT_PACKS, which is used to avoid looping\n> over the packs we're going to retain (which are marked as kept\n> in-core by 'read_cruft_objects()').\n>\n> This means that we will avoid enumerating additional packed copies\n> of objects found in any cruft packs which are larger than the given\n> size threshold. Thus there is no opportunity to call\n> 'create_object_entry()' whatsoever.\n>\n> 2. We likewise will discard the loose copy (if one exists) of any\n> unreachable object packed in a cruft pack that is larger than the\n> threshold. Here our call path is 'add_unreachable_loose_objects()',\n> which uses the 'add_loose_object()' callback.\n>\n> That function will eventually land us in 'want_object_in_pack()'\n> (via 'add_cruft_object_entry()'), and we'll discard the object as it\n> appears in one of the packs which we marked as kept in-core.\n>\n> This means in effect that it is impossible to freshen an unreachable\n> object once it appears in a cruft pack larger than the given threshold.\n>\n> Instead,\n\n...it does become clear here that the above was about the previous\nstate rather than the new state after the patches, but that's a while\nto leave the possible confusion dangling.\n\n> we should pack an additional copy of an unreachable object we\n> want to freshen even if it appears in a cruft pack, provided that the\n> cruft copy has an mtime which is before the mtime of the copy we are\n> trying to pack/freshen. This is sub-optimal in the sense that it\n> requires keeping an additional copy of unreachable objects upon\n> freshening, but we don't have a better alternative without the ability\n> to make in-place modifications to existing *.mtimes files.\n>\n> In order to implement this, we have to adjust the behavior of\n> 'want_found_object()'. When 'pack-objects' is told that we're *not*\n> going to retain any cruft packs (i.e. the set of packs marked as kept\n> in-core does not contain a cruft pack), the behavior is unchanged.\n>\n> But when there *is* at least one cruft pack that we're holding onto, it\n> is no longer sufficient to reject a copy of an object found in that\n> cruft pack for that reason alone. In this case, we only want to reject a\n> candidate object when copies of that object either:\n>\n> - exists in a non-cruft pack that we are retaining, regardless of that\n> pack's mtime, or\n>\n> - exists in a cruft pack with an mtime more recent than the copy we are\n> debating whether or not to pack, in which case freshening would be\n> redundant.\n\ns/more recent than/at least as recent as/ ?\n\n>\n> To do this, keep track of whether or not we have any cruft packs in our\n> in-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\n> flag. When we end up in this new special case, we replace a call to\n> 'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only\n> reject objects when we have a copy in an existing cruft pack with a more\n> recent mtime (in which case \"freshening\" would be redundant).\n\nAgain, s/a more recent/at least as recent/ ?\n\n>\n> Signed-off-by: Taylor Blau \n> ---\n> builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n> packfile.c | 3 +-\n> packfile.h | 2 +\n> t/t7704-repack-cruft.sh | 64 ++++++++++++++++++++++\n> 4 files changed, 169 insertions(+), 18 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 58a9b161262..79e1e6fb52b 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -206,6 +206,7 @@ static int have_non_local_packs;\n> static int incremental;\n> static int ignore_packed_keep_on_disk;\n> static int ignore_packed_keep_in_core;\n> +static int ignore_packed_keep_in_core_has_cruft;\n> static int allow_ofs_delta;\n> static struct pack_idx_option pack_idx_opts;\n> static const char *base_name;\n> @@ -1502,8 +1503,60 @@ static int have_duplicate_entry(const struct object_id *oid,\n> return 1;\n> }\n>\n> +static int want_cruft_object_mtime(struct repository *r,\n> + const struct object_id *oid,\n> + unsigned flags, uint32_t mtime)\n> +{\n> + struct packed_git **cache;\n> +\n> + for (cache = kept_pack_cache(r, flags); *cache; cache++) {\n> + struct packed_git *p = *cache;\n> + off_t ofs;\n> + uint32_t candidate_mtime;\n> +\n> + ofs = find_pack_entry_one(oid, p);\n> + if (!ofs)\n> + continue;\n> +\n> + /*\n> + * We have a copy of the object 'oid' in a non-cruft\n> + * pack. We can avoid packing an additional copy\n> + * regardless of what the existing copy's mtime is since\n> + * it is outside of a cruft pack.\n> + */\n> + if (!p->is_cruft)\n> + return 0;\n> +\n> + /*\n> + * If we have a copy of the object 'oid' in a cruft\n> + * pack, then either read the cruft pack's mtime for\n> + * that object, or, if that can't be loaded, assume the\n> + * pack's mtime itself.\n> + */\n> + if (!load_pack_mtimes(p)) {\n> + uint32_t pos;\n> + if (offset_to_pack_pos(p, ofs, &pos) < 0)\n> + continue;\n> + candidate_mtime = nth_packed_mtime(p, pos);\n> + } else {\n> + candidate_mtime = p->mtime;\n> + }\n> +\n> + /*\n> + * We have a surviving copy of the object in a cruft\n> + * pack whose mtime is greater than or equal to the one\n> + * we are considering. We can thus avoid packing an\n> + * additional copy of that object.\n> + */\n> + if (mtime <= candidate_mtime)\n> + return 0;\n\nHere is where you have the logic for avoiding packing an additional\ncopy when the mtimes are equal, leading to my above wording\nsuggestions for the commit message.\n\n> + }\n> +\n> + return -1;\n> +}\n\nGood comments making it clear what is going on in this function.\n\n\n> +\n> static int want_found_object(const struct object_id *oid, int exclude,\n> - struct packed_git *p)\n> + struct packed_git *p, uint32_t mtime)\n> {\n> if (exclude)\n> return 1;\n> @@ -1553,12 +1606,29 @@ static int want_found_object(const struct object_id *oid, int exclude,\n> if (ignore_packed_keep_in_core)\n> flags |= IN_CORE_KEEP_PACKS;\n>\n> - if (ignore_packed_keep_on_disk && p->pack_keep)\n> - return 0;\n> - if (ignore_packed_keep_in_core && p->pack_keep_in_core)\n> - return 0;\n> - if (has_object_kept_pack(p->repo, oid, flags))\n> - return 0;\n> + /*\n> + * If the object is in a pack that we want to ignore, *and* we\n> + * don't have any cruft packs that are being retained, we can\n> + * abort quickly.\n> + */\n> + if (!ignore_packed_keep_in_core_has_cruft) {\n> + if (ignore_packed_keep_on_disk && p->pack_keep)\n> + return 0;\n> + if (ignore_packed_keep_in_core && p->pack_keep_in_core)\n> + return 0;\n> + if (has_object_kept_pack(p->repo, oid, flags))\n> + return 0;\n> + } else {\n> + /*\n> + * But if there is at least one cruft pack which\n> + * is being kept, we only want to include the\n> + * provided object if it has a strictly greater\n> + * mtime than any existing cruft copy.\n> + */\n> + if (!want_cruft_object_mtime(p->repo, oid, flags,\n> + mtime))\n> + return 0;\n> + }\n> }\n\nHere's the core high-level logic behind this change.\n\n>\n> /*\n> @@ -1577,7 +1647,8 @@ static int want_object_in_pack_one(struct packed_git *p,\n> const struct object_id *oid,\n> int exclude,\n> struct packed_git **found_pack,\n> - off_t *found_offset)\n> + off_t *found_offset,\n> + uint32_t found_mtime)\n> {\n> off_t offset;\n>\n> @@ -1593,7 +1664,7 @@ static int want_object_in_pack_one(struct packed_git *p,\n> *found_offset = offset;\n> *found_pack = p;\n> }\n> - return want_found_object(oid, exclude, p);\n> + return want_found_object(oid, exclude, p, found_mtime);\n> }\n> return -1;\n> }\n> @@ -1607,10 +1678,11 @@ static int want_object_in_pack_one(struct packed_git *p,\n> * function finds if there is any pack that has the object and returns the pack\n> * and its offset in these variables.\n> */\n> -static int want_object_in_pack(const struct object_id *oid,\n> - int exclude,\n> - struct packed_git **found_pack,\n> - off_t *found_offset)\n> +static int want_object_in_pack_mtime(const struct object_id *oid,\n> + int exclude,\n> + struct packed_git **found_pack,\n> + off_t *found_offset,\n> + uint32_t found_mtime)\n> {\n> int want;\n> struct list_head *pos;\n> @@ -1625,7 +1697,8 @@ static int want_object_in_pack(const struct object_id *oid,\n> * are present we will determine the answer right now.\n> */\n> if (*found_pack) {\n> - want = want_found_object(oid, exclude, *found_pack);\n> + want = want_found_object(oid, exclude, *found_pack,\n> + found_mtime);\n> if (want != -1)\n> return want;\n>\n> @@ -1636,7 +1709,7 @@ static int want_object_in_pack(const struct object_id *oid,\n> for (m = get_multi_pack_index(the_repository); m; m = m->next) {\n> struct pack_entry e;\n> if (fill_midx_entry(the_repository, oid, &e, m)) {\n> - want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset);\n> + want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n> if (want != -1)\n> return want;\n> }\n> @@ -1644,7 +1717,7 @@ static int want_object_in_pack(const struct object_id *oid,\n>\n> list_for_each(pos, get_packed_git_mru(the_repository)) {\n> struct packed_git *p = list_entry(pos, struct packed_git, mru);\n> - want = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset);\n> + want = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n> if (!exclude && want > 0)\n> list_move(&p->mru,\n> get_packed_git_mru(the_repository));\n> @@ -1674,6 +1747,15 @@ static int want_object_in_pack(const struct object_id *oid,\n> return 1;\n> }\n>\n> +static inline int want_object_in_pack(const struct object_id *oid,\n> + int exclude,\n> + struct packed_git **found_pack,\n> + off_t *found_offset)\n> +{\n> + return want_object_in_pack_mtime(oid, exclude, found_pack, found_offset,\n> + 0);\n> +}\n> +\n> static struct object_entry *create_object_entry(const struct object_id *oid,\n> enum object_type type,\n> uint32_t hash,\n> @@ -3606,7 +3688,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type\n> entry->no_try_delta = no_try_delta(name);\n> }\n> } else {\n> - if (!want_object_in_pack(oid, 0, &pack, &offset))\n> + if (!want_object_in_pack_mtime(oid, 0, &pack, &offset, mtime))\n\nStuff since my last comment was plumbing the extra parameter through\nthe right places; makes sense.\n\n> return;\n> if (!pack && type == OBJ_BLOB && !has_loose_object(oid)) {\n> /*\n> @@ -3680,6 +3762,8 @@ static void mark_pack_kept_in_core(struct string_list *packs, unsigned keep)\n> struct packed_git *p = item->util;\n> if (!p)\n> die(_(\"could not find pack '%s'\"), item->string);\n> + if (p->is_cruft && keep)\n> + ignore_packed_keep_in_core_has_cruft = 1;\n\nAnd here you set the necessary flag for enabling it all.\n\n> p->pack_keep_in_core = keep;\n> }\n> }\n> diff --git a/packfile.c b/packfile.c\n> index 2d80d80cb38..9d09f8bc726 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -24,6 +24,7 @@\n> #include \"commit-graph.h\"\n> #include \"pack-revindex.h\"\n> #include \"promisor-remote.h\"\n> +#include \"pack-mtimes.h\"\n>\n> char *odb_pack_name(struct repository *r, struct strbuf *buf,\n> const unsigned char *hash, const char *ext)\n> @@ -2107,7 +2108,7 @@ static void maybe_invalidate_kept_pack_cache(struct repository *r,\n> r->objects->kept_pack_cache.flags = 0;\n> }\n>\n> -static struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n> +struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n> {\n> maybe_invalidate_kept_pack_cache(r, flags);\n>\n> diff --git a/packfile.h b/packfile.h\n> index 00ada7a938f..25097213d06 100644\n> --- a/packfile.h\n> +++ b/packfile.h\n> @@ -197,6 +197,8 @@ int has_object_pack(struct repository *r, const struct object_id *oid);\n> int has_object_kept_pack(struct repository *r, const struct object_id *oid,\n> unsigned flags);\n>\n> +struct packed_git **kept_pack_cache(struct repository *r, unsigned flags);\n> +\n> /*\n> * Return 1 if an object in a promisor packfile is or refers to the given\n> * object, 0 otherwise.\n\nYou need access to one more function to do your work; makes sense.\n\n> diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\n> index 5a76b541ddd..2b20d66333c 100755\n> --- a/t/t7704-repack-cruft.sh\n> +++ b/t/t7704-repack-cruft.sh\n> @@ -262,6 +262,70 @@ test_expect_success '--max-cruft-size with freshened objects (packed)' '\n> )\n> '\n>\n> +test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n> + git init max-cruft-size-threshold &&\n> + (\n> + cd max-cruft-size-threshold &&\n> +\n> + test_commit base &&\n> + foo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n> + bar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n> + baz=\"$(generate_random_blob baz $((2*1024*1024)))\" &&\n> +\n> + test-tool chmtime --get -100000 \\\n> + \"$objdir/$(test_oid_to_path \"$foo\")\" >foo.old &&\n> + test-tool chmtime --get -100000 \\\n> + \"$objdir/$(test_oid_to_path \"$bar\")\" >bar.old &&\n> + test-tool chmtime --get -100000 \\\n> + \"$objdir/$(test_oid_to_path \"$baz\")\" >baz.old &&\n> +\n> + git repack --cruft -d &&\n> +\n> + # Make a packed copy of object $foo with a more recent\n> + # mtime.\n\ns/$foo/foo/ ?\n\n> + foo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n\nI thought this was creating a completely different foo, which would\ndefeat the point of the test. It might be worth adding a comment that\nbecause generate_random_blob uses a very simplistic and repeatable\nrandom character generator with the first argument as the seed, that\nthis will regenerate the same loose object as above for foo.\n\n> + foo_pack=\"$(echo \"$foo\" | git pack-objects $packdir/pack)\" &&\n> + test-tool chmtime --get -100 \\\n> + \"$packdir/pack-$foo_pack.pack\" >foo.new &&\n> + git prune-packed &&\n> +\n> + # Make a loose copy of object $bar with a more recent\n> + # mtime.\n\ns/$bar/bar/ ?\n\n> + bar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n\nLikewise, this isn't a different bar than above because it uses the\nsame seed, \"bar\" that the previous one used.\n\n> + test-tool chmtime --get -100 \\\n> + \"$objdir/$(test_oid_to_path \"$bar\")\" >bar.new &&\n> +\n> + # Make a new cruft object $quux to ensure we do not\n> + # generate an identical pack to the existing cruft\n> + # pack.\n> + quux=\"$(generate_random_blob quux $((1024)))\" &&\n> + test-tool chmtime --get -100 \\\n> + \"$objdir/$(test_oid_to_path \"$quux\")\" >quux.new &&\n> +\n> + git repack --cruft --max-cruft-size=3M -d &&\n> +\n> + for p in $packdir/pack-*.mtimes\n> + do\n> + test-tool pack-mtimes \"$(basename \"$p\")\" || return 1\n> + done >actual.raw &&\n> + sort actual.raw >actual &&\n> +\n> + # Among the set of all cruft packs, we should see both\n> + # mtimes for object $foo and $bar, as well as the\n> + # single new copy of $baz.\n> + sort >expect <<-EOF &&\n> + $foo $(cat foo.old)\n> + $foo $(cat foo.new)\n> + $bar $(cat bar.old)\n> + $bar $(cat bar.new)\n> + $baz $(cat baz.old)\n> + $quux $(cat quux.new)\n> + EOF\n\nMakes sense, but only once you understand that the second foo and bar\nobjects really were the same objects as the first foo and bar objects.\n"},{"id":"513184","messageId":"CABPp-BEK3T28iqDFxbMHa7GVrythYs+ze7wCZ5Q-LDjOmquiVQ@mail.gmail.com","threadId":"63018","inReplyTo":"cover.1740680964.git.me@ttaylorr.com","subject":"Re: [PATCH 0/2] pack-objects: freshen objects with multi-cruft packs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-02-27T19:28:18Z","receivedAt":"2025-02-27T19:28:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Feb 27, 2025 at 10:29 AM Taylor Blau wrote:\n>\n> This short series contains a fix for a bug I noticed while rolling out\n> multi-cruft packs (via 'git repack --max-cruft-size') within GitHub's\n> infrastructure.\n>\n> The series is structured as follows:\n>\n> - The first patch simplifies how 'repack' aggregates cruft packs\n> together when their size is below the '--max-cruft-size' or\n> '--max-pack-size' threshold. This simplification changes behavior\n> slightly, but not in a meaningful way. It occurred to me while\n> writing the second patch.\n>\n> - The second patch describes and fixes the main bug. The gist here is\n> that objects which are (a) unreachable, (b) exist in a cruft pack\n> being retained, and (c) were freshened to have a more recent mtime\n> than any existing cruft copy are unable to be freshened.\n>\n> The fix pursued in the second patch changes the rules around when we\n> want to retain an object via builtin/pack-objects.c::want_found_object()\n> when at least one cruft pack will survive the repack.\n>\n> Previously the rule was to discard any object which appears in any\n> surviving pack, regardless of mtime. The rule now is to only discard an\n> object if it appears in either (a) a non-cruft pack which will survive\n> the repack, or (b) a cruft pack whose mtime for that object is older\n> than the one we are trying to pack.\n\nI think in (b) you got the meaning reversed, and instead mean s/older\nthan/at least as new as/ ?\n\n> I think that this is the right behavior, but admittedly putting this\n> series together hurt my brain trying to think through all of the cases.\n> I'm fairly confident in the testing here as I remember it being fairly\n> exhaustive of all interesting cases. But I'd appreciate a sanity check\n> from others that they too are convinced this is the right approach.\n>\n> Thanks in advance for your review!\n>\n> Taylor Blau (2):\n> builtin/repack.c: simplify cruft pack aggregation\n> builtin/pack-objects.c: freshen objects from existing cruft packs\n>\n> builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n> builtin/repack.c | 38 +------------\n> packfile.c | 3 +-\n> packfile.h | 2 +\n> t/t7704-repack-cruft.sh | 106 ++++++++++++++++++++++--------------\n> 5 files changed, 171 insertions(+), 96 deletions(-)\n\nCode changes look good to me, but I had some wording suggestions in a\nfew places for commit messages and comments. (Sorry for missing some\nof those in my preliminary review before you sent this series to the\nlist.)\n"},{"id":"513192","messageId":"Z8Ds206MUMQzqGB6@nand.local","threadId":"63018","inReplyTo":"CABPp-BE9=WUv1typWB7JB4HvP04Q_Bh6h23D=LzcpsAsggQ+7A@mail.gmail.com","subject":"Re: [PATCH 1/2] builtin/repack.c: simplify cruft pack aggregation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-27T22:53:15Z","receivedAt":"2025-02-27T22:53:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Feb 27, 2025 at 11:23:02AM -0800, Elijah Newren wrote:\n> > The original idea behind this approach was that smaller cruft packs\n> > would get combined together until the sum of their sizes was no larger\n> > than the given max pack size.\n> >\n> > There is a much simpler way to achieve this, however, which is to simply\n> > combine *all* cruft packs which are smaller than the threshold,\n> > regardless of what their sum is. With '--max-pack-size', 'pack-objects'\n> > will split out the resulting pack into individual pack(s) if necessary\n> > to ensure that the written pack(s) are each no larger than the provided\n> > threshold.\n>\n> That doesn't really \"achieve this\" though, unless the antecedent of\n> \"this\" isn't what was described in the previous paragraph but\n> something elsewhere. I suspect your actual meaning was something\n> along the lines of \"There is a much simpler way to combine cruft\n> packs, however, which...\" ?\n\nGreat suggestion, thanks. I swapped out \"achieve this\" for your\nrecommendation.\n\n> > This yields a slight behavior change, which is reflected in the removed\n> > test. Previous to this change, we would aggregate smaller cruft packs\n> > first, whereas now we will opportunistically combine as many cruft packs\n> > as possible. As as result, that test is no longer relevant, and can be\n> > deleted.\n>\n> I like the idea, since it sounds like it should be simpler...\n\nHeh. I don't know why I wrote it the way it was originally. I wrote the\nsecond patch in this series first, and when I was trying to explain how\nmulti-cruft pack aggregation works I paused and then wrote what is now\nthe first patch.\n\nHindsight is 20/20, I suppose ;-).\n\nThanks,\nTaylor\n"},{"id":"513193","messageId":"Z8DvLkWys6KaQNJl@nand.local","threadId":"63018","inReplyTo":"CABPp-BGv6J307nTo1sUgAnE+7ZnueSPm4CJLb10wBGQGaPPDWA@mail.gmail.com","subject":"Re: [PATCH 2/2] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-27T23:03:10Z","receivedAt":"2025-02-27T23:03:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Feb 27, 2025 at 11:26:32AM -0800, Elijah Newren wrote:\n> > However, this process breaks down when we attempt to freshen an object\n> > packed in an earlier cruft pack that is larger than the threshold and\n> > thus will survive the repack.\n>\n> ...packed in an earlier cruft pack, and that cruft pack is larger than\n> the threshold...\n>\n> (Otherwise, it's unclear whether you are talking about the object or\n> the cruft pack it is in being larger than the threshold.)\n\nGood suggestion, thanks!\n\n> > When this is the case, it is impossible to freshen objects in cruft\n> > pack(s) which are larger than the threshold. This is because we avoid\n> > writing them in the new cruft pack entirely, for a couple of reasons.\n>\n> ...freshen objects in cruft packs when those cruft packs are larger\n> than the threshold...\n>\n> Again, just to clarify what thing is \"larger\".\n\nLikewise, this makes sense as well, and I applied it in my local copy.\n\n> Also, this paragraph while fine on its own is slightly unclear whether\n> you are discussing pre-patch or post-patch state, which when reading\n> the next two items causes some double takes. Perhaps just spell it\n> out slightly clearer here that for the next two enumerated items you\n> are discussing the existing state previous to your changes?\n\nI adjusted the paragraph before this one to make it a little clearer.\nInstead of saying \"However, [...]\", I rewrote it as \"Prior to this\npatch, however, [...]\".\n\n> > - exists in a non-cruft pack that we are retaining, regardless of that\n> > pack's mtime, or\n> >\n> > - exists in a cruft pack with an mtime more recent than the copy we are\n> > debating whether or not to pack, in which case freshening would be\n> > redundant.\n>\n> s/more recent than/at least as recent as/ ?\n\nThanks for the careful read, and yes, the comparison here is a >= rather\nthan a strict >, and that difference is worth being precise about.\n\n> >\n> > To do this, keep track of whether or not we have any cruft packs in our\n> > in-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\n> > flag. When we end up in this new special case, we replace a call to\n> > 'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only\n> > reject objects when we have a copy in an existing cruft pack with a more\n> > recent mtime (in which case \"freshening\" would be redundant).\n>\n> Again, s/a more recent/at least as recent/ ?\n\nI like this suggestion, but I think the wording ends up a little awkward\nif applied as-is. I turned this sentence into:\n\n [...], and only reject objects when we have a copy in an existing\n cruft pack with at least as recent an mtime as our candidate (in which\n case \"freshening\" would be redundant).\n\nLet me know what you think!\n> > +test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n> > + git init max-cruft-size-threshold &&\n> > + (\n> > + cd max-cruft-size-threshold &&\n> > +\n> > + test_commit base &&\n> > + foo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n> > + bar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n> > + baz=\"$(generate_random_blob baz $((2*1024*1024)))\" &&\n> > +\n> > + test-tool chmtime --get -100000 \\\n> > + \"$objdir/$(test_oid_to_path \"$foo\")\" >foo.old &&\n> > + test-tool chmtime --get -100000 \\\n> > + \"$objdir/$(test_oid_to_path \"$bar\")\" >bar.old &&\n> > + test-tool chmtime --get -100000 \\\n> > + \"$objdir/$(test_oid_to_path \"$baz\")\" >baz.old &&\n> > +\n> > + git repack --cruft -d &&\n> > +\n> > + # Make a packed copy of object $foo with a more recent\n> > + # mtime.\n>\n> s/$foo/foo/ ?\n\nEh. $foo holds the OID of that blob, so \"foo\" on its own doesn't really\nmean anything (even though the implicit meaning is clear from context).\nI think changing it is fine (leaving it alone is equally fine in my\nmind, but I don't feel strongly about it).\n\n> > + foo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n>\n> I thought this was creating a completely different foo, which would\n> defeat the point of the test. It might be worth adding a comment that\n> because generate_random_blob uses a very simplistic and repeatable\n> random character generator with the first argument as the seed, that\n> this will regenerate the same loose object as above for foo.\n\nI think the part of the comment which reads \"packed copy of\" makes it\nclear-ish that we're creating an identical copy, but it doesn't hurt to\nbe more explicit here.\n\nThanks for the careful read!\n\nThanks,\nTaylor\n"},{"id":"513195","messageId":"Z8Dvr3F1t7JKuNQf@nand.local","threadId":"63018","inReplyTo":"CABPp-BEK3T28iqDFxbMHa7GVrythYs+ze7wCZ5Q-LDjOmquiVQ@mail.gmail.com","subject":"Re: [PATCH 0/2] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-27T23:05:19Z","receivedAt":"2025-02-27T23:05:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Feb 27, 2025 at 11:28:18AM -0800, Elijah Newren wrote:\n> > Previously the rule was to discard any object which appears in any\n> > surviving pack, regardless of mtime. The rule now is to only discard an\n> > object if it appears in either (a) a non-cruft pack which will survive\n> > the repack, or (b) a cruft pack whose mtime for that object is older\n> > than the one we are trying to pack.\n>\n> I think in (b) you got the meaning reversed, and instead mean s/older\n> than/at least as new as/ ?\n\nOops, you are definitely right, that is a clear typo on my part.\n\n> Code changes look good to me, but I had some wording suggestions in a\n> few places for commit messages and comments. (Sorry for missing some\n> of those in my preliminary review before you sent this series to the\n> list.)\n\nNo problem. I had only shared them before sending them here for a sanity\ncheck on the implementation, so I was figuring that I'd get some\ncomments here on both the patch message and contents.\n\nI adjusted the wording of both patches slightly to reflect your review.\nI'll hold off on sending another round for a day or so in case any other\nreviewers want to chime in. Thanks again for the careful review!\n\nThanks,\nTaylor\n"},{"id":"513234","messageId":"Z8FrIfAj_oVwxz3X@pks.im","threadId":"63018","inReplyTo":"8564f98259727225391edcb5ab3b47dd53f00e48.1740680964.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] builtin/repack.c: simplify cruft pack aggregation","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-28T07:52:01Z","receivedAt":"2025-02-28T07:52:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 27, 2025 at 01:29:28PM -0500, Taylor Blau wrote:\n> In 37dc6d8104 (builtin/repack.c: implement support for\n> `--max-cruft-size`, 2023-10-02), 'git repack' built on support for\n> multiple cruft packs in Git by instructing 'git pack-objects --cruft'\n> how to aggregate smaller cruft packs up to the provided threshold.\n> \n> The implementation in 37dc6d8104 worked something like the following\n> pseudo-code:\n> \n> total_size = 0;\n> \n> for (p in cruft packs) {\n> if (p->pack_size + total_size < max_size) {\n> total_size += p->pack_size;\n> collapse(p)\n> } else {\n> retain(p);\n> }\n> }\n> \n> The original idea behind this approach was that smaller cruft packs\n> would get combined together until the sum of their sizes was no larger\n> than the given max pack size.\n> \n> There is a much simpler way to achieve this, however, which is to simply\n> combine *all* cruft packs which are smaller than the threshold,\n> regardless of what their sum is. With '--max-pack-size', 'pack-objects'\n> will split out the resulting pack into individual pack(s) if necessary\n> to ensure that the written pack(s) are each no larger than the provided\n> threshold.\n\nHm. So the result would be a new set of packfiles where each of them is\nsmaller than the threshold, right? Wouldn't that mean that the next time\nwe'll again do the same thing and try to combine the new set of cruft\npacks into one, and basically never arrive at a state where we don't\ntouch the cruft packs anymore?\n\nPatrick\n"},{"id":"513572","messageId":"cover.1741124116.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1740680964.git.me@ttaylorr.com","subject":"[PATCH v2 0/2] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-04T21:35:17Z","receivedAt":"2025-03-04T21:35:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This is a minor reroll of my series to fix a bug in freshening objects\nwith multiple cruft packs.\n\nSince last time the only updates to the series are textual changes to\nthe commit message, so everything functionally should be working as it\nwas before.\n\nAs usual, there is a range-diff showing as much below, and the original\ncover letter is as follows:\n\n---\n\nThis short series contains a fix for a bug I noticed while rolling out\nmulti-cruft packs (via 'git repack --max-cruft-size') within GitHub's\ninfrastructure.\n\nThe series is structured as follows:\n\n - The first patch simplifies how 'repack' aggregates cruft packs\n together when their size is below the '--max-cruft-size' or\n '--max-pack-size' threshold. This simplification changes behavior\n slightly, but not in a meaningful way. It occurred to me while\n writing the second patch.\n\n - The second patch describes and fixes the main bug. The gist here is\n that objects which are (a) unreachable, (b) exist in a cruft pack\n being retained, and (c) were freshened to have a more recent mtime\n than any existing cruft copy are unable to be freshened.\n\nThe fix pursued in the second patch changes the rules around when we\nwant to retain an object via builtin/pack-objects.c::want_found_object()\nwhen at least one cruft pack will survive the repack.\n\nPreviously the rule was to discard any object which appears in any\nsurviving pack, regardless of mtime. The rule now is to only discard an\nobject if it appears in either (a) a non-cruft pack which will survive\nthe repack, or (b) a cruft pack whose mtime for that object is older\nthan the one we are trying to pack.\n\nI think that this is the right behavior, but admittedly putting this\nseries together hurt my brain trying to think through all of the cases.\nI'm fairly confident in the testing here as I remember it being fairly\nexhaustive of all interesting cases. But I'd appreciate a sanity check\nfrom others that they too are convinced this is the right approach.\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n builtin/repack.c: simplify cruft pack aggregation\n builtin/pack-objects.c: freshen objects from existing cruft packs\n\n builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n builtin/repack.c | 38 +------------\n packfile.c | 3 +-\n packfile.h | 2 +\n t/t7704-repack-cruft.sh | 105 +++++++++++++++++++++--------------\n 5 files changed, 170 insertions(+), 96 deletions(-)\n\nRange-diff against v1:\n1: 8564f982597 ! 1: 63ea9d4d00e builtin/repack.c: simplify cruft pack aggregation\n @@ Commit message\n would get combined together until the sum of their sizes was no larger\n than the given max pack size.\n \n - There is a much simpler way to achieve this, however, which is to simply\n - combine *all* cruft packs which are smaller than the threshold,\n + There is a much simpler way to combine cruft packs, however, which is to\n + simply combine *all* cruft packs which are smaller than the threshold,\n regardless of what their sum is. With '--max-pack-size', 'pack-objects'\n will split out the resulting pack into individual pack(s) if necessary\n to ensure that the written pack(s) are each no larger than the provided\n2: c0c926adde2 ! 2: 7ba9054701b builtin/pack-objects.c: freshen objects from existing cruft packs\n @@ Commit message\n only be modified in a pruning GC, or if the threshold itself is\n adjusted.\n \n - However, this process breaks down when we attempt to freshen an object\n - packed in an earlier cruft pack that is larger than the threshold and\n - thus will survive the repack.\n + Prior to this patch, however, this process breaks down when we attempt\n + to freshen an object packed in an earlier cruft pack, and that cruft\n + pack is larger than the threshold and thus will survive the repack.\n \n When this is the case, it is impossible to freshen objects in cruft\n - pack(s) which are larger than the threshold. This is because we avoid\n - writing them in the new cruft pack entirely, for a couple of reasons.\n + pack(s) when those cruft packs are larger than the threshold. This is\n + because we would avoid writing them in the new cruft pack entirely, for\n + a couple of reasons.\n \n 1. When enumerating packed objects via 'add_objects_in_unpacked_packs()'\n we pass the SKIP_IN_CORE_KEPT_PACKS, which is used to avoid looping\n @@ Commit message\n - exists in a non-cruft pack that we are retaining, regardless of that\n pack's mtime, or\n \n - - exists in a cruft pack with an mtime more recent than the copy we are\n - debating whether or not to pack, in which case freshening would be\n - redundant.\n + - exists in a cruft pack with an mtime at least as recent as the copy\n + we are debating whether or not to pack, in which case freshening\n + would be redundant.\n \n To do this, keep track of whether or not we have any cruft packs in our\n in-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\n flag. When we end up in this new special case, we replace a call to\n - 'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only\n - reject objects when we have a copy in an existing cruft pack with a more\n - recent mtime (in which case \"freshening\" would be redundant).\n + 'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only reject\n + objects when we have a copy in an existing cruft pack with at least as\n + recent an mtime as our candidate (in which case \"freshening\" would be\n + redundant).\n \n Signed-off-by: Taylor Blau \n \n @@ t/t7704-repack-cruft.sh: test_expect_success '--max-cruft-size with freshened ob\n +\n +\t\tgit repack --cruft -d &&\n +\n -+\t\t# Make a packed copy of object $foo with a more recent\n -+\t\t# mtime.\n ++\t\t# Make an identical copy of foo stored in a pack with a more\n ++\t\t# recent mtime.\n +\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n +\t\tfoo_pack=\"$(echo \"$foo\" | git pack-objects $packdir/pack)\" &&\n +\t\ttest-tool chmtime --get -100 \\\n +\t\t\t\"$packdir/pack-$foo_pack.pack\" >foo.new &&\n +\t\tgit prune-packed &&\n +\n -+\t\t# Make a loose copy of object $bar with a more recent\n -+\t\t# mtime.\n ++\t\t# Make a loose copy of bar, also with a more recent mtime.\n +\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n +\t\ttest-tool chmtime --get -100 \\\n +\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.new &&\n\nbase-commit: 08bdfd453584e489d5a551aecbdcb77584e1b958\n-- \n2.49.0.rc0.57.gdb91954e186.dirty\n"},{"id":"513573","messageId":"63ea9d4d00ee3a54071f1bf585b7cf95227cca9b.1741124117.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1741124116.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] builtin/repack.c: simplify cruft pack aggregation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-04T21:35:20Z","receivedAt":"2025-03-04T21:35:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 37dc6d8104 (builtin/repack.c: implement support for\n`--max-cruft-size`, 2023-10-02), 'git repack' built on support for\nmultiple cruft packs in Git by instructing 'git pack-objects --cruft'\nhow to aggregate smaller cruft packs up to the provided threshold.\n\nThe implementation in 37dc6d8104 worked something like the following\npseudo-code:\n\n total_size = 0;\n\n for (p in cruft packs) {\n if (p->pack_size + total_size < max_size) {\n total_size += p->pack_size;\n collapse(p)\n } else {\n retain(p);\n }\n }\n\nThe original idea behind this approach was that smaller cruft packs\nwould get combined together until the sum of their sizes was no larger\nthan the given max pack size.\n\nThere is a much simpler way to combine cruft packs, however, which is to\nsimply combine *all* cruft packs which are smaller than the threshold,\nregardless of what their sum is. With '--max-pack-size', 'pack-objects'\nwill split out the resulting pack into individual pack(s) if necessary\nto ensure that the written pack(s) are each no larger than the provided\nthreshold.\n\nThis yields a slight behavior change, which is reflected in the removed\ntest. Previous to this change, we would aggregate smaller cruft packs\nfirst, whereas now we will opportunistically combine as many cruft packs\nas possible. As as result, that test is no longer relevant, and can be\ndeleted.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 38 ++-----------------------------------\n t/t7704-repack-cruft.sh | 42 -----------------------------------------\n 2 files changed, 2 insertions(+), 78 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 75e3752353a..4d83d40f39f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1022,29 +1022,13 @@ static int write_filtered_pack(const struct pack_objects_args *args,\n \treturn finish_pack_objects_cmd(&cmd, names, local);\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 packed_git *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@@ -1056,24 +1040,7 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size,\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\tif (p->pack_size < max_size) {\n \t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n \t\t} else {\n \t\t\tretain_cruft_pack(existing, p);\n@@ -1086,7 +1053,6 @@ 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,\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 959e6e26488..5a76b541ddd 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -194,48 +194,6 @@ test_expect_success '--max-cruft-size combines existing packs when below thresho\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-- \n2.49.0.rc0.57.gdb91954e186.dirty\n\n"},{"id":"513574","messageId":"7ba9054701bb84c89fb1d9197978cc5e5e0f887c.1741124117.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1741124116.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-04T21:35:23Z","receivedAt":"2025-03-04T21:35:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Once an object is written into a cruft pack, we can only freshen it by\nwriting a new loose or packed copy of that object with a more recent\nmtime.\n\nPrior to 61568efa95 (builtin/pack-objects.c: support `--max-pack-size`\nwith `--cruft`, 2023-08-28), we typically had at most one cruft pack in\na repository at any given time. So freshening unreachable objects was\nstraightforward when already rewriting the cruft pack (and its *.mtimes\nfile).\n\nBut 61568efa95 changes things: 'pack-objects' now supports writing\nmultiple cruft packs when invoked with `--cruft` and the\n`--max-pack-size` flag. Cruft packs are rewritten until they reach some\nsize threshold, at which point they are considered \"frozen\", and will\nonly be modified in a pruning GC, or if the threshold itself is\nadjusted.\n\nPrior to this patch, however, this process breaks down when we attempt\nto freshen an object packed in an earlier cruft pack, and that cruft\npack is larger than the threshold and thus will survive the repack.\n\nWhen this is the case, it is impossible to freshen objects in cruft\npack(s) when those cruft packs are larger than the threshold. This is\nbecause we would avoid writing them in the new cruft pack entirely, for\na couple of reasons.\n\n 1. When enumerating packed objects via 'add_objects_in_unpacked_packs()'\n we pass the SKIP_IN_CORE_KEPT_PACKS, which is used to avoid looping\n over the packs we're going to retain (which are marked as kept\n in-core by 'read_cruft_objects()').\n\n This means that we will avoid enumerating additional packed copies\n of objects found in any cruft packs which are larger than the given\n size threshold. Thus there is no opportunity to call\n 'create_object_entry()' whatsoever.\n\n 2. We likewise will discard the loose copy (if one exists) of any\n unreachable object packed in a cruft pack that is larger than the\n threshold. Here our call path is 'add_unreachable_loose_objects()',\n which uses the 'add_loose_object()' callback.\n\n That function will eventually land us in 'want_object_in_pack()'\n (via 'add_cruft_object_entry()'), and we'll discard the object as it\n appears in one of the packs which we marked as kept in-core.\n\nThis means in effect that it is impossible to freshen an unreachable\nobject once it appears in a cruft pack larger than the given threshold.\n\nInstead, we should pack an additional copy of an unreachable object we\nwant to freshen even if it appears in a cruft pack, provided that the\ncruft copy has an mtime which is before the mtime of the copy we are\ntrying to pack/freshen. This is sub-optimal in the sense that it\nrequires keeping an additional copy of unreachable objects upon\nfreshening, but we don't have a better alternative without the ability\nto make in-place modifications to existing *.mtimes files.\n\nIn order to implement this, we have to adjust the behavior of\n'want_found_object()'. When 'pack-objects' is told that we're *not*\ngoing to retain any cruft packs (i.e. the set of packs marked as kept\nin-core does not contain a cruft pack), the behavior is unchanged.\n\nBut when there *is* at least one cruft pack that we're holding onto, it\nis no longer sufficient to reject a copy of an object found in that\ncruft pack for that reason alone. In this case, we only want to reject a\ncandidate object when copies of that object either:\n\n - exists in a non-cruft pack that we are retaining, regardless of that\n pack's mtime, or\n\n - exists in a cruft pack with an mtime at least as recent as the copy\n we are debating whether or not to pack, in which case freshening\n would be redundant.\n\nTo do this, keep track of whether or not we have any cruft packs in our\nin-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\nflag. When we end up in this new special case, we replace a call to\n'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only reject\nobjects when we have a copy in an existing cruft pack with at least as\nrecent an mtime as our candidate (in which case \"freshening\" would be\nredundant).\n\nSigned-off-by: Taylor Blau \n---\n builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n packfile.c | 3 +-\n packfile.h | 2 +\n t/t7704-repack-cruft.sh | 63 +++++++++++++++++++++\n 4 files changed, 168 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 58a9b161262..79e1e6fb52b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -206,6 +206,7 @@ static int have_non_local_packs;\n static int incremental;\n static int ignore_packed_keep_on_disk;\n static int ignore_packed_keep_in_core;\n+static int ignore_packed_keep_in_core_has_cruft;\n static int allow_ofs_delta;\n static struct pack_idx_option pack_idx_opts;\n static const char *base_name;\n@@ -1502,8 +1503,60 @@ static int have_duplicate_entry(const struct object_id *oid,\n \treturn 1;\n }\n \n+static int want_cruft_object_mtime(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t unsigned flags, uint32_t mtime)\n+{\n+\tstruct packed_git **cache;\n+\n+\tfor (cache = kept_pack_cache(r, flags); *cache; cache++) {\n+\t\tstruct packed_git *p = *cache;\n+\t\toff_t ofs;\n+\t\tuint32_t candidate_mtime;\n+\n+\t\tofs = find_pack_entry_one(oid, p);\n+\t\tif (!ofs)\n+\t\t\tcontinue;\n+\n+\t\t/*\n+\t\t * We have a copy of the object 'oid' in a non-cruft\n+\t\t * pack. We can avoid packing an additional copy\n+\t\t * regardless of what the existing copy's mtime is since\n+\t\t * it is outside of a cruft pack.\n+\t\t */\n+\t\tif (!p->is_cruft)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * If we have a copy of the object 'oid' in a cruft\n+\t\t * pack, then either read the cruft pack's mtime for\n+\t\t * that object, or, if that can't be loaded, assume the\n+\t\t * pack's mtime itself.\n+\t\t */\n+\t\tif (!load_pack_mtimes(p)) {\n+\t\t\tuint32_t pos;\n+\t\t\tif (offset_to_pack_pos(p, ofs, &pos) < 0)\n+\t\t\t\tcontinue;\n+\t\t\tcandidate_mtime = nth_packed_mtime(p, pos);\n+\t\t} else {\n+\t\t\tcandidate_mtime = p->mtime;\n+\t\t}\n+\n+\t\t/*\n+\t\t * We have a surviving copy of the object in a cruft\n+\t\t * pack whose mtime is greater than or equal to the one\n+\t\t * we are considering. We can thus avoid packing an\n+\t\t * additional copy of that object.\n+\t\t */\n+\t\tif (mtime <= candidate_mtime)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n static int want_found_object(const struct object_id *oid, int exclude,\n-\t\t\t struct packed_git *p)\n+\t\t\t struct packed_git *p, uint32_t mtime)\n {\n \tif (exclude)\n \t\treturn 1;\n@@ -1553,12 +1606,29 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\tif (ignore_packed_keep_in_core)\n \t\t\tflags |= IN_CORE_KEEP_PACKS;\n \n-\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n-\t\t\treturn 0;\n-\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n-\t\t\treturn 0;\n-\t\tif (has_object_kept_pack(p->repo, oid, flags))\n-\t\t\treturn 0;\n+\t\t/*\n+\t\t * If the object is in a pack that we want to ignore, *and* we\n+\t\t * don't have any cruft packs that are being retained, we can\n+\t\t * abort quickly.\n+\t\t */\n+\t\tif (!ignore_packed_keep_in_core_has_cruft) {\n+\t\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n+\t\t\t\treturn 0;\n+\t\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n+\t\t\t\treturn 0;\n+\t\t\tif (has_object_kept_pack(p->repo, oid, flags))\n+\t\t\t\treturn 0;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * But if there is at least one cruft pack which\n+\t\t\t * is being kept, we only want to include the\n+\t\t\t * provided object if it has a strictly greater\n+\t\t\t * mtime than any existing cruft copy.\n+\t\t\t */\n+\t\t\tif (!want_cruft_object_mtime(p->repo, oid, flags,\n+\t\t\t\t\t\t mtime))\n+\t\t\t\treturn 0;\n+\t\t}\n \t}\n \n \t/*\n@@ -1577,7 +1647,8 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t int exclude,\n \t\t\t\t struct packed_git **found_pack,\n-\t\t\t\t off_t *found_offset)\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \toff_t offset;\n \n@@ -1593,7 +1664,7 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t*found_offset = offset;\n \t\t\t*found_pack = p;\n \t\t}\n-\t\treturn want_found_object(oid, exclude, p);\n+\t\treturn want_found_object(oid, exclude, p, found_mtime);\n \t}\n \treturn -1;\n }\n@@ -1607,10 +1678,11 @@ static int want_object_in_pack_one(struct packed_git *p,\n * function finds if there is any pack that has the object and returns the pack\n * and its offset in these variables.\n */\n-static int want_object_in_pack(const struct object_id *oid,\n-\t\t\t int exclude,\n-\t\t\t struct packed_git **found_pack,\n-\t\t\t off_t *found_offset)\n+static int want_object_in_pack_mtime(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \tint want;\n \tstruct list_head *pos;\n@@ -1625,7 +1697,8 @@ static int want_object_in_pack(const struct object_id *oid,\n \t * are present we will determine the answer right now.\n \t */\n \tif (*found_pack) {\n-\t\twant = want_found_object(oid, exclude, *found_pack);\n+\t\twant = want_found_object(oid, exclude, *found_pack,\n+\t\t\t\t\t found_mtime);\n \t\tif (want != -1)\n \t\t\treturn want;\n \n@@ -1636,7 +1709,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \tfor (m = get_multi_pack_index(the_repository); m; m = m->next) {\n \t\tstruct pack_entry e;\n \t\tif (fill_midx_entry(the_repository, oid, &e, m)) {\n-\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset);\n+\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\n \t\t}\n@@ -1644,7 +1717,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \n \tlist_for_each(pos, get_packed_git_mru(the_repository)) {\n \t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n-\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset);\n+\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\tif (!exclude && want > 0)\n \t\t\tlist_move(&p->mru,\n \t\t\t\t get_packed_git_mru(the_repository));\n@@ -1674,6 +1747,15 @@ static int want_object_in_pack(const struct object_id *oid,\n \treturn 1;\n }\n \n+static inline int want_object_in_pack(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset)\n+{\n+\treturn want_object_in_pack_mtime(oid, exclude, found_pack, found_offset,\n+\t\t\t\t\t 0);\n+}\n+\n static struct object_entry *create_object_entry(const struct object_id *oid,\n \t\t\t\t\t\tenum object_type type,\n \t\t\t\t\t\tuint32_t hash,\n@@ -3606,7 +3688,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type\n \t\t\tentry->no_try_delta = no_try_delta(name);\n \t\t}\n \t} else {\n-\t\tif (!want_object_in_pack(oid, 0, &pack, &offset))\n+\t\tif (!want_object_in_pack_mtime(oid, 0, &pack, &offset, mtime))\n \t\t\treturn;\n \t\tif (!pack && type == OBJ_BLOB && !has_loose_object(oid)) {\n \t\t\t/*\n@@ -3680,6 +3762,8 @@ static void mark_pack_kept_in_core(struct string_list *packs, unsigned keep)\n \t\tstruct packed_git *p = item->util;\n \t\tif (!p)\n \t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n+\t\tif (p->is_cruft && keep)\n+\t\t\tignore_packed_keep_in_core_has_cruft = 1;\n \t\tp->pack_keep_in_core = keep;\n \t}\n }\ndiff --git a/packfile.c b/packfile.c\nindex 2d80d80cb38..9d09f8bc726 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -24,6 +24,7 @@\n #include \"commit-graph.h\"\n #include \"pack-revindex.h\"\n #include \"promisor-remote.h\"\n+#include \"pack-mtimes.h\"\n \n char *odb_pack_name(struct repository *r, struct strbuf *buf,\n \t\t const unsigned char *hash, const char *ext)\n@@ -2107,7 +2108,7 @@ static void maybe_invalidate_kept_pack_cache(struct repository *r,\n \tr->objects->kept_pack_cache.flags = 0;\n }\n \n-static struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n {\n \tmaybe_invalidate_kept_pack_cache(r, flags);\n \ndiff --git a/packfile.h b/packfile.h\nindex 00ada7a938f..25097213d06 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -197,6 +197,8 @@ int has_object_pack(struct repository *r, const struct object_id *oid);\n int has_object_kept_pack(struct repository *r, const struct object_id *oid,\n \t\t\t unsigned flags);\n \n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags);\n+\n /*\n * Return 1 if an object in a promisor packfile is or refers to the given\n * object, 0 otherwise.\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 5a76b541ddd..183cb239447 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -262,6 +262,69 @@ test_expect_success '--max-cruft-size with freshened objects (packed)' '\n \t)\n '\n \n+test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n+\tgit init max-cruft-size-threshold &&\n+\t(\n+\t\tcd max-cruft-size-threshold &&\n+\n+\t\ttest_commit base &&\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\tbaz=\"$(generate_random_blob baz $((2*1024*1024)))\" &&\n+\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$foo\")\" >foo.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$baz\")\" >baz.old &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\t# Make an identical copy of foo stored in a pack with a more\n+\t\t# recent mtime.\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tfoo_pack=\"$(echo \"$foo\" | git pack-objects $packdir/pack)\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$packdir/pack-$foo_pack.pack\" >foo.new &&\n+\t\tgit prune-packed &&\n+\n+\t\t# Make a loose copy of bar, also with a more recent mtime.\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.new &&\n+\n+\t\t# Make a new cruft object $quux to ensure we do not\n+\t\t# generate an identical pack to the existing cruft\n+\t\t# pack.\n+\t\tquux=\"$(generate_random_blob quux $((1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$quux\")\" >quux.new &&\n+\n+\t\tgit repack --cruft --max-cruft-size=3M -d &&\n+\n+\t\tfor p in $packdir/pack-*.mtimes\n+\t\tdo\n+\t\t\ttest-tool pack-mtimes \"$(basename \"$p\")\" || return 1\n+\t\tdone >actual.raw &&\n+\t\tsort actual.raw >actual &&\n+\n+\t\t# Among the set of all cruft packs, we should see both\n+\t\t# mtimes for object $foo and $bar, as well as the\n+\t\t# single new copy of $baz.\n+\t\tsort >expect <<-EOF &&\n+\t\t$foo $(cat foo.old)\n+\t\t$foo $(cat foo.new)\n+\t\t$bar $(cat bar.old)\n+\t\t$bar $(cat bar.new)\n+\t\t$baz $(cat baz.old)\n+\t\t$quux $(cat quux.new)\n+\t\tEOF\n+\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-- \n2.49.0.rc0.57.gdb91954e186.dirty\n"},{"id":"513576","messageId":"CABPp-BGnbQdeSC=XUwTWOnr8=PiGMnt1Q0OXkUAzD=ZiaZoUhw@mail.gmail.com","threadId":"63018","inReplyTo":"Z8FrIfAj_oVwxz3X@pks.im","subject":"Re: [PATCH 1/2] builtin/repack.c: simplify cruft pack aggregation","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-04T21:52:58Z","receivedAt":"2025-03-04T21:53:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Feb 27, 2025 at 11:52 PM Patrick Steinhardt wrote:\n>\n> On Thu, Feb 27, 2025 at 01:29:28PM -0500, Taylor Blau wrote:\n> > In 37dc6d8104 (builtin/repack.c: implement support for\n> > `--max-cruft-size`, 2023-10-02), 'git repack' built on support for\n> > multiple cruft packs in Git by instructing 'git pack-objects --cruft'\n> > how to aggregate smaller cruft packs up to the provided threshold.\n> >\n> > The implementation in 37dc6d8104 worked something like the following\n> > pseudo-code:\n> >\n> > total_size = 0;\n> >\n> > for (p in cruft packs) {\n> > if (p->pack_size + total_size < max_size) {\n> > total_size += p->pack_size;\n> > collapse(p)\n> > } else {\n> > retain(p);\n> > }\n> > }\n> >\n> > The original idea behind this approach was that smaller cruft packs\n> > would get combined together until the sum of their sizes was no larger\n> > than the given max pack size.\n> >\n> > There is a much simpler way to achieve this, however, which is to simply\n> > combine *all* cruft packs which are smaller than the threshold,\n> > regardless of what their sum is. With '--max-pack-size', 'pack-objects'\n> > will split out the resulting pack into individual pack(s) if necessary\n> > to ensure that the written pack(s) are each no larger than the provided\n> > threshold.\n>\n> Hm. So the result would be a new set of packfiles where each of them is\n> smaller than the threshold, right?\n\nAre you assuming there's only one threshold, or that --max-pack-size\n== --max-cruft-size?\n\nI read this assuming --max-pack-size >> --max-cruft-size, so the odds\nthat the N packs smaller than --max-cruft-size add up to more than\n--max-pack-size is small -- but even if it does happen, it just\nresults in the cruft packs being split out into a couple packs.\n\n> Wouldn't that mean that the next time\n> we'll again do the same thing and try to combine the new set of cruft\n> packs into one, and basically never arrive at a state where we don't\n> touch the cruft packs anymore?\n\nThis would be a risk if we allow --max-cruft-size to approach or be\nequal to --max-pack-size. (And if --max-pack-size is less than\n--max-cruft-size, then we'll perversely split into even more cruft\npacks rather than combining as intended.)\n"},{"id":"513580","messageId":"CABPp-BHv2_tgeQBKU6v4mnwU8_wOPHfPPRcpRQZ8wEP9P-HovA@mail.gmail.com","threadId":"63018","inReplyTo":"cover.1741124116.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/2] pack-objects: freshen objects with multi-cruft packs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-04T22:55:00Z","receivedAt":"2025-03-04T22:56:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Mar 4, 2025 at 1:35 PM Taylor Blau wrote:\n>\n> This is a minor reroll of my series to fix a bug in freshening objects\n> with multiple cruft packs.\n>\n> Since last time the only updates to the series are textual changes to\n> the commit message, so everything functionally should be working as it\n> was before.\n>\n> As usual, there is a range-diff showing as much below, and the original\n> cover letter is as follows:\n>\n> ---\n>\n> This short series contains a fix for a bug I noticed while rolling out\n> multi-cruft packs (via 'git repack --max-cruft-size') within GitHub's\n> infrastructure.\n>\n> The series is structured as follows:\n>\n> - The first patch simplifies how 'repack' aggregates cruft packs\n> together when their size is below the '--max-cruft-size' or\n> '--max-pack-size' threshold. This simplification changes behavior\n> slightly, but not in a meaningful way. It occurred to me while\n> writing the second patch.\n>\n> - The second patch describes and fixes the main bug. The gist here is\n> that objects which are (a) unreachable, (b) exist in a cruft pack\n> being retained, and (c) were freshened to have a more recent mtime\n> than any existing cruft copy are unable to be freshened.\n>\n> The fix pursued in the second patch changes the rules around when we\n> want to retain an object via builtin/pack-objects.c::want_found_object()\n> when at least one cruft pack will survive the repack.\n>\n> Previously the rule was to discard any object which appears in any\n> surviving pack, regardless of mtime. The rule now is to only discard an\n> object if it appears in either (a) a non-cruft pack which will survive\n> the repack, or (b) a cruft pack whose mtime for that object is older\n> than the one we are trying to pack.\n>\n> I think that this is the right behavior, but admittedly putting this\n> series together hurt my brain trying to think through all of the cases.\n> I'm fairly confident in the testing here as I remember it being fairly\n> exhaustive of all interesting cases. But I'd appreciate a sanity check\n> from others that they too are convinced this is the right approach.\n>\n> Thanks in advance for your review!\n>\n> Taylor Blau (2):\n> builtin/repack.c: simplify cruft pack aggregation\n> builtin/pack-objects.c: freshen objects from existing cruft packs\n>\n> builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n> builtin/repack.c | 38 +------------\n> packfile.c | 3 +-\n> packfile.h | 2 +\n> t/t7704-repack-cruft.sh | 105 +++++++++++++++++++++--------------\n> 5 files changed, 170 insertions(+), 96 deletions(-)\n>\n> Range-diff against v1:\n> 1: 8564f982597 ! 1: 63ea9d4d00e builtin/repack.c: simplify cruft pack aggregation\n> @@ Commit message\n> would get combined together until the sum of their sizes was no larger\n> than the given max pack size.\n>\n> - There is a much simpler way to achieve this, however, which is to simply\n> - combine *all* cruft packs which are smaller than the threshold,\n> + There is a much simpler way to combine cruft packs, however, which is to\n> + simply combine *all* cruft packs which are smaller than the threshold,\n> regardless of what their sum is. With '--max-pack-size', 'pack-objects'\n> will split out the resulting pack into individual pack(s) if necessary\n> to ensure that the written pack(s) are each no larger than the provided\n> 2: c0c926adde2 ! 2: 7ba9054701b builtin/pack-objects.c: freshen objects from existing cruft packs\n> @@ Commit message\n> only be modified in a pruning GC, or if the threshold itself is\n> adjusted.\n>\n> - However, this process breaks down when we attempt to freshen an object\n> - packed in an earlier cruft pack that is larger than the threshold and\n> - thus will survive the repack.\n> + Prior to this patch, however, this process breaks down when we attempt\n> + to freshen an object packed in an earlier cruft pack, and that cruft\n> + pack is larger than the threshold and thus will survive the repack.\n>\n> When this is the case, it is impossible to freshen objects in cruft\n> - pack(s) which are larger than the threshold. This is because we avoid\n> - writing them in the new cruft pack entirely, for a couple of reasons.\n> + pack(s) when those cruft packs are larger than the threshold. This is\n> + because we would avoid writing them in the new cruft pack entirely, for\n> + a couple of reasons.\n>\n> 1. When enumerating packed objects via 'add_objects_in_unpacked_packs()'\n> we pass the SKIP_IN_CORE_KEPT_PACKS, which is used to avoid looping\n> @@ Commit message\n> - exists in a non-cruft pack that we are retaining, regardless of that\n> pack's mtime, or\n>\n> - - exists in a cruft pack with an mtime more recent than the copy we are\n> - debating whether or not to pack, in which case freshening would be\n> - redundant.\n> + - exists in a cruft pack with an mtime at least as recent as the copy\n> + we are debating whether or not to pack, in which case freshening\n> + would be redundant.\n>\n> To do this, keep track of whether or not we have any cruft packs in our\n> in-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\n> flag. When we end up in this new special case, we replace a call to\n> - 'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only\n> - reject objects when we have a copy in an existing cruft pack with a more\n> - recent mtime (in which case \"freshening\" would be redundant).\n> + 'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only reject\n> + objects when we have a copy in an existing cruft pack with at least as\n> + recent an mtime as our candidate (in which case \"freshening\" would be\n> + redundant).\n>\n> Signed-off-by: Taylor Blau \n>\n> @@ t/t7704-repack-cruft.sh: test_expect_success '--max-cruft-size with freshened ob\n> +\n> + git repack --cruft -d &&\n> +\n> -+ # Make a packed copy of object $foo with a more recent\n> -+ # mtime.\n> ++ # Make an identical copy of foo stored in a pack with a more\n> ++ # recent mtime.\n> + foo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n> + foo_pack=\"$(echo \"$foo\" | git pack-objects $packdir/pack)\" &&\n> + test-tool chmtime --get -100 \\\n> + \"$packdir/pack-$foo_pack.pack\" >foo.new &&\n> + git prune-packed &&\n> +\n> -+ # Make a loose copy of object $bar with a more recent\n> -+ # mtime.\n> ++ # Make a loose copy of bar, also with a more recent mtime.\n> + bar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n> + test-tool chmtime --get -100 \\\n> + \"$objdir/$(test_oid_to_path \"$bar\")\" >bar.new &&\n>\n> base-commit: 08bdfd453584e489d5a551aecbdcb77584e1b958\n> --\n> 2.49.0.rc0.57.gdb91954e186.dirty\n\nv2 looks good to me; though I'm curious if some wording improvement in\nthe commit message might help in distinguishing between\n--max-cruft-size and --max-pack-size...and whether we want to provide\nany checks on the relative sizes of the two.\n"},{"id":"513585","messageId":"Z8eVcn1wdnaV2VNH@nand.local","threadId":"63018","inReplyTo":"CABPp-BHv2_tgeQBKU6v4mnwU8_wOPHfPPRcpRQZ8wEP9P-HovA@mail.gmail.com","subject":"Re: [PATCH v2 0/2] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-05T00:06:10Z","receivedAt":"2025-03-05T00:06:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 04, 2025 at 02:55:00PM -0800, Elijah Newren wrote:\n> v2 looks good to me; though I'm curious if some wording improvement in\n> the commit message might help in distinguishing between\n> --max-cruft-size and --max-pack-size...and whether we want to provide\n> any checks on the relative sizes of the two.\n\nThanks for the review! :-)\n\n--max-cruft-size and --max-pack-size are the same thing from\npack-objects' perspective; the two flags exist at the repack level in\ncase you want to set a different maximum pack size for cruft- and\nnon-cruft packs.\n\nBoth end up as a --max-pack-size value when repack invokes pack-objects\nfor the cruft and non-cruft case.\n\nThanks,\nTaylor\n"},{"id":"513589","messageId":"Z8eWU2G7Q8WYptl0@nand.local","threadId":"63018","inReplyTo":"Z8FrIfAj_oVwxz3X@pks.im","subject":"Re: [PATCH 1/2] builtin/repack.c: simplify cruft pack aggregation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-05T00:09:55Z","receivedAt":"2025-03-05T00:09:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Feb 28, 2025 at 08:52:01AM +0100, Patrick Steinhardt wrote:\n> On Thu, Feb 27, 2025 at 01:29:28PM -0500, Taylor Blau wrote:\n> > In 37dc6d8104 (builtin/repack.c: implement support for\n> > `--max-cruft-size`, 2023-10-02), 'git repack' built on support for\n> > multiple cruft packs in Git by instructing 'git pack-objects --cruft'\n> > how to aggregate smaller cruft packs up to the provided threshold.\n> >\n> > The implementation in 37dc6d8104 worked something like the following\n> > pseudo-code:\n> >\n> > total_size = 0;\n> >\n> > for (p in cruft packs) {\n> > if (p->pack_size + total_size < max_size) {\n> > total_size += p->pack_size;\n> > collapse(p)\n> > } else {\n> > retain(p);\n> > }\n> > }\n> >\n> > The original idea behind this approach was that smaller cruft packs\n> > would get combined together until the sum of their sizes was no larger\n> > than the given max pack size.\n> >\n> > There is a much simpler way to achieve this, however, which is to simply\n> > combine *all* cruft packs which are smaller than the threshold,\n> > regardless of what their sum is. With '--max-pack-size', 'pack-objects'\n> > will split out the resulting pack into individual pack(s) if necessary\n> > to ensure that the written pack(s) are each no larger than the provided\n> > threshold.\n>\n> Hm. So the result would be a new set of packfiles where each of them is\n> smaller than the threshold, right? Wouldn't that mean that the next time\n> we'll again do the same thing and try to combine the new set of cruft\n> packs into one, and basically never arrive at a state where we don't\n> touch the cruft packs anymore?\n\nUgh. You are most definitely right, now I remember why I wrote the\noriginal series the way I did ;-).\n\nOf course, I read your message after sending a new round. Let's abort\nthat v2 and I'll send a v3 which just contains the second patch. Thanks\nfor catching.\n\nThanks,\nTaylor\n"},{"id":"513590","messageId":"Z8eXJwyO9Hh9npLQ@nand.local","threadId":"63018","inReplyTo":"Z8eVcn1wdnaV2VNH@nand.local","subject":"Re: [PATCH v2 0/2] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-05T00:13:27Z","receivedAt":"2025-03-05T00:13:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 04, 2025 at 07:06:10PM -0500, Taylor Blau wrote:\n> --max-cruft-size and --max-pack-size are the same thing from\n> pack-objects' perspective; the two flags exist at the repack level in\n> case you want to set a different maximum pack size for cruft- and\n> non-cruft packs.\n>\n> Both end up as a --max-pack-size value when repack invokes pack-objects\n> for the cruft and non-cruft case.\n\nOf course, I should have read Patrick's and your discussion earlier in\nthe thread before sending a new round. The first patch should\n*definitely* not be queued.\n\nI'll send a clean v3 that Junio can apply if he likes.\n\nThanks,\nTaylor\n"},{"id":"513591","messageId":"cover.1741133712.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1740680964.git.me@ttaylorr.com","subject":"[PATCH v3 0/1] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-05T00:15:14Z","receivedAt":"2025-03-05T00:15:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This is a(nother) reroll of my series to fix a bug in freshening objects\nwith multiple cruft packs.\n\nThe only update since last time is that I dropped the first patch, after\nPatrick astutely pointed out a flaw with the approach pursued there.\nThat flaw is why I wrote '--max-cruft-size' in what appeared to be a\nconvoluted fashion, but I couldn't remember my line of thinking at the\ntime.\n\nAs usual, there is a range-diff showing as much below. Thanks again,\nPatrick, for catching what would be a very frustrating issue to deal\nwith later on ;-).\n\nTaylor Blau (1):\n builtin/pack-objects.c: freshen objects from existing cruft packs\n\n builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n packfile.c | 3 +-\n packfile.h | 2 +\n t/t7704-repack-cruft.sh | 63 +++++++++++++++++++++\n 4 files changed, 168 insertions(+), 18 deletions(-)\n\nRange-diff against v2:\n1: 63ea9d4d00e < -: ----------- builtin/repack.c: simplify cruft pack aggregation\n2: 7ba9054701b = 1: 6e93471f9a8 builtin/pack-objects.c: freshen objects from existing cruft packs\n\nbase-commit: 08bdfd453584e489d5a551aecbdcb77584e1b958\n-- \n2.49.0.rc0.1.g6e93471f9a8\n"},{"id":"513592","messageId":"6e93471f9a8e6a3dde36342088748ba17e4f7f95.1741133712.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1741133712.git.me@ttaylorr.com","subject":"[PATCH v3 1/1] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-05T00:15:18Z","receivedAt":"2025-03-05T00:15:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Once an object is written into a cruft pack, we can only freshen it by\nwriting a new loose or packed copy of that object with a more recent\nmtime.\n\nPrior to 61568efa95 (builtin/pack-objects.c: support `--max-pack-size`\nwith `--cruft`, 2023-08-28), we typically had at most one cruft pack in\na repository at any given time. So freshening unreachable objects was\nstraightforward when already rewriting the cruft pack (and its *.mtimes\nfile).\n\nBut 61568efa95 changes things: 'pack-objects' now supports writing\nmultiple cruft packs when invoked with `--cruft` and the\n`--max-pack-size` flag. Cruft packs are rewritten until they reach some\nsize threshold, at which point they are considered \"frozen\", and will\nonly be modified in a pruning GC, or if the threshold itself is\nadjusted.\n\nPrior to this patch, however, this process breaks down when we attempt\nto freshen an object packed in an earlier cruft pack, and that cruft\npack is larger than the threshold and thus will survive the repack.\n\nWhen this is the case, it is impossible to freshen objects in cruft\npack(s) when those cruft packs are larger than the threshold. This is\nbecause we would avoid writing them in the new cruft pack entirely, for\na couple of reasons.\n\n 1. When enumerating packed objects via 'add_objects_in_unpacked_packs()'\n we pass the SKIP_IN_CORE_KEPT_PACKS, which is used to avoid looping\n over the packs we're going to retain (which are marked as kept\n in-core by 'read_cruft_objects()').\n\n This means that we will avoid enumerating additional packed copies\n of objects found in any cruft packs which are larger than the given\n size threshold. Thus there is no opportunity to call\n 'create_object_entry()' whatsoever.\n\n 2. We likewise will discard the loose copy (if one exists) of any\n unreachable object packed in a cruft pack that is larger than the\n threshold. Here our call path is 'add_unreachable_loose_objects()',\n which uses the 'add_loose_object()' callback.\n\n That function will eventually land us in 'want_object_in_pack()'\n (via 'add_cruft_object_entry()'), and we'll discard the object as it\n appears in one of the packs which we marked as kept in-core.\n\nThis means in effect that it is impossible to freshen an unreachable\nobject once it appears in a cruft pack larger than the given threshold.\n\nInstead, we should pack an additional copy of an unreachable object we\nwant to freshen even if it appears in a cruft pack, provided that the\ncruft copy has an mtime which is before the mtime of the copy we are\ntrying to pack/freshen. This is sub-optimal in the sense that it\nrequires keeping an additional copy of unreachable objects upon\nfreshening, but we don't have a better alternative without the ability\nto make in-place modifications to existing *.mtimes files.\n\nIn order to implement this, we have to adjust the behavior of\n'want_found_object()'. When 'pack-objects' is told that we're *not*\ngoing to retain any cruft packs (i.e. the set of packs marked as kept\nin-core does not contain a cruft pack), the behavior is unchanged.\n\nBut when there *is* at least one cruft pack that we're holding onto, it\nis no longer sufficient to reject a copy of an object found in that\ncruft pack for that reason alone. In this case, we only want to reject a\ncandidate object when copies of that object either:\n\n - exists in a non-cruft pack that we are retaining, regardless of that\n pack's mtime, or\n\n - exists in a cruft pack with an mtime at least as recent as the copy\n we are debating whether or not to pack, in which case freshening\n would be redundant.\n\nTo do this, keep track of whether or not we have any cruft packs in our\nin-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\nflag. When we end up in this new special case, we replace a call to\n'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only reject\nobjects when we have a copy in an existing cruft pack with at least as\nrecent an mtime as our candidate (in which case \"freshening\" would be\nredundant).\n\nSigned-off-by: Taylor Blau \n---\n builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n packfile.c | 3 +-\n packfile.h | 2 +\n t/t7704-repack-cruft.sh | 63 +++++++++++++++++++++\n 4 files changed, 168 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 58a9b161262..79e1e6fb52b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -206,6 +206,7 @@ static int have_non_local_packs;\n static int incremental;\n static int ignore_packed_keep_on_disk;\n static int ignore_packed_keep_in_core;\n+static int ignore_packed_keep_in_core_has_cruft;\n static int allow_ofs_delta;\n static struct pack_idx_option pack_idx_opts;\n static const char *base_name;\n@@ -1502,8 +1503,60 @@ static int have_duplicate_entry(const struct object_id *oid,\n \treturn 1;\n }\n \n+static int want_cruft_object_mtime(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t unsigned flags, uint32_t mtime)\n+{\n+\tstruct packed_git **cache;\n+\n+\tfor (cache = kept_pack_cache(r, flags); *cache; cache++) {\n+\t\tstruct packed_git *p = *cache;\n+\t\toff_t ofs;\n+\t\tuint32_t candidate_mtime;\n+\n+\t\tofs = find_pack_entry_one(oid, p);\n+\t\tif (!ofs)\n+\t\t\tcontinue;\n+\n+\t\t/*\n+\t\t * We have a copy of the object 'oid' in a non-cruft\n+\t\t * pack. We can avoid packing an additional copy\n+\t\t * regardless of what the existing copy's mtime is since\n+\t\t * it is outside of a cruft pack.\n+\t\t */\n+\t\tif (!p->is_cruft)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * If we have a copy of the object 'oid' in a cruft\n+\t\t * pack, then either read the cruft pack's mtime for\n+\t\t * that object, or, if that can't be loaded, assume the\n+\t\t * pack's mtime itself.\n+\t\t */\n+\t\tif (!load_pack_mtimes(p)) {\n+\t\t\tuint32_t pos;\n+\t\t\tif (offset_to_pack_pos(p, ofs, &pos) < 0)\n+\t\t\t\tcontinue;\n+\t\t\tcandidate_mtime = nth_packed_mtime(p, pos);\n+\t\t} else {\n+\t\t\tcandidate_mtime = p->mtime;\n+\t\t}\n+\n+\t\t/*\n+\t\t * We have a surviving copy of the object in a cruft\n+\t\t * pack whose mtime is greater than or equal to the one\n+\t\t * we are considering. We can thus avoid packing an\n+\t\t * additional copy of that object.\n+\t\t */\n+\t\tif (mtime <= candidate_mtime)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n static int want_found_object(const struct object_id *oid, int exclude,\n-\t\t\t struct packed_git *p)\n+\t\t\t struct packed_git *p, uint32_t mtime)\n {\n \tif (exclude)\n \t\treturn 1;\n@@ -1553,12 +1606,29 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\tif (ignore_packed_keep_in_core)\n \t\t\tflags |= IN_CORE_KEEP_PACKS;\n \n-\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n-\t\t\treturn 0;\n-\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n-\t\t\treturn 0;\n-\t\tif (has_object_kept_pack(p->repo, oid, flags))\n-\t\t\treturn 0;\n+\t\t/*\n+\t\t * If the object is in a pack that we want to ignore, *and* we\n+\t\t * don't have any cruft packs that are being retained, we can\n+\t\t * abort quickly.\n+\t\t */\n+\t\tif (!ignore_packed_keep_in_core_has_cruft) {\n+\t\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n+\t\t\t\treturn 0;\n+\t\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n+\t\t\t\treturn 0;\n+\t\t\tif (has_object_kept_pack(p->repo, oid, flags))\n+\t\t\t\treturn 0;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * But if there is at least one cruft pack which\n+\t\t\t * is being kept, we only want to include the\n+\t\t\t * provided object if it has a strictly greater\n+\t\t\t * mtime than any existing cruft copy.\n+\t\t\t */\n+\t\t\tif (!want_cruft_object_mtime(p->repo, oid, flags,\n+\t\t\t\t\t\t mtime))\n+\t\t\t\treturn 0;\n+\t\t}\n \t}\n \n \t/*\n@@ -1577,7 +1647,8 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t int exclude,\n \t\t\t\t struct packed_git **found_pack,\n-\t\t\t\t off_t *found_offset)\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \toff_t offset;\n \n@@ -1593,7 +1664,7 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t*found_offset = offset;\n \t\t\t*found_pack = p;\n \t\t}\n-\t\treturn want_found_object(oid, exclude, p);\n+\t\treturn want_found_object(oid, exclude, p, found_mtime);\n \t}\n \treturn -1;\n }\n@@ -1607,10 +1678,11 @@ static int want_object_in_pack_one(struct packed_git *p,\n * function finds if there is any pack that has the object and returns the pack\n * and its offset in these variables.\n */\n-static int want_object_in_pack(const struct object_id *oid,\n-\t\t\t int exclude,\n-\t\t\t struct packed_git **found_pack,\n-\t\t\t off_t *found_offset)\n+static int want_object_in_pack_mtime(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \tint want;\n \tstruct list_head *pos;\n@@ -1625,7 +1697,8 @@ static int want_object_in_pack(const struct object_id *oid,\n \t * are present we will determine the answer right now.\n \t */\n \tif (*found_pack) {\n-\t\twant = want_found_object(oid, exclude, *found_pack);\n+\t\twant = want_found_object(oid, exclude, *found_pack,\n+\t\t\t\t\t found_mtime);\n \t\tif (want != -1)\n \t\t\treturn want;\n \n@@ -1636,7 +1709,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \tfor (m = get_multi_pack_index(the_repository); m; m = m->next) {\n \t\tstruct pack_entry e;\n \t\tif (fill_midx_entry(the_repository, oid, &e, m)) {\n-\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset);\n+\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\n \t\t}\n@@ -1644,7 +1717,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \n \tlist_for_each(pos, get_packed_git_mru(the_repository)) {\n \t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n-\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset);\n+\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\tif (!exclude && want > 0)\n \t\t\tlist_move(&p->mru,\n \t\t\t\t get_packed_git_mru(the_repository));\n@@ -1674,6 +1747,15 @@ static int want_object_in_pack(const struct object_id *oid,\n \treturn 1;\n }\n \n+static inline int want_object_in_pack(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset)\n+{\n+\treturn want_object_in_pack_mtime(oid, exclude, found_pack, found_offset,\n+\t\t\t\t\t 0);\n+}\n+\n static struct object_entry *create_object_entry(const struct object_id *oid,\n \t\t\t\t\t\tenum object_type type,\n \t\t\t\t\t\tuint32_t hash,\n@@ -3606,7 +3688,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type\n \t\t\tentry->no_try_delta = no_try_delta(name);\n \t\t}\n \t} else {\n-\t\tif (!want_object_in_pack(oid, 0, &pack, &offset))\n+\t\tif (!want_object_in_pack_mtime(oid, 0, &pack, &offset, mtime))\n \t\t\treturn;\n \t\tif (!pack && type == OBJ_BLOB && !has_loose_object(oid)) {\n \t\t\t/*\n@@ -3680,6 +3762,8 @@ static void mark_pack_kept_in_core(struct string_list *packs, unsigned keep)\n \t\tstruct packed_git *p = item->util;\n \t\tif (!p)\n \t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n+\t\tif (p->is_cruft && keep)\n+\t\t\tignore_packed_keep_in_core_has_cruft = 1;\n \t\tp->pack_keep_in_core = keep;\n \t}\n }\ndiff --git a/packfile.c b/packfile.c\nindex 2d80d80cb38..9d09f8bc726 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -24,6 +24,7 @@\n #include \"commit-graph.h\"\n #include \"pack-revindex.h\"\n #include \"promisor-remote.h\"\n+#include \"pack-mtimes.h\"\n \n char *odb_pack_name(struct repository *r, struct strbuf *buf,\n \t\t const unsigned char *hash, const char *ext)\n@@ -2107,7 +2108,7 @@ static void maybe_invalidate_kept_pack_cache(struct repository *r,\n \tr->objects->kept_pack_cache.flags = 0;\n }\n \n-static struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n {\n \tmaybe_invalidate_kept_pack_cache(r, flags);\n \ndiff --git a/packfile.h b/packfile.h\nindex 00ada7a938f..25097213d06 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -197,6 +197,8 @@ int has_object_pack(struct repository *r, const struct object_id *oid);\n int has_object_kept_pack(struct repository *r, const struct object_id *oid,\n \t\t\t unsigned flags);\n \n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags);\n+\n /*\n * Return 1 if an object in a promisor packfile is or refers to the given\n * object, 0 otherwise.\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 959e6e26488..f427150de5b 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -304,6 +304,69 @@ test_expect_success '--max-cruft-size with freshened objects (packed)' '\n \t)\n '\n \n+test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n+\tgit init max-cruft-size-threshold &&\n+\t(\n+\t\tcd max-cruft-size-threshold &&\n+\n+\t\ttest_commit base &&\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\tbaz=\"$(generate_random_blob baz $((2*1024*1024)))\" &&\n+\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$foo\")\" >foo.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$baz\")\" >baz.old &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\t# Make an identical copy of foo stored in a pack with a more\n+\t\t# recent mtime.\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tfoo_pack=\"$(echo \"$foo\" | git pack-objects $packdir/pack)\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$packdir/pack-$foo_pack.pack\" >foo.new &&\n+\t\tgit prune-packed &&\n+\n+\t\t# Make a loose copy of bar, also with a more recent mtime.\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.new &&\n+\n+\t\t# Make a new cruft object $quux to ensure we do not\n+\t\t# generate an identical pack to the existing cruft\n+\t\t# pack.\n+\t\tquux=\"$(generate_random_blob quux $((1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$quux\")\" >quux.new &&\n+\n+\t\tgit repack --cruft --max-cruft-size=3M -d &&\n+\n+\t\tfor p in $packdir/pack-*.mtimes\n+\t\tdo\n+\t\t\ttest-tool pack-mtimes \"$(basename \"$p\")\" || return 1\n+\t\tdone >actual.raw &&\n+\t\tsort actual.raw >actual &&\n+\n+\t\t# Among the set of all cruft packs, we should see both\n+\t\t# mtimes for object $foo and $bar, as well as the\n+\t\t# single new copy of $baz.\n+\t\tsort >expect <<-EOF &&\n+\t\t$foo $(cat foo.old)\n+\t\t$foo $(cat foo.new)\n+\t\t$bar $(cat bar.old)\n+\t\t$bar $(cat bar.new)\n+\t\t$baz $(cat baz.old)\n+\t\t$quux $(cat quux.new)\n+\t\tEOF\n+\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-- \n2.49.0.rc0.1.g6e93471f9a8\n"},{"id":"513594","messageId":"xmqqsensmdwk.fsf@gitster.g","threadId":"63018","inReplyTo":"CABPp-BGnbQdeSC=XUwTWOnr8=PiGMnt1Q0OXkUAzD=ZiaZoUhw@mail.gmail.com","subject":"Re: [PATCH 1/2] builtin/repack.c: simplify cruft pack aggregation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-05T02:04:11Z","receivedAt":"2025-03-05T02:04:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> Hm. So the result would be a new set of packfiles where each of them is\n>> smaller than the threshold, right?\n>\n> Are you assuming there's only one threshold, or that --max-pack-size\n> == --max-cruft-size?\n>\n> I read this assuming --max-pack-size >> --max-cruft-size, so the odds\n> that the N packs smaller than --max-cruft-size add up to more than\n> --max-pack-size is small -- but even if it does happen, it just\n> results in the cruft packs being split out into a couple packs.\n\nInteresting observation ;-)\n"},{"id":"513642","messageId":"Z8l5hxNjEOALl_g-@pks.im","threadId":"63018","inReplyTo":"6e93471f9a8e6a3dde36342088748ba17e4f7f95.1741133712.git.me@ttaylorr.com","subject":"Re: [PATCH v3 1/1] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-06T10:31:35Z","receivedAt":"2025-03-06T10:31:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 04, 2025 at 07:15:18PM -0500, Taylor Blau wrote:\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 58a9b161262..79e1e6fb52b 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1502,8 +1503,60 @@ static int have_duplicate_entry(const struct object_id *oid,\n> \treturn 1;\n> }\n> \n> +static int want_cruft_object_mtime(struct repository *r,\n> +\t\t\t\t const struct object_id *oid,\n> +\t\t\t\t unsigned flags, uint32_t mtime)\n> +{\n> +\tstruct packed_git **cache;\n> +\n> +\tfor (cache = kept_pack_cache(r, flags); *cache; cache++) {\n> +\t\tstruct packed_git *p = *cache;\n> +\t\toff_t ofs;\n> +\t\tuint32_t candidate_mtime;\n> +\n> +\t\tofs = find_pack_entry_one(oid, p);\n> +\t\tif (!ofs)\n> +\t\t\tcontinue;\n> +\n> +\t\t/*\n> +\t\t * We have a copy of the object 'oid' in a non-cruft\n> +\t\t * pack. We can avoid packing an additional copy\n> +\t\t * regardless of what the existing copy's mtime is since\n> +\t\t * it is outside of a cruft pack.\n> +\t\t */\n> +\t\tif (!p->is_cruft)\n> +\t\t\treturn 0;\n> +\n> +\t\t/*\n> +\t\t * If we have a copy of the object 'oid' in a cruft\n> +\t\t * pack, then either read the cruft pack's mtime for\n> +\t\t * that object, or, if that can't be loaded, assume the\n> +\t\t * pack's mtime itself.\n> +\t\t */\n> +\t\tif (!load_pack_mtimes(p)) {\n> +\t\t\tuint32_t pos;\n> +\t\t\tif (offset_to_pack_pos(p, ofs, &pos) < 0)\n> +\t\t\t\tcontinue;\n> +\t\t\tcandidate_mtime = nth_packed_mtime(p, pos);\n> +\t\t} else {\n> +\t\t\tcandidate_mtime = p->mtime;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * We have a surviving copy of the object in a cruft\n> +\t\t * pack whose mtime is greater than or equal to the one\n> +\t\t * we are considering. We can thus avoid packing an\n> +\t\t * additional copy of that object.\n> +\t\t */\n> +\t\tif (mtime <= candidate_mtime)\n> +\t\t\treturn 0;\n> +\t}\n> +\n> +\treturn -1;\n> +}\n> +\n\nMinor nit: it is a bit unusual that a negative value, which typically\nindicates an error, is used as a boolean value here to indicate that we\ndon't want to have the object.\n\n> diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\n> index 959e6e26488..f427150de5b 100755\n> --- a/t/t7704-repack-cruft.sh\n> +++ b/t/t7704-repack-cruft.sh\n> @@ -304,6 +304,69 @@ test_expect_success '--max-cruft-size with freshened objects (packed)' '\n> \t)\n> '\n> \n> +test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n> +\tgit init max-cruft-size-threshold &&\n\nLet's also delete the repository via `test_when_finished`.\n\nPatrick\n"},{"id":"513643","messageId":"Z8l5j-uOpits9r0w@pks.im","threadId":"63018","inReplyTo":"cover.1741133712.git.me@ttaylorr.com","subject":"Re: [PATCH v3 0/1] pack-objects: freshen objects with multi-cruft packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-06T10:31:43Z","receivedAt":"2025-03-06T10:31:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 04, 2025 at 07:15:14PM -0500, Taylor Blau wrote:\n> This is a(nother) reroll of my series to fix a bug in freshening objects\n> with multiple cruft packs.\n> \n> The only update since last time is that I dropped the first patch, after\n> Patrick astutely pointed out a flaw with the approach pursued there.\n> That flaw is why I wrote '--max-cruft-size' in what appeared to be a\n> convoluted fashion, but I couldn't remember my line of thinking at the\n> time.\n> \n> As usual, there is a range-diff showing as much below. Thanks again,\n> Patrick, for catching what would be a very frustrating issue to deal\n> with later on ;-).\n\nWould it maybe make sense to add a commit that explains _why_ the\nexisting algorithm looks as it does so that the next person won't be\ntriggered to remove the code again?\n\nPatrick\n"},{"id":"513961","messageId":"cover.1741648467.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1740680964.git.me@ttaylorr.com","subject":"[PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-11T00:21:35Z","receivedAt":"2025-03-11T00:21:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a slightly larger reroll of my series to fix object freshening\nwhen using multi-cruft packs that I have been meaning to send for a\ncouple of days.\n\nI realized after sending the last round that not only was the first\ncommit from v1 flawed (for the reasons Patrick identified) but that\nthere is currently no way to grow a new cruft pack past the configured\nlimit.\n\nIndependent of this series suppose for example that we have two 100 MiB\npacks, and the threshold is 200 MiB. We should able to in theory combine\nthose packs together. But we can't! The largest pack we'll make is\n199MiB (and change), since builtin/pack-objects.c::write_one() will\nrefuse to write any object which would bust the limit given by\n--max-pack-size.\n\nThis series resurrects the first patch from v1 after introducing a\nbehavior change for 'git pack-objects --cruft --max-pack-size'. When\ngiven with '--cruft', '--max-pack-size' now allows pack-objects to grow\na pack *just* past the given limit by at most one object. This allows\npacks to grow past their threshold and age out of the active generation\nof cruft packs so they are no longer repacked with each 'git repack\n--cruft'.\n\nWe're way too late into the -rc cycle for this to land in the\nforthcoming release, but I wanted to get this off of my workstation\nanyway to allow folks to review it as they have time.\n\nAs usual, there is a range-diff since last time. Thanks in advance for\nyour review!\n\nTaylor Blau (6):\n t/t5329-pack-objects-cruft.sh: evict 'repack'-related tests\n t7704-repack-cruft.sh: consolidate `write_blob()`\n t/lib-cruft.sh: extract some cruft-related helpers\n pack-objects: generate cruft packs at most one object over threshold\n builtin/repack.c: simplify cruft pack aggregation\n builtin/pack-objects.c: freshen objects from existing cruft packs\n\n Documentation/config/pack.adoc | 4 +\n Documentation/git-pack-objects.adoc | 4 +\n builtin/pack-objects.c | 150 +++++++++--\n builtin/repack.c | 38 +--\n packfile.c | 3 +-\n packfile.h | 2 +\n t/lib-cruft.sh | 23 ++\n t/t5329-pack-objects-cruft.sh | 317 +++++-------------------\n t/t7704-repack-cruft.sh | 372 +++++++++++++++++++++++-----\n 9 files changed, 544 insertions(+), 369 deletions(-)\n create mode 100644 t/lib-cruft.sh\n\nRange-diff against v3:\n-: ---------- > 1: 390c3a6d85 t/t5329-pack-objects-cruft.sh: evict 'repack'-related tests\n-: ---------- > 2: e7ebe6c460 t7704-repack-cruft.sh: consolidate `write_blob()`\n-: ---------- > 3: aa7588f817 t/lib-cruft.sh: extract some cruft-related helpers\n-: ---------- > 4: f2ca92245a pack-objects: generate cruft packs at most one object over threshold\n-: ---------- > 5: 12ddea7603 builtin/repack.c: simplify cruft pack aggregation\n1: c80188164e = 6: d44a124c81 builtin/pack-objects.c: freshen objects from existing cruft packs\n\nbase-commit: 87a0bdbf0f72b7561f3cd50636eee33dcb7dbcc3\n-- \n2.49.0.rc2.6.g9a1eecd400\n"},{"id":"513962","messageId":"390c3a6d85b287b9d141167a9cce9ce555e5509d.1741648467.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1741648467.git.me@ttaylorr.com","subject":"[PATCH v4 1/6] t/t5329-pack-objects-cruft.sh: evict 'repack'-related tests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-11T00:21:38Z","receivedAt":"2025-03-11T00:21:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The cruft pack feature has two primary test scripts which exercise\nvarious parts of it, which are:\n\n - t5329-pack-objects-cruft.sh\n - t7704-repack-cruft.sh\n\nThe former is designed to test low-level pack generation mechanics at\nthe 'git pack-objects --cruft'-level, which is plumbing. The latter, on\nthe other hand, is designed to test the user-facing behavior through\n'git repack --cruft', which is porcelain (under the \"ancillary\nmanipulators\" sub-section).\n\nAt some point a handful of tests which should have been added to the\nlatter script were instead written to the former. This isn't a huge\ndeal, but rectifying it is straightforward. Move a handful of\n'repack'-related tests out of t5329 and into their rightful home in\nt7704.\n\nSigned-off-by: Taylor Blau \n---\n t/t5329-pack-objects-cruft.sh | 250 ----------------------------------\n t/t7704-repack-cruft.sh | 250 ++++++++++++++++++++++++++++++++++\n 2 files changed, 250 insertions(+), 250 deletions(-)\n\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex b71a0aef40..60dac8312d 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -360,43 +360,6 @@ test_expect_success 'expired objects are pruned' '\n \t)\n '\n \n-test_expect_success 'repack --cruft generates a cruft pack' '\n-\tgit init repo &&\n-\ttest_when_finished \"rm -fr repo\" &&\n-\t(\n-\t\tcd repo &&\n-\n-\t\ttest_commit reachable &&\n-\t\tgit branch -M main &&\n-\t\tgit checkout --orphan other &&\n-\t\ttest_commit unreachable &&\n-\n-\t\tgit checkout main &&\n-\t\tgit branch -D other &&\n-\t\tgit tag -d unreachable &&\n-\t\t# objects are not cruft if they are contained in the reflogs\n-\t\tgit reflog expire --all --expire=all &&\n-\n-\t\tgit rev-list --objects --all --no-object-names >reachable.raw &&\n-\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects &&\n-\t\tsort reachable &&\n-\t\tcomm -13 reachable objects >unreachable &&\n-\n-\t\tgit repack --cruft -d &&\n-\n-\t\tcruft=$(basename $(ls $packdir/pack-*.mtimes) .mtimes) &&\n-\t\tpack=$(basename $(ls $packdir/pack-*.pack | grep -v $cruft) .pack) &&\n-\n-\t\tgit show-index <$packdir/$pack.idx >actual.raw &&\n-\t\tcut -f2 -d\" \" actual.raw | sort >actual &&\n-\t\ttest_cmp reachable actual &&\n-\n-\t\tgit show-index <$packdir/$cruft.idx >actual.raw &&\n-\t\tcut -f2 -d\" \" actual.raw | sort >actual &&\n-\t\ttest_cmp unreachable actual\n-\t)\n-'\n-\n test_expect_success 'loose objects mtimes upsert others' '\n \tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n@@ -470,219 +433,6 @@ test_expect_success 'expiring cruft objects with git gc' '\n \t)\n '\n \n-test_expect_success 'cruft packs are not included in geometric repack' '\n-\tgit init repo &&\n-\ttest_when_finished \"rm -fr repo\" &&\n-\t(\n-\t\tcd repo &&\n-\n-\t\ttest_commit reachable &&\n-\t\tgit repack -Ad &&\n-\t\tgit branch -M main &&\n-\n-\t\tgit checkout --orphan other &&\n-\t\ttest_commit cruft &&\n-\t\tgit repack -d &&\n-\n-\t\tgit checkout main &&\n-\t\tgit branch -D other &&\n-\t\tgit tag -d cruft &&\n-\t\tgit reflog expire --all --expire=all &&\n-\n-\t\tgit repack --cruft &&\n-\n-\t\tfind $packdir -type f | sort >before &&\n-\t\tgit repack --geometric=2 -d &&\n-\t\tfind $packdir -type f | sort >after &&\n-\n-\t\ttest_cmp before after\n-\t)\n-'\n-\n-test_expect_success 'repack --geometric collects once-cruft objects' '\n-\tgit init repo &&\n-\ttest_when_finished \"rm -fr repo\" &&\n-\t(\n-\t\tcd repo &&\n-\n-\t\ttest_commit reachable &&\n-\t\tgit repack -Ad &&\n-\t\tgit branch -M main &&\n-\n-\t\tgit checkout --orphan other &&\n-\t\tgit rm -rf . &&\n-\t\ttest_commit --no-tag cruft &&\n-\t\tcruft=\"$(git rev-parse HEAD)\" &&\n-\n-\t\tgit checkout main &&\n-\t\tgit branch -D other &&\n-\t\tgit reflog expire --all --expire=all &&\n-\n-\t\t# Pack the objects created in the previous step into a cruft\n-\t\t# pack. Intentionally leave loose copies of those objects\n-\t\t# around so we can pick them up in a subsequent --geometric\n-\t\t# reapack.\n-\t\tgit repack --cruft &&\n-\n-\t\t# Now make those objects reachable, and ensure that they are\n-\t\t# packed into the new pack created via a --geometric repack.\n-\t\tgit update-ref refs/heads/other $cruft &&\n-\n-\t\t# Without this object, the set of unpacked objects is exactly\n-\t\t# the set of objects already in the cruft pack. Tweak that set\n-\t\t# to ensure we do not overwrite the cruft pack entirely.\n-\t\ttest_commit reachable2 &&\n-\n-\t\tfind $packdir -name \"pack-*.idx\" | sort >before &&\n-\t\tgit repack --geometric=2 -d &&\n-\t\tfind $packdir -name \"pack-*.idx\" | sort >after &&\n-\n-\t\t{\n-\t\t\tgit rev-list --objects --no-object-names $cruft &&\n-\t\t\tgit rev-list --objects --no-object-names reachable..reachable2\n-\t\t} >want.raw &&\n-\t\tsort want.raw >want &&\n-\n-\t\tpack=$(comm -13 before after) &&\n-\t\tgit show-index <$pack >objects.raw &&\n-\n-\t\tcut -d\" \" -f2 objects.raw | sort >got &&\n-\n-\t\ttest_cmp want got\n-\t)\n-'\n-\n-test_expect_success 'cruft repack with no reachable objects' '\n-\tgit init repo &&\n-\ttest_when_finished \"rm -fr repo\" &&\n-\t(\n-\t\tcd repo &&\n-\n-\t\ttest_commit base &&\n-\t\tgit repack -ad &&\n-\n-\t\tbase=\"$(git rev-parse base)\" &&\n-\n-\t\tgit for-each-ref --format=\"delete %(refname)\" >in &&\n-\t\tgit update-ref --stdin in &&\n-\tgit hash-object -w -t blob in\n-}\n-\n-find_pack () {\n-\tfor idx in $(ls $packdir/pack-*.idx)\n-\tdo\n-\t\tgit show-index <$idx >out &&\n-\t\tif grep -q \"$1\" out\n-\t\tthen\n-\t\t\techo $idx\n-\t\tfi || return 1\n-\tdone\n-}\n-\n-test_expect_success 'cruft repack with --max-pack-size' '\n-\tgit init max-pack-size &&\n-\t(\n-\t\tcd max-pack-size &&\n-\t\ttest_commit base &&\n-\n-\t\t# two cruft objects which exceed the maximum pack size\n-\t\tfoo=$(write_blob foo 1048576) &&\n-\t\tbar=$(write_blob bar 1048576) &&\n-\t\ttest-tool chmtime --get -1000 \\\n-\t\t\t\"$objdir/$(test_oid_to_path $foo)\" >foo.mtime &&\n-\t\ttest-tool chmtime --get -2000 \\\n-\t\t\t\"$objdir/$(test_oid_to_path $bar)\" >bar.mtime &&\n-\t\tgit repack --cruft --max-pack-size=1M &&\n-\t\tfind $packdir -name \"*.mtimes\" >cruft &&\n-\t\ttest_line_count = 2 cruft &&\n-\n-\t\tfoo_mtimes=\"$(basename $(find_pack $foo) .idx).mtimes\" &&\n-\t\tbar_mtimes=\"$(basename $(find_pack $bar) .idx).mtimes\" &&\n-\t\ttest-tool pack-mtimes $foo_mtimes >foo.actual &&\n-\t\ttest-tool pack-mtimes $bar_mtimes >bar.actual &&\n-\n-\t\techo \"$foo $(cat foo.mtime)\" >foo.expect &&\n-\t\techo \"$bar $(cat bar.mtime)\" >bar.expect &&\n-\n-\t\ttest_cmp foo.expect foo.actual &&\n-\t\ttest_cmp bar.expect bar.actual &&\n-\t\ttest \"$foo_mtimes\" != \"$bar_mtimes\"\n-\t)\n-'\n-\n-test_expect_success 'cruft repack with pack.packSizeLimit' '\n-\t(\n-\t\tcd max-pack-size &&\n-\t\t# repack everything back together to remove the existing cruft\n-\t\t# pack (but to keep its objects)\n-\t\tgit repack -adk &&\n-\t\tgit -c pack.packSizeLimit=1M repack --cruft &&\n-\t\t# ensure the same post condition is met when --max-pack-size\n-\t\t# would otherwise be inferred from the configuration\n-\t\tfind $packdir -name \"*.mtimes\" >cruft &&\n-\t\ttest_line_count = 2 cruft &&\n-\t\tfor pack in $(cat cruft)\n-\t\tdo\n-\t\t\ttest-tool pack-mtimes \"$(basename $pack)\" >objects &&\n-\t\t\ttest_line_count = 1 objects || return 1\n-\t\tdone\n-\t)\n-'\n-\n-test_expect_success 'cruft repack respects repack.cruftWindow' '\n-\tgit init repo &&\n-\ttest_when_finished \"rm -fr repo\" &&\n-\t(\n-\t\tcd repo &&\n-\n-\t\ttest_commit base &&\n-\n-\t\tGIT_TRACE2_EVENT=$(pwd)/event.trace \\\n-\t\tgit -c pack.window=1 -c repack.cruftWindow=2 repack \\\n-\t\t --cruft --window=3 &&\n-\n-\t\tgrep \"pack-objects.*--window=2.*--cruft\" event.trace\n-\t)\n-'\n-\n-test_expect_success 'cruft repack respects --window by default' '\n-\tgit init repo &&\n-\ttest_when_finished \"rm -fr repo\" &&\n-\t(\n-\t\tcd repo &&\n-\n-\t\ttest_commit base &&\n-\n-\t\tGIT_TRACE2_EVENT=$(pwd)/event.trace \\\n-\t\tgit -c pack.window=2 repack --cruft --window=3 &&\n-\n-\t\tgrep \"pack-objects.*--window=3.*--cruft\" event.trace\n-\t)\n-'\n-\n-test_expect_success 'cruft repack respects --quiet' '\n-\tgit init repo &&\n-\ttest_when_finished \"rm -fr repo\" &&\n-\t(\n-\t\tcd repo &&\n-\n-\t\ttest_commit base &&\n-\t\tGIT_PROGRESS_DELAY=0 git repack --cruft --quiet 2>err &&\n-\t\ttest_must_be_empty err\n-\t)\n-'\n-\n test_expect_success 'cruft --local drops unreachable objects' '\n \tgit init alternate &&\n \tgit init repo &&\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 959e6e2648..aa5d8913ae 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -411,4 +411,254 @@ test_expect_success 'reachable packs are preferred over cruft ones' '\n \t)\n '\n \n+test_expect_success 'repack --cruft generates a cruft pack' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit reachable &&\n+\t\tgit branch -M main &&\n+\t\tgit checkout --orphan other &&\n+\t\ttest_commit unreachable &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D other &&\n+\t\tgit tag -d unreachable &&\n+\t\t# objects are not cruft if they are contained in the reflogs\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\tgit rev-list --objects --all --no-object-names >reachable.raw &&\n+\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects &&\n+\t\tsort reachable &&\n+\t\tcomm -13 reachable objects >unreachable &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\tcruft=$(basename $(ls $packdir/pack-*.mtimes) .mtimes) &&\n+\t\tpack=$(basename $(ls $packdir/pack-*.pack | grep -v $cruft) .pack) &&\n+\n+\t\tgit show-index <$packdir/$pack.idx >actual.raw &&\n+\t\tcut -f2 -d\" \" actual.raw | sort >actual &&\n+\t\ttest_cmp reachable actual &&\n+\n+\t\tgit show-index <$packdir/$cruft.idx >actual.raw &&\n+\t\tcut -f2 -d\" \" actual.raw | sort >actual &&\n+\t\ttest_cmp unreachable actual\n+\t)\n+'\n+\n+test_expect_success 'cruft packs are not included in geometric repack' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit reachable &&\n+\t\tgit repack -Ad &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout --orphan other &&\n+\t\ttest_commit cruft &&\n+\t\tgit repack -d &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D other &&\n+\t\tgit tag -d cruft &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\tgit repack --cruft &&\n+\n+\t\tfind $packdir -type f | sort >before &&\n+\t\tgit repack --geometric=2 -d &&\n+\t\tfind $packdir -type f | sort >after &&\n+\n+\t\ttest_cmp before after\n+\t)\n+'\n+\n+test_expect_success 'repack --geometric collects once-cruft objects' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit reachable &&\n+\t\tgit repack -Ad &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout --orphan other &&\n+\t\tgit rm -rf . &&\n+\t\ttest_commit --no-tag cruft &&\n+\t\tcruft=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D other &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\t# Pack the objects created in the previous step into a cruft\n+\t\t# pack. Intentionally leave loose copies of those objects\n+\t\t# around so we can pick them up in a subsequent --geometric\n+\t\t# reapack.\n+\t\tgit repack --cruft &&\n+\n+\t\t# Now make those objects reachable, and ensure that they are\n+\t\t# packed into the new pack created via a --geometric repack.\n+\t\tgit update-ref refs/heads/other $cruft &&\n+\n+\t\t# Without this object, the set of unpacked objects is exactly\n+\t\t# the set of objects already in the cruft pack. Tweak that set\n+\t\t# to ensure we do not overwrite the cruft pack entirely.\n+\t\ttest_commit reachable2 &&\n+\n+\t\tfind $packdir -name \"pack-*.idx\" | sort >before &&\n+\t\tgit repack --geometric=2 -d &&\n+\t\tfind $packdir -name \"pack-*.idx\" | sort >after &&\n+\n+\t\t{\n+\t\t\tgit rev-list --objects --no-object-names $cruft &&\n+\t\t\tgit rev-list --objects --no-object-names reachable..reachable2\n+\t\t} >want.raw &&\n+\t\tsort want.raw >want &&\n+\n+\t\tpack=$(comm -13 before after) &&\n+\t\tgit show-index <$pack >objects.raw &&\n+\n+\t\tcut -d\" \" -f2 objects.raw | sort >got &&\n+\n+\t\ttest_cmp want got\n+\t)\n+'\n+\n+test_expect_success 'cruft repack with no reachable objects' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\t\tgit repack -ad &&\n+\n+\t\tbase=\"$(git rev-parse base)\" &&\n+\n+\t\tgit for-each-ref --format=\"delete %(refname)\" >in &&\n+\t\tgit update-ref --stdin in &&\n+\tgit hash-object -w -t blob in\n+}\n+\n+find_pack () {\n+\tfor idx in $(ls $packdir/pack-*.idx)\n+\tdo\n+\t\tgit show-index <$idx >out &&\n+\t\tif grep -q \"$1\" out\n+\t\tthen\n+\t\t\techo $idx\n+\t\tfi || return 1\n+\tdone\n+}\n+\n+test_expect_success 'cruft repack with --max-pack-size' '\n+\tgit init max-pack-size &&\n+\t(\n+\t\tcd max-pack-size &&\n+\t\ttest_commit base &&\n+\n+\t\t# two cruft objects which exceed the maximum pack size\n+\t\tfoo=$(write_blob foo 1048576) &&\n+\t\tbar=$(write_blob bar 1048576) &&\n+\t\ttest-tool chmtime --get -1000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path $foo)\" >foo.mtime &&\n+\t\ttest-tool chmtime --get -2000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path $bar)\" >bar.mtime &&\n+\t\tgit repack --cruft --max-pack-size=1M &&\n+\t\tfind $packdir -name \"*.mtimes\" >cruft &&\n+\t\ttest_line_count = 2 cruft &&\n+\n+\t\tfoo_mtimes=\"$(basename $(find_pack $foo) .idx).mtimes\" &&\n+\t\tbar_mtimes=\"$(basename $(find_pack $bar) .idx).mtimes\" &&\n+\t\ttest-tool pack-mtimes $foo_mtimes >foo.actual &&\n+\t\ttest-tool pack-mtimes $bar_mtimes >bar.actual &&\n+\n+\t\techo \"$foo $(cat foo.mtime)\" >foo.expect &&\n+\t\techo \"$bar $(cat bar.mtime)\" >bar.expect &&\n+\n+\t\ttest_cmp foo.expect foo.actual &&\n+\t\ttest_cmp bar.expect bar.actual &&\n+\t\ttest \"$foo_mtimes\" != \"$bar_mtimes\"\n+\t)\n+'\n+\n+test_expect_success 'cruft repack with pack.packSizeLimit' '\n+\t(\n+\t\tcd max-pack-size &&\n+\t\t# repack everything back together to remove the existing cruft\n+\t\t# pack (but to keep its objects)\n+\t\tgit repack -adk &&\n+\t\tgit -c pack.packSizeLimit=1M repack --cruft &&\n+\t\t# ensure the same post condition is met when --max-pack-size\n+\t\t# would otherwise be inferred from the configuration\n+\t\tfind $packdir -name \"*.mtimes\" >cruft &&\n+\t\ttest_line_count = 2 cruft &&\n+\t\tfor pack in $(cat cruft)\n+\t\tdo\n+\t\t\ttest-tool pack-mtimes \"$(basename $pack)\" >objects &&\n+\t\t\ttest_line_count = 1 objects || return 1\n+\t\tdone\n+\t)\n+'\n+\n+test_expect_success 'cruft repack respects repack.cruftWindow' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\n+\t\tGIT_TRACE2_EVENT=$(pwd)/event.trace \\\n+\t\tgit -c pack.window=1 -c repack.cruftWindow=2 repack \\\n+\t\t --cruft --window=3 &&\n+\n+\t\tgrep \"pack-objects.*--window=2.*--cruft\" event.trace\n+\t)\n+'\n+\n+test_expect_success 'cruft repack respects --window by default' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\n+\t\tGIT_TRACE2_EVENT=$(pwd)/event.trace \\\n+\t\tgit -c pack.window=2 repack --cruft --window=3 &&\n+\n+\t\tgrep \"pack-objects.*--window=3.*--cruft\" event.trace\n+\t)\n+'\n+\n+test_expect_success 'cruft repack respects --quiet' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\t\tGIT_PROGRESS_DELAY=0 git repack --cruft --quiet 2>err &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n test_done\n-- \n2.49.0.rc2.6.g9a1eecd400\n\n"},{"id":"513963","messageId":"e7ebe6c460e2013d565dd87bc4bd584027e97590.1741648467.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1741648467.git.me@ttaylorr.com","subject":"[PATCH v4 2/6] t7704-repack-cruft.sh: consolidate `write_blob()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-11T00:21:41Z","receivedAt":"2025-03-11T00:21:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The last commit moved a handful of tests from a different script into\nt7704, including one that relies on generating random blobs.\n\nIncidentally, the original home of this test defined its own helper\n\"write_blob\" for doing so, which is identical in function to our\n\"generate_random_blob\" (and is slightly inferior to the latter, which\ncleans up after itself).\n\nRewrite the test that uses \"write_blob\" to no longer do so and then\nremove the function.\n\nSigned-off-by: Taylor Blau \n---\n t/t7704-repack-cruft.sh | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex aa5d8913ae..5ce2648a29 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -552,11 +552,6 @@ test_expect_success 'cruft repack with no reachable objects' '\n \t)\n '\n \n-write_blob () {\n-\ttest-tool genrandom \"$@\" >in &&\n-\tgit hash-object -w -t blob in\n-}\n-\n find_pack () {\n \tfor idx in $(ls $packdir/pack-*.idx)\n \tdo\n@@ -575,8 +570,8 @@ test_expect_success 'cruft repack with --max-pack-size' '\n \t\ttest_commit base &&\n \n \t\t# two cruft objects which exceed the maximum pack size\n-\t\tfoo=$(write_blob foo 1048576) &&\n-\t\tbar=$(write_blob bar 1048576) &&\n+\t\tfoo=$(generate_random_blob foo 1048576) &&\n+\t\tbar=$(generate_random_blob bar 1048576) &&\n \t\ttest-tool chmtime --get -1000 \\\n \t\t\t\"$objdir/$(test_oid_to_path $foo)\" >foo.mtime &&\n \t\ttest-tool chmtime --get -2000 \\\n-- \n2.49.0.rc2.6.g9a1eecd400\n\n"},{"id":"513964","messageId":"aa7588f8176b7e699005680697746ba5dcf75005.1741648467.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1741648467.git.me@ttaylorr.com","subject":"[PATCH v4 3/6] t/lib-cruft.sh: extract some cruft-related helpers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-11T00:21:44Z","receivedAt":"2025-03-11T00:21:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The test script in t7704-repack-cruft.sh uses a handful of helpers to\ngenerate and pack random blobs, but a forthcoming test in t5329 will\nwant to make use of generate_random_blob().\n\nInstead of rewriting that function, move it (and the related helpers) to\nits own reusable library to avoid duplicating the function.\n\nSigned-off-by: Taylor Blau \n---\n t/lib-cruft.sh | 23 +++++++++++++++++++++++\n t/t7704-repack-cruft.sh | 22 +---------------------\n 2 files changed, 24 insertions(+), 21 deletions(-)\n create mode 100644 t/lib-cruft.sh\n\ndiff --git a/t/lib-cruft.sh b/t/lib-cruft.sh\nnew file mode 100644\nindex 0000000000..9fdc0b01b0\n--- /dev/null\n+++ b/t/lib-cruft.sh\n@@ -0,0 +1,23 @@\n+# generate_random_blob []\n+generate_random_blob () {\n+\ttest-tool genrandom \"$@\" >blob &&\n+\tgit hash-object -w -t blob blob &&\n+\trm blob\n+}\n+\n+# pack_random_blob []\n+pack_random_blob () {\n+\tgenerate_random_blob \"$@\" &&\n+\tgit repack -d -q >/dev/null\n+}\n+\n+# generate_cruft_pack []\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 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 \nSigned-off-by: Taylor Blau \n---\n Documentation/config/pack.adoc | 4 ++\n Documentation/git-pack-objects.adoc | 4 ++\n builtin/pack-objects.c | 32 +++++++++++++-\n t/t5329-pack-objects-cruft.sh | 67 +++++++++++++++++++++++++++++\n 4 files changed, 105 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc\nindex da527377fa..0a90931b93 100644\n--- a/Documentation/config/pack.adoc\n+++ b/Documentation/config/pack.adoc\n@@ -119,6 +119,10 @@ sizes (e.g., removable media that cannot store the whole repository),\n you are likely better off creating a single large packfile and splitting\n it using a generic multi-volume archive tool (e.g., Unix `split`).\n +\n+When generating cruft packs with `git pack-objects`, this option has a\n+slightly different interpretation than above; see the documentation for\n+`--max-pack-size` option in linkgit:git-pack-objects[1].\n++\n The minimum size allowed is limited to 1 MiB. The default is unlimited.\n Common unit suffixes of 'k', 'm', or 'g' are supported.\n \ndiff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc\nindex 7f69ae4855..aee467c496 100644\n--- a/Documentation/git-pack-objects.adoc\n+++ b/Documentation/git-pack-objects.adoc\n@@ -161,6 +161,10 @@ depth is 4095.\n \t`pack.packSizeLimit` is set. Note that this option may result in\n \ta larger and slower repository; see the discussion in\n \t`pack.packSizeLimit`.\n++\n+When used with `--cruft`, the output packfile(s) may be as large or\n+larger than the configured maximum size. The pack will exceed the\n+specified maximum by no more than one object.\n \n --honor-pack-keep::\n \tThis flag causes an object already in a local pack that\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 58a9b16126..f701b4c9ec 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -692,11 +692,39 @@ static off_t write_object(struct hashfile *f,\n \toff_t len;\n \tint usable_delta, to_reuse;\n \n+\tif (cruft && pack_size_limit && pack_size_limit <= write_offset) {\n+\t\t/*\n+\t\t * When writing a cruft pack with a limited size,\n+\t\t * perform the --max-pack-size check *before* writing\n+\t\t * the object.\n+\t\t *\n+\t\t * When we have not yet reached the size limit, this\n+\t\t * combined with the fact that we act as if there is no\n+\t\t * limit when writing objects via write_object() allows\n+\t\t * us to grow one object *past* the specified limit.\n+\t\t *\n+\t\t * This is important for generating cruft packs with a\n+\t\t * --max-pack-size so we can generate packs that are\n+\t\t * just over the threshold to avoid repacking them in\n+\t\t * the future.\n+\t\t */\n+\t\treturn 0;\n+\t}\n+\n \tif (!pack_to_stdout)\n \t\tcrc32_begin(f);\n \n-\t/* apply size limit if limited packsize and not first object */\n-\tif (!pack_size_limit || !nr_written)\n+\t/*\n+\t * Apply size limit when one is provided, with the following\n+\t * exceptions:\n+\t *\n+\t * - We are writing the first object.\n+\t *\n+\t * - We are writing a cruft pack with a size limit. The check\n+\t * above covers this case while letting the pack grow at most\n+\t * one object beyond the limit.\n+\t */\n+\tif (!pack_size_limit || !nr_written || cruft)\n \t\tlimit = 0;\n \telse if (pack_size_limit <= write_offset)\n \t\t/*\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 60dac8312d..9cbc21a65d 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -3,6 +3,7 @@\n test_description='cruft pack related pack-objects tests'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-cruft.sh\n \n objdir=.git/objects\n packdir=$objdir/pack\n@@ -695,4 +696,70 @@ test_expect_success 'additional cruft blobs via gc.recentObjectsHook' '\n \t)\n '\n \n+test_expect_success 'cruft pack generation beyond --max-pack-size' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Disable pack compression to ensure the pack size is\n+\t\t# predictable.\n+\t\tgit config pack.compression 0 &&\n+\n+\t\tsz=524288 && # 0.5 MiB\n+\t\tfoo=\"$(generate_random_blob foo $sz)\" &&\n+\t\tbar=\"$(generate_random_blob bar $sz)\" &&\n+\t\tbaz=\"$(generate_random_blob baz $sz)\" &&\n+\t\tquux=\"$(generate_random_blob quux $sz)\" &&\n+\n+\t\tprintf \"%s\\n\" \"$foo\" \"$bar\" >A.objects &&\n+\t\tprintf \"%s\\n\" \"$baz\" \"$quux\" >B.objects &&\n+\n+\t\tA=\"$(git pack-objects $packdir/pack C.in &&\n+\t\tgit pack-objects --cruft --max-pack-size=$sz $packdir/pack \\\n+\t\t\tC.out &&\n+\n+\t\ttest_line_count = 2 C.out &&\n+\t\tC_large=\"$(head -n 1 C.out)\" &&\n+\t\tC_small=\"$(tail -n 1 C.out)\" &&\n+\n+\t\t# Swap $C_large and $C_small if necessary.\n+\t\tif test \"$(test_file_size $packdir/pack-$C_large.idx)\" -lt \\\n+\t\t\t\"$(test_file_size $packdir/pack-$C_small.idx)\"\n+\t\tthen\n+\t\t\ttmp=\"$C_large\" &&\n+\t\t\tC_large=\"$C_small\" &&\n+\t\t\tC_small=\"$tmp\"\n+\t\tfi &&\n+\n+\t\t# Ensure the large pack is no smaller than the threshold\n+\t\t# such that it does not get repacked in subsequent runs\n+\t\t# with the same --max-pack-size setting.\n+\t\ttest $(test_file_size $packdir/pack-$C_large.pack) -ge $sz &&\n+\n+\t\t{\n+\t\t\tgit show-index <\"$packdir/pack-$C_large.idx\" &&\n+\t\t\tgit show-index <\"$packdir/pack-$C_small.idx\"\n+\t\t} >actual.raw &&\n+\t\tprintf \"%s\\n\" \"$foo\" \"$bar\" \"$baz\" \"$quux\" >expect.raw &&\n+\n+\t\tsort expect &&\n+\t\tcut -d \" \" -f 2 actual.raw | sort >actual &&\n+\n+\t\t# Ensure that all of the objects are present in the two\n+\t\t# cruft packs we just generated.\n+\t\t#\n+\t\t# Note that the contents of \"actual\" are not\n+\t\t# de-duplicated. This is intentional to ensure we avoid\n+\t\t# packing the same object twice (once in each pack).\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.49.0.rc2.6.g9a1eecd400\n\n"},{"id":"513966","messageId":"12ddea7603e305e8a7af5c05dbe021add834f2fc.1741648467.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1741648467.git.me@ttaylorr.com","subject":"[PATCH v4 5/6] builtin/repack.c: simplify cruft pack aggregation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-11T00:21:49Z","receivedAt":"2025-03-11T00:21:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 37dc6d8104 (builtin/repack.c: implement support for\n`--max-cruft-size`, 2023-10-02), 'git repack' built on support for\nmultiple cruft packs in Git by instructing 'git pack-objects --cruft'\nhow to aggregate smaller cruft packs up to the provided threshold.\n\nThe implementation in 37dc6d8104 worked something like the following\npseudo-code:\n\n total_size = 0;\n\n for (p in cruft packs) {\n if (p->pack_size + total_size < max_size) {\n total_size += p->pack_size;\n collapse(p)\n } else {\n retain(p);\n }\n }\n\nThe original idea behind this approach was that smaller cruft packs\nwould get combined together until the sum of their sizes was no larger\nthan the given max pack size.\n\nThere is a much simpler way to achieve this, however, which is to simply\ncombine *all* cruft packs which are smaller than the threshold,\nregardless of what their sum is. With '--max-pack-size', 'pack-objects'\nwill split out the resulting pack into individual pack(s) if necessary\nto ensure that the written pack(s) are each at most one object larger\nthan the provided threshold.\n\nThis yields a slight behavior change, which is reflected in the removed\ntest. Previous to this change, we would aggregate smaller cruft packs\nfirst, whereas now we will opportunistically combine as many cruft packs\nas possible. As as result, that test is no longer relevant, and can be\ndeleted.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 38 ++-----------------------------------\n t/t7704-repack-cruft.sh | 42 -----------------------------------------\n 2 files changed, 2 insertions(+), 78 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 75e3752353..4d83d40f39 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1022,29 +1022,13 @@ static int write_filtered_pack(const struct pack_objects_args *args,\n \treturn finish_pack_objects_cmd(&cmd, names, local);\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 packed_git *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@@ -1056,24 +1040,7 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size,\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\tif (p->pack_size < max_size) {\n \t\t\tfprintf(in, \"-%s\\n\", pack_basename(p));\n \t\t} else {\n \t\t\tretain_cruft_pack(existing, p);\n@@ -1086,7 +1053,6 @@ 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,\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 88c6ce2913..fb52bb36a2 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -174,48 +174,6 @@ test_expect_success '--max-cruft-size combines existing packs when below thresho\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-- \n2.49.0.rc2.6.g9a1eecd400\n\n"},{"id":"513967","messageId":"d44a124c81386bbbdb06522d0af2765b05fb8e84.1741648467.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1741648467.git.me@ttaylorr.com","subject":"[PATCH v4 6/6] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-11T00:21:52Z","receivedAt":"2025-03-11T00:21:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Once an object is written into a cruft pack, we can only freshen it by\nwriting a new loose or packed copy of that object with a more recent\nmtime.\n\nPrior to 61568efa95 (builtin/pack-objects.c: support `--max-pack-size`\nwith `--cruft`, 2023-08-28), we typically had at most one cruft pack in\na repository at any given time. So freshening unreachable objects was\nstraightforward when already rewriting the cruft pack (and its *.mtimes\nfile).\n\nBut 61568efa95 changes things: 'pack-objects' now supports writing\nmultiple cruft packs when invoked with `--cruft` and the\n`--max-pack-size` flag. Cruft packs are rewritten until they reach some\nsize threshold, at which point they are considered \"frozen\", and will\nonly be modified in a pruning GC, or if the threshold itself is\nadjusted.\n\nPrior to this patch, however, this process breaks down when we attempt\nto freshen an object packed in an earlier cruft pack, and that cruft\npack is larger than the threshold and thus will survive the repack.\n\nWhen this is the case, it is impossible to freshen objects in cruft\npack(s) when those cruft packs are larger than the threshold. This is\nbecause we would avoid writing them in the new cruft pack entirely, for\na couple of reasons.\n\n 1. When enumerating packed objects via 'add_objects_in_unpacked_packs()'\n we pass the SKIP_IN_CORE_KEPT_PACKS, which is used to avoid looping\n over the packs we're going to retain (which are marked as kept\n in-core by 'read_cruft_objects()').\n\n This means that we will avoid enumerating additional packed copies\n of objects found in any cruft packs which are larger than the given\n size threshold. Thus there is no opportunity to call\n 'create_object_entry()' whatsoever.\n\n 2. We likewise will discard the loose copy (if one exists) of any\n unreachable object packed in a cruft pack that is larger than the\n threshold. Here our call path is 'add_unreachable_loose_objects()',\n which uses the 'add_loose_object()' callback.\n\n That function will eventually land us in 'want_object_in_pack()'\n (via 'add_cruft_object_entry()'), and we'll discard the object as it\n appears in one of the packs which we marked as kept in-core.\n\nThis means in effect that it is impossible to freshen an unreachable\nobject once it appears in a cruft pack larger than the given threshold.\n\nInstead, we should pack an additional copy of an unreachable object we\nwant to freshen even if it appears in a cruft pack, provided that the\ncruft copy has an mtime which is before the mtime of the copy we are\ntrying to pack/freshen. This is sub-optimal in the sense that it\nrequires keeping an additional copy of unreachable objects upon\nfreshening, but we don't have a better alternative without the ability\nto make in-place modifications to existing *.mtimes files.\n\nIn order to implement this, we have to adjust the behavior of\n'want_found_object()'. When 'pack-objects' is told that we're *not*\ngoing to retain any cruft packs (i.e. the set of packs marked as kept\nin-core does not contain a cruft pack), the behavior is unchanged.\n\nBut when there *is* at least one cruft pack that we're holding onto, it\nis no longer sufficient to reject a copy of an object found in that\ncruft pack for that reason alone. In this case, we only want to reject a\ncandidate object when copies of that object either:\n\n - exists in a non-cruft pack that we are retaining, regardless of that\n pack's mtime, or\n\n - exists in a cruft pack with an mtime at least as recent as the copy\n we are debating whether or not to pack, in which case freshening\n would be redundant.\n\nTo do this, keep track of whether or not we have any cruft packs in our\nin-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\nflag. When we end up in this new special case, we replace a call to\n'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only reject\nobjects when we have a copy in an existing cruft pack with at least as\nrecent an mtime as our candidate (in which case \"freshening\" would be\nredundant).\n\nSigned-off-by: Taylor Blau \n---\n builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n packfile.c | 3 +-\n packfile.h | 2 +\n t/t7704-repack-cruft.sh | 63 +++++++++++++++++++++\n 4 files changed, 168 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f701b4c9ec..1a3e7dd3d3 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -206,6 +206,7 @@ static int have_non_local_packs;\n static int incremental;\n static int ignore_packed_keep_on_disk;\n static int ignore_packed_keep_in_core;\n+static int ignore_packed_keep_in_core_has_cruft;\n static int allow_ofs_delta;\n static struct pack_idx_option pack_idx_opts;\n static const char *base_name;\n@@ -1530,8 +1531,60 @@ static int have_duplicate_entry(const struct object_id *oid,\n \treturn 1;\n }\n \n+static int want_cruft_object_mtime(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t unsigned flags, uint32_t mtime)\n+{\n+\tstruct packed_git **cache;\n+\n+\tfor (cache = kept_pack_cache(r, flags); *cache; cache++) {\n+\t\tstruct packed_git *p = *cache;\n+\t\toff_t ofs;\n+\t\tuint32_t candidate_mtime;\n+\n+\t\tofs = find_pack_entry_one(oid, p);\n+\t\tif (!ofs)\n+\t\t\tcontinue;\n+\n+\t\t/*\n+\t\t * We have a copy of the object 'oid' in a non-cruft\n+\t\t * pack. We can avoid packing an additional copy\n+\t\t * regardless of what the existing copy's mtime is since\n+\t\t * it is outside of a cruft pack.\n+\t\t */\n+\t\tif (!p->is_cruft)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * If we have a copy of the object 'oid' in a cruft\n+\t\t * pack, then either read the cruft pack's mtime for\n+\t\t * that object, or, if that can't be loaded, assume the\n+\t\t * pack's mtime itself.\n+\t\t */\n+\t\tif (!load_pack_mtimes(p)) {\n+\t\t\tuint32_t pos;\n+\t\t\tif (offset_to_pack_pos(p, ofs, &pos) < 0)\n+\t\t\t\tcontinue;\n+\t\t\tcandidate_mtime = nth_packed_mtime(p, pos);\n+\t\t} else {\n+\t\t\tcandidate_mtime = p->mtime;\n+\t\t}\n+\n+\t\t/*\n+\t\t * We have a surviving copy of the object in a cruft\n+\t\t * pack whose mtime is greater than or equal to the one\n+\t\t * we are considering. We can thus avoid packing an\n+\t\t * additional copy of that object.\n+\t\t */\n+\t\tif (mtime <= candidate_mtime)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n static int want_found_object(const struct object_id *oid, int exclude,\n-\t\t\t struct packed_git *p)\n+\t\t\t struct packed_git *p, uint32_t mtime)\n {\n \tif (exclude)\n \t\treturn 1;\n@@ -1581,12 +1634,29 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\tif (ignore_packed_keep_in_core)\n \t\t\tflags |= IN_CORE_KEEP_PACKS;\n \n-\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n-\t\t\treturn 0;\n-\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n-\t\t\treturn 0;\n-\t\tif (has_object_kept_pack(p->repo, oid, flags))\n-\t\t\treturn 0;\n+\t\t/*\n+\t\t * If the object is in a pack that we want to ignore, *and* we\n+\t\t * don't have any cruft packs that are being retained, we can\n+\t\t * abort quickly.\n+\t\t */\n+\t\tif (!ignore_packed_keep_in_core_has_cruft) {\n+\t\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n+\t\t\t\treturn 0;\n+\t\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n+\t\t\t\treturn 0;\n+\t\t\tif (has_object_kept_pack(p->repo, oid, flags))\n+\t\t\t\treturn 0;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * But if there is at least one cruft pack which\n+\t\t\t * is being kept, we only want to include the\n+\t\t\t * provided object if it has a strictly greater\n+\t\t\t * mtime than any existing cruft copy.\n+\t\t\t */\n+\t\t\tif (!want_cruft_object_mtime(p->repo, oid, flags,\n+\t\t\t\t\t\t mtime))\n+\t\t\t\treturn 0;\n+\t\t}\n \t}\n \n \t/*\n@@ -1605,7 +1675,8 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t int exclude,\n \t\t\t\t struct packed_git **found_pack,\n-\t\t\t\t off_t *found_offset)\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \toff_t offset;\n \n@@ -1621,7 +1692,7 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t*found_offset = offset;\n \t\t\t*found_pack = p;\n \t\t}\n-\t\treturn want_found_object(oid, exclude, p);\n+\t\treturn want_found_object(oid, exclude, p, found_mtime);\n \t}\n \treturn -1;\n }\n@@ -1635,10 +1706,11 @@ static int want_object_in_pack_one(struct packed_git *p,\n * function finds if there is any pack that has the object and returns the pack\n * and its offset in these variables.\n */\n-static int want_object_in_pack(const struct object_id *oid,\n-\t\t\t int exclude,\n-\t\t\t struct packed_git **found_pack,\n-\t\t\t off_t *found_offset)\n+static int want_object_in_pack_mtime(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \tint want;\n \tstruct list_head *pos;\n@@ -1653,7 +1725,8 @@ static int want_object_in_pack(const struct object_id *oid,\n \t * are present we will determine the answer right now.\n \t */\n \tif (*found_pack) {\n-\t\twant = want_found_object(oid, exclude, *found_pack);\n+\t\twant = want_found_object(oid, exclude, *found_pack,\n+\t\t\t\t\t found_mtime);\n \t\tif (want != -1)\n \t\t\treturn want;\n \n@@ -1664,7 +1737,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \tfor (m = get_multi_pack_index(the_repository); m; m = m->next) {\n \t\tstruct pack_entry e;\n \t\tif (fill_midx_entry(the_repository, oid, &e, m)) {\n-\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset);\n+\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\n \t\t}\n@@ -1672,7 +1745,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \n \tlist_for_each(pos, get_packed_git_mru(the_repository)) {\n \t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n-\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset);\n+\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\tif (!exclude && want > 0)\n \t\t\tlist_move(&p->mru,\n \t\t\t\t get_packed_git_mru(the_repository));\n@@ -1702,6 +1775,15 @@ static int want_object_in_pack(const struct object_id *oid,\n \treturn 1;\n }\n \n+static inline int want_object_in_pack(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset)\n+{\n+\treturn want_object_in_pack_mtime(oid, exclude, found_pack, found_offset,\n+\t\t\t\t\t 0);\n+}\n+\n static struct object_entry *create_object_entry(const struct object_id *oid,\n \t\t\t\t\t\tenum object_type type,\n \t\t\t\t\t\tuint32_t hash,\n@@ -3634,7 +3716,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type\n \t\t\tentry->no_try_delta = no_try_delta(name);\n \t\t}\n \t} else {\n-\t\tif (!want_object_in_pack(oid, 0, &pack, &offset))\n+\t\tif (!want_object_in_pack_mtime(oid, 0, &pack, &offset, mtime))\n \t\t\treturn;\n \t\tif (!pack && type == OBJ_BLOB && !has_loose_object(oid)) {\n \t\t\t/*\n@@ -3708,6 +3790,8 @@ static void mark_pack_kept_in_core(struct string_list *packs, unsigned keep)\n \t\tstruct packed_git *p = item->util;\n \t\tif (!p)\n \t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n+\t\tif (p->is_cruft && keep)\n+\t\t\tignore_packed_keep_in_core_has_cruft = 1;\n \t\tp->pack_keep_in_core = keep;\n \t}\n }\ndiff --git a/packfile.c b/packfile.c\nindex 2d80d80cb3..9d09f8bc72 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -24,6 +24,7 @@\n #include \"commit-graph.h\"\n #include \"pack-revindex.h\"\n #include \"promisor-remote.h\"\n+#include \"pack-mtimes.h\"\n \n char *odb_pack_name(struct repository *r, struct strbuf *buf,\n \t\t const unsigned char *hash, const char *ext)\n@@ -2107,7 +2108,7 @@ static void maybe_invalidate_kept_pack_cache(struct repository *r,\n \tr->objects->kept_pack_cache.flags = 0;\n }\n \n-static struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n {\n \tmaybe_invalidate_kept_pack_cache(r, flags);\n \ndiff --git a/packfile.h b/packfile.h\nindex 00ada7a938..25097213d0 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -197,6 +197,8 @@ int has_object_pack(struct repository *r, const struct object_id *oid);\n int has_object_kept_pack(struct repository *r, const struct object_id *oid,\n \t\t\t unsigned flags);\n \n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags);\n+\n /*\n * Return 1 if an object in a promisor packfile is or refers to the given\n * object, 0 otherwise.\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex fb52bb36a2..3082e65817 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -242,6 +242,69 @@ test_expect_success '--max-cruft-size with freshened objects (packed)' '\n \t)\n '\n \n+test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n+\tgit init max-cruft-size-threshold &&\n+\t(\n+\t\tcd max-cruft-size-threshold &&\n+\n+\t\ttest_commit base &&\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\tbaz=\"$(generate_random_blob baz $((2*1024*1024)))\" &&\n+\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$foo\")\" >foo.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$baz\")\" >baz.old &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\t# Make an identical copy of foo stored in a pack with a more\n+\t\t# recent mtime.\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tfoo_pack=\"$(echo \"$foo\" | git pack-objects $packdir/pack)\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$packdir/pack-$foo_pack.pack\" >foo.new &&\n+\t\tgit prune-packed &&\n+\n+\t\t# Make a loose copy of bar, also with a more recent mtime.\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.new &&\n+\n+\t\t# Make a new cruft object $quux to ensure we do not\n+\t\t# generate an identical pack to the existing cruft\n+\t\t# pack.\n+\t\tquux=\"$(generate_random_blob quux $((1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$quux\")\" >quux.new &&\n+\n+\t\tgit repack --cruft --max-cruft-size=3M -d &&\n+\n+\t\tfor p in $packdir/pack-*.mtimes\n+\t\tdo\n+\t\t\ttest-tool pack-mtimes \"$(basename \"$p\")\" || return 1\n+\t\tdone >actual.raw &&\n+\t\tsort actual.raw >actual &&\n+\n+\t\t# Among the set of all cruft packs, we should see both\n+\t\t# mtimes for object $foo and $bar, as well as the\n+\t\t# single new copy of $baz.\n+\t\tsort >expect <<-EOF &&\n+\t\t$foo $(cat foo.old)\n+\t\t$foo $(cat foo.new)\n+\t\t$bar $(cat bar.old)\n+\t\t$bar $(cat bar.new)\n+\t\t$baz $(cat baz.old)\n+\t\t$quux $(cat quux.new)\n+\t\tEOF\n+\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-- \n2.49.0.rc2.6.g9a1eecd400\n"},{"id":"513999","messageId":"xmqqr0332un3.fsf@gitster.g","threadId":"63018","inReplyTo":"cover.1741648467.git.me@ttaylorr.com","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-11T20:13:20Z","receivedAt":"2025-03-11T20:13:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Here is a slightly larger reroll of my series to fix object freshening\n> when using multi-cruft packs that I have been meaning to send for a\n> couple of days.\n>\n> I realized after sending the last round that not only was the first\n> commit from v1 flawed (for the reasons Patrick identified) but that\n> there is currently no way to grow a new cruft pack past the configured\n> limit.\n>\n> Independent of this series suppose for example that we have two 100 MiB\n> packs, and the threshold is 200 MiB. We should able to in theory combine\n> those packs together. But we can't! The largest pack we'll make is\n> 199MiB (and change), since builtin/pack-objects.c::write_one() will\n> refuse to write any object which would bust the limit given by\n> --max-pack-size.\n\nI am not sure why that is a problem. If we have many loose objects\nand the threshold is set at 200, wouldn't we also give up at 199\nplus a change when packing these loose objects into a pack? If the\nlast object that makes us bust the threshold is unusually large, say\n50, we may give up at 150 plus a bit, unless we go back to the queue\nand pick smaller objects among the remaining ones to fill the\nremaining 50 minus a bit, and because we do not do that to enforce\nmax-pack-size, I am not sure how \"give up just before the threshold\"\nis too bad and needs to be replaced with \"give up just after\".\n\nOr is the problem that the threshold is applied differently based on\nwhere the objects come from? E.g., packing many loose objects would\nstop just after, but repacking from cruft would stop just before, or\nsomething? If the problem is that we are inconsistent, then I would\nunderstand that it may be good to make things consistent.\n\n> This series resurrects the first patch from v1 after introducing a\n> behavior change for 'git pack-objects --cruft --max-pack-size'. When\n> given with '--cruft', '--max-pack-size' now allows pack-objects to grow\n> a pack *just* past the given limit by at most one object.\n\nAnd what happens when the last object appended is very large, like\n70? Would we end up with 270 when the threshold says 200?\n\nI still am not getting what you are trying to explain in the above\ntwo paragraphs, but in general, \"give up just before\" would be a\nbetter choice than \"give up just after\", exactly because the threshold\nwe are letting the user to give is the maximum.\n\n\n\n"},{"id":"514008","messageId":"xmqqikof2pqp.fsf@gitster.g","threadId":"63018","inReplyTo":"f2ca92245ada74825806b50f786aab312275fd85.1741648467.git.me@ttaylorr.com","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-11T21:59:10Z","receivedAt":"2025-03-11T21:59:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> When generating multiple cruft packs with 'git repack --max-cruft-size',\n> we use 'git pack-objects --cruft --max-pack-size' (with many other\n> elided options), filling in the '--max-pack-size' value with whatever\n> was provided via the '--max-cruft-size' flag.\n>\n> This causes us to generate a pack that is smaller than the specified\n> threshold. This poses a problem since we will never be able to generate\n> a cruft pack that crosses the threshold.\n\nSo far I see absolutely *NO* problem described in the above. The\nuser said \"I want to chop them into 200MB pieces but do not exceed\nthe threshold\" and the system honored that wish.\n\n> In effect, this means that we\n> will try and repack its contents over and over again.\n\nThe end effect however may be problematic, but isn't it due to the\nway when to repack is determined? You see 199MB piece of cruft pack\nplus some other cruft data. You have generated no new cruft and no\nexisting cruft expired out, but you do not know these facts until\nyou try to repack. Because 200MB is the limit, you include the\n199MB one as part of the ones to be recombined into the new cruft\npack because 199MB is smaller than 200MB and you do not know that\nthe reason why it is 199MB is because the earlier repack operation\nfound all remaining cruft material to be larger than 1MB; if there\nwere a 0.5MB cruft, it may have made it closer to 200MB.\n\nSo would it be feasible to remember how 199MB cruft pack is lying in\nthe object store (i.e. earlier we packed as much as possible), and\nadd a logic that says \"if there is nothing to expire out of this\none, do not attempt to repack---this is fine as-is\"?\n\n> Instead, change the meaning of '--max-pack-size' in pack-objects when\n> combined with '--cruft'. When put together, '--max-pack-size' allows the\n> pack to grow larger than the specified threshold, but only by one\n> additional object.\n\nI do not think that would work well. You have no control over the\nsize of that one additional object---it may weigh more than 100MB,\ncombining your 199MB cruft pack with something else to make it ~300MB\ncruft. In other words, \"just a little bit larger\" sounds like a\nwishful thinking handwaving.\n"},{"id":"514061","messageId":"Z9Gmo2P3Fnt3JeOs@nand.local","threadId":"63018","inReplyTo":"xmqqikof2pqp.fsf@gitster.g","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T15:22:11Z","receivedAt":"2025-03-12T15:22:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 11, 2025 at 02:59:10PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > When generating multiple cruft packs with 'git repack --max-cruft-size',\n> > we use 'git pack-objects --cruft --max-pack-size' (with many other\n> > elided options), filling in the '--max-pack-size' value with whatever\n> > was provided via the '--max-cruft-size' flag.\n> >\n> > This causes us to generate a pack that is smaller than the specified\n> > threshold. This poses a problem since we will never be able to generate\n> > a cruft pack that crosses the threshold.\n>\n> So far I see absolutely *NO* problem described in the above. The\n> user said \"I want to chop them into 200MB pieces but do not exceed\n> the threshold\" and the system honored that wish.\n>\n> > In effect, this means that we\n> > will try and repack its contents over and over again.\n>\n> The end effect however may be problematic, but isn't it due to the\n> way when to repack is determined? You see 199MB piece of cruft pack\n> plus some other cruft data. You have generated no new cruft and no\n> existing cruft expired out, but you do not know these facts until\n> you try to repack. Because 200MB is the limit, you include the\n> 199MB one as part of the ones to be recombined into the new cruft\n> pack because 199MB is smaller than 200MB and you do not know that\n> the reason why it is 199MB is because the earlier repack operation\n> found all remaining cruft material to be larger than 1MB; if there\n> were a 0.5MB cruft, it may have made it closer to 200MB.\n>\n> So would it be feasible to remember how 199MB cruft pack is lying in\n> the object store (i.e. earlier we packed as much as possible), and\n> add a logic that says \"if there is nothing to expire out of this\n> one, do not attempt to repack---this is fine as-is\"?\n\nI had a similar thought when first thinking about multi-cruft packs, but\nthe line of thinking is somewhat flawed. When we do a pruning GC, the\nvast majority of objects should be expired out of the repository,\nleaving only the recent ones that have mtime newer than the cutoff. So\nthe majority of packs in this case should all be removed, and the small\namount of cruft data remaining can be repacked into a small number of\npacks relatively quickly.\n\n> > Instead, change the meaning of '--max-pack-size' in pack-objects when\n> > combined with '--cruft'. When put together, '--max-pack-size' allows the\n> > pack to grow larger than the specified threshold, but only by one\n> > additional object.\n>\n> I do not think that would work well. You have no control over the\n> size of that one additional object---it may weigh more than 100MB,\n> combining your 199MB cruft pack with something else to make it ~300MB\n> cruft. In other words, \"just a little bit larger\" sounds like a\n> wishful thinking handwaving.\n\nI think that it is somewhat of a handwave, but I would note that our\ncurrent rules around --max-pack-size are not quite as strict as I\noriginally thought. If you have a single object that is 100MB and your\npack limit is 50MB, then pack-objects will generate a 100MB pack today\ncontaining just that object. So I don't think that our --max-pack-size\nrules are quite that strict.\n\nHere is the case that I am worried about:\n\nSuppose you have a 100MB cruft limit, and there are two cruft packs in\nthe repository: one that is 99MB and another that is 1MB in size. Let's\nsuppose further that if you combine these two packs, the resulting pack\nwould be exactly 100MB in size.\n\nToday, repack will say, \"I have two packs that sum together to be the\nvalue of --max-cruft-size\", and mark them both to be removed (and\nreplaced with the combined pack generated by pack-objects). But if the\ncombined pack is exactly 100MB, then pack-objects will break the pack\ninto two just before the 100MB limit, and we'll end up with the same two\npacks we started with.\n\nIdeally we would combine those packs into one that is at most one\nobject's size larger than the threshold, and the steady state would be\nto avoid repacking it further. But in current Git we will keep repacking\nthe two together, only to generate the same two packs we started with\nforever.\n\nSo I think a reasonable stop-gap here is to let pack-objects generate\ncruft packs with a --max-pack-size that are allowed to grow *just*\nbeyond the threshold by at most one object. Yes, that object can be\nlarge, and so it's possible that you could end up with a pack that is\nsignificantly larger in size than the threshold, if the one-extra-object\nis itself large.\n\nBut the point of --max-pack-size in conjunction with --cruft is not in\nthe original spirit of --max-pack-size, which was to work around\nfilesystems that don't do well with large files. Instead, the utility\nhere is to bound the amount of repacking we have to do when generating\ncruft packs in repositories that have many unreachable objects.\n\nIn other words, if my --max-cruft-size is 1G, and I have 20GB of cruft\ndata, I am less concerned about generating a pack that is 1.1G in size\nthan I am about repeatedly repacking the same 20GB over and over again\neach time I want to add a single unreachable object.\n\nThanks,\nTaylor\n"},{"id":"514062","messageId":"Z9GpQqm4YBvWF7Ff@nand.local","threadId":"63018","inReplyTo":"xmqqr0332un3.fsf@gitster.g","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T15:33:22Z","receivedAt":"2025-03-12T15:33:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 11, 2025 at 01:13:20PM -0700, Junio C Hamano wrote:\n> > This series resurrects the first patch from v1 after introducing a\n> > behavior change for 'git pack-objects --cruft --max-pack-size'. When\n> > given with '--cruft', '--max-pack-size' now allows pack-objects to grow\n> > a pack *just* past the given limit by at most one object.\n>\n> And what happens when the last object appended is very large, like\n> 70? Would we end up with 270 when the threshold says 200?\n>\n> I still am not getting what you are trying to explain in the above\n> two paragraphs, but in general, \"give up just before\" would be a\n> better choice than \"give up just after\", exactly because the threshold\n> we are letting the user to give is the maximum.\n\nI think this is similar to the discussion earlier in the thread, but let\nme know if there is something here I'm missing.\n\nThanks,\nTaylor\n"},{"id":"514092","messageId":"xmqqjz8uxfyq.fsf@gitster.g","threadId":"63018","inReplyTo":"Z9Gmo2P3Fnt3JeOs@nand.local","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-12T18:26:53Z","receivedAt":"2025-03-12T18:26:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> So would it be feasible to remember how 199MB cruft pack is lying in\n>> the object store (i.e. earlier we packed as much as possible), and\n>> add a logic that says \"if there is nothing to expire out of this\n>> one, do not attempt to repack---this is fine as-is\"?\n> .... So\n> the majority of packs in this case should all be removed, and the small\n> amount of cruft data remaining can be repacked into a small number of\n> packs relatively quickly.\n\nGiven the above ...\n\n> Suppose you have a 100MB cruft limit, and there are two cruft packs in\n> the repository: one that is 99MB and another that is 1MB in size. Let's\n> suppose further that if you combine these two packs, the resulting pack\n> would be exactly 100MB in size.\n>\n> Today, repack will say, \"I have two packs that sum together to be the\n> value of --max-cruft-size\", and mark them both to be removed (and\n> replaced with the combined pack generated by pack-objects).\n\n... yes, this logic to reach the above decision is exactly what I\nsaid is broken. Is there no way to fix that?\n\n> But if the\n> combined pack is exactly 100MB, then pack-objects will break the pack\n> into two just before the 100MB limit, and we'll end up with the same two\n> packs we started with.\n\nIf \"the majority of packs should all be removed and remainder combined\"\nyou stated earlier is true, then this case falls in a tiny minority\nthat we do not have to worry about, doesn't it?\n\n> Ideally we would combine those packs into one that is at most one\n> object's size larger than the threshold, and the steady state would be\n> to avoid repacking it further.\n\nI do not see why you can call that \"Ideally\". Ideally, we would\ncombine those packs to create a pack (or two) without busting the\nthreshold, *and* avoid needress repacking. Busting the given limit\nshould not be part of the definition of \"ideal\" solution.\n\n> But in current Git we will keep repacking\n> the two together, only to generate the same two packs we started with\n> forever.\n\nYes. That is because the logic that decides these packs need to be\nbroken and recombined is flawed. Maybe it does not have sufficient\ninformation to decide that it is no use to attempt combining them,\nin which case leaving some more info to help the later invocation of\nrepack to tell that it would be useless to attempt combining these\npacks when you do the initial repack would help, which was what I\nsuggested. You've thought about the issue much longer than I did,\nand would be able to come up with better ideas.\n"},{"id":"514093","messageId":"xmqqfrjixfwe.fsf@gitster.g","threadId":"63018","inReplyTo":"Z9GpQqm4YBvWF7Ff@nand.local","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-12T18:28:17Z","receivedAt":"2025-03-12T18:28:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Tue, Mar 11, 2025 at 01:13:20PM -0700, Junio C Hamano wrote:\n>> > This series resurrects the first patch from v1 after introducing a\n>> > behavior change for 'git pack-objects --cruft --max-pack-size'. When\n>> > given with '--cruft', '--max-pack-size' now allows pack-objects to grow\n>> > a pack *just* past the given limit by at most one object.\n>>\n>> And what happens when the last object appended is very large, like\n>> 70? Would we end up with 270 when the threshold says 200?\n>>\n>> I still am not getting what you are trying to explain in the above\n>> two paragraphs, but in general, \"give up just before\" would be a\n>> better choice than \"give up just after\", exactly because the threshold\n>> we are letting the user to give is the maximum.\n>\n> I think this is similar to the discussion earlier in the thread, but let\n> me know if there is something here I'm missing.\n\nI think the only thing you are missing is that max specified is the\nceiling, and \"you can bust it, hoping by a little but you do not\nknow how huge the error is\" is unacceptable.\n\nThanks.\n\n"},{"id":"514094","messageId":"Z9HaYEyYgBYTiia3@nand.local","threadId":"63018","inReplyTo":"xmqqjz8uxfyq.fsf@gitster.g","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T19:02:56Z","receivedAt":"2025-03-12T19:02:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 12, 2025 at 11:26:53AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> >> So would it be feasible to remember how 199MB cruft pack is lying in\n> >> the object store (i.e. earlier we packed as much as possible), and\n> >> add a logic that says \"if there is nothing to expire out of this\n> >> one, do not attempt to repack---this is fine as-is\"?\n> > .... So\n> > the majority of packs in this case should all be removed, and the small\n> > amount of cruft data remaining can be repacked into a small number of\n> > packs relatively quickly.\n>\n> Given the above ...\n>\n> > Suppose you have a 100MB cruft limit, and there are two cruft packs in\n> > the repository: one that is 99MB and another that is 1MB in size. Let's\n> > suppose further that if you combine these two packs, the resulting pack\n> > would be exactly 100MB in size.\n> >\n> > Today, repack will say, \"I have two packs that sum together to be the\n> > value of --max-cruft-size\", and mark them both to be removed (and\n> > replaced with the combined pack generated by pack-objects).\n>\n> ... yes, this logic to reach the above decision is exactly what I\n> said is broken. Is there no way to fix that?\n>\n> > But if the\n> > combined pack is exactly 100MB, then pack-objects will break the pack\n> > into two just before the 100MB limit, and we'll end up with the same two\n> > packs we started with.\n>\n> If \"the majority of packs should all be removed and remainder combined\"\n> you stated earlier is true, then this case falls in a tiny minority\n> that we do not have to worry about, doesn't it?\n\nYeah, it is a niche case. But the more I think about it the more I see\nit your way. I apologize for all of the back-and-forth here, this is\nquite a tricky problem to think through (at least for me), so I\nappreciate your patience.\n\nThe original implementation in repack was designed to aggregate smaller\ncruft packs together first until the combined size exceeds the\nthreshold. So the above would all be true if no new unreachable objects\nwere ever added to the repository, but if another 1MB cruft pack\nappears, then we would:\n\n - See the first 1MB pack, and decide we can repack it as it's under\n the 100MB threshold.\n\n - See the second 1MB pack, and repack it for the similar reasons (this\n time because 1+1<100, not 1<100).\n\n - See the 100MB pack, and refuse to repack it because the combined\n size of 102MB would be over the threshold.\n\nSo I think it's reasonable that if we keep the current behavior of\nrepacking the smaller ones first that this case is niche enough for me\nto feel OK not worrying about it too much.\n\nAnd, yes, breaking --max-pack-size when given with --cruft is ugly.\n\n> > But in current Git we will keep repacking\n> > the two together, only to generate the same two packs we started with\n> > forever.\n>\n> Yes. That is because the logic that decides these packs need to be\n> broken and recombined is flawed. Maybe it does not have sufficient\n> information to decide that it is no use to attempt combining them,\n> in which case leaving some more info to help the later invocation of\n> repack to tell that it would be useless to attempt combining these\n> packs when you do the initial repack would help, which was what I\n> suggested. You've thought about the issue much longer than I did,\n> and would be able to come up with better ideas.\n\nI think in the short term I came up with a worse idea than you would\nhave ;-).\n\nProbably there is a way to improve this niche case as described above,\nbut I think the solution space is probably complicated enough that given\nhow narrow of a case it is that it's not worth introducing that much\ncomplexity.\n\nSo I think at this point we should consider v3 to be the \"good\" version,\nand discard this v4. I can re-send v3 as v5 if you'd like, or I can\navoid it if it would be redundant. Either is fine, just LMK.\n\nThanks,\nTaylor\n"},{"id":"514095","messageId":"Z9Ha2mFXpojI+aIR@nand.local","threadId":"63018","inReplyTo":"xmqqfrjixfwe.fsf@gitster.g","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T19:04:58Z","receivedAt":"2025-03-12T19:05:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 12, 2025 at 11:28:17AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > On Tue, Mar 11, 2025 at 01:13:20PM -0700, Junio C Hamano wrote:\n> >> > This series resurrects the first patch from v1 after introducing a\n> >> > behavior change for 'git pack-objects --cruft --max-pack-size'. When\n> >> > given with '--cruft', '--max-pack-size' now allows pack-objects to grow\n> >> > a pack *just* past the given limit by at most one object.\n> >>\n> >> And what happens when the last object appended is very large, like\n> >> 70? Would we end up with 270 when the threshold says 200?\n> >>\n> >> I still am not getting what you are trying to explain in the above\n> >> two paragraphs, but in general, \"give up just before\" would be a\n> >> better choice than \"give up just after\", exactly because the threshold\n> >> we are letting the user to give is the maximum.\n> >\n> > I think this is similar to the discussion earlier in the thread, but let\n> > me know if there is something here I'm missing.\n>\n> I think the only thing you are missing is that max specified is the\n> ceiling, and \"you can bust it, hoping by a little but you do not\n> know how huge the error is\" is unacceptable.\n\nI agree that in the general case it is unacceptable. I think I might see\nit slightly different than you, since for cruft packs the idea is to\nbound the working set of what you're repacking using the size of the\nresulting packs as a proxy for the former.\n\nMaybe we should call the option something else that makes the cruft pack\nuse-case clearer. But in the other thread I came around to the idea that\nthis case is too niche to address completely, so I think we can discard\nthis round as a terrible idea.\n\nThanks,\nTaylor\n"},{"id":"514096","messageId":"CABPp-BH0rbieCV4Z11pHOX-mwrtEO-FPNdywV0P5HxXnusdRKQ@mail.gmail.com","threadId":"63018","inReplyTo":"Z9HaYEyYgBYTiia3@nand.local","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-12T19:13:10Z","receivedAt":"2025-03-12T19:13:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 12, 2025 at 12:02 PM Taylor Blau wrote:\n>\n> On Wed, Mar 12, 2025 at 11:26:53AM -0700, Junio C Hamano wrote:\n> > Taylor Blau writes:\n> >\n> > >> So would it be feasible to remember how 199MB cruft pack is lying in\n> > >> the object store (i.e. earlier we packed as much as possible), and\n> > >> add a logic that says \"if there is nothing to expire out of this\n> > >> one, do not attempt to repack---this is fine as-is\"?\n> > > .... So\n> > > the majority of packs in this case should all be removed, and the small\n> > > amount of cruft data remaining can be repacked into a small number of\n> > > packs relatively quickly.\n> >\n> > Given the above ...\n> >\n> > > Suppose you have a 100MB cruft limit, and there are two cruft packs in\n> > > the repository: one that is 99MB and another that is 1MB in size. Let's\n> > > suppose further that if you combine these two packs, the resulting pack\n> > > would be exactly 100MB in size.\n> > >\n> > > Today, repack will say, \"I have two packs that sum together to be the\n> > > value of --max-cruft-size\", and mark them both to be removed (and\n> > > replaced with the combined pack generated by pack-objects).\n> >\n> > ... yes, this logic to reach the above decision is exactly what I\n> > said is broken. Is there no way to fix that?\n> >\n> > > But if the\n> > > combined pack is exactly 100MB, then pack-objects will break the pack\n> > > into two just before the 100MB limit, and we'll end up with the same two\n> > > packs we started with.\n> >\n> > If \"the majority of packs should all be removed and remainder combined\"\n> > you stated earlier is true, then this case falls in a tiny minority\n> > that we do not have to worry about, doesn't it?\n>\n> Yeah, it is a niche case. But the more I think about it the more I see\n> it your way. I apologize for all of the back-and-forth here, this is\n> quite a tricky problem to think through (at least for me), so I\n> appreciate your patience.\n>\n> The original implementation in repack was designed to aggregate smaller\n> cruft packs together first until the combined size exceeds the\n> threshold. So the above would all be true if no new unreachable objects\n> were ever added to the repository, but if another 1MB cruft pack\n> appears, then we would:\n>\n> - See the first 1MB pack, and decide we can repack it as it's under\n> the 100MB threshold.\n>\n> - See the second 1MB pack, and repack it for the similar reasons (this\n> time because 1+1<100, not 1<100).\n>\n> - See the 100MB pack, and refuse to repack it because the combined\n> size of 102MB would be over the threshold.\n>\n> So I think it's reasonable that if we keep the current behavior of\n> repacking the smaller ones first that this case is niche enough for me\n> to feel OK not worrying about it too much.\n>\n> And, yes, breaking --max-pack-size when given with --cruft is ugly.\n>\n> > > But in current Git we will keep repacking\n> > > the two together, only to generate the same two packs we started with\n> > > forever.\n> >\n> > Yes. That is because the logic that decides these packs need to be\n> > broken and recombined is flawed. Maybe it does not have sufficient\n> > information to decide that it is no use to attempt combining them,\n> > in which case leaving some more info to help the later invocation of\n> > repack to tell that it would be useless to attempt combining these\n> > packs when you do the initial repack would help, which was what I\n> > suggested. You've thought about the issue much longer than I did,\n> > and would be able to come up with better ideas.\n>\n> I think in the short term I came up with a worse idea than you would\n> have ;-).\n>\n> Probably there is a way to improve this niche case as described above,\n> but I think the solution space is probably complicated enough that given\n> how narrow of a case it is that it's not worth introducing that much\n> complexity.\n\nWould it make sense to break the assumption that --max-cruft-size ==\n--max-pack-size and perhaps rename the former? I think the problem is\nthat the two imply different things (one is a minimum, the other a\nmaximum), and thus really should be different values. E.g.\n--combine-cruft-below-size that is set to e.g. half of\n--max-pack-size, and then you can continue combining cruft packs\ntogether until they do go above the cruft threshold, while avoiding\nactually exceeding the pack size threshold?\n"},{"id":"514097","messageId":"Z9HhnpYYbe/FNUWT@nand.local","threadId":"63018","inReplyTo":"CABPp-BH0rbieCV4Z11pHOX-mwrtEO-FPNdywV0P5HxXnusdRKQ@mail.gmail.com","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T19:33:50Z","receivedAt":"2025-03-12T19:33:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 12, 2025 at 12:13:10PM -0700, Elijah Newren wrote:\n> > > > But in current Git we will keep repacking\n> > > > the two together, only to generate the same two packs we started with\n> > > > forever.\n> > >\n> > > Yes. That is because the logic that decides these packs need to be\n> > > broken and recombined is flawed. Maybe it does not have sufficient\n> > > information to decide that it is no use to attempt combining them,\n> > > in which case leaving some more info to help the later invocation of\n> > > repack to tell that it would be useless to attempt combining these\n> > > packs when you do the initial repack would help, which was what I\n> > > suggested. You've thought about the issue much longer than I did,\n> > > and would be able to come up with better ideas.\n> >\n> > I think in the short term I came up with a worse idea than you would\n> > have ;-).\n> >\n> > Probably there is a way to improve this niche case as described above,\n> > but I think the solution space is probably complicated enough that given\n> > how narrow of a case it is that it's not worth introducing that much\n> > complexity.\n>\n> Would it make sense to break the assumption that --max-cruft-size ==\n> --max-pack-size and perhaps rename the former? I think the problem is\n> that the two imply different things (one is a minimum, the other a\n> maximum), and thus really should be different values. E.g.\n> --combine-cruft-below-size that is set to e.g. half of\n> --max-pack-size, and then you can continue combining cruft packs\n> together until they do go above the cruft threshold, while avoiding\n> actually exceeding the pack size threshold?\n\nWe could, though alternatively I think leaving the behavior as is\npresented in v3 is equally OK.\n\nWe'll never make a pack that is as large or larger than the given\n--max-cruft-size, but because repack tries to aggregate smaller packs\ntogether first, it's unlikely that we would ever repeatedly repack the\nlarger ones making them effectively \"frozen\", which is the goal.\n\nThanks,\nTaylor\n"},{"id":"514098","messageId":"xmqqa59qxc9b.fsf@gitster.g","threadId":"63018","inReplyTo":"Z9Ha2mFXpojI+aIR@nand.local","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-12T19:46:56Z","receivedAt":"2025-03-12T19:46:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Maybe we should call the option something else that makes the cruft pack\n> use-case clearer. But in the other thread I came around to the idea that\n> this case is too niche to address completely, so I think we can discard\n> this round as a terrible idea.\n\nYeah, I do not think \"an earlier attempt wouldn't fill the bucket\nfully, so a later logic may try to fill the bucket with the same\nmaterial in vain, failing the same way\" is a problem that is limited\nto cruft pack use case. If you are repacking real data into 200MB\nchunks every week, the 198MB pack created last week that is lying in\nthe object store that houses the oldest part of the history is\nunlikely to be improved by repacking it (since the attempt failed to\nfill the remaining 2MB last week, and the oldest part of the history\nhasn't changed) but it is entirely possible that with the pieces of\ninformation left by the current set of tools, the machinery may not\nrealize that fact.\n\nAllowing the resulting pack to deliberately exceed 200MB ceiling is\none possible way (I still do not think it is the best way among\npossible ways) to mark the pack \"frozen-do not further touch\".\n\nNow, if the essense of the problem is when to mark a pack that we\nstopped appending to due to max-size limit as \"frozen and do not\ntouch\" to avoid this problem, how such a marking is done, and who\nuses it to avoid repeated work that is known (to us) to be futile,\nthere should be better ways to do such a marking. We have .keep\nmechanism which is ugly but proven to work and we know how to deal\nwith, for example.\n\nI wonder if our pack header or trailer is extensible enough so that\nwe can record \"this pack was closed due to max-size limit of 200MB\nand appending even one more object in the queue would have busted\nthe limit\".\n\nNo matter how such a marking is done, later repack that sees a pack\nmarked as such, when it is limited to 200MB, should be able to tell\nthat and act a bit more intelligently than what it currently does.\n\n\n"},{"id":"514099","messageId":"Z9Hl+UpnEq07qFeW@nand.local","threadId":"63018","inReplyTo":"xmqqa59qxc9b.fsf@gitster.g","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T19:52:25Z","receivedAt":"2025-03-12T19:52:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 12, 2025 at 12:46:56PM -0700, Junio C Hamano wrote:\n> Allowing the resulting pack to deliberately exceed 200MB ceiling is\n> one possible way (I still do not think it is the best way among\n> possible ways) to mark the pack \"frozen-do not further touch\".\n>\n> Now, if the essense of the problem is when to mark a pack that we\n> stopped appending to due to max-size limit as \"frozen and do not\n> touch\" to avoid this problem, how such a marking is done, and who\n> uses it to avoid repeated work that is known (to us) to be futile,\n> there should be better ways to do such a marking. We have .keep\n> mechanism which is ugly but proven to work and we know how to deal\n> with, for example.\n\nYeah... I think a .keep-like mechanism would be sufficient here to mark\npacks which are close to the --max-cruft-size as frozen if we truly\nwanted to avoid this problem.\n\nBut if we have, say, packs that are 198MB, 1MB, 2MB, and 3MB in size,\nthat we're going to accumulate in order of increasing size, so we would\nalmost never repack the 198MB pack.\n\nThe .keep mechanism by itself wouldn't quite do the trick, though,\nbecause we'd have to distinguish between \"marked as .kept by the user\nand so we absolutely must not delete it\" and \"marked as frozen by us,\nbut we could delete it if we needed to (e.g., when pruning)\".\n\nSo that would maybe suggest introducing a .frozen mechanism, but that is\nbolting further ugliness onto the .keep mechanism, which I think is the\nwrong direction to be going in.\n\n> I wonder if our pack header or trailer is extensible enough so that\n> we can record \"this pack was closed due to max-size limit of 200MB\n> and appending even one more object in the queue would have busted\n> the limit\".\n>\n> No matter how such a marking is done, later repack that sees a pack\n> marked as such, when it is limited to 200MB, should be able to tell\n> that and act a bit more intelligently than what it currently does.\n\nPerhaps in the longer term, but I think for the reasons above that the\nexisting behavior (plus the new patch from v3, which we should still\nqueue) is sufficient.\n\nThanks,\nTaylor\n"},{"id":"514107","messageId":"xmqq5xkex9md.fsf@gitster.g","threadId":"63018","inReplyTo":"CABPp-BH0rbieCV4Z11pHOX-mwrtEO-FPNdywV0P5HxXnusdRKQ@mail.gmail.com","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-12T20:43:54Z","receivedAt":"2025-03-12T20:43:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Would it make sense to break the assumption that --max-cruft-size ==\n> --max-pack-size and perhaps rename the former? I think the problem is\n> that the two imply different things (one is a minimum, the other a\n> maximum), and thus really should be different values. E.g.\n> --combine-cruft-below-size that is set to e.g. half of\n> --max-pack-size, and then you can continue combining cruft packs\n> together until they do go above the cruft threshold, while avoiding\n> actually exceeding the pack size threshold?\n\nWith below-size and max-size set to say 180 and 200 respectively, an\nattempt to combine the crufts may end up filling a cruft pack to 170\nbut the smallest of the remaining cruft may weigh 40, which means\nincluding it would cause the max-size to be exceeded. In such a\nscenario, there may not be a solution to satisfy given constraints,\ni.e. go above the below-size without stay below the max-size.\n\nSo I am not sure if the approach would really solve much.\n\nOther than that a separate names, especially losing \"max\" from the\nthreshold that really does not mean \"max\", would solve the confusion\nthat comes from naming, that is.\n\n"},{"id":"514110","messageId":"CABPp-BH35cn1nXSaF=U=dbSKUxTFL5uP+yUvhbXMi66tJMuhLQ@mail.gmail.com","threadId":"63018","inReplyTo":"xmqq5xkex9md.fsf@gitster.g","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-12T20:49:17Z","receivedAt":"2025-03-12T20:49:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 12, 2025 at 1:43 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > Would it make sense to break the assumption that --max-cruft-size ==\n> > --max-pack-size and perhaps rename the former? I think the problem is\n> > that the two imply different things (one is a minimum, the other a\n> > maximum), and thus really should be different values. E.g.\n> > --combine-cruft-below-size that is set to e.g. half of\n> > --max-pack-size, and then you can continue combining cruft packs\n> > together until they do go above the cruft threshold, while avoiding\n> > actually exceeding the pack size threshold?\n>\n> With below-size and max-size set to say 180 and 200 respectively, an\n> attempt to combine the crufts may end up filling a cruft pack to 170\n> but the smallest of the remaining cruft may weigh 40, which means\n> including it would cause the max-size to be exceeded. In such a\n> scenario, there may not be a solution to satisfy given constraints,\n> i.e. go above the below-size without stay below the max-size.\n>\n> So I am not sure if the approach would really solve much.\n>\n> Other than that a separate names, especially losing \"max\" from the\n> threshold that really does not mean \"max\", would solve the confusion\n> that comes from naming, that is.\n\n--max-pack-size is a constraint. --combine-cruft-below-size is not.\nThink particularly of the case where the user doesn't even have any\ncruft packs yet and has only accumulated a little bit of cruft. That\noption is merely a guide post to say that if it's smaller than that\nsize, then feel free to keep trying to add to it (so long as it\ndoesn't violate constraints such as --max-pack-size).\n"},{"id":"514163","messageId":"20250313062951.GA96035@coredump.intra.peff.net","threadId":"63018","inReplyTo":"Z9Ha2mFXpojI+aIR@nand.local","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-13T06:29:51Z","receivedAt":"2025-03-13T06:29:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 12, 2025 at 03:04:58PM -0400, Taylor Blau wrote:\n\n> > > I think this is similar to the discussion earlier in the thread, but let\n> > > me know if there is something here I'm missing.\n> >\n> > I think the only thing you are missing is that max specified is the\n> > ceiling, and \"you can bust it, hoping by a little but you do not\n> > know how huge the error is\" is unacceptable.\n> \n> I agree that in the general case it is unacceptable. I think I might see\n> it slightly different than you, since for cruft packs the idea is to\n> bound the working set of what you're repacking using the size of the\n> resulting packs as a proxy for the former.\n> \n> Maybe we should call the option something else that makes the cruft pack\n> use-case clearer. But in the other thread I came around to the idea that\n> this case is too niche to address completely, so I think we can discard\n> this round as a terrible idea.\n\nHaving read through the thread, I think the naming is really the issue.\nElijah's earlier response was the most clear to me: the new feature is\nnot a max size at all, but a --combine-cruft-below-size. Our goal is not\nto have small packs, but to avoid rewriting bytes over and over again\n(which would happen if we simply rolled all cruft packs into one on each\nrepack). But also to avoid having a zillion cruft packs (which would\nhappen if we never rolled them up).\n\nSo I think all of the discussion about \"would we bust the max limit\" is\nmostly a red herring.\n\nBut also, I think there is no need to tell pack-objects to limit the\nsize of the resulting cruft pack at all. Let's say you have three cruft\npacks with sizes 30MB, 20MB, and 10MB. I want to roll up so that each\ncruft pack is at least 40MB. We could say (and this is what I think\nyour series does, based on our earlier off-list discussions, but please\ncorrect me if things have changed):\n\n 1. All of those are candidates for rolling up, because they're below\n our threshold.\n\n 2. We'll feed the packs along with the \"max\" size to pack-objects,\n which will then roll it all up into a 40MB pack, plus a 20MB pack\n left over. We'll have written all of the bytes once, but on the\n next repack we'd only rewrite 20MB (plus whatever new cruft comes\n along).\n\nBut do we actually care about eventually having a series of 40MB packs?\nOr do we care about having some cutoff so that we don't rewrite those\nfirst 40MB on subsequent repacks?\n\nIf the latter, then for step 2, what if we don't feed a max size? We'd\nend up with one 60MB pack (again, having written all of the bytes once).\nAnd on the next repack we'd leave it be (since it's over the threshold).\nWe'll start forming new packs, which will eventually aggregate to 40MB\n(or possibly larger).\n\nIf I understand the main purpose of the series, it is that we must\nrescue objects out of cruft packs if they became fresher (by having\nloose copies made). But that is orthogonal to max pack sizes, isn't it?\nWe just need for pack-objects to be fed those objects (which should be\nhappening already) and decide _not_ to omit them based on their presence\nin the kept cruft packs (based on the mtime in those cruft packs, of\ncourse). Which looks like what your want_cruft_object_mtime() is doing.\n\n-Peff\n"},{"id":"514174","messageId":"xmqqmsdpruqg.fsf@gitster.g","threadId":"63018","inReplyTo":"CABPp-BH35cn1nXSaF=U=dbSKUxTFL5uP+yUvhbXMi66tJMuhLQ@mail.gmail.com","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T12:16:39Z","receivedAt":"2025-03-13T12:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> With below-size and max-size set to say 180 and 200 respectively, an\n>> attempt to combine the crufts may end up filling a cruft pack to 170\n>> but the smallest of the remaining cruft may weigh 40, which means\n>> including it would cause the max-size to be exceeded. In such a\n>> scenario, there may not be a solution to satisfy given constraints,\n>> i.e. go above the below-size without stay below the max-size.\n>>\n>> So I am not sure if the approach would really solve much.\n>>\n>> Other than that a separate names, especially losing \"max\" from the\n>> threshold that really does not mean \"max\", would solve the confusion\n>> that comes from naming, that is.\n>\n> --max-pack-size is a constraint. --combine-cruft-below-size is not.\n> Think particularly of the case where the user doesn't even have any\n> cruft packs yet and has only accumulated a little bit of cruft. That\n> option is merely a guide post to say that if it's smaller than that\n> size, then feel free to keep trying to add to it (so long as it\n> doesn't violate constraints such as --max-pack-size).\n\nThat is correct and it is why I said the suggestion solves the name\nconfusion. But think about the sample situation, before and after\nsuch a repack with two thresholds. You had below- and max-size set\nto 180 and 200 respectively, and a cruft pack of size 170, and you\nfailed to grow that cruft pack beyond 180 because the next available\ncruft weighed 40. Then you'll repeat the exercise again, find 170\nthat is smaller than the below- threshold, try to cram more and\nwould fail. Isn't that what Taylor's series wanted to prevent from\nhappening, and isn't the two-threshod approach supposed to be a way\nto improve on it?\n"},{"id":"514197","messageId":"xmqqr031q80i.fsf@gitster.g","threadId":"63018","inReplyTo":"20250313062951.GA96035@coredump.intra.peff.net","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T15:12:45Z","receivedAt":"2025-03-13T15:12:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But do we actually care about eventually having a series of 40MB packs?\n> Or do we care about having some cutoff so that we don't rewrite those\n> first 40MB on subsequent repacks?\n>\n> If the latter, then for step 2, what if we don't feed a max size? We'd\n> end up with one 60MB pack (again, having written all of the bytes once).\n> And on the next repack we'd leave it be (since it's over the threshold).\n> We'll start forming new packs, which will eventually aggregate to 40MB\n> (or possibly larger).\n>\n> If I understand the main purpose of the series, it is that we must\n> rescue objects out of cruft packs if they became fresher (by having\n> loose copies made). But that is orthogonal to max pack sizes, isn't it?\n> We just need for pack-objects to be fed those objects (which should be\n> happening already) and decide _not_ to omit them based on their presence\n> in the kept cruft packs (based on the mtime in those cruft packs, of\n> course). Which looks like what your want_cruft_object_mtime() is doing.\n\nYeah, no packsize limit on the output side, but making sure that the\ndecision to roll up existing cruft packs is made sensibly, is what\nthe above gives us, which I life a lot. The one-before and -after\nconfusion came exactly because we somehow tried to have threshold on\nthe output side.\n\n\n\n"},{"id":"514205","messageId":"CABPp-BFSXVkZb76va0CO_w3G+MtNuvC3jv6-9vo-oR3LHW4YNQ@mail.gmail.com","threadId":"63018","inReplyTo":"xmqqmsdpruqg.fsf@gitster.g","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-13T16:23:35Z","receivedAt":"2025-03-13T16:23:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 13, 2025 at 5:16 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> >> With below-size and max-size set to say 180 and 200 respectively, an\n> >> attempt to combine the crufts may end up filling a cruft pack to 170\n> >> but the smallest of the remaining cruft may weigh 40, which means\n> >> including it would cause the max-size to be exceeded. In such a\n> >> scenario, there may not be a solution to satisfy given constraints,\n> >> i.e. go above the below-size without stay below the max-size.\n> >>\n> >> So I am not sure if the approach would really solve much.\n> >>\n> >> Other than that a separate names, especially losing \"max\" from the\n> >> threshold that really does not mean \"max\", would solve the confusion\n> >> that comes from naming, that is.\n> >\n> > --max-pack-size is a constraint. --combine-cruft-below-size is not.\n> > Think particularly of the case where the user doesn't even have any\n> > cruft packs yet and has only accumulated a little bit of cruft. That\n> > option is merely a guide post to say that if it's smaller than that\n> > size, then feel free to keep trying to add to it (so long as it\n> > doesn't violate constraints such as --max-pack-size).\n>\n> That is correct and it is why I said the suggestion solves the name\n> confusion. But think about the sample situation, before and after\n> such a repack with two thresholds. You had below- and max-size set\n> to 180 and 200 respectively, and a cruft pack of size 170, and you\n> failed to grow that cruft pack beyond 180 because the next available\n> cruft weighed 40. Then you'll repeat the exercise again, find 170\n> that is smaller than the below- threshold, try to cram more and\n> would fail. Isn't that what Taylor's series wanted to prevent from\n> happening, and isn't the two-threshod approach supposed to be a way\n> to improve on it?\n\nI don't think \"always combine\" is necessary for improvement. Perhaps,\nin your example, this round of repacking can't combine things. But\nthe next time we want to repack cruft objects and there is anything\nnew that (individually or collectively) weighs between 10 and 30, we\ncan add it and get something over the lower threshold and then ignore\nthe resulting cruft pack it in the future.\n\nIn contrast, the single threshold either has to violate the maximum\nconstraint, or always reconsider everything. The two threshold system\nallows progress to be made (so long as it doesn't just look at the\nfirst biggest object and fail every time), but particularly if you set\nthe thresholds too close to each other or you just have really large\ncruft, then you _sometimes_ might not make progress.\n\nPersonally, I think I'd set the --combine-cruft-below-size to half of\n--max-pack-size, because that guarantees that any two existing cruft\npacks being considered for combining can be, and the resulting\ncombined cruft pack if big enough can then be ignored in the future.\nIn other words, this scheme would allow you to always make progress.\n\nAm I missing something?\n"},{"id":"514210","messageId":"xmqq7c4sq2qn.fsf@gitster.g","threadId":"63018","inReplyTo":"CABPp-BFSXVkZb76va0CO_w3G+MtNuvC3jv6-9vo-oR3LHW4YNQ@mail.gmail.com","subject":"Re: [PATCH v4 4/6] pack-objects: generate cruft packs at most one object over threshold","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T17:06:40Z","receivedAt":"2025-03-13T17:06:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Personally, I think I'd set the --combine-cruft-below-size to half of\n> --max-pack-size, because that guarantees that any two existing cruft\n> packs being considered for combining can be, and the resulting\n> combined cruft pack if big enough can then be ignored in the future.\n\nYup, combine-below can be set to arbitrarily a small value, and ...\n\n> In other words, this scheme would allow you to always make progress.\n\n... I would even think that setting it to too high a value may be a\nmisconfiguration. As you outlined above, a half of the max-size\nwould probably be the practical maximum for the combine-below value\nto avoid the issue and to always make progress.\n\n"},{"id":"514212","messageId":"xmqq1pv0q27u.fsf@gitster.g","threadId":"63018","inReplyTo":"Z9Hl+UpnEq07qFeW@nand.local","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T17:17:57Z","receivedAt":"2025-03-13T17:18:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Perhaps in the longer term, but I think for the reasons above that the\n> existing behavior (plus the new patch from v3, which we should still\n> queue) is sufficient.\n\nThe older iteration has a few loose ends \nwe would want to tie. Elijah's \"decision to combine should be made\nat half or below the max size to always make a progress\" would also\nmakes sense.\n\nThanks.\n\n"},{"id":"514215","messageId":"Z9MWp6+TqwD+LdxE@nand.local","threadId":"63018","inReplyTo":"Z8l5hxNjEOALl_g-@pks.im","subject":"Re: [PATCH v3 1/1] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-13T17:32:23Z","receivedAt":"2025-03-13T17:32:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 06, 2025 at 11:31:35AM +0100, Patrick Steinhardt wrote:\n> Minor nit: it is a bit unusual that a negative value, which typically\n> indicates an error, is used as a boolean value here to indicate that we\n> don't want to have the object.\n\nThis matches the convention of the other \"do we want this object?\"\nfunctions, where returning -1 indicates that we don't yet know whether\nor not we want the object, and should continue looking elsewhere.\n\nReturning -1 from 'want_cruft_object_mtime()' doesn't mean that we can\nnecessarily return -1 immediately from its caller in\n'want_found_object()' because we might pick it up from further down in\nthat function or in one of its callers.\n\nSo I think the return values of that function are consistent. Because\nthat function never says \"yes, I want this object\" and only \"no\" or\n\"maybe\", we could return 0/1 indicating \"no\" or \"maybe\". But that feels\nlike a bug waiting to happen if someone later on mistakes \"1\" (which in\nthis hypothetical would mean \"maybe\") as \"yes\".\n\n> > diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\n> > index 959e6e26488..f427150de5b 100755\n> > --- a/t/t7704-repack-cruft.sh\n> > +++ b/t/t7704-repack-cruft.sh\n> > @@ -304,6 +304,69 @@ test_expect_success '--max-cruft-size with freshened objects (packed)' '\n> > \t)\n> > '\n> >\n> > +test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n> > +\tgit init max-cruft-size-threshold &&\n>\n> Let's also delete the repository via `test_when_finished`.\n\nEh. The point of naming these uniquely is that we don't have to remember\nto clean them up afterwords, but I can do so if you want.\n\nThanks,\nTaylor\n"},{"id":"514216","messageId":"Z9MXWeXd5G1d4kY5@nand.local","threadId":"63018","inReplyTo":"xmqq1pv0q27u.fsf@gitster.g","subject":"Re: [PATCH v4 0/6] pack-objects: freshen objects with multi-cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-13T17:35:21Z","receivedAt":"2025-03-13T17:35:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 13, 2025 at 10:17:57AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Perhaps in the longer term, but I think for the reasons above that the\n> > existing behavior (plus the new patch from v3, which we should still\n> > queue) is sufficient.\n>\n> The older iteration has a few loose ends \n> we would want to tie. Elijah's \"decision to combine should be made\n> at half or below the max size to always make a progress\" would also\n> makes sense.\n\nLet's split these two out. This series (or single patch, as it will\nshortly become in v5) is fixing a legitimate bug in freshening\nunreachable objects when they appear in multiple cruft packs.\n\nThat will allow us a little more time to think through the more\ncomplicated issues while fixing the more straightforward ones quickly.\n\nThanks,\nTaylor\n"},{"id":"514221","messageId":"1563552bbda0bc910c9f41b0fabc3225c4d778fc.1741889018.git.me@ttaylorr.com","threadId":"63018","inReplyTo":"cover.1740680964.git.me@ttaylorr.com","subject":"[PATCH v5] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-13T18:09:47Z","receivedAt":"2025-03-13T18:09:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Once an object is written into a cruft pack, we can only freshen it by\nwriting a new loose or packed copy of that object with a more recent\nmtime.\n\nPrior to 61568efa95 (builtin/pack-objects.c: support `--max-pack-size`\nwith `--cruft`, 2023-08-28), we typically had at most one cruft pack in\na repository at any given time. So freshening unreachable objects was\nstraightforward when already rewriting the cruft pack (and its *.mtimes\nfile).\n\nBut 61568efa95 changes things: 'pack-objects' now supports writing\nmultiple cruft packs when invoked with `--cruft` and the\n`--max-pack-size` flag. Cruft packs are rewritten until they reach some\nsize threshold, at which point they are considered \"frozen\", and will\nonly be modified in a pruning GC, or if the threshold itself is\nadjusted.\n\nPrior to this patch, however, this process breaks down when we attempt\nto freshen an object packed in an earlier cruft pack, and that cruft\npack is larger than the threshold and thus will survive the repack.\n\nWhen this is the case, it is impossible to freshen objects in cruft\npack(s) when those cruft packs are larger than the threshold. This is\nbecause we would avoid writing them in the new cruft pack entirely, for\na couple of reasons.\n\n 1. When enumerating packed objects via 'add_objects_in_unpacked_packs()'\n we pass the SKIP_IN_CORE_KEPT_PACKS, which is used to avoid looping\n over the packs we're going to retain (which are marked as kept\n in-core by 'read_cruft_objects()').\n\n This means that we will avoid enumerating additional packed copies\n of objects found in any cruft packs which are larger than the given\n size threshold. Thus there is no opportunity to call\n 'create_object_entry()' whatsoever.\n\n 2. We likewise will discard the loose copy (if one exists) of any\n unreachable object packed in a cruft pack that is larger than the\n threshold. Here our call path is 'add_unreachable_loose_objects()',\n which uses the 'add_loose_object()' callback.\n\n That function will eventually land us in 'want_object_in_pack()'\n (via 'add_cruft_object_entry()'), and we'll discard the object as it\n appears in one of the packs which we marked as kept in-core.\n\nThis means in effect that it is impossible to freshen an unreachable\nobject once it appears in a cruft pack larger than the given threshold.\n\nInstead, we should pack an additional copy of an unreachable object we\nwant to freshen even if it appears in a cruft pack, provided that the\ncruft copy has an mtime which is before the mtime of the copy we are\ntrying to pack/freshen. This is sub-optimal in the sense that it\nrequires keeping an additional copy of unreachable objects upon\nfreshening, but we don't have a better alternative without the ability\nto make in-place modifications to existing *.mtimes files.\n\nIn order to implement this, we have to adjust the behavior of\n'want_found_object()'. When 'pack-objects' is told that we're *not*\ngoing to retain any cruft packs (i.e. the set of packs marked as kept\nin-core does not contain a cruft pack), the behavior is unchanged.\n\nBut when there *is* at least one cruft pack that we're holding onto, it\nis no longer sufficient to reject a copy of an object found in that\ncruft pack for that reason alone. In this case, we only want to reject a\ncandidate object when copies of that object either:\n\n - exists in a non-cruft pack that we are retaining, regardless of that\n pack's mtime, or\n\n - exists in a cruft pack with an mtime at least as recent as the copy\n we are debating whether or not to pack, in which case freshening\n would be redundant.\n\nTo do this, keep track of whether or not we have any cruft packs in our\nin-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\nflag. When we end up in this new special case, we replace a call to\n'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only reject\nobjects when we have a copy in an existing cruft pack with at least as\nrecent an mtime as our candidate (in which case \"freshening\" would be\nredundant).\n\nSigned-off-by: Taylor Blau \n---\n builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n packfile.c | 3 +-\n packfile.h | 2 +\n t/t7704-repack-cruft.sh | 66 ++++++++++++++++++++++\n 4 files changed, 171 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 58a9b16126..79e1e6fb52 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -206,6 +206,7 @@ static int have_non_local_packs;\n static int incremental;\n static int ignore_packed_keep_on_disk;\n static int ignore_packed_keep_in_core;\n+static int ignore_packed_keep_in_core_has_cruft;\n static int allow_ofs_delta;\n static struct pack_idx_option pack_idx_opts;\n static const char *base_name;\n@@ -1502,8 +1503,60 @@ static int have_duplicate_entry(const struct object_id *oid,\n \treturn 1;\n }\n \n+static int want_cruft_object_mtime(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t unsigned flags, uint32_t mtime)\n+{\n+\tstruct packed_git **cache;\n+\n+\tfor (cache = kept_pack_cache(r, flags); *cache; cache++) {\n+\t\tstruct packed_git *p = *cache;\n+\t\toff_t ofs;\n+\t\tuint32_t candidate_mtime;\n+\n+\t\tofs = find_pack_entry_one(oid, p);\n+\t\tif (!ofs)\n+\t\t\tcontinue;\n+\n+\t\t/*\n+\t\t * We have a copy of the object 'oid' in a non-cruft\n+\t\t * pack. We can avoid packing an additional copy\n+\t\t * regardless of what the existing copy's mtime is since\n+\t\t * it is outside of a cruft pack.\n+\t\t */\n+\t\tif (!p->is_cruft)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * If we have a copy of the object 'oid' in a cruft\n+\t\t * pack, then either read the cruft pack's mtime for\n+\t\t * that object, or, if that can't be loaded, assume the\n+\t\t * pack's mtime itself.\n+\t\t */\n+\t\tif (!load_pack_mtimes(p)) {\n+\t\t\tuint32_t pos;\n+\t\t\tif (offset_to_pack_pos(p, ofs, &pos) < 0)\n+\t\t\t\tcontinue;\n+\t\t\tcandidate_mtime = nth_packed_mtime(p, pos);\n+\t\t} else {\n+\t\t\tcandidate_mtime = p->mtime;\n+\t\t}\n+\n+\t\t/*\n+\t\t * We have a surviving copy of the object in a cruft\n+\t\t * pack whose mtime is greater than or equal to the one\n+\t\t * we are considering. We can thus avoid packing an\n+\t\t * additional copy of that object.\n+\t\t */\n+\t\tif (mtime <= candidate_mtime)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n static int want_found_object(const struct object_id *oid, int exclude,\n-\t\t\t struct packed_git *p)\n+\t\t\t struct packed_git *p, uint32_t mtime)\n {\n \tif (exclude)\n \t\treturn 1;\n@@ -1553,12 +1606,29 @@ static int want_found_object(const struct object_id *oid, int exclude,\n \t\tif (ignore_packed_keep_in_core)\n \t\t\tflags |= IN_CORE_KEEP_PACKS;\n \n-\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n-\t\t\treturn 0;\n-\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n-\t\t\treturn 0;\n-\t\tif (has_object_kept_pack(p->repo, oid, flags))\n-\t\t\treturn 0;\n+\t\t/*\n+\t\t * If the object is in a pack that we want to ignore, *and* we\n+\t\t * don't have any cruft packs that are being retained, we can\n+\t\t * abort quickly.\n+\t\t */\n+\t\tif (!ignore_packed_keep_in_core_has_cruft) {\n+\t\t\tif (ignore_packed_keep_on_disk && p->pack_keep)\n+\t\t\t\treturn 0;\n+\t\t\tif (ignore_packed_keep_in_core && p->pack_keep_in_core)\n+\t\t\t\treturn 0;\n+\t\t\tif (has_object_kept_pack(p->repo, oid, flags))\n+\t\t\t\treturn 0;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * But if there is at least one cruft pack which\n+\t\t\t * is being kept, we only want to include the\n+\t\t\t * provided object if it has a strictly greater\n+\t\t\t * mtime than any existing cruft copy.\n+\t\t\t */\n+\t\t\tif (!want_cruft_object_mtime(p->repo, oid, flags,\n+\t\t\t\t\t\t mtime))\n+\t\t\t\treturn 0;\n+\t\t}\n \t}\n \n \t/*\n@@ -1577,7 +1647,8 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t int exclude,\n \t\t\t\t struct packed_git **found_pack,\n-\t\t\t\t off_t *found_offset)\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \toff_t offset;\n \n@@ -1593,7 +1664,7 @@ static int want_object_in_pack_one(struct packed_git *p,\n \t\t\t*found_offset = offset;\n \t\t\t*found_pack = p;\n \t\t}\n-\t\treturn want_found_object(oid, exclude, p);\n+\t\treturn want_found_object(oid, exclude, p, found_mtime);\n \t}\n \treturn -1;\n }\n@@ -1607,10 +1678,11 @@ static int want_object_in_pack_one(struct packed_git *p,\n * function finds if there is any pack that has the object and returns the pack\n * and its offset in these variables.\n */\n-static int want_object_in_pack(const struct object_id *oid,\n-\t\t\t int exclude,\n-\t\t\t struct packed_git **found_pack,\n-\t\t\t off_t *found_offset)\n+static int want_object_in_pack_mtime(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset,\n+\t\t\t\t uint32_t found_mtime)\n {\n \tint want;\n \tstruct list_head *pos;\n@@ -1625,7 +1697,8 @@ static int want_object_in_pack(const struct object_id *oid,\n \t * are present we will determine the answer right now.\n \t */\n \tif (*found_pack) {\n-\t\twant = want_found_object(oid, exclude, *found_pack);\n+\t\twant = want_found_object(oid, exclude, *found_pack,\n+\t\t\t\t\t found_mtime);\n \t\tif (want != -1)\n \t\t\treturn want;\n \n@@ -1636,7 +1709,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \tfor (m = get_multi_pack_index(the_repository); m; m = m->next) {\n \t\tstruct pack_entry e;\n \t\tif (fill_midx_entry(the_repository, oid, &e, m)) {\n-\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset);\n+\t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\n \t\t}\n@@ -1644,7 +1717,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \n \tlist_for_each(pos, get_packed_git_mru(the_repository)) {\n \t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n-\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset);\n+\t\twant = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\tif (!exclude && want > 0)\n \t\t\tlist_move(&p->mru,\n \t\t\t\t get_packed_git_mru(the_repository));\n@@ -1674,6 +1747,15 @@ static int want_object_in_pack(const struct object_id *oid,\n \treturn 1;\n }\n \n+static inline int want_object_in_pack(const struct object_id *oid,\n+\t\t\t\t int exclude,\n+\t\t\t\t struct packed_git **found_pack,\n+\t\t\t\t off_t *found_offset)\n+{\n+\treturn want_object_in_pack_mtime(oid, exclude, found_pack, found_offset,\n+\t\t\t\t\t 0);\n+}\n+\n static struct object_entry *create_object_entry(const struct object_id *oid,\n \t\t\t\t\t\tenum object_type type,\n \t\t\t\t\t\tuint32_t hash,\n@@ -3606,7 +3688,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type\n \t\t\tentry->no_try_delta = no_try_delta(name);\n \t\t}\n \t} else {\n-\t\tif (!want_object_in_pack(oid, 0, &pack, &offset))\n+\t\tif (!want_object_in_pack_mtime(oid, 0, &pack, &offset, mtime))\n \t\t\treturn;\n \t\tif (!pack && type == OBJ_BLOB && !has_loose_object(oid)) {\n \t\t\t/*\n@@ -3680,6 +3762,8 @@ static void mark_pack_kept_in_core(struct string_list *packs, unsigned keep)\n \t\tstruct packed_git *p = item->util;\n \t\tif (!p)\n \t\t\tdie(_(\"could not find pack '%s'\"), item->string);\n+\t\tif (p->is_cruft && keep)\n+\t\t\tignore_packed_keep_in_core_has_cruft = 1;\n \t\tp->pack_keep_in_core = keep;\n \t}\n }\ndiff --git a/packfile.c b/packfile.c\nindex 2d80d80cb3..9d09f8bc72 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -24,6 +24,7 @@\n #include \"commit-graph.h\"\n #include \"pack-revindex.h\"\n #include \"promisor-remote.h\"\n+#include \"pack-mtimes.h\"\n \n char *odb_pack_name(struct repository *r, struct strbuf *buf,\n \t\t const unsigned char *hash, const char *ext)\n@@ -2107,7 +2108,7 @@ static void maybe_invalidate_kept_pack_cache(struct repository *r,\n \tr->objects->kept_pack_cache.flags = 0;\n }\n \n-static struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags)\n {\n \tmaybe_invalidate_kept_pack_cache(r, flags);\n \ndiff --git a/packfile.h b/packfile.h\nindex 00ada7a938..25097213d0 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -197,6 +197,8 @@ int has_object_pack(struct repository *r, const struct object_id *oid);\n int has_object_kept_pack(struct repository *r, const struct object_id *oid,\n \t\t\t unsigned flags);\n \n+struct packed_git **kept_pack_cache(struct repository *r, unsigned flags);\n+\n /*\n * Return 1 if an object in a promisor packfile is or refers to the given\n * object, 0 otherwise.\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 959e6e2648..43d2947d28 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -304,6 +304,72 @@ test_expect_success '--max-cruft-size with freshened objects (packed)' '\n \t)\n '\n \n+test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n+\trepo=\"max-cruft-size-threshold\" &&\n+\n+\ttest_when_finished \"rm -fr $repo\" &&\n+\tgit init \"$repo\" &&\n+\t(\n+\t\tcd \"$repo\" &&\n+\n+\t\ttest_commit base &&\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\tbaz=\"$(generate_random_blob baz $((2*1024*1024)))\" &&\n+\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$foo\")\" >foo.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.old &&\n+\t\ttest-tool chmtime --get -100000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$baz\")\" >baz.old &&\n+\n+\t\tgit repack --cruft -d &&\n+\n+\t\t# Make an identical copy of foo stored in a pack with a more\n+\t\t# recent mtime.\n+\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n+\t\tfoo_pack=\"$(echo \"$foo\" | git pack-objects $packdir/pack)\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$packdir/pack-$foo_pack.pack\" >foo.new &&\n+\t\tgit prune-packed &&\n+\n+\t\t# Make a loose copy of bar, also with a more recent mtime.\n+\t\tbar=\"$(generate_random_blob bar $((2*1024*1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$bar\")\" >bar.new &&\n+\n+\t\t# Make a new cruft object $quux to ensure we do not\n+\t\t# generate an identical pack to the existing cruft\n+\t\t# pack.\n+\t\tquux=\"$(generate_random_blob quux $((1024)))\" &&\n+\t\ttest-tool chmtime --get -100 \\\n+\t\t\t\"$objdir/$(test_oid_to_path \"$quux\")\" >quux.new &&\n+\n+\t\tgit repack --cruft --max-cruft-size=3M -d &&\n+\n+\t\tfor p in $packdir/pack-*.mtimes\n+\t\tdo\n+\t\t\ttest-tool pack-mtimes \"$(basename \"$p\")\" || return 1\n+\t\tdone >actual.raw &&\n+\t\tsort actual.raw >actual &&\n+\n+\t\t# Among the set of all cruft packs, we should see both\n+\t\t# mtimes for object $foo and $bar, as well as the\n+\t\t# single new copy of $baz.\n+\t\tsort >expect <<-EOF &&\n+\t\t$foo $(cat foo.old)\n+\t\t$foo $(cat foo.new)\n+\t\t$bar $(cat bar.old)\n+\t\t$bar $(cat bar.new)\n+\t\t$baz $(cat baz.old)\n+\t\t$quux $(cat quux.new)\n+\t\tEOF\n+\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\nRange-diff against v4:\n1: 390c3a6d85 < -: ---------- t/t5329-pack-objects-cruft.sh: evict 'repack'-related tests\n2: e7ebe6c460 < -: ---------- t7704-repack-cruft.sh: consolidate `write_blob()`\n3: aa7588f817 < -: ---------- t/lib-cruft.sh: extract some cruft-related helpers\n4: f2ca92245a < -: ---------- pack-objects: generate cruft packs at most one object over threshold\n5: 12ddea7603 < -: ---------- builtin/repack.c: simplify cruft pack aggregation\n6: d44a124c81 ! 1: 1563552bbd builtin/pack-objects.c: freshen objects from existing cruft packs\n @@ t/t7704-repack-cruft.sh: test_expect_success '--max-cruft-size with freshened ob\n '\n \n +test_expect_success '--max-cruft-size with freshened objects (previously cruft)' '\n -+\tgit init max-cruft-size-threshold &&\n ++\trepo=\"max-cruft-size-threshold\" &&\n ++\n ++\ttest_when_finished \"rm -fr $repo\" &&\n ++\tgit init \"$repo\" &&\n +\t(\n -+\t\tcd max-cruft-size-threshold &&\n ++\t\tcd \"$repo\" &&\n +\n +\t\ttest_commit base &&\n +\t\tfoo=\"$(generate_random_blob foo $((2*1024*1024)))\" &&\n\nbase-commit: 87a0bdbf0f72b7561f3cd50636eee33dcb7dbcc3\n-- \n2.49.0.rc2.1.g1563552bbd\n"},{"id":"514224","messageId":"xmqq8qp8ojsq.fsf@gitster.g","threadId":"63018","inReplyTo":"1563552bbda0bc910c9f41b0fabc3225c4d778fc.1741889018.git.me@ttaylorr.com","subject":"Re: [PATCH v5] builtin/pack-objects.c: freshen objects from existing cruft packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T18:41:09Z","receivedAt":"2025-03-13T18:41:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Once an object is written into a cruft pack, we can only freshen it by\n> writing a new loose or packed copy of that object with a more recent\n> mtime.\n>\n> Prior to 61568efa95 (builtin/pack-objects.c: support `--max-pack-size`\n> with `--cruft`, 2023-08-28), we typically had at most one cruft pack in\n> a repository at any given time. So freshening unreachable objects was\n> straightforward when already rewriting the cruft pack (and its *.mtimes\n> file).\n>\n> But 61568efa95 changes things: 'pack-objects' now supports writing\n> multiple cruft packs when invoked with `--cruft` and the\n> `--max-pack-size` flag. Cruft packs are rewritten until they reach some\n> size threshold, at which point they are considered \"frozen\", and will\n> only be modified in a pruning GC, or if the threshold itself is\n> adjusted.\n>\n> Prior to this patch, however, this process breaks down when we attempt\n> to freshen an object packed in an earlier cruft pack, and that cruft\n> pack is larger than the threshold and thus will survive the repack.\n>\n> When this is the case, it is impossible to freshen objects in cruft\n> pack(s) when those cruft packs are larger than the threshold. This is\n> because we would avoid writing them in the new cruft pack entirely, for\n> a couple of reasons.\n>\n> 1. When enumerating packed objects via 'add_objects_in_unpacked_packs()'\n> we pass the SKIP_IN_CORE_KEPT_PACKS, which is used to avoid looping\n> over the packs we're going to retain (which are marked as kept\n> in-core by 'read_cruft_objects()').\n>\n> This means that we will avoid enumerating additional packed copies\n> of objects found in any cruft packs which are larger than the given\n> size threshold. Thus there is no opportunity to call\n> 'create_object_entry()' whatsoever.\n>\n> 2. We likewise will discard the loose copy (if one exists) of any\n> unreachable object packed in a cruft pack that is larger than the\n> threshold. Here our call path is 'add_unreachable_loose_objects()',\n> which uses the 'add_loose_object()' callback.\n>\n> That function will eventually land us in 'want_object_in_pack()'\n> (via 'add_cruft_object_entry()'), and we'll discard the object as it\n> appears in one of the packs which we marked as kept in-core.\n>\n> This means in effect that it is impossible to freshen an unreachable\n> object once it appears in a cruft pack larger than the given threshold.\n>\n> Instead, we should pack an additional copy of an unreachable object we\n> want to freshen even if it appears in a cruft pack, provided that the\n> cruft copy has an mtime which is before the mtime of the copy we are\n> trying to pack/freshen. This is sub-optimal in the sense that it\n> requires keeping an additional copy of unreachable objects upon\n> freshening, but we don't have a better alternative without the ability\n> to make in-place modifications to existing *.mtimes files.\n>\n> In order to implement this, we have to adjust the behavior of\n> 'want_found_object()'. When 'pack-objects' is told that we're *not*\n> going to retain any cruft packs (i.e. the set of packs marked as kept\n> in-core does not contain a cruft pack), the behavior is unchanged.\n>\n> But when there *is* at least one cruft pack that we're holding onto, it\n> is no longer sufficient to reject a copy of an object found in that\n> cruft pack for that reason alone. In this case, we only want to reject a\n> candidate object when copies of that object either:\n>\n> - exists in a non-cruft pack that we are retaining, regardless of that\n> pack's mtime, or\n>\n> - exists in a cruft pack with an mtime at least as recent as the copy\n> we are debating whether or not to pack, in which case freshening\n> would be redundant.\n>\n> To do this, keep track of whether or not we have any cruft packs in our\n> in-core kept list with a new 'ignore_packed_keep_in_core_has_cruft'\n> flag. When we end up in this new special case, we replace a call to\n> 'has_object_kept_pack()' to 'want_cruft_object_mtime()', and only reject\n> objects when we have a copy in an existing cruft pack with at least as\n> recent an mtime as our candidate (in which case \"freshening\" would be\n> redundant).\n>\n> Signed-off-by: Taylor Blau \n> ---\n> builtin/pack-objects.c | 118 ++++++++++++++++++++++++++++++++++------\n> packfile.c | 3 +-\n> packfile.h | 2 +\n> t/t7704-repack-cruft.sh | 66 ++++++++++++++++++++++\n> 4 files changed, 171 insertions(+), 18 deletions(-)\n\nThanks. Will queue.\n"}]}