Re: [RFC PATCH 6/7] builtin/repack: actually drop filtered promisor blobs

Make --drop-filtered remove the enumerated promisor blobs instead of
only listing them.

The drop set is computed before repack_promisor_objects() runs, and on
a real run it is passed in so the rebuilt promisor pack omits those
blobs. --drop-filtered implies -d so the old promisor packs, which
still contain the dropped blobs, are removed. Without this the blobs
would survive in the redundant packs. The existing repack machinery
performs the write-before-delete and fsync, so the drop is crash-safe.

The dropped blobs become absent locally but remain recoverable from the
promisor remote, so a later access lazy-fetches them back
transparently. --dry-run keeps its previous behavior, i.e. it lists the
candidates and changes nothing.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Siddharth Asthana [off-list ref]
Signed-off-by: Siddharth Shrimali <redacted>
---
  builtin/repack.c                | 75 ++++++++++++++++++---------------
  repack-filtered.c               | 17 ++------
  repack.h                        |  4 +-
  t/t7706-repack-drop-filtered.sh | 18 +++++---
  4 files changed, 59 insertions(+), 55 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index c2b07477d2..aa3257a98a 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -15,6 +15,8 @@
  #include "repack.h"
  #include "shallow.h"
  #include "list-objects-filter-options.h"
+#include "oidset.h"
+#include "hex.h"
  
  #define ALL_INTO_ONE 1
  #define LOOSEN_UNREACHABLE 2

