Re: [PATCH 1/1] clean: further clean-up of implementation around "--force"

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2024, 22:18 UTC
Message-ID: <xmqq5xy3vu0x.fsf@gitster.g>
In-Reply-To: <20240303220600.2491792-2-gitster@pobox.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> @@ -950,13 +950,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>  			     0);
>  
> -	/* Dry run won't remove anything, so requiring force makes no sense */
> -	if (dry_run)
> -		require_force = 0;
> -
> -	if (require_force != 0 && !force && !interactive)
> -		die(_("clean.requireForce is true and neither -f nor -i given:"
> -				  " refusing to clean"));
> +	if (require_force != 0 && !force && !interactive && !dry_run)
> +		die(_("clean.requireForce is true and -f not given: refusing to clean"));
>  
>  	if (force > 1)
>  		rm_flags = 0;

An obvious alternative way to clean-up the logic is to do this instead:

	if (dry_run || interactive))
		require_force = 0;
 	if (require_force != 0 && !force)
		die(_("clean.requireForce is true and ..."));

But as I wrote, the most important improvement done by Sergey's patch was to remove the dual meaning of the "force" variable so that it indicates if the "--force" option was given and nothing else, while the "require_force" variable indicates if clean.requireForce was given and nothing else. From that point of view, the conditional tweaking done to require_force in the above alternative makes the code worse, relative to Sergey's patch, and certainly to its follow up, my patch about "--interactive".