repack: don't lose objects to a ".keep" that appears mid-run

A concurrent push can make "git repack -d" delete a pack whose objects were never copied anywhere, and exit 0. We hit this in production: a ref pointing at a commit that no longer exists, on git 2.43, and it reproduces on master.

What happens:

* repack scans for ".keep" files and decides which packs to delete, then spawns pack-objects with --honor-pack-keep, which scans again;
* in between, a push of content identical to an earlier one finishes migrating its quarantine. Its pack is a duplicate and is dropped, but its ".keep" is linked into place, onto the old pack;
* pack-objects sees that ".keep" and leaves the pack's objects out; repack deletes the pack by its earlier list, with force_delete.

Two things are wrong, and each is fixed on its own:

* 1/6: receive-pack removes a ".keep" it never installed -- the one it linked onto somebody else's pack, or a foreign one when its own push was rejected before any migration. Only remove a ".keep" that carries our own message.
* 6/6: repack and pack-objects each scan for ".keep" files. Hand pack-objects the snapshot repack took at startup instead.

Patches 2-5 are what 6/6 needs to be safe:

* 2/6: under --stdin-packs=follow, a --keep-pack pack stops the traversal like a "^" pack; on-disk ".keep" packs never did.
* 3/6: the cruft walk goes by a stale kept-pack cache, which --honor-pack-keep happened to mask. Pre-existing, reproducible today.
* 4/6: look --keep-pack names up in a sorted list; it gets long.
* 5/6: --keep-pack-from-file, since a repository can have more kept packs than fit on a command line (32K characters on Windows).

Every fix comes with a test that fails without it; the race itself is reproduced in t7703 by having a ".keep" appear as pack-objects starts. The full suite passes, and the series merges cleanly into next and seen.

Qin ShiCheng (6):
odb: don't remove a ".keep" we never installed
pack-objects: keep --keep-pack open when following
pack-objects: reset kept-pack cache for cruft walk
pack-objects: sort --keep-pack list for lookup
pack-objects: add --keep-pack-from-file
repack: tell pack-objects which packs are kept

 Documentation/git-pack-objects.adoc |  8 +++
 builtin/pack-objects.c              | 71 +++++++++++++++++----
 builtin/repack.c                    | 15 +++++
 object-file.c                       | 95 ++++++++++++++++++++++-------
 odb/source-packed.h                 |  3 +-
 packfile.c                          |  9 ++-
 packfile.h                          |  7 +++
 repack-filtered.c                   |  3 -
 repack.c                            | 34 ++++++++++-
 repack.h                            | 17 +++++-
 t/t5329-pack-objects-cruft.sh       | 40 ++++++++++++
 t/t5331-pack-objects-stdin.sh       | 87 ++++++++++++++++++++++++++
 t/t5547-push-quarantine.sh          | 52 ++++++++++++++++
 t/t7700-repack.sh                   | 43 +++++++++++++
 t/t7703-repack-geometric.sh         | 72 ++++++++++++++++++++++
 tempfile.c                          | 12 ++++
 tempfile.h                          |  9 +++
 17 files changed, 533 insertions(+), 44 deletions(-)

base-commit: 3cb9185f65410273787f74333cc027d2ea5daada
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2219%2Fqeesung%2Frepack-kept-packs-snapshot-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2219/qeesung/repack-kept-packs-snapshot-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2219

gitgitgadget

From: Qin ShiCheng <[email protected]>

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.

---
 object-file.c              | 95 +++++++++++++++++++++++++++++---------
 t/t5547-push-quarantine.sh | 52 +++++++++++++++++++++
 tempfile.c                 | 12 +++++
 tempfile.h                 |  9 ++++
 4 files changed, 147 insertions(+), 21 deletions(-)

index a4cbf8b081..21513ee535 100644
@@ -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) {

index dc9ca4e645..10db4fbc7f 100644
@@ -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;

index f571f3c609..b439066a30 100644
@@ -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

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.

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.

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

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.

[...]

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

From: Qin ShiCheng <[email protected]>

"--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.

---
 builtin/pack-objects.c        | 20 +++++++++++++----
 t/t5331-pack-objects-stdin.sh | 41 +++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 4 deletions(-)

index 708b719f40..6f579173b0 100644
@@ -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;
 

gitgitgadget

From: Qin ShiCheng <[email protected]>

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.

---
 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(-)

index 6f579173b0..8ca8255176 100644
@@ -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"));

index a0f6b5096d..9e42311916 100644
@@ -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;

index 4fa5fd67c8..90459ec4d7 100644
@@ -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,

index 6d30d15a00..493faf0010 100644
@@ -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;

index 12cda06373..6302f60b75 100755
@@ -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

From: Qin ShiCheng <[email protected]>

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.

---
 builtin/pack-objects.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

index 8ca8255176..1fcb4ef8a5 100644
@@ -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

From: Qin ShiCheng <[email protected]>

"--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.

---
 Documentation/git-pack-objects.adoc |  8 +++++
 builtin/pack-objects.c              | 28 ++++++++++++++++++
 t/t5331-pack-objects-stdin.sh       | 46 +++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)

index 65cd00c152..938e27f69d 100644
@@ -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.

index 1fcb4ef8a5..9f8c4b9135 100644
@@ -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;

gitgitgadget

From: Qin ShiCheng <[email protected]>

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.

