[PATCH v4 3/3] repack: tell pack-objects which packs it may delete

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

From: Qin ShiCheng <qeesung@live.com>

repack works out which packs are redundant by looking for ".keep"
files when it starts, then runs pack-objects, which looks for them all
over again when it opens the packs. Two scans of the same directory,
seconds apart, with nothing holding them together.

A ".keep" that turns up in between loses objects. The parent did not
see it, so the pack is on its list to delete. The child does see it,
so it leaves that pack to itself: its objects are not copied into the
new pack, not loosened for "-A", not kept for "-k" or "--cruft". The
parent deletes the pack regardless, as repack_remove_redundant_pack()
passes force_delete, which skips the ".keep" check in
unlink_pack_path(). The objects are gone and repack exits successfully.

The gap is easy to land in. index-pack names a pack after its contents
and writes its ".keep" before it renames the packfile into place, so a
"git fetch", or a push being migrated out of its quarantine, that
brings in a pack identical to one we already have puts a ".keep" next
to that pack. Checking for the ".keep" once more right before deleting
would not help: a push holds it for a fraction of a second, and it may
well be gone again by the time pack-objects has finished.

When we may delete packs, that is with "-d" and either "-a", "-A",
"--cruft" or "--geometric", write the names of the packs we may
delete, the non-kept and cruft packs, to a file next to the refs
snapshot we already write for "git multi-pack-index write", and pass it
to every pack-objects we spawn with "--ignore-pack-keep-from".
pack-objects then never treats those packs as kept, whatever appears
on the disk. Every other pack, whether kept when we started or showing
up later, still goes by its ".keep" file, so "--honor-pack-keep"
stays. Otherwise nothing gets deleted and nothing is passed.

The file has one name per line. A name with a newline in it gets split
there, which at worst makes some other pack not count as kept: its
objects may be copied, but none are lost.

Unreachable objects of such a pack are still dropped by "-a -d", or by
"--cruft-expiration" and "--unpack-unreachable" once they are old
enough, as these drop unreachable objects by design, and a pack written
again under the same name does not make them any younger; what changes
is that the objects that are to be kept no longer go with them.

The tests have a ".keep" appear on the pack about to be deleted as
pack-objects starts, through a "git" wrapper found via "--exec-path",
for "-a", "-A", "-k", "--cruft" with and without an expiration,
"--pack-kept-objects", "--filter" and "--geometric".

