Junio C Hamano [off-list ref] writes:

Sergey Organov [off-list ref] writes:

quoted

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.

quoted

 	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.

quoted

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

However.

quoted

-	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