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

On 17/07/26 02:38, Junio C Hamano wrote:
> Siddharth Shrimali <[email protected]> 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"));