Re: [PATCH] clean: improve -n and -f implementation and documentation

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2024, 16:31 UTC
Message-ID: <xmqqv864zjbf.fsf@gitster.g>
In-Reply-To: <875xy76qe1.fsf@osv.gnss.ru>

Sergey Organov <sorganov@gmail.com> writes:

> What -n actually does in addition to its documented behavior is
> ignoring of configuration variable clean.requireForce, that makes
> sense provided -n prevents files removal anyway.

There is another thing I noticed.

This part to get rid of "config_set" does make sense.

Show 5 quoted lines

>  	git_config(git_clean_config, NULL);
> -	if (force < 0)
> -		force = 0;
> -	else
> -		config_set = 1;

We used to think "force" variable is the master switch to do anything , and requireForce configuration was a way to flip its default to 0 (so that you need to set it to 1 again from the command line). This separates "force" (which can only given via the command line) and "require_force" (which controls when the "force" is used) and makes the logic simpler.

>  	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>  			     0);

However.

Show 6 quoted lines

> -	if (!interactive && !dry_run && !force) {
> -		if (config_set)
> -			die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
> +	/* Dry run won't remove anything, so requiring force makes no sense */
> +	if(dry_run)
> +		require_force = 0;

I am not sure if this is making things inconsistent.

Dry run will be harmless, and we can be lenient and not require force. But below, we do not require force when going interactive, either. So we could instead add

	if (dry_run || interactive)
		require_force = 0;

above, drop the "&& !interactive" from the guard for the clean.requireForce block.

Or we can go the opposite way. We do not have to tweak require_force at all based on other conditions. Instead we can update the guard below to check "!force && !interactive && !dry_run" before entering the clean.requireForce block, no?

But the code after this patch makes me feel that it is somewhere in the middle between these two optimum places.

Another thing. Stepping back and thinking _why_ the code can treat dry_run and interactive the same way (either to make them drop require_force above, or neither of them contributes to the value of require_force), if we are dropping "you didn't give me --dry-run" in the error message below, we should also drop "you didn't give me --interactive, either" as well, when complaining about the lack of "--force".

One possible objection I can think of against doing so is that it might not be so obvious why "interactive" does not have to require "force" (even though it is clearly obvious to me). But if that were the objection, then to somebody else "dry-run does not have to require force" may equally not be so obvious (at least it wasn't so obvious to me during the last round of this discussion).

So I can live without the "drop 'nor -i'" part I suggested in the above. We would not drop "nor -i" and add "nor --dry-run" back to the message instead.

So from that angle, the message after this patch makes me feel that it is somewhere in the middle between two more sensible places.

Show 15 quoted lines

> +	if (!force && !interactive) {
> +		if (require_force > 0)
> +			die(_("clean.requireForce set to true and neither -f, nor -i given; "
> +				  "refusing to clean"));
> +		else if (require_force < 0)
> +			die(_("clean.requireForce defaults to true and neither -f, nor -i given; "
>  				  "refusing to clean"));
> -		else
> -			die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;"
> -				  " refusing to clean"));
>  	}
>  
>  	if (force > 1)
>
> base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d