Signed-off-by: Qin ShiCheng <qeesung@live.com>
---
 builtin/repack.c            | 23 +++++++++++++++++
 repack.c                    | 26 ++++++++++++++++++++
 repack.h                    | 14 +++++++++--
 t/t7700-repack.sh           | 49 +++++++++++++++++++++++++++++++++++++
 t/t7703-repack-geometric.sh | 39 +++++++++++++++++++++++++++++
 5 files changed, 149 insertions(+), 2 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 5d06872d77..8faf7ce2ef 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -457,6 +457,28 @@ int cmd_repack(int argc,
 	existing.repo = repo;
 	existing_packs_collect(&existing, &keep_pack_list);
 
+	/*
+	 * Tell pack-objects which packs we may delete. pack-objects looks
+	 * for ".keep" files on its own, and one that appears on such a
+	 * pack from now on must not make it leave out objects that would
+	 * then be deleted with the pack.
+	 */
+	if (delete_redundant &&
+	    ((pack_everything & ALL_INTO_ONE) || geometry.split_factor) &&
+	    existing_packs_has_non_kept(&existing)) {
+		struct strbuf path = STRBUF_INIT;
+		struct tempfile *non_kept_packs;
+
+		strbuf_addf(&path, "%s/tmp_non_kept_packs_XXXXXX",
+			    repo_get_object_directory(repo));
+
+		non_kept_packs = xmks_tempfile(path.buf);
+		existing_packs_write_non_kept(&existing, non_kept_packs);
+		po_args.ignore_pack_keep_from = get_tempfile_path(non_kept_packs);
+
+		strbuf_release(&path);
+	}
+
 	if (geometry.split_factor) {
 		if (pack_everything)
 			die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a");
@@ -644,6 +666,7 @@ int cmd_repack(int argc,
 		cruft_po_args.quiet = po_args.quiet;
 		cruft_po_args.delta_base_offset = po_args.delta_base_offset;
 		cruft_po_args.pack_kept_objects = 0;
+		cruft_po_args.ignore_pack_keep_from = po_args.ignore_pack_keep_from;
 
 		ret = write_cruft_pack(&opts, cruft_expiration,
 				       combine_cruft_below_size, &names,
diff --git a/repack.c b/repack.c
index 53c34197f3..3af8c7a5f8 100644
--- a/repack.c
+++ b/repack.c
@@ -40,6 +40,9 @@ void prepare_pack_objects(struct child_process *cmd,
 		strvec_push(&cmd->args,  "--delta-base-offset");
 	if (!args->pack_kept_objects)
 		strvec_push(&cmd->args,  "--honor-pack-keep");
+	if (args->ignore_pack_keep_from)
+		strvec_pushf(&cmd->args, "--ignore-pack-keep-from=%s",
+			     args->ignore_pack_keep_from);
 	strvec_push(&cmd->args, out);
 	cmd->git_cmd = 1;
 	cmd->out = -1;
@@ -172,6 +175,29 @@ int existing_packs_has_non_kept(const struct existing_packs *existing)
 	return existing->non_kept_packs.nr || existing->cruft_packs.nr;
 }
 
+void existing_packs_write_non_kept(const struct existing_packs *existing,
+				   struct tempfile *f)
+{
+	struct string_list_item *item;
+	FILE *out = fdopen_tempfile(f, "w");
+
+	if (!out)
+		die(_("could not open tempfile %s for writing"),
+		    get_tempfile_path(f));
+
+	for_each_string_list_item(item, &existing->non_kept_packs)
+		fprintf(out, "%s.pack\n", item->string);
+	for_each_string_list_item(item, &existing->cruft_packs)
+		fprintf(out, "%s.pack\n", item->string);
+
+	if (close_tempfile_gently(f)) {
+		int save_errno = errno;
+		delete_tempfile(&f);
+		errno = save_errno;
+		die_errno(_("could not close non-kept packs tempfile"));
+	}
+}
+
 static void existing_pack_mark_for_deletion(struct string_list_item *item)
 {
 	item->util = (void*)((uintptr_t)item->util | DELETE_PACK);
diff --git a/repack.h b/repack.h
index 61e554e4ed..e3bc840b9a 100644
--- a/repack.h
+++ b/repack.h
@@ -19,6 +19,11 @@ struct pack_objects_args {
 	int path_walk;
 	int delta_base_offset;
 	int pack_kept_objects;
+	/*
+	 * A file naming the packs this repack may delete, for
+	 * pack-objects' "--ignore-pack-keep-from"; the path is borrowed.
+	 */
+	const char *ignore_pack_keep_from;
 	struct list_objects_filter_options filter_options;
 };
 
@@ -28,6 +33,7 @@ struct pack_objects_args {
 }
 
 struct child_process;
+struct tempfile;
 
 void prepare_pack_objects(struct child_process *cmd,
 			  const struct pack_objects_args *args,
@@ -80,6 +86,12 @@ struct existing_packs {
 void existing_packs_collect(struct existing_packs *existing,
 			    const struct string_list *extra_keep);
 int existing_packs_has_non_kept(const struct existing_packs *existing);
+/*
+ * Write the names of the non-kept and cruft packs, the ones a repack may
+ * delete, to "f", one per line, for pack-objects' "--ignore-pack-keep-from".
+ */
+void existing_packs_write_non_kept(const struct existing_packs *existing,
+				   struct tempfile *f);
 int existing_pack_is_marked_for_deletion(struct string_list_item *item);
 void existing_packs_retain_cruft(struct existing_packs *existing,
 				 struct packed_git *cruft);
@@ -138,8 +150,6 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,
 				    bool wrote_incremental_midx);
 void pack_geometry_release(struct pack_geometry *geometry);
 
-struct tempfile;
-
 enum repack_write_midx_mode {
 	REPACK_WRITE_MIDX_NONE,
 	REPACK_WRITE_MIDX_DEFAULT,
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 00fe259767..79fa3189f6 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -941,4 +941,53 @@ test_expect_success 'pending objects are repacked appropriately' '
 	)
 '
 
+# Put a reachable commit and an unreachable one into a single pack X in a
+# new repository "keep-race", run "git repack" with the given options and
+# have a ".keep" appear on X as pack-objects starts, i.e. after the repack
+# has decided to delete X. The unreachable commit is saved in the file
+# "keep-race/unreachable".
+keep_race_repack () {
+	git init keep-race &&
+	(
+		cd keep-race &&
+		test_commit base &&
+		git commit-tree -m unreachable HEAD^{tree} >unreachable &&
+
+		git rev-list --objects --all $(cat unreachable) >objects &&
+		X=$(git pack-objects .git/objects/pack/pack <objects) &&
+		git prune-packed &&
+
+		mkdir shim &&
+		write_script shim/git <<-EOF &&
+		test "\$1" = "pack-objects" &&
+		>"$(pwd)/.git/objects/pack/pack-$X.keep" &&
+		>"$(pwd)/keep-created"
+		GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
+		EOF
+
+		git --exec-path="$(pwd)/shim" repack "$@" &&
+		test_path_is_file keep-created &&
+		test_path_is_missing .git/objects/pack/pack-$X.pack &&
+		git fsck
+	)
+}
+
+for args in "-a -d" "-a -d --filter=blob:none"
+do
+	test_expect_success "a .keep that shows up mid-repack loses nothing: $args" '
+		test_when_finished "rm -fr keep-race" &&
+		keep_race_repack $args
+	'
+done
+
+for args in "-A -d" "-A -d --pack-kept-objects" "-a -k -d" "--cruft -d" \
+	    "--cruft --cruft-expiration=1.week.ago -d"
+do
+	test_expect_success "a .keep that shows up mid-repack loses nothing: $args" '
+		test_when_finished "rm -fr keep-race" &&
+		keep_race_repack $args &&
+		git -C keep-race cat-file -e $(cat keep-race/unreachable)
+	'
+done
+
 test_done
diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index f3a0650cfe..7a5e3b9ada 100755
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -541,4 +541,43 @@ test_expect_success 'geometric repack works with promisor packs' '
 	)
 '
 
+test_expect_success 'a ".keep" that shows up mid-repack does not lose objects' '
+	test_when_finished "rm -fr race" &&
+	git init race &&
+	(
+		cd race &&
+
+		test_commit kept &&
+		test_commit pack &&
+
+		KEPT=$(git pack-objects --revs $packdir/pack <<-EOF
+		refs/tags/kept
+		EOF
+		) &&
+		git pack-objects --revs $packdir/pack <<-EOF &&
+		refs/tags/pack
+		^refs/tags/kept
+		EOF
+		git prune-packed &&
+
+		# Neither pack is twice the size of the other, so both are
+		# rolled up and deleted. Have a ".keep" appear on one of them
+		# as pack-objects starts, after the repack has decided to
+		# delete it; its objects must still end up in the new pack.
+		mkdir shim &&
+		write_script shim/git <<-EOF &&
+		test "\$1" = "pack-objects" &&
+		>"$(pwd)/$packdir/pack-$KEPT.keep" &&
+		>"$(pwd)/keep-created"
+		GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
+		EOF
+
+		git --exec-path="$(pwd)/shim" repack --geometric 2 -d &&
+		test_path_is_file keep-created &&
+		test_path_is_missing $packdir/pack-$KEPT.pack &&
+
+		git fsck
+	)
+'
+
 test_done
-- 
gitgitgadget