Junio C Hamano <gitster@xxxxxxxxx> writes:
> Sergey Organov <sorganov@xxxxxxxxx> 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
