# [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run 18 messages from 2026-09-14 to 2026-09-23. Participants: qeesung via GitGitGadget, Qin ShiCheng via GitGitGadget, Justin Tobler, Qin ShiCheng, Junio C Hamano. Thread: https://gitlist.dev/t/66327 ## qeesung via GitGitGadget, 2026-09-14 11:31 Subject: [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run Message-ID: ``` A concurrent push can make "git repack -d" delete a pack whose objects were never copied anywhere, and exit 0. We hit this in production: a ref pointing at a commit that no longer exists, on git 2.43, and it reproduces on master. What happens: * repack scans for ".keep" files and decides which packs to delete, then spawns pack-objects with --honor-pack-keep, which scans again; * in between, a push of content identical to an earlier one finishes migrating its quarantine. Its pack is a duplicate and is dropped, but its ".keep" is linked into place, onto the old pack; * pack-objects sees that ".keep" and leaves the pack's objects out; repack deletes the pack by its earlier list, with force_delete. Two things are wrong, and each is fixed on its own: * 1/6: receive-pack removes a ".keep" it never installed -- the one it linked onto somebody else's pack, or a foreign one when its own push was rejected before any migration. Only remove a ".keep" that carries our own message. * 6/6: repack and pack-objects each scan for ".keep" files. Hand pack-objects the snapshot repack took at startup instead. Patches 2-5 are what 6/6 needs to be safe: * 2/6: under --stdin-packs=follow, a --keep-pack pack stops the traversal like a "^" pack; on-disk ".keep" packs never did. * 3/6: the cruft walk goes by a stale kept-pack cache, which --honor-pack-keep happened to mask. Pre-existing, reproducible today. * 4/6: look --keep-pack names up in a sorted list; it gets long. * 5/6: --keep-pack-from-file, since a repository can have more kept packs than fit on a command line (32K characters on Windows). Every fix comes with a test that fails without it; the race itself is reproduced in t7703 by having a ".keep" appear as pack-objects starts. The full suite passes, and the series merges cleanly into next and seen. Qin ShiCheng (6): odb: don't remove a ".keep" we never installed pack-objects: keep --keep-pack open when following pack-objects: reset kept-pack cache for cruft walk pack-objects: sort --keep-pack list for lookup pack-objects: add --keep-pack-from-file repack: tell pack-objects which packs are kept Documentation/git-pack-objects.adoc | 8 +++ builtin/pack-objects.c | 71 +++++++++++++++++---- builtin/repack.c | 15 +++++ object-file.c | 95 ++++++++++++++++++++++------- odb/source-packed.h | 3 +- packfile.c | 9 ++- packfile.h | 7 +++ repack-filtered.c | 3 - repack.c | 34 ++++++++++- repack.h | 17 +++++- t/t5329-pack-objects-cruft.sh | 40 ++++++++++++ t/t5331-pack-objects-stdin.sh | 87 ++++++++++++++++++++++++++ t/t5547-push-quarantine.sh | 52 ++++++++++++++++ t/t7700-repack.sh | 43 +++++++++++++ t/t7703-repack-geometric.sh | 72 ++++++++++++++++++++++ tempfile.c | 12 ++++ tempfile.h | 9 +++ 17 files changed, 533 insertions(+), 44 deletions(-) base-commit: 3cb9185f65410273787f74333cc027d2ea5daada Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2219%2Fqeesung%2Frepack-kept-packs-snapshot-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2219/qeesung/repack-kept-packs-snapshot-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/2219 -- gitgitgadget ``` ## Qin ShiCheng via GitGitGadget, 2026-09-14 11:31 Subject: [PATCH 1/6] odb: don't remove a ".keep" we never installed Message-ID: <932e8e425aecfbd33c1e5caf66c80a0226abacba.1789385483.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Qin ShiCheng receive-pack runs index-pack with "--keep" over the quarantine, which writes a "pack-XXX.keep" there. The path we register as a tempfile is a different one: where that ".keep" will land once the quarantine is migrated into the main object database. Nothing of ours is at that path yet, and something else may be. Two pushes of identical content produce identical thin packs, index-pack names a pack after its contents, and so both want the same ".keep" in the main object database. If the other push still holds it, that file is what keeps its pack from being repacked away, and we remove it at exit regardless -- even when pre-receive rejected our push and nothing was migrated at all. Register the path right before the migration instead, and once the migration has returned, read the files back. index-pack wrote the message we handed it; a file that says something else was not written for us, so let go of it without removing it. tempfile gains unregister_tempfile() for that. Registering only after the migration would leave a window: the ".keep" is the first thing migrated, and for a push that duplicates a large pack the migration then spends a while comparing the two packfiles. A signal in between would leave our ".keep" behind, with our message in it, and every later push of the same content would fail to migrate over it. Registering first keeps that window closed, as it is today. Reading the files back also covers a migration that fails partway through with our ".keep" already in place: we go by what is there, not by whether the migration succeeded, and still remove it. Signed-off-by: Qin ShiCheng --- object-file.c | 95 +++++++++++++++++++++++++++++--------- t/t5547-push-quarantine.sh | 52 +++++++++++++++++++++ tempfile.c | 12 +++++ tempfile.h | 9 ++++ 4 files changed, 147 insertions(+), 21 deletions(-) diff --git a/object-file.c b/object-file.c index a4cbf8b081..21513ee535 100644 --- a/object-file.c +++ b/object-file.c @@ -29,6 +29,7 @@ #include "read-cache-ll.h" #include "run-command.h" #include "setup.h" +#include "string-list.h" #include "strvec.h" #include "tempfile.h" #include "tmp-objdir.h" @@ -492,9 +493,13 @@ struct odb_transaction_files { struct transaction_packfile packfile; const char *prefix; - struct tempfile **pack_lockfiles; - size_t pack_lockfiles_nr; - size_t pack_lockfiles_alloc; + /* + * The message index-pack writes into its ".keep" files, and where + * those files end up once the quarantine is migrated. Each "util" + * holds a tempfile for as long as we consider that file ours. + */ + char *keep_msg; + struct string_list pack_lockfiles; }; int odb_transaction_files_prepare(struct odb_transaction *base) @@ -1256,6 +1261,45 @@ out: return ret; } +/* + * Track the ".keep" files before the migration moves them into place, so + * that a signal in the middle of it removes ours. + */ +static void register_pack_lockfiles(struct odb_transaction_files *transaction) +{ + struct string_list_item *item; + + for_each_string_list_item(item, &transaction->pack_lockfiles) + item->util = register_tempfile(item->string); +} + +/* + * The migration stops at the first file that differs from what is already + * at its destination, and a ".keep" left by somebody else's push is one + * such file. Rather than work out what got installed, read the files + * back: one that does not carry our message is not ours to remove. + */ +static void disown_foreign_pack_lockfiles(struct odb_transaction_files *transaction) +{ + struct strbuf buf = STRBUF_INIT; + struct string_list_item *item; + + for_each_string_list_item(item, &transaction->pack_lockfiles) { + struct tempfile *lockfile = item->util; + + strbuf_reset(&buf); + if (strbuf_read_file(&buf, item->string, 0) >= 0) { + strbuf_trim_trailing_newline(&buf); + if (!strcmp(buf.buf, transaction->keep_msg)) + continue; + } + unregister_tempfile(&lockfile); + item->util = NULL; + } + + strbuf_release(&buf); +} + static int odb_transaction_files_commit(struct odb_transaction *base) { struct odb_transaction_files *transaction = @@ -1264,6 +1308,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base) if (transaction->objdir) { struct strbuf temp_path = STRBUF_INIT; struct tempfile *temp; + int ret; /* * Issue a full hardware flush against a temporary file to ensure @@ -1285,7 +1330,10 @@ static int odb_transaction_files_commit(struct odb_transaction *base) * Make the object files visible in the primary ODB after their data is * fully durable. */ - if (tmp_objdir_migrate(transaction->objdir)) + register_pack_lockfiles(transaction); + ret = tmp_objdir_migrate(transaction->objdir); + disown_foreign_pack_lockfiles(transaction); + if (ret) return error(_("unable to migrate temporary objects")); transaction->objdir = NULL; @@ -1393,10 +1441,10 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, if (xgethostname(hostname, sizeof(hostname))) xsnprintf(hostname, sizeof(hostname), "localhost"); - strvec_pushf(&child.args, - "--keep=receive-pack %"PRIuMAX" on %s", - (uintmax_t)getpid(), - hostname); + free(transaction->keep_msg); + transaction->keep_msg = xstrfmt("receive-pack %"PRIuMAX" on %s", + (uintmax_t)getpid(), hostname); + strvec_pushf(&child.args, "--keep=%s", transaction->keep_msg); if (!opts->quiet && err_fd) strvec_push(&child.args, "--show-resolving-progress"); @@ -1423,18 +1471,13 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base, /* * The lockfile filepath is expected to be the final location of * the ".keep" file after being migrated to the main ODB source. - * This ensures the lockfile can be found and removed later - * after the ODB transaction has been committed. + * We start tracking it right before that migration; see + * odb_transaction_files_commit(). */ lockfile = index_pack_lockfile(base->source, child.out, NULL); - if (lockfile) { - ALLOC_GROW(transaction->pack_lockfiles, - transaction->pack_lockfiles_nr + 1, - transaction->pack_lockfiles_alloc); - transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] = - register_tempfile(lockfile); - free(lockfile); - } + if (lockfile) + string_list_append_nodup(&transaction->pack_lockfiles, + lockfile); close(child.out); status = finish_command(&child); @@ -1454,12 +1497,21 @@ static int odb_transaction_files_finalize(struct odb_transaction *base) { struct odb_transaction_files *transaction = container_of(base, struct odb_transaction_files, base); + struct string_list_item *item; int ret = 0; - for (size_t i = 0; i < transaction->pack_lockfiles_nr; i++) - ret |= delete_tempfile(&transaction->pack_lockfiles[i]); + /* + * Only the ".keep" files that turned out to be ours still have a + * tempfile attached; delete_tempfile() does nothing for the rest. + */ + for_each_string_list_item(item, &transaction->pack_lockfiles) { + struct tempfile *lockfile = item->util; + + ret |= delete_tempfile(&lockfile); + } - free(transaction->pack_lockfiles); + string_list_clear(&transaction->pack_lockfiles, 0); + FREE_AND_NULL(transaction->keep_msg); return ret; } @@ -1492,6 +1544,7 @@ int odb_transaction_files_begin(struct odb_source *source, transaction->base.write_pack = odb_transaction_files_write_pack; transaction->base.env = odb_transaction_files_env; transaction->flags = flags; + string_list_init_dup(&transaction->pack_lockfiles); transaction->prefix = "bulk-fsync"; if (flags & ODB_TRANSACTION_RECEIVE) { diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh index 1b7097179e..8623d2d6c1 100755 --- a/t/t5547-push-quarantine.sh +++ b/t/t5547-push-quarantine.sh @@ -101,4 +101,56 @@ test_expect_success '.keep file is removed after push' ' test_path_is_missing "$keep" ' +test_expect_success 'a rejected push does not remove a foreign ".keep"' ' + test_when_finished rm -rf foreign.git && + git init --bare foreign.git && + git -C foreign.git config set receive.unpackLimit 0 && + + # Get a packfile into the main object database without updating any + # ref, so that pushing the same objects again reuses its name. + test_hook -C foreign.git update <<-\EOF && + exit 1 + EOF + test_commit foreign && + test_must_fail git push foreign.git HEAD:refs/heads/one && + + pack="$(ls foreign.git/objects/pack/pack-*.pack)" && + keep="${pack%.pack}.keep" && + + # Pretend somebody else holds the lock on that packfile, and let the + # next push be rejected before its objects are ever migrated. + >"$keep" && + test_hook -C foreign.git pre-receive <<-\EOF && + exit 1 + EOF + test_must_fail git push foreign.git HEAD:refs/heads/two && + test_path_is_file "$keep" +' + +test_expect_success 'a ".keep" installed by a failed migration is removed' ' + test_when_finished rm -rf partial.git && + git init --bare partial.git && + git -C partial.git config set receive.unpackLimit 0 && + git -C partial.git config set pack.indexVersion 1 && + + # Leave the objects in the main object database without a ref, so + # that pushing them again produces a pack with the same name. + test_hook -C partial.git update <<-\EOF && + exit 1 + EOF + test_commit partial && + test_must_fail git push partial.git HEAD:refs/heads/one && + + # The same pack now arrives with a differently formatted index. The + # ".keep" is migrated first and goes in fine; the index then collides + # with the one already there, and the migration fails with our + # ".keep" already installed. + git -C partial.git config set pack.indexVersion 2 && + test_must_fail git push partial.git HEAD:refs/heads/two 2>err && + test_grep "unable to migrate" err && + + pack="$(ls partial.git/objects/pack/pack-*.pack)" && + test_path_is_missing "${pack%.pack}.keep" +' + test_done diff --git a/tempfile.c b/tempfile.c index dc9ca4e645..10db4fbc7f 100644 --- a/tempfile.c +++ b/tempfile.c @@ -373,6 +373,18 @@ int delete_tempfile(struct tempfile **tempfile_p) return err ? -1 : 0; } +void unregister_tempfile(struct tempfile **tempfile_p) +{ + struct tempfile *tempfile = *tempfile_p; + + if (!is_tempfile_active(tempfile)) + return; + + close_tempfile_gently(tempfile); + deactivate_tempfile(tempfile); + *tempfile_p = NULL; +} + void reassign_tempfile_ownership(pid_t from, pid_t to) { volatile struct volatile_list_head *pos; diff --git a/tempfile.h b/tempfile.h index f571f3c609..b439066a30 100644 --- a/tempfile.h +++ b/tempfile.h @@ -275,6 +275,15 @@ int reopen_tempfile(struct tempfile *tempfile); */ int delete_tempfile(struct tempfile **tempfile_p); +/* + * Stop tracking `tempfile` without removing the file: close the file + * descriptor and/or file pointer if they are still open, and leave the + * file where it is, no longer to be removed at exit or on a signal. It + * is a NOOP to call `unregister_tempfile()` for a `tempfile` object + * that is not currently active. + */ +void unregister_tempfile(struct tempfile **tempfile_p); + /* * Close the file descriptor and/or file pointer if they are still * open, and atomically rename the temporary file to `path`. `path` -- gitgitgadget ``` ## Qin ShiCheng via GitGitGadget, 2026-09-14 11:31 Subject: [PATCH 2/6] pack-objects: keep --keep-pack open when following Message-ID: <9349ea48b09347eff5da8a8862268d63605af690.1789385483.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Qin ShiCheng "--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 708b719f40..6f579173b0 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 c74b5861af..4e1fde1b08 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" && -- gitgitgadget ``` ## Qin ShiCheng via GitGitGadget, 2026-09-14 11:31 Subject: [PATCH 3/6] pack-objects: reset kept-pack cache for cruft walk Message-ID: In-Reply-To: ``` From: Qin ShiCheng 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 6f579173b0..8ca8255176 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 a0f6b5096d..9e42311916 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 4fa5fd67c8..90459ec4d7 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 6d30d15a00..493faf0010 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 12cda06373..6302f60b75 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" && -- gitgitgadget ``` ## Qin ShiCheng via GitGitGadget, 2026-09-14 11:31 Subject: [PATCH 4/6] pack-objects: sort --keep-pack list for lookup Message-ID: <38070935dc479099375b89f76a2f3c1db52e6577.1789385483.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Qin ShiCheng 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 8ca8255176..1fcb4ef8a5 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); -- gitgitgadget ``` ## Qin ShiCheng via GitGitGadget, 2026-09-14 11:31 Subject: [PATCH 5/6] pack-objects: add --keep-pack-from-file Message-ID: In-Reply-To: ``` From: Qin ShiCheng "--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 65cd00c152..938e27f69d 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=