[RFC PATCH 0/7] repack: add --drop-filtered to reclaim space in partial clones

[RFC PATCH 0/7] repack: add --drop-filtered to reclaim space in partial clones

[Siddharth Shrimali]

This is an RFC series seeking feedback on the design and approach.
Several pieces are still missing (noted below) and the commit
organization needs cleanup.

Partial clones let you work with large repositories without downloading
every blob up front and the missing blobs are lazily fetched from the promisor
remote on demand. Over time, though, these lazily-fetched blobs
accumulate locally and there is currently no safe, built-in way to
reclaim that disk space instead of re-cloning.

This series adds a "git repack --drop-filtered --filter=<spec>" command
that removes large, locally-held promisor blobs that are recoverable
from the promisor remote. The dropped blobs become absent locally but
remain lazily re-fetchable, making the partial-clone still reversible.

How it works:
  * Enumerate promisor objects directly (ODB_FOR_EACH_OBJECT_PROMISOR_ONLY)
    and select the blobs exceeding the filter threshold. Because every
    enumerated object is a promisor object, it is guaranteed recoverable and
    locally-created objects are never candidates.

  * Rebuild the promisor pack without the selected blobs, reusing the
    existing repack machinery, so the drop is crash-safe.

  * Record each dropped object in a drop log
    ($GIT_DIR/objects/info/promisor-dropped) so a later change can
    explain a failed lazy fetch (when it was dropped, which filter
    matched, which remotes) instead of a bare "could not fetch" error.

  * --dry-run lists the candidates and changes nothing.

Planned follow-ups:
  * Safety guards: refuse to run while a merge/rebase/cherry-pick is in
    progress, and refuse to drop blobs referenced by the current index.

  * Authoritative remote verification: the drop log currently lists all
    configured promisor remotes rather than the exact remote each object
    is recoverable from, because there is no client-side way to query a
    remote for object availability yet. A "remote-object-info" command
    is being added to the "git cat-file --batch" protocol for this. Once
    available, the exact remote can be recorded.

Known issues to address in v2:
  * There is churn between "enumerate promisor blobs" and "actually drop
    filtered promisor blobs". The former introduces
    enumerate_promisor_blobs() with an interim signature that the latter
    rewrites. These will be reorganized so the function is introduced
    in its final form.

  * The tests are in a standalone commit. They will instead be
    distributed into the commits that introduce the behavior they test.

Siddharth Shrimali (7):
  builtin/repack.c: add --drop-filtered and --dry-run options
  list-objects-filter: add list_objects_filter__filter_oidset()
  repack-promisor: allow excluding objects from the rebuilt promisor
    pack
  builtin/repack: enumerate promisor blobs for --drop-filtered
  t7706: test --drop-filtered enumeration and validation
  builtin/repack: actually drop filtered promisor blobs
  repack-promisor: record dropped objects in a drop log

 builtin/repack.c                |  76 ++++++++++++++++-
 list-objects-filter.c           |  45 ++++++++++
 list-objects-filter.h           |  16 ++++
 repack-filtered.c               |  81 ++++++++++++++++++
 repack-promisor.c               | 106 ++++++++++++++++++++++-
 repack.h                        |  12 ++-
 t/meson.build                   |   1 +
 t/t7706-repack-drop-filtered.sh | 145 ++++++++++++++++++++++++++++++++
 8 files changed, 478 insertions(+), 4 deletions(-)
 create mode 100755 t/t7706-repack-drop-filtered.sh

-- 
2.54.0

[RFC PATCH 1/7] builtin/repack.c: add --drop-filtered and --dry-run options

[Siddharth Shrimali]

Add two new command-line options to 'git-repack':

  --drop-filtered: intended to eventually delete objects that match
                   the filter specification. Requires --filter and -a,
                   and is incompatible with --filter-to.
  --dry-run: show which objects would be dropped without making any
             changes. Only meaningful with --drop-filtered.

--drop-filtered also requires a promisor remote to be configured,
since dropping objects without a remote to fetch them back from would
be permanent data loss.

--drop-filtered is incompatible with bitmap writing: filtering breaks
the "all objects in one pack" closure that bitmaps require. An explicit
-b is rejected with a clear error and a default-on bitmap configuration is
silently disabled for the duration of the command.

These options currently only perform validation. The actual enumeration
and deletion will be added in follow-up commits.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 builtin/repack.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/builtin/repack.c b/builtin/repack.c
index db504d673f..f4db0fc535 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -14,6 +14,7 @@
 #include "promisor-remote.h"
 #include "repack.h"
 #include "shallow.h"
+#include "list-objects-filter-options.h"
 
 #define ALL_INTO_ONE 1
 #define LOOSEN_UNREACHABLE 2
@@ -28,6 +29,8 @@ static int use_delta_islands;
 static int run_update_server_info = 1;
 static char *packdir, *packtmp_name, *packtmp;
 static int midx_must_contain_cruft = 1;
+static int drop_filtered;
+static int dry_run;
 
 static const char *const git_repack_usage[] = {
 	N_("git repack [-a] [-A] [-d] [-f] [-F] [-l] [-n] [-q] [-b] [-m]\n"
@@ -231,6 +234,10 @@ int cmd_repack(int argc,
 			   N_("pack prefix to store a pack containing pruned objects")),
 		OPT_STRING(0, "filter-to", &filter_to, N_("dir"),
 			   N_("pack prefix to store a pack containing filtered out objects")),
+		OPT_BOOL(0, "drop-filtered", &drop_filtered,
+				N_("delete filtered out objects (requires --filter)")),
+		OPT_BOOL(0, "dry-run", &dry_run,
+				N_("only show which objects would be dropped")),
 		OPT_END()
 	};
 
@@ -252,6 +259,43 @@ int cmd_repack(int argc,
 	po_args.depth = xstrdup_or_null(opt_depth);
 	po_args.threads = xstrdup_or_null(opt_threads);
 
+	die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
+		!!filter_to, "--filter-to");
+
+	die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
+		write_bitmaps > 0, "--write-bitmap-index");
+
+	if (dry_run && !drop_filtered)
+		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"));
+
+		if (!(pack_everything & ALL_INTO_ONE))
+			die(_("--drop-filtered requires -a"));
+
+		/*
+		 * Only blob:limit=<n> is supported for now. Reject other
+		 * filter choices early, before walking the object database.
+		 */
+		if (po_args.filter_options.choice != LOFC_BLOB_LIMIT)
+			die(_("--drop-filtered only supports --filter=blob:limit=<n> for now"));
+
+		/*
+		 * Without a promisor remote there is nowhere to re-fetch the
+		 * dropped objects from, so dropping them would be permanent
+		 * data loss.
+		 */
+		if (!repo_has_promisor_remote(repo))
+			die(_("--drop-filtered requires a promisor remote"));
+
+		write_bitmaps = 0;
+	}
+
 	if (delete_redundant && repo->repository_format_precious_objects)
 		die(_("cannot delete packs in a precious-objects repo"));
 
