[PATCH 3/6] pack-objects: reset kept-pack cache for cruft walk

["Qin ShiCheng via GitGitGadget" <gitgitgadget@gmail.com>]

From: Qin ShiCheng <qeesung@live.com>

When writing a cruft pack with an expiration, pack-objects first
collects the recent objects and then walks from them to rescue
whatever they reach, expired or not. A pack the caller did not list is
marked kept while collecting, so that its objects are not copied into
the cruft pack, and unmarked before the walk, so that the walk can go
through it.

The walk does not see the unmarking. Whether an object sits in a kept
pack is answered from a cache that is built on first use and only
dropped when asked about a different kind of kept pack. Collecting
builds it while the unlisted pack is still marked, the walk asks the
same kind of question, and so the unlisted pack stays in it: the walk
stops there, and whatever lies beyond it in an expired pack is lost.

This went unnoticed because of "--honor-pack-keep". repack passes it,
and when there is a ".keep" file it makes the collecting side ask
about on-disk and in-core kept packs together while the walk asks
about in-core ones alone; the cache is rebuilt each time the question
changes, and by accident the walk sees the current marks. Take the
".keep" file away and the objects are lost today. A later commit stops
repack from passing "--honor-pack-keep" at all, so fix this first.

Expose the invalidation packfile.c already has and call it after
re-marking. The test builds an unreachable chain whose middle commit
sits in a pack pack-objects is not told about and whose oldest objects
have expired; without the fix the cruft pack holds only the recent tip.

Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
 builtin/pack-objects.c        |  8 +++++++
 odb/source-packed.h           |  3 ++-
 packfile.c                    |  9 ++++++--
 packfile.h                    |  7 ++++++
 t/t5329-pack-objects-cruft.sh | 40 +++++++++++++++++++++++++++++++++++
 5 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6f579173b0..8ca8255176 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4274,6 +4274,7 @@ static void enumerate_cruft_objects(void)
 static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)
 {
 	struct packed_git *p;
+	struct odb_source *source;
 	struct rev_info revs;
 	int ret;
 
@@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
 	/*
 	 * Re-mark only the fresh packs as kept so that objects in
 	 * unknown packs do not halt the reachability traversal early.
+	 * The kept-pack cache was built while those packs were still
+	 * marked, so drop it too.
 	 */
 	repo_for_each_pack(the_repository, p)
 		p->pack_keep_in_core = 0;
 	mark_pack_kept_in_core(fresh_packs, 1);
+	for (source = the_repository->objects->sources; source;
+	     source = source->next) {
+		struct odb_source_files *files = odb_source_files_downcast(source);
+		packfile_store_invalidate_kept_pack_cache(files->packed);
+	}
 
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
diff --git a/odb/source-packed.h b/odb/source-packed.h
index a0f6b5096d..9e42311916 100644
--- a/odb/source-packed.h
+++ b/odb/source-packed.h
@@ -25,7 +25,8 @@ struct odb_source_packed {
 	 * Should not be accessed directly, but via
 	 * `packfile_store_get_kept_pack_cache()`. The list of packs gets
 	 * invalidated when the stored flags and the flags passed to
-	 * `packfile_store_get_kept_pack_cache()` mismatch.
+	 * `packfile_store_get_kept_pack_cache()` mismatch, or explicitly via
+	 * `packfile_store_invalidate_kept_pack_cache()`.
 	 */
 	struct {
 		struct packed_git **packs;
diff --git a/packfile.c b/packfile.c
index 4fa5fd67c8..90459ec4d7 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1870,6 +1870,12 @@ int packfile_fill_entry(struct packed_git *p,
 	return 1;
 }
 
+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store)
+{
+	FREE_AND_NULL(store->kept_cache.packs);
+	store->kept_cache.flags = 0;
+}
+
 static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
 					     unsigned flags)
 {
@@ -1877,8 +1883,7 @@ static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
 		return;
 	if (store->kept_cache.flags == flags)
 		return;
-	FREE_AND_NULL(store->kept_cache.packs);
-	store->kept_cache.flags = 0;
+	packfile_store_invalidate_kept_pack_cache(store);
 }
 
 struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
diff --git a/packfile.h b/packfile.h
index 6d30d15a00..493faf0010 100644
--- a/packfile.h
+++ b/packfile.h
@@ -144,6 +144,13 @@ enum kept_pack_type {
 struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
 						       unsigned flags);
 
+/*
+ * Drop the cache of kept packs so that the next call to
+ * `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after changing
+ * which packs are kept in core.
+ */
+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store);
+
 struct pack_window {
 	struct pack_window *next;
 	unsigned char *base;
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index 12cda06373..6302f60b75 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -332,6 +332,46 @@ test_expect_success 'cruft trees rescue sub-trees, blobs' '
 	)
 '
 
+test_expect_success 'cruft traversal rescues through a pack it was not told about' '
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		test_commit packed &&
+		git repack -Ad &&
+		keep="$(basename "$(ls $packdir/pack-*.pack)")" &&
+
+		test_commit old &&
+		test_commit mid &&
+		test_commit new &&
+
+		# "old" has expired, "new" is recent, and "mid" sits in a
+		# pack that pack-objects is not told about. Rescuing "old"
+		# from "new" means walking through that pack.
+		git rev-list --objects --no-object-names packed..old >old &&
+		while read object
+		do
+			test-tool chmtime -1000 \
+				"$objdir/$(test_oid_to_path $object)" || exit 1
+		done <old &&
+		git rev-list --objects --no-object-names old..mid |
+		git pack-objects $packdir/pack >/dev/null &&
+		git prune-packed &&
+
+		cruft="$(echo $keep | git pack-objects --cruft \
+			--cruft-expiration=750.seconds.ago \
+			$packdir/pack)" &&
+		test-tool pack-mtimes "pack-$cruft.mtimes" >actual.raw &&
+
+		cut -d" " -f1 <actual.raw | sort >actual &&
+		git rev-list --objects --no-object-names packed..new >expect.raw &&
+		sort <expect.raw >expect &&
+
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'expired objects are pruned' '
 	git init repo &&
 	test_when_finished "rm -fr repo" &&
-- 
gitgitgadget