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

On 30/07/26 23:11, Siddharth Shrimali 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.
> 
> Keep --dry-run as a separate option rather than folding it into
> --drop-filtered (e.g --drop-filtered=dry-run), to stay consistent with
> the --dry-run option other Git commands already provide and to leave
> room for it to describe other repack behavior later. A
> --drop-filtered=<mode> form can still be added later if more
> drop-specific modes are needed.
> 
> --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. Snapshot
> the bitmap setting after config but before option parsing so an
> explicit -b/--write-bitmap-index on the command line can be told apart
> from a repack.writeBitmaps configuration value. An explicit -b is
> reported as a conflict, while a config-provided default 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 <[email protected]>
> Mentored-by: Siddharth Asthana <[email protected]>
> Signed-off-by: Siddharth Shrimali <[email protected]>
> ---
>   builtin/repack.c                | 63 +++++++++++++++++++++++++++++++++
>   t/meson.build                   |  1 +
>   t/t7706-repack-drop-filtered.sh | 49 +++++++++++++++++++++++++
>   3 files changed, 113 insertions(+)
>   create mode 100755 t/t7706-repack-drop-filtered.sh
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index db504d673f..322b01cb3e 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"
> @@ -148,6 +151,7 @@ int cmd_repack(int argc,
>   	/* variables to be filled by option parsing */
>   	struct repack_config_ctx config_ctx;
>   	int delete_redundant = 0;
> +	int write_bitmaps_before_parse;
>   	const char *unpack_unreachable = NULL;
>   	int keep_unreachable = 0;
>   	struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
> @@ -231,6 +235,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()
>   	};
>   
> @@ -244,6 +252,13 @@ int cmd_repack(int argc,
>   
>   	repo_config(repo, repack_config, &config_ctx);
>   
> +	/*
> +	 * update the bitmap setting after config but before command line
> +	 * parsing, so we can later tell whether -b/--write-bitmap-index was
> +	 * given explicitly on the command line or not
> +	 */
> +	write_bitmaps_before_parse = write_bitmaps;
> +
>   	argc = parse_options(argc, argv, prefix, builtin_repack_options,
>   				git_repack_usage, 0);
>   
> @@ -252,6 +267,54 @@ 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) {
> +		int bitmaps_from_cmdline = (write_bitmaps != write_bitmaps_before_parse);
> +
> +		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"));
> +
> +		/*
> +		 * an explicit -b on the command line is a conflict we have to
> +		 * report, a bitmap setting from config is silently overridden
> +		 * for the duration of the command
> +		 */
> +		if (bitmaps_from_cmdline && write_bitmaps > 0)
> +			die(_("options '%s' and '%s' cannot be used together"),
> +				"--drop-filtered", "--write-bitmap-index");


Thanks for tackling the bitmap CLI vs config split.

One case still looks off: if repack.writeBitmaps is already true and
the user also passes -b, write_bitmaps is 1 before and after parse, so
bitmaps_from_cmdline stays false and we never error. I think explicit
-b should still be rejected there.

Thanks.
Siddharth



> +
> +		/*
> +		 * 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"));
>   
> diff --git a/t/meson.build b/t/meson.build
> index d8161c368b..c2bf60d129 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -963,6 +963,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..65be756e33
> --- /dev/null
> +++ b/t/t7706-repack-drop-filtered.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +
> +test_description='git repack --drop-filtered option validation'
> +
> +. ./test-lib.sh
> +
> +# 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 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
> +'
> +
> +test_done