[PATCH 2/2] repack: also copy loose objects excluded by --filter-to-loose

["Adithya Chakilam via GitGitGadget" <gitgitgadget@gmail.com>]

From: Adithya Chakilam <achakilam@nvidia.com>

--filter-to-loose only writes objects that were in an existing pack,
because the filtered pack is built with --stdin-packs. Loose objects
that the filter excludes stay behind in the source repository, so the
destination does not get the complete set of filtered-out objects.

Copy such loose objects to the destination as well. An object already
present in the destination is not rewritten. With -d, the source copies
are removed once everything is written.

Signed-off-by: Adithya Chakilam <achakilam@nvidia.com>
---
 Documentation/git-repack.adoc |   4 ++
 builtin/repack.c              |  11 ++++
 repack-filtered.c             | 108 ++++++++++++++++++++++++++++++++++
 repack.h                      |  13 ++++
 t/t7700-repack.sh             |  59 +++++++++++++++++++
 5 files changed, 195 insertions(+)

diff --git a/Documentation/git-repack.adoc b/Documentation/git-repack.adoc
index df1390154f..e2025c29ee 100644
--- a/Documentation/git-repack.adoc
+++ b/Documentation/git-repack.adoc
@@ -191,6 +191,10 @@ depth is 4095.
 	not rewritten. All objects are written before any existing
 	pack is removed.
 
+	Loose objects in this repository that the filter excludes are
+	copied to `<dir>` as well, and with `-d` the source copies are
+	removed once everything has been written.
+
 --drop-filtered::
 	Delete the local objects that match the `--filter` specification
 	instead of keeping them in a separate packfile, reclaiming the
