{"thread":{"id":"66327","subject":"[PATCH 0/6] repack: don't lose objects to a \".keep\" that appears mid-run","startedAt":"2026-09-14T11:31:26Z","lastAt":"2026-09-23T17:45:16Z","messageCount":18,"participants":["qeesung via GitGitGadget","Qin ShiCheng via GitGitGadget","Justin Tobler","Qin ShiCheng","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"552693","messageId":"pull.2219.git.1789385483.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":null,"subject":"[PATCH 0/6] repack: don't lose objects to a \".keep\" that appears mid-run","fromName":"qeesung via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-14T11:31:17Z","receivedAt":"2026-09-14T11:31:26Z","isPatch":true,"body":"A concurrent push can make \"git repack -d\" delete a pack whose objects were\nnever copied anywhere, and exit 0. We hit this in production: a ref pointing\nat a commit that no longer exists, on git 2.43, and it reproduces on master.\n\nWhat happens:\n\n * repack scans for \".keep\" files and decides which packs to delete, then\n   spawns pack-objects with --honor-pack-keep, which scans again;\n * in between, a push of content identical to an earlier one finishes\n   migrating its quarantine. Its pack is a duplicate and is dropped, but its\n   \".keep\" is linked into place, onto the old pack;\n * pack-objects sees that \".keep\" and leaves the pack's objects out; repack\n   deletes the pack by its earlier list, with force_delete.\n\nTwo things are wrong, and each is fixed on its own:\n\n * 1/6: receive-pack removes a \".keep\" it never installed -- the one it\n   linked onto somebody else's pack, or a foreign one when its own push was\n   rejected before any migration. Only remove a \".keep\" that carries our own\n   message.\n * 6/6: repack and pack-objects each scan for \".keep\" files. Hand\n   pack-objects the snapshot repack took at startup instead.\n\nPatches 2-5 are what 6/6 needs to be safe:\n\n * 2/6: under --stdin-packs=follow, a --keep-pack pack stops the traversal\n   like a \"^\" pack; on-disk \".keep\" packs never did.\n * 3/6: the cruft walk goes by a stale kept-pack cache, which\n   --honor-pack-keep happened to mask. Pre-existing, reproducible today.\n * 4/6: look --keep-pack names up in a sorted list; it gets long.\n * 5/6: --keep-pack-from-file, since a repository can have more kept packs\n   than fit on a command line (32K characters on Windows).\n\nEvery fix comes with a test that fails without it; the race itself is\nreproduced in t7703 by having a \".keep\" appear as pack-objects starts. The\nfull suite passes, and the series merges cleanly into next and seen.\n\nQin ShiCheng (6):\n  odb: don't remove a \".keep\" we never installed\n  pack-objects: keep --keep-pack open when following\n  pack-objects: reset kept-pack cache for cruft walk\n  pack-objects: sort --keep-pack list for lookup\n  pack-objects: add --keep-pack-from-file\n  repack: tell pack-objects which packs are kept\n\n Documentation/git-pack-objects.adoc |  8 +++\n builtin/pack-objects.c              | 71 +++++++++++++++++----\n builtin/repack.c                    | 15 +++++\n object-file.c                       | 95 ++++++++++++++++++++++-------\n odb/source-packed.h                 |  3 +-\n packfile.c                          |  9 ++-\n packfile.h                          |  7 +++\n repack-filtered.c                   |  3 -\n repack.c                            | 34 ++++++++++-\n repack.h                            | 17 +++++-\n t/t5329-pack-objects-cruft.sh       | 40 ++++++++++++\n t/t5331-pack-objects-stdin.sh       | 87 ++++++++++++++++++++++++++\n t/t5547-push-quarantine.sh          | 52 ++++++++++++++++\n t/t7700-repack.sh                   | 43 +++++++++++++\n t/t7703-repack-geometric.sh         | 72 ++++++++++++++++++++++\n tempfile.c                          | 12 ++++\n tempfile.h                          |  9 +++\n 17 files changed, 533 insertions(+), 44 deletions(-)\n\n\nbase-commit: 3cb9185f65410273787f74333cc027d2ea5daada\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2219%2Fqeesung%2Frepack-kept-packs-snapshot-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2219/qeesung/repack-kept-packs-snapshot-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2219\n-- \ngitgitgadget\n"},{"id":"552694","messageId":"932e8e425aecfbd33c1e5caf66c80a0226abacba.1789385483.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.git.1789385483.gitgitgadget@gmail.com","subject":"[PATCH 1/6] odb: don't remove a \".keep\" we never installed","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-14T11:31:18Z","receivedAt":"2026-09-14T11:31:27Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\nreceive-pack runs index-pack with \"--keep\" over the quarantine, which\nwrites a \"pack-XXX.keep\" there. The path we register as a tempfile is\na different one: where that \".keep\" will land once the quarantine is\nmigrated into the main object database.\n\nNothing of ours is at that path yet, and something else may be. Two\npushes of identical content produce identical thin packs, index-pack\nnames a pack after its contents, and so both want the same \".keep\" in\nthe main object database. If the other push still holds it, that file\nis what keeps its pack from being repacked away, and we remove it at\nexit regardless -- even when pre-receive rejected our push and nothing\nwas migrated at all.\n\nRegister the path right before the migration instead, and once the\nmigration has returned, read the files back. index-pack wrote the\nmessage we handed it; a file that says something else was not written\nfor us, so let go of it without removing it. tempfile gains\nunregister_tempfile() for that.\n\nRegistering only after the migration would leave a window: the \".keep\"\nis the first thing migrated, and for a push that duplicates a large\npack the migration then spends a while comparing the two packfiles. A\nsignal in between would leave our \".keep\" behind, with our message in\nit, and every later push of the same content would fail to migrate\nover it. Registering first keeps that window closed, as it is today.\n\nReading the files back also covers a migration that fails partway\nthrough with our \".keep\" already in place: we go by what is there, not\nby whether the migration succeeded, and still remove it.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n object-file.c              | 95 +++++++++++++++++++++++++++++---------\n t/t5547-push-quarantine.sh | 52 +++++++++++++++++++++\n tempfile.c                 | 12 +++++\n tempfile.h                 |  9 ++++\n 4 files changed, 147 insertions(+), 21 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex a4cbf8b081..21513ee535 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -29,6 +29,7 @@\n #include \"read-cache-ll.h\"\n #include \"run-command.h\"\n #include \"setup.h\"\n+#include \"string-list.h\"\n #include \"strvec.h\"\n #include \"tempfile.h\"\n #include \"tmp-objdir.h\"\n@@ -492,9 +493,13 @@ struct odb_transaction_files {\n \tstruct transaction_packfile packfile;\n \tconst char *prefix;\n \n-\tstruct tempfile **pack_lockfiles;\n-\tsize_t pack_lockfiles_nr;\n-\tsize_t pack_lockfiles_alloc;\n+\t/*\n+\t * The message index-pack writes into its \".keep\" files, and where\n+\t * those files end up once the quarantine is migrated. Each \"util\"\n+\t * holds a tempfile for as long as we consider that file ours.\n+\t */\n+\tchar *keep_msg;\n+\tstruct string_list pack_lockfiles;\n };\n \n int odb_transaction_files_prepare(struct odb_transaction *base)\n@@ -1256,6 +1261,45 @@ out:\n \treturn ret;\n }\n \n+/*\n+ * Track the \".keep\" files before the migration moves them into place, so\n+ * that a signal in the middle of it removes ours.\n+ */\n+static void register_pack_lockfiles(struct odb_transaction_files *transaction)\n+{\n+\tstruct string_list_item *item;\n+\n+\tfor_each_string_list_item(item, &transaction->pack_lockfiles)\n+\t\titem->util = register_tempfile(item->string);\n+}\n+\n+/*\n+ * The migration stops at the first file that differs from what is already\n+ * at its destination, and a \".keep\" left by somebody else's push is one\n+ * such file. Rather than work out what got installed, read the files\n+ * back: one that does not carry our message is not ours to remove.\n+ */\n+static void disown_foreign_pack_lockfiles(struct odb_transaction_files *transaction)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\n+\tfor_each_string_list_item(item, &transaction->pack_lockfiles) {\n+\t\tstruct tempfile *lockfile = item->util;\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tif (strbuf_read_file(&buf, item->string, 0) >= 0) {\n+\t\t\tstrbuf_trim_trailing_newline(&buf);\n+\t\t\tif (!strcmp(buf.buf, transaction->keep_msg))\n+\t\t\t\tcontinue;\n+\t\t}\n+\t\tunregister_tempfile(&lockfile);\n+\t\titem->util = NULL;\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static int odb_transaction_files_commit(struct odb_transaction *base)\n {\n \tstruct odb_transaction_files *transaction =\n@@ -1264,6 +1308,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)\n \tif (transaction->objdir) {\n \t\tstruct strbuf temp_path = STRBUF_INIT;\n \t\tstruct tempfile *temp;\n+\t\tint ret;\n \n \t\t/*\n \t\t * Issue a full hardware flush against a temporary file to ensure\n@@ -1285,7 +1330,10 @@ static int odb_transaction_files_commit(struct odb_transaction *base)\n \t\t * Make the object files visible in the primary ODB after their data is\n \t\t * fully durable.\n \t\t */\n-\t\tif (tmp_objdir_migrate(transaction->objdir))\n+\t\tregister_pack_lockfiles(transaction);\n+\t\tret = tmp_objdir_migrate(transaction->objdir);\n+\t\tdisown_foreign_pack_lockfiles(transaction);\n+\t\tif (ret)\n \t\t\treturn error(_(\"unable to migrate temporary objects\"));\n \n \t\ttransaction->objdir = NULL;\n@@ -1393,10 +1441,10 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,\n \n \t\tif (xgethostname(hostname, sizeof(hostname)))\n \t\t\txsnprintf(hostname, sizeof(hostname), \"localhost\");\n-\t\tstrvec_pushf(&child.args,\n-\t\t\t     \"--keep=receive-pack %\"PRIuMAX\" on %s\",\n-\t\t\t     (uintmax_t)getpid(),\n-\t\t\t     hostname);\n+\t\tfree(transaction->keep_msg);\n+\t\ttransaction->keep_msg = xstrfmt(\"receive-pack %\"PRIuMAX\" on %s\",\n+\t\t\t\t\t\t(uintmax_t)getpid(), hostname);\n+\t\tstrvec_pushf(&child.args, \"--keep=%s\", transaction->keep_msg);\n \n \t\tif (!opts->quiet && err_fd)\n \t\t\tstrvec_push(&child.args, \"--show-resolving-progress\");\n@@ -1423,18 +1471,13 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,\n \t\t/*\n \t\t * The lockfile filepath is expected to be the final location of\n \t\t * the \".keep\" file after being migrated to the main ODB source.\n-\t\t * This ensures the lockfile can be found and removed later\n-\t\t * after the ODB transaction has been committed.\n+\t\t * We start tracking it right before that migration; see\n+\t\t * odb_transaction_files_commit().\n \t\t */\n \t\tlockfile = index_pack_lockfile(base->source, child.out, NULL);\n-\t\tif (lockfile) {\n-\t\t\tALLOC_GROW(transaction->pack_lockfiles,\n-\t\t\t\t   transaction->pack_lockfiles_nr + 1,\n-\t\t\t\t   transaction->pack_lockfiles_alloc);\n-\t\t\ttransaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =\n-\t\t\t\tregister_tempfile(lockfile);\n-\t\t\tfree(lockfile);\n-\t\t}\n+\t\tif (lockfile)\n+\t\t\tstring_list_append_nodup(&transaction->pack_lockfiles,\n+\t\t\t\t\t\t lockfile);\n \t\tclose(child.out);\n \n \t\tstatus = finish_command(&child);\n@@ -1454,12 +1497,21 @@ static int odb_transaction_files_finalize(struct odb_transaction *base)\n {\n \tstruct odb_transaction_files *transaction =\n \t\tcontainer_of(base, struct odb_transaction_files, base);\n+\tstruct string_list_item *item;\n \tint ret = 0;\n \n-\tfor (size_t i = 0; i < transaction->pack_lockfiles_nr; i++)\n-\t\tret |= delete_tempfile(&transaction->pack_lockfiles[i]);\n+\t/*\n+\t * Only the \".keep\" files that turned out to be ours still have a\n+\t * tempfile attached; delete_tempfile() does nothing for the rest.\n+\t */\n+\tfor_each_string_list_item(item, &transaction->pack_lockfiles) {\n+\t\tstruct tempfile *lockfile = item->util;\n+\n+\t\tret |= delete_tempfile(&lockfile);\n+\t}\n \n-\tfree(transaction->pack_lockfiles);\n+\tstring_list_clear(&transaction->pack_lockfiles, 0);\n+\tFREE_AND_NULL(transaction->keep_msg);\n \n \treturn ret;\n }\n@@ -1492,6 +1544,7 @@ int odb_transaction_files_begin(struct odb_source *source,\n \ttransaction->base.write_pack = odb_transaction_files_write_pack;\n \ttransaction->base.env = odb_transaction_files_env;\n \ttransaction->flags = flags;\n+\tstring_list_init_dup(&transaction->pack_lockfiles);\n \n \ttransaction->prefix = \"bulk-fsync\";\n \tif (flags & ODB_TRANSACTION_RECEIVE) {\ndiff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh\nindex 1b7097179e..8623d2d6c1 100755\n--- a/t/t5547-push-quarantine.sh\n+++ b/t/t5547-push-quarantine.sh\n@@ -101,4 +101,56 @@ test_expect_success '.keep file is removed after push' '\n \ttest_path_is_missing \"$keep\"\n '\n \n+test_expect_success 'a rejected push does not remove a foreign \".keep\"' '\n+\ttest_when_finished rm -rf foreign.git &&\n+\tgit init --bare foreign.git &&\n+\tgit -C foreign.git config set receive.unpackLimit 0 &&\n+\n+\t# Get a packfile into the main object database without updating any\n+\t# ref, so that pushing the same objects again reuses its name.\n+\ttest_hook -C foreign.git update <<-\\EOF &&\n+\texit 1\n+\tEOF\n+\ttest_commit foreign &&\n+\ttest_must_fail git push foreign.git HEAD:refs/heads/one &&\n+\n+\tpack=\"$(ls foreign.git/objects/pack/pack-*.pack)\" &&\n+\tkeep=\"${pack%.pack}.keep\" &&\n+\n+\t# Pretend somebody else holds the lock on that packfile, and let the\n+\t# next push be rejected before its objects are ever migrated.\n+\t>\"$keep\" &&\n+\ttest_hook -C foreign.git pre-receive <<-\\EOF &&\n+\texit 1\n+\tEOF\n+\ttest_must_fail git push foreign.git HEAD:refs/heads/two &&\n+\ttest_path_is_file \"$keep\"\n+'\n+\n+test_expect_success 'a \".keep\" installed by a failed migration is removed' '\n+\ttest_when_finished rm -rf partial.git &&\n+\tgit init --bare partial.git &&\n+\tgit -C partial.git config set receive.unpackLimit 0 &&\n+\tgit -C partial.git config set pack.indexVersion 1 &&\n+\n+\t# Leave the objects in the main object database without a ref, so\n+\t# that pushing them again produces a pack with the same name.\n+\ttest_hook -C partial.git update <<-\\EOF &&\n+\texit 1\n+\tEOF\n+\ttest_commit partial &&\n+\ttest_must_fail git push partial.git HEAD:refs/heads/one &&\n+\n+\t# The same pack now arrives with a differently formatted index. The\n+\t# \".keep\" is migrated first and goes in fine; the index then collides\n+\t# with the one already there, and the migration fails with our\n+\t# \".keep\" already installed.\n+\tgit -C partial.git config set pack.indexVersion 2 &&\n+\ttest_must_fail git push partial.git HEAD:refs/heads/two 2>err &&\n+\ttest_grep \"unable to migrate\" err &&\n+\n+\tpack=\"$(ls partial.git/objects/pack/pack-*.pack)\" &&\n+\ttest_path_is_missing \"${pack%.pack}.keep\"\n+'\n+\n test_done\ndiff --git a/tempfile.c b/tempfile.c\nindex dc9ca4e645..10db4fbc7f 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -373,6 +373,18 @@ int delete_tempfile(struct tempfile **tempfile_p)\n \treturn err ? -1 : 0;\n }\n \n+void unregister_tempfile(struct tempfile **tempfile_p)\n+{\n+\tstruct tempfile *tempfile = *tempfile_p;\n+\n+\tif (!is_tempfile_active(tempfile))\n+\t\treturn;\n+\n+\tclose_tempfile_gently(tempfile);\n+\tdeactivate_tempfile(tempfile);\n+\t*tempfile_p = NULL;\n+}\n+\n void reassign_tempfile_ownership(pid_t from, pid_t to)\n {\n \tvolatile struct volatile_list_head *pos;\ndiff --git a/tempfile.h b/tempfile.h\nindex f571f3c609..b439066a30 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -275,6 +275,15 @@ int reopen_tempfile(struct tempfile *tempfile);\n  */\n int delete_tempfile(struct tempfile **tempfile_p);\n \n+/*\n+ * Stop tracking `tempfile` without removing the file: close the file\n+ * descriptor and/or file pointer if they are still open, and leave the\n+ * file where it is, no longer to be removed at exit or on a signal. It\n+ * is a NOOP to call `unregister_tempfile()` for a `tempfile` object\n+ * that is not currently active.\n+ */\n+void unregister_tempfile(struct tempfile **tempfile_p);\n+\n /*\n  * Close the file descriptor and/or file pointer if they are still\n  * open, and atomically rename the temporary file to `path`. `path`\n-- \ngitgitgadget\n\n"},{"id":"552695","messageId":"9349ea48b09347eff5da8a8862268d63605af690.1789385483.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.git.1789385483.gitgitgadget@gmail.com","subject":"[PATCH 2/6] pack-objects: keep --keep-pack open when following","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-14T11:31:19Z","receivedAt":"2026-09-14T11:31:29Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\n\"--stdin-packs=follow\" distinguishes excluded packs that are closed\nunder reachability (\"^\") from those that are not (\"!\"). The traversal\nstops at objects in the former, and goes on through the latter to\nrescue whatever they depend on that would otherwise be left out.\n\nA pack named with \"--keep-pack\" gets the same in-core flag as a \"^\"\npack, so the traversal stops at it too. Nothing warrants that: the\ncaller said not to repack it, not that it is self-contained. When it\nholds a commit but not that commit's tree, the tree is never rescued,\nand writing a bitmap over the result fails for lack of closure.\n\nIn follow mode, mark such a pack as kept-open instead, the way repack\nalready lists the packs it cannot vouch for as \"!\" on stdin. Its\nobjects stay out of the result, and the traversal can go through it.\n\nThis matters more once repack names its \".keep\" packs this way instead\nof passing \"--honor-pack-keep\": on-disk kept packs never were a\nboundary, and they should not become one.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n builtin/pack-objects.c        | 20 +++++++++++++----\n t/t5331-pack-objects-stdin.sh | 41 +++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 708b719f40..6f579173b0 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4999,7 +4999,8 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)\n \toid_array_clear(&recent_objects);\n }\n \n-static void add_extra_kept_packs(const struct string_list *names)\n+static void add_extra_kept_packs(const struct string_list *names,\n+\t\t\t\t enum stdin_packs_mode stdin_packs)\n {\n \tstruct packed_git *p;\n \n@@ -5018,8 +5019,19 @@ static void add_extra_kept_packs(const struct string_list *names)\n \t\t\t\tbreak;\n \n \t\tif (i < names->nr) {\n-\t\t\tp->pack_keep_in_core = 1;\n-\t\t\tignore_packed_keep_in_core = 1;\n+\t\t\t/*\n+\t\t\t * When following, treat the pack like a \"!\" pack, not\n+\t\t\t * a \"^\" one: nobody said it is closed under\n+\t\t\t * reachability, so the traversal must be able to go\n+\t\t\t * through it.\n+\t\t\t */\n+\t\t\tif (stdin_packs == STDIN_PACKS_MODE_FOLLOW) {\n+\t\t\t\tp->pack_keep_in_core_open = 1;\n+\t\t\t\tignore_packed_keep_in_core_open = 1;\n+\t\t\t} else {\n+\t\t\t\tp->pack_keep_in_core = 1;\n+\t\t\t\tignore_packed_keep_in_core = 1;\n+\t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t}\n@@ -5443,7 +5455,7 @@ int cmd_pack_objects(int argc,\n \tif (progress && all_progress_implied)\n \t\tprogress = 2;\n \n-\tadd_extra_kept_packs(&keep_pack_list);\n+\tadd_extra_kept_packs(&keep_pack_list, stdin_packs);\n \tif (ignore_packed_keep_on_disk) {\n \t\tstruct packed_git *p;\n \ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex c74b5861af..4e1fde1b08 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -483,6 +483,47 @@ test_expect_success '--stdin-packs=follow with open-excluded packs' '\n \t)\n '\n \n+test_expect_success '--stdin-packs=follow walks through a --keep-pack pack' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set maintenance.auto false &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB_ONLY=\"$(git rev-parse B | git pack-objects $packdir/pack)\" &&\n+\t\tgit prune-packed &&\n+\n+\t\t# Pack C is included and pack A is excluded and closed. The\n+\t\t# commit B is in the kept pack B_ONLY, but its tree and blob\n+\t\t# are only in pack B, which pack-objects is not told about.\n+\t\t# The kept pack keeps B out of the result, and the walk has\n+\t\t# to go through it to rescue the tree and the blob.\n+\t\tP=$(git pack-objects --stdin-packs=follow \\\n+\t\t\t--keep-pack=pack-$B_ONLY.pack $packdir/pack <<-EOF\n+\t\tpack-$C.pack\n+\t\t^pack-$A.pack\n+\t\tEOF\n+\t\t) &&\n+\n+\t\t{\n+\t\t\tobjects_in_packs $C &&\n+\t\t\tgit rev-parse \"B^{tree}\" B:B.t\n+\t\t} >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\tobjects_in_packs $P >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success '--stdin-packs with !-delimited pack without follow' '\n \ttest_when_finished \"rm -fr repo\" &&\n \n-- \ngitgitgadget\n\n"},{"id":"552696","messageId":"a1b85c0a2579d93c399269b8f32671d13443a580.1789385483.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.git.1789385483.gitgitgadget@gmail.com","subject":"[PATCH 3/6] pack-objects: reset kept-pack cache for cruft walk","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-14T11:31:20Z","receivedAt":"2026-09-14T11:31:31Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\nWhen writing a cruft pack with an expiration, pack-objects first\ncollects the recent objects and then walks from them to rescue\nwhatever they reach, expired or not. A pack the caller did not list is\nmarked kept while collecting, so that its objects are not copied into\nthe cruft pack, and unmarked before the walk, so that the walk can go\nthrough it.\n\nThe walk does not see the unmarking. Whether an object sits in a kept\npack is answered from a cache that is built on first use and only\ndropped when asked about a different kind of kept pack. Collecting\nbuilds it while the unlisted pack is still marked, the walk asks the\nsame kind of question, and so the unlisted pack stays in it: the walk\nstops there, and whatever lies beyond it in an expired pack is lost.\n\nThis went unnoticed because of \"--honor-pack-keep\". repack passes it,\nand when there is a \".keep\" file it makes the collecting side ask\nabout on-disk and in-core kept packs together while the walk asks\nabout in-core ones alone; the cache is rebuilt each time the question\nchanges, and by accident the walk sees the current marks. Take the\n\".keep\" file away and the objects are lost today. A later commit stops\nrepack from passing \"--honor-pack-keep\" at all, so fix this first.\n\nExpose the invalidation packfile.c already has and call it after\nre-marking. The test builds an unreachable chain whose middle commit\nsits in a pack pack-objects is not told about and whose oldest objects\nhave expired; without the fix the cruft pack holds only the recent tip.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n builtin/pack-objects.c        |  8 +++++++\n odb/source-packed.h           |  3 ++-\n packfile.c                    |  9 ++++++--\n packfile.h                    |  7 ++++++\n t/t5329-pack-objects-cruft.sh | 40 +++++++++++++++++++++++++++++++++++\n 5 files changed, 64 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 6f579173b0..8ca8255176 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4274,6 +4274,7 @@ static void enumerate_cruft_objects(void)\n static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)\n {\n \tstruct packed_git *p;\n+\tstruct odb_source *source;\n \tstruct rev_info revs;\n \tint ret;\n \n@@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs\n \t/*\n \t * Re-mark only the fresh packs as kept so that objects in\n \t * unknown packs do not halt the reachability traversal early.\n+\t * The kept-pack cache was built while those packs were still\n+\t * marked, so drop it too.\n \t */\n \trepo_for_each_pack(the_repository, p)\n \t\tp->pack_keep_in_core = 0;\n \tmark_pack_kept_in_core(fresh_packs, 1);\n+\tfor (source = the_repository->objects->sources; source;\n+\t     source = source->next) {\n+\t\tstruct odb_source_files *files = odb_source_files_downcast(source);\n+\t\tpackfile_store_invalidate_kept_pack_cache(files->packed);\n+\t}\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\ndiff --git a/odb/source-packed.h b/odb/source-packed.h\nindex a0f6b5096d..9e42311916 100644\n--- a/odb/source-packed.h\n+++ b/odb/source-packed.h\n@@ -25,7 +25,8 @@ struct odb_source_packed {\n \t * Should not be accessed directly, but via\n \t * `packfile_store_get_kept_pack_cache()`. The list of packs gets\n \t * invalidated when the stored flags and the flags passed to\n-\t * `packfile_store_get_kept_pack_cache()` mismatch.\n+\t * `packfile_store_get_kept_pack_cache()` mismatch, or explicitly via\n+\t * `packfile_store_invalidate_kept_pack_cache()`.\n \t */\n \tstruct {\n \t\tstruct packed_git **packs;\ndiff --git a/packfile.c b/packfile.c\nindex 4fa5fd67c8..90459ec4d7 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1870,6 +1870,12 @@ int packfile_fill_entry(struct packed_git *p,\n \treturn 1;\n }\n \n+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store)\n+{\n+\tFREE_AND_NULL(store->kept_cache.packs);\n+\tstore->kept_cache.flags = 0;\n+}\n+\n static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,\n \t\t\t\t\t     unsigned flags)\n {\n@@ -1877,8 +1883,7 @@ static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,\n \t\treturn;\n \tif (store->kept_cache.flags == flags)\n \t\treturn;\n-\tFREE_AND_NULL(store->kept_cache.packs);\n-\tstore->kept_cache.flags = 0;\n+\tpackfile_store_invalidate_kept_pack_cache(store);\n }\n \n struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,\ndiff --git a/packfile.h b/packfile.h\nindex 6d30d15a00..493faf0010 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -144,6 +144,13 @@ enum kept_pack_type {\n struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,\n \t\t\t\t\t\t       unsigned flags);\n \n+/*\n+ * Drop the cache of kept packs so that the next call to\n+ * `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after changing\n+ * which packs are kept in core.\n+ */\n+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store);\n+\n struct pack_window {\n \tstruct pack_window *next;\n \tunsigned char *base;\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 12cda06373..6302f60b75 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -332,6 +332,46 @@ test_expect_success 'cruft trees rescue sub-trees, blobs' '\n \t)\n '\n \n+test_expect_success 'cruft traversal rescues through a pack it was not told about' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit packed &&\n+\t\tgit repack -Ad &&\n+\t\tkeep=\"$(basename \"$(ls $packdir/pack-*.pack)\")\" &&\n+\n+\t\ttest_commit old &&\n+\t\ttest_commit mid &&\n+\t\ttest_commit new &&\n+\n+\t\t# \"old\" has expired, \"new\" is recent, and \"mid\" sits in a\n+\t\t# pack that pack-objects is not told about. Rescuing \"old\"\n+\t\t# from \"new\" means walking through that pack.\n+\t\tgit rev-list --objects --no-object-names packed..old >old &&\n+\t\twhile read object\n+\t\tdo\n+\t\t\ttest-tool chmtime -1000 \\\n+\t\t\t\t\"$objdir/$(test_oid_to_path $object)\" || exit 1\n+\t\tdone <old &&\n+\t\tgit rev-list --objects --no-object-names old..mid |\n+\t\tgit pack-objects $packdir/pack >/dev/null &&\n+\t\tgit prune-packed &&\n+\n+\t\tcruft=\"$(echo $keep | git pack-objects --cruft \\\n+\t\t\t--cruft-expiration=750.seconds.ago \\\n+\t\t\t$packdir/pack)\" &&\n+\t\ttest-tool pack-mtimes \"pack-$cruft.mtimes\" >actual.raw &&\n+\n+\t\tcut -d\" \" -f1 <actual.raw | sort >actual &&\n+\t\tgit rev-list --objects --no-object-names packed..new >expect.raw &&\n+\t\tsort <expect.raw >expect &&\n+\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'expired objects are pruned' '\n \tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n-- \ngitgitgadget\n\n"},{"id":"552697","messageId":"38070935dc479099375b89f76a2f3c1db52e6577.1789385483.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.git.1789385483.gitgitgadget@gmail.com","subject":"[PATCH 4/6] pack-objects: sort --keep-pack list for lookup","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-14T11:31:21Z","receivedAt":"2026-09-14T11:31:32Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\nadd_extra_kept_packs() scans the whole \"--keep-pack\" list once per\npack in the repository. That is fine for the handful of names it gets\ntoday, but the next commit lets a caller name every kept pack in the\nrepository, and with thousands of them the scan dominates: matching\n20,000 kept packs against 20,000 names takes 11 seconds here, against\nunder a second with \"--honor-pack-keep\".\n\nSort the list once and look each pack up in it. The comparison stays\nfspathcmp(), so what matches does not change.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n builtin/pack-objects.c | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8ca8255176..1fcb4ef8a5 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -5007,7 +5007,7 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)\n \toid_array_clear(&recent_objects);\n }\n \n-static void add_extra_kept_packs(const struct string_list *names,\n+static void add_extra_kept_packs(struct string_list *names,\n \t\t\t\t enum stdin_packs_mode stdin_packs)\n {\n \tstruct packed_git *p;\n@@ -5015,18 +5015,13 @@ static void add_extra_kept_packs(const struct string_list *names,\n \tif (!names->nr)\n \t\treturn;\n \n-\trepo_for_each_pack(the_repository, p) {\n-\t\tconst char *name = basename(p->pack_name);\n-\t\tint i;\n+\tstring_list_sort(names);\n \n+\trepo_for_each_pack(the_repository, p) {\n \t\tif (!p->pack_local)\n \t\t\tcontinue;\n \n-\t\tfor (i = 0; i < names->nr; i++)\n-\t\t\tif (!fspathcmp(name, names->items[i].string))\n-\t\t\t\tbreak;\n-\n-\t\tif (i < names->nr) {\n+\t\tif (string_list_has_string(names, basename(p->pack_name))) {\n \t\t\t/*\n \t\t\t * When following, treat the pack like a \"!\" pack, not\n \t\t\t * a \"^\" one: nobody said it is closed under\n@@ -5151,7 +5146,9 @@ int cmd_pack_objects(int argc,\n \tint rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;\n \tint rev_list_index = 0;\n \tenum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;\n-\tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n+\tstruct string_list keep_pack_list = {\n+\t\t.cmp = fspathcmp,\n+\t};\n \tstruct list_objects_filter_options filter_options =\n \t\tLIST_OBJECTS_FILTER_INIT;\n \tstruct repo_config_values *cfg = repo_config_values(the_repository);\n-- \ngitgitgadget\n\n"},{"id":"552698","messageId":"f8e27b7aacb969fa1162847606871f7df6748274.1789385483.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.git.1789385483.gitgitgadget@gmail.com","subject":"[PATCH 5/6] pack-objects: add --keep-pack-from-file","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-14T11:31:22Z","receivedAt":"2026-09-14T11:31:35Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\n\"--keep-pack\" names one pack per occurrence, and there is only so much\nroom on the command line: ARG_MAX is shared with the environment, and\non Windows the whole line is capped at 32,767 characters, which a few\nhundred pack names fill. Past that the spawn fails before pack-objects\nhas started. fetch-pack grew \"--stdin\" in 078b895fef (fetch-pack: new\n--stdin option to read refs from stdin, 2012-04-02) for the same\nreason.\n\nstdin is taken here: every mode repack drives pack-objects in already\nuses it, for the revision list under \"-a\", object names for the\npromisor pack, and pack lists for \"--stdin-packs\" and \"--cruft\". So\nread the names from a file instead, one per line, skipping empty\nlines. They go into the same list as the \"--keep-pack\" names and are\ntreated exactly alike: matched against local packs, ignored when they\nmatch nothing, and kept open under \"--stdin-packs=follow\". A relative\npath is resolved against the directory the user ran from, as\n\"--refs-snapshot\" of \"git multi-pack-index write\" is.\n\nThe list now holds strings from two sources, so let it own its copies.\n\nrepack is about to use this to hand pack-objects its own snapshot of\nthe packs that have a \".keep\" file.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n Documentation/git-pack-objects.adoc |  8 +++++\n builtin/pack-objects.c              | 28 ++++++++++++++++++\n t/t5331-pack-objects-stdin.sh       | 46 +++++++++++++++++++++++++++++\n 3 files changed, 82 insertions(+)\n\ndiff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc\nindex 65cd00c152..938e27f69d 100644\n--- a/Documentation/git-pack-objects.adoc\n+++ b/Documentation/git-pack-objects.adoc\n@@ -13,6 +13,7 @@ SYNOPSIS\n \t\t   [--no-reuse-delta] [--delta-base-offset] [--non-empty]\n \t\t   [--local] [--incremental] [--window=<n>] [--depth=<n>]\n \t\t   [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]\n+\t\t   [--keep-pack-from-file=<file>]\n \t\t   [--cruft] [--cruft-expiration=<time>]\n \t\t   [--stdout [--filter=<filter-spec>] | <base-name>]\n \t\t   [--shallow] [--keep-true-parents] [--[no-]sparse]\n@@ -193,6 +194,13 @@ depth is 4095.\n \tleading directory (e.g. `pack-123.pack`). The option could be\n \tspecified multiple times to keep multiple packs.\n \n+--keep-pack-from-file=<file>::\n+\tRead names of packs to keep from `<file>`, one per line, and\n+\ttreat each of them as if it had been given with `--keep-pack`.\n+\tEmpty lines are ignored. This is meant for callers such as\n+\tlinkgit:git-repack[1] that may have to name more packs than fit\n+\ton a command line.\n+\n --incremental::\n \tThis flag causes an object already in a pack to be ignored\n \teven if it would have otherwise been packed.\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1fcb4ef8a5..9f8c4b9135 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -194,6 +194,7 @@ static const char *const pack_usage[] = {\n \t   \"                 [--no-reuse-delta] [--delta-base-offset] [--non-empty]\\n\"\n \t   \"                 [--local] [--incremental] [--window=<n>] [--depth=<n>]\\n\"\n \t   \"                 [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]\\n\"\n+\t   \"                 [--keep-pack-from-file=<file>]\\n\"\n \t   \"                 [--cruft] [--cruft-expiration=<time>]\\n\"\n \t   \"                 [--stdout [--filter=<filter-spec>] | <base-name>]\\n\"\n \t   \"                 [--shallow] [--keep-true-parents] [--[no-]sparse]\\n\"\n@@ -5007,6 +5008,26 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)\n \toid_array_clear(&recent_objects);\n }\n \n+/*\n+ * Read pack names from the file, one per line, as if each of them had\n+ * been given with \"--keep-pack\".\n+ */\n+static void read_keep_pack_list(struct string_list *names, const char *path)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *fp = xfopen(path, \"r\");\n+\n+\twhile (strbuf_getline(&buf, fp) != EOF) {\n+\t\tif (!buf.len)\n+\t\t\tcontinue;\n+\t\tstring_list_append(names, buf.buf);\n+\t}\n+\tif (ferror(fp))\n+\t\tdie_errno(_(\"could not read '%s'\"), path);\n+\tfclose(fp);\n+\tstrbuf_release(&buf);\n+}\n+\n static void add_extra_kept_packs(struct string_list *names,\n \t\t\t\t enum stdin_packs_mode stdin_packs)\n {\n@@ -5147,8 +5168,10 @@ int cmd_pack_objects(int argc,\n \tint rev_list_index = 0;\n \tenum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;\n \tstruct string_list keep_pack_list = {\n+\t\t.strdup_strings = 1,\n \t\t.cmp = fspathcmp,\n \t};\n+\tchar *keep_pack_from_file = NULL;\n \tstruct list_objects_filter_options filter_options =\n \t\tLIST_OBJECTS_FILTER_INIT;\n \tstruct repo_config_values *cfg = repo_config_values(the_repository);\n@@ -5233,6 +5256,8 @@ int cmd_pack_objects(int argc,\n \t\t\t N_(\"ignore packs that have companion .keep file\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n \t\t\t\tN_(\"ignore this pack\")),\n+\t\tOPT_FILENAME(0, \"keep-pack-from-file\", &keep_pack_from_file,\n+\t\t\t     N_(\"ignore the packs named in <file>\")),\n \t\tOPT_INTEGER(0, \"compression\", &cfg->pack_compression_level,\n \t\t\t    N_(\"pack compression level\")),\n \t\tOPT_BOOL(0, \"keep-true-parents\", &grafts_keep_true_parents,\n@@ -5460,6 +5485,8 @@ int cmd_pack_objects(int argc,\n \tif (progress && all_progress_implied)\n \t\tprogress = 2;\n \n+\tif (keep_pack_from_file)\n+\t\tread_keep_pack_list(&keep_pack_list, keep_pack_from_file);\n \tadd_extra_kept_packs(&keep_pack_list, stdin_packs);\n \tif (ignore_packed_keep_on_disk) {\n \t\tstruct packed_git *p;\n@@ -5554,6 +5581,7 @@ cleanup:\n \tclear_packing_data(&to_pack);\n \tlist_objects_filter_release(&filter_options);\n \tstring_list_clear(&keep_pack_list, 0);\n+\tfree(keep_pack_from_file);\n \tstrvec_clear(&rp);\n \n \treturn 0;\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 4e1fde1b08..d590aa4dad 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -561,4 +561,50 @@ test_expect_success '--stdin-packs with !-delimited pack without follow' '\n \t)\n '\n \n+test_expect_success '--keep-pack-from-file names packs to keep' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set maintenance.auto false &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\t\tgit prune-packed &&\n+\n+\t\t# Empty lines and names that match no pack are ignored,\n+\t\t# as they would be with --keep-pack.\n+\t\tcat >keep <<-EOF &&\n+\t\tpack-$A.pack\n+\n+\t\tpack-$B.pack\n+\t\tpack-does-not-exist.pack\n+\t\tEOF\n+\n+\t\tP=$(git pack-objects --all --keep-pack=pack-$A.pack \\\n+\t\t\t--keep-pack=pack-$B.pack from-argv </dev/null) &&\n+\t\tpacked_objects from-argv-$P.idx >expect &&\n+\n+\t\tP=$(git pack-objects --all --keep-pack-from-file=keep \\\n+\t\t\tfrom-file </dev/null) &&\n+\t\tpacked_objects from-file-$P.idx >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tobjects_in_packs $C >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--keep-pack-from-file with a missing file' '\n+\ttest_must_fail git pack-objects --stdout \\\n+\t\t--keep-pack-from-file=does-not-exist </dev/null 2>err &&\n+\ttest_grep \"could not open .does-not-exist. for reading\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"552699","messageId":"a18e354e73ade433e3b9854f1662338488a5e672.1789385483.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.git.1789385483.gitgitgadget@gmail.com","subject":"[PATCH 6/6] repack: tell pack-objects which packs are kept","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-14T11:31:23Z","receivedAt":"2026-09-14T11:31:37Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\nrepack works out which packs are redundant by looking for \".keep\"\nfiles when it starts, then passes \"--honor-pack-keep\" to the\npack-objects it spawns, which looks for them all over again. Two scans\nof the same directory, seconds apart, with nothing holding them\ntogether.\n\nA \".keep\" that turns up in between loses objects. The parent did not\nsee it, so the pack is on its list to delete. The child does see it,\nso it leaves that pack's objects out of the replacement. The parent\ndeletes the pack regardless: repack_remove_redundant_pack() passes\nforce_delete, which skips the \".keep\" check in unlink_pack_path(). The\nobjects are gone and repack exits successfully.\n\nThe gap is easy to land in. index-pack writes its \".keep\" before it\nrenames the packfile into place, so a \"git fetch\" or a push being\nmigrated out of its quarantine will do it. Checking for the \".keep\"\nonce more right before deleting would not help: a push holds it for a\nfraction of a second, and it may well be gone again by the time\npack-objects has finished.\n\nHand pack-objects the kept packs we collected at startup and drop\n\"--honor-pack-keep\". Both processes then work from one snapshot, and a\n\".keep\" appearing or disappearing while we run cannot make them\ndisagree. An earlier commit made sure a pack kept this way is no more\nof a boundary to the traversal than a \".keep\" file was.\n\nThe list goes into a file next to the refs snapshot we already write\nfor \"git multi-pack-index write\", and is passed with\n\"--keep-pack-from-file\" to every pack-objects we spawn when\n\"--pack-kept-objects\" is not in effect, which is when\n\"--honor-pack-keep\" used to be. The cruft pack-objects already has the\nkept packs on its stdin; the file is redundant there, but it sees the\nsame list as everybody else. With nothing to keep, no file is written\nand nothing is passed, which is what \"--honor-pack-keep\" came down to\nwhen it found no \".keep\".\n\nThe names go one per line, so a name with a newline in it cannot be\npassed. \"--stdin-packs\" and \"--cruft\" have the same limit and die on a\nname they cannot find, but \"--keep-pack\" ignores such a name, and the\ntwo halves of a garbled one could go on to exclude some other pack;\nrefuse it up front instead.\n\nThe user's own \"--keep-pack\" arguments keep being forwarded, since\nthey apply either way. write_filtered_pack() had a loop passing the\nkept packs too, but without the \".pack\" suffix pack-objects compares\nagainst; it goes. Kept packs borrowed from an alternate object\ndirectory were covered by \"--honor-pack-keep\" and are not by the\nsnapshot, which only ever held local packs; repack never deletes\nthose, so their objects now get packed rather than skipped, which\ncosts room but cannot lose anything.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n builtin/repack.c            | 15 ++++++++\n repack-filtered.c           |  3 --\n repack.c                    | 34 ++++++++++++++++--\n repack.h                    | 17 +++++++--\n t/t7700-repack.sh           | 43 ++++++++++++++++++++++\n t/t7703-repack-geometric.sh | 72 +++++++++++++++++++++++++++++++++++++\n 6 files changed, 177 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c4360382c1..78bc98c4f1 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -167,6 +167,7 @@ int cmd_repack(int argc,\n \tstruct oidset drop_oids = OIDSET_INIT;\n \tstruct pack_geometry geometry = { 0 };\n \tstruct tempfile *refs_snapshot = NULL;\n+\tstruct tempfile *kept_packs_snapshot = NULL;\n \tint i, ret;\n \tint show_progress;\n \n@@ -456,6 +457,19 @@ int cmd_repack(int argc,\n \n \texisting.repo = repo;\n \texisting_packs_collect(&existing, &keep_pack_list);\n+\tif (existing.kept_packs.nr) {\n+\t\tstruct strbuf path = STRBUF_INIT;\n+\n+\t\tstrbuf_addf(&path, \"%s/%s_XXXXXX\",\n+\t\t\t    repo_get_object_directory(repo), \"kept-packs\");\n+\n+\t\tkept_packs_snapshot = xmks_tempfile(path.buf);\n+\t\texisting_packs_snapshot_kept(&existing, kept_packs_snapshot);\n+\t\tpo_args.kept_packs_snapshot =\n+\t\t\tget_tempfile_path(kept_packs_snapshot);\n+\n+\t\tstrbuf_release(&path);\n+\t}\n \n \tif (geometry.split_factor) {\n \t\tif (pack_everything)\n@@ -644,6 +658,7 @@ int cmd_repack(int argc,\n \t\tcruft_po_args.quiet = po_args.quiet;\n \t\tcruft_po_args.delta_base_offset = po_args.delta_base_offset;\n \t\tcruft_po_args.pack_kept_objects = 0;\n+\t\tcruft_po_args.kept_packs_snapshot = po_args.kept_packs_snapshot;\n \n \t\tret = write_cruft_pack(&opts, cruft_expiration,\n \t\t\t\t       combine_cruft_below_size, &names,\ndiff --git a/repack-filtered.c b/repack-filtered.c\nindex 869b9fc6e3..db8de9f633 100644\n--- a/repack-filtered.c\n+++ b/repack-filtered.c\n@@ -25,9 +25,6 @@ int write_filtered_pack(const struct write_pack_opts *opts,\n \n \tstrvec_push(&cmd.args, \"--stdin-packs\");\n \n-\tfor_each_string_list_item(item, &existing->kept_packs)\n-\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\", item->string);\n-\n \tcmd.in = -1;\n \n \tret = start_command(&cmd);\ndiff --git a/repack.c b/repack.c\nindex d2aa58e134..a794486035 100644\n--- a/repack.c\n+++ b/repack.c\n@@ -38,8 +38,9 @@ void prepare_pack_objects(struct child_process *cmd,\n \t\tstrvec_push(&cmd->args,  \"--quiet\");\n \tif (args->delta_base_offset)\n \t\tstrvec_push(&cmd->args,  \"--delta-base-offset\");\n-\tif (!args->pack_kept_objects)\n-\t\tstrvec_push(&cmd->args,  \"--honor-pack-keep\");\n+\tif (!args->pack_kept_objects && args->kept_packs_snapshot)\n+\t\tstrvec_pushf(&cmd->args, \"--keep-pack-from-file=%s\",\n+\t\t\t     args->kept_packs_snapshot);\n \tstrvec_push(&cmd->args, out);\n \tcmd->git_cmd = 1;\n \tcmd->out = -1;\n@@ -167,6 +168,35 @@ void existing_packs_collect(struct existing_packs *existing,\n \tstrbuf_release(&buf);\n }\n \n+void existing_packs_snapshot_kept(const struct existing_packs *existing,\n+\t\t\t\t  struct tempfile *f)\n+{\n+\tstruct string_list_item *item;\n+\tFILE *out = fdopen_tempfile(f, \"w\");\n+\n+\tif (!out)\n+\t\tdie(_(\"could not open tempfile %s for writing\"),\n+\t\t    get_tempfile_path(f));\n+\n+\tfor_each_string_list_item(item, &existing->kept_packs) {\n+\t\t/*\n+\t\t * A newline would split the name in two, and pack-objects\n+\t\t * quietly keeps whichever packs the halves happen to name.\n+\t\t */\n+\t\tif (strchr(item->string, '\\n'))\n+\t\t\tdie(_(\"cannot keep pack '%s': its name contains a newline\"),\n+\t\t\t    item->string);\n+\t\tfprintf(out, \"%s.pack\\n\", item->string);\n+\t}\n+\n+\tif (close_tempfile_gently(f)) {\n+\t\tint save_errno = errno;\n+\t\tdelete_tempfile(&f);\n+\t\terrno = save_errno;\n+\t\tdie_errno(_(\"could not close kept packs snapshot tempfile\"));\n+\t}\n+}\n+\n int existing_packs_has_non_kept(const struct existing_packs *existing)\n {\n \treturn existing->non_kept_packs.nr || existing->cruft_packs.nr;\ndiff --git a/repack.h b/repack.h\nindex 61e554e4ed..1c0aeca3e8 100644\n--- a/repack.h\n+++ b/repack.h\n@@ -19,6 +19,14 @@ struct pack_objects_args {\n \tint path_walk;\n \tint delta_base_offset;\n \tint pack_kept_objects;\n+\t/*\n+\t * File naming the packs to leave alone, one \"<name>.pack\" per line;\n+\t * NULL when there are none. pack-objects reads it rather than\n+\t * looking for \".keep\" files itself, so that a \".keep\" created or\n+\t * removed while we run cannot make the two of us disagree over\n+\t * which packs are being repacked.\n+\t */\n+\tconst char *kept_packs_snapshot;\n \tstruct list_objects_filter_options filter_options;\n };\n \n@@ -28,6 +36,7 @@ struct pack_objects_args {\n }\n \n struct child_process;\n+struct tempfile;\n \n void prepare_pack_objects(struct child_process *cmd,\n \t\t\t  const struct pack_objects_args *args,\n@@ -79,6 +88,12 @@ struct existing_packs {\n  */\n void existing_packs_collect(struct existing_packs *existing,\n \t\t\t    const struct string_list *extra_keep);\n+/*\n+ * Writes the names of the kept packs, one \"<name>.pack\" per line, into\n+ * the given tempfile, for pack-objects to read with --keep-pack-from-file.\n+ */\n+void existing_packs_snapshot_kept(const struct existing_packs *existing,\n+\t\t\t\t  struct tempfile *f);\n int existing_packs_has_non_kept(const struct existing_packs *existing);\n int existing_pack_is_marked_for_deletion(struct string_list_item *item);\n void existing_packs_retain_cruft(struct existing_packs *existing,\n@@ -138,8 +153,6 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,\n \t\t\t\t    bool wrote_incremental_midx);\n void pack_geometry_release(struct pack_geometry *geometry);\n \n-struct tempfile;\n-\n enum repack_write_midx_mode {\n \tREPACK_WRITE_MIDX_NONE,\n \tREPACK_WRITE_MIDX_DEFAULT,\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex f0a390e3c6..845f032bea 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -254,6 +254,49 @@ test_expect_success 'repack --keep-pack' '\n \t)\n '\n \n+test_expect_success 'repack --keep-pack with --pack-kept-objects' '\n+\ttest_create_repo keep-pack-kept-objects &&\n+\t(\n+\t\tcd keep-pack-kept-objects &&\n+\t\tgit config pack.window 0 &&\n+\t\tgit config maintenance.auto false &&\n+\t\tP1=$(commit_and_pack 1) &&\n+\t\tP2=$(commit_and_pack 2) &&\n+\n+\t\t# \"--pack-kept-objects\" is about packs that have a \".keep\"\n+\t\t# file. A pack named with \"--keep-pack\" stays out of the\n+\t\t# result regardless, objects included.\n+\t\tgit repack -a -d --pack-kept-objects --keep-pack $P1 &&\n+\t\tls .git/objects/pack/*.pack >counts &&\n+\t\ttest_line_count = 2 counts &&\n+\t\ttest-tool find-pack -c 1 HEAD~1 &&\n+\t\ttest-tool find-pack -c 1 HEAD~1: &&\n+\t\tgit fsck\n+\t)\n+'\n+\n+test_expect_success FUNNYNAMES 'a kept pack whose name has a newline is refused' '\n+\ttest_create_repo keep-pack-newline &&\n+\t(\n+\t\tcd keep-pack-newline &&\n+\t\tgit config maintenance.auto false &&\n+\t\ttest_commit base &&\n+\t\tgit repack -ad &&\n+\n+\t\t# The names pack-objects is told to keep go one per line, so\n+\t\t# this one would come out as two, and the first of them is\n+\t\t# the name of the pack holding everything else.\n+\t\tvictim=\"$(basename \"$(ls .git/objects/pack/pack-*.pack)\")\" &&\n+\t\tname=\"$(printf \"%s\\nother\" \"$victim\")\" &&\n+\t\tP=$(git rev-parse HEAD | git pack-objects \".git/objects/pack/$name\") &&\n+\t\t>\".git/objects/pack/$name-$P.keep\" &&\n+\n+\t\ttest_must_fail git repack -ad 2>err &&\n+\t\ttest_grep \"contains a newline\" err &&\n+\t\tgit fsck\n+\t)\n+'\n+\n test_expect_success 'repacking fails when missing .pack actually means missing objects' '\n \ttest_create_repo idx-without-pack &&\n \t(\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex f3a0650cfe..6b914a2a80 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -541,4 +541,76 @@ test_expect_success 'geometric repack works with promisor packs' '\n \t)\n '\n \n+test_expect_success 'a \".keep\" that shows up mid-repack does not lose objects' '\n+\ttest_when_finished \"rm -fr race\" &&\n+\tgit init race &&\n+\t(\n+\t\tcd race &&\n+\n+\t\ttest_commit kept &&\n+\t\ttest_commit pack &&\n+\n+\t\tKEPT=$(git pack-objects --revs $packdir/pack <<-EOF\n+\t\trefs/tags/kept\n+\t\tEOF\n+\t\t) &&\n+\t\tgit pack-objects --revs $packdir/pack <<-EOF &&\n+\t\trefs/tags/pack\n+\t\t^refs/tags/kept\n+\t\tEOF\n+\t\tgit prune-packed &&\n+\n+\t\t# Neither pack is twice the size of the other, so both are\n+\t\t# redundant and get deleted. Have a \".keep\" appear on one of\n+\t\t# them as pack-objects starts, after the repack has decided\n+\t\t# to delete it: pack-objects used to notice the \".keep\" and\n+\t\t# leave those objects out of the replacement pack.\n+\t\tmkdir shim &&\n+\t\twrite_script shim/git <<-EOF &&\n+\t\ttest \"\\$1\" = \"pack-objects\" && >\"$(pwd)/$packdir/pack-$KEPT.keep\"\n+\t\tGIT_EXEC_PATH=\"$GIT_EXEC_PATH\" exec \"$GIT_EXEC_PATH/git\" \"\\$@\"\n+\t\tEOF\n+\n+\t\tgit --exec-path=\"$(pwd)/shim\" repack --geometric 2 -d &&\n+\n+\t\tgit fsck\n+\t)\n+'\n+\n+test_expect_success 'a kept pack does not stop the traversal from rescuing objects' '\n+\ttest_when_finished \"rm -fr kept-open\" &&\n+\tgit init kept-open &&\n+\t(\n+\t\tcd kept-open &&\n+\t\tgit config repack.midxMustContainCruft false &&\n+\n+\t\ttest_commit a &&\n+\t\ttest_commit b &&\n+\t\tb=$(git rev-parse b) &&\n+\t\tgit repack -ad &&\n+\n+\t\t# Make \"b\" unreachable and sweep it, together with its tree\n+\t\t# and blob, into a cruft pack.\n+\t\tgit tag -d b &&\n+\t\tgit reset --hard a &&\n+\t\tgit reflog expire --all --expire=all &&\n+\t\tgit repack -ad --cruft &&\n+\n+\t\t# Bring the commit back on its own, in a pack marked as kept.\n+\t\t# Its tree and blob are still only in the cruft pack.\n+\t\tkept=$(echo $b | git pack-objects $packdir/pack) &&\n+\t\t>$packdir/pack-$kept.keep &&\n+\n+\t\t# Build on top of it, so that the repack has to look through\n+\t\t# the kept pack to find out what the new commit depends on.\n+\t\tgit update-ref refs/heads/master \\\n+\t\t\t$(git commit-tree a^{tree} -p $b -m c) &&\n+\n+\t\tgit repack --geometric 2 -d --write-midx --write-bitmap-index &&\n+\t\ttest_path_is_file $packdir/multi-pack-index &&\n+\t\tls $packdir/multi-pack-index-*.bitmap >bitmaps &&\n+\t\ttest_line_count = 1 bitmaps\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"552764","messageId":"aql8Wt2q9RnQpjEC@jtobler--20250820-SHC54","threadId":"66327","inReplyTo":"932e8e425aecfbd33c1e5caf66c80a0226abacba.1789385483.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/6] odb: don't remove a \".keep\" we never installed","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-09-15T18:25:19Z","receivedAt":"2026-09-15T18:25:27Z","isPatch":true,"body":"On 26/09/14 11:31AM, Qin ShiCheng via GitGitGadget wrote:\n>From: Qin ShiCheng <qeesung@live.com>\n>\n>receive-pack runs index-pack with \"--keep\" over the quarantine, which\n>writes a \"pack-XXX.keep\" there. The path we register as a tempfile is\n>a different one: where that \".keep\" will land once the quarantine is\n>migrated into the main object database.\n\nYup, when the \".keep\" file gets registered as a tempfile, it needs to\nknow where it will eventually be located post-migration. That way it can\nbe deleted after the references have been updated or if the process\nexits early. This is a bit awkward, but its a result of use relying on\ngit-index-pack(1) to create the \".keep\" file for us and it gets written\nto the quaratine directory.\n\n>Nothing of ours is at that path yet, and something else may be. Two\n>pushes of identical content produce identical thin packs, index-pack\n>names a pack after its contents, and so both want the same \".keep\" in\n>the main object database. If the other push still holds it, that file\n>is what keeps its pack from being repacked away, and we remove it at\n>exit regardless -- even when pre-receive rejected our push and nothing\n>was migrated at all.\n\nInteresting, for a pair of identical concurrent pushes, if one exits\nearly it could end of deleting the other processes packfile out from\nunder it. Really the process should probably only delete a \".keep\" file\nthat itself created.\n\nSomething worth noting, if there are two concurrent identical pushes,\nboth will generate the same \".keep\", but the keep message contained will\ndiffer. In such cases, when the quarantined files are migrated to the\nODB, the \".keep\" file that gets migrated first \"wins\" and the other push\nwill fail because the competing \".keep\" fails the collision check and\nconsequently the push fails. I mention this because the current behavior\nfor how Git handles concurrent identical pushes is to reject one of\nthem. So if a process encounters an already existing \".keep\" file in the\nmain ODB, it may be sufficient to abort early anyways.\n\n>Register the path right before the migration instead, and once the\n>migration has returned, read the files back. index-pack wrote the\n>message we handed it; a file that says something else was not written\n>for us, so let go of it without removing it. tempfile gains\n>unregister_tempfile() for that.\n\nRight, registering the temporary \".keep\" files doesn't really need to\nhappen prior to the ODB transaction commit anyways. In fact, we could go\na step further and stop using git-index-pack(1) to prematurely create\n\".keep\" files altogether in favor of letting the commit phase of the ODB\ntransaction create it explicitly. This has a couple of benefits:\n\n\t- It avoids the already awkward tracking of \".keep\" files in ODB\n\t  transaction pre-commit.\n\t- It would also make fixing the issue in question a bit easier\n\t  by allowing us to simply try to create the \".keep\" file and if\n\t  it already exists, unregister the tempfile and abort early.\n\nCompletely unrelated to this bug as part of another series I'm working\non locally, I've already have some patches that start creating \".keep\"\nfiles explicitly during the ODB commit phase in the \"files\" backend. I\nwould be happy to pick these patches out and send them upstream with\nsome small adjustments to also fix the issue here in your first patch.\nJust let me know what you would perfer. :)\n\nThanks,\n-Justin\n"},{"id":"552780","messageId":"PH0PR84MB2999B0A3F2D64F46FC62E659DDB92@PH0PR84MB2999.NAMPRD84.PROD.OUTLOOK.COM","threadId":"66327","inReplyTo":"aql8Wt2q9RnQpjEC@jtobler--20250820-SHC54","subject":"Re: [PATCH 1/6] odb: don't remove a \".keep\" we never installed","fromName":"Qin ShiCheng","fromEmail":"qeesung@live.com","sentAt":"2026-09-16T06:01:14Z","receivedAt":"2026-09-16T06:01:24Z","isPatch":true,"body":"On 26/09/15 01:25PM, Justin Tobler wrote:\n> Something worth noting, if there are two concurrent identical pushes,\n> both will generate the same \".keep\", but the keep message contained will\n> differ. In such cases, when the quarantined files are migrated to the\n> ODB, the \".keep\" file that gets migrated first \"wins\" and the other push\n> will fail because the competing \".keep\" fails the collision check and\n> consequently the push fails. I mention this because the current behavior\n> for how Git handles concurrent identical pushes is to reject one of\n> them. So if a process encounters an already existing \".keep\" file in the\n> main ODB, it may be sufficient to abort early anyways.\n\nAgreed, aborting is fine there. The two pushes that bit us were seconds\napart rather than concurrent: the first had already finished and removed\nits \".keep\", so the second found nothing at that path and linked its own\n\".keep\" onto the first push's pack. What the patch is after is narrower\nthan handling the collision: whatever happens on the way out, we should\nonly remove a \".keep\" we created ourselves. Today finalize unlinks the\npath unconditionally, even when pre-receive rejected the push and\nnothing was migrated at all, which is what the first t5547 test pins.\n\n> Right, registering the temporary \".keep\" files doesn't really need to\n> happen prior to the ODB transaction commit anyways. In fact, we could go\n> a step further and stop using git-index-pack(1) to prematurely create\n> \".keep\" files altogether in favor of letting the commit phase of the ODB\n> transaction create it explicitly.\n[...]\n> Completely unrelated to this bug as part of another series I'm working\n> on locally, I've already have some patches that start creating \".keep\"\n> files explicitly during the ODB commit phase in the \"files\" backend. I\n> would be happy to pick these patches out and send them upstream with\n> some small adjustments to also fix the issue here in your first patch.\n> Just let me know what you would perfer. :)\n\nThat is the better shape for it, please do. Creating the file at commit\ntime and registering it right there gives the two guarantees this patch\ngets by reading the files back: nothing foreign is removed, and a signal\nafter our \".keep\" is in place still cleans it up. (v1 registers before\nthe migration for the latter: the \".keep\" is migrated first, and for a\nduplicate of a large pack the migration then spends a while in\ncheck_collision().) One case worth keeping in mind is a migration that\nfails partway with our \".keep\" already installed, e.g. the \".idx\"\ncolliding because pack.indexVersion changed between the two pushes; the\nsecond t5547 test covers that.\n\nI will drop 1/6 from v2 so the series is only the repack side; 2/6-6/6\ndo not depend on it. Feel free to take the two t5547 tests if they are\nof use to your series.\n\nThanks,\nQin\n"},{"id":"552842","messageId":"pull.2219.v2.git.1789700615.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.git.1789385483.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] repack: don't lose objects to a \".keep\" that appears mid-run","fromName":"qeesung via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T03:03:30Z","receivedAt":"2026-09-18T03:03:41Z","isPatch":true,"body":"A concurrent push or fetch can make \"git repack -d\" delete a pack whose\nobjects were never copied anywhere, and exit 0. We hit this in production: a\nref pointing at a commit that no longer exists, on git 2.43, and it\nreproduces on master.\n\nWhat happens:\n\n * repack scans for \".keep\" files and decides which packs to delete, then\n   spawns pack-objects with --honor-pack-keep, which scans again;\n * in between, an index-pack --keep finishes -- a push migrating its\n   quarantine, or a fetch -- and installs a \".keep\" next to a pack that\n   repack has already decided to delete;\n * pack-objects sees that \".keep\" and leaves the pack's objects out; repack\n   deletes the pack by its earlier list, with force_delete.\n\nThe fix is to stop the two processes from scanning separately: hand\npack-objects the snapshot repack took at startup (5/5). Patches 1-4 are what\n5/5 needs to be safe:\n\n * 1/5: under --stdin-packs=follow, a --keep-pack pack stops the traversal\n   like a \"^\" pack; on-disk \".keep\" packs never did.\n * 2/5: the cruft walk goes by a stale kept-pack cache, which\n   --honor-pack-keep happened to mask. Pre-existing, reproducible today.\n * 3/5: look --keep-pack names up in a sorted list; it gets long.\n * 4/5: --keep-pack-from-file, since a repository can have more kept packs\n   than fit on a command line (32K characters on Windows).\n\nEvery fix comes with a test that fails without it; the race itself is\nreproduced in t7703 by having a \".keep\" appear as pack-objects starts. The\nfull suite passes, and the series merges cleanly into next and seen.\n\nChanges since v1:\n\n * Dropped 1/6 (odb: don't remove a \".keep\" we never installed). Justin\n   Tobler is going to fix the receive-pack side properly, by having the ODB\n   transaction create the \".keep\" itself at commit time rather than reading\n   back what index-pack wrote:\n   https://lore.kernel.org/git/aql8Wt2q9RnQpjEC@jtobler--20250820-SHC54/\n * The remaining patches are unchanged apart from renumbering.\n\nQin ShiCheng (5):\n  pack-objects: keep --keep-pack open when following\n  pack-objects: reset kept-pack cache for cruft walk\n  pack-objects: sort --keep-pack list for lookup\n  pack-objects: add --keep-pack-from-file\n  repack: tell pack-objects which packs are kept\n\n Documentation/git-pack-objects.adoc |  8 +++\n builtin/pack-objects.c              | 71 ++++++++++++++++++-----\n builtin/repack.c                    | 15 +++++\n odb/source-packed.h                 |  3 +-\n packfile.c                          |  9 ++-\n packfile.h                          |  7 +++\n repack-filtered.c                   |  3 -\n repack.c                            | 34 ++++++++++-\n repack.h                            | 17 +++++-\n t/t5329-pack-objects-cruft.sh       | 40 +++++++++++++\n t/t5331-pack-objects-stdin.sh       | 87 +++++++++++++++++++++++++++++\n t/t7700-repack.sh                   | 43 ++++++++++++++\n t/t7703-repack-geometric.sh         | 72 ++++++++++++++++++++++++\n 13 files changed, 386 insertions(+), 23 deletions(-)\n\n\nbase-commit: 3cb9185f65410273787f74333cc027d2ea5daada\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2219%2Fqeesung%2Frepack-kept-packs-snapshot-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2219/qeesung/repack-kept-packs-snapshot-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/2219\n\nRange-diff vs v1:\n\n 1:  932e8e425a < -:  ---------- odb: don't remove a \".keep\" we never installed\n 2:  9349ea48b0 = 1:  8cf72312c5 pack-objects: keep --keep-pack open when following\n 3:  a1b85c0a25 = 2:  77aec8941f pack-objects: reset kept-pack cache for cruft walk\n 4:  38070935dc = 3:  b76e06a467 pack-objects: sort --keep-pack list for lookup\n 5:  f8e27b7aac = 4:  20a051cfb6 pack-objects: add --keep-pack-from-file\n 6:  a18e354e73 = 5:  4684fd8552 repack: tell pack-objects which packs are kept\n\n-- \ngitgitgadget\n"},{"id":"552843","messageId":"8cf72312c5e0c2a11166a7024c521c53d9b7a1ec.1789700615.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.v2.git.1789700615.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] pack-objects: keep --keep-pack open when following","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T03:03:31Z","receivedAt":"2026-09-18T03:03:41Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\n\"--stdin-packs=follow\" distinguishes excluded packs that are closed\nunder reachability (\"^\") from those that are not (\"!\"). The traversal\nstops at objects in the former, and goes on through the latter to\nrescue whatever they depend on that would otherwise be left out.\n\nA pack named with \"--keep-pack\" gets the same in-core flag as a \"^\"\npack, so the traversal stops at it too. Nothing warrants that: the\ncaller said not to repack it, not that it is self-contained. When it\nholds a commit but not that commit's tree, the tree is never rescued,\nand writing a bitmap over the result fails for lack of closure.\n\nIn follow mode, mark such a pack as kept-open instead, the way repack\nalready lists the packs it cannot vouch for as \"!\" on stdin. Its\nobjects stay out of the result, and the traversal can go through it.\n\nThis matters more once repack names its \".keep\" packs this way instead\nof passing \"--honor-pack-keep\": on-disk kept packs never were a\nboundary, and they should not become one.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n builtin/pack-objects.c        | 20 +++++++++++++----\n t/t5331-pack-objects-stdin.sh | 41 +++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 708b719f40..6f579173b0 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4999,7 +4999,8 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)\n \toid_array_clear(&recent_objects);\n }\n \n-static void add_extra_kept_packs(const struct string_list *names)\n+static void add_extra_kept_packs(const struct string_list *names,\n+\t\t\t\t enum stdin_packs_mode stdin_packs)\n {\n \tstruct packed_git *p;\n \n@@ -5018,8 +5019,19 @@ static void add_extra_kept_packs(const struct string_list *names)\n \t\t\t\tbreak;\n \n \t\tif (i < names->nr) {\n-\t\t\tp->pack_keep_in_core = 1;\n-\t\t\tignore_packed_keep_in_core = 1;\n+\t\t\t/*\n+\t\t\t * When following, treat the pack like a \"!\" pack, not\n+\t\t\t * a \"^\" one: nobody said it is closed under\n+\t\t\t * reachability, so the traversal must be able to go\n+\t\t\t * through it.\n+\t\t\t */\n+\t\t\tif (stdin_packs == STDIN_PACKS_MODE_FOLLOW) {\n+\t\t\t\tp->pack_keep_in_core_open = 1;\n+\t\t\t\tignore_packed_keep_in_core_open = 1;\n+\t\t\t} else {\n+\t\t\t\tp->pack_keep_in_core = 1;\n+\t\t\t\tignore_packed_keep_in_core = 1;\n+\t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t}\n@@ -5443,7 +5455,7 @@ int cmd_pack_objects(int argc,\n \tif (progress && all_progress_implied)\n \t\tprogress = 2;\n \n-\tadd_extra_kept_packs(&keep_pack_list);\n+\tadd_extra_kept_packs(&keep_pack_list, stdin_packs);\n \tif (ignore_packed_keep_on_disk) {\n \t\tstruct packed_git *p;\n \ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex c74b5861af..4e1fde1b08 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -483,6 +483,47 @@ test_expect_success '--stdin-packs=follow with open-excluded packs' '\n \t)\n '\n \n+test_expect_success '--stdin-packs=follow walks through a --keep-pack pack' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set maintenance.auto false &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB_ONLY=\"$(git rev-parse B | git pack-objects $packdir/pack)\" &&\n+\t\tgit prune-packed &&\n+\n+\t\t# Pack C is included and pack A is excluded and closed. The\n+\t\t# commit B is in the kept pack B_ONLY, but its tree and blob\n+\t\t# are only in pack B, which pack-objects is not told about.\n+\t\t# The kept pack keeps B out of the result, and the walk has\n+\t\t# to go through it to rescue the tree and the blob.\n+\t\tP=$(git pack-objects --stdin-packs=follow \\\n+\t\t\t--keep-pack=pack-$B_ONLY.pack $packdir/pack <<-EOF\n+\t\tpack-$C.pack\n+\t\t^pack-$A.pack\n+\t\tEOF\n+\t\t) &&\n+\n+\t\t{\n+\t\t\tobjects_in_packs $C &&\n+\t\t\tgit rev-parse \"B^{tree}\" B:B.t\n+\t\t} >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\tobjects_in_packs $P >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success '--stdin-packs with !-delimited pack without follow' '\n \ttest_when_finished \"rm -fr repo\" &&\n \n-- \ngitgitgadget\n\n"},{"id":"552844","messageId":"77aec8941f5d17654f58956c7c643b47dd5a8d93.1789700615.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.v2.git.1789700615.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T03:03:32Z","receivedAt":"2026-09-18T03:03:41Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\nWhen writing a cruft pack with an expiration, pack-objects first\ncollects the recent objects and then walks from them to rescue\nwhatever they reach, expired or not. A pack the caller did not list is\nmarked kept while collecting, so that its objects are not copied into\nthe cruft pack, and unmarked before the walk, so that the walk can go\nthrough it.\n\nThe walk does not see the unmarking. Whether an object sits in a kept\npack is answered from a cache that is built on first use and only\ndropped when asked about a different kind of kept pack. Collecting\nbuilds it while the unlisted pack is still marked, the walk asks the\nsame kind of question, and so the unlisted pack stays in it: the walk\nstops there, and whatever lies beyond it in an expired pack is lost.\n\nThis went unnoticed because of \"--honor-pack-keep\". repack passes it,\nand when there is a \".keep\" file it makes the collecting side ask\nabout on-disk and in-core kept packs together while the walk asks\nabout in-core ones alone; the cache is rebuilt each time the question\nchanges, and by accident the walk sees the current marks. Take the\n\".keep\" file away and the objects are lost today. A later commit stops\nrepack from passing \"--honor-pack-keep\" at all, so fix this first.\n\nExpose the invalidation packfile.c already has and call it after\nre-marking. The test builds an unreachable chain whose middle commit\nsits in a pack pack-objects is not told about and whose oldest objects\nhave expired; without the fix the cruft pack holds only the recent tip.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n builtin/pack-objects.c        |  8 +++++++\n odb/source-packed.h           |  3 ++-\n packfile.c                    |  9 ++++++--\n packfile.h                    |  7 ++++++\n t/t5329-pack-objects-cruft.sh | 40 +++++++++++++++++++++++++++++++++++\n 5 files changed, 64 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 6f579173b0..8ca8255176 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4274,6 +4274,7 @@ static void enumerate_cruft_objects(void)\n static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)\n {\n \tstruct packed_git *p;\n+\tstruct odb_source *source;\n \tstruct rev_info revs;\n \tint ret;\n \n@@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs\n \t/*\n \t * Re-mark only the fresh packs as kept so that objects in\n \t * unknown packs do not halt the reachability traversal early.\n+\t * The kept-pack cache was built while those packs were still\n+\t * marked, so drop it too.\n \t */\n \trepo_for_each_pack(the_repository, p)\n \t\tp->pack_keep_in_core = 0;\n \tmark_pack_kept_in_core(fresh_packs, 1);\n+\tfor (source = the_repository->objects->sources; source;\n+\t     source = source->next) {\n+\t\tstruct odb_source_files *files = odb_source_files_downcast(source);\n+\t\tpackfile_store_invalidate_kept_pack_cache(files->packed);\n+\t}\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\ndiff --git a/odb/source-packed.h b/odb/source-packed.h\nindex a0f6b5096d..9e42311916 100644\n--- a/odb/source-packed.h\n+++ b/odb/source-packed.h\n@@ -25,7 +25,8 @@ struct odb_source_packed {\n \t * Should not be accessed directly, but via\n \t * `packfile_store_get_kept_pack_cache()`. The list of packs gets\n \t * invalidated when the stored flags and the flags passed to\n-\t * `packfile_store_get_kept_pack_cache()` mismatch.\n+\t * `packfile_store_get_kept_pack_cache()` mismatch, or explicitly via\n+\t * `packfile_store_invalidate_kept_pack_cache()`.\n \t */\n \tstruct {\n \t\tstruct packed_git **packs;\ndiff --git a/packfile.c b/packfile.c\nindex 4fa5fd67c8..90459ec4d7 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1870,6 +1870,12 @@ int packfile_fill_entry(struct packed_git *p,\n \treturn 1;\n }\n \n+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store)\n+{\n+\tFREE_AND_NULL(store->kept_cache.packs);\n+\tstore->kept_cache.flags = 0;\n+}\n+\n static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,\n \t\t\t\t\t     unsigned flags)\n {\n@@ -1877,8 +1883,7 @@ static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,\n \t\treturn;\n \tif (store->kept_cache.flags == flags)\n \t\treturn;\n-\tFREE_AND_NULL(store->kept_cache.packs);\n-\tstore->kept_cache.flags = 0;\n+\tpackfile_store_invalidate_kept_pack_cache(store);\n }\n \n struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,\ndiff --git a/packfile.h b/packfile.h\nindex 6d30d15a00..493faf0010 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -144,6 +144,13 @@ enum kept_pack_type {\n struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,\n \t\t\t\t\t\t       unsigned flags);\n \n+/*\n+ * Drop the cache of kept packs so that the next call to\n+ * `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after changing\n+ * which packs are kept in core.\n+ */\n+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store);\n+\n struct pack_window {\n \tstruct pack_window *next;\n \tunsigned char *base;\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 12cda06373..6302f60b75 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -332,6 +332,46 @@ test_expect_success 'cruft trees rescue sub-trees, blobs' '\n \t)\n '\n \n+test_expect_success 'cruft traversal rescues through a pack it was not told about' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit packed &&\n+\t\tgit repack -Ad &&\n+\t\tkeep=\"$(basename \"$(ls $packdir/pack-*.pack)\")\" &&\n+\n+\t\ttest_commit old &&\n+\t\ttest_commit mid &&\n+\t\ttest_commit new &&\n+\n+\t\t# \"old\" has expired, \"new\" is recent, and \"mid\" sits in a\n+\t\t# pack that pack-objects is not told about. Rescuing \"old\"\n+\t\t# from \"new\" means walking through that pack.\n+\t\tgit rev-list --objects --no-object-names packed..old >old &&\n+\t\twhile read object\n+\t\tdo\n+\t\t\ttest-tool chmtime -1000 \\\n+\t\t\t\t\"$objdir/$(test_oid_to_path $object)\" || exit 1\n+\t\tdone <old &&\n+\t\tgit rev-list --objects --no-object-names old..mid |\n+\t\tgit pack-objects $packdir/pack >/dev/null &&\n+\t\tgit prune-packed &&\n+\n+\t\tcruft=\"$(echo $keep | git pack-objects --cruft \\\n+\t\t\t--cruft-expiration=750.seconds.ago \\\n+\t\t\t$packdir/pack)\" &&\n+\t\ttest-tool pack-mtimes \"pack-$cruft.mtimes\" >actual.raw &&\n+\n+\t\tcut -d\" \" -f1 <actual.raw | sort >actual &&\n+\t\tgit rev-list --objects --no-object-names packed..new >expect.raw &&\n+\t\tsort <expect.raw >expect &&\n+\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'expired objects are pruned' '\n \tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n-- \ngitgitgadget\n\n"},{"id":"552845","messageId":"20a051cfb6fb23b6cdb7adf6cf53a2b8eb2f4391.1789700616.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.v2.git.1789700615.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] pack-objects: add --keep-pack-from-file","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T03:03:34Z","receivedAt":"2026-09-18T03:03:43Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\n\"--keep-pack\" names one pack per occurrence, and there is only so much\nroom on the command line: ARG_MAX is shared with the environment, and\non Windows the whole line is capped at 32,767 characters, which a few\nhundred pack names fill. Past that the spawn fails before pack-objects\nhas started. fetch-pack grew \"--stdin\" in 078b895fef (fetch-pack: new\n--stdin option to read refs from stdin, 2012-04-02) for the same\nreason.\n\nstdin is taken here: every mode repack drives pack-objects in already\nuses it, for the revision list under \"-a\", object names for the\npromisor pack, and pack lists for \"--stdin-packs\" and \"--cruft\". So\nread the names from a file instead, one per line, skipping empty\nlines. They go into the same list as the \"--keep-pack\" names and are\ntreated exactly alike: matched against local packs, ignored when they\nmatch nothing, and kept open under \"--stdin-packs=follow\". A relative\npath is resolved against the directory the user ran from, as\n\"--refs-snapshot\" of \"git multi-pack-index write\" is.\n\nThe list now holds strings from two sources, so let it own its copies.\n\nrepack is about to use this to hand pack-objects its own snapshot of\nthe packs that have a \".keep\" file.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n Documentation/git-pack-objects.adoc |  8 +++++\n builtin/pack-objects.c              | 28 ++++++++++++++++++\n t/t5331-pack-objects-stdin.sh       | 46 +++++++++++++++++++++++++++++\n 3 files changed, 82 insertions(+)\n\ndiff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc\nindex 65cd00c152..938e27f69d 100644\n--- a/Documentation/git-pack-objects.adoc\n+++ b/Documentation/git-pack-objects.adoc\n@@ -13,6 +13,7 @@ SYNOPSIS\n \t\t   [--no-reuse-delta] [--delta-base-offset] [--non-empty]\n \t\t   [--local] [--incremental] [--window=<n>] [--depth=<n>]\n \t\t   [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]\n+\t\t   [--keep-pack-from-file=<file>]\n \t\t   [--cruft] [--cruft-expiration=<time>]\n \t\t   [--stdout [--filter=<filter-spec>] | <base-name>]\n \t\t   [--shallow] [--keep-true-parents] [--[no-]sparse]\n@@ -193,6 +194,13 @@ depth is 4095.\n \tleading directory (e.g. `pack-123.pack`). The option could be\n \tspecified multiple times to keep multiple packs.\n \n+--keep-pack-from-file=<file>::\n+\tRead names of packs to keep from `<file>`, one per line, and\n+\ttreat each of them as if it had been given with `--keep-pack`.\n+\tEmpty lines are ignored. This is meant for callers such as\n+\tlinkgit:git-repack[1] that may have to name more packs than fit\n+\ton a command line.\n+\n --incremental::\n \tThis flag causes an object already in a pack to be ignored\n \teven if it would have otherwise been packed.\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1fcb4ef8a5..9f8c4b9135 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -194,6 +194,7 @@ static const char *const pack_usage[] = {\n \t   \"                 [--no-reuse-delta] [--delta-base-offset] [--non-empty]\\n\"\n \t   \"                 [--local] [--incremental] [--window=<n>] [--depth=<n>]\\n\"\n \t   \"                 [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]\\n\"\n+\t   \"                 [--keep-pack-from-file=<file>]\\n\"\n \t   \"                 [--cruft] [--cruft-expiration=<time>]\\n\"\n \t   \"                 [--stdout [--filter=<filter-spec>] | <base-name>]\\n\"\n \t   \"                 [--shallow] [--keep-true-parents] [--[no-]sparse]\\n\"\n@@ -5007,6 +5008,26 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)\n \toid_array_clear(&recent_objects);\n }\n \n+/*\n+ * Read pack names from the file, one per line, as if each of them had\n+ * been given with \"--keep-pack\".\n+ */\n+static void read_keep_pack_list(struct string_list *names, const char *path)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *fp = xfopen(path, \"r\");\n+\n+\twhile (strbuf_getline(&buf, fp) != EOF) {\n+\t\tif (!buf.len)\n+\t\t\tcontinue;\n+\t\tstring_list_append(names, buf.buf);\n+\t}\n+\tif (ferror(fp))\n+\t\tdie_errno(_(\"could not read '%s'\"), path);\n+\tfclose(fp);\n+\tstrbuf_release(&buf);\n+}\n+\n static void add_extra_kept_packs(struct string_list *names,\n \t\t\t\t enum stdin_packs_mode stdin_packs)\n {\n@@ -5147,8 +5168,10 @@ int cmd_pack_objects(int argc,\n \tint rev_list_index = 0;\n \tenum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;\n \tstruct string_list keep_pack_list = {\n+\t\t.strdup_strings = 1,\n \t\t.cmp = fspathcmp,\n \t};\n+\tchar *keep_pack_from_file = NULL;\n \tstruct list_objects_filter_options filter_options =\n \t\tLIST_OBJECTS_FILTER_INIT;\n \tstruct repo_config_values *cfg = repo_config_values(the_repository);\n@@ -5233,6 +5256,8 @@ int cmd_pack_objects(int argc,\n \t\t\t N_(\"ignore packs that have companion .keep file\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n \t\t\t\tN_(\"ignore this pack\")),\n+\t\tOPT_FILENAME(0, \"keep-pack-from-file\", &keep_pack_from_file,\n+\t\t\t     N_(\"ignore the packs named in <file>\")),\n \t\tOPT_INTEGER(0, \"compression\", &cfg->pack_compression_level,\n \t\t\t    N_(\"pack compression level\")),\n \t\tOPT_BOOL(0, \"keep-true-parents\", &grafts_keep_true_parents,\n@@ -5460,6 +5485,8 @@ int cmd_pack_objects(int argc,\n \tif (progress && all_progress_implied)\n \t\tprogress = 2;\n \n+\tif (keep_pack_from_file)\n+\t\tread_keep_pack_list(&keep_pack_list, keep_pack_from_file);\n \tadd_extra_kept_packs(&keep_pack_list, stdin_packs);\n \tif (ignore_packed_keep_on_disk) {\n \t\tstruct packed_git *p;\n@@ -5554,6 +5581,7 @@ cleanup:\n \tclear_packing_data(&to_pack);\n \tlist_objects_filter_release(&filter_options);\n \tstring_list_clear(&keep_pack_list, 0);\n+\tfree(keep_pack_from_file);\n \tstrvec_clear(&rp);\n \n \treturn 0;\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 4e1fde1b08..d590aa4dad 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -561,4 +561,50 @@ test_expect_success '--stdin-packs with !-delimited pack without follow' '\n \t)\n '\n \n+test_expect_success '--keep-pack-from-file names packs to keep' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set maintenance.auto false &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tA=\"$(echo A | git pack-objects --revs $packdir/pack)\" &&\n+\t\tB=\"$(echo A..B | git pack-objects --revs $packdir/pack)\" &&\n+\t\tC=\"$(echo B..C | git pack-objects --revs $packdir/pack)\" &&\n+\t\tgit prune-packed &&\n+\n+\t\t# Empty lines and names that match no pack are ignored,\n+\t\t# as they would be with --keep-pack.\n+\t\tcat >keep <<-EOF &&\n+\t\tpack-$A.pack\n+\n+\t\tpack-$B.pack\n+\t\tpack-does-not-exist.pack\n+\t\tEOF\n+\n+\t\tP=$(git pack-objects --all --keep-pack=pack-$A.pack \\\n+\t\t\t--keep-pack=pack-$B.pack from-argv </dev/null) &&\n+\t\tpacked_objects from-argv-$P.idx >expect &&\n+\n+\t\tP=$(git pack-objects --all --keep-pack-from-file=keep \\\n+\t\t\tfrom-file </dev/null) &&\n+\t\tpacked_objects from-file-$P.idx >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tobjects_in_packs $C >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--keep-pack-from-file with a missing file' '\n+\ttest_must_fail git pack-objects --stdout \\\n+\t\t--keep-pack-from-file=does-not-exist </dev/null 2>err &&\n+\ttest_grep \"could not open .does-not-exist. for reading\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"552846","messageId":"b76e06a4672ed7881fb4ccfd389d5282a6f312b7.1789700616.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.v2.git.1789700615.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] pack-objects: sort --keep-pack list for lookup","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T03:03:33Z","receivedAt":"2026-09-18T03:03:43Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\nadd_extra_kept_packs() scans the whole \"--keep-pack\" list once per\npack in the repository. That is fine for the handful of names it gets\ntoday, but the next commit lets a caller name every kept pack in the\nrepository, and with thousands of them the scan dominates: matching\n20,000 kept packs against 20,000 names takes 11 seconds here, against\nunder a second with \"--honor-pack-keep\".\n\nSort the list once and look each pack up in it. The comparison stays\nfspathcmp(), so what matches does not change.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n builtin/pack-objects.c | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8ca8255176..1fcb4ef8a5 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -5007,7 +5007,7 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)\n \toid_array_clear(&recent_objects);\n }\n \n-static void add_extra_kept_packs(const struct string_list *names,\n+static void add_extra_kept_packs(struct string_list *names,\n \t\t\t\t enum stdin_packs_mode stdin_packs)\n {\n \tstruct packed_git *p;\n@@ -5015,18 +5015,13 @@ static void add_extra_kept_packs(const struct string_list *names,\n \tif (!names->nr)\n \t\treturn;\n \n-\trepo_for_each_pack(the_repository, p) {\n-\t\tconst char *name = basename(p->pack_name);\n-\t\tint i;\n+\tstring_list_sort(names);\n \n+\trepo_for_each_pack(the_repository, p) {\n \t\tif (!p->pack_local)\n \t\t\tcontinue;\n \n-\t\tfor (i = 0; i < names->nr; i++)\n-\t\t\tif (!fspathcmp(name, names->items[i].string))\n-\t\t\t\tbreak;\n-\n-\t\tif (i < names->nr) {\n+\t\tif (string_list_has_string(names, basename(p->pack_name))) {\n \t\t\t/*\n \t\t\t * When following, treat the pack like a \"!\" pack, not\n \t\t\t * a \"^\" one: nobody said it is closed under\n@@ -5151,7 +5146,9 @@ int cmd_pack_objects(int argc,\n \tint rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;\n \tint rev_list_index = 0;\n \tenum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;\n-\tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n+\tstruct string_list keep_pack_list = {\n+\t\t.cmp = fspathcmp,\n+\t};\n \tstruct list_objects_filter_options filter_options =\n \t\tLIST_OBJECTS_FILTER_INIT;\n \tstruct repo_config_values *cfg = repo_config_values(the_repository);\n-- \ngitgitgadget\n\n"},{"id":"552847","messageId":"4684fd8552f1bbcdb3cee00f1714730d924c2cce.1789700616.git.gitgitgadget@gmail.com","threadId":"66327","inReplyTo":"pull.2219.v2.git.1789700615.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] repack: tell pack-objects which packs are kept","fromName":"Qin ShiCheng via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-18T03:03:35Z","receivedAt":"2026-09-18T03:03:46Z","isPatch":true,"body":"From: Qin ShiCheng <qeesung@live.com>\n\nrepack works out which packs are redundant by looking for \".keep\"\nfiles when it starts, then passes \"--honor-pack-keep\" to the\npack-objects it spawns, which looks for them all over again. Two scans\nof the same directory, seconds apart, with nothing holding them\ntogether.\n\nA \".keep\" that turns up in between loses objects. The parent did not\nsee it, so the pack is on its list to delete. The child does see it,\nso it leaves that pack's objects out of the replacement. The parent\ndeletes the pack regardless: repack_remove_redundant_pack() passes\nforce_delete, which skips the \".keep\" check in unlink_pack_path(). The\nobjects are gone and repack exits successfully.\n\nThe gap is easy to land in. index-pack writes its \".keep\" before it\nrenames the packfile into place, so a \"git fetch\" or a push being\nmigrated out of its quarantine will do it. Checking for the \".keep\"\nonce more right before deleting would not help: a push holds it for a\nfraction of a second, and it may well be gone again by the time\npack-objects has finished.\n\nHand pack-objects the kept packs we collected at startup and drop\n\"--honor-pack-keep\". Both processes then work from one snapshot, and a\n\".keep\" appearing or disappearing while we run cannot make them\ndisagree. An earlier commit made sure a pack kept this way is no more\nof a boundary to the traversal than a \".keep\" file was.\n\nThe list goes into a file next to the refs snapshot we already write\nfor \"git multi-pack-index write\", and is passed with\n\"--keep-pack-from-file\" to every pack-objects we spawn when\n\"--pack-kept-objects\" is not in effect, which is when\n\"--honor-pack-keep\" used to be. The cruft pack-objects already has the\nkept packs on its stdin; the file is redundant there, but it sees the\nsame list as everybody else. With nothing to keep, no file is written\nand nothing is passed, which is what \"--honor-pack-keep\" came down to\nwhen it found no \".keep\".\n\nThe names go one per line, so a name with a newline in it cannot be\npassed. \"--stdin-packs\" and \"--cruft\" have the same limit and die on a\nname they cannot find, but \"--keep-pack\" ignores such a name, and the\ntwo halves of a garbled one could go on to exclude some other pack;\nrefuse it up front instead.\n\nThe user's own \"--keep-pack\" arguments keep being forwarded, since\nthey apply either way. write_filtered_pack() had a loop passing the\nkept packs too, but without the \".pack\" suffix pack-objects compares\nagainst; it goes. Kept packs borrowed from an alternate object\ndirectory were covered by \"--honor-pack-keep\" and are not by the\nsnapshot, which only ever held local packs; repack never deletes\nthose, so their objects now get packed rather than skipped, which\ncosts room but cannot lose anything.\n\nSigned-off-by: Qin ShiCheng <qeesung@live.com>\n---\n builtin/repack.c            | 15 ++++++++\n repack-filtered.c           |  3 --\n repack.c                    | 34 ++++++++++++++++--\n repack.h                    | 17 +++++++--\n t/t7700-repack.sh           | 43 ++++++++++++++++++++++\n t/t7703-repack-geometric.sh | 72 +++++++++++++++++++++++++++++++++++++\n 6 files changed, 177 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c4360382c1..78bc98c4f1 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -167,6 +167,7 @@ int cmd_repack(int argc,\n \tstruct oidset drop_oids = OIDSET_INIT;\n \tstruct pack_geometry geometry = { 0 };\n \tstruct tempfile *refs_snapshot = NULL;\n+\tstruct tempfile *kept_packs_snapshot = NULL;\n \tint i, ret;\n \tint show_progress;\n \n@@ -456,6 +457,19 @@ int cmd_repack(int argc,\n \n \texisting.repo = repo;\n \texisting_packs_collect(&existing, &keep_pack_list);\n+\tif (existing.kept_packs.nr) {\n+\t\tstruct strbuf path = STRBUF_INIT;\n+\n+\t\tstrbuf_addf(&path, \"%s/%s_XXXXXX\",\n+\t\t\t    repo_get_object_directory(repo), \"kept-packs\");\n+\n+\t\tkept_packs_snapshot = xmks_tempfile(path.buf);\n+\t\texisting_packs_snapshot_kept(&existing, kept_packs_snapshot);\n+\t\tpo_args.kept_packs_snapshot =\n+\t\t\tget_tempfile_path(kept_packs_snapshot);\n+\n+\t\tstrbuf_release(&path);\n+\t}\n \n \tif (geometry.split_factor) {\n \t\tif (pack_everything)\n@@ -644,6 +658,7 @@ int cmd_repack(int argc,\n \t\tcruft_po_args.quiet = po_args.quiet;\n \t\tcruft_po_args.delta_base_offset = po_args.delta_base_offset;\n \t\tcruft_po_args.pack_kept_objects = 0;\n+\t\tcruft_po_args.kept_packs_snapshot = po_args.kept_packs_snapshot;\n \n \t\tret = write_cruft_pack(&opts, cruft_expiration,\n \t\t\t\t       combine_cruft_below_size, &names,\ndiff --git a/repack-filtered.c b/repack-filtered.c\nindex 869b9fc6e3..db8de9f633 100644\n--- a/repack-filtered.c\n+++ b/repack-filtered.c\n@@ -25,9 +25,6 @@ int write_filtered_pack(const struct write_pack_opts *opts,\n \n \tstrvec_push(&cmd.args, \"--stdin-packs\");\n \n-\tfor_each_string_list_item(item, &existing->kept_packs)\n-\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\", item->string);\n-\n \tcmd.in = -1;\n \n \tret = start_command(&cmd);\ndiff --git a/repack.c b/repack.c\nindex d2aa58e134..a794486035 100644\n--- a/repack.c\n+++ b/repack.c\n@@ -38,8 +38,9 @@ void prepare_pack_objects(struct child_process *cmd,\n \t\tstrvec_push(&cmd->args,  \"--quiet\");\n \tif (args->delta_base_offset)\n \t\tstrvec_push(&cmd->args,  \"--delta-base-offset\");\n-\tif (!args->pack_kept_objects)\n-\t\tstrvec_push(&cmd->args,  \"--honor-pack-keep\");\n+\tif (!args->pack_kept_objects && args->kept_packs_snapshot)\n+\t\tstrvec_pushf(&cmd->args, \"--keep-pack-from-file=%s\",\n+\t\t\t     args->kept_packs_snapshot);\n \tstrvec_push(&cmd->args, out);\n \tcmd->git_cmd = 1;\n \tcmd->out = -1;\n@@ -167,6 +168,35 @@ void existing_packs_collect(struct existing_packs *existing,\n \tstrbuf_release(&buf);\n }\n \n+void existing_packs_snapshot_kept(const struct existing_packs *existing,\n+\t\t\t\t  struct tempfile *f)\n+{\n+\tstruct string_list_item *item;\n+\tFILE *out = fdopen_tempfile(f, \"w\");\n+\n+\tif (!out)\n+\t\tdie(_(\"could not open tempfile %s for writing\"),\n+\t\t    get_tempfile_path(f));\n+\n+\tfor_each_string_list_item(item, &existing->kept_packs) {\n+\t\t/*\n+\t\t * A newline would split the name in two, and pack-objects\n+\t\t * quietly keeps whichever packs the halves happen to name.\n+\t\t */\n+\t\tif (strchr(item->string, '\\n'))\n+\t\t\tdie(_(\"cannot keep pack '%s': its name contains a newline\"),\n+\t\t\t    item->string);\n+\t\tfprintf(out, \"%s.pack\\n\", item->string);\n+\t}\n+\n+\tif (close_tempfile_gently(f)) {\n+\t\tint save_errno = errno;\n+\t\tdelete_tempfile(&f);\n+\t\terrno = save_errno;\n+\t\tdie_errno(_(\"could not close kept packs snapshot tempfile\"));\n+\t}\n+}\n+\n int existing_packs_has_non_kept(const struct existing_packs *existing)\n {\n \treturn existing->non_kept_packs.nr || existing->cruft_packs.nr;\ndiff --git a/repack.h b/repack.h\nindex 61e554e4ed..1c0aeca3e8 100644\n--- a/repack.h\n+++ b/repack.h\n@@ -19,6 +19,14 @@ struct pack_objects_args {\n \tint path_walk;\n \tint delta_base_offset;\n \tint pack_kept_objects;\n+\t/*\n+\t * File naming the packs to leave alone, one \"<name>.pack\" per line;\n+\t * NULL when there are none. pack-objects reads it rather than\n+\t * looking for \".keep\" files itself, so that a \".keep\" created or\n+\t * removed while we run cannot make the two of us disagree over\n+\t * which packs are being repacked.\n+\t */\n+\tconst char *kept_packs_snapshot;\n \tstruct list_objects_filter_options filter_options;\n };\n \n@@ -28,6 +36,7 @@ struct pack_objects_args {\n }\n \n struct child_process;\n+struct tempfile;\n \n void prepare_pack_objects(struct child_process *cmd,\n \t\t\t  const struct pack_objects_args *args,\n@@ -79,6 +88,12 @@ struct existing_packs {\n  */\n void existing_packs_collect(struct existing_packs *existing,\n \t\t\t    const struct string_list *extra_keep);\n+/*\n+ * Writes the names of the kept packs, one \"<name>.pack\" per line, into\n+ * the given tempfile, for pack-objects to read with --keep-pack-from-file.\n+ */\n+void existing_packs_snapshot_kept(const struct existing_packs *existing,\n+\t\t\t\t  struct tempfile *f);\n int existing_packs_has_non_kept(const struct existing_packs *existing);\n int existing_pack_is_marked_for_deletion(struct string_list_item *item);\n void existing_packs_retain_cruft(struct existing_packs *existing,\n@@ -138,8 +153,6 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,\n \t\t\t\t    bool wrote_incremental_midx);\n void pack_geometry_release(struct pack_geometry *geometry);\n \n-struct tempfile;\n-\n enum repack_write_midx_mode {\n \tREPACK_WRITE_MIDX_NONE,\n \tREPACK_WRITE_MIDX_DEFAULT,\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex f0a390e3c6..845f032bea 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -254,6 +254,49 @@ test_expect_success 'repack --keep-pack' '\n \t)\n '\n \n+test_expect_success 'repack --keep-pack with --pack-kept-objects' '\n+\ttest_create_repo keep-pack-kept-objects &&\n+\t(\n+\t\tcd keep-pack-kept-objects &&\n+\t\tgit config pack.window 0 &&\n+\t\tgit config maintenance.auto false &&\n+\t\tP1=$(commit_and_pack 1) &&\n+\t\tP2=$(commit_and_pack 2) &&\n+\n+\t\t# \"--pack-kept-objects\" is about packs that have a \".keep\"\n+\t\t# file. A pack named with \"--keep-pack\" stays out of the\n+\t\t# result regardless, objects included.\n+\t\tgit repack -a -d --pack-kept-objects --keep-pack $P1 &&\n+\t\tls .git/objects/pack/*.pack >counts &&\n+\t\ttest_line_count = 2 counts &&\n+\t\ttest-tool find-pack -c 1 HEAD~1 &&\n+\t\ttest-tool find-pack -c 1 HEAD~1: &&\n+\t\tgit fsck\n+\t)\n+'\n+\n+test_expect_success FUNNYNAMES 'a kept pack whose name has a newline is refused' '\n+\ttest_create_repo keep-pack-newline &&\n+\t(\n+\t\tcd keep-pack-newline &&\n+\t\tgit config maintenance.auto false &&\n+\t\ttest_commit base &&\n+\t\tgit repack -ad &&\n+\n+\t\t# The names pack-objects is told to keep go one per line, so\n+\t\t# this one would come out as two, and the first of them is\n+\t\t# the name of the pack holding everything else.\n+\t\tvictim=\"$(basename \"$(ls .git/objects/pack/pack-*.pack)\")\" &&\n+\t\tname=\"$(printf \"%s\\nother\" \"$victim\")\" &&\n+\t\tP=$(git rev-parse HEAD | git pack-objects \".git/objects/pack/$name\") &&\n+\t\t>\".git/objects/pack/$name-$P.keep\" &&\n+\n+\t\ttest_must_fail git repack -ad 2>err &&\n+\t\ttest_grep \"contains a newline\" err &&\n+\t\tgit fsck\n+\t)\n+'\n+\n test_expect_success 'repacking fails when missing .pack actually means missing objects' '\n \ttest_create_repo idx-without-pack &&\n \t(\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex f3a0650cfe..6b914a2a80 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -541,4 +541,76 @@ test_expect_success 'geometric repack works with promisor packs' '\n \t)\n '\n \n+test_expect_success 'a \".keep\" that shows up mid-repack does not lose objects' '\n+\ttest_when_finished \"rm -fr race\" &&\n+\tgit init race &&\n+\t(\n+\t\tcd race &&\n+\n+\t\ttest_commit kept &&\n+\t\ttest_commit pack &&\n+\n+\t\tKEPT=$(git pack-objects --revs $packdir/pack <<-EOF\n+\t\trefs/tags/kept\n+\t\tEOF\n+\t\t) &&\n+\t\tgit pack-objects --revs $packdir/pack <<-EOF &&\n+\t\trefs/tags/pack\n+\t\t^refs/tags/kept\n+\t\tEOF\n+\t\tgit prune-packed &&\n+\n+\t\t# Neither pack is twice the size of the other, so both are\n+\t\t# redundant and get deleted. Have a \".keep\" appear on one of\n+\t\t# them as pack-objects starts, after the repack has decided\n+\t\t# to delete it: pack-objects used to notice the \".keep\" and\n+\t\t# leave those objects out of the replacement pack.\n+\t\tmkdir shim &&\n+\t\twrite_script shim/git <<-EOF &&\n+\t\ttest \"\\$1\" = \"pack-objects\" && >\"$(pwd)/$packdir/pack-$KEPT.keep\"\n+\t\tGIT_EXEC_PATH=\"$GIT_EXEC_PATH\" exec \"$GIT_EXEC_PATH/git\" \"\\$@\"\n+\t\tEOF\n+\n+\t\tgit --exec-path=\"$(pwd)/shim\" repack --geometric 2 -d &&\n+\n+\t\tgit fsck\n+\t)\n+'\n+\n+test_expect_success 'a kept pack does not stop the traversal from rescuing objects' '\n+\ttest_when_finished \"rm -fr kept-open\" &&\n+\tgit init kept-open &&\n+\t(\n+\t\tcd kept-open &&\n+\t\tgit config repack.midxMustContainCruft false &&\n+\n+\t\ttest_commit a &&\n+\t\ttest_commit b &&\n+\t\tb=$(git rev-parse b) &&\n+\t\tgit repack -ad &&\n+\n+\t\t# Make \"b\" unreachable and sweep it, together with its tree\n+\t\t# and blob, into a cruft pack.\n+\t\tgit tag -d b &&\n+\t\tgit reset --hard a &&\n+\t\tgit reflog expire --all --expire=all &&\n+\t\tgit repack -ad --cruft &&\n+\n+\t\t# Bring the commit back on its own, in a pack marked as kept.\n+\t\t# Its tree and blob are still only in the cruft pack.\n+\t\tkept=$(echo $b | git pack-objects $packdir/pack) &&\n+\t\t>$packdir/pack-$kept.keep &&\n+\n+\t\t# Build on top of it, so that the repack has to look through\n+\t\t# the kept pack to find out what the new commit depends on.\n+\t\tgit update-ref refs/heads/master \\\n+\t\t\t$(git commit-tree a^{tree} -p $b -m c) &&\n+\n+\t\tgit repack --geometric 2 -d --write-midx --write-bitmap-index &&\n+\t\ttest_path_is_file $packdir/multi-pack-index &&\n+\t\tls $packdir/multi-pack-index-*.bitmap >bitmaps &&\n+\t\ttest_line_count = 1 bitmaps\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"553026","messageId":"xmqqjyocdijn.fsf@gitster.g","threadId":"66327","inReplyTo":"77aec8941f5d17654f58956c7c643b47dd5a8d93.1789700615.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-22T22:31:56Z","receivedAt":"2026-09-22T22:32:02Z","isPatch":true,"body":"\"Qin ShiCheng via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> @@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs\n>  \t/*\n>  \t * Re-mark only the fresh packs as kept so that objects in\n>  \t * unknown packs do not halt the reachability traversal early.\n> +\t * The kept-pack cache was built while those packs were still\n> +\t * marked, so drop it too.\n>  \t */\n>  \trepo_for_each_pack(the_repository, p)\n>  \t\tp->pack_keep_in_core = 0;\n>  \tmark_pack_kept_in_core(fresh_packs, 1);\n> +\tfor (source = the_repository->objects->sources; source;\n> +\t     source = source->next) {\n> +\t\tstruct odb_source_files *files = odb_source_files_downcast(source);\n> +\t\tpackfile_store_invalidate_kept_pack_cache(files->packed);\n> +\t}\n\nThis question is primarily meant for folks who are pushing different\nODB backends, but I am not sure this is safe in the long term.\n\nWhen downcasting finds that 'source' is not from the files backend,\nwe immediately hit BUG().  Is checking the type of 'source' first\nand calling packfile_store_invalidate_kept_pack_cache() only when\nit is from the files backend a sensible workaround?  That sounds\nlike a blatant layering violation.\n\nOne of the recent design decisions, unrelated to this, was to make\nthe concept of \"alternate object store\" an implementation detail of\nthe files backend, if I recall correctly.  Do we need a similar\nrearchitecting of the code here, pushing details like packfile\nmanagement down to the files backend layer, before we can properly\nfix this?\n\nOf course, until an ODB backend other than files materializes, all\nof the above is merely academic and the proposed change might be\nsufficient.  However, relying on an unchecked downcast feels like\nlaying mines for our future selves.\n"},{"id":"553029","messageId":"SJ0PR84MB2993BE38DCAD2ECA5159EC24DD822@SJ0PR84MB2993.NAMPRD84.PROD.OUTLOOK.COM","threadId":"66327","inReplyTo":"xmqqjyocdijn.fsf@gitster.g","subject":"Re: [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk","fromName":"Qin ShiCheng","fromEmail":"qeesung@live.com","sentAt":"2026-09-23T03:08:00Z","receivedAt":"2026-09-23T03:08:12Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> When downcasting finds that 'source' is not from the files backend,\n> we immediately hit BUG().  Is checking the type of 'source' first\n> and calling packfile_store_invalidate_kept_pack_cache() only when\n> it is from the files backend a sensible workaround?  That sounds\n> like a blatant layering violation.\n\nAgreed, and I would rather not have pack-objects look at the type of\na source at all.\n\nThe assumption is already made two lines above the new loop, though:\nrepo_for_each_pack() downcasts every source in the same way, and so\ndoes has_object_kept_pack(), which is what reads this cache in the\nfirst place. So the loop is not wrong so much as in the wrong place.\nIt belongs next to its reader in packfile.c, not in the builtin.\n\nFor v3 I have this instead:\n\n\tvoid repo_invalidate_kept_pack_caches(struct repository *r)\n\t{\n\t\tstruct odb_source *source;\n\n\t\tfor (source = r->objects->sources; source; source = source->next) {\n\t\t\tstruct odb_source_files *files = odb_source_files_downcast(source);\n\t\t\tinvalidate_kept_pack_cache(files->packed);\n\t\t}\n\t}\n\nwith the per-store function made static again, and the caller in\npack-objects reduced to\n\n\tmark_pack_kept_in_core(fresh_packs, 1);\n\trepo_invalidate_kept_pack_caches(the_repository);\n\nThis does not make the code work with another backend -- nothing\naround it would either -- but pack-objects no longer gains a new\ndependency on the files backend, and the downcast sits with the\nothers that will have to move together.\n\n> Do we need a similar\n> rearchitecting of the code here, pushing details like packfile\n> management down to the files backend layer, before we can properly\n> fix this?\n\nI hope not. Without this patch, a cruft repack with an expiration\ndrops objects that are only reachable through a pack pack-objects was\nnot told about; the new test in t5329 shows it happening today. When\npackfile management does move down to the files backend, this\nfunction should go along with has_object_kept_pack(), and nothing in\nthe fix depends on where they end up. Patrick may well know better\nhow that is meant to look.\n\nThanks,\nQin\n"},{"id":"553088","messageId":"xmqqcxu3c15i.fsf@gitster.g","threadId":"66327","inReplyTo":"SJ0PR84MB2993BE38DCAD2ECA5159EC24DD822@SJ0PR84MB2993.NAMPRD84.PROD.OUTLOOK.COM","subject":"Re: [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-23T17:45:13Z","receivedAt":"2026-09-23T17:45:16Z","isPatch":true,"body":"Qin ShiCheng <qeesung@live.com> writes:\n\n> This does not make the code work with another backend -- nothing\n> around it would either -- but pack-objects no longer gains a new\n> dependency on the files backend, and the downcast sits with the\n> others that will have to move together.\n\nOK.\n\n>> Do we need a similar\n>> rearchitecting of the code here, pushing details like packfile\n>> management down to the files backend layer, before we can properly\n>> fix this?\n>\n> I hope not. Without this patch, a cruft repack with an expiration\n> drops objects ...\n\nAh, I think you misunderstood.\n\nBy fix \"this\" I meant fixing \"the layering violation\" and not what\nyour topic originally wanted to achieve.  And as we agreed above,\nthese downcasts that sit together with existing ones need to move in\norder to avoid layering violation, which is what I meant by\n\"rearchitecting\".  Until that happens, layering violation is left\nunfixed, but addressing the kept pack cache issue with layering\nviolation can be better than not addressing the issue at all.\n\nIn any case, my original question to experts\n\n>> This question is primarily meant for folks who are pushing different\n>> ODB backends, but I am not sure this is safe in the long term.\n\nstill stands.  I think we between two of us agreed the answer is \"no\nit is not safe in the long term\", but others may have ideas to solve\nit more cleanly, hopefully.\n\nThanks.\n\n"}]}