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

From: Sergey Organov <sorganov@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] clean: improve -n and -f implementation and documentation
Date: Sat, 02 Mar 2024 22:59:59 +0300	[thread overview]
Message-ID: <87wmqk8kw0.fsf@osv.gnss.ru> (raw)
In-Reply-To: <xmqqv864zjbf.fsf@gitster.g> (Junio C. Hamano's message of "Sat, 02 Mar 2024 08:31:48 -0800")

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

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

I believe things rather got more consistent, see below.

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

Except, unlike dry-run, interactive is not harmless, similar to -f.

> So we could instead add
>
> 	if (dry_run || interactive)
> 		require_force = 0;
>
> above, drop the "&& !interactive" from the guard for the
> clean.requireForce block.

That'd be less consistent, as dry-run is harmless, whereas neither force
nor interactive are.

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

No, we do need to tweak require_force, as another if() that is inside
and produces error message does in fact check for require_force being
either negative or positive, i.e., non-zero.

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

I believe it's rather right in the spot. I left '-i' to stay with '-f',
as it was before the patch, as both are very distinct (even if in
different manner) when compared to '-n', so now only '-n' is now treated
separately.

The very idea of dry-run is that it is orthogonal to any other behavior,
so if I were designing it, I'd left bailing-out without -f or -i in
place even if -n were given, to show what exactly would happen without
-n. With new code it'd be as simple as removing "if (dry_run)
require_force = 0" line that introduces the original dependency.

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

In fact, the new code rather keep treating -f and -i somewhat similarly,
rather than -i and -n, intentionally.

That said, if somebody is going to re-consider -f vs -i issue, they now
have more cleaner code that doesn't involve -n anymore.

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

I'm not sure about interactive not requiring force, and I intentionally
avoided this issue in the patch in question, though I think the patch
makes it easier to reason about -i vs -f in the future by removing -n
handling from the picture.

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

I'm afraid we can't meaningfully keep -n (--dry-run) in the messages. As
it stands, having -n there was a mistake right from the beginning.
Please consider the original message, but without -i and -f, for the
sake of the argument:

 "clean.requireForce set to true and -n is not given; refusing to clean"

to me it sounds like nonsense, as it suggests that if were given -n,
we'd perform cleanup, that is simply false as no cleanup is ever
performed once -n is there. Adding -i and -f back to the message
somewhat blurs the problem, yet -n still does not belong there.

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

I don't think so, see above. I rather believe that even if everything
else in the patch were denied, the -n should be removed from the error
message, so I did exactly that, and only that (i.e., didn't merge 2
messages into one).

Thanks,
-- Sergey Organov