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

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

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.

>  	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.

> -	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.

> +	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