@@ -143,6 +145,7 @@ int cmd_repack(int argc,
  	struct string_list_item *item;
  	struct string_list names = STRING_LIST_INIT_DUP;
  	struct existing_packs existing = EXISTING_PACKS_INIT;
+	struct oidset drop_oids = OIDSET_INIT;
  	struct pack_geometry geometry = { 0 };
  	struct tempfile *refs_snapshot = NULL;
  	int i, ret;

@@ -269,9 +272,6 @@ int cmd_repack(int argc,
  		die(_("--dry-run only takes effect with --drop-filtered"));
  
  	if (drop_filtered) {
-		if (!dry_run)
-			die(_("--drop-filtered doesn't work without --dry-run yet"));
-
  		if (!po_args.filter_options.choice)
  			die(_("--drop-filtered requires --filter"));
  

@@ -294,6 +294,28 @@ int cmd_repack(int argc,
  			die(_("--drop-filtered requires a promisor remote"));
  
  		write_bitmaps = 0;
+
+		/*
+		 * Dropping objects means rebuilding the promisor packs
+		 * without them and then removing the old packs, so the
+		 * redundant packs must be deleted. Imply -d on a real run.
+		 */
+		if (!dry_run)
+			delete_redundant = 1;

+
+		ret = enumerate_promisor_blobs(repo, &po_args.filter_options, &drop_oids);
+
+		if (ret)
+			goto cleanup;
+
+		if (dry_run) {
+			struct oidset_iter iter;
+			const struct object_id *oid;
+
+			oidset_iter_init(&drop_oids, &iter);
+			while ((oid = oidset_iter_next(&iter)))
+				printf("%s\n", oid_to_hex(oid));
+		}
  	}
  
  	if (delete_redundant && repo->repository_format_precious_objects)

@@ -406,7 +428,8 @@ int cmd_repack(int argc,
  		strvec_push(&cmd.args, "--delta-islands");
  
  	if (pack_everything & ALL_INTO_ONE) {
-		repack_promisor_objects(repo, &po_args, &names, packtmp, NULL);
+		repack_promisor_objects(repo, &po_args, &names, packtmp,
+			(drop_filtered && !dry_run) ? &drop_oids : NULL);
  
  		if (existing_packs_has_non_kept(&existing) &&
  		    delete_redundant &&

@@ -589,35 +612,20 @@ int cmd_repack(int argc,
  		}
  	}
  
-	if (po_args.filter_options.choice) {
-		if (drop_filtered) {
-			/*
-			 * Enumerate promisor objects directly rather than
-			 * going through write_filtered_pack(). The filter
-			 * machinery cannot see promisor objects because
-			 * repack_promisor_objects() handles them separately
-			 * before the filter runs.
-			 */
-			ret = enumerate_promisor_blobs(repo,
-					&po_args.filter_options,
-					dry_run);
-			if (ret)
-				goto cleanup;
-		} else {
-			struct write_pack_opts opts = {
-				.po_args = &po_args,
-				.destination = filter_to,
-				.packdir = packdir,
-				.packtmp = packtmp,
-			};
-
-			if (!opts.destination)
-				opts.destination = packtmp;
-
-			ret = write_filtered_pack(&opts, &existing, &names);
-			if (ret)
-				goto cleanup;
-		}
+	if (po_args.filter_options.choice && !drop_filtered) {
+		struct write_pack_opts opts = {
+			.po_args = &po_args,
+			.destination = filter_to,
+			.packdir = packdir,
+			.packtmp = packtmp,
+		};
+
+		if (!opts.destination)
+			opts.destination = packtmp;
+
+		ret = write_filtered_pack(&opts, &existing, &names);
+		if (ret)
+			goto cleanup;
  	}
  
  	string_list_sort(&names);

@@ -697,6 +705,7 @@ int cmd_repack(int argc,
  cleanup:
  	string_list_clear(&keep_pack_list, 0);
  	string_list_clear(&names, 1);
+	oidset_clear(&drop_oids);
  	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 f5a1dae5b1..6f0cecca9b 100644
--- a/repack-filtered.c
+++ b/repack-filtered.c

@@ -87,16 +87,13 @@ static int collect_promisor_blob(const struct object_id *oid,
  
  int enumerate_promisor_blobs(struct repository *repo,
  			const struct list_objects_filter_options *filter,
-			int dry_run)
+			struct oidset *to_drop)
  {
  	struct oidset all_promisor_blobs = OIDSET_INIT;
-	struct oidset to_drop = OIDSET_INIT;
  	struct collect_cb_data cb = {
  		.repo = repo,
  		.set = &all_promisor_blobs
  	};
-	struct oidset_iter iter;
-	const struct object_id *oid;
  	int ret = 0;
  
  	/*

@@ -122,22 +119,14 @@ int enumerate_promisor_blobs(struct repository *repo,
  
  	/*
  	 * Apply the filter to find which blobs exceed the threshold.
+	 * The caller has to_drop and is responsible for clearing it.
  	 */
  	ret = list_objects_filter__filter_oidset(repo,
  		(struct list_objects_filter_options *)filter,
  		&all_promisor_blobs,
-		&to_drop);
-	if (ret)
-		goto cleanup;
-
-	if (dry_run) {
-		oidset_iter_init(&to_drop, &iter);
-		while ((oid = oidset_iter_next(&iter)))
-			printf("%s\n", oid_to_hex(oid));
-	}
+		to_drop);
  
  cleanup:
  	oidset_clear(&all_promisor_blobs);
-	oidset_clear(&to_drop);
  	return ret;
  }

diff --git a/repack.h b/repack.h
index d08e25b852..61e554e4ed 100644
--- a/repack.h
+++ b/repack.h

@@ -168,8 +168,8 @@ int write_filtered_pack(const struct write_pack_opts *opts,
  			struct string_list *names);
  
  int enumerate_promisor_blobs(struct repository *repo,
-			       const struct list_objects_filter_options *filter,
-			       int dry_run);
+			     const struct list_objects_filter_options *filter,
+			     struct oidset *to_drop);
  
  int write_cruft_pack(const struct write_pack_opts *opts,
  		     const char *cruft_expiration,

diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh
index b558807847..41e7941799 100755
--- a/t/t7706-repack-drop-filtered.sh
+++ b/t/t7706-repack-drop-filtered.sh

@@ -56,12 +56,6 @@ test_expect_success '--dry-run only takes effect with --drop-filtered' '
  	test_grep "dry-run only takes effect with --drop-filtered" err
  '
  
-test_expect_success '--drop-filtered without --dry-run is rejected' '
-	test_must_fail git -C plain.git repack --drop-filtered \
-		--filter=blob:limit=1k -a 2>err &&
-	test_grep "drop-filtered doesn.t work without --dry-run yet" err
-'
-
  test_expect_success '--drop-filtered requires -a' '
  	test_must_fail git -C plain.git repack --drop-filtered \
  		--filter=blob:limit=1k --dry-run 2>err &&

@@ -136,4 +130,16 @@ test_expect_success '--dry-run does not remove the filtered objects' '
  	git -C repo cat-file -e "$BIG"
  '
  
+test_expect_success '--drop-filtered removes the promisor blob locally' '
+	BIG=$(cat big_oid) &&
+	SMALL=$(cat small_oid) &&
+
+	git -C repo -c repack.writeBitmaps=false \
+		repack --drop-filtered --filter=blob:limit=1k -a &&
+
+	git -C repo cat-file --batch-all-objects --batch-check="%(objectname)" >present &&
+	! grep -q "$BIG" present &&
+	grep -q "$SMALL" present
+'
+
  test_done