-- 
2.54.0

[RFC PATCH 2/7] list-objects-filter: add list_objects_filter__filter_oidset()

[Siddharth Shrimali]

The existing filter entry point, list_objects_filter__filter_object(),
is built around the object-walk path: it expects traversal context and
provisional omit sets, and is meant to be called as objects are
visited during a walk. A caller that already has a set of OIDs in hand
and only wants to know which ones a filter would select has no usable
entry point into the filter API.

--drop-filtered is exactly such a caller: it collects promisor blobs
into an oidset and needs to know which of them exceed the filter
threshold, without performing an object walk.

Add a helper, list_objects_filter__filter_oidset(), that takes a set
of OIDs and populates an "omitted" set with those that would be
filtered out by the given filter options. Only blob:limit=N filters
are supported for now.

This helper does not actually reuse the existing filter machinery.
It reimplements the blob:limit size check directly. That machinery
is tied to the object-walk path and cannot easily be driven
from a plain oidset. A NEEDSWORK comment marks this so the helper can
later be refactored to reuse the real filter logic instead of
duplicating it.

OBJECT_INFO_SKIP_FETCH_OBJECT is passed when reading object info so
the helper never triggers a lazy fetch.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 list-objects-filter.c | 45 +++++++++++++++++++++++++++++++++++++++++++
 list-objects-filter.h | 16 +++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/list-objects-filter.c b/list-objects-filter.c
index c912ff3079..6a2e9d5b24 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -828,3 +828,48 @@ void list_objects_filter__free(struct filter *filter)
 	filter->free_fn(filter->filter_data);
 	free(filter);
 }