diff --git a/builtin/repack.c b/builtin/repack.c
index 140a992f1b..a946ad28a3 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -183,6 +183,7 @@ int cmd_repack(int argc,
 	const char *expire_to = NULL;
 	const char *filter_to = NULL;
 	const char *filter_to_loose = NULL;
+	struct oidset moved_loose = OIDSET_INIT;
 	const char *opt_window = NULL;
 	const char *opt_window_memory = NULL;
 	const char *opt_depth = NULL;
@@ -721,6 +722,14 @@ int cmd_repack(int argc,
 		ret = write_filtered_pack(&opts, &existing, &names);
 		if (ret)
 			goto cleanup;
+
+		if (filter_to_loose) {
+			ret = move_loose_filtered_objects(repo,
+					&po_args.filter_options, filter_to,
+					&moved_loose);
+			if (ret)
+				goto cleanup;
+		}
 	}
 
 	string_list_sort(&names);
@@ -777,6 +786,7 @@ int cmd_repack(int argc,
 		if (show_progress)
 			opts |= PRUNE_PACKED_VERBOSE;
 		prune_packed_objects(repo, opts);
+		remove_moved_loose_objects(repo, &moved_loose);
 
 		if (!keep_unreachable &&
 		    (!(pack_everything & LOOSEN_UNREACHABLE) ||
@@ -801,6 +811,7 @@ cleanup:
 	string_list_clear(&keep_pack_list, 0);
 	string_list_clear(&names, 1);
 	oidset_clear(&drop_oids);
+	oidset_clear(&moved_loose);
 	existing_packs_release(&existing);
 	pack_geometry_release(&geometry);
 	pack_objects_args_release(&po_args);
diff --git a/repack-filtered.c b/repack-filtered.c
index 51c9dfeb13..cb1f034f09 100644
--- a/repack-filtered.c
+++ b/repack-filtered.c
@@ -9,6 +9,11 @@
 #include "list-objects-filter.h"
 #include "odb.h"
 #include "promisor-remote.h"
+#include "copy.h"
+#include "object-file.h"
+#include "odb/source-files.h"
+#include "oidset.h"
+#include "path.h"
 #include "sigchain.h"
 
 static void start_unpack_objects(struct child_process *unpack,
@@ -171,3 +176,106 @@ cleanup:
 	oidset_clear(&all_promisor_blobs);
 	return ret;
 }
+
+static int collect_loose_oid(const struct object_id *oid,
+			     const char *path UNUSED, void *cb_data)
+{
+	oidset_insert(cb_data, oid);
+	return 0;
+}
+
+static void dest_loose_path(struct strbuf *buf, const char *objdir,
+			    const struct object_id *oid)
+{
+	const char *hex = oid_to_hex(oid);
+
+	strbuf_reset(buf);
+	strbuf_addf(buf, "%s/%.2s/%s", objdir, hex, hex + 2);
+}
+
+int move_loose_filtered_objects(struct repository *repo,
+				const struct list_objects_filter_options *filter,
+				const char *dest_objdir,
+				struct oidset *moved)
+{
+	struct odb_source_files *files =
+		odb_source_files_downcast(repo->objects->source);
+	struct oidset loose = OIDSET_INIT;
+	struct oidset omitted = OIDSET_INIT;
+	struct oidset_iter iter;
+	const struct object_id *oid;
+	struct strbuf src = STRBUF_INIT;
+	struct strbuf dst = STRBUF_INIT;
+	struct strbuf tmp = STRBUF_INIT;
+	int ret = 0;
+
+	if (filter->choice != LOFC_BLOB_LIMIT)
+		return 0;
+
+	ret = for_each_loose_file_in_source(&files->dirs->loose->base,
+					    collect_loose_oid, NULL, NULL,
+					    &loose);
+	if (ret)
+		goto out;
+	ret = list_objects_filter__filter_oidset(repo, filter, &loose,
+						 &omitted);
+	if (ret)
+		goto out;
+
+	oidset_iter_init(&omitted, &iter);
+	while ((oid = oidset_iter_next(&iter))) {
+		odb_loose_path(files->dirs->loose, &src, oid);
+		dest_loose_path(&dst, dest_objdir, oid);
+
+		/*
+		 * Only the destination decides whether the copy is needed;
+		 * the object being present in the source says nothing.
+		 */
+		if (!access(dst.buf, F_OK)) {
+			oidset_insert(moved, oid);
+			continue;
+		}
+
+		if (safe_create_leading_directories(repo, dst.buf) < 0) {
+			ret = error_errno(_("unable to create leading directories of '%s'"),
+					  dst.buf);
+			goto out;
+		}
+		strbuf_reset(&tmp);
+		strbuf_addf(&tmp, "%s.tmp-%"PRIuMAX, dst.buf,
+			    (uintmax_t)getpid());
+		if (copy_file(repo, tmp.buf, src.buf, 0444) ||
+		    finalize_object_file(repo, tmp.buf, dst.buf)) {
+			unlink(tmp.buf);
+			ret = error(_("unable to copy loose object '%s' to '%s'"),
+				    src.buf, dst.buf);
+			goto out;
+		}
+		oidset_insert(moved, oid);
+	}
+
+out:
+	oidset_clear(&loose);
+	oidset_clear(&omitted);
+	strbuf_release(&src);
+	strbuf_release(&dst);
+	strbuf_release(&tmp);
+	return ret;
+}
+
+void remove_moved_loose_objects(struct repository *repo,
+				const struct oidset *moved)
+{
+	struct odb_source_files *files =
+		odb_source_files_downcast(repo->objects->source);
+	struct strbuf path = STRBUF_INIT;
+	struct oidset_iter iter;
+	const struct object_id *oid;
+
+	oidset_iter_init(moved, &iter);
+	while ((oid = oidset_iter_next(&iter))) {
+		odb_loose_path(files->dirs->loose, &path, oid);
+		unlink(path.buf);
+	}
+	strbuf_release(&path);
+}
diff --git a/repack.h b/repack.h
index d5528745ec..18a0dc6314 100644
--- a/repack.h
+++ b/repack.h
@@ -168,6 +168,19 @@ int write_filtered_pack(const struct write_pack_opts *opts,
 			struct existing_packs *existing,
 			struct string_list *names);
 
+/*
+ * Copy loose objects that the filter excludes into the loose-object store
+ * at `dest_objdir`, recording them in `moved`.
+ */
+int move_loose_filtered_objects(struct repository *repo,
+				const struct list_objects_filter_options *filter,
+				const char *dest_objdir,
+				struct oidset *moved);
+
+/* Delete the source copies of objects copied by move_loose_filtered_objects. */
+void remove_moved_loose_objects(struct repository *repo,
+				const struct oidset *moved);
+
 int enumerate_promisor_blobs(struct repository *repo,
 			     const struct list_objects_filter_options *filter,
 			     struct oidset *to_drop);
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 5aa4b698f1..37dc5fdb13 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -547,6 +547,44 @@ setup_loose_src () {
 	fi
 }
 
+test_expect_success '--filter-to-loose copies loose filtered out blobs, and -d removes the source copy' '
+	setup_loose_src loose &&
+	test_path_is_file loose-src.git/objects/$(test_oid_to_path $big) &&
+
+	# without -d the source copy is left alone
+	git -C loose-src.git -c repack.writebitmaps=false repack -a \
+		--filter=blob:limit=1k --filter-to-loose=../loose-dst.git/objects &&
+	test_path_is_file loose-dst.git/objects/$(test_oid_to_path $big) &&
+	test_path_is_file loose-src.git/objects/$(test_oid_to_path $big) &&
+
+	# with -d it is removed from the source after being copied
+	rm -rf loose-dst.git/objects/$(dirname $(test_oid_to_path $big)) &&
+	git -C loose-src.git -c repack.writebitmaps=false repack -a -d \
+		--filter=blob:limit=1k --filter-to-loose=../loose-dst.git/objects &&
+	test_path_is_file loose-dst.git/objects/$(test_oid_to_path $big) &&
+	test_path_is_missing loose-src.git/objects/$(test_oid_to_path $big) &&
+	echo $(pwd)/loose-dst.git/objects >loose-src.git/objects/info/alternates &&
+	git -C loose-src.git cat-file -e $big
+'
+
+test_expect_success '--filter-to-loose leaves an identical existing destination object alone' '
+	setup_loose_src &&
+
+	test-tool genrandom big 2k |
+	GIT_OBJECT_DIRECTORY=loose-dst.git/objects git hash-object -w --stdin &&
+
+	path=loose-dst.git/objects/$(test_oid_to_path $big) &&
+	test_path_is_file $path &&
+
+	ls -i $path >before &&
+	git -C loose-src.git -c repack.writebitmaps=false repack -a -d \
+		--filter=blob:limit=1k --filter-to-loose=../loose-dst.git/objects &&
+
+	ls -i $path >after &&
+	test_cmp before after &&
+	git -C loose-dst.git fsck
+'
+
 test_expect_success '--filter-to-loose failure to write leaves the source intact' '
 	setup_loose_src &&
 	>not-a-dir &&
@@ -564,6 +602,27 @@ test_expect_success '--filter-to-loose failure to write leaves the source intact
 	git -C loose-src.git cat-file -e $big
 '
 
+test_expect_success '--filter-to-loose failure to copy a loose object leaves the source intact' '
+	setup_loose_src loose &&
+	path=$(test_oid_to_path $big) &&
+	>loose-dst.git/objects/$(dirname $path) &&
+
+	find loose-src.git/objects/pack -name "*.pack" | sort >packs-before &&
+	test_must_fail git -C loose-src.git -c repack.writebitmaps=false \
+		repack -a -d --filter=blob:limit=1k \
+		--filter-to-loose=../loose-dst.git/objects 2>err &&
+
+	test_grep "unable to create leading directories" err &&
+	find loose-src.git/objects/pack -name "*.pack" | sort >packs-after &&
+	test_cmp packs-before packs-after &&
+
+	find loose-src.git/objects/pack -name ".tmp-*" >tmp-leftovers &&
+	test_must_be_empty tmp-leftovers &&
+
+	test_path_is_file loose-src.git/objects/$path &&
+	git -C loose-src.git cat-file -e $big
+'
+
 test_expect_success '--filter-to-loose does not move objects from kept packs' '
 	setup_loose_src &&
 
-- 
gitgitgadget