repack: tell pack-objects which packs are kept

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

A ".keep" that turns up in between loses objects. The parent did not
see it, so the pack is on its list to delete. The child does see it,
so it leaves that pack's objects out of the replacement. The parent
deletes the pack regardless: repack_remove_redundant_pack() passes
force_delete, which skips the ".keep" check in unlink_pack_path(). The
objects are gone and repack exits successfully.

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

Hand pack-objects the kept packs we collected at startup and drop
"--honor-pack-keep". Both processes then work from one snapshot, and a
".keep" appearing or disappearing while we run cannot make them
disagree. An earlier commit made sure a pack kept this way is no more
of a boundary to the traversal than a ".keep" file was.

The list goes into a file next to the refs snapshot we already write
for "git multi-pack-index write", and is passed with
"--keep-pack-from-file" to every pack-objects we spawn when
"--pack-kept-objects" is not in effect, which is when
"--honor-pack-keep" used to be. The cruft pack-objects already has the
kept packs on its stdin; the file is redundant there, but it sees the
same list as everybody else. With nothing to keep, no file is written
and nothing is passed, which is what "--honor-pack-keep" came down to
when it found no ".keep".

The names go one per line, so a name with a newline in it cannot be
passed. "--stdin-packs" and "--cruft" have the same limit and die on a
name they cannot find, but "--keep-pack" ignores such a name, and the
two halves of a garbled one could go on to exclude some other pack;
refuse it up front instead.

The user's own "--keep-pack" arguments keep being forwarded, since
they apply either way. write_filtered_pack() had a loop passing the
kept packs too, but without the ".pack" suffix pack-objects compares
against; it goes. Kept packs borrowed from an alternate object
directory were covered by "--honor-pack-keep" and are not by the
snapshot, which only ever held local packs; repack never deletes
those, so their objects now get packed rather than skipped, which
costs room but cannot lose anything.

Signed-off-by: Qin ShiCheng <qeesung@live.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Author: qeesung

builtin/repack.c