+
+/*
+ * NEEDSWORK: this reimplements the blob:limit size check rather than
+ * reusing the existing filter machinery in
+ * list_objects_filter__filter_object(). That machinery is currently
+ * tied to the object-walk path and cannot easily be driven from a
+ * plain oidset. It would be nice to refactor the filter code so this
+ * helper can reuse it instead of duplicating the size check.
+ */
+int list_objects_filter__filter_oidset(struct repository *r,
+	struct list_objects_filter_options *opts,
+	const struct oidset *in,
+	struct oidset *omitted)
+{
+	struct oidset_iter iter;
+	const struct object_id *oid;
+
+	if (opts->choice != LOFC_BLOB_LIMIT)
+		return error(_("filter_oidset: only blob:limit filters are supported"));
+
+	oidset_iter_init(in, &iter);
+	while ((oid = oidset_iter_next(&iter))) {
+		struct object_info info = OBJECT_INFO_INIT;
+		enum object_type type;
+		unsigned long size;
+
+		info.typep = &type;
+		info.sizep = &size;
+
+		/*
+		 * Use OBJECT_INFO_SKIP_FETCH_OBJECT to avoid triggering
+		 * a lazy fetch while inspecting candidates for removal.
+		 */
+		if (odb_read_object_info_extended(r->objects, oid, &info,
+				OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)
+			continue;
+
+		if (type != OBJ_BLOB)
+			continue;
+
+		if (size >= opts->blob_limit_value)
+			oidset_insert(omitted, oid);
+	}
+	return 0;
+}
diff --git a/list-objects-filter.h b/list-objects-filter.h
index 9e98814111..56a2d87aa0 100644
--- a/list-objects-filter.h
+++ b/list-objects-filter.h
@@ -94,4 +94,20 @@ enum list_objects_filter_result list_objects_filter__filter_object(
  */
 void list_objects_filter__free(struct filter *filter);
 
+/*
+ * Given a set of OIDs in 'in', populate 'omitted' with those that
+ * would be filtered by 'opts'. Currently only blob:limit=N is
+ * supported. Objects that cannot be read are silently skipped.
+ *
+ * NEEDSWORK: this reimplements the blob:limit size check rather than
+ * reusing the existing filter machinery. See the matching comment in
+ * list-objects-filter.c.
+ *
+ * Return 0 on success, -1 if the filter is not supported.
+ */
+int list_objects_filter__filter_oidset(struct repository *r,
+	struct list_objects_filter_options *opts,
+	const struct oidset *in,
+	struct oidset *omitted);
+
 #endif /* LIST_OBJECTS_FILTER_H */
-- 
2.54.0

[RFC PATCH 3/7] repack-promisor: allow excluding objects from the rebuilt promisor pack

[Siddharth Shrimali]

Add a to_drop oidset parameter to repack_promisor_objects(). When it is
non-NULL, write_oid() omits those objects from the rebuilt promisor
pack. This is the mechanism --drop-filtered will use to remove promisor
blobs, i.e. rebuild the promisor pack without them.

All existing callers pass NULL, so behavior is unchanged.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 builtin/repack.c  |  2 +-
 repack-promisor.c | 15 ++++++++++++++-
 repack.h          |  4 +++-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index f4db0fc535..433b2c8205 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -406,7 +406,7 @@ 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);
+		repack_promisor_objects(repo, &po_args, &names, packtmp, NULL);
 
 		if (existing_packs_has_non_kept(&existing) &&
 		    delete_redundant &&
diff --git a/repack-promisor.c b/repack-promisor.c
index 90318ce150..fabfdc168a 100644
--- a/repack-promisor.c
+++ b/repack-promisor.c
@@ -6,10 +6,12 @@
 #include "path.h"
 #include "repository.h"
 #include "run-command.h"
+#include "oidset.h"
 
 struct write_oid_context {
 	struct child_process *cmd;
 	const struct git_hash_algo *algop;
+	const struct oidset *to_drop;
 };
 
 /*
@@ -23,6 +25,15 @@ static int write_oid(const struct object_id *oid,
 	struct write_oid_context *ctx = data;
 	struct child_process *cmd = ctx->cmd;
 
+	/*
+	 * Objects in to_drop are being removed from the repository, so
+	 * omit them from the rebuilt promisor pack. Each such object is a
+	 * promisor object and therefore remains recoverable from the
+	 * promisor remote.
+	 */
+	if (ctx->to_drop && oidset_contains(ctx->to_drop, oid))
+		return 0;
+
 	if (cmd->in == -1) {
 		if (start_command(cmd))
 			die(_("could not start pack-objects to repack promisor objects"));
@@ -81,7 +92,8 @@ static void finish_repacking_promisor_objects(struct repository *repo,
 
 void repack_promisor_objects(struct repository *repo,
 			     const struct pack_objects_args *args,
-			     struct string_list *names, const char *packtmp)
+			     struct string_list *names, const char *packtmp,
+			     const struct oidset *to_drop)
 {
 	struct write_oid_context ctx;
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -98,6 +110,7 @@ void repack_promisor_objects(struct repository *repo,
 	 */
 	ctx.cmd = &cmd;
 	ctx.algop = repo->hash_algo;
+	ctx.to_drop = to_drop;
 	odb_for_each_object(repo->objects, NULL, write_oid, &ctx,
 			    ODB_FOR_EACH_OBJECT_PROMISOR_ONLY);
 
diff --git a/repack.h b/repack.h
index f9fbc895f0..a5a3f7c6ba 100644
--- a/repack.h
+++ b/repack.h
@@ -3,6 +3,7 @@
 
 #include "list-objects-filter-options.h"
 #include "string-list.h"
+#include "oidset.h"
 
 struct pack_objects_args {
 	char *window;
@@ -100,7 +101,8 @@ void generated_pack_install(struct generated_pack *pack, const char *name,
 
 void repack_promisor_objects(struct repository *repo,
 			     const struct pack_objects_args *args,
-			     struct string_list *names, const char *packtmp);
+			     struct string_list *names, const char *packtmp,
+			     const struct oidset *to_drop);
 
 struct pack_geometry {
 	struct packed_git **pack;
-- 
2.54.0

[RFC PATCH 4/7] builtin/repack: enumerate promisor blobs for --drop-filtered

[Siddharth Shrimali]

Add enumeration logic for --drop-filtered. In --dry-run mode, print
the OIDs of locally-held promisor blobs that exceed the filter
threshold, as candidates for removal.

Reading from write_filtered_pack() cannot work for partial clones.
git repack routes promisor objects through a separate path:
repack_promisor_objects() repacks them first, and the main
pack-objects run uses --exclude-promisor-objects. By the time
write_filtered_pack() runs, the promisor blobs are already consumed by
the main pack. The filtered pack is always empty on a partial clone.

Instead, walk promisor objects directly via odb_for_each_object() with
ODB_FOR_EACH_OBJECT_PROMISOR_ONLY, collecting all promisor blobs into
an oidset. The blobs exceeding the filter threshold are then selected
using list_objects_filter__filter_oidset().

Every object enumerated this way is a promisor object by construction,
so it is guaranteed to be recoverable from the promisor remote and is
safe to drop. No separate is_promisor_object() check is needed.

OBJECT_INFO_SKIP_FETCH_OBJECT is passed to every object info query so
enumeration never triggers a lazy fetch.

Deletion of the enumerated objects, together with the required
promisor-remote verification, will be added separately.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 builtin/repack.c  | 41 ++++++++++++++-------
 repack-filtered.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++
 repack.h          |  4 +++
 3 files changed, 124 insertions(+), 13 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 433b2c8205..c2b07477d2 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -590,19 +590,34 @@ int cmd_repack(int argc,
 	}
 
 	if (po_args.filter_options.choice) {
-		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 (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;
+		}
 	}
 
 	string_list_sort(&names);
diff --git a/repack-filtered.c b/repack-filtered.c
index edcf7667c5..f5a1dae5b1 100644
--- a/repack-filtered.c
+++ b/repack-filtered.c
@@ -3,6 +3,12 @@
 #include "repository.h"
 #include "run-command.h"
 #include "string-list.h"
+#include "hex.h"
+#include "packfile.h"
+#include "list-objects-filter-options.h"
+#include "list-objects-filter.h"
+#include "odb.h"
+#include "promisor-remote.h"
 
 int write_filtered_pack(const struct write_pack_opts *opts,
 			struct existing_packs *existing,
@@ -49,3 +55,89 @@ int write_filtered_pack(const struct write_pack_opts *opts,
 	return finish_pack_objects_cmd(existing->repo->hash_algo, opts, &cmd,
 				       names);
 }
+
+struct collect_cb_data {
+	struct repository *repo;
+	struct oidset *set;
+};
+
+static int collect_promisor_blob(const struct object_id *oid,
+		struct object_info *oi UNUSED,
+		void *cb_data)
+{
+	struct collect_cb_data *data = cb_data;
+	struct object_info info = OBJECT_INFO_INIT;
+	enum object_type type;
+
+	info.typep = &type;
+
+	/*
+	 * Use OBJECT_INFO_SKIP_FETCH_OBJECT to avoid triggering a
+	 * lazy fetch while collecting promisor blobs.
+	 */
+	if (odb_read_object_info_extended(data->repo->objects, oid, &info,
+			OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)
+		return 0;
+
+	if (type == OBJ_BLOB)
+		oidset_insert(data->set, oid);
+
+	return 0;
+}
+
+int enumerate_promisor_blobs(struct repository *repo,
+			const struct list_objects_filter_options *filter,
+			int dry_run)
+{
+	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;
+
+	/*
+	 * The caller (cmd_repack) is responsible for validating that a
+	 * blob:limit filter and a promisor remote are present before
+	 * calling this function.
+	 *
+	 * Walk only promisor objects. Every object visited here is
+	 * guaranteed to be recoverable from the promisor remote, so
+	 * it is safe to drop.
+	 *
+	 * We do not use write_filtered_pack() here because git repack
+	 * routes promisor objects through repack_promisor_objects()
+	 * before the filter machinery runs, so the filtered pack never
+	 * contains promisor blobs. Direct enumeration via
+	 * ODB_FOR_EACH_OBJECT_PROMISOR_ONLY is the correct approach.
+	 */
+	ret = odb_for_each_object(repo->objects, NULL,
+			collect_promisor_blob, &cb,
+			ODB_FOR_EACH_OBJECT_PROMISOR_ONLY);
+	if (ret)
+		goto cleanup;
+
+	/*
+	 * Apply the filter to find which blobs exceed the threshold.
+	 */
+	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));
+	}
+
+cleanup:
+	oidset_clear(&all_promisor_blobs);
+	oidset_clear(&to_drop);
+	return ret;
+}
diff --git a/repack.h b/repack.h
index a5a3f7c6ba..d08e25b852 100644
--- a/repack.h
+++ b/repack.h
@@ -167,6 +167,10 @@ int write_filtered_pack(const struct write_pack_opts *opts,
 			struct existing_packs *existing,
 			struct string_list *names);
 
+int enumerate_promisor_blobs(struct repository *repo,
+			       const struct list_objects_filter_options *filter,
+			       int dry_run);
+
 int write_cruft_pack(const struct write_pack_opts *opts,
 		     const char *cruft_expiration,
 		     unsigned long combine_cruft_below_size,
-- 
2.54.0

[RFC PATCH 5/7] t7706: test --drop-filtered enumeration and validation

[Siddharth Shrimali]

Add tests for the --drop-filtered option.

  * Validation: --drop-filtered requires --filter and -a, is
    incompatible with --filter-to and --write-bitmap-index, --dry-run
    only takes effect with --drop-filtered, and --drop-filtered
    requires a promisor remote.

  * Enumeration: in a repository with a promisor remote, --dry-run
    lists promisor blobs above the filter threshold and excludes
    smaller ones. Promisor blobs are created with a synthetic promisor
    pack, following the helper pattern used in t0410.

  * Safety: a locally created large blob, which is not a promisor
    object and therefore not recoverable from the remote, is never
    listed as a drop candidate.

  * Non-destructiveness: --dry-run leaves the filtered objects intact
    in the repository.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 t/meson.build                   |   1 +
 t/t7706-repack-drop-filtered.sh | 139 ++++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+)
 create mode 100755 t/t7706-repack-drop-filtered.sh

diff --git a/t/meson.build b/t/meson.build
index 8ae6ab6c5f..37f272d7f4 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -962,6 +962,7 @@ integration_tests = [
   't7703-repack-geometric.sh',
   't7704-repack-cruft.sh',
   't7705-repack-incremental-midx.sh',
+  't7706-repack-drop-filtered.sh',
   't7800-difftool.sh',
   't7810-grep.sh',
   't7811-grep-open.sh',
diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh
new file mode 100755
index 0000000000..b558807847
--- /dev/null
+++ b/t/t7706-repack-drop-filtered.sh
@@ -0,0 +1,139 @@
+#!/bin/sh
+
+test_description='git repack --drop-filtered enumerates
+filtered promisor blobs'
+
+. ./test-lib.sh
+
+delete_object () {
+	local repo="$1" &&
+	local obj="$2" &&
+	local path="$repo/.git/objects/$(test_oid_to_path "$obj")" &&
+	rm "$path"
+}
+
+# pack the objects into a promisor pack inside "repo",
+# it is a pack accompanied by an empty ".promisor" marker file. objects
+# in such a pack are treated as recoverable from the promisor remote.
+pack_as_from_promisor () {
+	HASH=$(git -C repo pack-objects .git/objects/pack/pack) &&
+	>repo/.git/objects/pack/pack-$HASH.promisor &&
+	echo $HASH
+}
+
+# write a blob of $1 bytes into "repo", record it as coming from the
+# promisor remote (promisor pack), and remove the loose copy so the
+# object is only present in the promisor pack.
+promisor_blob () {
+	test-tool genrandom "$1" "$2" >blob_content &&
+	OID=$(git -C repo hash-object -w --stdin <blob_content) &&
+	printf "%s\n" "$OID" | pack_as_from_promisor >/dev/null &&
+	delete_object repo "$OID" &&
+	echo "$OID"
+}
+
+# checks for options validations before any promisor walk
+test_expect_success 'setup plain repo for validation' '
+	git init plain &&
+	test_commit -C plain initial &&
+	git clone --bare plain plain.git &&
+	git -C plain.git repack -a -d
+'
+
+test_expect_success '--drop-filtered requires --filter' '
+	test_must_fail git -C plain.git repack --drop-filtered --dry-run -a 2>err &&
+	test_grep "drop-filtered requires --filter" err
+'
+
+test_expect_success '--drop-filtered cannot be used with --filter-to' '
+	test_must_fail git -C plain.git repack --drop-filtered \
+		--filter=blob:limit=1k --filter-to=./filter-out 2>err &&
+	test_grep "options .--drop-filtered. and .--filter-to. cannot be used together" err
+'
+
+test_expect_success '--dry-run only takes effect with --drop-filtered' '
+	test_must_fail git -C plain.git repack --dry-run 2>err &&
+	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 &&
+	test_grep "drop-filtered requires -a" err
+'
+
+test_expect_success '--drop-filtered fails with --write-bitmap-index' '
+	test_must_fail git -C plain.git repack --drop-filtered \
+		--filter=blob:limit=1k --dry-run -a -b 2>err &&
+	test_grep "options .--drop-filtered. and .--write-bitmap-index. cannot be used together" err
+'
+
+test_expect_success '--drop-filtered fails without a promisor remote' '
+	test_must_fail git -C plain.git repack --drop-filtered \
+		--filter=blob:limit=1k --dry-run -a 2>err &&
+	test_grep "drop-filtered requires a promisor remote" err
+'
+
+# enumeration and safety tests using promisor packs
+test_expect_success 'setup repo with a promisor remote' '
+	rm -rf repo &&
+	test_create_repo repo &&
+	test_commit -C repo base &&
+
+	# mark the repo as a partial clone with a promisor remote so the
+	# promisor walk and the safety guard are satisfied.
+	git -C repo config core.repositoryformatversion 1 &&
+	git -C repo config extensions.partialclone origin &&
+	git -C repo config remote.origin.promisor true &&
+	git -C repo config remote.origin.url "." &&
+
+	BIG=$(promisor_blob big 3072) &&
+	SMALL=$(promisor_blob small 512) &&
+	echo "$BIG" >big_oid &&
+	echo "$SMALL" >small_oid
+'
+
+test_expect_success 'promisor blob over the threshold is listed' '
+	BIG=$(cat big_oid) &&
+	SMALL=$(cat small_oid) &&
+
+	git -C repo -c repack.writeBitmaps=false \
+		repack --drop-filtered --filter=blob:limit=1k --dry-run -a >out &&
+
+	test_grep "$BIG" out &&
+	test_grep ! "$SMALL" out
+'
+
+test_expect_success 'locally created blob is never listed' '
+	BIG=$(cat big_oid) &&
+
+	# large blob that exists only locally (no promisor pack) must
+	# never be a drop candidate: dropping it would be unrecoverable
+	# data loss.
+	test-tool genrandom local 4096 >local_content &&
+	LOCAL=$(git -C repo hash-object -w --stdin <local_content) &&
+
+	git -C repo -c repack.writeBitmaps=false \
+		repack --drop-filtered --filter=blob:limit=1k --dry-run -a >out &&
+
+	test_grep "$BIG" out &&
+	test_grep ! "$LOCAL" out
+'
+
+test_expect_success '--dry-run does not remove the filtered objects' '
+	BIG=$(cat big_oid) &&
+
+	git -C repo -c repack.writeBitmaps=false \
+		repack --drop-filtered --filter=blob:limit=1k --dry-run -a >out &&
+
+	# candidate blob must still be present after a dry run.
+	git -C repo cat-file -e "$BIG"
+'
+
+test_done
-- 
2.54.0

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

[Siddharth Shrimali]

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 <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 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
-- 
2.54.0

[RFC PATCH 7/7] repack-promisor: record dropped objects in a drop log

[Siddharth Shrimali]

After --drop-filtered removes promisor blobs, append a record of each
dropped object to $GIT_DIR/objects/info/promisor-dropped. Each line
records the object ID, a reflog-style timestamp (Unix seconds and
timezone), the filter spec, and the promisor remote it was attested
recoverable from like the following:

  <oid> <time> <tz> filter=<spec> remote=<name>

If a dropped object later becomes unrecoverable (for example, the
branch holding it is deleted on the promisor remote), a lazy fetch
fails with a generic error. This persistent record lets a later change
explain that the object was dropped deliberately, when, under which
filter, and from which remote it was expected to be recoverable.

The remote field lists all configured promisor remotes rather than the
specific one each dropped object is recoverable from. Determining the
exact remote would require asking the remote whether it has the object.
A "remote-object-info" command is being added to the "git cat-file
--batch" protocol for this kind of query, but it is not available yet.
A NEEDSWORK marks this for a follow-up.

The log is written only on a real run, i.e. --dry-run changes nothing.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 builtin/repack.c  |  4 +++
 repack-promisor.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++
 repack.h          |  4 +++
 3 files changed, 99 insertions(+)

diff --git a/builtin/repack.c b/builtin/repack.c
index aa3257a98a..49dcbbc567 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -702,6 +702,10 @@ int cmd_repack(int argc,
 		write_midx_file(files->packed, NULL, NULL, flags);
 	}
 
+	if (drop_filtered && !dry_run)
+		append_drop_log(repo, &drop_oids,
+			expand_list_objects_filter_spec(&po_args.filter_options));
+
 cleanup:
 	string_list_clear(&keep_pack_list, 0);
 	string_list_clear(&names, 1);
diff --git a/repack-promisor.c b/repack-promisor.c
index fabfdc168a..60913a5150 100644
--- a/repack-promisor.c
+++ b/repack-promisor.c
@@ -7,6 +7,97 @@
 #include "repository.h"
 #include "run-command.h"
 #include "oidset.h"
+#include "date.h"
+#include "promisor-remote.h"
+#include "strbuf.h"
+
+/*
+ * Append the drop-log entries to the already-computed path.
+ * Returns -1 on any I/O failure so the caller can warn once.
+ * Keeping this in a separate helper avoids goto-based cleanup
+ * in append_drop_log();
+ */
+static int write_to_drop_log(struct repository *repo,
+			     const char *path,
+			     const struct oidset *dropped,
+			     const char *stamp,
+			     const char *filter_spec,
+			     const char *remotes)
+{
+	struct oidset_iter iter;
+	const struct object_id *oid;
+	FILE *fp;
+
+	if (safe_create_leading_directories(repo, (char *)path)) {
+		warning(_("could not create leading directories for '%s'"), path);
+		return -1;
+	}
+
+	fp = fopen(path, "a");
+	if (!fp) {
+		warning_errno(_("could not open '%s'"), path);
+		return -1;
+	}
+
+	oidset_iter_init(dropped, &iter);
+	while ((oid = oidset_iter_next(&iter))) {
+		if (fprintf(fp, "%s %s filter=%s remote=%s\n",
+				oid_to_hex(oid), stamp,
+				filter_spec ? filter_spec : "",
+				remotes) < 0) {
+			warning(_("could not write to '%s'"), path);
+			fclose(fp);
+			return -1;
+		}
+	}
+
+	if (fclose(fp)) {
+		warning_errno(_("could not close '%s'"), path);
+		return -1;
+	}
+
+	return 0;
+}
+
+void append_drop_log(struct repository *repo,
+		     const struct oidset *dropped,
+		     const char *filter_spec)
+{
+	char *path;
+	struct strbuf stamp = STRBUF_INIT;
+	struct strbuf remotes = STRBUF_INIT;
+	struct promisor_remote *pr;
+
+	if (!oidset_size(dropped))
+		return;
+
+	datestamp(&stamp);
+
+	/*
+	 * NEEDSWORK: we temporarily record all configured promisor remotes rather
+	 * than the specific one a given object is recoverable from because there
+	 * is currently no way to determine that locally. it would require
+	 * asking the remote whether it has the object. A "remote-object-info"
+	 * command is being added to the "git cat-file --batch" protocol for
+	 * this kind of query. Once it is merged in the codebase, this should
+	 * record the exact promisor remote that has each dropped object.
+	 */
+	for (pr = repo_promisor_remote_find(repo, NULL); pr; pr = pr->next) {
+		if (remotes.len)
+			strbuf_addch(&remotes, ',');
+		strbuf_addstr(&remotes, pr->name);
+	}
+
+	path = repo_git_path(repo, "objects/info/promisor-dropped");
+
+	if (write_to_drop_log(repo, path, dropped, stamp.buf,
+			filter_spec, remotes.buf))
+		warning(_("could not record all dropped objects in the drop log"));
+
+	strbuf_release(&stamp);
+	strbuf_release(&remotes);
+	free(path);
+}
 
 struct write_oid_context {
 	struct child_process *cmd;
diff --git a/repack.h b/repack.h
index 61e554e4ed..33309548ce 100644
--- a/repack.h
+++ b/repack.h
@@ -171,6 +171,10 @@ int enumerate_promisor_blobs(struct repository *repo,
 			     const struct list_objects_filter_options *filter,
 			     struct oidset *to_drop);
 
+void append_drop_log(struct repository *repo,
+		     const struct oidset *dropped,
+		     const char *filter_spec);
+
 int write_cruft_pack(const struct write_pack_opts *opts,
 		     const char *cruft_expiration,
 		     unsigned long combine_cruft_below_size,
-- 
2.54.0

Re: [RFC PATCH 1/7] builtin/repack.c: add --drop-filtered and --dry-run options

[Junio C Hamano]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

> --drop-filtered is incompatible with bitmap writing: filtering breaks
> the "all objects in one pack" closure that bitmaps require. An explicit
> -b is rejected with a clear error and a default-on bitmap configuration is
> silently disabled for the duration of the command.

That is very well intentioned.

> @@ -231,6 +234,10 @@ int cmd_repack(int argc,
>  			   N_("pack prefix to store a pack containing pruned objects")),
>  		OPT_STRING(0, "filter-to", &filter_to, N_("dir"),
>  			   N_("pack prefix to store a pack containing filtered out objects")),
> +		OPT_BOOL(0, "drop-filtered", &drop_filtered,
> +				N_("delete filtered out objects (requires --filter)")),
> +		OPT_BOOL(0, "dry-run", &dry_run,
> +				N_("only show which objects would be dropped")),
>  		OPT_END()
>  	};
>  
> @@ -252,6 +259,43 @@ int cmd_repack(int argc,
>  	po_args.depth = xstrdup_or_null(opt_depth);
>  	po_args.threads = xstrdup_or_null(opt_threads);
>  
> +	die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
> +		!!filter_to, "--filter-to");
> +
> +	die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
> +		write_bitmaps > 0, "--write-bitmap-index");

Hmph.  Since this step does not change the parsing or configuration
for write_bitmaps, we cannot tell if (write_bitmaps == 1) at this
point in the execution came from the command line (e.g., an earlier
call to parse_options() around line 247 of builtin/repack.c) or from
the configuration files (e.g., a call to repo_config() around
line 245).  In other words, wouldn't it be ...

> +	if (dry_run && !drop_filtered)
> +		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"));
> +
> +		if (!(pack_everything & ALL_INTO_ONE))
> +			die(_("--drop-filtered requires -a"));
> +
> +		/*
> +		 * Only blob:limit=<n> is supported for now. Reject other
> +		 * filter choices early, before walking the object database.
> +		 */
> +		if (po_args.filter_options.choice != LOFC_BLOB_LIMIT)
> +			die(_("--drop-filtered only supports --filter=blob:limit=<n> for now"));
> +
> +		/*
> +		 * Without a promisor remote there is nowhere to re-fetch the
> +		 * dropped objects from, so dropping them would be permanent
> +		 * data loss.
> +		 */
> +		if (!repo_has_promisor_remote(repo))
> +			die(_("--drop-filtered requires a promisor remote"));
> +
> +		write_bitmaps = 0;

... way too late to drop the flag here?

> +	}
> +
>  	if (delete_redundant && repo->repository_format_precious_objects)
>  		die(_("cannot delete packs in a precious-objects repo"));

Re: [RFC PATCH 1/7] builtin/repack.c: add --drop-filtered and --dry-run options

[Siddharth Shrimali]

On Fri, 17 Jul 2026 at 02:38, Junio C Hamano <gitster@pobox.com> wrote:
>
> > +     die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
> > +             !!filter_to, "--filter-to");
> > +
> > +     die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
> > +             write_bitmaps > 0, "--write-bitmap-index");
>
> Hmph.  Since this step does not change the parsing or configuration
> for write_bitmaps, we cannot tell if (write_bitmaps == 1) at this
> point in the execution came from the command line (e.g., an earlier
> call to parse_options() around line 247 of builtin/repack.c) or from
> the configuration files (e.g., a call to repo_config() around
> line 245).  In other words, wouldn't it be ...
>
> > +             write_bitmaps = 0;
>
> ... way too late to drop the flag here?
>

right, thanks! The commit message claims I distinguish an explicit
-b/--write-bitmap-index from a config-provided default, but the code
only tests write_bitmaps > 0, which cannot tell the two apart at this point

For v2, alongside other changes, i'll distinguish the two, so that an explicit
-b on the command line errors out, while a config-provided default is
silently disabled for the duration of the command..

Thanks!
Siddharth Shrimali

Re: [RFC PATCH 1/7] builtin/repack.c: add --drop-filtered and --dry-run options

[Christian Couder]

On Thu, Jul 16, 2026 at 3:29 PM Siddharth Shrimali
<r.siddharth.shrimali@gmail.com> wrote:
>
> Add two new command-line options to 'git-repack':
>
>   --drop-filtered: intended to eventually delete objects that match
>                    the filter specification. Requires --filter and -a,
>                    and is incompatible with --filter-to.
>   --dry-run: show which objects would be dropped without making any
>              changes. Only meaningful with --drop-filtered.

An alternative would be `--drop-filtered[=dry-run]`, which might be
extended with other `--drop-filtered` specific options later.

I think separating `--dry-run` from `--drop-filtered` like this patch
does makes sense though if we think that `--dry-run` could be useful
later without `--drop-filtered`. The fact that a number of other
commands already have a `--dry-run` option might be a good sign.

Anyway it would be nice if the commit message explained a bit the
choice to have a separate `--dry-run` option.

Re: [RFC PATCH 1/7] builtin/repack.c: add --drop-filtered and --dry-run options

[Siddharth Shrimali]

Hey Christian,

On Sat, 18 Jul 2026 at 18:01, Christian Couder
<christian.couder@gmail.com> wrote:
> An alternative would be `--drop-filtered[=dry-run]`, which might be
> extended with other `--drop-filtered` specific options later.
>
> I think separating `--dry-run` from `--drop-filtered` like this patch
> does makes sense though if we think that `--dry-run` could be useful
> later without `--drop-filtered`. The fact that a number of other
> commands already have a `--dry-run` option might be a good sign.

Agreed. I chose a separate --dry-run mainly for consistency with the many other
Git commands that already use it that way, and because it leaves room
for --dry-run
to apply to other repack behavior later rather than being tied to
--drop-filtered.

That said, I do like the --drop-filtered[=dry-run] form for keeping future
--drop-filtered-specific options together, and it might end up being the tidier
choice.

Since this is an RFC, I'd genuinely welcome others' thoughts on which one
holds up better in the long run, before settling on any one command-line.

>
> Anyway it would be nice if the commit message explained a bit the
> choice to have a separate `--dry-run` option.

Either way, for v2 I'll add a note to the commit message explaining
the final choice.

Thanks!

Re: [RFC PATCH 0/7] repack: add --drop-filtered to reclaim space in partial clones

[Siddharth Asthana]

On 16/07/26 18:58, Siddharth Shrimali wrote:
> This is an RFC series seeking feedback on the design and approach.
> Several pieces are still missing (noted below) and the commit
> organization needs cleanup.
> 
> Partial clones let you work with large repositories without downloading
> every blob up front and the missing blobs are lazily fetched from the promisor
> remote on demand. Over time, though, these lazily-fetched blobs
> accumulate locally and there is currently no safe, built-in way to
> reclaim that disk space instead of re-cloning.
> 
> This series adds a "git repack --drop-filtered --filter=<spec>" command
> that removes large, locally-held promisor blobs that are recoverable
> from the promisor remote. The dropped blobs become absent locally but
> remain lazily re-fetchable, making the partial-clone still reversible.
> 
> How it works:
>    * Enumerate promisor objects directly (ODB_FOR_EACH_OBJECT_PROMISOR_ONLY)
>      and select the blobs exceeding the filter threshold. Because every
>      enumerated object is a promisor object, it is guaranteed recoverable and
>      locally-created objects are never candidates.


This looks like the right approach to me. Going through the promisor 
repack path instead of write_filtered_pack() matches how repack already 
splits promisor objects out.


> 
>    * Rebuild the promisor pack without the selected blobs, reusing the
>      existing repack machinery, so the drop is crash-safe.
> 
>    * Record each dropped object in a drop log
>      ($GIT_DIR/objects/info/promisor-dropped) so a later change can
>      explain a failed lazy fetch (when it was dropped, which filter
>      matched, which remotes) instead of a bare "could not fetch" error.
> 
>    * --dry-run lists the candidates and changes nothing.
> 
> Planned follow-ups:
>    * Safety guards: refuse to run while a merge/rebase/cherry-pick is in
>      progress, and refuse to drop blobs referenced by the current index.


I think these matter before we present this as a real space-reclaim
tool. Without the index guard especially, users may drop blobs and then
immediately fetch them back on the next command that needs the worktree.

The drop log and remote-object-info can wait. I would not block the
next RFC round on them.

On the UI, I am fine with a separate --dry-run for now (same as
Christian). We can revisit a --drop-filtered=<mode> form later if we
grow more drop-specific options.

Thanks.
Siddharth


> 
>    * Authoritative remote verification: the drop log currently lists all
>      configured promisor remotes rather than the exact remote each object
>      is recoverable from, because there is no client-side way to query a
>      remote for object availability yet. A "remote-object-info" command
>      is being added to the "git cat-file --batch" protocol for this. Once
>      available, the exact remote can be recorded.
> 
> Known issues to address in v2:
>    * There is churn between "enumerate promisor blobs" and "actually drop
>      filtered promisor blobs". The former introduces
>      enumerate_promisor_blobs() with an interim signature that the latter
>      rewrites. These will be reorganized so the function is introduced
>      in its final form.
> 
>    * The tests are in a standalone commit. They will instead be
>      distributed into the commits that introduce the behavior they test.
> 
> Siddharth Shrimali (7):
>    builtin/repack.c: add --drop-filtered and --dry-run options
>    list-objects-filter: add list_objects_filter__filter_oidset()
>    repack-promisor: allow excluding objects from the rebuilt promisor
>      pack
>    builtin/repack: enumerate promisor blobs for --drop-filtered
>    t7706: test --drop-filtered enumeration and validation
>    builtin/repack: actually drop filtered promisor blobs
>    repack-promisor: record dropped objects in a drop log
> 
>   builtin/repack.c                |  76 ++++++++++++++++-
>   list-objects-filter.c           |  45 ++++++++++
>   list-objects-filter.h           |  16 ++++
>   repack-filtered.c               |  81 ++++++++++++++++++
>   repack-promisor.c               | 106 ++++++++++++++++++++++-
>   repack.h                        |  12 ++-
>   t/meson.build                   |   1 +
>   t/t7706-repack-drop-filtered.sh | 145 ++++++++++++++++++++++++++++++++
>   8 files changed, 478 insertions(+), 4 deletions(-)
>   create mode 100755 t/t7706-repack-drop-filtered.sh
> 

Re: [RFC PATCH 1/7] builtin/repack.c: add --drop-filtered and --dry-run options

[Siddharth Asthana]

On 17/07/26 02:38, Junio C Hamano wrote:
> Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:
> 
>> --drop-filtered is incompatible with bitmap writing: filtering breaks
>> the "all objects in one pack" closure that bitmaps require. An explicit
>> -b is rejected with a clear error and a default-on bitmap configuration is
>> silently disabled for the duration of the command.
> 
> That is very well intentioned.
> 
>> @@ -231,6 +234,10 @@ int cmd_repack(int argc,
>>   			   N_("pack prefix to store a pack containing pruned objects")),
>>   		OPT_STRING(0, "filter-to", &filter_to, N_("dir"),
>>   			   N_("pack prefix to store a pack containing filtered out objects")),
>> +		OPT_BOOL(0, "drop-filtered", &drop_filtered,
>> +				N_("delete filtered out objects (requires --filter)")),
>> +		OPT_BOOL(0, "dry-run", &dry_run,
>> +				N_("only show which objects would be dropped")),
>>   		OPT_END()
>>   	};
>>   
>> @@ -252,6 +259,43 @@ int cmd_repack(int argc,
>>   	po_args.depth = xstrdup_or_null(opt_depth);
>>   	po_args.threads = xstrdup_or_null(opt_threads);
>>   
>> +	die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
>> +		!!filter_to, "--filter-to");
>> +
>> +	die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
>> +		write_bitmaps > 0, "--write-bitmap-index");
> 
> Hmph.  Since this step does not change the parsing or configuration
> for write_bitmaps, we cannot tell if (write_bitmaps == 1) at this
> point in the execution came from the command line (e.g., an earlier
> call to parse_options() around line 247 of builtin/repack.c) or from
> the configuration files (e.g., a call to repo_config() around
> line 245).  In other words, wouldn't it be ...
> 
>> +	if (dry_run && !drop_filtered)
>> +		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"));
>> +
>> +		if (!(pack_everything & ALL_INTO_ONE))
>> +			die(_("--drop-filtered requires -a"));
>> +
>> +		/*
>> +		 * Only blob:limit=<n> is supported for now. Reject other
>> +		 * filter choices early, before walking the object database.
>> +		 */
>> +		if (po_args.filter_options.choice != LOFC_BLOB_LIMIT)
>> +			die(_("--drop-filtered only supports --filter=blob:limit=<n> for now"));
>> +
>> +		/*
>> +		 * Without a promisor remote there is nowhere to re-fetch the
>> +		 * dropped objects from, so dropping them would be permanent
>> +		 * data loss.
>> +		 */
>> +		if (!repo_has_promisor_remote(repo))
>> +			die(_("--drop-filtered requires a promisor remote"));
>> +
>> +		write_bitmaps = 0;
> 
> ... way too late to drop the flag here?



Yes, I agree. At that point write_bitmaps > 0 can come from either
-b/--write-bitmap-index or repack.writeBitmaps, so we cannot both
error on an explicit -b and silently clear a config default with the
same check.

For v2 it would be nice to treat those two cases differently.

Thanks.
Siddharth


> 
>> +	}
>> +
>>   	if (delete_redundant && repo->repository_format_precious_objects)
>>   		die(_("cannot delete packs in a precious-objects repo"));

Re: [RFC PATCH 7/7] repack-promisor: record dropped objects in a drop log

[Siddharth Asthana]

On 16/07/26 18:58, Siddharth Shrimali wrote:
> After --drop-filtered removes promisor blobs, append a record of each
> dropped object to $GIT_DIR/objects/info/promisor-dropped. Each line
> records the object ID, a reflog-style timestamp (Unix seconds and
> timezone), the filter spec, and the promisor remote it was attested
> recoverable from like the following:
> 
>    <oid> <time> <tz> filter=<spec> remote=<name>
> 
> If a dropped object later becomes unrecoverable (for example, the
> branch holding it is deleted on the promisor remote), a lazy fetch
> fails with a generic error. This persistent record lets a later change
> explain that the object was dropped deliberately, when, under which
> filter, and from which remote it was expected to be recoverable.


I like the idea of better errors when a later lazy fetch fails.

An alternative would be to wait until we actually have that error-path
change in the same series, so we do not grow an on-disk format that
nothing reads yet. I think keeping the log in the RFC is fine though
if you find it useful while developing; I would not treat it as
required for the first mergeable version.



> 
> The remote field lists all configured promisor remotes rather than the
> specific one each dropped object is recoverable from. Determining the
> exact remote would require asking the remote whether it has the object.
> A "remote-object-info" command is being added to the "git cat-file
> --batch" protocol for this kind of query, but it is not available yet.
> A NEEDSWORK marks this for a follow-up.
> 
> The log is written only on a real run, i.e. --dry-run changes nothing.
> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
>   builtin/repack.c  |  4 +++
>   repack-promisor.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++
>   repack.h          |  4 +++
>   3 files changed, 99 insertions(+)
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index aa3257a98a..49dcbbc567 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -702,6 +702,10 @@ int cmd_repack(int argc,
>   		write_midx_file(files->packed, NULL, NULL, flags);
>   	}
>   
> +	if (drop_filtered && !dry_run)
> +		append_drop_log(repo, &drop_oids,
> +			expand_list_objects_filter_spec(&po_args.filter_options));
> +
>   cleanup:
>   	string_list_clear(&keep_pack_list, 0);
>   	string_list_clear(&names, 1);
> diff --git a/repack-promisor.c b/repack-promisor.c
> index fabfdc168a..60913a5150 100644
> --- a/repack-promisor.c
> +++ b/repack-promisor.c
> @@ -7,6 +7,97 @@
>   #include "repository.h"
>   #include "run-command.h"
>   #include "oidset.h"
> +#include "date.h"
> +#include "promisor-remote.h"
> +#include "strbuf.h"
> +
> +/*
> + * Append the drop-log entries to the already-computed path.
> + * Returns -1 on any I/O failure so the caller can warn once.
> + * Keeping this in a separate helper avoids goto-based cleanup
> + * in append_drop_log();
> + */
> +static int write_to_drop_log(struct repository *repo,
> +			     const char *path,
> +			     const struct oidset *dropped,
> +			     const char *stamp,
> +			     const char *filter_spec,
> +			     const char *remotes)
> +{
> +	struct oidset_iter iter;
> +	const struct object_id *oid;
> +	FILE *fp;
> +
> +	if (safe_create_leading_directories(repo, (char *)path)) {
> +		warning(_("could not create leading directories for '%s'"), path);
> +		return -1;
> +	}
> +
> +	fp = fopen(path, "a");
> +	if (!fp) {
> +		warning_errno(_("could not open '%s'"), path);
> +		return -1;
> +	}
> +
> +	oidset_iter_init(dropped, &iter);
> +	while ((oid = oidset_iter_next(&iter))) {
> +		if (fprintf(fp, "%s %s filter=%s remote=%s\n",
> +				oid_to_hex(oid), stamp,
> +				filter_spec ? filter_spec : "",
> +				remotes) < 0) {
> +			warning(_("could not write to '%s'"), path);
> +			fclose(fp);
> +			return -1;
> +		}
> +	}
> +
> +	if (fclose(fp)) {
> +		warning_errno(_("could not close '%s'"), path);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +void append_drop_log(struct repository *repo,
> +		     const struct oidset *dropped,
> +		     const char *filter_spec)
> +{
> +	char *path;
> +	struct strbuf stamp = STRBUF_INIT;
> +	struct strbuf remotes = STRBUF_INIT;
> +	struct promisor_remote *pr;
> +
> +	if (!oidset_size(dropped))
> +		return;
> +
> +	datestamp(&stamp);
> +
> +	/*
> +	 * NEEDSWORK: we temporarily record all configured promisor remotes rather
> +	 * than the specific one a given object is recoverable from because there


Recording all promisor remotes for now looks OK to me with that
NEEDSWORK. I would not block this on remote-object-info.


> +	 * is currently no way to determine that locally. it would require
> +	 * asking the remote whether it has the object. A "remote-object-info"
> +	 * command is being added to the "git cat-file --batch" protocol for
> +	 * this kind of query. Once it is merged in the codebase, this should
> +	 * record the exact promisor remote that has each dropped object.
> +	 */
> +	for (pr = repo_promisor_remote_find(repo, NULL); pr; pr = pr->next) {
> +		if (remotes.len)
> +			strbuf_addch(&remotes, ',');
> +		strbuf_addstr(&remotes, pr->name);
> +	}
> +
> +	path = repo_git_path(repo, "objects/info/promisor-dropped");


If we keep it, it would be nice to document this path (for example in
gitrepository-layout) and to have a small test that a real drop appends
a line.


Thanks
Siddharth


> +
> +	if (write_to_drop_log(repo, path, dropped, stamp.buf,
> +			filter_spec, remotes.buf))
> +		warning(_("could not record all dropped objects in the drop log"));
> +
> +	strbuf_release(&stamp);
> +	strbuf_release(&remotes);
> +	free(path);
> +}
>   
>   struct write_oid_context {
>   	struct child_process *cmd;
> diff --git a/repack.h b/repack.h
> index 61e554e4ed..33309548ce 100644
> --- a/repack.h
> +++ b/repack.h
> @@ -171,6 +171,10 @@ int enumerate_promisor_blobs(struct repository *repo,
>   			     const struct list_objects_filter_options *filter,
>   			     struct oidset *to_drop);
>   
> +void append_drop_log(struct repository *repo,
> +		     const struct oidset *dropped,
> +		     const char *filter_spec);
> +
>   int write_cruft_pack(const struct write_pack_opts *opts,
>   		     const char *cruft_expiration,
>   		     unsigned long combine_cruft_below_size,

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

[Siddharth Asthana]

On 16/07/26 18:58, Siddharth Shrimali wrote:
> 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 <christian.couder@gmail.com>
> Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
>   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;


Yes, without that the drop would not actually reclaim space.

It would be nice if the documentation mentioned that a real
--drop-filtered run implies -d.


Thanks


> +
> +		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