From 8cf72312c5e0c2a11166a7024c521c53d9b7a1ec Mon Sep 17 00:00:00 2001 From: Qin ShiCheng Date: Tue, 8 Sep 2026 22:04:39 +0800 Subject: [PATCH 1/5] pack-objects: keep --keep-pack open when following "--stdin-packs=follow" distinguishes excluded packs that are closed under reachability ("^") from those that are not ("!"). The traversal stops at objects in the former, and goes on through the latter to rescue whatever they depend on that would otherwise be left out. A pack named with "--keep-pack" gets the same in-core flag as a "^" pack, so the traversal stops at it too. Nothing warrants that: the caller said not to repack it, not that it is self-contained. When it holds a commit but not that commit's tree, the tree is never rescued, and writing a bitmap over the result fails for lack of closure. In follow mode, mark such a pack as kept-open instead, the way repack already lists the packs it cannot vouch for as "!" on stdin. Its objects stay out of the result, and the traversal can go through it. This matters more once repack names its ".keep" packs this way instead of passing "--honor-pack-keep": on-disk kept packs never were a boundary, and they should not become one. Signed-off-by: Qin ShiCheng --- builtin/pack-objects.c | 20 +++++++++++++---- t/t5331-pack-objects-stdin.sh | 41 +++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 708b719f403b96..6f579173b06552 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4999,7 +4999,8 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv) oid_array_clear(&recent_objects); } -static void add_extra_kept_packs(const struct string_list *names) +static void add_extra_kept_packs(const struct string_list *names, + enum stdin_packs_mode stdin_packs) { struct packed_git *p; @@ -5018,8 +5019,19 @@ static void add_extra_kept_packs(const struct string_list *names) break; if (i < names->nr) { - p->pack_keep_in_core = 1; - ignore_packed_keep_in_core = 1; + /* + * When following, treat the pack like a "!" pack, not + * a "^" one: nobody said it is closed under + * reachability, so the traversal must be able to go + * through it. + */ + if (stdin_packs == STDIN_PACKS_MODE_FOLLOW) { + p->pack_keep_in_core_open = 1; + ignore_packed_keep_in_core_open = 1; + } else { + p->pack_keep_in_core = 1; + ignore_packed_keep_in_core = 1; + } continue; } } @@ -5443,7 +5455,7 @@ int cmd_pack_objects(int argc, if (progress && all_progress_implied) progress = 2; - add_extra_kept_packs(&keep_pack_list); + add_extra_kept_packs(&keep_pack_list, stdin_packs); if (ignore_packed_keep_on_disk) { struct packed_git *p; diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh index c74b5861af322f..4e1fde1b089471 100755 --- a/t/t5331-pack-objects-stdin.sh +++ b/t/t5331-pack-objects-stdin.sh @@ -483,6 +483,47 @@ test_expect_success '--stdin-packs=follow with open-excluded packs' ' ) ' +test_expect_success '--stdin-packs=follow walks through a --keep-pack pack' ' + test_when_finished "rm -fr repo" && + + git init repo && + ( + cd repo && + git config set maintenance.auto false && + + test_commit A && + test_commit B && + test_commit C && + + A="$(echo A | git pack-objects --revs $packdir/pack)" && + B="$(echo A..B | git pack-objects --revs $packdir/pack)" && + C="$(echo B..C | git pack-objects --revs $packdir/pack)" && + B_ONLY="$(git rev-parse B | git pack-objects $packdir/pack)" && + git prune-packed && + + # Pack C is included and pack A is excluded and closed. The + # commit B is in the kept pack B_ONLY, but its tree and blob + # are only in pack B, which pack-objects is not told about. + # The kept pack keeps B out of the result, and the walk has + # to go through it to rescue the tree and the blob. + P=$(git pack-objects --stdin-packs=follow \ + --keep-pack=pack-$B_ONLY.pack $packdir/pack <<-EOF + pack-$C.pack + ^pack-$A.pack + EOF + ) && + + { + objects_in_packs $C && + git rev-parse "B^{tree}" B:B.t + } >expect.raw && + sort expect.raw >expect && + + objects_in_packs $P >actual && + test_cmp expect actual + ) +' + test_expect_success '--stdin-packs with !-delimited pack without follow' ' test_when_finished "rm -fr repo" && From 77aec8941f5d17654f58956c7c643b47dd5a8d93 Mon Sep 17 00:00:00 2001 From: Qin ShiCheng Date: Tue, 8 Sep 2026 22:04:39 +0800 Subject: [PATCH 2/5] pack-objects: reset kept-pack cache for cruft walk When writing a cruft pack with an expiration, pack-objects first collects the recent objects and then walks from them to rescue whatever they reach, expired or not. A pack the caller did not list is marked kept while collecting, so that its objects are not copied into the cruft pack, and unmarked before the walk, so that the walk can go through it. The walk does not see the unmarking. Whether an object sits in a kept pack is answered from a cache that is built on first use and only dropped when asked about a different kind of kept pack. Collecting builds it while the unlisted pack is still marked, the walk asks the same kind of question, and so the unlisted pack stays in it: the walk stops there, and whatever lies beyond it in an expired pack is lost. This went unnoticed because of "--honor-pack-keep". repack passes it, and when there is a ".keep" file it makes the collecting side ask about on-disk and in-core kept packs together while the walk asks about in-core ones alone; the cache is rebuilt each time the question changes, and by accident the walk sees the current marks. Take the ".keep" file away and the objects are lost today. A later commit stops repack from passing "--honor-pack-keep" at all, so fix this first. Expose the invalidation packfile.c already has and call it after re-marking. The test builds an unreachable chain whose middle commit sits in a pack pack-objects is not told about and whose oldest objects have expired; without the fix the cruft pack holds only the recent tip. Signed-off-by: Qin ShiCheng --- builtin/pack-objects.c | 8 +++++++ odb/source-packed.h | 3 ++- packfile.c | 9 ++++++-- packfile.h | 7 ++++++ t/t5329-pack-objects-cruft.sh | 40 +++++++++++++++++++++++++++++++++++ 5 files changed, 64 insertions(+), 3 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 6f579173b06552..8ca8255176c90c 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4274,6 +4274,7 @@ static void enumerate_cruft_objects(void) static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs) { struct packed_git *p; + struct odb_source *source; struct rev_info revs; int ret; @@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs /* * Re-mark only the fresh packs as kept so that objects in * unknown packs do not halt the reachability traversal early. + * The kept-pack cache was built while those packs were still + * marked, so drop it too. */ repo_for_each_pack(the_repository, p) p->pack_keep_in_core = 0; mark_pack_kept_in_core(fresh_packs, 1); + for (source = the_repository->objects->sources; source; + source = source->next) { + struct odb_source_files *files = odb_source_files_downcast(source); + packfile_store_invalidate_kept_pack_cache(files->packed); + } if (prepare_revision_walk(&revs)) die(_("revision walk setup failed")); diff --git a/odb/source-packed.h b/odb/source-packed.h index a0f6b5096dcd0f..9e42311916e2d5 100644 --- a/odb/source-packed.h +++ b/odb/source-packed.h @@ -25,7 +25,8 @@ struct odb_source_packed { * Should not be accessed directly, but via * `packfile_store_get_kept_pack_cache()`. The list of packs gets * invalidated when the stored flags and the flags passed to - * `packfile_store_get_kept_pack_cache()` mismatch. + * `packfile_store_get_kept_pack_cache()` mismatch, or explicitly via + * `packfile_store_invalidate_kept_pack_cache()`. */ struct { struct packed_git **packs; diff --git a/packfile.c b/packfile.c index 4fa5fd67c8497f..90459ec4d79ffc 100644 --- a/packfile.c +++ b/packfile.c @@ -1870,6 +1870,12 @@ int packfile_fill_entry(struct packed_git *p, return 1; } +void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store) +{ + FREE_AND_NULL(store->kept_cache.packs); + store->kept_cache.flags = 0; +} + static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store, unsigned flags) { @@ -1877,8 +1883,7 @@ static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store, return; if (store->kept_cache.flags == flags) return; - FREE_AND_NULL(store->kept_cache.packs); - store->kept_cache.flags = 0; + packfile_store_invalidate_kept_pack_cache(store); } struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store, diff --git a/packfile.h b/packfile.h index 6d30d15a0053b3..493faf001038b2 100644 --- a/packfile.h +++ b/packfile.h @@ -144,6 +144,13 @@ enum kept_pack_type { struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store, unsigned flags); +/* + * Drop the cache of kept packs so that the next call to + * `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after changing + * which packs are kept in core. + */ +void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store); + struct pack_window { struct pack_window *next; unsigned char *base; diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh index 12cda063730372..6302f60b759b89 100755 --- a/t/t5329-pack-objects-cruft.sh +++ b/t/t5329-pack-objects-cruft.sh @@ -332,6 +332,46 @@ test_expect_success 'cruft trees rescue sub-trees, blobs' ' ) ' +test_expect_success 'cruft traversal rescues through a pack it was not told about' ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + test_commit packed && + git repack -Ad && + keep="$(basename "$(ls $packdir/pack-*.pack)")" && + + test_commit old && + test_commit mid && + test_commit new && + + # "old" has expired, "new" is recent, and "mid" sits in a + # pack that pack-objects is not told about. Rescuing "old" + # from "new" means walking through that pack. + git rev-list --objects --no-object-names packed..old >old && + while read object + do + test-tool chmtime -1000 \ + "$objdir/$(test_oid_to_path $object)" || exit 1 + done /dev/null && + git prune-packed && + + cruft="$(echo $keep | git pack-objects --cruft \ + --cruft-expiration=750.seconds.ago \ + $packdir/pack)" && + test-tool pack-mtimes "pack-$cruft.mtimes" >actual.raw && + + cut -d" " -f1 actual && + git rev-list --objects --no-object-names packed..new >expect.raw && + sort expect && + + test_cmp expect actual + ) +' + test_expect_success 'expired objects are pruned' ' git init repo && test_when_finished "rm -fr repo" && From b76e06a4672ed7881fb4ccfd389d5282a6f312b7 Mon Sep 17 00:00:00 2001 From: Qin ShiCheng Date: Tue, 8 Sep 2026 22:04:39 +0800 Subject: [PATCH 3/5] pack-objects: sort --keep-pack list for lookup add_extra_kept_packs() scans the whole "--keep-pack" list once per pack in the repository. That is fine for the handful of names it gets today, but the next commit lets a caller name every kept pack in the repository, and with thousands of them the scan dominates: matching 20,000 kept packs against 20,000 names takes 11 seconds here, against under a second with "--honor-pack-keep". Sort the list once and look each pack up in it. The comparison stays fspathcmp(), so what matches does not change. Signed-off-by: Qin ShiCheng --- builtin/pack-objects.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 8ca8255176c90c..1fcb4ef8a53b40 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -5007,7 +5007,7 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv) oid_array_clear(&recent_objects); } -static void add_extra_kept_packs(const struct string_list *names, +static void add_extra_kept_packs(struct string_list *names, enum stdin_packs_mode stdin_packs) { struct packed_git *p; @@ -5015,18 +5015,13 @@ static void add_extra_kept_packs(const struct string_list *names, if (!names->nr) return; - repo_for_each_pack(the_repository, p) { - const char *name = basename(p->pack_name); - int i; + string_list_sort(names); + repo_for_each_pack(the_repository, p) { if (!p->pack_local) continue; - for (i = 0; i < names->nr; i++) - if (!fspathcmp(name, names->items[i].string)) - break; - - if (i < names->nr) { + if (string_list_has_string(names, basename(p->pack_name))) { /* * When following, treat the pack like a "!" pack, not * a "^" one: nobody said it is closed under @@ -5151,7 +5146,9 @@ int cmd_pack_objects(int argc, int rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0; int rev_list_index = 0; enum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE; - struct string_list keep_pack_list = STRING_LIST_INIT_NODUP; + struct string_list keep_pack_list = { + .cmp = fspathcmp, + }; struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT; struct repo_config_values *cfg = repo_config_values(the_repository); From 20a051cfb6fb23b6cdb7adf6cf53a2b8eb2f4391 Mon Sep 17 00:00:00 2001 From: Qin ShiCheng Date: Tue, 8 Sep 2026 22:04:39 +0800 Subject: [PATCH 4/5] pack-objects: add --keep-pack-from-file "--keep-pack" names one pack per occurrence, and there is only so much room on the command line: ARG_MAX is shared with the environment, and on Windows the whole line is capped at 32,767 characters, which a few hundred pack names fill. Past that the spawn fails before pack-objects has started. fetch-pack grew "--stdin" in 078b895fef (fetch-pack: new --stdin option to read refs from stdin, 2012-04-02) for the same reason. stdin is taken here: every mode repack drives pack-objects in already uses it, for the revision list under "-a", object names for the promisor pack, and pack lists for "--stdin-packs" and "--cruft". So read the names from a file instead, one per line, skipping empty lines. They go into the same list as the "--keep-pack" names and are treated exactly alike: matched against local packs, ignored when they match nothing, and kept open under "--stdin-packs=follow". A relative path is resolved against the directory the user ran from, as "--refs-snapshot" of "git multi-pack-index write" is. The list now holds strings from two sources, so let it own its copies. repack is about to use this to hand pack-objects its own snapshot of the packs that have a ".keep" file. Signed-off-by: Qin ShiCheng --- Documentation/git-pack-objects.adoc | 8 +++++ builtin/pack-objects.c | 28 ++++++++++++++++++ t/t5331-pack-objects-stdin.sh | 46 +++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+) diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc index 65cd00c152f495..938e27f69dc5da 100644 --- a/Documentation/git-pack-objects.adoc +++ b/Documentation/git-pack-objects.adoc @@ -13,6 +13,7 @@ SYNOPSIS [--no-reuse-delta] [--delta-base-offset] [--non-empty] [--local] [--incremental] [--window=] [--depth=] [--revs [--unpacked | --all]] [--keep-pack=] + [--keep-pack-from-file=] [--cruft] [--cruft-expiration=