@@ -167,6 +167,7 @@ int cmd_repack(int argc,
 	struct oidset drop_oids = OIDSET_INIT;
 	struct pack_geometry geometry = { 0 };
 	struct tempfile *refs_snapshot = NULL;
+	struct tempfile *kept_packs_snapshot = NULL;
 	int i, ret;
 	int show_progress;
 
@@ -456,6 +457,19 @@ int cmd_repack(int argc,
 
 	existing.repo = repo;
 	existing_packs_collect(&existing, &keep_pack_list);
+	if (existing.kept_packs.nr) {
+		struct strbuf path = STRBUF_INIT;
+
+		strbuf_addf(&path, "%s/%s_XXXXXX",
+			    repo_get_object_directory(repo), "kept-packs");
+
+		kept_packs_snapshot = xmks_tempfile(path.buf);
+		existing_packs_snapshot_kept(&existing, kept_packs_snapshot);
+		po_args.kept_packs_snapshot =
+			get_tempfile_path(kept_packs_snapshot);
+
+		strbuf_release(&path);
+	}
 
 	if (geometry.split_factor) {
 		if (pack_everything)
@@ -644,6 +658,7 @@ int cmd_repack(int argc,
 		cruft_po_args.quiet = po_args.quiet;
 		cruft_po_args.delta_base_offset = po_args.delta_base_offset;
 		cruft_po_args.pack_kept_objects = 0;
+		cruft_po_args.kept_packs_snapshot = po_args.kept_packs_snapshot;
 
 		ret = write_cruft_pack(&opts, cruft_expiration,
 				       combine_cruft_below_size, &names,

repack-filtered.c

@@ -25,9 +25,6 @@ int write_filtered_pack(const struct write_pack_opts *opts,
 
 	strvec_push(&cmd.args, "--stdin-packs");
 
-	for_each_string_list_item(item, &existing->kept_packs)
-		strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);
-
 	cmd.in = -1;
 
 	ret = start_command(&cmd);

repack.c

@@ -38,8 +38,9 @@ void prepare_pack_objects(struct child_process *cmd,
 		strvec_push(&cmd->args,  "--quiet");
 	if (args->delta_base_offset)
 		strvec_push(&cmd->args,  "--delta-base-offset");
-	if (!args->pack_kept_objects)
-		strvec_push(&cmd->args,  "--honor-pack-keep");
+	if (!args->pack_kept_objects && args->kept_packs_snapshot)
+		strvec_pushf(&cmd->args, "--keep-pack-from-file=%s",
+			     args->kept_packs_snapshot);
 	strvec_push(&cmd->args, out);
 	cmd->git_cmd = 1;
 	cmd->out = -1;
@@ -167,6 +168,35 @@ void existing_packs_collect(struct existing_packs *existing,
 	strbuf_release(&buf);
 }
 
+void existing_packs_snapshot_kept(const struct existing_packs *existing,
+				  struct tempfile *f)
+{
+	struct string_list_item *item;
+	FILE *out = fdopen_tempfile(f, "w");
+
+	if (!out)
+		die(_("could not open tempfile %s for writing"),
+		    get_tempfile_path(f));
+
+	for_each_string_list_item(item, &existing->kept_packs) {
+		/*
+		 * A newline would split the name in two, and pack-objects
+		 * quietly keeps whichever packs the halves happen to name.
+		 */
+		if (strchr(item->string, '\n'))
+			die(_("cannot keep pack '%s': its name contains a newline"),
+			    item->string);
+		fprintf(out, "%s.pack\n", item->string);
+	}
+
+	if (close_tempfile_gently(f)) {
+		int save_errno = errno;
+		delete_tempfile(&f);
+		errno = save_errno;
+		die_errno(_("could not close kept packs snapshot tempfile"));
+	}
+}
+
 int existing_packs_has_non_kept(const struct existing_packs *existing)
 {
 	return existing->non_kept_packs.nr || existing->cruft_packs.nr;

repack.h

@@ -19,6 +19,14 @@ struct pack_objects_args {
 	int path_walk;
 	int delta_base_offset;
 	int pack_kept_objects;
+	/*
+	 * File naming the packs to leave alone, one "<name>.pack" per line;
+	 * NULL when there are none. pack-objects reads it rather than
+	 * looking for ".keep" files itself, so that a ".keep" created or
+	 * removed while we run cannot make the two of us disagree over
+	 * which packs are being repacked.
+	 */
+	const char *kept_packs_snapshot;
 	struct list_objects_filter_options filter_options;
 };
 
@@ -28,6 +36,7 @@ struct pack_objects_args {
 }
 
 struct child_process;
+struct tempfile;
 
 void prepare_pack_objects(struct child_process *cmd,
 			  const struct pack_objects_args *args,
@@ -79,6 +88,12 @@ struct existing_packs {
  */
 void existing_packs_collect(struct existing_packs *existing,
 			    const struct string_list *extra_keep);
+/*
+ * Writes the names of the kept packs, one "<name>.pack" per line, into
+ * the given tempfile, for pack-objects to read with --keep-pack-from-file.
+ */
+void existing_packs_snapshot_kept(const struct existing_packs *existing,
+				  struct tempfile *f);
 int existing_packs_has_non_kept(const struct existing_packs *existing);
 int existing_pack_is_marked_for_deletion(struct string_list_item *item);
 void existing_packs_retain_cruft(struct existing_packs *existing,
@@ -138,8 +153,6 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,
 				    bool wrote_incremental_midx);
 void pack_geometry_release(struct pack_geometry *geometry);
 
-struct tempfile;
-
 enum repack_write_midx_mode {
 	REPACK_WRITE_MIDX_NONE,
 	REPACK_WRITE_MIDX_DEFAULT,

t/t7700-repack.sh

@@ -254,6 +254,49 @@ test_expect_success 'repack --keep-pack' '
 	)
 '
 
+test_expect_success 'repack --keep-pack with --pack-kept-objects' '
+	test_create_repo keep-pack-kept-objects &&
+	(
+		cd keep-pack-kept-objects &&
+		git config pack.window 0 &&
+		git config maintenance.auto false &&
+		P1=$(commit_and_pack 1) &&
+		P2=$(commit_and_pack 2) &&
+
+		# "--pack-kept-objects" is about packs that have a ".keep"
+		# file. A pack named with "--keep-pack" stays out of the
+		# result regardless, objects included.
+		git repack -a -d --pack-kept-objects --keep-pack $P1 &&
+		ls .git/objects/pack/*.pack >counts &&
+		test_line_count = 2 counts &&
+		test-tool find-pack -c 1 HEAD~1 &&
+		test-tool find-pack -c 1 HEAD~1: &&
+		git fsck
+	)
+'
+
+test_expect_success FUNNYNAMES 'a kept pack whose name has a newline is refused' '
+	test_create_repo keep-pack-newline &&
+	(
+		cd keep-pack-newline &&
+		git config maintenance.auto false &&
+		test_commit base &&
+		git repack -ad &&
+
+		# The names pack-objects is told to keep go one per line, so
+		# this one would come out as two, and the first of them is
+		# the name of the pack holding everything else.
+		victim="$(basename "$(ls .git/objects/pack/pack-*.pack)")" &&
+		name="$(printf "%s\nother" "$victim")" &&
+		P=$(git rev-parse HEAD | git pack-objects ".git/objects/pack/$name") &&
+		>".git/objects/pack/$name-$P.keep" &&
+
+		test_must_fail git repack -ad 2>err &&
+		test_grep "contains a newline" err &&
+		git fsck
+	)
+'
+
 test_expect_success 'repacking fails when missing .pack actually means missing objects' '
 	test_create_repo idx-without-pack &&
 	(

t/t7703-repack-geometric.sh

@@ -541,4 +541,76 @@ test_expect_success 'geometric repack works with promisor packs' '
 	)
 '
 
+test_expect_success 'a ".keep" that shows up mid-repack does not lose objects' '
+	test_when_finished "rm -fr race" &&
+	git init race &&
+	(
+		cd race &&
+
+		test_commit kept &&
+		test_commit pack &&
+
+		KEPT=$(git pack-objects --revs $packdir/pack <<-EOF
+		refs/tags/kept
+		EOF
+		) &&
+		git pack-objects --revs $packdir/pack <<-EOF &&
+		refs/tags/pack
+		^refs/tags/kept
+		EOF
+		git prune-packed &&
+
+		# Neither pack is twice the size of the other, so both are
+		# redundant and get deleted. Have a ".keep" appear on one of
+		# them as pack-objects starts, after the repack has decided
+		# to delete it: pack-objects used to notice the ".keep" and
+		# leave those objects out of the replacement pack.
+		mkdir shim &&
+		write_script shim/git <<-EOF &&
+		test "\$1" = "pack-objects" && >"$(pwd)/$packdir/pack-$KEPT.keep"
+		GIT_EXEC_PATH="$GIT_EXEC_PATH" exec "$GIT_EXEC_PATH/git" "\$@"
+		EOF
+
+		git --exec-path="$(pwd)/shim" repack --geometric 2 -d &&
+
+		git fsck
+	)
+'
+
+test_expect_success 'a kept pack does not stop the traversal from rescuing objects' '
+	test_when_finished "rm -fr kept-open" &&
+	git init kept-open &&
+	(
+		cd kept-open &&
+		git config repack.midxMustContainCruft false &&
+
+		test_commit a &&
+		test_commit b &&
+		b=$(git rev-parse b) &&
+		git repack -ad &&
+
+		# Make "b" unreachable and sweep it, together with its tree
+		# and blob, into a cruft pack.
+		git tag -d b &&
+		git reset --hard a &&
+		git reflog expire --all --expire=all &&
+		git repack -ad --cruft &&
+
+		# Bring the commit back on its own, in a pack marked as kept.
+		# Its tree and blob are still only in the cruft pack.
+		kept=$(echo $b | git pack-objects $packdir/pack) &&
+		>$packdir/pack-$kept.keep &&
+
+		# Build on top of it, so that the repack has to look through
+		# the kept pack to find out what the new commit depends on.
+		git update-ref refs/heads/master \
+			$(git commit-tree a^{tree} -p $b -m c) &&
+
+		git repack --geometric 2 -d --write-midx --write-bitmap-index &&
+		test_path_is_file $packdir/multi-pack-index &&
+		ls $packdir/multi-pack-index-*.bitmap >bitmaps &&
+		test_line_count = 1 bitmaps
+	)
+'
+
 test_done