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

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 <[email protected]>
> Mentored-by: Siddharth Asthana <[email protected]>
> Signed-off-by: Siddharth Shrimali <[email protected]>
> ---
>   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