* [PATCH 1/6] odb: don't remove a ".keep" we never installed
2026-09-14 11:31 [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
@ 2026-09-14 11:31 ` Qin ShiCheng via GitGitGadget
2026-09-15 18:25 ` Justin Tobler
2026-09-14 11:31 ` [PATCH 2/6] pack-objects: keep --keep-pack open when following Qin ShiCheng via GitGitGadget
` (6 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-14 11:31 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
receive-pack runs index-pack with "--keep" over the quarantine, which
writes a "pack-XXX.keep" there. The path we register as a tempfile is
a different one: where that ".keep" will land once the quarantine is
migrated into the main object database.
Nothing of ours is at that path yet, and something else may be. Two
pushes of identical content produce identical thin packs, index-pack
names a pack after its contents, and so both want the same ".keep" in
the main object database. If the other push still holds it, that file
is what keeps its pack from being repacked away, and we remove it at
exit regardless -- even when pre-receive rejected our push and nothing
was migrated at all.
Register the path right before the migration instead, and once the
migration has returned, read the files back. index-pack wrote the
message we handed it; a file that says something else was not written
for us, so let go of it without removing it. tempfile gains
unregister_tempfile() for that.
Registering only after the migration would leave a window: the ".keep"
is the first thing migrated, and for a push that duplicates a large
pack the migration then spends a while comparing the two packfiles. A
signal in between would leave our ".keep" behind, with our message in
it, and every later push of the same content would fail to migrate
over it. Registering first keeps that window closed, as it is today.
Reading the files back also covers a migration that fails partway
through with our ".keep" already in place: we go by what is there, not
by whether the migration succeeded, and still remove it.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
object-file.c | 95 +++++++++++++++++++++++++++++---------
t/t5547-push-quarantine.sh | 52 +++++++++++++++++++++
tempfile.c | 12 +++++
tempfile.h | 9 ++++
4 files changed, 147 insertions(+), 21 deletions(-)
diff --git a/object-file.c b/object-file.c
index a4cbf8b081..21513ee535 100644
--- a/object-file.c
+++ b/object-file.c
@@ -29,6 +29,7 @@
#include "read-cache-ll.h"
#include "run-command.h"
#include "setup.h"
+#include "string-list.h"
#include "strvec.h"
#include "tempfile.h"
#include "tmp-objdir.h"
@@ -492,9 +493,13 @@ struct odb_transaction_files {
struct transaction_packfile packfile;
const char *prefix;
- struct tempfile **pack_lockfiles;
- size_t pack_lockfiles_nr;
- size_t pack_lockfiles_alloc;
+ /*
+ * The message index-pack writes into its ".keep" files, and where
+ * those files end up once the quarantine is migrated. Each "util"
+ * holds a tempfile for as long as we consider that file ours.
+ */
+ char *keep_msg;
+ struct string_list pack_lockfiles;
};
int odb_transaction_files_prepare(struct odb_transaction *base)
@@ -1256,6 +1261,45 @@ out:
return ret;
}
+/*
+ * Track the ".keep" files before the migration moves them into place, so
+ * that a signal in the middle of it removes ours.
+ */
+static void register_pack_lockfiles(struct odb_transaction_files *transaction)
+{
+ struct string_list_item *item;
+
+ for_each_string_list_item(item, &transaction->pack_lockfiles)
+ item->util = register_tempfile(item->string);
+}
+
+/*
+ * The migration stops at the first file that differs from what is already
+ * at its destination, and a ".keep" left by somebody else's push is one
+ * such file. Rather than work out what got installed, read the files
+ * back: one that does not carry our message is not ours to remove.
+ */
+static void disown_foreign_pack_lockfiles(struct odb_transaction_files *transaction)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct string_list_item *item;
+
+ for_each_string_list_item(item, &transaction->pack_lockfiles) {
+ struct tempfile *lockfile = item->util;
+
+ strbuf_reset(&buf);
+ if (strbuf_read_file(&buf, item->string, 0) >= 0) {
+ strbuf_trim_trailing_newline(&buf);
+ if (!strcmp(buf.buf, transaction->keep_msg))
+ continue;
+ }
+ unregister_tempfile(&lockfile);
+ item->util = NULL;
+ }
+
+ strbuf_release(&buf);
+}
+
static int odb_transaction_files_commit(struct odb_transaction *base)
{
struct odb_transaction_files *transaction =
@@ -1264,6 +1308,7 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
if (transaction->objdir) {
struct strbuf temp_path = STRBUF_INIT;
struct tempfile *temp;
+ int ret;
/*
* Issue a full hardware flush against a temporary file to ensure
@@ -1285,7 +1330,10 @@ static int odb_transaction_files_commit(struct odb_transaction *base)
* Make the object files visible in the primary ODB after their data is
* fully durable.
*/
- if (tmp_objdir_migrate(transaction->objdir))
+ register_pack_lockfiles(transaction);
+ ret = tmp_objdir_migrate(transaction->objdir);
+ disown_foreign_pack_lockfiles(transaction);
+ if (ret)
return error(_("unable to migrate temporary objects"));
transaction->objdir = NULL;
@@ -1393,10 +1441,10 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
if (xgethostname(hostname, sizeof(hostname)))
xsnprintf(hostname, sizeof(hostname), "localhost");
- strvec_pushf(&child.args,
- "--keep=receive-pack %"PRIuMAX" on %s",
- (uintmax_t)getpid(),
- hostname);
+ free(transaction->keep_msg);
+ transaction->keep_msg = xstrfmt("receive-pack %"PRIuMAX" on %s",
+ (uintmax_t)getpid(), hostname);
+ strvec_pushf(&child.args, "--keep=%s", transaction->keep_msg);
if (!opts->quiet && err_fd)
strvec_push(&child.args, "--show-resolving-progress");
@@ -1423,18 +1471,13 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
/*
* The lockfile filepath is expected to be the final location of
* the ".keep" file after being migrated to the main ODB source.
- * This ensures the lockfile can be found and removed later
- * after the ODB transaction has been committed.
+ * We start tracking it right before that migration; see
+ * odb_transaction_files_commit().
*/
lockfile = index_pack_lockfile(base->source, child.out, NULL);
- if (lockfile) {
- ALLOC_GROW(transaction->pack_lockfiles,
- transaction->pack_lockfiles_nr + 1,
- transaction->pack_lockfiles_alloc);
- transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
- register_tempfile(lockfile);
- free(lockfile);
- }
+ if (lockfile)
+ string_list_append_nodup(&transaction->pack_lockfiles,
+ lockfile);
close(child.out);
status = finish_command(&child);
@@ -1454,12 +1497,21 @@ static int odb_transaction_files_finalize(struct odb_transaction *base)
{
struct odb_transaction_files *transaction =
container_of(base, struct odb_transaction_files, base);
+ struct string_list_item *item;
int ret = 0;
- for (size_t i = 0; i < transaction->pack_lockfiles_nr; i++)
- ret |= delete_tempfile(&transaction->pack_lockfiles[i]);
+ /*
+ * Only the ".keep" files that turned out to be ours still have a
+ * tempfile attached; delete_tempfile() does nothing for the rest.
+ */
+ for_each_string_list_item(item, &transaction->pack_lockfiles) {
+ struct tempfile *lockfile = item->util;
+
+ ret |= delete_tempfile(&lockfile);
+ }
- free(transaction->pack_lockfiles);
+ string_list_clear(&transaction->pack_lockfiles, 0);
+ FREE_AND_NULL(transaction->keep_msg);
return ret;
}
@@ -1492,6 +1544,7 @@ int odb_transaction_files_begin(struct odb_source *source,
transaction->base.write_pack = odb_transaction_files_write_pack;
transaction->base.env = odb_transaction_files_env;
transaction->flags = flags;
+ string_list_init_dup(&transaction->pack_lockfiles);
transaction->prefix = "bulk-fsync";
if (flags & ODB_TRANSACTION_RECEIVE) {
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 1b7097179e..8623d2d6c1 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -101,4 +101,56 @@ test_expect_success '.keep file is removed after push' '
test_path_is_missing "$keep"
'
+test_expect_success 'a rejected push does not remove a foreign ".keep"' '
+ test_when_finished rm -rf foreign.git &&
+ git init --bare foreign.git &&
+ git -C foreign.git config set receive.unpackLimit 0 &&
+
+ # Get a packfile into the main object database without updating any
+ # ref, so that pushing the same objects again reuses its name.
+ test_hook -C foreign.git update <<-\EOF &&
+ exit 1
+ EOF
+ test_commit foreign &&
+ test_must_fail git push foreign.git HEAD:refs/heads/one &&
+
+ pack="$(ls foreign.git/objects/pack/pack-*.pack)" &&
+ keep="${pack%.pack}.keep" &&
+
+ # Pretend somebody else holds the lock on that packfile, and let the
+ # next push be rejected before its objects are ever migrated.
+ >"$keep" &&
+ test_hook -C foreign.git pre-receive <<-\EOF &&
+ exit 1
+ EOF
+ test_must_fail git push foreign.git HEAD:refs/heads/two &&
+ test_path_is_file "$keep"
+'
+
+test_expect_success 'a ".keep" installed by a failed migration is removed' '
+ test_when_finished rm -rf partial.git &&
+ git init --bare partial.git &&
+ git -C partial.git config set receive.unpackLimit 0 &&
+ git -C partial.git config set pack.indexVersion 1 &&
+
+ # Leave the objects in the main object database without a ref, so
+ # that pushing them again produces a pack with the same name.
+ test_hook -C partial.git update <<-\EOF &&
+ exit 1
+ EOF
+ test_commit partial &&
+ test_must_fail git push partial.git HEAD:refs/heads/one &&
+
+ # The same pack now arrives with a differently formatted index. The
+ # ".keep" is migrated first and goes in fine; the index then collides
+ # with the one already there, and the migration fails with our
+ # ".keep" already installed.
+ git -C partial.git config set pack.indexVersion 2 &&
+ test_must_fail git push partial.git HEAD:refs/heads/two 2>err &&
+ test_grep "unable to migrate" err &&
+
+ pack="$(ls partial.git/objects/pack/pack-*.pack)" &&
+ test_path_is_missing "${pack%.pack}.keep"
+'
+
test_done
diff --git a/tempfile.c b/tempfile.c
index dc9ca4e645..10db4fbc7f 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -373,6 +373,18 @@ int delete_tempfile(struct tempfile **tempfile_p)
return err ? -1 : 0;
}
+void unregister_tempfile(struct tempfile **tempfile_p)
+{
+ struct tempfile *tempfile = *tempfile_p;
+
+ if (!is_tempfile_active(tempfile))
+ return;
+
+ close_tempfile_gently(tempfile);
+ deactivate_tempfile(tempfile);
+ *tempfile_p = NULL;
+}
+
void reassign_tempfile_ownership(pid_t from, pid_t to)
{
volatile struct volatile_list_head *pos;
diff --git a/tempfile.h b/tempfile.h
index f571f3c609..b439066a30 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -275,6 +275,15 @@ int reopen_tempfile(struct tempfile *tempfile);
*/
int delete_tempfile(struct tempfile **tempfile_p);
+/*
+ * Stop tracking `tempfile` without removing the file: close the file
+ * descriptor and/or file pointer if they are still open, and leave the
+ * file where it is, no longer to be removed at exit or on a signal. It
+ * is a NOOP to call `unregister_tempfile()` for a `tempfile` object
+ * that is not currently active.
+ */
+void unregister_tempfile(struct tempfile **tempfile_p);
+
/*
* Close the file descriptor and/or file pointer if they are still
* open, and atomically rename the temporary file to `path`. `path`
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 1/6] odb: don't remove a ".keep" we never installed
2026-09-14 11:31 ` [PATCH 1/6] odb: don't remove a ".keep" we never installed Qin ShiCheng via GitGitGadget
@ 2026-09-15 18:25 ` Justin Tobler
2026-09-16 6:01 ` Qin ShiCheng
0 siblings, 1 reply; 26+ messages in thread
From: Justin Tobler @ 2026-09-15 18:25 UTC (permalink / raw)
To: Qin ShiCheng via GitGitGadget
Cc: git, Patrick Steinhardt, Taylor Blau, Junio C Hamano, qeesung
On 26/09/14 11:31AM, Qin ShiCheng via GitGitGadget wrote:
>From: Qin ShiCheng <qeesung@live.com>
>
>receive-pack runs index-pack with "--keep" over the quarantine, which
>writes a "pack-XXX.keep" there. The path we register as a tempfile is
>a different one: where that ".keep" will land once the quarantine is
>migrated into the main object database.
Yup, when the ".keep" file gets registered as a tempfile, it needs to
know where it will eventually be located post-migration. That way it can
be deleted after the references have been updated or if the process
exits early. This is a bit awkward, but its a result of use relying on
git-index-pack(1) to create the ".keep" file for us and it gets written
to the quaratine directory.
>Nothing of ours is at that path yet, and something else may be. Two
>pushes of identical content produce identical thin packs, index-pack
>names a pack after its contents, and so both want the same ".keep" in
>the main object database. If the other push still holds it, that file
>is what keeps its pack from being repacked away, and we remove it at
>exit regardless -- even when pre-receive rejected our push and nothing
>was migrated at all.
Interesting, for a pair of identical concurrent pushes, if one exits
early it could end of deleting the other processes packfile out from
under it. Really the process should probably only delete a ".keep" file
that itself created.
Something worth noting, if there are two concurrent identical pushes,
both will generate the same ".keep", but the keep message contained will
differ. In such cases, when the quarantined files are migrated to the
ODB, the ".keep" file that gets migrated first "wins" and the other push
will fail because the competing ".keep" fails the collision check and
consequently the push fails. I mention this because the current behavior
for how Git handles concurrent identical pushes is to reject one of
them. So if a process encounters an already existing ".keep" file in the
main ODB, it may be sufficient to abort early anyways.
>Register the path right before the migration instead, and once the
>migration has returned, read the files back. index-pack wrote the
>message we handed it; a file that says something else was not written
>for us, so let go of it without removing it. tempfile gains
>unregister_tempfile() for that.
Right, registering the temporary ".keep" files doesn't really need to
happen prior to the ODB transaction commit anyways. In fact, we could go
a step further and stop using git-index-pack(1) to prematurely create
".keep" files altogether in favor of letting the commit phase of the ODB
transaction create it explicitly. This has a couple of benefits:
- It avoids the already awkward tracking of ".keep" files in ODB
transaction pre-commit.
- It would also make fixing the issue in question a bit easier
by allowing us to simply try to create the ".keep" file and if
it already exists, unregister the tempfile and abort early.
Completely unrelated to this bug as part of another series I'm working
on locally, I've already have some patches that start creating ".keep"
files explicitly during the ODB commit phase in the "files" backend. I
would be happy to pick these patches out and send them upstream with
some small adjustments to also fix the issue here in your first patch.
Just let me know what you would perfer. :)
Thanks,
-Justin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/6] odb: don't remove a ".keep" we never installed
2026-09-15 18:25 ` Justin Tobler
@ 2026-09-16 6:01 ` Qin ShiCheng
0 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng @ 2026-09-16 6:01 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, Patrick Steinhardt, Taylor Blau, Junio C Hamano
On 26/09/15 01:25PM, Justin Tobler wrote:
> Something worth noting, if there are two concurrent identical pushes,
> both will generate the same ".keep", but the keep message contained will
> differ. In such cases, when the quarantined files are migrated to the
> ODB, the ".keep" file that gets migrated first "wins" and the other push
> will fail because the competing ".keep" fails the collision check and
> consequently the push fails. I mention this because the current behavior
> for how Git handles concurrent identical pushes is to reject one of
> them. So if a process encounters an already existing ".keep" file in the
> main ODB, it may be sufficient to abort early anyways.
Agreed, aborting is fine there. The two pushes that bit us were seconds
apart rather than concurrent: the first had already finished and removed
its ".keep", so the second found nothing at that path and linked its own
".keep" onto the first push's pack. What the patch is after is narrower
than handling the collision: whatever happens on the way out, we should
only remove a ".keep" we created ourselves. Today finalize unlinks the
path unconditionally, even when pre-receive rejected the push and
nothing was migrated at all, which is what the first t5547 test pins.
> Right, registering the temporary ".keep" files doesn't really need to
> happen prior to the ODB transaction commit anyways. In fact, we could go
> a step further and stop using git-index-pack(1) to prematurely create
> ".keep" files altogether in favor of letting the commit phase of the ODB
> transaction create it explicitly.
[...]
> Completely unrelated to this bug as part of another series I'm working
> on locally, I've already have some patches that start creating ".keep"
> files explicitly during the ODB commit phase in the "files" backend. I
> would be happy to pick these patches out and send them upstream with
> some small adjustments to also fix the issue here in your first patch.
> Just let me know what you would perfer. :)
That is the better shape for it, please do. Creating the file at commit
time and registering it right there gives the two guarantees this patch
gets by reading the files back: nothing foreign is removed, and a signal
after our ".keep" is in place still cleans it up. (v1 registers before
the migration for the latter: the ".keep" is migrated first, and for a
duplicate of a large pack the migration then spends a while in
check_collision().) One case worth keeping in mind is a migration that
fails partway with our ".keep" already installed, e.g. the ".idx"
colliding because pack.indexVersion changed between the two pushes; the
second t5547 test covers that.
I will drop 1/6 from v2 so the series is only the repack side; 2/6-6/6
do not depend on it. Feel free to take the two t5547 tests if they are
of use to your series.
Thanks,
Qin
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/6] pack-objects: keep --keep-pack open when following
2026-09-14 11:31 [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
2026-09-14 11:31 ` [PATCH 1/6] odb: don't remove a ".keep" we never installed Qin ShiCheng via GitGitGadget
@ 2026-09-14 11:31 ` Qin ShiCheng via GitGitGadget
2026-09-14 11:31 ` [PATCH 3/6] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
` (5 subsequent siblings)
7 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-14 11:31 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
"--stdin-packs=follow" distinguishes excluded packs that are closed
under reachability ("^") from those that are not ("!"). The traversal
stops at objects in the former, and goes on through the latter to
rescue whatever they depend on that would otherwise be left out.
A pack named with "--keep-pack" gets the same in-core flag as a "^"
pack, so the traversal stops at it too. Nothing warrants that: the
caller said not to repack it, not that it is self-contained. When it
holds a commit but not that commit's tree, the tree is never rescued,
and writing a bitmap over the result fails for lack of closure.
In follow mode, mark such a pack as kept-open instead, the way repack
already lists the packs it cannot vouch for as "!" on stdin. Its
objects stay out of the result, and the traversal can go through it.
This matters more once repack names its ".keep" packs this way instead
of passing "--honor-pack-keep": on-disk kept packs never were a
boundary, and they should not become one.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 20 +++++++++++++----
t/t5331-pack-objects-stdin.sh | 41 +++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 708b719f40..6f579173b0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4999,7 +4999,8 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
-static void add_extra_kept_packs(const struct string_list *names)
+static void add_extra_kept_packs(const struct string_list *names,
+ enum stdin_packs_mode stdin_packs)
{
struct packed_git *p;
@@ -5018,8 +5019,19 @@ static void add_extra_kept_packs(const struct string_list *names)
break;
if (i < names->nr) {
- p->pack_keep_in_core = 1;
- ignore_packed_keep_in_core = 1;
+ /*
+ * When following, treat the pack like a "!" pack, not
+ * a "^" one: nobody said it is closed under
+ * reachability, so the traversal must be able to go
+ * through it.
+ */
+ if (stdin_packs == STDIN_PACKS_MODE_FOLLOW) {
+ p->pack_keep_in_core_open = 1;
+ ignore_packed_keep_in_core_open = 1;
+ } else {
+ p->pack_keep_in_core = 1;
+ ignore_packed_keep_in_core = 1;
+ }
continue;
}
}
@@ -5443,7 +5455,7 @@ int cmd_pack_objects(int argc,
if (progress && all_progress_implied)
progress = 2;
- add_extra_kept_packs(&keep_pack_list);
+ add_extra_kept_packs(&keep_pack_list, stdin_packs);
if (ignore_packed_keep_on_disk) {
struct packed_git *p;
diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh
index c74b5861af..4e1fde1b08 100755
--- a/t/t5331-pack-objects-stdin.sh
+++ b/t/t5331-pack-objects-stdin.sh
@@ -483,6 +483,47 @@ test_expect_success '--stdin-packs=follow with open-excluded packs' '
)
'
+test_expect_success '--stdin-packs=follow walks through a --keep-pack pack' '
+ test_when_finished "rm -fr repo" &&
+
+ git init repo &&
+ (
+ cd repo &&
+ git config set maintenance.auto false &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+
+ A="$(echo A | git pack-objects --revs $packdir/pack)" &&
+ B="$(echo A..B | git pack-objects --revs $packdir/pack)" &&
+ C="$(echo B..C | git pack-objects --revs $packdir/pack)" &&
+ B_ONLY="$(git rev-parse B | git pack-objects $packdir/pack)" &&
+ git prune-packed &&
+
+ # Pack C is included and pack A is excluded and closed. The
+ # commit B is in the kept pack B_ONLY, but its tree and blob
+ # are only in pack B, which pack-objects is not told about.
+ # The kept pack keeps B out of the result, and the walk has
+ # to go through it to rescue the tree and the blob.
+ P=$(git pack-objects --stdin-packs=follow \
+ --keep-pack=pack-$B_ONLY.pack $packdir/pack <<-EOF
+ pack-$C.pack
+ ^pack-$A.pack
+ EOF
+ ) &&
+
+ {
+ objects_in_packs $C &&
+ git rev-parse "B^{tree}" B:B.t
+ } >expect.raw &&
+ sort expect.raw >expect &&
+
+ objects_in_packs $P >actual &&
+ test_cmp expect actual
+ )
+'
+
test_expect_success '--stdin-packs with !-delimited pack without follow' '
test_when_finished "rm -fr repo" &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 3/6] pack-objects: reset kept-pack cache for cruft walk
2026-09-14 11:31 [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
2026-09-14 11:31 ` [PATCH 1/6] odb: don't remove a ".keep" we never installed Qin ShiCheng via GitGitGadget
2026-09-14 11:31 ` [PATCH 2/6] pack-objects: keep --keep-pack open when following Qin ShiCheng via GitGitGadget
@ 2026-09-14 11:31 ` Qin ShiCheng via GitGitGadget
2026-09-14 11:31 ` [PATCH 4/6] pack-objects: sort --keep-pack list for lookup Qin ShiCheng via GitGitGadget
` (4 subsequent siblings)
7 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-14 11:31 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
When writing a cruft pack with an expiration, pack-objects first
collects the recent objects and then walks from them to rescue
whatever they reach, expired or not. A pack the caller did not list is
marked kept while collecting, so that its objects are not copied into
the cruft pack, and unmarked before the walk, so that the walk can go
through it.
The walk does not see the unmarking. Whether an object sits in a kept
pack is answered from a cache that is built on first use and only
dropped when asked about a different kind of kept pack. Collecting
builds it while the unlisted pack is still marked, the walk asks the
same kind of question, and so the unlisted pack stays in it: the walk
stops there, and whatever lies beyond it in an expired pack is lost.
This went unnoticed because of "--honor-pack-keep". repack passes it,
and when there is a ".keep" file it makes the collecting side ask
about on-disk and in-core kept packs together while the walk asks
about in-core ones alone; the cache is rebuilt each time the question
changes, and by accident the walk sees the current marks. Take the
".keep" file away and the objects are lost today. A later commit stops
repack from passing "--honor-pack-keep" at all, so fix this first.
Expose the invalidation packfile.c already has and call it after
re-marking. The test builds an unreachable chain whose middle commit
sits in a pack pack-objects is not told about and whose oldest objects
have expired; without the fix the cruft pack holds only the recent tip.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 8 +++++++
odb/source-packed.h | 3 ++-
packfile.c | 9 ++++++--
packfile.h | 7 ++++++
t/t5329-pack-objects-cruft.sh | 40 +++++++++++++++++++++++++++++++++++
5 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6f579173b0..8ca8255176 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4274,6 +4274,7 @@ static void enumerate_cruft_objects(void)
static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)
{
struct packed_git *p;
+ struct odb_source *source;
struct rev_info revs;
int ret;
@@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
/*
* Re-mark only the fresh packs as kept so that objects in
* unknown packs do not halt the reachability traversal early.
+ * The kept-pack cache was built while those packs were still
+ * marked, so drop it too.
*/
repo_for_each_pack(the_repository, p)
p->pack_keep_in_core = 0;
mark_pack_kept_in_core(fresh_packs, 1);
+ for (source = the_repository->objects->sources; source;
+ source = source->next) {
+ struct odb_source_files *files = odb_source_files_downcast(source);
+ packfile_store_invalidate_kept_pack_cache(files->packed);
+ }
if (prepare_revision_walk(&revs))
die(_("revision walk setup failed"));
diff --git a/odb/source-packed.h b/odb/source-packed.h
index a0f6b5096d..9e42311916 100644
--- a/odb/source-packed.h
+++ b/odb/source-packed.h
@@ -25,7 +25,8 @@ struct odb_source_packed {
* Should not be accessed directly, but via
* `packfile_store_get_kept_pack_cache()`. The list of packs gets
* invalidated when the stored flags and the flags passed to
- * `packfile_store_get_kept_pack_cache()` mismatch.
+ * `packfile_store_get_kept_pack_cache()` mismatch, or explicitly via
+ * `packfile_store_invalidate_kept_pack_cache()`.
*/
struct {
struct packed_git **packs;
diff --git a/packfile.c b/packfile.c
index 4fa5fd67c8..90459ec4d7 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1870,6 +1870,12 @@ int packfile_fill_entry(struct packed_git *p,
return 1;
}
+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store)
+{
+ FREE_AND_NULL(store->kept_cache.packs);
+ store->kept_cache.flags = 0;
+}
+
static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
unsigned flags)
{
@@ -1877,8 +1883,7 @@ static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
return;
if (store->kept_cache.flags == flags)
return;
- FREE_AND_NULL(store->kept_cache.packs);
- store->kept_cache.flags = 0;
+ packfile_store_invalidate_kept_pack_cache(store);
}
struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
diff --git a/packfile.h b/packfile.h
index 6d30d15a00..493faf0010 100644
--- a/packfile.h
+++ b/packfile.h
@@ -144,6 +144,13 @@ enum kept_pack_type {
struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
unsigned flags);
+/*
+ * Drop the cache of kept packs so that the next call to
+ * `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after changing
+ * which packs are kept in core.
+ */
+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store);
+
struct pack_window {
struct pack_window *next;
unsigned char *base;
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index 12cda06373..6302f60b75 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -332,6 +332,46 @@ test_expect_success 'cruft trees rescue sub-trees, blobs' '
)
'
+test_expect_success 'cruft traversal rescues through a pack it was not told about' '
+ git init repo &&
+ test_when_finished "rm -fr repo" &&
+ (
+ cd repo &&
+
+ test_commit packed &&
+ git repack -Ad &&
+ keep="$(basename "$(ls $packdir/pack-*.pack)")" &&
+
+ test_commit old &&
+ test_commit mid &&
+ test_commit new &&
+
+ # "old" has expired, "new" is recent, and "mid" sits in a
+ # pack that pack-objects is not told about. Rescuing "old"
+ # from "new" means walking through that pack.
+ git rev-list --objects --no-object-names packed..old >old &&
+ while read object
+ do
+ test-tool chmtime -1000 \
+ "$objdir/$(test_oid_to_path $object)" || exit 1
+ done <old &&
+ git rev-list --objects --no-object-names old..mid |
+ git pack-objects $packdir/pack >/dev/null &&
+ git prune-packed &&
+
+ cruft="$(echo $keep | git pack-objects --cruft \
+ --cruft-expiration=750.seconds.ago \
+ $packdir/pack)" &&
+ test-tool pack-mtimes "pack-$cruft.mtimes" >actual.raw &&
+
+ cut -d" " -f1 <actual.raw | sort >actual &&
+ git rev-list --objects --no-object-names packed..new >expect.raw &&
+ sort <expect.raw >expect &&
+
+ test_cmp expect actual
+ )
+'
+
test_expect_success 'expired objects are pruned' '
git init repo &&
test_when_finished "rm -fr repo" &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 4/6] pack-objects: sort --keep-pack list for lookup
2026-09-14 11:31 [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (2 preceding siblings ...)
2026-09-14 11:31 ` [PATCH 3/6] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
@ 2026-09-14 11:31 ` Qin ShiCheng via GitGitGadget
2026-09-14 11:31 ` [PATCH 5/6] pack-objects: add --keep-pack-from-file Qin ShiCheng via GitGitGadget
` (3 subsequent siblings)
7 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-14 11:31 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
add_extra_kept_packs() scans the whole "--keep-pack" list once per
pack in the repository. That is fine for the handful of names it gets
today, but the next commit lets a caller name every kept pack in the
repository, and with thousands of them the scan dominates: matching
20,000 kept packs against 20,000 names takes 11 seconds here, against
under a second with "--honor-pack-keep".
Sort the list once and look each pack up in it. The comparison stays
fspathcmp(), so what matches does not change.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8ca8255176..1fcb4ef8a5 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -5007,7 +5007,7 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
-static void add_extra_kept_packs(const struct string_list *names,
+static void add_extra_kept_packs(struct string_list *names,
enum stdin_packs_mode stdin_packs)
{
struct packed_git *p;
@@ -5015,18 +5015,13 @@ static void add_extra_kept_packs(const struct string_list *names,
if (!names->nr)
return;
- repo_for_each_pack(the_repository, p) {
- const char *name = basename(p->pack_name);
- int i;
+ string_list_sort(names);
+ repo_for_each_pack(the_repository, p) {
if (!p->pack_local)
continue;
- for (i = 0; i < names->nr; i++)
- if (!fspathcmp(name, names->items[i].string))
- break;
-
- if (i < names->nr) {
+ if (string_list_has_string(names, basename(p->pack_name))) {
/*
* When following, treat the pack like a "!" pack, not
* a "^" one: nobody said it is closed under
@@ -5151,7 +5146,9 @@ int cmd_pack_objects(int argc,
int rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;
int rev_list_index = 0;
enum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;
- struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
+ struct string_list keep_pack_list = {
+ .cmp = fspathcmp,
+ };
struct list_objects_filter_options filter_options =
LIST_OBJECTS_FILTER_INIT;
struct repo_config_values *cfg = repo_config_values(the_repository);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 5/6] pack-objects: add --keep-pack-from-file
2026-09-14 11:31 [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (3 preceding siblings ...)
2026-09-14 11:31 ` [PATCH 4/6] pack-objects: sort --keep-pack list for lookup Qin ShiCheng via GitGitGadget
@ 2026-09-14 11:31 ` Qin ShiCheng via GitGitGadget
2026-09-14 11:31 ` [PATCH 6/6] repack: tell pack-objects which packs are kept Qin ShiCheng via GitGitGadget
` (2 subsequent siblings)
7 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-14 11:31 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
"--keep-pack" names one pack per occurrence, and there is only so much
room on the command line: ARG_MAX is shared with the environment, and
on Windows the whole line is capped at 32,767 characters, which a few
hundred pack names fill. Past that the spawn fails before pack-objects
has started. fetch-pack grew "--stdin" in 078b895fef (fetch-pack: new
--stdin option to read refs from stdin, 2012-04-02) for the same
reason.
stdin is taken here: every mode repack drives pack-objects in already
uses it, for the revision list under "-a", object names for the
promisor pack, and pack lists for "--stdin-packs" and "--cruft". So
read the names from a file instead, one per line, skipping empty
lines. They go into the same list as the "--keep-pack" names and are
treated exactly alike: matched against local packs, ignored when they
match nothing, and kept open under "--stdin-packs=follow". A relative
path is resolved against the directory the user ran from, as
"--refs-snapshot" of "git multi-pack-index write" is.
The list now holds strings from two sources, so let it own its copies.
repack is about to use this to hand pack-objects its own snapshot of
the packs that have a ".keep" file.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
Documentation/git-pack-objects.adoc | 8 +++++
builtin/pack-objects.c | 28 ++++++++++++++++++
t/t5331-pack-objects-stdin.sh | 46 +++++++++++++++++++++++++++++
3 files changed, 82 insertions(+)
diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc
index 65cd00c152..938e27f69d 100644
--- a/Documentation/git-pack-objects.adoc
+++ b/Documentation/git-pack-objects.adoc
@@ -13,6 +13,7 @@ SYNOPSIS
[--no-reuse-delta] [--delta-base-offset] [--non-empty]
[--local] [--incremental] [--window=<n>] [--depth=<n>]
[--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
+ [--keep-pack-from-file=<file>]
[--cruft] [--cruft-expiration=<time>]
[--stdout [--filter=<filter-spec>] | <base-name>]
[--shallow] [--keep-true-parents] [--[no-]sparse]
@@ -193,6 +194,13 @@ depth is 4095.
leading directory (e.g. `pack-123.pack`). The option could be
specified multiple times to keep multiple packs.
+--keep-pack-from-file=<file>::
+ Read names of packs to keep from `<file>`, one per line, and
+ treat each of them as if it had been given with `--keep-pack`.
+ Empty lines are ignored. This is meant for callers such as
+ linkgit:git-repack[1] that may have to name more packs than fit
+ on a command line.
+
--incremental::
This flag causes an object already in a pack to be ignored
even if it would have otherwise been packed.
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1fcb4ef8a5..9f8c4b9135 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -194,6 +194,7 @@ static const char *const pack_usage[] = {
" [--no-reuse-delta] [--delta-base-offset] [--non-empty]\n"
" [--local] [--incremental] [--window=<n>] [--depth=<n>]\n"
" [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]\n"
+ " [--keep-pack-from-file=<file>]\n"
" [--cruft] [--cruft-expiration=<time>]\n"
" [--stdout [--filter=<filter-spec>] | <base-name>]\n"
" [--shallow] [--keep-true-parents] [--[no-]sparse]\n"
@@ -5007,6 +5008,26 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
+/*
+ * Read pack names from the file, one per line, as if each of them had
+ * been given with "--keep-pack".
+ */
+static void read_keep_pack_list(struct string_list *names, const char *path)
+{
+ struct strbuf buf = STRBUF_INIT;
+ FILE *fp = xfopen(path, "r");
+
+ while (strbuf_getline(&buf, fp) != EOF) {
+ if (!buf.len)
+ continue;
+ string_list_append(names, buf.buf);
+ }
+ if (ferror(fp))
+ die_errno(_("could not read '%s'"), path);
+ fclose(fp);
+ strbuf_release(&buf);
+}
+
static void add_extra_kept_packs(struct string_list *names,
enum stdin_packs_mode stdin_packs)
{
@@ -5147,8 +5168,10 @@ int cmd_pack_objects(int argc,
int rev_list_index = 0;
enum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;
struct string_list keep_pack_list = {
+ .strdup_strings = 1,
.cmp = fspathcmp,
};
+ char *keep_pack_from_file = NULL;
struct list_objects_filter_options filter_options =
LIST_OBJECTS_FILTER_INIT;
struct repo_config_values *cfg = repo_config_values(the_repository);
@@ -5233,6 +5256,8 @@ int cmd_pack_objects(int argc,
N_("ignore packs that have companion .keep file")),
OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
N_("ignore this pack")),
+ OPT_FILENAME(0, "keep-pack-from-file", &keep_pack_from_file,
+ N_("ignore the packs named in <file>")),
OPT_INTEGER(0, "compression", &cfg->pack_compression_level,
N_("pack compression level")),
OPT_BOOL(0, "keep-true-parents", &grafts_keep_true_parents,
@@ -5460,6 +5485,8 @@ int cmd_pack_objects(int argc,
if (progress && all_progress_implied)
progress = 2;
+ if (keep_pack_from_file)
+ read_keep_pack_list(&keep_pack_list, keep_pack_from_file);
add_extra_kept_packs(&keep_pack_list, stdin_packs);
if (ignore_packed_keep_on_disk) {
struct packed_git *p;
@@ -5554,6 +5581,7 @@ cleanup:
clear_packing_data(&to_pack);
list_objects_filter_release(&filter_options);
string_list_clear(&keep_pack_list, 0);
+ free(keep_pack_from_file);
strvec_clear(&rp);
return 0;
diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh
index 4e1fde1b08..d590aa4dad 100755
--- a/t/t5331-pack-objects-stdin.sh
+++ b/t/t5331-pack-objects-stdin.sh
@@ -561,4 +561,50 @@ test_expect_success '--stdin-packs with !-delimited pack without follow' '
)
'
+test_expect_success '--keep-pack-from-file names packs to keep' '
+ test_when_finished "rm -fr repo" &&
+
+ git init repo &&
+ (
+ cd repo &&
+ git config set maintenance.auto false &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+
+ A="$(echo A | git pack-objects --revs $packdir/pack)" &&
+ B="$(echo A..B | git pack-objects --revs $packdir/pack)" &&
+ C="$(echo B..C | git pack-objects --revs $packdir/pack)" &&
+ git prune-packed &&
+
+ # Empty lines and names that match no pack are ignored,
+ # as they would be with --keep-pack.
+ cat >keep <<-EOF &&
+ pack-$A.pack
+
+ pack-$B.pack
+ pack-does-not-exist.pack
+ EOF
+
+ P=$(git pack-objects --all --keep-pack=pack-$A.pack \
+ --keep-pack=pack-$B.pack from-argv </dev/null) &&
+ packed_objects from-argv-$P.idx >expect &&
+
+ P=$(git pack-objects --all --keep-pack-from-file=keep \
+ from-file </dev/null) &&
+ packed_objects from-file-$P.idx >actual &&
+ test_cmp expect actual &&
+
+ objects_in_packs $C >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success '--keep-pack-from-file with a missing file' '
+ test_must_fail git pack-objects --stdout \
+ --keep-pack-from-file=does-not-exist </dev/null 2>err &&
+ test_grep "could not open .does-not-exist. for reading" err
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 6/6] repack: tell pack-objects which packs are kept
2026-09-14 11:31 [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (4 preceding siblings ...)
2026-09-14 11:31 ` [PATCH 5/6] pack-objects: add --keep-pack-from-file Qin ShiCheng via GitGitGadget
@ 2026-09-14 11:31 ` Qin ShiCheng via GitGitGadget
2026-09-18 3:03 ` [PATCH v2 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
2026-10-08 9:52 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
7 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-14 11:31 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
repack works out which packs are redundant by looking for ".keep"
files when it starts, then passes "--honor-pack-keep" to the
pack-objects it spawns, which looks for them all over again. Two scans
of the same directory, seconds apart, with nothing holding them
together.
A ".keep" that turns up in between loses objects. The parent did not
see it, so the pack is on its list to delete. The child does see it,
so it leaves that pack's objects out of the replacement. The parent
deletes the pack regardless: repack_remove_redundant_pack() passes
force_delete, which skips the ".keep" check in unlink_pack_path(). The
objects are gone and repack exits successfully.
The gap is easy to land in. index-pack writes its ".keep" before it
renames the packfile into place, so a "git fetch" or a push being
migrated out of its quarantine will do it. Checking for the ".keep"
once more right before deleting would not help: a push holds it for a
fraction of a second, and it may well be gone again by the time
pack-objects has finished.
Hand pack-objects the kept packs we collected at startup and drop
"--honor-pack-keep". Both processes then work from one snapshot, and a
".keep" appearing or disappearing while we run cannot make them
disagree. An earlier commit made sure a pack kept this way is no more
of a boundary to the traversal than a ".keep" file was.
The list goes into a file next to the refs snapshot we already write
for "git multi-pack-index write", and is passed with
"--keep-pack-from-file" to every pack-objects we spawn when
"--pack-kept-objects" is not in effect, which is when
"--honor-pack-keep" used to be. The cruft pack-objects already has the
kept packs on its stdin; the file is redundant there, but it sees the
same list as everybody else. With nothing to keep, no file is written
and nothing is passed, which is what "--honor-pack-keep" came down to
when it found no ".keep".
The names go one per line, so a name with a newline in it cannot be
passed. "--stdin-packs" and "--cruft" have the same limit and die on a
name they cannot find, but "--keep-pack" ignores such a name, and the
two halves of a garbled one could go on to exclude some other pack;
refuse it up front instead.
The user's own "--keep-pack" arguments keep being forwarded, since
they apply either way. write_filtered_pack() had a loop passing the
kept packs too, but without the ".pack" suffix pack-objects compares
against; it goes. Kept packs borrowed from an alternate object
directory were covered by "--honor-pack-keep" and are not by the
snapshot, which only ever held local packs; repack never deletes
those, so their objects now get packed rather than skipped, which
costs room but cannot lose anything.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/repack.c | 15 ++++++++
repack-filtered.c | 3 --
repack.c | 34 ++++++++++++++++--
repack.h | 17 +++++++--
t/t7700-repack.sh | 43 ++++++++++++++++++++++
t/t7703-repack-geometric.sh | 72 +++++++++++++++++++++++++++++++++++++
6 files changed, 177 insertions(+), 7 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index c4360382c1..78bc98c4f1 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -167,6 +167,7 @@ int cmd_repack(int argc,
struct oidset drop_oids = OIDSET_INIT;
struct pack_geometry geometry = { 0 };
struct tempfile *refs_snapshot = NULL;
+ struct tempfile *kept_packs_snapshot = NULL;
int i, ret;
int show_progress;
@@ -456,6 +457,19 @@ int cmd_repack(int argc,
existing.repo = repo;
existing_packs_collect(&existing, &keep_pack_list);
+ if (existing.kept_packs.nr) {
+ struct strbuf path = STRBUF_INIT;
+
+ strbuf_addf(&path, "%s/%s_XXXXXX",
+ repo_get_object_directory(repo), "kept-packs");
+
+ kept_packs_snapshot = xmks_tempfile(path.buf);
+ existing_packs_snapshot_kept(&existing, kept_packs_snapshot);
+ po_args.kept_packs_snapshot =
+ get_tempfile_path(kept_packs_snapshot);
+
+ strbuf_release(&path);
+ }
if (geometry.split_factor) {
if (pack_everything)
@@ -644,6 +658,7 @@ int cmd_repack(int argc,
cruft_po_args.quiet = po_args.quiet;
cruft_po_args.delta_base_offset = po_args.delta_base_offset;
cruft_po_args.pack_kept_objects = 0;
+ cruft_po_args.kept_packs_snapshot = po_args.kept_packs_snapshot;
ret = write_cruft_pack(&opts, cruft_expiration,
combine_cruft_below_size, &names,
diff --git a/repack-filtered.c b/repack-filtered.c
index 869b9fc6e3..db8de9f633 100644
--- a/repack-filtered.c
+++ b/repack-filtered.c
@@ -25,9 +25,6 @@ int write_filtered_pack(const struct write_pack_opts *opts,
strvec_push(&cmd.args, "--stdin-packs");
- for_each_string_list_item(item, &existing->kept_packs)
- strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);
-
cmd.in = -1;
ret = start_command(&cmd);
diff --git a/repack.c b/repack.c
index d2aa58e134..a794486035 100644
--- a/repack.c
+++ b/repack.c
@@ -38,8 +38,9 @@ void prepare_pack_objects(struct child_process *cmd,
strvec_push(&cmd->args, "--quiet");
if (args->delta_base_offset)
strvec_push(&cmd->args, "--delta-base-offset");
- if (!args->pack_kept_objects)
- strvec_push(&cmd->args, "--honor-pack-keep");
+ if (!args->pack_kept_objects && args->kept_packs_snapshot)
+ strvec_pushf(&cmd->args, "--keep-pack-from-file=%s",
+ args->kept_packs_snapshot);
strvec_push(&cmd->args, out);
cmd->git_cmd = 1;
cmd->out = -1;
@@ -167,6 +168,35 @@ void existing_packs_collect(struct existing_packs *existing,
strbuf_release(&buf);
}
+void existing_packs_snapshot_kept(const struct existing_packs *existing,
+ struct tempfile *f)
+{
+ struct string_list_item *item;
+ FILE *out = fdopen_tempfile(f, "w");
+
+ if (!out)
+ die(_("could not open tempfile %s for writing"),
+ get_tempfile_path(f));
+
+ for_each_string_list_item(item, &existing->kept_packs) {
+ /*
+ * A newline would split the name in two, and pack-objects
+ * quietly keeps whichever packs the halves happen to name.
+ */
+ if (strchr(item->string, '\n'))
+ die(_("cannot keep pack '%s': its name contains a newline"),
+ item->string);
+ fprintf(out, "%s.pack\n", item->string);
+ }
+
+ if (close_tempfile_gently(f)) {
+ int save_errno = errno;
+ delete_tempfile(&f);
+ errno = save_errno;
+ die_errno(_("could not close kept packs snapshot tempfile"));
+ }
+}
+
int existing_packs_has_non_kept(const struct existing_packs *existing)
{
return existing->non_kept_packs.nr || existing->cruft_packs.nr;
diff --git a/repack.h b/repack.h
index 61e554e4ed..1c0aeca3e8 100644
--- a/repack.h
+++ b/repack.h
@@ -19,6 +19,14 @@ struct pack_objects_args {
int path_walk;
int delta_base_offset;
int pack_kept_objects;
+ /*
+ * File naming the packs to leave alone, one "<name>.pack" per line;
+ * NULL when there are none. pack-objects reads it rather than
+ * looking for ".keep" files itself, so that a ".keep" created or
+ * removed while we run cannot make the two of us disagree over
+ * which packs are being repacked.
+ */
+ const char *kept_packs_snapshot;
struct list_objects_filter_options filter_options;
};
@@ -28,6 +36,7 @@ struct pack_objects_args {
}
struct child_process;
+struct tempfile;
void prepare_pack_objects(struct child_process *cmd,
const struct pack_objects_args *args,
@@ -79,6 +88,12 @@ struct existing_packs {
*/
void existing_packs_collect(struct existing_packs *existing,
const struct string_list *extra_keep);
+/*
+ * Writes the names of the kept packs, one "<name>.pack" per line, into
+ * the given tempfile, for pack-objects to read with --keep-pack-from-file.
+ */
+void existing_packs_snapshot_kept(const struct existing_packs *existing,
+ struct tempfile *f);
int existing_packs_has_non_kept(const struct existing_packs *existing);
int existing_pack_is_marked_for_deletion(struct string_list_item *item);
void existing_packs_retain_cruft(struct existing_packs *existing,
@@ -138,8 +153,6 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,
bool wrote_incremental_midx);
void pack_geometry_release(struct pack_geometry *geometry);
-struct tempfile;
-
enum repack_write_midx_mode {
REPACK_WRITE_MIDX_NONE,
REPACK_WRITE_MIDX_DEFAULT,
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f0a390e3c6..845f032bea 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -254,6 +254,49 @@ test_expect_success 'repack --keep-pack' '
)
'
+test_expect_success 'repack --keep-pack with --pack-kept-objects' '
+ test_create_repo keep-pack-kept-objects &&
+ (
+ cd keep-pack-kept-objects &&
+ git config pack.window 0 &&
+ git config maintenance.auto false &&
+ P1=$(commit_and_pack 1) &&
+ P2=$(commit_and_pack 2) &&
+
+ # "--pack-kept-objects" is about packs that have a ".keep"
+ # file. A pack named with "--keep-pack" stays out of the
+ # result regardless, objects included.
+ git repack -a -d --pack-kept-objects --keep-pack $P1 &&
+ ls .git/objects/pack/*.pack >counts &&
+ test_line_count = 2 counts &&
+ test-tool find-pack -c 1 HEAD~1 &&
+ test-tool find-pack -c 1 HEAD~1: &&
+ git fsck
+ )
+'
+
+test_expect_success FUNNYNAMES 'a kept pack whose name has a newline is refused' '
+ test_create_repo keep-pack-newline &&
+ (
+ cd keep-pack-newline &&
+ git config maintenance.auto false &&
+ test_commit base &&
+ git repack -ad &&
+
+ # The names pack-objects is told to keep go one per line, so
+ # this one would come out as two, and the first of them is
+ # the name of the pack holding everything else.
+ victim="$(basename "$(ls .git/objects/pack/pack-*.pack)")" &&
+ name="$(printf "%s\nother" "$victim")" &&
+ P=$(git rev-parse HEAD | git pack-objects ".git/objects/pack/$name") &&
+ >".git/objects/pack/$name-$P.keep" &&
+
+ test_must_fail git repack -ad 2>err &&
+ test_grep "contains a newline" err &&
+ git fsck
+ )
+'
+
test_expect_success 'repacking fails when missing .pack actually means missing objects' '
test_create_repo idx-without-pack &&
(
diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index f3a0650cfe..6b914a2a80 100755
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -541,4 +541,76 @@ test_expect_success 'geometric repack works with promisor packs' '
)
'
+test_expect_success 'a ".keep" that shows up mid-repack does not lose objects' '
+ test_when_finished "rm -fr race" &&
+ git init race &&
+ (
+ cd race &&
+
+ test_commit kept &&
+ test_commit pack &&
+
+ KEPT=$(git pack-objects --revs $packdir/pack <<-EOF
+ refs/tags/kept
+ EOF
+ ) &&
+ git pack-objects --revs $packdir/pack <<-EOF &&
+ refs/tags/pack
+ ^refs/tags/kept
+ EOF
+ git prune-packed &&
+
+ # Neither pack is twice the size of the other, so both are
+ # redundant and get deleted. Have a ".keep" appear on one of
+ # them as pack-objects starts, after the repack has decided
+ # to delete it: pack-objects used to notice the ".keep" and
+ # leave those objects out of the replacement pack.
+ mkdir shim &&
+ write_script shim/git <<-EOF &&
+ test "\$1" = "pack-objects" && >"$(pwd)/$packdir/pack-$KEPT.keep"
+ GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
+ EOF
+
+ git --exec-path="$(pwd)/shim" repack --geometric 2 -d &&
+
+ git fsck
+ )
+'
+
+test_expect_success 'a kept pack does not stop the traversal from rescuing objects' '
+ test_when_finished "rm -fr kept-open" &&
+ git init kept-open &&
+ (
+ cd kept-open &&
+ git config repack.midxMustContainCruft false &&
+
+ test_commit a &&
+ test_commit b &&
+ b=$(git rev-parse b) &&
+ git repack -ad &&
+
+ # Make "b" unreachable and sweep it, together with its tree
+ # and blob, into a cruft pack.
+ git tag -d b &&
+ git reset --hard a &&
+ git reflog expire --all --expire=all &&
+ git repack -ad --cruft &&
+
+ # Bring the commit back on its own, in a pack marked as kept.
+ # Its tree and blob are still only in the cruft pack.
+ kept=$(echo $b | git pack-objects $packdir/pack) &&
+ >$packdir/pack-$kept.keep &&
+
+ # Build on top of it, so that the repack has to look through
+ # the kept pack to find out what the new commit depends on.
+ git update-ref refs/heads/master \
+ $(git commit-tree a^{tree} -p $b -m c) &&
+
+ git repack --geometric 2 -d --write-midx --write-bitmap-index &&
+ test_path_is_file $packdir/multi-pack-index &&
+ ls $packdir/multi-pack-index-*.bitmap >bitmaps &&
+ test_line_count = 1 bitmaps
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 0/5] repack: don't lose objects to a ".keep" that appears mid-run
2026-09-14 11:31 [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (5 preceding siblings ...)
2026-09-14 11:31 ` [PATCH 6/6] repack: tell pack-objects which packs are kept Qin ShiCheng via GitGitGadget
@ 2026-09-18 3:03 ` qeesung via GitGitGadget
2026-09-18 3:03 ` [PATCH v2 1/5] pack-objects: keep --keep-pack open when following Qin ShiCheng via GitGitGadget
` (4 more replies)
2026-10-08 9:52 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
7 siblings, 5 replies; 26+ messages in thread
From: qeesung via GitGitGadget @ 2026-09-18 3:03 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung
A concurrent push or fetch can make "git repack -d" delete a pack whose
objects were never copied anywhere, and exit 0. We hit this in production: a
ref pointing at a commit that no longer exists, on git 2.43, and it
reproduces on master.
What happens:
* repack scans for ".keep" files and decides which packs to delete, then
spawns pack-objects with --honor-pack-keep, which scans again;
* in between, an index-pack --keep finishes -- a push migrating its
quarantine, or a fetch -- and installs a ".keep" next to a pack that
repack has already decided to delete;
* pack-objects sees that ".keep" and leaves the pack's objects out; repack
deletes the pack by its earlier list, with force_delete.
The fix is to stop the two processes from scanning separately: hand
pack-objects the snapshot repack took at startup (5/5). Patches 1-4 are what
5/5 needs to be safe:
* 1/5: under --stdin-packs=follow, a --keep-pack pack stops the traversal
like a "^" pack; on-disk ".keep" packs never did.
* 2/5: the cruft walk goes by a stale kept-pack cache, which
--honor-pack-keep happened to mask. Pre-existing, reproducible today.
* 3/5: look --keep-pack names up in a sorted list; it gets long.
* 4/5: --keep-pack-from-file, since a repository can have more kept packs
than fit on a command line (32K characters on Windows).
Every fix comes with a test that fails without it; the race itself is
reproduced in t7703 by having a ".keep" appear as pack-objects starts. The
full suite passes, and the series merges cleanly into next and seen.
Changes since v1:
* Dropped 1/6 (odb: don't remove a ".keep" we never installed). Justin
Tobler is going to fix the receive-pack side properly, by having the ODB
transaction create the ".keep" itself at commit time rather than reading
back what index-pack wrote:
https://lore.kernel.org/git/aql8Wt2q9RnQpjEC@jtobler--20250820-SHC54/
* The remaining patches are unchanged apart from renumbering.
Qin ShiCheng (5):
pack-objects: keep --keep-pack open when following
pack-objects: reset kept-pack cache for cruft walk
pack-objects: sort --keep-pack list for lookup
pack-objects: add --keep-pack-from-file
repack: tell pack-objects which packs are kept
Documentation/git-pack-objects.adoc | 8 +++
builtin/pack-objects.c | 71 ++++++++++++++++++-----
builtin/repack.c | 15 +++++
odb/source-packed.h | 3 +-
packfile.c | 9 ++-
packfile.h | 7 +++
repack-filtered.c | 3 -
repack.c | 34 ++++++++++-
repack.h | 17 +++++-
t/t5329-pack-objects-cruft.sh | 40 +++++++++++++
t/t5331-pack-objects-stdin.sh | 87 +++++++++++++++++++++++++++++
t/t7700-repack.sh | 43 ++++++++++++++
t/t7703-repack-geometric.sh | 72 ++++++++++++++++++++++++
13 files changed, 386 insertions(+), 23 deletions(-)
base-commit: 3cb9185f65410273787f74333cc027d2ea5daada
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2219%2Fqeesung%2Frepack-kept-packs-snapshot-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2219/qeesung/repack-kept-packs-snapshot-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2219
Range-diff vs v1:
1: 932e8e425a < -: ---------- odb: don't remove a ".keep" we never installed
2: 9349ea48b0 = 1: 8cf72312c5 pack-objects: keep --keep-pack open when following
3: a1b85c0a25 = 2: 77aec8941f pack-objects: reset kept-pack cache for cruft walk
4: 38070935dc = 3: b76e06a467 pack-objects: sort --keep-pack list for lookup
5: f8e27b7aac = 4: 20a051cfb6 pack-objects: add --keep-pack-from-file
6: a18e354e73 = 5: 4684fd8552 repack: tell pack-objects which packs are kept
--
gitgitgadget
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v2 1/5] pack-objects: keep --keep-pack open when following
2026-09-18 3:03 ` [PATCH v2 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
@ 2026-09-18 3:03 ` Qin ShiCheng via GitGitGadget
2026-09-18 3:03 ` [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
` (3 subsequent siblings)
4 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-18 3:03 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
"--stdin-packs=follow" distinguishes excluded packs that are closed
under reachability ("^") from those that are not ("!"). The traversal
stops at objects in the former, and goes on through the latter to
rescue whatever they depend on that would otherwise be left out.
A pack named with "--keep-pack" gets the same in-core flag as a "^"
pack, so the traversal stops at it too. Nothing warrants that: the
caller said not to repack it, not that it is self-contained. When it
holds a commit but not that commit's tree, the tree is never rescued,
and writing a bitmap over the result fails for lack of closure.
In follow mode, mark such a pack as kept-open instead, the way repack
already lists the packs it cannot vouch for as "!" on stdin. Its
objects stay out of the result, and the traversal can go through it.
This matters more once repack names its ".keep" packs this way instead
of passing "--honor-pack-keep": on-disk kept packs never were a
boundary, and they should not become one.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 20 +++++++++++++----
t/t5331-pack-objects-stdin.sh | 41 +++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 708b719f40..6f579173b0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4999,7 +4999,8 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
-static void add_extra_kept_packs(const struct string_list *names)
+static void add_extra_kept_packs(const struct string_list *names,
+ enum stdin_packs_mode stdin_packs)
{
struct packed_git *p;
@@ -5018,8 +5019,19 @@ static void add_extra_kept_packs(const struct string_list *names)
break;
if (i < names->nr) {
- p->pack_keep_in_core = 1;
- ignore_packed_keep_in_core = 1;
+ /*
+ * When following, treat the pack like a "!" pack, not
+ * a "^" one: nobody said it is closed under
+ * reachability, so the traversal must be able to go
+ * through it.
+ */
+ if (stdin_packs == STDIN_PACKS_MODE_FOLLOW) {
+ p->pack_keep_in_core_open = 1;
+ ignore_packed_keep_in_core_open = 1;
+ } else {
+ p->pack_keep_in_core = 1;
+ ignore_packed_keep_in_core = 1;
+ }
continue;
}
}
@@ -5443,7 +5455,7 @@ int cmd_pack_objects(int argc,
if (progress && all_progress_implied)
progress = 2;
- add_extra_kept_packs(&keep_pack_list);
+ add_extra_kept_packs(&keep_pack_list, stdin_packs);
if (ignore_packed_keep_on_disk) {
struct packed_git *p;
diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh
index c74b5861af..4e1fde1b08 100755
--- a/t/t5331-pack-objects-stdin.sh
+++ b/t/t5331-pack-objects-stdin.sh
@@ -483,6 +483,47 @@ test_expect_success '--stdin-packs=follow with open-excluded packs' '
)
'
+test_expect_success '--stdin-packs=follow walks through a --keep-pack pack' '
+ test_when_finished "rm -fr repo" &&
+
+ git init repo &&
+ (
+ cd repo &&
+ git config set maintenance.auto false &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+
+ A="$(echo A | git pack-objects --revs $packdir/pack)" &&
+ B="$(echo A..B | git pack-objects --revs $packdir/pack)" &&
+ C="$(echo B..C | git pack-objects --revs $packdir/pack)" &&
+ B_ONLY="$(git rev-parse B | git pack-objects $packdir/pack)" &&
+ git prune-packed &&
+
+ # Pack C is included and pack A is excluded and closed. The
+ # commit B is in the kept pack B_ONLY, but its tree and blob
+ # are only in pack B, which pack-objects is not told about.
+ # The kept pack keeps B out of the result, and the walk has
+ # to go through it to rescue the tree and the blob.
+ P=$(git pack-objects --stdin-packs=follow \
+ --keep-pack=pack-$B_ONLY.pack $packdir/pack <<-EOF
+ pack-$C.pack
+ ^pack-$A.pack
+ EOF
+ ) &&
+
+ {
+ objects_in_packs $C &&
+ git rev-parse "B^{tree}" B:B.t
+ } >expect.raw &&
+ sort expect.raw >expect &&
+
+ objects_in_packs $P >actual &&
+ test_cmp expect actual
+ )
+'
+
test_expect_success '--stdin-packs with !-delimited pack without follow' '
test_when_finished "rm -fr repo" &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk
2026-09-18 3:03 ` [PATCH v2 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
2026-09-18 3:03 ` [PATCH v2 1/5] pack-objects: keep --keep-pack open when following Qin ShiCheng via GitGitGadget
@ 2026-09-18 3:03 ` Qin ShiCheng via GitGitGadget
2026-09-22 22:31 ` Junio C Hamano
2026-09-18 3:03 ` [PATCH v2 3/5] pack-objects: sort --keep-pack list for lookup Qin ShiCheng via GitGitGadget
` (2 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-18 3:03 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
When writing a cruft pack with an expiration, pack-objects first
collects the recent objects and then walks from them to rescue
whatever they reach, expired or not. A pack the caller did not list is
marked kept while collecting, so that its objects are not copied into
the cruft pack, and unmarked before the walk, so that the walk can go
through it.
The walk does not see the unmarking. Whether an object sits in a kept
pack is answered from a cache that is built on first use and only
dropped when asked about a different kind of kept pack. Collecting
builds it while the unlisted pack is still marked, the walk asks the
same kind of question, and so the unlisted pack stays in it: the walk
stops there, and whatever lies beyond it in an expired pack is lost.
This went unnoticed because of "--honor-pack-keep". repack passes it,
and when there is a ".keep" file it makes the collecting side ask
about on-disk and in-core kept packs together while the walk asks
about in-core ones alone; the cache is rebuilt each time the question
changes, and by accident the walk sees the current marks. Take the
".keep" file away and the objects are lost today. A later commit stops
repack from passing "--honor-pack-keep" at all, so fix this first.
Expose the invalidation packfile.c already has and call it after
re-marking. The test builds an unreachable chain whose middle commit
sits in a pack pack-objects is not told about and whose oldest objects
have expired; without the fix the cruft pack holds only the recent tip.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 8 +++++++
odb/source-packed.h | 3 ++-
packfile.c | 9 ++++++--
packfile.h | 7 ++++++
t/t5329-pack-objects-cruft.sh | 40 +++++++++++++++++++++++++++++++++++
5 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6f579173b0..8ca8255176 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4274,6 +4274,7 @@ static void enumerate_cruft_objects(void)
static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)
{
struct packed_git *p;
+ struct odb_source *source;
struct rev_info revs;
int ret;
@@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
/*
* Re-mark only the fresh packs as kept so that objects in
* unknown packs do not halt the reachability traversal early.
+ * The kept-pack cache was built while those packs were still
+ * marked, so drop it too.
*/
repo_for_each_pack(the_repository, p)
p->pack_keep_in_core = 0;
mark_pack_kept_in_core(fresh_packs, 1);
+ for (source = the_repository->objects->sources; source;
+ source = source->next) {
+ struct odb_source_files *files = odb_source_files_downcast(source);
+ packfile_store_invalidate_kept_pack_cache(files->packed);
+ }
if (prepare_revision_walk(&revs))
die(_("revision walk setup failed"));
diff --git a/odb/source-packed.h b/odb/source-packed.h
index a0f6b5096d..9e42311916 100644
--- a/odb/source-packed.h
+++ b/odb/source-packed.h
@@ -25,7 +25,8 @@ struct odb_source_packed {
* Should not be accessed directly, but via
* `packfile_store_get_kept_pack_cache()`. The list of packs gets
* invalidated when the stored flags and the flags passed to
- * `packfile_store_get_kept_pack_cache()` mismatch.
+ * `packfile_store_get_kept_pack_cache()` mismatch, or explicitly via
+ * `packfile_store_invalidate_kept_pack_cache()`.
*/
struct {
struct packed_git **packs;
diff --git a/packfile.c b/packfile.c
index 4fa5fd67c8..90459ec4d7 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1870,6 +1870,12 @@ int packfile_fill_entry(struct packed_git *p,
return 1;
}
+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store)
+{
+ FREE_AND_NULL(store->kept_cache.packs);
+ store->kept_cache.flags = 0;
+}
+
static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
unsigned flags)
{
@@ -1877,8 +1883,7 @@ static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
return;
if (store->kept_cache.flags == flags)
return;
- FREE_AND_NULL(store->kept_cache.packs);
- store->kept_cache.flags = 0;
+ packfile_store_invalidate_kept_pack_cache(store);
}
struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
diff --git a/packfile.h b/packfile.h
index 6d30d15a00..493faf0010 100644
--- a/packfile.h
+++ b/packfile.h
@@ -144,6 +144,13 @@ enum kept_pack_type {
struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
unsigned flags);
+/*
+ * Drop the cache of kept packs so that the next call to
+ * `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after changing
+ * which packs are kept in core.
+ */
+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store);
+
struct pack_window {
struct pack_window *next;
unsigned char *base;
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index 12cda06373..6302f60b75 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -332,6 +332,46 @@ test_expect_success 'cruft trees rescue sub-trees, blobs' '
)
'
+test_expect_success 'cruft traversal rescues through a pack it was not told about' '
+ git init repo &&
+ test_when_finished "rm -fr repo" &&
+ (
+ cd repo &&
+
+ test_commit packed &&
+ git repack -Ad &&
+ keep="$(basename "$(ls $packdir/pack-*.pack)")" &&
+
+ test_commit old &&
+ test_commit mid &&
+ test_commit new &&
+
+ # "old" has expired, "new" is recent, and "mid" sits in a
+ # pack that pack-objects is not told about. Rescuing "old"
+ # from "new" means walking through that pack.
+ git rev-list --objects --no-object-names packed..old >old &&
+ while read object
+ do
+ test-tool chmtime -1000 \
+ "$objdir/$(test_oid_to_path $object)" || exit 1
+ done <old &&
+ git rev-list --objects --no-object-names old..mid |
+ git pack-objects $packdir/pack >/dev/null &&
+ git prune-packed &&
+
+ cruft="$(echo $keep | git pack-objects --cruft \
+ --cruft-expiration=750.seconds.ago \
+ $packdir/pack)" &&
+ test-tool pack-mtimes "pack-$cruft.mtimes" >actual.raw &&
+
+ cut -d" " -f1 <actual.raw | sort >actual &&
+ git rev-list --objects --no-object-names packed..new >expect.raw &&
+ sort <expect.raw >expect &&
+
+ test_cmp expect actual
+ )
+'
+
test_expect_success 'expired objects are pruned' '
git init repo &&
test_when_finished "rm -fr repo" &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk
2026-09-18 3:03 ` [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
@ 2026-09-22 22:31 ` Junio C Hamano
2026-09-23 3:08 ` Qin ShiCheng
0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2026-09-22 22:31 UTC (permalink / raw)
To: Qin ShiCheng via GitGitGadget
Cc: git, Patrick Steinhardt, Taylor Blau, Justin Tobler, qeesung
"Qin ShiCheng via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
> /*
> * Re-mark only the fresh packs as kept so that objects in
> * unknown packs do not halt the reachability traversal early.
> + * The kept-pack cache was built while those packs were still
> + * marked, so drop it too.
> */
> repo_for_each_pack(the_repository, p)
> p->pack_keep_in_core = 0;
> mark_pack_kept_in_core(fresh_packs, 1);
> + for (source = the_repository->objects->sources; source;
> + source = source->next) {
> + struct odb_source_files *files = odb_source_files_downcast(source);
> + packfile_store_invalidate_kept_pack_cache(files->packed);
> + }
This question is primarily meant for folks who are pushing different
ODB backends, but I am not sure this is safe in the long term.
When downcasting finds that 'source' is not from the files backend,
we immediately hit BUG(). Is checking the type of 'source' first
and calling packfile_store_invalidate_kept_pack_cache() only when
it is from the files backend a sensible workaround? That sounds
like a blatant layering violation.
One of the recent design decisions, unrelated to this, was to make
the concept of "alternate object store" an implementation detail of
the files backend, if I recall correctly. Do we need a similar
rearchitecting of the code here, pushing details like packfile
management down to the files backend layer, before we can properly
fix this?
Of course, until an ODB backend other than files materializes, all
of the above is merely academic and the proposed change might be
sufficient. However, relying on an unchecked downcast feels like
laying mines for our future selves.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk
2026-09-22 22:31 ` Junio C Hamano
@ 2026-09-23 3:08 ` Qin ShiCheng
2026-09-23 17:45 ` Junio C Hamano
0 siblings, 1 reply; 26+ messages in thread
From: Qin ShiCheng @ 2026-09-23 3:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, Taylor Blau, Justin Tobler
Junio C Hamano <gitster@pobox.com> writes:
> When downcasting finds that 'source' is not from the files backend,
> we immediately hit BUG(). Is checking the type of 'source' first
> and calling packfile_store_invalidate_kept_pack_cache() only when
> it is from the files backend a sensible workaround? That sounds
> like a blatant layering violation.
Agreed, and I would rather not have pack-objects look at the type of
a source at all.
The assumption is already made two lines above the new loop, though:
repo_for_each_pack() downcasts every source in the same way, and so
does has_object_kept_pack(), which is what reads this cache in the
first place. So the loop is not wrong so much as in the wrong place.
It belongs next to its reader in packfile.c, not in the builtin.
For v3 I have this instead:
void repo_invalidate_kept_pack_caches(struct repository *r)
{
struct odb_source *source;
for (source = r->objects->sources; source; source = source->next) {
struct odb_source_files *files = odb_source_files_downcast(source);
invalidate_kept_pack_cache(files->packed);
}
}
with the per-store function made static again, and the caller in
pack-objects reduced to
mark_pack_kept_in_core(fresh_packs, 1);
repo_invalidate_kept_pack_caches(the_repository);
This does not make the code work with another backend -- nothing
around it would either -- but pack-objects no longer gains a new
dependency on the files backend, and the downcast sits with the
others that will have to move together.
> Do we need a similar
> rearchitecting of the code here, pushing details like packfile
> management down to the files backend layer, before we can properly
> fix this?
I hope not. Without this patch, a cruft repack with an expiration
drops objects that are only reachable through a pack pack-objects was
not told about; the new test in t5329 shows it happening today. When
packfile management does move down to the files backend, this
function should go along with has_object_kept_pack(), and nothing in
the fix depends on where they end up. Patrick may well know better
how that is meant to look.
Thanks,
Qin
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk
2026-09-23 3:08 ` Qin ShiCheng
@ 2026-09-23 17:45 ` Junio C Hamano
0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2026-09-23 17:45 UTC (permalink / raw)
To: Qin ShiCheng; +Cc: git, Patrick Steinhardt, Taylor Blau, Justin Tobler
Qin ShiCheng <qeesung@live.com> writes:
> This does not make the code work with another backend -- nothing
> around it would either -- but pack-objects no longer gains a new
> dependency on the files backend, and the downcast sits with the
> others that will have to move together.
OK.
>> Do we need a similar
>> rearchitecting of the code here, pushing details like packfile
>> management down to the files backend layer, before we can properly
>> fix this?
>
> I hope not. Without this patch, a cruft repack with an expiration
> drops objects ...
Ah, I think you misunderstood.
By fix "this" I meant fixing "the layering violation" and not what
your topic originally wanted to achieve. And as we agreed above,
these downcasts that sit together with existing ones need to move in
order to avoid layering violation, which is what I meant by
"rearchitecting". Until that happens, layering violation is left
unfixed, but addressing the kept pack cache issue with layering
violation can be better than not addressing the issue at all.
In any case, my original question to experts
>> This question is primarily meant for folks who are pushing different
>> ODB backends, but I am not sure this is safe in the long term.
still stands. I think we between two of us agreed the answer is "no
it is not safe in the long term", but others may have ideas to solve
it more cleanly, hopefully.
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 3/5] pack-objects: sort --keep-pack list for lookup
2026-09-18 3:03 ` [PATCH v2 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
2026-09-18 3:03 ` [PATCH v2 1/5] pack-objects: keep --keep-pack open when following Qin ShiCheng via GitGitGadget
2026-09-18 3:03 ` [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
@ 2026-09-18 3:03 ` Qin ShiCheng via GitGitGadget
2026-09-18 3:03 ` [PATCH v2 4/5] pack-objects: add --keep-pack-from-file Qin ShiCheng via GitGitGadget
2026-09-18 3:03 ` [PATCH v2 5/5] repack: tell pack-objects which packs are kept Qin ShiCheng via GitGitGadget
4 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-18 3:03 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
add_extra_kept_packs() scans the whole "--keep-pack" list once per
pack in the repository. That is fine for the handful of names it gets
today, but the next commit lets a caller name every kept pack in the
repository, and with thousands of them the scan dominates: matching
20,000 kept packs against 20,000 names takes 11 seconds here, against
under a second with "--honor-pack-keep".
Sort the list once and look each pack up in it. The comparison stays
fspathcmp(), so what matches does not change.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8ca8255176..1fcb4ef8a5 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -5007,7 +5007,7 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
-static void add_extra_kept_packs(const struct string_list *names,
+static void add_extra_kept_packs(struct string_list *names,
enum stdin_packs_mode stdin_packs)
{
struct packed_git *p;
@@ -5015,18 +5015,13 @@ static void add_extra_kept_packs(const struct string_list *names,
if (!names->nr)
return;
- repo_for_each_pack(the_repository, p) {
- const char *name = basename(p->pack_name);
- int i;
+ string_list_sort(names);
+ repo_for_each_pack(the_repository, p) {
if (!p->pack_local)
continue;
- for (i = 0; i < names->nr; i++)
- if (!fspathcmp(name, names->items[i].string))
- break;
-
- if (i < names->nr) {
+ if (string_list_has_string(names, basename(p->pack_name))) {
/*
* When following, treat the pack like a "!" pack, not
* a "^" one: nobody said it is closed under
@@ -5151,7 +5146,9 @@ int cmd_pack_objects(int argc,
int rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;
int rev_list_index = 0;
enum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;
- struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
+ struct string_list keep_pack_list = {
+ .cmp = fspathcmp,
+ };
struct list_objects_filter_options filter_options =
LIST_OBJECTS_FILTER_INIT;
struct repo_config_values *cfg = repo_config_values(the_repository);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 4/5] pack-objects: add --keep-pack-from-file
2026-09-18 3:03 ` [PATCH v2 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (2 preceding siblings ...)
2026-09-18 3:03 ` [PATCH v2 3/5] pack-objects: sort --keep-pack list for lookup Qin ShiCheng via GitGitGadget
@ 2026-09-18 3:03 ` Qin ShiCheng via GitGitGadget
2026-09-18 3:03 ` [PATCH v2 5/5] repack: tell pack-objects which packs are kept Qin ShiCheng via GitGitGadget
4 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-18 3:03 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
"--keep-pack" names one pack per occurrence, and there is only so much
room on the command line: ARG_MAX is shared with the environment, and
on Windows the whole line is capped at 32,767 characters, which a few
hundred pack names fill. Past that the spawn fails before pack-objects
has started. fetch-pack grew "--stdin" in 078b895fef (fetch-pack: new
--stdin option to read refs from stdin, 2012-04-02) for the same
reason.
stdin is taken here: every mode repack drives pack-objects in already
uses it, for the revision list under "-a", object names for the
promisor pack, and pack lists for "--stdin-packs" and "--cruft". So
read the names from a file instead, one per line, skipping empty
lines. They go into the same list as the "--keep-pack" names and are
treated exactly alike: matched against local packs, ignored when they
match nothing, and kept open under "--stdin-packs=follow". A relative
path is resolved against the directory the user ran from, as
"--refs-snapshot" of "git multi-pack-index write" is.
The list now holds strings from two sources, so let it own its copies.
repack is about to use this to hand pack-objects its own snapshot of
the packs that have a ".keep" file.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
Documentation/git-pack-objects.adoc | 8 +++++
builtin/pack-objects.c | 28 ++++++++++++++++++
t/t5331-pack-objects-stdin.sh | 46 +++++++++++++++++++++++++++++
3 files changed, 82 insertions(+)
diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc
index 65cd00c152..938e27f69d 100644
--- a/Documentation/git-pack-objects.adoc
+++ b/Documentation/git-pack-objects.adoc
@@ -13,6 +13,7 @@ SYNOPSIS
[--no-reuse-delta] [--delta-base-offset] [--non-empty]
[--local] [--incremental] [--window=<n>] [--depth=<n>]
[--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
+ [--keep-pack-from-file=<file>]
[--cruft] [--cruft-expiration=<time>]
[--stdout [--filter=<filter-spec>] | <base-name>]
[--shallow] [--keep-true-parents] [--[no-]sparse]
@@ -193,6 +194,13 @@ depth is 4095.
leading directory (e.g. `pack-123.pack`). The option could be
specified multiple times to keep multiple packs.
+--keep-pack-from-file=<file>::
+ Read names of packs to keep from `<file>`, one per line, and
+ treat each of them as if it had been given with `--keep-pack`.
+ Empty lines are ignored. This is meant for callers such as
+ linkgit:git-repack[1] that may have to name more packs than fit
+ on a command line.
+
--incremental::
This flag causes an object already in a pack to be ignored
even if it would have otherwise been packed.
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1fcb4ef8a5..9f8c4b9135 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -194,6 +194,7 @@ static const char *const pack_usage[] = {
" [--no-reuse-delta] [--delta-base-offset] [--non-empty]\n"
" [--local] [--incremental] [--window=<n>] [--depth=<n>]\n"
" [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]\n"
+ " [--keep-pack-from-file=<file>]\n"
" [--cruft] [--cruft-expiration=<time>]\n"
" [--stdout [--filter=<filter-spec>] | <base-name>]\n"
" [--shallow] [--keep-true-parents] [--[no-]sparse]\n"
@@ -5007,6 +5008,26 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
+/*
+ * Read pack names from the file, one per line, as if each of them had
+ * been given with "--keep-pack".
+ */
+static void read_keep_pack_list(struct string_list *names, const char *path)
+{
+ struct strbuf buf = STRBUF_INIT;
+ FILE *fp = xfopen(path, "r");
+
+ while (strbuf_getline(&buf, fp) != EOF) {
+ if (!buf.len)
+ continue;
+ string_list_append(names, buf.buf);
+ }
+ if (ferror(fp))
+ die_errno(_("could not read '%s'"), path);
+ fclose(fp);
+ strbuf_release(&buf);
+}
+
static void add_extra_kept_packs(struct string_list *names,
enum stdin_packs_mode stdin_packs)
{
@@ -5147,8 +5168,10 @@ int cmd_pack_objects(int argc,
int rev_list_index = 0;
enum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;
struct string_list keep_pack_list = {
+ .strdup_strings = 1,
.cmp = fspathcmp,
};
+ char *keep_pack_from_file = NULL;
struct list_objects_filter_options filter_options =
LIST_OBJECTS_FILTER_INIT;
struct repo_config_values *cfg = repo_config_values(the_repository);
@@ -5233,6 +5256,8 @@ int cmd_pack_objects(int argc,
N_("ignore packs that have companion .keep file")),
OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
N_("ignore this pack")),
+ OPT_FILENAME(0, "keep-pack-from-file", &keep_pack_from_file,
+ N_("ignore the packs named in <file>")),
OPT_INTEGER(0, "compression", &cfg->pack_compression_level,
N_("pack compression level")),
OPT_BOOL(0, "keep-true-parents", &grafts_keep_true_parents,
@@ -5460,6 +5485,8 @@ int cmd_pack_objects(int argc,
if (progress && all_progress_implied)
progress = 2;
+ if (keep_pack_from_file)
+ read_keep_pack_list(&keep_pack_list, keep_pack_from_file);
add_extra_kept_packs(&keep_pack_list, stdin_packs);
if (ignore_packed_keep_on_disk) {
struct packed_git *p;
@@ -5554,6 +5581,7 @@ cleanup:
clear_packing_data(&to_pack);
list_objects_filter_release(&filter_options);
string_list_clear(&keep_pack_list, 0);
+ free(keep_pack_from_file);
strvec_clear(&rp);
return 0;
diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh
index 4e1fde1b08..d590aa4dad 100755
--- a/t/t5331-pack-objects-stdin.sh
+++ b/t/t5331-pack-objects-stdin.sh
@@ -561,4 +561,50 @@ test_expect_success '--stdin-packs with !-delimited pack without follow' '
)
'
+test_expect_success '--keep-pack-from-file names packs to keep' '
+ test_when_finished "rm -fr repo" &&
+
+ git init repo &&
+ (
+ cd repo &&
+ git config set maintenance.auto false &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+
+ A="$(echo A | git pack-objects --revs $packdir/pack)" &&
+ B="$(echo A..B | git pack-objects --revs $packdir/pack)" &&
+ C="$(echo B..C | git pack-objects --revs $packdir/pack)" &&
+ git prune-packed &&
+
+ # Empty lines and names that match no pack are ignored,
+ # as they would be with --keep-pack.
+ cat >keep <<-EOF &&
+ pack-$A.pack
+
+ pack-$B.pack
+ pack-does-not-exist.pack
+ EOF
+
+ P=$(git pack-objects --all --keep-pack=pack-$A.pack \
+ --keep-pack=pack-$B.pack from-argv </dev/null) &&
+ packed_objects from-argv-$P.idx >expect &&
+
+ P=$(git pack-objects --all --keep-pack-from-file=keep \
+ from-file </dev/null) &&
+ packed_objects from-file-$P.idx >actual &&
+ test_cmp expect actual &&
+
+ objects_in_packs $C >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success '--keep-pack-from-file with a missing file' '
+ test_must_fail git pack-objects --stdout \
+ --keep-pack-from-file=does-not-exist </dev/null 2>err &&
+ test_grep "could not open .does-not-exist. for reading" err
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 5/5] repack: tell pack-objects which packs are kept
2026-09-18 3:03 ` [PATCH v2 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (3 preceding siblings ...)
2026-09-18 3:03 ` [PATCH v2 4/5] pack-objects: add --keep-pack-from-file Qin ShiCheng via GitGitGadget
@ 2026-09-18 3:03 ` Qin ShiCheng via GitGitGadget
4 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-09-18 3:03 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
repack works out which packs are redundant by looking for ".keep"
files when it starts, then passes "--honor-pack-keep" to the
pack-objects it spawns, which looks for them all over again. Two scans
of the same directory, seconds apart, with nothing holding them
together.
A ".keep" that turns up in between loses objects. The parent did not
see it, so the pack is on its list to delete. The child does see it,
so it leaves that pack's objects out of the replacement. The parent
deletes the pack regardless: repack_remove_redundant_pack() passes
force_delete, which skips the ".keep" check in unlink_pack_path(). The
objects are gone and repack exits successfully.
The gap is easy to land in. index-pack writes its ".keep" before it
renames the packfile into place, so a "git fetch" or a push being
migrated out of its quarantine will do it. Checking for the ".keep"
once more right before deleting would not help: a push holds it for a
fraction of a second, and it may well be gone again by the time
pack-objects has finished.
Hand pack-objects the kept packs we collected at startup and drop
"--honor-pack-keep". Both processes then work from one snapshot, and a
".keep" appearing or disappearing while we run cannot make them
disagree. An earlier commit made sure a pack kept this way is no more
of a boundary to the traversal than a ".keep" file was.
The list goes into a file next to the refs snapshot we already write
for "git multi-pack-index write", and is passed with
"--keep-pack-from-file" to every pack-objects we spawn when
"--pack-kept-objects" is not in effect, which is when
"--honor-pack-keep" used to be. The cruft pack-objects already has the
kept packs on its stdin; the file is redundant there, but it sees the
same list as everybody else. With nothing to keep, no file is written
and nothing is passed, which is what "--honor-pack-keep" came down to
when it found no ".keep".
The names go one per line, so a name with a newline in it cannot be
passed. "--stdin-packs" and "--cruft" have the same limit and die on a
name they cannot find, but "--keep-pack" ignores such a name, and the
two halves of a garbled one could go on to exclude some other pack;
refuse it up front instead.
The user's own "--keep-pack" arguments keep being forwarded, since
they apply either way. write_filtered_pack() had a loop passing the
kept packs too, but without the ".pack" suffix pack-objects compares
against; it goes. Kept packs borrowed from an alternate object
directory were covered by "--honor-pack-keep" and are not by the
snapshot, which only ever held local packs; repack never deletes
those, so their objects now get packed rather than skipped, which
costs room but cannot lose anything.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/repack.c | 15 ++++++++
repack-filtered.c | 3 --
repack.c | 34 ++++++++++++++++--
repack.h | 17 +++++++--
t/t7700-repack.sh | 43 ++++++++++++++++++++++
t/t7703-repack-geometric.sh | 72 +++++++++++++++++++++++++++++++++++++
6 files changed, 177 insertions(+), 7 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index c4360382c1..78bc98c4f1 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -167,6 +167,7 @@ int cmd_repack(int argc,
struct oidset drop_oids = OIDSET_INIT;
struct pack_geometry geometry = { 0 };
struct tempfile *refs_snapshot = NULL;
+ struct tempfile *kept_packs_snapshot = NULL;
int i, ret;
int show_progress;
@@ -456,6 +457,19 @@ int cmd_repack(int argc,
existing.repo = repo;
existing_packs_collect(&existing, &keep_pack_list);
+ if (existing.kept_packs.nr) {
+ struct strbuf path = STRBUF_INIT;
+
+ strbuf_addf(&path, "%s/%s_XXXXXX",
+ repo_get_object_directory(repo), "kept-packs");
+
+ kept_packs_snapshot = xmks_tempfile(path.buf);
+ existing_packs_snapshot_kept(&existing, kept_packs_snapshot);
+ po_args.kept_packs_snapshot =
+ get_tempfile_path(kept_packs_snapshot);
+
+ strbuf_release(&path);
+ }
if (geometry.split_factor) {
if (pack_everything)
@@ -644,6 +658,7 @@ int cmd_repack(int argc,
cruft_po_args.quiet = po_args.quiet;
cruft_po_args.delta_base_offset = po_args.delta_base_offset;
cruft_po_args.pack_kept_objects = 0;
+ cruft_po_args.kept_packs_snapshot = po_args.kept_packs_snapshot;
ret = write_cruft_pack(&opts, cruft_expiration,
combine_cruft_below_size, &names,
diff --git a/repack-filtered.c b/repack-filtered.c
index 869b9fc6e3..db8de9f633 100644
--- a/repack-filtered.c
+++ b/repack-filtered.c
@@ -25,9 +25,6 @@ int write_filtered_pack(const struct write_pack_opts *opts,
strvec_push(&cmd.args, "--stdin-packs");
- for_each_string_list_item(item, &existing->kept_packs)
- strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);
-
cmd.in = -1;
ret = start_command(&cmd);
diff --git a/repack.c b/repack.c
index d2aa58e134..a794486035 100644
--- a/repack.c
+++ b/repack.c
@@ -38,8 +38,9 @@ void prepare_pack_objects(struct child_process *cmd,
strvec_push(&cmd->args, "--quiet");
if (args->delta_base_offset)
strvec_push(&cmd->args, "--delta-base-offset");
- if (!args->pack_kept_objects)
- strvec_push(&cmd->args, "--honor-pack-keep");
+ if (!args->pack_kept_objects && args->kept_packs_snapshot)
+ strvec_pushf(&cmd->args, "--keep-pack-from-file=%s",
+ args->kept_packs_snapshot);
strvec_push(&cmd->args, out);
cmd->git_cmd = 1;
cmd->out = -1;
@@ -167,6 +168,35 @@ void existing_packs_collect(struct existing_packs *existing,
strbuf_release(&buf);
}
+void existing_packs_snapshot_kept(const struct existing_packs *existing,
+ struct tempfile *f)
+{
+ struct string_list_item *item;
+ FILE *out = fdopen_tempfile(f, "w");
+
+ if (!out)
+ die(_("could not open tempfile %s for writing"),
+ get_tempfile_path(f));
+
+ for_each_string_list_item(item, &existing->kept_packs) {
+ /*
+ * A newline would split the name in two, and pack-objects
+ * quietly keeps whichever packs the halves happen to name.
+ */
+ if (strchr(item->string, '\n'))
+ die(_("cannot keep pack '%s': its name contains a newline"),
+ item->string);
+ fprintf(out, "%s.pack\n", item->string);
+ }
+
+ if (close_tempfile_gently(f)) {
+ int save_errno = errno;
+ delete_tempfile(&f);
+ errno = save_errno;
+ die_errno(_("could not close kept packs snapshot tempfile"));
+ }
+}
+
int existing_packs_has_non_kept(const struct existing_packs *existing)
{
return existing->non_kept_packs.nr || existing->cruft_packs.nr;
diff --git a/repack.h b/repack.h
index 61e554e4ed..1c0aeca3e8 100644
--- a/repack.h
+++ b/repack.h
@@ -19,6 +19,14 @@ struct pack_objects_args {
int path_walk;
int delta_base_offset;
int pack_kept_objects;
+ /*
+ * File naming the packs to leave alone, one "<name>.pack" per line;
+ * NULL when there are none. pack-objects reads it rather than
+ * looking for ".keep" files itself, so that a ".keep" created or
+ * removed while we run cannot make the two of us disagree over
+ * which packs are being repacked.
+ */
+ const char *kept_packs_snapshot;
struct list_objects_filter_options filter_options;
};
@@ -28,6 +36,7 @@ struct pack_objects_args {
}
struct child_process;
+struct tempfile;
void prepare_pack_objects(struct child_process *cmd,
const struct pack_objects_args *args,
@@ -79,6 +88,12 @@ struct existing_packs {
*/
void existing_packs_collect(struct existing_packs *existing,
const struct string_list *extra_keep);
+/*
+ * Writes the names of the kept packs, one "<name>.pack" per line, into
+ * the given tempfile, for pack-objects to read with --keep-pack-from-file.
+ */
+void existing_packs_snapshot_kept(const struct existing_packs *existing,
+ struct tempfile *f);
int existing_packs_has_non_kept(const struct existing_packs *existing);
int existing_pack_is_marked_for_deletion(struct string_list_item *item);
void existing_packs_retain_cruft(struct existing_packs *existing,
@@ -138,8 +153,6 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,
bool wrote_incremental_midx);
void pack_geometry_release(struct pack_geometry *geometry);
-struct tempfile;
-
enum repack_write_midx_mode {
REPACK_WRITE_MIDX_NONE,
REPACK_WRITE_MIDX_DEFAULT,
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f0a390e3c6..845f032bea 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -254,6 +254,49 @@ test_expect_success 'repack --keep-pack' '
)
'
+test_expect_success 'repack --keep-pack with --pack-kept-objects' '
+ test_create_repo keep-pack-kept-objects &&
+ (
+ cd keep-pack-kept-objects &&
+ git config pack.window 0 &&
+ git config maintenance.auto false &&
+ P1=$(commit_and_pack 1) &&
+ P2=$(commit_and_pack 2) &&
+
+ # "--pack-kept-objects" is about packs that have a ".keep"
+ # file. A pack named with "--keep-pack" stays out of the
+ # result regardless, objects included.
+ git repack -a -d --pack-kept-objects --keep-pack $P1 &&
+ ls .git/objects/pack/*.pack >counts &&
+ test_line_count = 2 counts &&
+ test-tool find-pack -c 1 HEAD~1 &&
+ test-tool find-pack -c 1 HEAD~1: &&
+ git fsck
+ )
+'
+
+test_expect_success FUNNYNAMES 'a kept pack whose name has a newline is refused' '
+ test_create_repo keep-pack-newline &&
+ (
+ cd keep-pack-newline &&
+ git config maintenance.auto false &&
+ test_commit base &&
+ git repack -ad &&
+
+ # The names pack-objects is told to keep go one per line, so
+ # this one would come out as two, and the first of them is
+ # the name of the pack holding everything else.
+ victim="$(basename "$(ls .git/objects/pack/pack-*.pack)")" &&
+ name="$(printf "%s\nother" "$victim")" &&
+ P=$(git rev-parse HEAD | git pack-objects ".git/objects/pack/$name") &&
+ >".git/objects/pack/$name-$P.keep" &&
+
+ test_must_fail git repack -ad 2>err &&
+ test_grep "contains a newline" err &&
+ git fsck
+ )
+'
+
test_expect_success 'repacking fails when missing .pack actually means missing objects' '
test_create_repo idx-without-pack &&
(
diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index f3a0650cfe..6b914a2a80 100755
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -541,4 +541,76 @@ test_expect_success 'geometric repack works with promisor packs' '
)
'
+test_expect_success 'a ".keep" that shows up mid-repack does not lose objects' '
+ test_when_finished "rm -fr race" &&
+ git init race &&
+ (
+ cd race &&
+
+ test_commit kept &&
+ test_commit pack &&
+
+ KEPT=$(git pack-objects --revs $packdir/pack <<-EOF
+ refs/tags/kept
+ EOF
+ ) &&
+ git pack-objects --revs $packdir/pack <<-EOF &&
+ refs/tags/pack
+ ^refs/tags/kept
+ EOF
+ git prune-packed &&
+
+ # Neither pack is twice the size of the other, so both are
+ # redundant and get deleted. Have a ".keep" appear on one of
+ # them as pack-objects starts, after the repack has decided
+ # to delete it: pack-objects used to notice the ".keep" and
+ # leave those objects out of the replacement pack.
+ mkdir shim &&
+ write_script shim/git <<-EOF &&
+ test "\$1" = "pack-objects" && >"$(pwd)/$packdir/pack-$KEPT.keep"
+ GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
+ EOF
+
+ git --exec-path="$(pwd)/shim" repack --geometric 2 -d &&
+
+ git fsck
+ )
+'
+
+test_expect_success 'a kept pack does not stop the traversal from rescuing objects' '
+ test_when_finished "rm -fr kept-open" &&
+ git init kept-open &&
+ (
+ cd kept-open &&
+ git config repack.midxMustContainCruft false &&
+
+ test_commit a &&
+ test_commit b &&
+ b=$(git rev-parse b) &&
+ git repack -ad &&
+
+ # Make "b" unreachable and sweep it, together with its tree
+ # and blob, into a cruft pack.
+ git tag -d b &&
+ git reset --hard a &&
+ git reflog expire --all --expire=all &&
+ git repack -ad --cruft &&
+
+ # Bring the commit back on its own, in a pack marked as kept.
+ # Its tree and blob are still only in the cruft pack.
+ kept=$(echo $b | git pack-objects $packdir/pack) &&
+ >$packdir/pack-$kept.keep &&
+
+ # Build on top of it, so that the repack has to look through
+ # the kept pack to find out what the new commit depends on.
+ git update-ref refs/heads/master \
+ $(git commit-tree a^{tree} -p $b -m c) &&
+
+ git repack --geometric 2 -d --write-midx --write-bitmap-index &&
+ test_path_is_file $packdir/multi-pack-index &&
+ ls $packdir/multi-pack-index-*.bitmap >bitmaps &&
+ test_line_count = 1 bitmaps
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run
2026-09-14 11:31 [PATCH 0/6] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (6 preceding siblings ...)
2026-09-18 3:03 ` [PATCH v2 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
@ 2026-10-08 9:52 ` qeesung via GitGitGadget
2026-10-08 9:52 ` [PATCH v3 1/5] pack-objects: keep --keep-pack open when following Qin ShiCheng via GitGitGadget
` (5 more replies)
7 siblings, 6 replies; 26+ messages in thread
From: qeesung via GitGitGadget @ 2026-10-08 9:52 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung
A concurrent push or fetch can make "git repack -d" delete a pack whose
objects were never copied anywhere, and exit 0. We hit this in production: a
ref pointing at a commit that no longer exists, on git 2.43, and it
reproduces on master.
What happens:
* repack scans for ".keep" files and decides which packs to delete, then
spawns pack-objects with --honor-pack-keep, which scans again;
* in between, an index-pack --keep finishes -- a push migrating its
quarantine, or a fetch -- and installs a ".keep" next to a pack that
repack has already decided to delete;
* pack-objects sees that ".keep" and leaves the pack's objects out; repack
deletes the pack by its earlier list, with force_delete.
The fix is to stop the two processes from scanning separately: hand
pack-objects the snapshot repack took at startup (5/5). Patches 1-4 are what
5/5 needs to be safe:
* 1/5: under --stdin-packs=follow, a --keep-pack pack stops the traversal
like a "^" pack; on-disk ".keep" packs never did.
* 2/5: the cruft walk goes by a stale kept-pack cache, which
--honor-pack-keep happened to mask. Pre-existing, reproducible today.
* 3/5: look --keep-pack names up in a sorted list; it gets long.
* 4/5: --keep-pack-from-file, since a repository can have more kept packs
than fit on a command line (32K characters on Windows).
Every fix comes with a test that fails without it; the race itself is
reproduced in t7703 by having a ".keep" appear as pack-objects starts. The
full suite passes.
Interaction with topics in seen:
* ps/odb-files-alternates turns the list of object sources into a single
files source with a list of object directories, so
repo_invalidate_kept_pack_caches() from 2/5 needs to walk those instead.
The textual merge is clean, but the build breaks; this resolution follows
has_object_kept_pack() on that topic:
void repo_invalidate_kept_pack_caches(struct repository *r) { struct
odb_source_files *files = odb_source_files_downcast(r->objects->source);
for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
invalidate_kept_pack_cache(dir->packed); }
* tb/repack-cruft-less-midx-corner-cases appends tests to the end of t5331,
as does 4/5; keep both. That topic also hands kept packs to the
"--stdin-packs=follow" walk as '^' or '!', so with this series such a
pack is named twice, once on stdin and once in the file from 5/5. The two
compose: a '^' pack still stops the walk, and a '!' pack is open either
way.
With that resolution, seen with this series merged passes the full suite.
Changes since v2:
* 2/5: the loop that drops the kept-pack cache of every object source moved
from pack-objects into packfile.c, as repo_invalidate_kept_pack_caches(),
next to has_object_kept_pack() which reads that cache; the per-store
helper is static again. This keeps pack-objects from gaining a direct use
of the files backend, but the loop still assumes every source is one,
like its neighbours, so the layering issue Junio raised is not solved
here: https://lore.kernel.org/git/xmqqcxu3c15i.fsf@gitster.g/ (I first
misread his question as asking whether the fix itself had to wait for
that rework; it was about the layering.)
* The rest is unchanged.
Changes since v1:
* Dropped the receive-pack patch; Justin Tobler is fixing that side by
having the ODB transaction create the ".keep" itself:
https://lore.kernel.org/git/aql8Wt2q9RnQpjEC@jtobler--20250820-SHC54/
Qin ShiCheng (5):
pack-objects: keep --keep-pack open when following
pack-objects: reset kept-pack cache for cruft walk
pack-objects: sort --keep-pack list for lookup
pack-objects: add --keep-pack-from-file
repack: tell pack-objects which packs are kept
Documentation/git-pack-objects.adoc | 8 +++
builtin/pack-objects.c | 66 +++++++++++++++++-----
builtin/repack.c | 15 +++++
odb/source-packed.h | 3 +-
packfile.c | 19 ++++++-
packfile.h | 7 +++
repack-filtered.c | 3 -
repack.c | 34 ++++++++++-
repack.h | 17 +++++-
t/t5329-pack-objects-cruft.sh | 40 +++++++++++++
t/t5331-pack-objects-stdin.sh | 87 +++++++++++++++++++++++++++++
t/t7700-repack.sh | 43 ++++++++++++++
t/t7703-repack-geometric.sh | 72 ++++++++++++++++++++++++
13 files changed, 391 insertions(+), 23 deletions(-)
base-commit: 3cb9185f65410273787f74333cc027d2ea5daada
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2219%2Fqeesung%2Frepack-kept-packs-snapshot-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2219/qeesung/repack-kept-packs-snapshot-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2219
Range-diff vs v2:
1: 8cf72312c5 = 1: 8cf72312c5 pack-objects: keep --keep-pack open when following
2: 77aec8941f ! 2: 58019e4983 pack-objects: reset kept-pack cache for cruft walk
@@ Commit message
dropped when asked about a different kind of kept pack. Collecting
builds it while the unlisted pack is still marked, the walk asks the
same kind of question, and so the unlisted pack stays in it: the walk
- stops there, and whatever lies beyond it in an expired pack is lost.
+ stops there, and whatever lies beyond it in an expired pack is left
+ out of the cruft pack, to go when that pack is deleted.
This went unnoticed because of "--honor-pack-keep". repack passes it,
and when there is a ".keep" file it makes the collecting side ask
@@ Commit message
".keep" file away and the objects are lost today. A later commit stops
repack from passing "--honor-pack-keep" at all, so fix this first.
- Expose the invalidation packfile.c already has and call it after
- re-marking. The test builds an unreachable chain whose middle commit
- sits in a pack pack-objects is not told about and whose oldest objects
- have expired; without the fix the cruft pack holds only the recent tip.
+ Drop the cache after re-marking. The loop over the object sources
+ that does so lives in packfile.c, as repo_invalidate_kept_pack_caches(),
+ next to has_object_kept_pack() which reads the cache. Like it, the
+ loop assumes every source is a files backend; keeping that assumption
+ in packfile.c rather than adding it to pack-objects means the two can
+ move together once packfile management is pushed down into that
+ backend.
+
+ The test builds an unreachable chain whose middle commit sits in a
+ pack pack-objects is not told about and whose oldest objects have
+ expired; without the fix the cruft pack holds only the recent tip.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
## builtin/pack-objects.c ##
-@@ builtin/pack-objects.c: static void enumerate_cruft_objects(void)
- static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)
- {
- struct packed_git *p;
-+ struct odb_source *source;
- struct rev_info revs;
- int ret;
-
@@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
/*
* Re-mark only the fresh packs as kept so that objects in
@@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct
repo_for_each_pack(the_repository, p)
p->pack_keep_in_core = 0;
mark_pack_kept_in_core(fresh_packs, 1);
-+ for (source = the_repository->objects->sources; source;
-+ source = source->next) {
-+ struct odb_source_files *files = odb_source_files_downcast(source);
-+ packfile_store_invalidate_kept_pack_cache(files->packed);
-+ }
++ repo_invalidate_kept_pack_caches(the_repository);
if (prepare_revision_walk(&revs))
die(_("revision walk setup failed"));
@@ odb/source-packed.h: struct odb_source_packed {
* invalidated when the stored flags and the flags passed to
- * `packfile_store_get_kept_pack_cache()` mismatch.
+ * `packfile_store_get_kept_pack_cache()` mismatch, or explicitly via
-+ * `packfile_store_invalidate_kept_pack_cache()`.
++ * `repo_invalidate_kept_pack_caches()`.
*/
struct {
struct packed_git **packs;
@@ packfile.c: int packfile_fill_entry(struct packed_git *p,
return 1;
}
-+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store)
++static void invalidate_kept_pack_cache(struct odb_source_packed *store)
+{
+ FREE_AND_NULL(store->kept_cache.packs);
+ store->kept_cache.flags = 0;
+}
++
++void repo_invalidate_kept_pack_caches(struct repository *r)
++{
++ struct odb_source *source;
++
++ for (source = r->objects->sources; source; source = source->next) {
++ struct odb_source_files *files = odb_source_files_downcast(source);
++ invalidate_kept_pack_cache(files->packed);
++ }
++}
+
static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
unsigned flags)
@@ packfile.c: static void maybe_invalidate_kept_pack_cache(struct odb_source_packe
return;
- FREE_AND_NULL(store->kept_cache.packs);
- store->kept_cache.flags = 0;
-+ packfile_store_invalidate_kept_pack_cache(store);
++ invalidate_kept_pack_cache(store);
}
struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
@@ packfile.h: enum kept_pack_type {
unsigned flags);
+/*
-+ * Drop the cache of kept packs so that the next call to
-+ * `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after changing
-+ * which packs are kept in core.
++ * Drop every packfile store's cache of kept packs, so that the next call
++ * to `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after
++ * changing which packs are kept in core.
+ */
-+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store);
++void repo_invalidate_kept_pack_caches(struct repository *r);
+
struct pack_window {
struct pack_window *next;
3: b76e06a467 = 3: ff2146d002 pack-objects: sort --keep-pack list for lookup
4: 20a051cfb6 = 4: 148175cfa0 pack-objects: add --keep-pack-from-file
5: 4684fd8552 = 5: 8f3b9d9ee0 repack: tell pack-objects which packs are kept
--
gitgitgadget
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v3 1/5] pack-objects: keep --keep-pack open when following
2026-10-08 9:52 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
@ 2026-10-08 9:52 ` Qin ShiCheng via GitGitGadget
2026-10-08 9:52 ` [PATCH v3 2/5] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
` (4 subsequent siblings)
5 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-10-08 9:52 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
"--stdin-packs=follow" distinguishes excluded packs that are closed
under reachability ("^") from those that are not ("!"). The traversal
stops at objects in the former, and goes on through the latter to
rescue whatever they depend on that would otherwise be left out.
A pack named with "--keep-pack" gets the same in-core flag as a "^"
pack, so the traversal stops at it too. Nothing warrants that: the
caller said not to repack it, not that it is self-contained. When it
holds a commit but not that commit's tree, the tree is never rescued,
and writing a bitmap over the result fails for lack of closure.
In follow mode, mark such a pack as kept-open instead, the way repack
already lists the packs it cannot vouch for as "!" on stdin. Its
objects stay out of the result, and the traversal can go through it.
This matters more once repack names its ".keep" packs this way instead
of passing "--honor-pack-keep": on-disk kept packs never were a
boundary, and they should not become one.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 20 +++++++++++++----
t/t5331-pack-objects-stdin.sh | 41 +++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 708b719f40..6f579173b0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4999,7 +4999,8 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
-static void add_extra_kept_packs(const struct string_list *names)
+static void add_extra_kept_packs(const struct string_list *names,
+ enum stdin_packs_mode stdin_packs)
{
struct packed_git *p;
@@ -5018,8 +5019,19 @@ static void add_extra_kept_packs(const struct string_list *names)
break;
if (i < names->nr) {
- p->pack_keep_in_core = 1;
- ignore_packed_keep_in_core = 1;
+ /*
+ * When following, treat the pack like a "!" pack, not
+ * a "^" one: nobody said it is closed under
+ * reachability, so the traversal must be able to go
+ * through it.
+ */
+ if (stdin_packs == STDIN_PACKS_MODE_FOLLOW) {
+ p->pack_keep_in_core_open = 1;
+ ignore_packed_keep_in_core_open = 1;
+ } else {
+ p->pack_keep_in_core = 1;
+ ignore_packed_keep_in_core = 1;
+ }
continue;
}
}
@@ -5443,7 +5455,7 @@ int cmd_pack_objects(int argc,
if (progress && all_progress_implied)
progress = 2;
- add_extra_kept_packs(&keep_pack_list);
+ add_extra_kept_packs(&keep_pack_list, stdin_packs);
if (ignore_packed_keep_on_disk) {
struct packed_git *p;
diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh
index c74b5861af..4e1fde1b08 100755
--- a/t/t5331-pack-objects-stdin.sh
+++ b/t/t5331-pack-objects-stdin.sh
@@ -483,6 +483,47 @@ test_expect_success '--stdin-packs=follow with open-excluded packs' '
)
'
+test_expect_success '--stdin-packs=follow walks through a --keep-pack pack' '
+ test_when_finished "rm -fr repo" &&
+
+ git init repo &&
+ (
+ cd repo &&
+ git config set maintenance.auto false &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+
+ A="$(echo A | git pack-objects --revs $packdir/pack)" &&
+ B="$(echo A..B | git pack-objects --revs $packdir/pack)" &&
+ C="$(echo B..C | git pack-objects --revs $packdir/pack)" &&
+ B_ONLY="$(git rev-parse B | git pack-objects $packdir/pack)" &&
+ git prune-packed &&
+
+ # Pack C is included and pack A is excluded and closed. The
+ # commit B is in the kept pack B_ONLY, but its tree and blob
+ # are only in pack B, which pack-objects is not told about.
+ # The kept pack keeps B out of the result, and the walk has
+ # to go through it to rescue the tree and the blob.
+ P=$(git pack-objects --stdin-packs=follow \
+ --keep-pack=pack-$B_ONLY.pack $packdir/pack <<-EOF
+ pack-$C.pack
+ ^pack-$A.pack
+ EOF
+ ) &&
+
+ {
+ objects_in_packs $C &&
+ git rev-parse "B^{tree}" B:B.t
+ } >expect.raw &&
+ sort expect.raw >expect &&
+
+ objects_in_packs $P >actual &&
+ test_cmp expect actual
+ )
+'
+
test_expect_success '--stdin-packs with !-delimited pack without follow' '
test_when_finished "rm -fr repo" &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 2/5] pack-objects: reset kept-pack cache for cruft walk
2026-10-08 9:52 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
2026-10-08 9:52 ` [PATCH v3 1/5] pack-objects: keep --keep-pack open when following Qin ShiCheng via GitGitGadget
@ 2026-10-08 9:52 ` Qin ShiCheng via GitGitGadget
2026-10-08 9:52 ` [PATCH v3 3/5] pack-objects: sort --keep-pack list for lookup Qin ShiCheng via GitGitGadget
` (3 subsequent siblings)
5 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-10-08 9:52 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
When writing a cruft pack with an expiration, pack-objects first
collects the recent objects and then walks from them to rescue
whatever they reach, expired or not. A pack the caller did not list is
marked kept while collecting, so that its objects are not copied into
the cruft pack, and unmarked before the walk, so that the walk can go
through it.
The walk does not see the unmarking. Whether an object sits in a kept
pack is answered from a cache that is built on first use and only
dropped when asked about a different kind of kept pack. Collecting
builds it while the unlisted pack is still marked, the walk asks the
same kind of question, and so the unlisted pack stays in it: the walk
stops there, and whatever lies beyond it in an expired pack is left
out of the cruft pack, to go when that pack is deleted.
This went unnoticed because of "--honor-pack-keep". repack passes it,
and when there is a ".keep" file it makes the collecting side ask
about on-disk and in-core kept packs together while the walk asks
about in-core ones alone; the cache is rebuilt each time the question
changes, and by accident the walk sees the current marks. Take the
".keep" file away and the objects are lost today. A later commit stops
repack from passing "--honor-pack-keep" at all, so fix this first.
Drop the cache after re-marking. The loop over the object sources
that does so lives in packfile.c, as repo_invalidate_kept_pack_caches(),
next to has_object_kept_pack() which reads the cache. Like it, the
loop assumes every source is a files backend; keeping that assumption
in packfile.c rather than adding it to pack-objects means the two can
move together once packfile management is pushed down into that
backend.
The test builds an unreachable chain whose middle commit sits in a
pack pack-objects is not told about and whose oldest objects have
expired; without the fix the cruft pack holds only the recent tip.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 3 +++
odb/source-packed.h | 3 ++-
packfile.c | 19 +++++++++++++++--
packfile.h | 7 ++++++
t/t5329-pack-objects-cruft.sh | 40 +++++++++++++++++++++++++++++++++++
5 files changed, 69 insertions(+), 3 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6f579173b0..f86b3661c5 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4301,10 +4301,13 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
/*
* Re-mark only the fresh packs as kept so that objects in
* unknown packs do not halt the reachability traversal early.
+ * The kept-pack cache was built while those packs were still
+ * marked, so drop it too.
*/
repo_for_each_pack(the_repository, p)
p->pack_keep_in_core = 0;
mark_pack_kept_in_core(fresh_packs, 1);
+ repo_invalidate_kept_pack_caches(the_repository);
if (prepare_revision_walk(&revs))
die(_("revision walk setup failed"));
diff --git a/odb/source-packed.h b/odb/source-packed.h
index a0f6b5096d..c8c6088a69 100644
--- a/odb/source-packed.h
+++ b/odb/source-packed.h
@@ -25,7 +25,8 @@ struct odb_source_packed {
* Should not be accessed directly, but via
* `packfile_store_get_kept_pack_cache()`. The list of packs gets
* invalidated when the stored flags and the flags passed to
- * `packfile_store_get_kept_pack_cache()` mismatch.
+ * `packfile_store_get_kept_pack_cache()` mismatch, or explicitly via
+ * `repo_invalidate_kept_pack_caches()`.
*/
struct {
struct packed_git **packs;
diff --git a/packfile.c b/packfile.c
index 4fa5fd67c8..399c0622cd 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1870,6 +1870,22 @@ int packfile_fill_entry(struct packed_git *p,
return 1;
}
+static void invalidate_kept_pack_cache(struct odb_source_packed *store)
+{
+ FREE_AND_NULL(store->kept_cache.packs);
+ store->kept_cache.flags = 0;
+}
+
+void repo_invalidate_kept_pack_caches(struct repository *r)
+{
+ struct odb_source *source;
+
+ for (source = r->objects->sources; source; source = source->next) {
+ struct odb_source_files *files = odb_source_files_downcast(source);
+ invalidate_kept_pack_cache(files->packed);
+ }
+}
+
static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
unsigned flags)
{
@@ -1877,8 +1893,7 @@ static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
return;
if (store->kept_cache.flags == flags)
return;
- FREE_AND_NULL(store->kept_cache.packs);
- store->kept_cache.flags = 0;
+ invalidate_kept_pack_cache(store);
}
struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
diff --git a/packfile.h b/packfile.h
index 6d30d15a00..e2db6bfff7 100644
--- a/packfile.h
+++ b/packfile.h
@@ -144,6 +144,13 @@ enum kept_pack_type {
struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
unsigned flags);
+/*
+ * Drop every packfile store's cache of kept packs, so that the next call
+ * to `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after
+ * changing which packs are kept in core.
+ */
+void repo_invalidate_kept_pack_caches(struct repository *r);
+
struct pack_window {
struct pack_window *next;
unsigned char *base;
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index 12cda06373..6302f60b75 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -332,6 +332,46 @@ test_expect_success 'cruft trees rescue sub-trees, blobs' '
)
'
+test_expect_success 'cruft traversal rescues through a pack it was not told about' '
+ git init repo &&
+ test_when_finished "rm -fr repo" &&
+ (
+ cd repo &&
+
+ test_commit packed &&
+ git repack -Ad &&
+ keep="$(basename "$(ls $packdir/pack-*.pack)")" &&
+
+ test_commit old &&
+ test_commit mid &&
+ test_commit new &&
+
+ # "old" has expired, "new" is recent, and "mid" sits in a
+ # pack that pack-objects is not told about. Rescuing "old"
+ # from "new" means walking through that pack.
+ git rev-list --objects --no-object-names packed..old >old &&
+ while read object
+ do
+ test-tool chmtime -1000 \
+ "$objdir/$(test_oid_to_path $object)" || exit 1
+ done <old &&
+ git rev-list --objects --no-object-names old..mid |
+ git pack-objects $packdir/pack >/dev/null &&
+ git prune-packed &&
+
+ cruft="$(echo $keep | git pack-objects --cruft \
+ --cruft-expiration=750.seconds.ago \
+ $packdir/pack)" &&
+ test-tool pack-mtimes "pack-$cruft.mtimes" >actual.raw &&
+
+ cut -d" " -f1 <actual.raw | sort >actual &&
+ git rev-list --objects --no-object-names packed..new >expect.raw &&
+ sort <expect.raw >expect &&
+
+ test_cmp expect actual
+ )
+'
+
test_expect_success 'expired objects are pruned' '
git init repo &&
test_when_finished "rm -fr repo" &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 3/5] pack-objects: sort --keep-pack list for lookup
2026-10-08 9:52 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
2026-10-08 9:52 ` [PATCH v3 1/5] pack-objects: keep --keep-pack open when following Qin ShiCheng via GitGitGadget
2026-10-08 9:52 ` [PATCH v3 2/5] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
@ 2026-10-08 9:52 ` Qin ShiCheng via GitGitGadget
2026-10-08 9:52 ` [PATCH v3 4/5] pack-objects: add --keep-pack-from-file Qin ShiCheng via GitGitGadget
` (2 subsequent siblings)
5 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-10-08 9:52 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
add_extra_kept_packs() scans the whole "--keep-pack" list once per
pack in the repository. That is fine for the handful of names it gets
today, but the next commit lets a caller name every kept pack in the
repository, and with thousands of them the scan dominates: matching
20,000 kept packs against 20,000 names takes 11 seconds here, against
under a second with "--honor-pack-keep".
Sort the list once and look each pack up in it. The comparison stays
fspathcmp(), so what matches does not change.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f86b3661c5..48faef2227 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -5002,7 +5002,7 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
-static void add_extra_kept_packs(const struct string_list *names,
+static void add_extra_kept_packs(struct string_list *names,
enum stdin_packs_mode stdin_packs)
{
struct packed_git *p;
@@ -5010,18 +5010,13 @@ static void add_extra_kept_packs(const struct string_list *names,
if (!names->nr)
return;
- repo_for_each_pack(the_repository, p) {
- const char *name = basename(p->pack_name);
- int i;
+ string_list_sort(names);
+ repo_for_each_pack(the_repository, p) {
if (!p->pack_local)
continue;
- for (i = 0; i < names->nr; i++)
- if (!fspathcmp(name, names->items[i].string))
- break;
-
- if (i < names->nr) {
+ if (string_list_has_string(names, basename(p->pack_name))) {
/*
* When following, treat the pack like a "!" pack, not
* a "^" one: nobody said it is closed under
@@ -5146,7 +5141,9 @@ int cmd_pack_objects(int argc,
int rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;
int rev_list_index = 0;
enum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;
- struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
+ struct string_list keep_pack_list = {
+ .cmp = fspathcmp,
+ };
struct list_objects_filter_options filter_options =
LIST_OBJECTS_FILTER_INIT;
struct repo_config_values *cfg = repo_config_values(the_repository);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 4/5] pack-objects: add --keep-pack-from-file
2026-10-08 9:52 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (2 preceding siblings ...)
2026-10-08 9:52 ` [PATCH v3 3/5] pack-objects: sort --keep-pack list for lookup Qin ShiCheng via GitGitGadget
@ 2026-10-08 9:52 ` Qin ShiCheng via GitGitGadget
2026-10-08 9:52 ` [PATCH v3 5/5] repack: tell pack-objects which packs are kept Qin ShiCheng via GitGitGadget
2026-10-08 19:19 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run Junio C Hamano
5 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-10-08 9:52 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
"--keep-pack" names one pack per occurrence, and there is only so much
room on the command line: ARG_MAX is shared with the environment, and
on Windows the whole line is capped at 32,767 characters, which a few
hundred pack names fill. Past that the spawn fails before pack-objects
has started. fetch-pack grew "--stdin" in 078b895fef (fetch-pack: new
--stdin option to read refs from stdin, 2012-04-02) for the same
reason.
stdin is taken here: every mode repack drives pack-objects in already
uses it, for the revision list under "-a", object names for the
promisor pack, and pack lists for "--stdin-packs" and "--cruft". So
read the names from a file instead, one per line, skipping empty
lines. They go into the same list as the "--keep-pack" names and are
treated exactly alike: matched against local packs, ignored when they
match nothing, and kept open under "--stdin-packs=follow". A relative
path is resolved against the directory the user ran from, as
"--refs-snapshot" of "git multi-pack-index write" is.
The list now holds strings from two sources, so let it own its copies.
repack is about to use this to hand pack-objects its own snapshot of
the packs that have a ".keep" file.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
Documentation/git-pack-objects.adoc | 8 +++++
builtin/pack-objects.c | 28 ++++++++++++++++++
t/t5331-pack-objects-stdin.sh | 46 +++++++++++++++++++++++++++++
3 files changed, 82 insertions(+)
diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc
index 65cd00c152..938e27f69d 100644
--- a/Documentation/git-pack-objects.adoc
+++ b/Documentation/git-pack-objects.adoc
@@ -13,6 +13,7 @@ SYNOPSIS
[--no-reuse-delta] [--delta-base-offset] [--non-empty]
[--local] [--incremental] [--window=<n>] [--depth=<n>]
[--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
+ [--keep-pack-from-file=<file>]
[--cruft] [--cruft-expiration=<time>]
[--stdout [--filter=<filter-spec>] | <base-name>]
[--shallow] [--keep-true-parents] [--[no-]sparse]
@@ -193,6 +194,13 @@ depth is 4095.
leading directory (e.g. `pack-123.pack`). The option could be
specified multiple times to keep multiple packs.
+--keep-pack-from-file=<file>::
+ Read names of packs to keep from `<file>`, one per line, and
+ treat each of them as if it had been given with `--keep-pack`.
+ Empty lines are ignored. This is meant for callers such as
+ linkgit:git-repack[1] that may have to name more packs than fit
+ on a command line.
+
--incremental::
This flag causes an object already in a pack to be ignored
even if it would have otherwise been packed.
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 48faef2227..621b333d0b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -194,6 +194,7 @@ static const char *const pack_usage[] = {
" [--no-reuse-delta] [--delta-base-offset] [--non-empty]\n"
" [--local] [--incremental] [--window=<n>] [--depth=<n>]\n"
" [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]\n"
+ " [--keep-pack-from-file=<file>]\n"
" [--cruft] [--cruft-expiration=<time>]\n"
" [--stdout [--filter=<filter-spec>] | <base-name>]\n"
" [--shallow] [--keep-true-parents] [--[no-]sparse]\n"
@@ -5002,6 +5003,26 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
+/*
+ * Read pack names from the file, one per line, as if each of them had
+ * been given with "--keep-pack".
+ */
+static void read_keep_pack_list(struct string_list *names, const char *path)
+{
+ struct strbuf buf = STRBUF_INIT;
+ FILE *fp = xfopen(path, "r");
+
+ while (strbuf_getline(&buf, fp) != EOF) {
+ if (!buf.len)
+ continue;
+ string_list_append(names, buf.buf);
+ }
+ if (ferror(fp))
+ die_errno(_("could not read '%s'"), path);
+ fclose(fp);
+ strbuf_release(&buf);
+}
+
static void add_extra_kept_packs(struct string_list *names,
enum stdin_packs_mode stdin_packs)
{
@@ -5142,8 +5163,10 @@ int cmd_pack_objects(int argc,
int rev_list_index = 0;
enum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;
struct string_list keep_pack_list = {
+ .strdup_strings = 1,
.cmp = fspathcmp,
};
+ char *keep_pack_from_file = NULL;
struct list_objects_filter_options filter_options =
LIST_OBJECTS_FILTER_INIT;
struct repo_config_values *cfg = repo_config_values(the_repository);
@@ -5228,6 +5251,8 @@ int cmd_pack_objects(int argc,
N_("ignore packs that have companion .keep file")),
OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
N_("ignore this pack")),
+ OPT_FILENAME(0, "keep-pack-from-file", &keep_pack_from_file,
+ N_("ignore the packs named in <file>")),
OPT_INTEGER(0, "compression", &cfg->pack_compression_level,
N_("pack compression level")),
OPT_BOOL(0, "keep-true-parents", &grafts_keep_true_parents,
@@ -5455,6 +5480,8 @@ int cmd_pack_objects(int argc,
if (progress && all_progress_implied)
progress = 2;
+ if (keep_pack_from_file)
+ read_keep_pack_list(&keep_pack_list, keep_pack_from_file);
add_extra_kept_packs(&keep_pack_list, stdin_packs);
if (ignore_packed_keep_on_disk) {
struct packed_git *p;
@@ -5549,6 +5576,7 @@ cleanup:
clear_packing_data(&to_pack);
list_objects_filter_release(&filter_options);
string_list_clear(&keep_pack_list, 0);
+ free(keep_pack_from_file);
strvec_clear(&rp);
return 0;
diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh
index 4e1fde1b08..d590aa4dad 100755
--- a/t/t5331-pack-objects-stdin.sh
+++ b/t/t5331-pack-objects-stdin.sh
@@ -561,4 +561,50 @@ test_expect_success '--stdin-packs with !-delimited pack without follow' '
)
'
+test_expect_success '--keep-pack-from-file names packs to keep' '
+ test_when_finished "rm -fr repo" &&
+
+ git init repo &&
+ (
+ cd repo &&
+ git config set maintenance.auto false &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+
+ A="$(echo A | git pack-objects --revs $packdir/pack)" &&
+ B="$(echo A..B | git pack-objects --revs $packdir/pack)" &&
+ C="$(echo B..C | git pack-objects --revs $packdir/pack)" &&
+ git prune-packed &&
+
+ # Empty lines and names that match no pack are ignored,
+ # as they would be with --keep-pack.
+ cat >keep <<-EOF &&
+ pack-$A.pack
+
+ pack-$B.pack
+ pack-does-not-exist.pack
+ EOF
+
+ P=$(git pack-objects --all --keep-pack=pack-$A.pack \
+ --keep-pack=pack-$B.pack from-argv </dev/null) &&
+ packed_objects from-argv-$P.idx >expect &&
+
+ P=$(git pack-objects --all --keep-pack-from-file=keep \
+ from-file </dev/null) &&
+ packed_objects from-file-$P.idx >actual &&
+ test_cmp expect actual &&
+
+ objects_in_packs $C >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success '--keep-pack-from-file with a missing file' '
+ test_must_fail git pack-objects --stdout \
+ --keep-pack-from-file=does-not-exist </dev/null 2>err &&
+ test_grep "could not open .does-not-exist. for reading" err
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v3 5/5] repack: tell pack-objects which packs are kept
2026-10-08 9:52 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (3 preceding siblings ...)
2026-10-08 9:52 ` [PATCH v3 4/5] pack-objects: add --keep-pack-from-file Qin ShiCheng via GitGitGadget
@ 2026-10-08 9:52 ` Qin ShiCheng via GitGitGadget
2026-10-08 19:19 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run Junio C Hamano
5 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-10-08 9:52 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung, Qin ShiCheng
From: Qin ShiCheng <qeesung@live.com>
repack works out which packs are redundant by looking for ".keep"
files when it starts, then passes "--honor-pack-keep" to the
pack-objects it spawns, which looks for them all over again. Two scans
of the same directory, seconds apart, with nothing holding them
together.
A ".keep" that turns up in between loses objects. The parent did not
see it, so the pack is on its list to delete. The child does see it,
so it leaves that pack's objects out of the replacement. The parent
deletes the pack regardless: repack_remove_redundant_pack() passes
force_delete, which skips the ".keep" check in unlink_pack_path(). The
objects are gone and repack exits successfully.
The gap is easy to land in. index-pack writes its ".keep" before it
renames the packfile into place, so a "git fetch" or a push being
migrated out of its quarantine will do it. Checking for the ".keep"
once more right before deleting would not help: a push holds it for a
fraction of a second, and it may well be gone again by the time
pack-objects has finished.
Hand pack-objects the kept packs we collected at startup and drop
"--honor-pack-keep". Both processes then work from one snapshot, and a
".keep" appearing or disappearing while we run cannot make them
disagree. An earlier commit made sure a pack kept this way is no more
of a boundary to the traversal than a ".keep" file was.
The list goes into a file next to the refs snapshot we already write
for "git multi-pack-index write", and is passed with
"--keep-pack-from-file" to every pack-objects we spawn when
"--pack-kept-objects" is not in effect, which is when
"--honor-pack-keep" used to be. The cruft pack-objects already has the
kept packs on its stdin; the file is redundant there, but it sees the
same list as everybody else. With nothing to keep, no file is written
and nothing is passed, which is what "--honor-pack-keep" came down to
when it found no ".keep".
The names go one per line, so a name with a newline in it cannot be
passed. "--stdin-packs" and "--cruft" have the same limit and die on a
name they cannot find, but "--keep-pack" ignores such a name, and the
two halves of a garbled one could go on to exclude some other pack;
refuse it up front instead.
The user's own "--keep-pack" arguments keep being forwarded, since
they apply either way. write_filtered_pack() had a loop passing the
kept packs too, but without the ".pack" suffix pack-objects compares
against; it goes. Kept packs borrowed from an alternate object
directory were covered by "--honor-pack-keep" and are not by the
snapshot, which only ever held local packs; repack never deletes
those, so their objects now get packed rather than skipped, which
costs room but cannot lose anything.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/repack.c | 15 ++++++++
repack-filtered.c | 3 --
repack.c | 34 ++++++++++++++++--
repack.h | 17 +++++++--
t/t7700-repack.sh | 43 ++++++++++++++++++++++
t/t7703-repack-geometric.sh | 72 +++++++++++++++++++++++++++++++++++++
6 files changed, 177 insertions(+), 7 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index c4360382c1..78bc98c4f1 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -167,6 +167,7 @@ int cmd_repack(int argc,
struct oidset drop_oids = OIDSET_INIT;
struct pack_geometry geometry = { 0 };
struct tempfile *refs_snapshot = NULL;
+ struct tempfile *kept_packs_snapshot = NULL;
int i, ret;
int show_progress;
@@ -456,6 +457,19 @@ int cmd_repack(int argc,
existing.repo = repo;
existing_packs_collect(&existing, &keep_pack_list);
+ if (existing.kept_packs.nr) {
+ struct strbuf path = STRBUF_INIT;
+
+ strbuf_addf(&path, "%s/%s_XXXXXX",
+ repo_get_object_directory(repo), "kept-packs");
+
+ kept_packs_snapshot = xmks_tempfile(path.buf);
+ existing_packs_snapshot_kept(&existing, kept_packs_snapshot);
+ po_args.kept_packs_snapshot =
+ get_tempfile_path(kept_packs_snapshot);
+
+ strbuf_release(&path);
+ }
if (geometry.split_factor) {
if (pack_everything)
@@ -644,6 +658,7 @@ int cmd_repack(int argc,
cruft_po_args.quiet = po_args.quiet;
cruft_po_args.delta_base_offset = po_args.delta_base_offset;
cruft_po_args.pack_kept_objects = 0;
+ cruft_po_args.kept_packs_snapshot = po_args.kept_packs_snapshot;
ret = write_cruft_pack(&opts, cruft_expiration,
combine_cruft_below_size, &names,
diff --git a/repack-filtered.c b/repack-filtered.c
index 869b9fc6e3..db8de9f633 100644
--- a/repack-filtered.c
+++ b/repack-filtered.c
@@ -25,9 +25,6 @@ int write_filtered_pack(const struct write_pack_opts *opts,
strvec_push(&cmd.args, "--stdin-packs");
- for_each_string_list_item(item, &existing->kept_packs)
- strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);
-
cmd.in = -1;
ret = start_command(&cmd);
diff --git a/repack.c b/repack.c
index d2aa58e134..a794486035 100644
--- a/repack.c
+++ b/repack.c
@@ -38,8 +38,9 @@ void prepare_pack_objects(struct child_process *cmd,
strvec_push(&cmd->args, "--quiet");
if (args->delta_base_offset)
strvec_push(&cmd->args, "--delta-base-offset");
- if (!args->pack_kept_objects)
- strvec_push(&cmd->args, "--honor-pack-keep");
+ if (!args->pack_kept_objects && args->kept_packs_snapshot)
+ strvec_pushf(&cmd->args, "--keep-pack-from-file=%s",
+ args->kept_packs_snapshot);
strvec_push(&cmd->args, out);
cmd->git_cmd = 1;
cmd->out = -1;
@@ -167,6 +168,35 @@ void existing_packs_collect(struct existing_packs *existing,
strbuf_release(&buf);
}
+void existing_packs_snapshot_kept(const struct existing_packs *existing,
+ struct tempfile *f)
+{
+ struct string_list_item *item;
+ FILE *out = fdopen_tempfile(f, "w");
+
+ if (!out)
+ die(_("could not open tempfile %s for writing"),
+ get_tempfile_path(f));
+
+ for_each_string_list_item(item, &existing->kept_packs) {
+ /*
+ * A newline would split the name in two, and pack-objects
+ * quietly keeps whichever packs the halves happen to name.
+ */
+ if (strchr(item->string, '\n'))
+ die(_("cannot keep pack '%s': its name contains a newline"),
+ item->string);
+ fprintf(out, "%s.pack\n", item->string);
+ }
+
+ if (close_tempfile_gently(f)) {
+ int save_errno = errno;
+ delete_tempfile(&f);
+ errno = save_errno;
+ die_errno(_("could not close kept packs snapshot tempfile"));
+ }
+}
+
int existing_packs_has_non_kept(const struct existing_packs *existing)
{
return existing->non_kept_packs.nr || existing->cruft_packs.nr;
diff --git a/repack.h b/repack.h
index 61e554e4ed..1c0aeca3e8 100644
--- a/repack.h
+++ b/repack.h
@@ -19,6 +19,14 @@ struct pack_objects_args {
int path_walk;
int delta_base_offset;
int pack_kept_objects;
+ /*
+ * File naming the packs to leave alone, one "<name>.pack" per line;
+ * NULL when there are none. pack-objects reads it rather than
+ * looking for ".keep" files itself, so that a ".keep" created or
+ * removed while we run cannot make the two of us disagree over
+ * which packs are being repacked.
+ */
+ const char *kept_packs_snapshot;
struct list_objects_filter_options filter_options;
};
@@ -28,6 +36,7 @@ struct pack_objects_args {
}
struct child_process;
+struct tempfile;
void prepare_pack_objects(struct child_process *cmd,
const struct pack_objects_args *args,
@@ -79,6 +88,12 @@ struct existing_packs {
*/
void existing_packs_collect(struct existing_packs *existing,
const struct string_list *extra_keep);
+/*
+ * Writes the names of the kept packs, one "<name>.pack" per line, into
+ * the given tempfile, for pack-objects to read with --keep-pack-from-file.
+ */
+void existing_packs_snapshot_kept(const struct existing_packs *existing,
+ struct tempfile *f);
int existing_packs_has_non_kept(const struct existing_packs *existing);
int existing_pack_is_marked_for_deletion(struct string_list_item *item);
void existing_packs_retain_cruft(struct existing_packs *existing,
@@ -138,8 +153,6 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,
bool wrote_incremental_midx);
void pack_geometry_release(struct pack_geometry *geometry);
-struct tempfile;
-
enum repack_write_midx_mode {
REPACK_WRITE_MIDX_NONE,
REPACK_WRITE_MIDX_DEFAULT,
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f0a390e3c6..845f032bea 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -254,6 +254,49 @@ test_expect_success 'repack --keep-pack' '
)
'
+test_expect_success 'repack --keep-pack with --pack-kept-objects' '
+ test_create_repo keep-pack-kept-objects &&
+ (
+ cd keep-pack-kept-objects &&
+ git config pack.window 0 &&
+ git config maintenance.auto false &&
+ P1=$(commit_and_pack 1) &&
+ P2=$(commit_and_pack 2) &&
+
+ # "--pack-kept-objects" is about packs that have a ".keep"
+ # file. A pack named with "--keep-pack" stays out of the
+ # result regardless, objects included.
+ git repack -a -d --pack-kept-objects --keep-pack $P1 &&
+ ls .git/objects/pack/*.pack >counts &&
+ test_line_count = 2 counts &&
+ test-tool find-pack -c 1 HEAD~1 &&
+ test-tool find-pack -c 1 HEAD~1: &&
+ git fsck
+ )
+'
+
+test_expect_success FUNNYNAMES 'a kept pack whose name has a newline is refused' '
+ test_create_repo keep-pack-newline &&
+ (
+ cd keep-pack-newline &&
+ git config maintenance.auto false &&
+ test_commit base &&
+ git repack -ad &&
+
+ # The names pack-objects is told to keep go one per line, so
+ # this one would come out as two, and the first of them is
+ # the name of the pack holding everything else.
+ victim="$(basename "$(ls .git/objects/pack/pack-*.pack)")" &&
+ name="$(printf "%s\nother" "$victim")" &&
+ P=$(git rev-parse HEAD | git pack-objects ".git/objects/pack/$name") &&
+ >".git/objects/pack/$name-$P.keep" &&
+
+ test_must_fail git repack -ad 2>err &&
+ test_grep "contains a newline" err &&
+ git fsck
+ )
+'
+
test_expect_success 'repacking fails when missing .pack actually means missing objects' '
test_create_repo idx-without-pack &&
(
diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index f3a0650cfe..6b914a2a80 100755
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -541,4 +541,76 @@ test_expect_success 'geometric repack works with promisor packs' '
)
'
+test_expect_success 'a ".keep" that shows up mid-repack does not lose objects' '
+ test_when_finished "rm -fr race" &&
+ git init race &&
+ (
+ cd race &&
+
+ test_commit kept &&
+ test_commit pack &&
+
+ KEPT=$(git pack-objects --revs $packdir/pack <<-EOF
+ refs/tags/kept
+ EOF
+ ) &&
+ git pack-objects --revs $packdir/pack <<-EOF &&
+ refs/tags/pack
+ ^refs/tags/kept
+ EOF
+ git prune-packed &&
+
+ # Neither pack is twice the size of the other, so both are
+ # redundant and get deleted. Have a ".keep" appear on one of
+ # them as pack-objects starts, after the repack has decided
+ # to delete it: pack-objects used to notice the ".keep" and
+ # leave those objects out of the replacement pack.
+ mkdir shim &&
+ write_script shim/git <<-EOF &&
+ test "\$1" = "pack-objects" && >"$(pwd)/$packdir/pack-$KEPT.keep"
+ GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
+ EOF
+
+ git --exec-path="$(pwd)/shim" repack --geometric 2 -d &&
+
+ git fsck
+ )
+'
+
+test_expect_success 'a kept pack does not stop the traversal from rescuing objects' '
+ test_when_finished "rm -fr kept-open" &&
+ git init kept-open &&
+ (
+ cd kept-open &&
+ git config repack.midxMustContainCruft false &&
+
+ test_commit a &&
+ test_commit b &&
+ b=$(git rev-parse b) &&
+ git repack -ad &&
+
+ # Make "b" unreachable and sweep it, together with its tree
+ # and blob, into a cruft pack.
+ git tag -d b &&
+ git reset --hard a &&
+ git reflog expire --all --expire=all &&
+ git repack -ad --cruft &&
+
+ # Bring the commit back on its own, in a pack marked as kept.
+ # Its tree and blob are still only in the cruft pack.
+ kept=$(echo $b | git pack-objects $packdir/pack) &&
+ >$packdir/pack-$kept.keep &&
+
+ # Build on top of it, so that the repack has to look through
+ # the kept pack to find out what the new commit depends on.
+ git update-ref refs/heads/master \
+ $(git commit-tree a^{tree} -p $b -m c) &&
+
+ git repack --geometric 2 -d --write-midx --write-bitmap-index &&
+ test_path_is_file $packdir/multi-pack-index &&
+ ls $packdir/multi-pack-index-*.bitmap >bitmaps &&
+ test_line_count = 1 bitmaps
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run
2026-10-08 9:52 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run qeesung via GitGitGadget
` (4 preceding siblings ...)
2026-10-08 9:52 ` [PATCH v3 5/5] repack: tell pack-objects which packs are kept Qin ShiCheng via GitGitGadget
@ 2026-10-08 19:19 ` Junio C Hamano
2026-10-09 3:07 ` Qin ShiCheng
5 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2026-10-08 19:19 UTC (permalink / raw)
To: qeesung via GitGitGadget
Cc: git, Patrick Steinhardt, Taylor Blau, Justin Tobler, qeesung
"qeesung via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Interaction with topics in seen:
>
> * ps/odb-files-alternates turns the list of object sources into a single
> files source with a list of object directories, so
> repo_invalidate_kept_pack_caches() from 2/5 needs to walk those instead.
> The textual merge is clean, but the build breaks; this resolution follows
> has_object_kept_pack() on that topic:
>
> void repo_invalidate_kept_pack_caches(struct repository *r) { struct
> odb_source_files *files = odb_source_files_downcast(r->objects->source);
> for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
> invalidate_kept_pack_cache(dir->packed); }
I do not know if you meant to cram a function on four lines this
way, but I suspect that it may be easier for everybody to stop and
wait until the other topic solidifies a bit more, and then create a
synthetic base that merges the other topic into the tip of 'master'
and rebase these five patches on top of the resulting merge.
I wonder how close ps/odb-files-alternates topic is to the finish
line? Karthik did read through the initial round and then gave a
thumbs up on the current iteration. We would benefit from a
different set of eyes on the series [*].
Thanks.
[Reference]
* <CAOLa=ZTjrzNbuvZ-kr6k5TZSMyGGMFTb4iar6DZwdnCDvUrH9Q@mail.gmail.com>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run
2026-10-08 19:19 ` [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run Junio C Hamano
@ 2026-10-09 3:07 ` Qin ShiCheng
0 siblings, 0 replies; 26+ messages in thread
From: Qin ShiCheng @ 2026-10-09 3:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, Taylor Blau, Justin Tobler
Junio C Hamano <gitster@pobox.com> writes:
> I do not know if you meant to cram a function on four lines this
> way,
I did not; GitGitGadget reflowed the indented block in the PR
description. It was meant to read
void repo_invalidate_kept_pack_caches(struct repository *r)
{
struct odb_source_files *files = odb_source_files_downcast(r->objects->source);
for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
invalidate_kept_pack_cache(dir->packed);
}
> but I suspect that it may be easier for everybody to stop and
> wait until the other topic solidifies a bit more, and then create a
> synthetic base that merges the other topic into the tip of 'master'
> and rebase these five patches on top of the resulting merge.
Makes sense. I will hold off until ps/odb-files-alternates settles,
then send v4 on top of master with that topic merged in, and say so
in the cover letter.
Thanks,
Qin
^ permalink raw reply [flat|nested] 26+ messages in thread