---
 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(-)

index c4360382c1..78bc98c4f1 100644
@@ -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,

index 869b9fc6e3..db8de9f633 100644
@@ -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);

index d2aa58e134..a794486035 100644
@@ -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;

index 61e554e4ed..1c0aeca3e8 100644
@@ -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,

gitgitgadget

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

From: Qin ShiCheng <[email protected]>

"--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.

---
 builtin/pack-objects.c        | 20 +++++++++++++----
 t/t5331-pack-objects-stdin.sh | 41 +++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 4 deletions(-)

index 708b719f40..6f579173b0 100644
@@ -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;
 

gitgitgadget

From: Qin ShiCheng <[email protected]>

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.

---
 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(-)

index 6f579173b0..8ca8255176 100644
@@ -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"));

index a0f6b5096d..9e42311916 100644
@@ -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;

index 4fa5fd67c8..90459ec4d7 100644
@@ -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,

index 6d30d15a00..493faf0010 100644
@@ -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;

index 12cda06373..6302f60b75 100755
@@ -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

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.

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

OK.

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

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.

From: Qin ShiCheng <[email protected]>

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.

---
 builtin/pack-objects.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

index 8ca8255176..1fcb4ef8a5 100644
@@ -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

From: Qin ShiCheng <[email protected]>

"--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.

---
 Documentation/git-pack-objects.adoc |  8 +++++
 builtin/pack-objects.c              | 28 ++++++++++++++++++
 t/t5331-pack-objects-stdin.sh       | 46 +++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)

index 65cd00c152..938e27f69d 100644
@@ -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.

index 1fcb4ef8a5..9f8c4b9135 100644
@@ -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;

gitgitgadget

From: Qin ShiCheng <[email protected]>

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.

---
 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(-)

index c4360382c1..78bc98c4f1 100644
@@ -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,

index 869b9fc6e3..db8de9f633 100644
@@ -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);

index d2aa58e134..a794486035 100644
@@ -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;

index 61e554e4ed..1c0aeca3e8 100644
@@ -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,

gitgitgadget

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/[email protected]/ (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.

## 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

From: Qin ShiCheng <[email protected]>

"--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.

---
 builtin/pack-objects.c        | 20 +++++++++++++----
 t/t5331-pack-objects-stdin.sh | 41 +++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 4 deletions(-)

index 708b719f40..6f579173b0 100644
@@ -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;
 

gitgitgadget

From: Qin ShiCheng <[email protected]>

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.

---
 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(-)

index 6f579173b0..f86b3661c5 100644
@@ -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"));

index a0f6b5096d..c8c6088a69 100644
@@ -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;

index 4fa5fd67c8..399c0622cd 100644
@@ -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,

index 6d30d15a00..e2db6bfff7 100644
@@ -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;

index 12cda06373..6302f60b75 100755
@@ -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

From: Qin ShiCheng <[email protected]>

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.

---
 builtin/pack-objects.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

index f86b3661c5..48faef2227 100644
@@ -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

From: Qin ShiCheng <[email protected]>

"--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.

---
 Documentation/git-pack-objects.adoc |  8 +++++
 builtin/pack-objects.c              | 28 ++++++++++++++++++
 t/t5331-pack-objects-stdin.sh       | 46 +++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)

index 65cd00c152..938e27f69d 100644
@@ -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.

index 48faef2227..621b333d0b 100644
@@ -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;

gitgitgadget

From: Qin ShiCheng <[email protected]>

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.

---
 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(-)

index c4360382c1..78bc98c4f1 100644
@@ -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,

index 869b9fc6e3..db8de9f633 100644
@@ -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);

index d2aa58e134..a794486035 100644
@@ -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;

index 61e554e4ed..1c0aeca3e8 100644
@@ -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,

gitgitgadget

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>

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);
	}

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

Thanks. In the meantime you can participate in reviewing Patrick's topic, of course, and that may help it move forward a bit faster ;-)

"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 <[email protected]>
      
     @@ 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 <[email protected]>
      
       ## 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 <[email protected]>
      
     @@ 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 <[email protected]>
      
       ## 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 <[email protected]>
      
       ## 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

From: Qin ShiCheng <[email protected]>

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".

---
 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(-)

index ca3a891dfb..5160e76842 100644
@@ -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

index a0f6b5096d..c8c6088a69 100644
@@ -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;

index b32b4cd75d..213f930edb 100644
@@ -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,

index ed3378c457..0ead9e2821 100644
@@ -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;

gitgitgadget

From: Qin ShiCheng <[email protected]>

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.

---
 Documentation/git-pack-objects.adoc | 12 ++++++++++
 builtin/pack-objects.c              | 37 +++++++++++++++++++++++++++++
 t/t7700-repack.sh                   | 37 +++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)

index ccad938c5b..43fb206ef0 100644
@@ -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.

index 5160e76842..f63baa7273 100644
@@ -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;

index f0a390e3c6..00fe259767 100755
@@ -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

From: Qin ShiCheng <[email protected]>

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".

---
 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(-)

index 5d06872d77..8faf7ce2ef 100644
@@ -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,

index 53c34197f3..3af8c7a5f8 100644
@@ -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);

index 61e554e4ed..e3bc840b9a 100644
@@ -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,

index f3a0650cfe..7a5e3b9ada 100755
@@ -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