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

[Junio C Hamano <gitster@pobox.com>]

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"));