* [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
` (7 subsequent siblings)
8 siblings, 1 reply; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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
` (6 subsequent siblings)
8 siblings, 0 replies; 31+ 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] 31+ 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
` (5 subsequent siblings)
8 siblings, 0 replies; 31+ 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] 31+ 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
` (4 subsequent siblings)
8 siblings, 0 replies; 31+ 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] 31+ 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
` (3 subsequent siblings)
8 siblings, 0 replies; 31+ 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] 31+ 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
` (2 subsequent siblings)
8 siblings, 0 replies; 31+ 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] 31+ 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
2026-10-26 9:06 ` [PATCH v4 0/3] " qeesung via GitGitGadget
8 siblings, 5 replies; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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)
2026-10-26 9:06 ` [PATCH v4 0/3] " qeesung via GitGitGadget
8 siblings, 6 replies; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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; 31+ 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] 31+ 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
2026-10-09 15:11 ` Junio C Hamano
0 siblings, 1 reply; 31+ 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] 31+ messages in thread* Re: [PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run
2026-10-09 3:07 ` Qin ShiCheng
@ 2026-10-09 15:11 ` Junio C Hamano
0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2026-10-09 15:11 UTC (permalink / raw)
To: Qin ShiCheng; +Cc: git, Patrick Steinhardt, Taylor Blau, Justin Tobler
Qin ShiCheng <qeesung@live.com> writes:
> 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. In the meantime you can participate in reviewing Patrick's
topic, of course, and that may help it move forward a bit faster ;-)
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v4 0/3] 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
` (7 preceding siblings ...)
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-26 9:06 ` qeesung via GitGitGadget
2026-10-26 9:06 ` [PATCH v4 1/3] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
` (2 more replies)
8 siblings, 3 replies; 31+ messages in thread
From: qeesung via GitGitGadget @ 2026-10-26 9:06 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Taylor Blau, Junio C Hamano, Justin Tobler,
qeesung
"git repack -d" could lose objects when a ".keep" file appeared on a pack it
had already decided to delete. repack now tells pack-objects which packs it
may delete, and pack-objects ignores ".keep" files on those packs.
We hit this in production (a ref pointing at a missing commit, on git 2.43),
and it reproduces on master:
* repack scans for ".keep" files and decides which packs to delete;
pack-objects scans again when it opens the packs;
* in between, a fetch or a push brings in a pack identical to one of those,
and a ".keep" ends up next to it;
* pack-objects then leaves that pack's objects out, or does not loosen or
keep them for -A, -k and --cruft, and repack deletes the pack anyway.
v3 passed pack-objects the kept packs repack saw at startup instead of
--honor-pack-keep. That missed -A, -k and --cruft, which skip packs with a
".keep" file regardless of that option. v4 goes the other way:
* 1/3 fixes a stale kept-pack cache in the cruft walk. It reproduces on
master, but under --honor-pack-keep any ".keep" file happens to hide it,
including the one the race leaves; 3/3 takes that one away, so this comes
first.
* 2/3 adds "pack-objects --ignore-pack-keep-from=", which treats the packs
named in the file as having no ".keep".
* 3/3 makes "repack -d" pass the packs it may delete that way.
Each fix has a test that fails without it; the race is reproduced in t7700
and t7703. The full suite passes.
Known limitations:
* Unreachable objects of a deleted pack are still dropped by "-a -d" and by
the expiration options, as before. A pack written again under the same
name keeps its old mtime, so objects a push brings back that way can
still expire. Freshening the pack on such a collision would help, but
that is for a separate series.
* The cruft walk now goes through the packs of an alternate, as it must
(see 1/3). On a "clone --shared" fork of git.git this took the walk in
"repack -d -l --cruft" from about 2ms to about 3.3s, which is what master
already does when a ".keep" file is around.
This is based on master, now that ps/odb-files-alternates is in. It merges
cleanly with next and seen (with v3 reverted), and the repack tests pass
there. jt/odb-keep-lockfile-fix, Justin's fix for the receive-pack side that
v1 had a patch for, leaves the race as it is.
Changes since v3:
* Redesigned as above. v3's 1/5, 3/5 and 4/5 are dropped, since nothing
passes --keep-pack any more; 2/3 and 3/3 are new.
* 1/3 (v3's 2/5) also lets the walk go through --keep-pack packs while
still leaving their objects out, and its message explains why the walk
goes through alternates. There are tests for both.
Qin ShiCheng (3):
pack-objects: reset kept-pack cache for cruft walk
pack-objects: add --ignore-pack-keep-from
repack: tell pack-objects which packs it may delete
Documentation/git-pack-objects.adoc | 12 ++++
builtin/pack-objects.c | 55 +++++++++++++++-
builtin/repack.c | 23 +++++++
odb/source-packed.h | 3 +-
packfile.c | 18 +++++-
packfile.h | 7 +++
repack.c | 26 ++++++++
repack.h | 14 ++++-
t/t5329-pack-objects-cruft.sh | 98 +++++++++++++++++++++++++++++
t/t7700-repack.sh | 86 +++++++++++++++++++++++++
t/t7703-repack-geometric.sh | 39 ++++++++++++
11 files changed, 375 insertions(+), 6 deletions(-)
base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2219%2Fqeesung%2Frepack-kept-packs-snapshot-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2219/qeesung/repack-kept-packs-snapshot-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/2219
Range-diff vs v3:
1: 8cf72312c5 < -: ---------- pack-objects: keep --keep-pack open when following
2: 58019e4983 ! 1: 8cce2b6eb5 pack-objects: reset kept-pack cache for cruft walk
@@ Commit message
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.
+ out of the cruft pack, to go when that pack is deleted. Whether that
+ happens depends on whether the question changes along the way, which
+ "--honor-pack-keep" does whenever there is a ".keep" file around.
- 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 unmarking. Only packfile.c is to touch it, so
+ add repo_invalidate_kept_pack_caches() there, which drops it for every
+ object directory.
- 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.
+ With the cache gone, the walk goes through every pack but the listed
+ fresh ones, as was intended, including packs borrowed from an
+ alternate. It has to: an object there may refer to a local one, for
+ example when two repositories borrow from each other, or when "git
+ repack --filter-to" has moved some objects to an alternate and left
+ the ones they refer to behind. Without "--local", whatever the walk
+ finds there is copied into the cruft pack, as it already is for loose
+ objects of an alternate; with "--local", nothing is. Either way, the
+ walk may now go through a history far bigger than the repository's
+ own, as it already did whenever there was a ".keep" file around.
- 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.
+ It also includes packs given with "--keep-pack", whose objects are to
+ be left out. Turn those into "open" kept packs, as
+ "--stdin-packs=follow" does for its "!" packs: the walk goes through
+ them, and their objects are still left out. This is how an unlisted
+ pack with a ".keep" file is treated under "--honor-pack-keep".
+
+ With "--keep-pack", the walk and the check that leaves the objects of
+ such a pack out ask about different kinds of kept packs, so the cache
+ is rebuilt each time they take turns, as already happens under
+ "--honor-pack-keep" whenever there is a ".keep" file around. repack
+ never gets here with "--keep-pack": it lists the packs it keeps as
+ fresh ones instead.
+
+ The tests walk from a recent commit to an expired one through a pack
+ pack-objects is not told about, through one given with "--keep-pack",
+ and through one of an alternate holding trees whose blobs "git repack
+ --filter-to" left behind, with and without "--local".
Signed-off-by: Qin ShiCheng <qeesung@live.com>
@@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct
/*
* 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.
++ * A pack given with --keep-pack must not halt it either, but as an
++ * "open" kept pack its objects are still left out. The kept-pack
++ * cache was built while the packs were still marked, so drop it
++ * too.
*/
- repo_for_each_pack(the_repository, p)
+- repo_for_each_pack(the_repository, p)
++ repo_for_each_pack(the_repository, p) {
p->pack_keep_in_core = 0;
++ if (p->pack_keep_in_core_open)
++ ignore_packed_keep_in_core_open = 1;
++ }
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"));
+@@ builtin/pack-objects.c: static void read_cruft_objects(void)
+
+ if (item) {
+ item->util = p;
++ } else if (p->pack_keep_in_core) {
++ /*
++ * Only --keep-pack marks a pack as kept this early.
++ * Make it an "open" kept pack as well, so that its
++ * objects are still left out once it is unmarked
++ * before the traversal; until then this has no effect.
++ */
++ p->pack_keep_in_core_open = 1;
+ } else {
+ /*
+ * This pack wasn't mentioned in either the "fresh" or
## odb/source-packed.h ##
@@ odb/source-packed.h: struct odb_source_packed {
@@ packfile.c: int packfile_fill_entry(struct packed_git *p,
+
+void repo_invalidate_kept_pack_caches(struct repository *r)
+{
-+ struct odb_source *source;
++ struct odb_source_files *files =
++ odb_source_files_downcast(r->objects->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);
-+ }
++ for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
++ invalidate_kept_pack_cache(dir->packed);
+}
+
static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
@@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft trees rescue sub-trees
)
'
-+test_expect_success 'cruft traversal rescues through a pack it was not told about' '
++# Build the history "packed", "old", "mid", "new" in the current repository:
++# "packed" is in the pack named in "in", "old" has expired, "mid" is in the
++# pack whose hash is in $mid and "new" is recent. The objects of packed..new
++# end up in "all.objects".
++cruft_chain () {
++ test_commit packed &&
++ git repack -Ad &&
++ basename "$(ls $packdir/pack-*.pack)" >in &&
++
++ test_commit old &&
++ test_commit mid &&
++ test_commit new &&
++
++ git rev-list --objects --no-object-names packed..old >old &&
++ while read object
++ do
++ test-tool chmtime -1000 \
++ "$objdir/$(test_oid_to_path $object)" || return 1
++ done <old &&
++ git rev-list --objects --no-object-names old..mid >mid.objects &&
++ mid=$(git pack-objects $packdir/pack <mid.objects) &&
++ git prune-packed &&
++ git rev-list --objects --no-object-names packed..new >all.objects
++}
++
++test_expect_success 'cruft traversal goes through a pack it was not told about' '
+ git init repo &&
+ test_when_finished "rm -fr repo" &&
+ (
+ cd repo &&
++ cruft_chain &&
+
-+ test_commit packed &&
-+ git repack -Ad &&
-+ keep="$(basename "$(ls $packdir/pack-*.pack)")" &&
-+
-+ test_commit old &&
-+ test_commit mid &&
-+ test_commit new &&
++ # "old" can only be rescued from "new" by walking through
++ # the "mid" pack.
++ cruft=$(git pack-objects --cruft \
++ --cruft-expiration=750.seconds.ago $packdir/pack <in) &&
++ test-tool pack-mtimes "pack-$cruft.mtimes" >actual.raw &&
++ cut -d" " -f1 <actual.raw | sort >actual &&
++ sort <all.objects >expect &&
++ test_cmp expect actual
++ )
++'
+
-+ # "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 &&
++test_expect_success 'cruft traversal goes through a --keep-pack pack' '
++ git init repo &&
++ test_when_finished "rm -fr repo" &&
++ (
++ cd repo &&
++ cruft_chain &&
+
-+ cruft="$(echo $keep | git pack-objects --cruft \
++ # The same, but with "mid" in a pack given with --keep-pack:
++ # "old" is still rescued, and none of the objects of that
++ # pack are copied.
++ cruft=$(git pack-objects --cruft \
+ --cruft-expiration=750.seconds.ago \
-+ $packdir/pack)" &&
++ --keep-pack=pack-$mid.pack $packdir/pack <in) &&
+ 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 &&
-+
++ sort <all.objects >all.sorted &&
++ sort <mid.objects >mid.sorted &&
++ comm -23 all.sorted mid.sorted >expect &&
+ test_cmp expect actual
+ )
+'
++
++for opt in "" --local
++do
++ test_expect_success "cruft traversal goes through an alternate${opt:+ ($opt)}" '
++ test_when_finished "rm -fr repo alt.git" &&
++ git init repo &&
++ git init --bare alt.git &&
++ echo "$(pwd)/alt.git/objects" >repo/.git/objects/info/alternates &&
++ test_commit -C repo base &&
++ test_commit -C repo old &&
++ tree=$(git -C repo rev-parse old^{tree}) &&
++ blob=$(git -C repo rev-parse old:old.t) &&
++
++ # Move everything but the blobs to the alternate, then let
++ # "old" expire: its tree is in the alternate, but its blob
++ # is in an old local pack.
++ git -C repo repack -a -d &&
++ git -C repo repack -a -d --filter=object:type=blob \
++ --filter-to="$(pwd)/alt.git/objects/pack/pack" &&
++ git -C repo tag -d old &&
++ git -C repo reset --hard base &&
++ git -C repo reflog expire --all --expire=all &&
++ test-tool chmtime -10000 repo/$packdir/pack-*.pack &&
++
++ # A recent unreachable commit in "repo" keeps the blob, but only
++ # if the traversal goes through the tree in the alternate, which
++ # it has to do even with --local.
++ outer=$(printf "040000 tree %s\tdir\n" $tree | git -C repo mktree) &&
++ git -C repo commit-tree -m recent $outer >/dev/null &&
++ git -C repo repack --cruft --cruft-expiration=1.hour.ago -d $opt &&
++ git -C repo cat-file -e $blob
++ '
++done
+
test_expect_success 'expired objects are pruned' '
git init repo &&
3: ff2146d002 < -: ---------- pack-objects: sort --keep-pack list for lookup
4: 148175cfa0 ! 2: 91ffbdf4fa pack-objects: add --keep-pack-from-file
@@ Metadata
Author: Qin ShiCheng <qeesung@live.com>
## Commit message ##
- pack-objects: add --keep-pack-from-file
+ pack-objects: add --ignore-pack-keep-from
- "--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.
+ repack decides which packs to delete before it runs pack-objects, but
+ pack-objects looks for ".keep" files on its own when it opens the
+ packs. A ".keep" that appears in between, for example from a push or
+ fetch whose index-pack wrote a pack identical to an existing one, makes
+ pack-objects treat as kept a pack that repack is about to delete. Its
+ objects are then left out of the new pack, or not loosened, or not
+ kept as unreachable, and they are gone once repack deletes the pack.
- 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.
+ Not all of that goes through "--honor-pack-keep": loosening objects
+ for "--unpack-unreachable", and collecting them for
+ "--keep-unreachable" and "--cruft", skip packs with a ".keep" file
+ regardless of it.
- The list now holds strings from two sources, so let it own its copies.
+ Let the caller name such packs. "--ignore-pack-keep-from=<file>" reads
+ names of local packs, one per line, given and matched as for
+ "--keep-pack", and treats them as if they had no ".keep" file. All of
+ the above go by p->pack_keep, which is read from the disk once, when a
+ pack is first seen, so clearing it for the named packs before anything
+ looks at it covers every one of them. Other packs, including any that
+ show up later, keep whatever their ".keep" files say.
- repack is about to use this to hand pack-objects its own snapshot of
- the packs that have a ".keep" file.
+ The names come from a file because there can be more of them than fit
+ on a command line (on Windows, the whole command line is limited to
+ 32767 characters), and the standard input is already taken in every
+ mode repack uses.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
@@ Documentation/git-pack-objects.adoc: 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>]
++ [--ignore-pack-keep-from=<file>]
[--cruft] [--cruft-expiration=<time>]
[--stdout [--filter=<filter-spec>] | <base-name>]
[--shallow] [--keep-true-parents] [--[no-]sparse]
@@ Documentation/git-pack-objects.adoc: 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.
++--ignore-pack-keep-from=<file>::
++ Read names of local packs from `<file>`, one per line in the
++ same form as for `--keep-pack`, and treat them as if they had
++ no .keep file, both for `--honor-pack-keep` and when deciding
++ which objects to loosen or keep with `--unpack-unreachable`,
++ `--keep-unreachable` and `--cruft`.
++ This is for callers like linkgit:git-repack[1] that are about
++ to delete those packs: a .keep file that appears on one of them
++ after the caller decided to delete it must not cause its objects
++ to be left out.
+
--incremental::
This flag causes an object already in a pack to be ignored
@@ builtin/pack-objects.c: 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"
++ " [--ignore-pack-keep-from=<file>]\n"
" [--cruft] [--cruft-expiration=<time>]\n"
" [--stdout [--filter=<filter-spec>] | <base-name>]\n"
" [--shallow] [--keep-true-parents] [--[no-]sparse]\n"
@@ builtin/pack-objects.c: static void get_object_list(struct rev_info *revs, struc
}
+/*
-+ * Read pack names from the file, one per line, as if each of them had
-+ * been given with "--keep-pack".
++ * Treat the local packs named in "path" as if they had no ".keep" file.
++ * The names are matched like those given with --keep-pack. This has to
++ * happen before anything looks at p->pack_keep, which is read from the
++ * disk only once, when the pack is first seen.
+ */
-+static void read_keep_pack_list(struct string_list *names, const char *path)
++static void ignore_pack_keep_from_file(const char *path)
+{
++ struct string_list names = STRING_LIST_INIT_DUP;
+ struct strbuf buf = STRBUF_INIT;
++ struct packed_git *p;
+ FILE *fp = xfopen(path, "r");
+
-+ while (strbuf_getline(&buf, fp) != EOF) {
-+ if (!buf.len)
-+ continue;
-+ string_list_append(names, buf.buf);
-+ }
++ names.cmp = fspathcmp;
++ while (strbuf_getline(&buf, fp) != EOF)
++ string_list_append(&names, buf.buf);
+ if (ferror(fp))
+ die_errno(_("could not read '%s'"), path);
+ fclose(fp);
++ string_list_sort(&names);
++
++ repo_for_each_pack(the_repository, p)
++ if (p->pack_local &&
++ string_list_has_string(&names, pack_basename(p)))
++ p->pack_keep = 0;
++
++ string_list_clear(&names, 0);
+ strbuf_release(&buf);
+}
+
- static void add_extra_kept_packs(struct string_list *names,
- enum stdin_packs_mode stdin_packs)
+ static void add_extra_kept_packs(const struct string_list *names)
{
+ struct packed_git *p;
@@ builtin/pack-objects.c: 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 string_list keep_pack_list = STRING_LIST_INIT_NODUP;
++ char *ignore_pack_keep_from = NULL;
struct list_objects_filter_options filter_options =
LIST_OBJECTS_FILTER_INIT;
struct repo_config_values *cfg = repo_config_values(the_repository);
@@ builtin/pack-objects.c: 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_FILENAME(0, "ignore-pack-keep-from", &ignore_pack_keep_from,
++ N_("ignore .keep files of 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,
@@ builtin/pack-objects.c: 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_pack_keep_from)
++ ignore_pack_keep_from_file(ignore_pack_keep_from);
+ add_extra_kept_packs(&keep_pack_list);
if (ignore_packed_keep_on_disk) {
struct packed_git *p;
@@ builtin/pack-objects.c: cleanup:
clear_packing_data(&to_pack);
list_objects_filter_release(&filter_options);
string_list_clear(&keep_pack_list, 0);
-+ free(keep_pack_from_file);
++ free(ignore_pack_keep_from);
strvec_clear(&rp);
return 0;
- ## t/t5331-pack-objects-stdin.sh ##
-@@ t/t5331-pack-objects-stdin.sh: test_expect_success '--stdin-packs with !-delimited pack without follow' '
- )
+ ## t/t7700-repack.sh ##
+@@ t/t7700-repack.sh: test_expect_success 'objects made unreachable by grafts only are kept' '
+ git cat-file -t $H1
'
-+test_expect_success '--keep-pack-from-file names packs to keep' '
-+ test_when_finished "rm -fr repo" &&
-+
-+ git init repo &&
++test_expect_success 'pack-objects --ignore-pack-keep-from' '
++ test_when_finished "rm -fr ignore-keep" &&
++ git init ignore-keep &&
+ (
-+ cd repo &&
-+ git config set maintenance.auto false &&
-+
-+ test_commit A &&
-+ test_commit B &&
-+ test_commit C &&
++ cd ignore-keep &&
++ test_commit base &&
++ unreachable=$(git commit-tree -m unreachable HEAD^{tree}) &&
+
-+ 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 rev-list --objects --all $unreachable >objects &&
++ P=$(git pack-objects .git/objects/pack/pack <objects) &&
++ test_commit kept &&
++ git rev-list --objects base..kept >objects &&
++ K=$(git pack-objects .git/objects/pack/pack <objects) &&
+ git prune-packed &&
++ >.git/objects/pack/pack-$P.keep &&
++ >.git/objects/pack/pack-$K.keep &&
++ echo pack-$P.pack >ignore &&
+
-+ # 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
++ # Both packs have a .keep file, but only P is named in
++ # "ignore": its reachable objects are packed and the others
++ # are loosened, while K is left alone.
++ git pack-objects --honor-pack-keep --all --reflog \
++ --indexed-objects --unpack-unreachable \
++ --ignore-pack-keep-from=ignore \
++ .git/objects/pack/pack </dev/null &&
++ test-tool find-pack --check-count 2 base &&
++ test-tool find-pack --check-count 1 kept &&
++ test_path_is_file .git/objects/$(test_oid_to_path $unreachable)
+ )
+'
+
-+test_expect_success '--keep-pack-from-file with a missing file' '
++test_expect_success 'pack-objects --ignore-pack-keep-from with a missing file' '
+ test_must_fail git pack-objects --stdout \
-+ --keep-pack-from-file=does-not-exist </dev/null 2>err &&
++ --ignore-pack-keep-from=does-not-exist </dev/null 2>err &&
+ test_grep "could not open .does-not-exist. for reading" err
+'
+
- test_done
+ test_expect_success 'repack --keep-pack' '
+ test_create_repo keep-pack &&
+ (
5: 8f3b9d9ee0 ! 3: 001875455b repack: tell pack-objects which packs are kept
@@ Metadata
Author: Qin ShiCheng <qeesung@live.com>
## Commit message ##
- repack: tell pack-objects which packs are kept
+ repack: tell pack-objects which packs it may delete
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.
+ files when it starts, then runs pack-objects, which looks for them all
+ over again when it opens the packs. 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.
+ so it leaves that pack to itself: its objects are not copied into the
+ new pack, not loosened for "-A", not kept for "-k" or "--cruft". The
+ parent deletes the pack regardless, as 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.
+ The gap is easy to land in. index-pack names a pack after its contents
+ and writes its ".keep" before it renames the packfile into place, so a
+ "git fetch", or a push being migrated out of its quarantine, that
+ brings in a pack identical to one we already have puts a ".keep" next
+ to that pack. 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.
+ When we may delete packs, that is with "-d" and either "-a", "-A",
+ "--cruft" or "--geometric", write the names of the packs we may
+ delete, the non-kept and cruft packs, to a file next to the refs
+ snapshot we already write for "git multi-pack-index write", and pass it
+ to every pack-objects we spawn with "--ignore-pack-keep-from".
+ pack-objects then never treats those packs as kept, whatever appears
+ on the disk. Every other pack, whether kept when we started or showing
+ up later, still goes by its ".keep" file, so "--honor-pack-keep"
+ stays. Otherwise nothing gets deleted and nothing is passed.
- 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 file has one name per line. A name with a newline in it gets split
+ there, which at worst makes some other pack not count as kept: its
+ objects may be copied, but none are lost.
- 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.
+ Unreachable objects of such a pack are still dropped by "-a -d", or by
+ "--cruft-expiration" and "--unpack-unreachable" once they are old
+ enough, as these drop unreachable objects by design, and a pack written
+ again under the same name does not make them any younger; what changes
+ is that the objects that are to be kept no longer go with them.
- 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.
+ The tests have a ".keep" appear on the pack about to be deleted as
+ pack-objects starts, through a "git" wrapper found via "--exec-path",
+ for "-a", "-A", "-k", "--cruft" with and without an expiration,
+ "--pack-kept-objects", "--filter" and "--geometric".
Signed-off-by: Qin ShiCheng <qeesung@live.com>
## builtin/repack.c ##
@@ builtin/repack.c: 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;
-
-@@ builtin/repack.c: int cmd_repack(int argc,
-
existing.repo = repo;
existing_packs_collect(&existing, &keep_pack_list);
-+ if (existing.kept_packs.nr) {
+
++ /*
++ * Tell pack-objects which packs we may delete. pack-objects looks
++ * for ".keep" files on its own, and one that appears on such a
++ * pack from now on must not make it leave out objects that would
++ * then be deleted with the pack.
++ */
++ if (delete_redundant &&
++ ((pack_everything & ALL_INTO_ONE) || geometry.split_factor) &&
++ existing_packs_has_non_kept(&existing)) {
+ struct strbuf path = STRBUF_INIT;
++ struct tempfile *non_kept_packs;
+
-+ strbuf_addf(&path, "%s/%s_XXXXXX",
-+ repo_get_object_directory(repo), "kept-packs");
++ strbuf_addf(&path, "%s/tmp_non_kept_packs_XXXXXX",
++ repo_get_object_directory(repo));
+
-+ 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);
++ non_kept_packs = xmks_tempfile(path.buf);
++ existing_packs_write_non_kept(&existing, non_kept_packs);
++ po_args.ignore_pack_keep_from = get_tempfile_path(non_kept_packs);
+
+ strbuf_release(&path);
+ }
-
++
if (geometry.split_factor) {
if (pack_everything)
+ die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a");
@@ builtin/repack.c: 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;
++ cruft_po_args.ignore_pack_keep_from = po_args.ignore_pack_keep_from;
ret = write_cruft_pack(&opts, cruft_expiration,
combine_cruft_below_size, &names,
- ## repack-filtered.c ##
-@@ repack-filtered.c: 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);
-
## repack.c ##
@@ repack.c: 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);
+ if (!args->pack_kept_objects)
+ strvec_push(&cmd->args, "--honor-pack-keep");
++ if (args->ignore_pack_keep_from)
++ strvec_pushf(&cmd->args, "--ignore-pack-keep-from=%s",
++ args->ignore_pack_keep_from);
strvec_push(&cmd->args, out);
cmd->git_cmd = 1;
cmd->out = -1;
-@@ repack.c: void existing_packs_collect(struct existing_packs *existing,
- strbuf_release(&buf);
+@@ repack.c: int existing_packs_has_non_kept(const struct existing_packs *existing)
+ return existing->non_kept_packs.nr || existing->cruft_packs.nr;
}
-+void existing_packs_snapshot_kept(const struct existing_packs *existing,
-+ struct tempfile *f)
++void existing_packs_write_non_kept(const struct existing_packs *existing,
++ struct tempfile *f)
+{
+ struct string_list_item *item;
+ FILE *out = fdopen_tempfile(f, "w");
@@ repack.c: void existing_packs_collect(struct existing_packs *existing,
+ 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);
++ for_each_string_list_item(item, &existing->non_kept_packs)
++ fprintf(out, "%s.pack\n", item->string);
++ for_each_string_list_item(item, &existing->cruft_packs)
+ 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"));
++ die_errno(_("could not close non-kept packs tempfile"));
+ }
+}
+
- int existing_packs_has_non_kept(const struct existing_packs *existing)
+ static void existing_pack_mark_for_deletion(struct string_list_item *item)
{
- return existing->non_kept_packs.nr || existing->cruft_packs.nr;
+ item->util = (void*)((uintptr_t)item->util | DELETE_PACK);
## repack.h ##
@@ repack.h: struct pack_objects_args {
@@ repack.h: struct pack_objects_args {
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.
++ * A file naming the packs this repack may delete, for
++ * pack-objects' "--ignore-pack-keep-from"; the path is borrowed.
+ */
-+ const char *kept_packs_snapshot;
++ const char *ignore_pack_keep_from;
struct list_objects_filter_options filter_options;
};
@@ repack.h: struct pack_objects_args {
void prepare_pack_objects(struct child_process *cmd,
const struct pack_objects_args *args,
@@ repack.h: struct existing_packs {
- */
void existing_packs_collect(struct existing_packs *existing,
const struct string_list *extra_keep);
+ int existing_packs_has_non_kept(const struct existing_packs *existing);
+/*
-+ * 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.
++ * Write the names of the non-kept and cruft packs, the ones a repack may
++ * delete, to "f", one per line, for pack-objects' "--ignore-pack-keep-from".
+ */
-+void existing_packs_snapshot_kept(const struct existing_packs *existing,
-+ struct tempfile *f);
- int existing_packs_has_non_kept(const struct existing_packs *existing);
++void existing_packs_write_non_kept(const struct existing_packs *existing,
++ struct tempfile *f);
int existing_pack_is_marked_for_deletion(struct string_list_item *item);
void existing_packs_retain_cruft(struct existing_packs *existing,
+ struct packed_git *cruft);
@@ repack.h: void pack_geometry_remove_redundant(struct pack_geometry *geometry,
bool wrote_incremental_midx);
void pack_geometry_release(struct pack_geometry *geometry);
@@ repack.h: void pack_geometry_remove_redundant(struct pack_geometry *geometry,
REPACK_WRITE_MIDX_DEFAULT,
## t/t7700-repack.sh ##
-@@ t/t7700-repack.sh: test_expect_success 'repack --keep-pack' '
+@@ t/t7700-repack.sh: test_expect_success 'pending objects are repacked appropriately' '
)
'
-+test_expect_success 'repack --keep-pack with --pack-kept-objects' '
-+ test_create_repo keep-pack-kept-objects &&
++# Put a reachable commit and an unreachable one into a single pack X in a
++# new repository "keep-race", run "git repack" with the given options and
++# have a ".keep" appear on X as pack-objects starts, i.e. after the repack
++# has decided to delete X. The unreachable commit is saved in the file
++# "keep-race/unreachable".
++keep_race_repack () {
++ git init keep-race &&
+ (
-+ 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 &&
++ cd keep-race &&
+ test_commit base &&
-+ git repack -ad &&
++ git commit-tree -m unreachable HEAD^{tree} >unreachable &&
+
-+ # 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" &&
++ git rev-list --objects --all $(cat unreachable) >objects &&
++ X=$(git pack-objects .git/objects/pack/pack <objects) &&
++ git prune-packed &&
+
-+ test_must_fail git repack -ad 2>err &&
-+ test_grep "contains a newline" err &&
++ mkdir shim &&
++ write_script shim/git <<-EOF &&
++ test "\$1" = "pack-objects" &&
++ >"$(pwd)/.git/objects/pack/pack-$X.keep" &&
++ >"$(pwd)/keep-created"
++ GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
++ EOF
++
++ git --exec-path="$(pwd)/shim" repack "$@" &&
++ test_path_is_file keep-created &&
++ test_path_is_missing .git/objects/pack/pack-$X.pack &&
+ git fsck
+ )
-+'
++}
++
++for args in "-a -d" "-a -d --filter=blob:none"
++do
++ test_expect_success "a .keep that shows up mid-repack loses nothing: $args" '
++ test_when_finished "rm -fr keep-race" &&
++ keep_race_repack $args
++ '
++done
+
- test_expect_success 'repacking fails when missing .pack actually means missing objects' '
- test_create_repo idx-without-pack &&
- (
++for args in "-A -d" "-A -d --pack-kept-objects" "-a -k -d" "--cruft -d" \
++ "--cruft --cruft-expiration=1.week.ago -d"
++do
++ test_expect_success "a .keep that shows up mid-repack loses nothing: $args" '
++ test_when_finished "rm -fr keep-race" &&
++ keep_race_repack $args &&
++ git -C keep-race cat-file -e $(cat keep-race/unreachable)
++ '
++done
++
+ test_done
## t/t7703-repack-geometric.sh ##
@@ t/t7703-repack-geometric.sh: test_expect_success 'geometric repack works with promisor packs' '
@@ t/t7703-repack-geometric.sh: test_expect_success 'geometric repack works with pr
+ 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.
++ # rolled up and deleted. Have a ".keep" appear on one of them
++ # as pack-objects starts, after the repack has decided to
++ # delete it; its objects must still end up in the new pack.
+ mkdir shim &&
+ write_script shim/git <<-EOF &&
-+ test "\$1" = "pack-objects" && >"$(pwd)/$packdir/pack-$KEPT.keep"
++ test "\$1" = "pack-objects" &&
++ >"$(pwd)/$packdir/pack-$KEPT.keep" &&
++ >"$(pwd)/keep-created"
+ GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
+ EOF
+
+ git --exec-path="$(pwd)/shim" repack --geometric 2 -d &&
++ test_path_is_file keep-created &&
++ test_path_is_missing $packdir/pack-$KEPT.pack &&
+
+ 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 [flat|nested] 31+ messages in thread* [PATCH v4 1/3] pack-objects: reset kept-pack cache for cruft walk
2026-10-26 9:06 ` [PATCH v4 0/3] " qeesung via GitGitGadget
@ 2026-10-26 9:06 ` Qin ShiCheng via GitGitGadget
2026-10-26 9:06 ` [PATCH v4 2/3] pack-objects: add --ignore-pack-keep-from Qin ShiCheng via GitGitGadget
2026-10-26 9:07 ` [PATCH v4 3/3] repack: tell pack-objects which packs it may delete Qin ShiCheng via GitGitGadget
2 siblings, 0 replies; 31+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-10-26 9:06 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. Whether that
happens depends on whether the question changes along the way, which
"--honor-pack-keep" does whenever there is a ".keep" file around.
Drop the cache after unmarking. Only packfile.c is to touch it, so
add repo_invalidate_kept_pack_caches() there, which drops it for every
object directory.
With the cache gone, the walk goes through every pack but the listed
fresh ones, as was intended, including packs borrowed from an
alternate. It has to: an object there may refer to a local one, for
example when two repositories borrow from each other, or when "git
repack --filter-to" has moved some objects to an alternate and left
the ones they refer to behind. Without "--local", whatever the walk
finds there is copied into the cruft pack, as it already is for loose
objects of an alternate; with "--local", nothing is. Either way, the
walk may now go through a history far bigger than the repository's
own, as it already did whenever there was a ".keep" file around.
It also includes packs given with "--keep-pack", whose objects are to
be left out. Turn those into "open" kept packs, as
"--stdin-packs=follow" does for its "!" packs: the walk goes through
them, and their objects are still left out. This is how an unlisted
pack with a ".keep" file is treated under "--honor-pack-keep".
With "--keep-pack", the walk and the check that leaves the objects of
such a pack out ask about different kinds of kept packs, so the cache
is rebuilt each time they take turns, as already happens under
"--honor-pack-keep" whenever there is a ".keep" file around. repack
never gets here with "--keep-pack": it lists the packs it keeps as
fresh ones instead.
The tests walk from a recent commit to an expired one through a pack
pack-objects is not told about, through one given with "--keep-pack",
and through one of an alternate holding trees whose blobs "git repack
--filter-to" left behind, with and without "--local".
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/pack-objects.c | 18 ++++++-
odb/source-packed.h | 3 +-
packfile.c | 18 ++++++-
packfile.h | 7 +++
t/t5329-pack-objects-cruft.sh | 98 +++++++++++++++++++++++++++++++++++
5 files changed, 140 insertions(+), 4 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ca3a891dfb..5160e76842 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4294,10 +4294,18 @@ 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.
+ * A pack given with --keep-pack must not halt it either, but as an
+ * "open" kept pack its objects are still left out. The kept-pack
+ * cache was built while the packs were still marked, so drop it
+ * too.
*/
- repo_for_each_pack(the_repository, p)
+ repo_for_each_pack(the_repository, p) {
p->pack_keep_in_core = 0;
+ if (p->pack_keep_in_core_open)
+ ignore_packed_keep_in_core_open = 1;
+ }
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"));
@@ -4343,6 +4351,14 @@ static void read_cruft_objects(void)
if (item) {
item->util = p;
+ } else if (p->pack_keep_in_core) {
+ /*
+ * Only --keep-pack marks a pack as kept this early.
+ * Make it an "open" kept pack as well, so that its
+ * objects are still left out once it is unmarked
+ * before the traversal; until then this has no effect.
+ */
+ p->pack_keep_in_core_open = 1;
} else {
/*
* This pack wasn't mentioned in either the "fresh" or
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 b32b4cd75d..213f930edb 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1881,6 +1881,21 @@ 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_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);
+}
+
static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
unsigned flags)
{
@@ -1888,8 +1903,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 ed3378c457..0ead9e2821 100644
--- a/packfile.h
+++ b/packfile.h
@@ -143,6 +143,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..4a1056e494 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -332,6 +332,104 @@ test_expect_success 'cruft trees rescue sub-trees, blobs' '
)
'
+# Build the history "packed", "old", "mid", "new" in the current repository:
+# "packed" is in the pack named in "in", "old" has expired, "mid" is in the
+# pack whose hash is in $mid and "new" is recent. The objects of packed..new
+# end up in "all.objects".
+cruft_chain () {
+ test_commit packed &&
+ git repack -Ad &&
+ basename "$(ls $packdir/pack-*.pack)" >in &&
+
+ test_commit old &&
+ test_commit mid &&
+ test_commit new &&
+
+ git rev-list --objects --no-object-names packed..old >old &&
+ while read object
+ do
+ test-tool chmtime -1000 \
+ "$objdir/$(test_oid_to_path $object)" || return 1
+ done <old &&
+ git rev-list --objects --no-object-names old..mid >mid.objects &&
+ mid=$(git pack-objects $packdir/pack <mid.objects) &&
+ git prune-packed &&
+ git rev-list --objects --no-object-names packed..new >all.objects
+}
+
+test_expect_success 'cruft traversal goes through a pack it was not told about' '
+ git init repo &&
+ test_when_finished "rm -fr repo" &&
+ (
+ cd repo &&
+ cruft_chain &&
+
+ # "old" can only be rescued from "new" by walking through
+ # the "mid" pack.
+ cruft=$(git pack-objects --cruft \
+ --cruft-expiration=750.seconds.ago $packdir/pack <in) &&
+ test-tool pack-mtimes "pack-$cruft.mtimes" >actual.raw &&
+ cut -d" " -f1 <actual.raw | sort >actual &&
+ sort <all.objects >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'cruft traversal goes through a --keep-pack pack' '
+ git init repo &&
+ test_when_finished "rm -fr repo" &&
+ (
+ cd repo &&
+ cruft_chain &&
+
+ # The same, but with "mid" in a pack given with --keep-pack:
+ # "old" is still rescued, and none of the objects of that
+ # pack are copied.
+ cruft=$(git pack-objects --cruft \
+ --cruft-expiration=750.seconds.ago \
+ --keep-pack=pack-$mid.pack $packdir/pack <in) &&
+ test-tool pack-mtimes "pack-$cruft.mtimes" >actual.raw &&
+ cut -d" " -f1 <actual.raw | sort >actual &&
+ sort <all.objects >all.sorted &&
+ sort <mid.objects >mid.sorted &&
+ comm -23 all.sorted mid.sorted >expect &&
+ test_cmp expect actual
+ )
+'
+
+for opt in "" --local
+do
+ test_expect_success "cruft traversal goes through an alternate${opt:+ ($opt)}" '
+ test_when_finished "rm -fr repo alt.git" &&
+ git init repo &&
+ git init --bare alt.git &&
+ echo "$(pwd)/alt.git/objects" >repo/.git/objects/info/alternates &&
+ test_commit -C repo base &&
+ test_commit -C repo old &&
+ tree=$(git -C repo rev-parse old^{tree}) &&
+ blob=$(git -C repo rev-parse old:old.t) &&
+
+ # Move everything but the blobs to the alternate, then let
+ # "old" expire: its tree is in the alternate, but its blob
+ # is in an old local pack.
+ git -C repo repack -a -d &&
+ git -C repo repack -a -d --filter=object:type=blob \
+ --filter-to="$(pwd)/alt.git/objects/pack/pack" &&
+ git -C repo tag -d old &&
+ git -C repo reset --hard base &&
+ git -C repo reflog expire --all --expire=all &&
+ test-tool chmtime -10000 repo/$packdir/pack-*.pack &&
+
+ # A recent unreachable commit in "repo" keeps the blob, but only
+ # if the traversal goes through the tree in the alternate, which
+ # it has to do even with --local.
+ outer=$(printf "040000 tree %s\tdir\n" $tree | git -C repo mktree) &&
+ git -C repo commit-tree -m recent $outer >/dev/null &&
+ git -C repo repack --cruft --cruft-expiration=1.hour.ago -d $opt &&
+ git -C repo cat-file -e $blob
+ '
+done
+
test_expect_success 'expired objects are pruned' '
git init repo &&
test_when_finished "rm -fr repo" &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v4 2/3] pack-objects: add --ignore-pack-keep-from
2026-10-26 9:06 ` [PATCH v4 0/3] " qeesung via GitGitGadget
2026-10-26 9:06 ` [PATCH v4 1/3] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
@ 2026-10-26 9:06 ` Qin ShiCheng via GitGitGadget
2026-10-26 9:07 ` [PATCH v4 3/3] repack: tell pack-objects which packs it may delete Qin ShiCheng via GitGitGadget
2 siblings, 0 replies; 31+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-10-26 9:06 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 decides which packs to delete before it runs pack-objects, but
pack-objects looks for ".keep" files on its own when it opens the
packs. A ".keep" that appears in between, for example from a push or
fetch whose index-pack wrote a pack identical to an existing one, makes
pack-objects treat as kept a pack that repack is about to delete. Its
objects are then left out of the new pack, or not loosened, or not
kept as unreachable, and they are gone once repack deletes the pack.
Not all of that goes through "--honor-pack-keep": loosening objects
for "--unpack-unreachable", and collecting them for
"--keep-unreachable" and "--cruft", skip packs with a ".keep" file
regardless of it.
Let the caller name such packs. "--ignore-pack-keep-from=<file>" reads
names of local packs, one per line, given and matched as for
"--keep-pack", and treats them as if they had no ".keep" file. All of
the above go by p->pack_keep, which is read from the disk once, when a
pack is first seen, so clearing it for the named packs before anything
looks at it covers every one of them. Other packs, including any that
show up later, keep whatever their ".keep" files say.
The names come from a file because there can be more of them than fit
on a command line (on Windows, the whole command line is limited to
32767 characters), and the standard input is already taken in every
mode repack uses.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
Documentation/git-pack-objects.adoc | 12 ++++++++++
builtin/pack-objects.c | 37 +++++++++++++++++++++++++++++
t/t7700-repack.sh | 37 +++++++++++++++++++++++++++++
3 files changed, 86 insertions(+)
diff --git a/Documentation/git-pack-objects.adoc b/Documentation/git-pack-objects.adoc
index ccad938c5b..43fb206ef0 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>]
+ [--ignore-pack-keep-from=<file>]
[--cruft] [--cruft-expiration=<time>]
[--stdout [--filter=<filter-spec>] | <base-name>]
[--shallow] [--keep-true-parents] [--[no-]sparse]
@@ -193,6 +194,17 @@ depth is 4095.
leading directory (e.g. `pack-123.pack`). The option could be
specified multiple times to keep multiple packs.
+--ignore-pack-keep-from=<file>::
+ Read names of local packs from `<file>`, one per line in the
+ same form as for `--keep-pack`, and treat them as if they had
+ no .keep file, both for `--honor-pack-keep` and when deciding
+ which objects to loosen or keep with `--unpack-unreachable`,
+ `--keep-unreachable` and `--cruft`.
+ This is for callers like linkgit:git-repack[1] that are about
+ to delete those packs: a .keep file that appears on one of them
+ after the caller decided to delete it must not cause its objects
+ to be left out.
+
--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 5160e76842..f63baa7273 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"
+ " [--ignore-pack-keep-from=<file>]\n"
" [--cruft] [--cruft-expiration=<time>]\n"
" [--stdout [--filter=<filter-spec>] | <base-name>]\n"
" [--shallow] [--keep-true-parents] [--[no-]sparse]\n"
@@ -5005,6 +5006,36 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}
+/*
+ * Treat the local packs named in "path" as if they had no ".keep" file.
+ * The names are matched like those given with --keep-pack. This has to
+ * happen before anything looks at p->pack_keep, which is read from the
+ * disk only once, when the pack is first seen.
+ */
+static void ignore_pack_keep_from_file(const char *path)
+{
+ struct string_list names = STRING_LIST_INIT_DUP;
+ struct strbuf buf = STRBUF_INIT;
+ struct packed_git *p;
+ FILE *fp = xfopen(path, "r");
+
+ names.cmp = fspathcmp;
+ while (strbuf_getline(&buf, fp) != EOF)
+ string_list_append(&names, buf.buf);
+ if (ferror(fp))
+ die_errno(_("could not read '%s'"), path);
+ fclose(fp);
+ string_list_sort(&names);
+
+ repo_for_each_pack(the_repository, p)
+ if (p->pack_local &&
+ string_list_has_string(&names, pack_basename(p)))
+ p->pack_keep = 0;
+
+ string_list_clear(&names, 0);
+ strbuf_release(&buf);
+}
+
static void add_extra_kept_packs(const struct string_list *names)
{
struct packed_git *p;
@@ -5138,6 +5169,7 @@ 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 = STRING_LIST_INIT_NODUP;
+ char *ignore_pack_keep_from = NULL;
struct list_objects_filter_options filter_options =
LIST_OBJECTS_FILTER_INIT;
struct repo_config_values *cfg = repo_config_values(the_repository);
@@ -5222,6 +5254,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, "ignore-pack-keep-from", &ignore_pack_keep_from,
+ N_("ignore .keep files of 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,
@@ -5449,6 +5483,8 @@ int cmd_pack_objects(int argc,
if (progress && all_progress_implied)
progress = 2;
+ if (ignore_pack_keep_from)
+ ignore_pack_keep_from_file(ignore_pack_keep_from);
add_extra_kept_packs(&keep_pack_list);
if (ignore_packed_keep_on_disk) {
struct packed_git *p;
@@ -5543,6 +5579,7 @@ cleanup:
clear_packing_data(&to_pack);
list_objects_filter_release(&filter_options);
string_list_clear(&keep_pack_list, 0);
+ free(ignore_pack_keep_from);
strvec_clear(&rp);
return 0;
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f0a390e3c6..00fe259767 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -211,6 +211,43 @@ test_expect_success 'objects made unreachable by grafts only are kept' '
git cat-file -t $H1
'
+test_expect_success 'pack-objects --ignore-pack-keep-from' '
+ test_when_finished "rm -fr ignore-keep" &&
+ git init ignore-keep &&
+ (
+ cd ignore-keep &&
+ test_commit base &&
+ unreachable=$(git commit-tree -m unreachable HEAD^{tree}) &&
+
+ git rev-list --objects --all $unreachable >objects &&
+ P=$(git pack-objects .git/objects/pack/pack <objects) &&
+ test_commit kept &&
+ git rev-list --objects base..kept >objects &&
+ K=$(git pack-objects .git/objects/pack/pack <objects) &&
+ git prune-packed &&
+ >.git/objects/pack/pack-$P.keep &&
+ >.git/objects/pack/pack-$K.keep &&
+ echo pack-$P.pack >ignore &&
+
+ # Both packs have a .keep file, but only P is named in
+ # "ignore": its reachable objects are packed and the others
+ # are loosened, while K is left alone.
+ git pack-objects --honor-pack-keep --all --reflog \
+ --indexed-objects --unpack-unreachable \
+ --ignore-pack-keep-from=ignore \
+ .git/objects/pack/pack </dev/null &&
+ test-tool find-pack --check-count 2 base &&
+ test-tool find-pack --check-count 1 kept &&
+ test_path_is_file .git/objects/$(test_oid_to_path $unreachable)
+ )
+'
+
+test_expect_success 'pack-objects --ignore-pack-keep-from with a missing file' '
+ test_must_fail git pack-objects --stdout \
+ --ignore-pack-keep-from=does-not-exist </dev/null 2>err &&
+ test_grep "could not open .does-not-exist. for reading" err
+'
+
test_expect_success 'repack --keep-pack' '
test_create_repo keep-pack &&
(
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v4 3/3] repack: tell pack-objects which packs it may delete
2026-10-26 9:06 ` [PATCH v4 0/3] " qeesung via GitGitGadget
2026-10-26 9:06 ` [PATCH v4 1/3] pack-objects: reset kept-pack cache for cruft walk Qin ShiCheng via GitGitGadget
2026-10-26 9:06 ` [PATCH v4 2/3] pack-objects: add --ignore-pack-keep-from Qin ShiCheng via GitGitGadget
@ 2026-10-26 9:07 ` Qin ShiCheng via GitGitGadget
2 siblings, 0 replies; 31+ messages in thread
From: Qin ShiCheng via GitGitGadget @ 2026-10-26 9:07 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 runs pack-objects, which looks for them all
over again when it opens the packs. 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 to itself: its objects are not copied into the
new pack, not loosened for "-A", not kept for "-k" or "--cruft". The
parent deletes the pack regardless, as 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 names a pack after its contents
and writes its ".keep" before it renames the packfile into place, so a
"git fetch", or a push being migrated out of its quarantine, that
brings in a pack identical to one we already have puts a ".keep" next
to that pack. 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.
When we may delete packs, that is with "-d" and either "-a", "-A",
"--cruft" or "--geometric", write the names of the packs we may
delete, the non-kept and cruft packs, to a file next to the refs
snapshot we already write for "git multi-pack-index write", and pass it
to every pack-objects we spawn with "--ignore-pack-keep-from".
pack-objects then never treats those packs as kept, whatever appears
on the disk. Every other pack, whether kept when we started or showing
up later, still goes by its ".keep" file, so "--honor-pack-keep"
stays. Otherwise nothing gets deleted and nothing is passed.
The file has one name per line. A name with a newline in it gets split
there, which at worst makes some other pack not count as kept: its
objects may be copied, but none are lost.
Unreachable objects of such a pack are still dropped by "-a -d", or by
"--cruft-expiration" and "--unpack-unreachable" once they are old
enough, as these drop unreachable objects by design, and a pack written
again under the same name does not make them any younger; what changes
is that the objects that are to be kept no longer go with them.
The tests have a ".keep" appear on the pack about to be deleted as
pack-objects starts, through a "git" wrapper found via "--exec-path",
for "-a", "-A", "-k", "--cruft" with and without an expiration,
"--pack-kept-objects", "--filter" and "--geometric".
Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
builtin/repack.c | 23 +++++++++++++++++
repack.c | 26 ++++++++++++++++++++
repack.h | 14 +++++++++--
t/t7700-repack.sh | 49 +++++++++++++++++++++++++++++++++++++
t/t7703-repack-geometric.sh | 39 +++++++++++++++++++++++++++++
5 files changed, 149 insertions(+), 2 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 5d06872d77..8faf7ce2ef 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -457,6 +457,28 @@ int cmd_repack(int argc,
existing.repo = repo;
existing_packs_collect(&existing, &keep_pack_list);
+ /*
+ * Tell pack-objects which packs we may delete. pack-objects looks
+ * for ".keep" files on its own, and one that appears on such a
+ * pack from now on must not make it leave out objects that would
+ * then be deleted with the pack.
+ */
+ if (delete_redundant &&
+ ((pack_everything & ALL_INTO_ONE) || geometry.split_factor) &&
+ existing_packs_has_non_kept(&existing)) {
+ struct strbuf path = STRBUF_INIT;
+ struct tempfile *non_kept_packs;
+
+ strbuf_addf(&path, "%s/tmp_non_kept_packs_XXXXXX",
+ repo_get_object_directory(repo));
+
+ non_kept_packs = xmks_tempfile(path.buf);
+ existing_packs_write_non_kept(&existing, non_kept_packs);
+ po_args.ignore_pack_keep_from = get_tempfile_path(non_kept_packs);
+
+ strbuf_release(&path);
+ }
+
if (geometry.split_factor) {
if (pack_everything)
die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a");
@@ -644,6 +666,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.ignore_pack_keep_from = po_args.ignore_pack_keep_from;
ret = write_cruft_pack(&opts, cruft_expiration,
combine_cruft_below_size, &names,
diff --git a/repack.c b/repack.c
index 53c34197f3..3af8c7a5f8 100644
--- a/repack.c
+++ b/repack.c
@@ -40,6 +40,9 @@ void prepare_pack_objects(struct child_process *cmd,
strvec_push(&cmd->args, "--delta-base-offset");
if (!args->pack_kept_objects)
strvec_push(&cmd->args, "--honor-pack-keep");
+ if (args->ignore_pack_keep_from)
+ strvec_pushf(&cmd->args, "--ignore-pack-keep-from=%s",
+ args->ignore_pack_keep_from);
strvec_push(&cmd->args, out);
cmd->git_cmd = 1;
cmd->out = -1;
@@ -172,6 +175,29 @@ int existing_packs_has_non_kept(const struct existing_packs *existing)
return existing->non_kept_packs.nr || existing->cruft_packs.nr;
}
+void existing_packs_write_non_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->non_kept_packs)
+ fprintf(out, "%s.pack\n", item->string);
+ for_each_string_list_item(item, &existing->cruft_packs)
+ 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 non-kept packs tempfile"));
+ }
+}
+
static void existing_pack_mark_for_deletion(struct string_list_item *item)
{
item->util = (void*)((uintptr_t)item->util | DELETE_PACK);
diff --git a/repack.h b/repack.h
index 61e554e4ed..e3bc840b9a 100644
--- a/repack.h
+++ b/repack.h
@@ -19,6 +19,11 @@ struct pack_objects_args {
int path_walk;
int delta_base_offset;
int pack_kept_objects;
+ /*
+ * A file naming the packs this repack may delete, for
+ * pack-objects' "--ignore-pack-keep-from"; the path is borrowed.
+ */
+ const char *ignore_pack_keep_from;
struct list_objects_filter_options filter_options;
};
@@ -28,6 +33,7 @@ struct pack_objects_args {
}
struct child_process;
+struct tempfile;
void prepare_pack_objects(struct child_process *cmd,
const struct pack_objects_args *args,
@@ -80,6 +86,12 @@ struct existing_packs {
void existing_packs_collect(struct existing_packs *existing,
const struct string_list *extra_keep);
int existing_packs_has_non_kept(const struct existing_packs *existing);
+/*
+ * Write the names of the non-kept and cruft packs, the ones a repack may
+ * delete, to "f", one per line, for pack-objects' "--ignore-pack-keep-from".
+ */
+void existing_packs_write_non_kept(const struct existing_packs *existing,
+ struct tempfile *f);
int existing_pack_is_marked_for_deletion(struct string_list_item *item);
void existing_packs_retain_cruft(struct existing_packs *existing,
struct packed_git *cruft);
@@ -138,8 +150,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 00fe259767..79fa3189f6 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -941,4 +941,53 @@ test_expect_success 'pending objects are repacked appropriately' '
)
'
+# Put a reachable commit and an unreachable one into a single pack X in a
+# new repository "keep-race", run "git repack" with the given options and
+# have a ".keep" appear on X as pack-objects starts, i.e. after the repack
+# has decided to delete X. The unreachable commit is saved in the file
+# "keep-race/unreachable".
+keep_race_repack () {
+ git init keep-race &&
+ (
+ cd keep-race &&
+ test_commit base &&
+ git commit-tree -m unreachable HEAD^{tree} >unreachable &&
+
+ git rev-list --objects --all $(cat unreachable) >objects &&
+ X=$(git pack-objects .git/objects/pack/pack <objects) &&
+ git prune-packed &&
+
+ mkdir shim &&
+ write_script shim/git <<-EOF &&
+ test "\$1" = "pack-objects" &&
+ >"$(pwd)/.git/objects/pack/pack-$X.keep" &&
+ >"$(pwd)/keep-created"
+ GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
+ EOF
+
+ git --exec-path="$(pwd)/shim" repack "$@" &&
+ test_path_is_file keep-created &&
+ test_path_is_missing .git/objects/pack/pack-$X.pack &&
+ git fsck
+ )
+}
+
+for args in "-a -d" "-a -d --filter=blob:none"
+do
+ test_expect_success "a .keep that shows up mid-repack loses nothing: $args" '
+ test_when_finished "rm -fr keep-race" &&
+ keep_race_repack $args
+ '
+done
+
+for args in "-A -d" "-A -d --pack-kept-objects" "-a -k -d" "--cruft -d" \
+ "--cruft --cruft-expiration=1.week.ago -d"
+do
+ test_expect_success "a .keep that shows up mid-repack loses nothing: $args" '
+ test_when_finished "rm -fr keep-race" &&
+ keep_race_repack $args &&
+ git -C keep-race cat-file -e $(cat keep-race/unreachable)
+ '
+done
+
test_done
diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index f3a0650cfe..7a5e3b9ada 100755
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -541,4 +541,43 @@ 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
+ # rolled up and deleted. Have a ".keep" appear on one of them
+ # as pack-objects starts, after the repack has decided to
+ # delete it; its objects must still end up in the new pack.
+ mkdir shim &&
+ write_script shim/git <<-EOF &&
+ test "\$1" = "pack-objects" &&
+ >"$(pwd)/$packdir/pack-$KEPT.keep" &&
+ >"$(pwd)/keep-created"
+ GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
+ EOF
+
+ git --exec-path="$(pwd)/shim" repack --geometric 2 -d &&
+ test_path_is_file keep-created &&
+ test_path_is_missing $packdir/pack-$KEPT.pack &&
+
+ git fsck
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread