what should "git clean -n -f [-d] [-x] <pattern>" do?

what should "git clean -n -f [-d] [-x] <pattern>" do?

[Junio C Hamano]

I think the current code makes "-n" take precedence, and ignores
"-f".  Shouldn't it either

 (1) error out with "-n and -f cannot be used together", or
 (2) let "-n" and "-f" follow the usual "last one wins" rule?

The latter may be logically cleaner but it is a change that breaks
backward compatibility big time in a more dangerous direction, so it
may not be desirable in practice, with too big a downside for a too
little gain.

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

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

> I think the current code makes "-n" take precedence, and ignores
> "-f".

To me it rather looks more like "-n" implies "-f", but then there is
"second -f" rule that makes things even more interesting:

  "Git will refuse to modify untracked nested git repositories
   (directories with a .git subdirectory) unless a second -f is given."

How do I figure what files will be deleted on

  git clean -f -f

when "-n" behaves as you (or me) described? I.e., what

  git clean -f -f -n

and

  git clean -f -n

will output?

>
> Shouldn't it either
>
>  (1) error out with "-n and -f cannot be used together", or
>  (2) let "-n" and "-f" follow the usual "last one wins" rule?
>
> The latter may be logically cleaner but it is a change that breaks
> backward compatibility big time in a more dangerous direction, so it
> may not be desirable in practice, with too big a downside for a too
> little gain.

I agree (2) is too dangerous and surprising, and (1) is limiting: I
believe the user should be able to see what will be done on

   git clean -f -f

by simply adding "-n" to the command-line.

So I figure I'd rather prefer yet another option:

(3) -n  dry run: show what will be done once "-n" is removed.

This way, e.g.,

  git clean

and

  git clean -n

will produce exactly the same output with default configuration:

  fatal: clean.requireForce defaults to true and neither -i, nor -f given; refusing to clean

and one will need to say, e.g.:

  git clean -n -f

to get the list of files to be deleted with "git clean -f".

With (3) "-n" becomes orthogonal to "-f", resulting in predictable and
useful behavior.

BR,
-- Sergey Organov

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Elijah Newren]

On Tue, Jan 9, 2024 at 12:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> I think the current code makes "-n" take precedence, and ignores
> "-f".

:-(

>  Shouldn't it either
>
>  (1) error out with "-n and -f cannot be used together", or
>  (2) let "-n" and "-f" follow the usual "last one wins" rule?

I believe so.

> The latter may be logically cleaner but it is a change that breaks
> backward compatibility big time in a more dangerous direction, so it
> may not be desirable in practice, with too big a downside for a too
> little gain.

Yeah, I think (1) is the safer option, for now.  We could potentially
do (1), then wait a long time, then switch to (2).

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> On Tue, Jan 9, 2024 at 12:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I think the current code makes "-n" take precedence, and ignores
>> "-f".
>
> :-(
>
>>  Shouldn't it either
>>
>>  (1) error out with "-n and -f cannot be used together", or
>>  (2) let "-n" and "-f" follow the usual "last one wins" rule?
>
> I believe so.

Then how does one figure what "git clean -f -f" will do without actually
doing it?

Please notice that -f -f is special according to the manual:

  "Git will refuse to modify untracked nested git repositories
   (directories with a .git subdirectory) unless a second -f is given."

I looks like neither (0), nor (1) nor (2) gives us any useful behavior
in this case.

I figure the best solution is to rather make -n orthogonal to -f, that
will solve the puzzle, and that is what actually expected from a "dry
run" option: don't change any behavior, except print actions instead of
performing them.

-- Sergey Organov

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Then how does one figure what "git clean -f -f" will do without actually
> doing it?

I think whoever came up with the bright idea of forcing twice
somehow does a totally different thing from forcing once should be
shot, twice ;-)  It does not mesh well with the idea behind the
clean.requireForce setting to make you explicitly choose either '-f'
or '-n' to express your intent.

I wonder how feasible is it to deprecate that misfeature introduced
with a0f4afbe (clean: require double -f options to nuke nested git
repository and work tree, 2009-06-30) and migrate its users (which
is marked as "This is rarely what the user wants") to a new option,
say, --nested-repo-too so that the "dry-run" version of the
invocations become

    git clean -n
    git clean -n --nested-repo-too

and you can substitute "-n" with "-f" to actually perform it?

Anybody care to come up with a sensible migration plan?

Thanks.

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Then how does one figure what "git clean -f -f" will do without actually
>> doing it?
>
> I think whoever came up with the bright idea of forcing twice
> somehow does a totally different thing from forcing once should be
> shot, twice ;-)  It does not mesh well with the idea behind the
> clean.requireForce setting to make you explicitly choose either '-f'
> or '-n' to express your intent.

I agree, yet I see it as another deficiency, in addition to that of -n,
and I used it as an example to emphasize the deficiency of -n.

> I wonder how feasible is it to deprecate that misfeature introduced
> with a0f4afbe (clean: require double -f options to nuke nested git
> repository and work tree, 2009-06-30) and migrate its users (which
> is marked as "This is rarely what the user wants") to a new option,
> say, --nested-repo-too so that the "dry-run" version of the
> invocations become
>
>     git clean -n
>     git clean -n --nested-repo-too
>
> and you can substitute "-n" with "-f" to actually perform it?

Whereas obsoleting second -f in favor of new --nested-repo might be a
good idea indeed, I believe it's still a mistake for "dry run" to
somehow interfere with -f, sorry.

Thanks,
-- Sergey Organov

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Whereas obsoleting second -f in favor of new --nested-repo might be a
> good idea indeed, I believe it's still a mistake for "dry run" to
> somehow interfere with -f, sorry.

No need to be sorry ;-)

I actually think the true culprit of making this an odd-man-out is
that the use of "-f" in "git clean", especially with its use of the
configuration variable clean.requireForce that defaults to true, is
utterly non-standard.

The usual pattern of defining what "-f" does is that the "git foo"
command without any options does its common thing but refuses to
perform undesirable operations (e.g. "git add ."  adds everything
but refrains from adding ignored paths). And "git foo -f" is a way
to also perform what it commonly skips.

In contrast, with clean.requireForce that defaults to true, "git
clean" does not do anything useful by default.  Without such a
safety, "git clean" would be a way to clean expendable paths, and
"git clean -f" might be to also clean precious paths.  But it does
not work that way.  It always requires "-f" to do anything.  Worse
yet, it is not even "by default it acts as if -n is given and -f is
a way to countermand that implicit -n".  It is "you must give me
either -f (i.e. please do work) or -n (i.e. please show what you
would do) before I do anything".

  $ git clean
  fatal: clean.requireForce defaults to true and neither -i, -n, nor -f given; refusing to clean

Given that, it is hard to argue that it would be a natural end-user
expectation that the command does something useful (i.e. show what
would be done) when it is given "-f" and "-n" at the same time.
What makes this a rather nonsense UI is the fact that "-f" does not
work the way we would expect for this command.

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Whereas obsoleting second -f in favor of new --nested-repo might be a
>> good idea indeed, I believe it's still a mistake for "dry run" to
>> somehow interfere with -f, sorry.
>
> No need to be sorry ;-)
>
> I actually think the true culprit of making this an odd-man-out is
> that the use of "-f" in "git clean", especially with its use of the
> configuration variable clean.requireForce that defaults to true, is
> utterly non-standard.
>
> The usual pattern of defining what "-f" does is that the "git foo"
> command without any options does its common thing but refuses to
> perform undesirable operations (e.g. "git add ."  adds everything
> but refrains from adding ignored paths). And "git foo -f" is a way
> to also perform what it commonly skips.
>
> In contrast, with clean.requireForce that defaults to true, "git
> clean" does not do anything useful by default.  Without such a
> safety, "git clean" would be a way to clean expendable paths, and
> "git clean -f" might be to also clean precious paths.  But it does
> not work that way.  It always requires "-f" to do anything.  Worse
> yet, it is not even "by default it acts as if -n is given and -f is
> a way to countermand that implicit -n".  It is "you must give me
> either -f (i.e. please do work) or -n (i.e. please show what you
> would do) before I do anything".
>
>   $ git clean
>   fatal: clean.requireForce defaults to true and neither -i, -n, nor -f given; refusing to clean
>
> Given that, it is hard to argue that it would be a natural end-user
> expectation that the command does something useful (i.e. show what
> would be done) when it is given "-f" and "-n" at the same time.
> What makes this a rather nonsense UI is the fact that "-f" does not
> work the way we would expect for this command.

I think we all agree that current UI is a kind of nonsense, but have
different views of the optimal target interface. My points are as
following:

1. The fact that bare "git clean" only produces error by default is
probably a good thing, as removal of untracked files is unrecoverable
operation in Git domain, so requiring -f by default is probably a good
thing as well, provided the *only* operation that "git clean" performs
is dangerous enough.

2. The "-n" behavior is pure nonsense.

So, how do we fix (2)? Let's try mental experiment. Suppose there is no
"-n" option for "git clean" and we are going to implement it. We start
from:

  $ git clean
  fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean
  $ git clean -f
  removing "a"
  removing "b"
  $

Please notice that there is no "-n" in the error message as there is no
such option yet in our experiment.

Now we are going to introduce "dry run" option "-n". Most simple and
obvious way to do it is to set internal flag "dry_run" and then at every
invocation of "remove(file_name)" put an if(dry_run) that will just
print(file_name) instead or removing it. Let's suppose we did just that.
We get this behavior:

  $ git clean -n
  fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean
  $ git clean -f -n
  would remove "a"
  would remove "b"
  $ git clean -f -f -n
  would remove "a"
  would remove "b"
  would remove "sub/a"
  $

I see this as logical, clean, and straightforward behavior, meeting user
expectations for "dry run" option, so I suggest to do just that.

Thanks,
-- Sergey Organov.

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Now we are going to introduce "dry run" option "-n". Most simple and
> obvious way to do it is to set internal flag "dry_run" and then at every
> invocation of "remove(file_name)" put an if(dry_run) that will just
> print(file_name) instead or removing it. Let's suppose we did just that.
> We get this behavior:
>
>   $ git clean -n
>   fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean
>   $ git clean -f -n
>   would remove "a"
>   would remove "b"
>   $ git clean -f -f -n
>   would remove "a"
>   would remove "b"
>   would remove "sub/a"
>   $
>
> I see this as logical, clean, and straightforward behavior, meeting user
> expectations for "dry run" option, so I suggest to do just that.

I think we are saying the same thing.  If the original semantics
were "you must force with -f to do anything useful", instead of "you
must choose either forcing with -f or not doing with -n", then it
would have led to the above behaviour.

The thing is, it is way too late to change it that way without
breaking too many folks, and that is the problem.

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Now we are going to introduce "dry run" option "-n". Most simple and
>> obvious way to do it is to set internal flag "dry_run" and then at every
>> invocation of "remove(file_name)" put an if(dry_run) that will just
>> print(file_name) instead or removing it. Let's suppose we did just that.
>> We get this behavior:
>>
>>   $ git clean -n
>>   fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean
>>   $ git clean -f -n
>>   would remove "a"
>>   would remove "b"
>>   $ git clean -f -f -n
>>   would remove "a"
>>   would remove "b"
>>   would remove "sub/a"
>>   $
>>
>> I see this as logical, clean, and straightforward behavior, meeting user
>> expectations for "dry run" option, so I suggest to do just that.
>
> I think we are saying the same thing.  If the original semantics
> were "you must force with -f to do anything useful", instead of "you
> must choose either forcing with -f or not doing with -n", then it
> would have led to the above behaviour.
>
> The thing is, it is way too late to change it that way without
> breaking too many folks, and that is the problem.

If we agree on the behavior above for sane "dry run", yet you worry
about backward compatibility so much to deny changing the behavior of
"-n", then a way to go could be to introduce, say, "-d" for sane "dry
run", and obsolete "-n" while keeping it alone.

Thanks,
-- Sergey Organov

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Now we are going to introduce "dry run" option "-n". Most simple and
>>> obvious way to do it is to set internal flag "dry_run" and then at every
>>> invocation of "remove(file_name)" put an if(dry_run) that will just
>>> print(file_name) instead or removing it. Let's suppose we did just that.
>>> We get this behavior:
>>>
>>>   $ git clean -n
>>>   fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean
>>>   $ git clean -f -n
>>>   would remove "a"
>>>   would remove "b"
>>>   $ git clean -f -f -n
>>>   would remove "a"
>>>   would remove "b"
>>>   would remove "sub/a"
>>>   $
>>>
>>> I see this as logical, clean, and straightforward behavior, meeting user
>>> expectations for "dry run" option, so I suggest to do just that.
>>
>> I think we are saying the same thing.  If the original semantics
>> were "you must force with -f to do anything useful", instead of "you
>> must choose either forcing with -f or not doing with -n", then it
>> would have led to the above behaviour.
>>
>> The thing is, it is way too late to change it that way without
>> breaking too many folks, and that is the problem.
>
> If we agree on the behavior above for sane "dry run", yet you worry
> about backward compatibility so much to deny changing the behavior of
> "-n", then a way to go could be to introduce, say, "-d" for sane "dry
> run", and obsolete "-n" while keeping it alone.

Except exactly "-d" is already taken, but you get the idea.

-- Sergey Organov

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>> ..
>>> ...  If the original semantics
>>> were "you must force with -f to do anything useful", instead of "you
>>> must choose either forcing with -f or not doing with -n", then it
>>> would have led to the above behaviour.
>> ...
>> If we agree on the behavior above for sane "dry run"...

Not so fast.  I said "if the original semantics were ... then it
would have led to the above behaviour".  As the original semantics
were not, that conclusion does not stand.

The "-n" option here were not added primarily as a dry-run option,
and haven't been treated as such forever.  As can be seen by the
"you must give either -f or -n option, and it is an error to give
neither" rule, from the end-user's point of view, it is a way to say
"between do-it (-f) and do-not-do-it (-n), I choose the latter for
this invocation".  And in that context, an attempt to make "-f -f"
mean a stronger form of forcing than "-f" was a mistake, because it
makes your "I want to see what happens if I tried that opration that
requires the stronger force" request impossible.

And there are two equally valid ways to deal with this misfeature.

One is to admit that "-f -f" was a mistake (which I already said),
and a natural consequence of that admission is to introduce a more
specific "in addition to what you do usually, this riskier operation
is allowed" option (e.g., --nested-repo).  This leads to a design
that matches real world usage better, even if we did not have the
"how to ask dry-run?" issue, because in the real world, when there
are multiple "risky" things you may have to explicitly ask to
enable, these things do not necessarily form a nice linear
"riskiness levels" that you can express your risk tolerance with the
number of "-f" options.  When you need to add special protection for
a new case other than "nested repo", for example, the "riskiness
levels" design may need to place it above the "nested repo" level of
riskiness and may require the user to give three "-f" options, but
that would make it impossible to protect against nuking of nested
repos while allowing only that newly added case.  By having more
specific "this particular risky operation is allowed", "-f" can
still be "between do-it and do-not-do-it, I choose the former", and
the "--nested-repo" (and other options to allow specific risky
operations we add in the future) would not have to have funny
interactions with "-n".

The other valid way is to treat the use of the "riskiness levels" to
specify what is forced still as a good idea.  If one comes from that
position, the resulting UI would be consistent with what you have
been advocating for.  One or more "-f" will specify what kind of
risky stuff are allowed, and "-n" will say whether the operation
gets carried out or merely shown what would happen if "-n" weren't
there.

It is just that I think "riskiness levels" I did in a0f4afbe (clean:
require double -f options to nuke nested git repository and work
tree, 2009-06-30) was an utter mistake, and that is why I feel very
hesitant to agree with the design that still promotes it.

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> ..
>>>> ...  If the original semantics
>>>> were "you must force with -f to do anything useful", instead of "you
>>>> must choose either forcing with -f or not doing with -n", then it
>>>> would have led to the above behaviour.
>>> ...
>>> If we agree on the behavior above for sane "dry run"...
>
> Not so fast.  I said "if the original semantics were ... then it
> would have led to the above behaviour".  As the original semantics
> were not, that conclusion does not stand.

OK, fine, then my point is that the original semantics if flawed.

>
> The "-n" option here were not added primarily as a dry-run option,
> and haven't been treated as such forever.  As can be seen by the
> "you must give either -f or -n option, and it is an error to give
> neither" rule, from the end-user's point of view, it is a way to say
> "between do-it (-f) and do-not-do-it (-n), I choose the latter for
> this invocation".

Yep, and in my opinion this is even more a mistake than "-f -f".

> And in that context, an attempt to make "-f -f"
> mean a stronger form of forcing than "-f" was a mistake, because it
> makes your "I want to see what happens if I tried that opration that
> requires the stronger force" request impossible.

I believe this just emphasizes the original mistake of "-n" design
meaning something else than simple "dry run".

>
> And there are two equally valid ways to deal with this misfeature.

I rather see two almost independent misfeatures here, so I believe both
are to be addressed.

>
> One is to admit that "-f -f" was a mistake (which I already said),
> and a natural consequence of that admission is to introduce a more
> specific "in addition to what you do usually, this riskier operation
> is allowed" option (e.g., --nested-repo).

This addresses one of the two deficiencies I see, yes.

> This leads to a design that matches real world usage better, even if
> we did not have the "how to ask dry-run?" issue, because in the real
> world, when there are multiple "risky" things you may have to
> explicitly ask to enable, these things do not necessarily form a nice
> linear "riskiness levels" that you can express your risk tolerance
> with the number of "-f" options. When you need to add special
> protection for a new case other than "nested repo", for example, the
> "riskiness levels" design may need to place it above the "nested repo"
> level of riskiness and may require the user to give three "-f"
> options, but that would make it impossible to protect against nuking
> of nested repos while allowing only that newly added case. By having
> more specific "this particular risky operation is allowed", "-f" can
> still be "between do-it and do-not-do-it, I choose the former",

Yep, makes sense.

> and  the "--nested-repo" (and other options to allow specific risky
> operations we add in the future) would not have to have funny
> interactions with "-n".

Yep, but it still leaves "-n" being defective, as it for whatever reason
surprisingly clashes with "-f". I believe it shouldn't.

> The other valid way is to treat the use of the "riskiness levels" to
> specify what is forced still as a good idea.  If one comes from that
> position, the resulting UI would be consistent with what you have
> been advocating for.  One or more "-f" will specify what kind of
> risky stuff are allowed, and "-n" will say whether the operation
> gets carried out or merely shown what would happen if "-n" weren't
> there.

I'm not arguing in favor of "-f -f". My point is that even if you fix
"-f -f", "-n" deficiency will still cry for fixing.

>
> It is just that I think "riskiness levels" I did in a0f4afbe (clean:
> require double -f options to nuke nested git repository and work
> tree, 2009-06-30) was an utter mistake, and that is why I feel very
> hesitant to agree with the design that still promotes it.

Again, I'm not arguing in favor of "-f -f", I'm rather neutral about it.

I'm still arguing in favor of fixing "-n", and I believe a fix is needed
independently from decision about "-f -f".

Thanks,
-- Sergey Organov

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> I'm still arguing in favor of fixing "-n", and I believe a fix is needed
> independently from decision about "-f -f".

Even though I do not personally like it, I do not think "which
between do-it (f) and do-not-do-it (n) do you want to use?" is
broken.  It sometimes irritates me to find "git clean" (without "-f"
or "-n", and with clean.requireForce not disabled) complain, and I
personally think "git clean" when clean.requireForce is in effect
and no "-n" or "-f" were given should pretend as if "-n" were given.
I wish if it were "without -n or -f, we pretend as if -n were given,
possibly with a warning that says 'you need -f if you actually want
to carry out these operations'".

But that is a separate usability issue.

What I find broken is that giving one 'f' and one 'n' in different
order, i.e. "-f -n" and "-n -f", does not do what I expect.  If you
are choosing between do-it (f) and do-not-do-it (n), you ought to be
able to rely on the usual last-one-wins rule.  That I find broken.

The mistake[*] of "-f -f" is rather obvious, given that the other
"normal" ways to tweak what is affected by the command are done as
"what else do we clean? directories (d)? ignored (x)?..." options.
When we add the upcoming "precious" bit support, we should make sure
that the way to trigger "oh, by the way, please clobber those paths
that are marked precious, too" is not by giving three '-f'.  It
would make it impossible to ask for that without also removing
nested repositories, which takes two '-f'.


[Footnote]

 * To a lessor extent, the -v (verbose) option shares the same
   problem as "-f -f" here, in that its worldview is to assume that
   a single "verbosity level" is sufficient.  Unlike the severity
   level thing, however, the user who wanted to see only messages
   about X but have to also see messages about Y and Z that are at
   the same or lessor verbosity level as X can filter out unwanted
   messages without causing a real harm.

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> I'm still arguing in favor of fixing "-n", and I believe a fix is needed
>> independently from decision about "-f -f".
>
> Even though I do not personally like it, I do not think "which
> between do-it (f) and do-not-do-it (n) do you want to use?" is
> broken.

Well, you are right, but "-n" is not documented as "do-not-do-it" in the
sense you use it here. 

> It sometimes irritates me to find "git clean" (without "-f"
> or "-n", and with clean.requireForce not disabled) complain, and I
> personally think "git clean" when clean.requireForce is in effect
> and no "-n" or "-f" were given should pretend as if "-n" were given.
> I wish if it were "without -n or -f, we pretend as if -n were given,
> possibly with a warning that says 'you need -f if you actually want
> to carry out these operations'".

Yep, then we'd not need "-n" that much, only if to cancel explicit "-f"
(provided "-f -f" feature is removed.)

>
> But that is a separate usability issue.

Yep, and that'd be very different design. 

>
> What I find broken is that giving one 'f' and one 'n' in different
> order, i.e. "-f -n" and "-n -f", does not do what I expect.  If you
> are choosing between do-it (f) and do-not-do-it (n), you ought to be
> able to rely on the usual last-one-wins rule.  That I find broken.

I fail to see where this expectation comes from, provided "-n" is not
documented as anything opposed to "-f":

       -n, --dry-run
           Don’t actually remove anything, just show what would be done.

This is typical convenient description of "dry run", and current "-n"
implementation is rather close to the description, that I'd still
rewrite to emphasize the primary goal of the --dry-run:

       -n, --dry-run
           Show what would be done, and don’t actually remove anything.

With these descriptions, the last thing that I'd expect is "-n -f"
removing my files.

Overall, as I see it, we have buggy implementation of suitably
documented "--dry-run" option, and the best course is to fix the
bug, with no semantic changes to the option itself.

Thanks,
-- Sergey Organov

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> I'm still arguing in favor of fixing "-n", and I believe a fix is needed
>> independently from decision about "-f -f".
>
> Even though I do not personally like it, I do not think "which
> between do-it (f) and do-not-do-it (n) do you want to use?" is
> broken.  It sometimes irritates me to find "git clean" (without "-f"
> or "-n", and with clean.requireForce not disabled) complain, and I
> personally think "git clean" when clean.requireForce is in effect
> and no "-n" or "-f" were given should pretend as if "-n" were given.

As a note, I'd consider to get rid of 'clean.requireForce' anyway, as
its default value provides safe reasonably behaving environment, and I
fail to see why anybody would need to set it to 'false'.

Thanks,
-- Sergey Organov

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Jeff King]

On Mon, Jan 29, 2024 at 12:35:49PM +0300, Sergey Organov wrote:

> >> I'm still arguing in favor of fixing "-n", and I believe a fix is needed
> >> independently from decision about "-f -f".
> >
> > Even though I do not personally like it, I do not think "which
> > between do-it (f) and do-not-do-it (n) do you want to use?" is
> > broken.  It sometimes irritates me to find "git clean" (without "-f"
> > or "-n", and with clean.requireForce not disabled) complain, and I
> > personally think "git clean" when clean.requireForce is in effect
> > and no "-n" or "-f" were given should pretend as if "-n" were given.
> 
> As a note, I'd consider to get rid of 'clean.requireForce' anyway, as
> its default value provides safe reasonably behaving environment, and I
> fail to see why anybody would need to set it to 'false'.

Please don't. I set it to "false", because I find the default behavior a
pointless roadblock if you are already aware that "git clean" can be
destructive. Surely I can't be the only one.

-Peff

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Kristoffer Haugsbakk]

On Sat, Jan 27, 2024, at 14:25, Sergey Organov wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:

I agree with Sergey.

Let’s suppose I’ve never used git-clean(1) (and I almost never use
it). I read the man page to find out what it’s about. Oh, it removes
files that I haven’t tracked. That sounds dangerous. But I see under
`-n, --dry-run` that I can simulate what it would do:

   “ Don’t actually remove anything, just show what would be done.

Great, this is what I want. So this seems to mean to run `git clean` and
just tell me what would happen. But now I’ve already read that it
requires `--force` in order to do anything. Which means that I don’t
want to just run:

```
git clean --dry-run
```

Since I presume that would give me the “no `--force` provided”
error. Which means that I want to tack on `--force`:

```
git clean --dry-run --force
```

Now I figure that this will run `git clean --force` but switch real
deletion with printing the filenames.[1]

Junio wrote:

> What I find broken is that giving one 'f' and one 'n' in different
> order, i.e. "-f -n" and "-n -f", does not do what I expect.  If you
> are choosing between do-it (f) and do-not-do-it (n), you ought to be
> able to rely on the usual last-one-wins rule.  That I find broken.

Now suppose I have noticed that some git(1) commands have these
`--[no-]do-it` options. I know that I can leverage this to override a
previous option. And that is useful when I for example have an alias
with `--do-it` but for this invocation I want `--no-do-it`. I read about
`--force` here but see that there is no `--no-force`. I then assume that
the only things that have to do with `--force` or not is that option and
the `requireForce` configuration variable.

I’ve also seen `--force` in other git(1) commands. And they usually are
about some specific scenario rather than the whole command itself, since
e.g. committing one too many times doesn’t really hurt. But I understand
how `--force` applies to all the useful work that git-clean(1) does
because all the useful work is also destructive work. So this is what I
expect from these options in general:

1. `--force`: require for the subset of actions that are potentially
   dangerous or may be unwanted in some way
2. `--dry-run`: simulate the action (specifically print everything that
   would happen but don’t do anything to `.git`, to untracked files, or
   anything else)

And I expect these two to be orthogonal. Because I might want—if the
option is there—to simulate some `--force` (e.g. `git push --force`)
with a `--dry-run`. As in: what would be printed? I wouldn’t expect
`--force` to override `--dry-run`.

† 1: I’m never this careful in real life. But this is about deleting
   files without any (from Git) recovery so I guess some prudence is
   required in this case.

-- 
Kristoffer Haugsbakk

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 29, 2024 at 12:35:49PM +0300, Sergey Organov wrote:
>
>> >> I'm still arguing in favor of fixing "-n", and I believe a fix is needed
>> >> independently from decision about "-f -f".
>> >
>> > Even though I do not personally like it, I do not think "which
>> > between do-it (f) and do-not-do-it (n) do you want to use?" is
>> > broken.  It sometimes irritates me to find "git clean" (without "-f"
>> > or "-n", and with clean.requireForce not disabled) complain, and I
>> > personally think "git clean" when clean.requireForce is in effect
>> > and no "-n" or "-f" were given should pretend as if "-n" were given.
>> 
>> As a note, I'd consider to get rid of 'clean.requireForce' anyway, as
>> its default value provides safe reasonably behaving environment, and I
>> fail to see why anybody would need to set it to 'false'.
>
> Please don't. I set it to "false", because I find the default behavior a
> pointless roadblock if you are already aware that "git clean" can be
> destructive. Surely I can't be the only one.

Well, provided there is at least one person who finds it useful to set
it to 'false', I withdraw my suggestion.

That said, did you consider to:

  $ git config --global alias.cl 'clean -f'

instead of

  $ git config --global clean.requireForce false

I wonder?

Thanks,
-- Sergey Organov

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Jeff King]

On Tue, Jan 30, 2024 at 12:49:08AM +0300, Sergey Organov wrote:

> > Please don't. I set it to "false", because I find the default behavior a
> > pointless roadblock if you are already aware that "git clean" can be
> > destructive. Surely I can't be the only one.
> 
> Well, provided there is at least one person who finds it useful to set
> it to 'false', I withdraw my suggestion.
> 
> That said, did you consider to:
> 
>   $ git config --global alias.cl 'clean -f'
> 
> instead of
> 
>   $ git config --global clean.requireForce false
> 
> I wonder?

Not really, as when I originally set the config in 2007, it was just
undoing the then-recent change to default clean.requireForce to true. I
already had muscle memory using "git clean" as it had worked
historically from 2005-2007.

I know that isn't necessarily relevant for new users today, but my point
is mostly that we have clean.requireForce already and people would
probably be annoyed if we took it away. :)

-Peff

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I know that isn't necessarily relevant for new users today, but my point
> is mostly that we have clean.requireForce already and people would
> probably be annoyed if we took it away. :)

Sounds quite sane and sensible position.

My favourite question Git Rev News may ask their interviewee is "if
there were no existing users to worry about, what would you change
in Git?".  I have many things in my mind I would change if we could,
but they remain only in my fantasy, because we have to care, and I
have to fight for, those existing users who are silent majority,
simply due to the age of the tool.

Re: what should "git clean -n -f [-d] [-x] <pattern>" do?

[Sergey Organov]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> I'm still arguing in favor of fixing "-n", and I believe a fix is needed
>>> independently from decision about "-f -f".
>>
>> Even though I do not personally like it, I do not think "which
>> between do-it (f) and do-not-do-it (n) do you want to use?" is
>> broken.
>
> Well, you are right, but "-n" is not documented as "do-not-do-it" in the
> sense you use it here. 
>
>> It sometimes irritates me to find "git clean" (without "-f"
>> or "-n", and with clean.requireForce not disabled) complain, and I
>> personally think "git clean" when clean.requireForce is in effect
>> and no "-n" or "-f" were given should pretend as if "-n" were given.
>> I wish if it were "without -n or -f, we pretend as if -n were given,
>> possibly with a warning that says 'you need -f if you actually want
>> to carry out these operations'".
>
> Yep, then we'd not need "-n" that much, only if to cancel explicit "-f"
> (provided "-f -f" feature is removed.)
>
>>
>> But that is a separate usability issue.
>
> Yep, and that'd be very different design. 
>
>>
>> What I find broken is that giving one 'f' and one 'n' in different
>> order, i.e. "-f -n" and "-n -f", does not do what I expect.  If you
>> are choosing between do-it (f) and do-not-do-it (n), you ought to be
>> able to rely on the usual last-one-wins rule.  That I find broken.
>
> I fail to see where this expectation comes from, provided "-n" is not
> documented as anything opposed to "-f":
>
>        -n, --dry-run
>            Don’t actually remove anything, just show what would be done.
>
> This is typical convenient description of "dry run", and current "-n"
> implementation is rather close to the description, that I'd still
> rewrite to emphasize the primary goal of the --dry-run:
>
>
> With these descriptions, the last thing that I'd expect is "-n -f"
> removing my files.
>
> Overall, as I see it, we have buggy implementation of suitably
> documented "--dry-run" option, and the best course is to fix the
> bug, with no semantic changes to the option itself.

OTOH, to preserve current actual behavior as much as possible, we can
probably first fix documentation like this:

        -n, --dry-run
            Show what would be done, and don’t actually remove anything.
            This sets 'clean.requireForce' to 'false' for the duration
            of this command execution.

that to me looks like a match for current observable behavior.

Then we can fix '-n' implementation exactly according to this updated
specification, making '-n' really independent from '-f', yet keeping
pure "git clean -n" as well as "git clean -f -n", and "git clean -n -f"
backward compatible.

As a bonus, the above solution will also free our hands in [re]defining
'-f -f' later, if needed.

WDYT?

Thanks,
-- Sergey Organov

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

[Sergey Organov]

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.

So, first, document this in the manual, and then modify implementation
to make this more explicit in the code.

Improved implementation also stops to share single internal variable
'force' between command-line -f option and configuration variable
clean.requireForce, resulting in more clear logic.

The error messages now do not mention -n as well, as it seems
unnecessary and does not reflect clarified implementation.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/git-clean.txt |  2 ++
 builtin/clean.c             | 26 +++++++++++++-------------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 69331e3f05a1..662eebb85207 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -49,6 +49,8 @@ OPTIONS
 -n::
 --dry-run::
 	Don't actually remove anything, just show what would be done.
+	Configuration variable clean.requireForce is ignored, as
+	nothing will be deleted anyway.
 
 -q::
 --quiet::
diff --git a/builtin/clean.c b/builtin/clean.c
index d90766cad3a0..fcc50d08ee9b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -25,7 +25,7 @@
 #include "help.h"
 #include "prompt.h"
 
-static int force = -1; /* unset */
+static int require_force = -1; /* unset */
 static int interactive;
 static struct string_list del_list = STRING_LIST_INIT_DUP;
 static unsigned int colopts;
@@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "clean.requireforce")) {
-		force = !git_config_bool(var, value);
+		require_force = git_config_bool(var, value);
 		return 0;
 	}
 
@@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 {
 	int i, res;
 	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
-	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
+	int ignored_only = 0, force = 0, errors = 0, gone = 1;
 	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
 	struct strbuf abs_path = STRBUF_INIT;
 	struct dir_struct dir = DIR_INIT;
@@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_clean_config, NULL);
-	if (force < 0)
-		force = 0;
-	else
-		config_set = 1;
 
 	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
 			     0);
 
-	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;
+
+	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
-- 
2.25.1

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

[Jean-Noël Avila]

Putting my documentation/translator hat:

Le 29/02/2024 à 20:07, Sergey Organov a écrit :
> 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.
> 
> So, first, document this in the manual, and then modify implementation
> to make this more explicit in the code.
> 
> Improved implementation also stops to share single internal variable
> 'force' between command-line -f option and configuration variable
> clean.requireForce, resulting in more clear logic.
> 
> The error messages now do not mention -n as well, as it seems
> unnecessary and does not reflect clarified implementation.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  Documentation/git-clean.txt |  2 ++
>  builtin/clean.c             | 26 +++++++++++++-------------
>  2 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
> index 69331e3f05a1..662eebb85207 100644
> --- a/Documentation/git-clean.txt
> +++ b/Documentation/git-clean.txt
> @@ -49,6 +49,8 @@ OPTIONS
>  -n::
>  --dry-run::
>  	Don't actually remove anything, just show what would be done.
> +	Configuration variable clean.requireForce is ignored, as
> +	nothing will be deleted anyway.

Please use backticks for options, configuration and environment names:
`clean.requireForce`
>  
>  -q::
>  --quiet::
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d90766cad3a0..fcc50d08ee9b 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -25,7 +25,7 @@
>  #include "help.h"
>  #include "prompt.h"
>  
> -static int force = -1; /* unset */
> +static int require_force = -1; /* unset */
>  static int interactive;
>  static struct string_list del_list = STRING_LIST_INIT_DUP;
>  static unsigned int colopts;
> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
>  	}
>  
>  	if (!strcmp(var, "clean.requireforce")) {
> -		force = !git_config_bool(var, value);
> +		require_force = git_config_bool(var, value);
>  		return 0;
>  	}
>  
> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  {
>  	int i, res;
>  	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
> -	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
> +	int ignored_only = 0, force = 0, errors = 0, gone = 1;
>  	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
>  	struct strbuf abs_path = STRBUF_INIT;
>  	struct dir_struct dir = DIR_INIT;
> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	};
>  
>  	git_config(git_clean_config, NULL);
> -	if (force < 0)
> -		force = 0;
> -	else
> -		config_set = 1;
>  
>  	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>  			     0);
>  
> -	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;
> +
> +	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"));
>  	}
>  

The last two cases can be coalesced into a single case (the last one),
because the difference in the messages does not bring more information
to the user.



>  	if (force > 1)
> 
> base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d

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

[Sergey Organov]

Jean-Noël Avila <avila.jn@gmail.com> writes:

> Putting my documentation/translator hat:
>
> Le 29/02/2024 à 20:07, Sergey Organov a écrit :
>> 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.
>> 
>> So, first, document this in the manual, and then modify implementation
>> to make this more explicit in the code.
>> 
>> Improved implementation also stops to share single internal variable
>> 'force' between command-line -f option and configuration variable
>> clean.requireForce, resulting in more clear logic.
>> 
>> The error messages now do not mention -n as well, as it seems
>> unnecessary and does not reflect clarified implementation.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  Documentation/git-clean.txt |  2 ++
>>  builtin/clean.c             | 26 +++++++++++++-------------
>>  2 files changed, 15 insertions(+), 13 deletions(-)
>> 
>> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
>> index 69331e3f05a1..662eebb85207 100644
>> --- a/Documentation/git-clean.txt
>> +++ b/Documentation/git-clean.txt
>> @@ -49,6 +49,8 @@ OPTIONS
>>  -n::
>>  --dry-run::
>>  	Don't actually remove anything, just show what would be done.
>> +	Configuration variable clean.requireForce is ignored, as
>> +	nothing will be deleted anyway.
>
> Please use backticks for options, configuration and environment names:
> `clean.requireForce`

I did consider this. However, existing text already has exactly this one
unquoted, so I just did the same. Hopefully it will be fixed altogether
later, or are you positive I better resend the patch with quotes? 

>>  
>>  -q::
>>  --quiet::
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index d90766cad3a0..fcc50d08ee9b 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -25,7 +25,7 @@
>>  #include "help.h"
>>  #include "prompt.h"
>>  
>> -static int force = -1; /* unset */
>> +static int require_force = -1; /* unset */
>>  static int interactive;
>>  static struct string_list del_list = STRING_LIST_INIT_DUP;
>>  static unsigned int colopts;
>> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
>>  	}
>>  
>>  	if (!strcmp(var, "clean.requireforce")) {
>> -		force = !git_config_bool(var, value);
>> +		require_force = git_config_bool(var, value);
>>  		return 0;
>>  	}
>>  
>> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>>  {
>>  	int i, res;
>>  	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
>> -	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
>> +	int ignored_only = 0, force = 0, errors = 0, gone = 1;
>>  	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
>>  	struct strbuf abs_path = STRBUF_INIT;
>>  	struct dir_struct dir = DIR_INIT;
>> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>>  	};
>>  
>>  	git_config(git_clean_config, NULL);
>> -	if (force < 0)
>> -		force = 0;
>> -	else
>> -		config_set = 1;
>>  
>>  	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>>  			     0);
>>  
>> -	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;
>> +
>> +	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"));
>>  	}
>>  
>
> The last two cases can be coalesced into a single case (the last one),
> because the difference in the messages does not bring more information
> to the user.

Did you misread the patch? There are only 2 cases here, the last (third)
one is marked with '-' (removed). Too easy to misread this, I'd say. New
code is:

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

and is basically unchanged from the original, except reference to '-n' has been
removed. Btw, is now comma needed after -f, and isn't it better to
substitute ':' for ';'?

Thank you for review!

-- Sergey Organov

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

[Kristoffer Haugsbakk]

On Fri, Mar 1, 2024, at 15:34, Sergey Organov wrote:
>> Please use backticks for options, configuration and environment names:
>> `clean.requireForce`
>
> I did consider this. However, existing text already has exactly this one
> unquoted, so I just did the same. Hopefully it will be fixed altogether
> later, or are you positive I better resend the patch with quotes?

Sometimes I see widespread changes (like formatting many files) get
rejected because it is considered _churn_. Not fixing this in this
series and then maybe someone else fixing it later seems like churn as
well. Isn’t it better to fix it while you are changing the text?

-- 
Kristoffer Haugsbakk

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

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> On Fri, Mar 1, 2024, at 15:34, Sergey Organov wrote:
>>> Please use backticks for options, configuration and environment names:
>>> `clean.requireForce`
>>
>> I did consider this. However, existing text already has exactly this one
>> unquoted, so I just did the same. Hopefully it will be fixed altogether
>> later, or are you positive I better resend the patch with quotes?
>
> Sometimes I see widespread changes (like formatting many files) get
> rejected because it is considered _churn_. Not fixing this in this
> series and then maybe someone else fixing it later seems like churn as
> well. Isn’t it better to fix it while you are changing the text?

Any one of these is fine:

 (1) add the new paragraph with mark-up consistent with existing
     text (which is what Sergey did).

 (2) add the new paragraph with correct mark-up, making the document
     less consistent overall.

 (3) have one patch to fix broken mark-up of existing text, followed
     by another patch to add the new paragraph with correct mark-up.

If you take one of the first two, it would be a very good idea to
have a comment in the proposed log message to note the need for
later clean-up.  Without being written down anywhere, your discovery
and the brain cycles you spent while deciding what to do will be
wasted, which is not what you want.

Thanks, both.

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

[Junio C Hamano]

Jean-Noël Avila <avila.jn@gmail.com> writes:

>> +	/* Dry run won't remove anything, so requiring force makes no sense */
>> +	if(dry_run)
>> +		require_force = 0;

Style.  "if (dry_run)".

Getting rid of "config_set", which was an extra variable that kept
track of where "force" came from, does make the logic cleaner, I
guess.  What we want to happen is that one of -i/-n/-f is required
when clean.requireForce is *not* unset (i.e. 0 <= require_force).

>> +	if (!force && !interactive) {

The require-force takes effect only when neither force or
interactive is given, so the new code structure puts the above
obvious conditional around "do we complain due to requireForce?"
logic.  Sensible.

>> +		if (require_force > 0)
>> +			die(_("clean.requireForce set to true and neither -f, nor -i given; "
>> +				  "refusing to clean"));

If it is explicitly set, we get this message.  And ...

>> +		else if (require_force < 0)
>> +			die(_("clean.requireForce defaults to true and neither -f, nor -i given; "
>>  				  "refusing to clean"));

... if it is set due to default (in other words, if it is not unset), we
get this message.

As you said, I do not think it matters too much either way to the
end-users where the truth setting of clean.requireForce came from,
either due to the default or the user explicitly configuring.  So
unifying to a single message may be helpful to both readers and
translators.

	clean.requireForce is true; unless interactive, -f is required

might be a bit shorter and more to the point.

> The last two cases can be coalesced into a single case (the last one),
> because the difference in the messages does not bring more information
> to the user.

Yeah.

Thanks.

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

[Junio C Hamano]

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

> Getting rid of "config_set", which was an extra variable that kept
> track of where "force" came from, does make the logic cleaner, I
> guess.  What we want to happen is that one of -i/-n/-f is required
> when clean.requireForce is *not* unset (i.e. 0 <= require_force).

Oh, noes.  require_force is unset explicitly it would be 0, so this
should have read (i.e. require_force != 0).  Sorry for a thinko.

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

[Sergey Organov]

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

> Jean-Noël Avila <avila.jn@gmail.com> writes:
>
>>> +	/* Dry run won't remove anything, so requiring force makes no sense */
>>> +	if(dry_run)
>>> +		require_force = 0;
>
> Style.  "if (dry_run)".

Ooops!

>
> Getting rid of "config_set", which was an extra variable that kept
> track of where "force" came from, does make the logic cleaner, I
> guess.  What we want to happen is that one of -i/-n/-f is required
> when clean.requireForce is *not* unset (i.e. 0 <= require_force).
>
>>> +	if (!force && !interactive) {
>
> The require-force takes effect only when neither force or
> interactive is given, so the new code structure puts the above
> obvious conditional around "do we complain due to requireForce?"
> logic.  Sensible.
>
>>> +		if (require_force > 0)
>>> +			die(_("clean.requireForce set to true and neither -f, nor -i given; "
>>> +				  "refusing to clean"));
>
> If it is explicitly set, we get this message.  And ...
>
>>> +		else if (require_force < 0)
>>> +			die(_("clean.requireForce defaults to true and neither -f, nor -i given; "
>>>  				  "refusing to clean"));
>
> ... if it is set due to default (in other words, if it is not unset), we
> get this message.
>
> As you said, I do not think it matters too much either way to the
> end-users where the truth setting of clean.requireForce came from,
> either due to the default or the user explicitly configuring.  So
> unifying to a single message may be helpful to both readers and
> translators.
>
> 	clean.requireForce is true; unless interactive, -f is required
>
> might be a bit shorter and more to the point.

Dunno, I tried to keep changes to the bare sensible minimum, especially
to avoid possible controversy. I'd leave this for somebody else to
decide upon and patch, if they feel like it.

>> The last two cases can be coalesced into a single case (the last one),
>> because the difference in the messages does not bring more information
>> to the user.
>
> Yeah.

"The last two cases" sounds like there are more of them, and there is
none, so to me it sounded like patch misread. Maybe I'm wrong.

Thanks for the review!

-- Sergey Organov

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

[Junio C Hamano]

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

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

[Jean-Noël AVILA]

On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote:
> Jean-Noël Avila <avila.jn@gmail.com> writes:
> 
> > Putting my documentation/translator hat:
> >
> > Le 29/02/2024 à 20:07, Sergey Organov a écrit :
> >> 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.
> >> 
> >> So, first, document this in the manual, and then modify implementation
> >> to make this more explicit in the code.
> >> 
> >> Improved implementation also stops to share single internal variable
> >> 'force' between command-line -f option and configuration variable
> >> clean.requireForce, resulting in more clear logic.
> >> 
> >> The error messages now do not mention -n as well, as it seems
> >> unnecessary and does not reflect clarified implementation.
> >> 
> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >> ---
> >>  Documentation/git-clean.txt |  2 ++
> >>  builtin/clean.c             | 26 +++++++++++++-------------
> >>  2 files changed, 15 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
> >> index 69331e3f05a1..662eebb85207 100644
> >> --- a/Documentation/git-clean.txt
> >> +++ b/Documentation/git-clean.txt
> >> @@ -49,6 +49,8 @@ OPTIONS
> >>  -n::
> >>  --dry-run::
> >>  	Don't actually remove anything, just show what would be done.
> >> +	Configuration variable clean.requireForce is ignored, as
> >> +	nothing will be deleted anyway.
> >
> > Please use backticks for options, configuration and environment names:
> > `clean.requireForce`
> 
> I did consider this. However, existing text already has exactly this one
> unquoted, so I just did the same. Hopefully it will be fixed altogether
> later, or are you positive I better resend the patch with quotes? 
> 
> >>  
> >>  -q::
> >>  --quiet::
> >> diff --git a/builtin/clean.c b/builtin/clean.c
> >> index d90766cad3a0..fcc50d08ee9b 100644
> >> --- a/builtin/clean.c
> >> +++ b/builtin/clean.c
> >> @@ -25,7 +25,7 @@
> >>  #include "help.h"
> >>  #include "prompt.h"
> >>  
> >> -static int force = -1; /* unset */
> >> +static int require_force = -1; /* unset */
> >>  static int interactive;
> >>  static struct string_list del_list = STRING_LIST_INIT_DUP;
> >>  static unsigned int colopts;
> >> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const 
char *value,
> >>  	}
> >>  
> >>  	if (!strcmp(var, "clean.requireforce")) {
> >> -		force = !git_config_bool(var, value);
> >> +		require_force = git_config_bool(var, value);
> >>  		return 0;
> >>  	}
> >>  
> >> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
> >>  {
> >>  	int i, res;
> >>  	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
> >> -	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
> >> +	int ignored_only = 0, force = 0, errors = 0, gone = 1;
> >>  	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
> >>  	struct strbuf abs_path = STRBUF_INIT;
> >>  	struct dir_struct dir = DIR_INIT;
> >> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const 
char *prefix)
> >>  	};
> >>  
> >>  	git_config(git_clean_config, NULL);
> >> -	if (force < 0)
> >> -		force = 0;
> >> -	else
> >> -		config_set = 1;
> >>  
> >>  	argc = parse_options(argc, argv, prefix, options, 
builtin_clean_usage,
> >>  			     0);
> >>  
> >> -	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;
> >> +
> >> +	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"));
> >>  	}
> >>  
> >
> > The last two cases can be coalesced into a single case (the last one),
> > because the difference in the messages does not bring more information
> > to the user.
> 
> Did you misread the patch? There are only 2 cases here, the last (third)
> one is marked with '-' (removed). Too easy to misread this, I'd say. New
> code is:
> 
> 		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; "
> 
> and is basically unchanged from the original, except reference to '-n' has 
been
> removed. Btw, is now comma needed after -f, and isn't it better to
> substitute ':' for ';'?
> 
> Thank you for review!
> 
> -- Sergey Organov
> 
> 

Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that specifying 
that this is the default or not is really useful. If the configuration was set 
to true, it is was a no-op. If set to false, no message will appear.

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

[Sergey Organov]

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

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

[Sergey Organov]

Jean-Noël AVILA <avila.jn@gmail.com> writes:

> On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote:
>> Jean-Noël Avila <avila.jn@gmail.com> writes:
>> 
>> > Putting my documentation/translator hat:
>> >
>> > Le 29/02/2024 à 20:07, Sergey Organov a écrit :
>> >> 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.
>> >> 
>> >> So, first, document this in the manual, and then modify implementation
>> >> to make this more explicit in the code.
>> >> 
>> >> Improved implementation also stops to share single internal variable
>> >> 'force' between command-line -f option and configuration variable
>> >> clean.requireForce, resulting in more clear logic.
>> >> 
>> >> The error messages now do not mention -n as well, as it seems
>> >> unnecessary and does not reflect clarified implementation.
>> >> 
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> ---
>> >>  Documentation/git-clean.txt |  2 ++
>> >>  builtin/clean.c             | 26 +++++++++++++-------------
>> >>  2 files changed, 15 insertions(+), 13 deletions(-)
>> >> 
>> >> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
>> >> index 69331e3f05a1..662eebb85207 100644
>> >> --- a/Documentation/git-clean.txt
>> >> +++ b/Documentation/git-clean.txt
>> >> @@ -49,6 +49,8 @@ OPTIONS
>> >>  -n::
>> >>  --dry-run::
>> >>  	Don't actually remove anything, just show what would be done.
>> >> +	Configuration variable clean.requireForce is ignored, as
>> >> +	nothing will be deleted anyway.
>> >
>> > Please use backticks for options, configuration and environment names:
>> > `clean.requireForce`
>> 
>> I did consider this. However, existing text already has exactly this one
>> unquoted, so I just did the same. Hopefully it will be fixed altogether
>> later, or are you positive I better resend the patch with quotes? 
>> 
>> >>  
>> >>  -q::
>> >>  --quiet::
>> >> diff --git a/builtin/clean.c b/builtin/clean.c
>> >> index d90766cad3a0..fcc50d08ee9b 100644
>> >> --- a/builtin/clean.c
>> >> +++ b/builtin/clean.c
>> >> @@ -25,7 +25,7 @@
>> >>  #include "help.h"
>> >>  #include "prompt.h"
>> >>  
>> >> -static int force = -1; /* unset */
>> >> +static int require_force = -1; /* unset */
>> >>  static int interactive;
>> >>  static struct string_list del_list = STRING_LIST_INIT_DUP;
>> >>  static unsigned int colopts;
>> >> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const 
> char *value,
>> >>  	}
>> >>  
>> >>  	if (!strcmp(var, "clean.requireforce")) {
>> >> -		force = !git_config_bool(var, value);
>> >> +		require_force = git_config_bool(var, value);
>> >>  		return 0;
>> >>  	}
>> >>  
>> >> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char 
> *prefix)
>> >>  {
>> >>  	int i, res;
>> >>  	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
>> >> -	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
>> >> +	int ignored_only = 0, force = 0, errors = 0, gone = 1;
>> >>  	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
>> >>  	struct strbuf abs_path = STRBUF_INIT;
>> >>  	struct dir_struct dir = DIR_INIT;
>> >> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const 
> char *prefix)
>> >>  	};
>> >>  
>> >>  	git_config(git_clean_config, NULL);
>> >> -	if (force < 0)
>> >> -		force = 0;
>> >> -	else
>> >> -		config_set = 1;
>> >>  
>> >>  	argc = parse_options(argc, argv, prefix, options, 
> builtin_clean_usage,
>> >>  			     0);
>> >>  
>> >> -	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;
>> >> +
>> >> +	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"));
>> >>  	}
>> >>  
>> >
>> > The last two cases can be coalesced into a single case (the last one),
>> > because the difference in the messages does not bring more information
>> > to the user.
>> 
>> Did you misread the patch? There are only 2 cases here, the last (third)
>> one is marked with '-' (removed). Too easy to misread this, I'd say. New
>> code is:
>> 
>> 		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; "
>> 
>> and is basically unchanged from the original, except reference to '-n' has 
> been
>> removed. Btw, is now comma needed after -f, and isn't it better to
>> substitute ':' for ';'?
>> 
>> Thank you for review!
>> 
>> -- Sergey Organov
>> 
>> 
>
> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
> specifying that this is the default or not is really useful. If the
> configuration was set to true, it is was a no-op. If set to false, no
> message will appear.

I'm not sure either, and as it's not the topic of this particular patch,
I'd like to delegate the decision on the issue.

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

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
>> specifying that this is the default or not is really useful. If the
>> configuration was set to true, it is was a no-op. If set to false, no
>> message will appear.
>
> I'm not sure either, and as it's not the topic of this particular patch,
> I'd like to delegate the decision on the issue.

It is very much spot on the topic of simplifying and clarifying the
code to unify these remaining two messages into a single one.

And involving the --interactive that allows users a chance to
rethink and refrain from removing some to the equation would also be
worth doing in the same topic, even though it might not fit your
immediate agenda of crusade against --dry-run.

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

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
>>> specifying that this is the default or not is really useful. If the
>>> configuration was set to true, it is was a no-op. If set to false, no
>>> message will appear.
>>
>> I'm not sure either, and as it's not the topic of this particular patch,
>> I'd like to delegate the decision on the issue.
>
> It is very much spot on the topic of simplifying and clarifying the
> code to unify these remaining two messages into a single one.

I'm inclined to be more against merging than for it, as for me it'd be
confusing to be told that a configuration variable is set to true when I
didn't set it, nor there is any way to figure where it is set, because
in fact it isn't, and it's rather the default that is in use.

Overall, to me the messages are fine as they are (except -n that doesn't
belong there), I don't see compelling reason to hide information from
the user, and thus I won't propose patch that gets rid of one of them.

> And involving the --interactive that allows users a chance to
> rethink and refrain from removing some to the equation would also be
> worth doing in the same topic,

Worth doing what? I'm afraid I lost the plot here, as --interactive
still looks fine to me.

> even though it might not fit your immediate agenda of crusade against
> --dry-run.

I'm hopefully crusading for --dry-run, not against, trying to get rid of
the cause of the original confusion that started -n/-f controversy.

Thanks,
-- Sergey Organov

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

[Sergey Organov]

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.

So, first, document this in the manual, and then modify implementation
to make this more explicit in the code.

Improved implementation also stops to share single internal variable
'force' between command-line -f option and configuration variable
clean.requireForce, resulting in more clear logic.

Two error messages with slightly different text depending on if
clean.requireForce was explicitly set or not, are merged into a single
one.

The resulting error message now does not mention -n as well, as it
neither matches intended clean.requireForce usage nor reflects
clarified implementation.

Documentation of clean.requireForce is changed accordingly.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---

Changes since v1:

 * Fixed style of the if() statement

 * Merged two error messages into one

 * clean.requireForce description changed accordingly

 Documentation/config/clean.txt |  4 ++--
 Documentation/git-clean.txt    |  2 ++
 builtin/clean.c                | 23 +++++++++--------------
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/Documentation/config/clean.txt b/Documentation/config/clean.txt
index f05b9403b5ad..b19ca210f39b 100644
--- a/Documentation/config/clean.txt
+++ b/Documentation/config/clean.txt
@@ -1,3 +1,3 @@
 clean.requireForce::
-	A boolean to make git-clean do nothing unless given -f,
-	-i, or -n.  Defaults to true.
+	A boolean to make git-clean refuse to delete files unless -f
+	or -i is given. Defaults to true.
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 69331e3f05a1..662eebb85207 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -49,6 +49,8 @@ OPTIONS
 -n::
 --dry-run::
 	Don't actually remove anything, just show what would be done.
+	Configuration variable clean.requireForce is ignored, as
+	nothing will be deleted anyway.
 
 -q::
 --quiet::
diff --git a/builtin/clean.c b/builtin/clean.c
index d90766cad3a0..41502dcb0dde 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -25,7 +25,7 @@
 #include "help.h"
 #include "prompt.h"
 
-static int force = -1; /* unset */
+static int require_force = -1; /* unset */
 static int interactive;
 static struct string_list del_list = STRING_LIST_INIT_DUP;
 static unsigned int colopts;
@@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "clean.requireforce")) {
-		force = !git_config_bool(var, value);
+		require_force = git_config_bool(var, value);
 		return 0;
 	}
 
@@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 {
 	int i, res;
 	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
-	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
+	int ignored_only = 0, force = 0, errors = 0, gone = 1;
 	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
 	struct strbuf abs_path = STRBUF_INIT;
 	struct dir_struct dir = DIR_INIT;
@@ -946,22 +946,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_clean_config, NULL);
-	if (force < 0)
-		force = 0;
-	else
-		config_set = 1;
 
 	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
 			     0);
 
-	if (!interactive && !dry_run && !force) {
-		if (config_set)
-			die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
-				  "refusing to clean"));
-		else
-			die(_("clean.requireForce defaults 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;
+
+	if (require_force != 0 && !force && !interactive)
+		die(_("clean.requireForce is true and neither -f nor -i given:"
 				  " refusing to clean"));
-	}
 
 	if (force > 1)
 		rm_flags = 0;

base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
-- 
2.25.1

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

[Sergey Organov]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
>>>> specifying that this is the default or not is really useful. If the
>>>> configuration was set to true, it is was a no-op. If set to false, no
>>>> message will appear.
>>>
>>> I'm not sure either, and as it's not the topic of this particular patch,
>>> I'd like to delegate the decision on the issue.
>>
>> It is very much spot on the topic of simplifying and clarifying the
>> code to unify these remaining two messages into a single one.
>
> I'm inclined to be more against merging than for it, as for me it'd be
> confusing to be told that a configuration variable is set to true when I
> didn't set it, nor there is any way to figure where it is set, because
> in fact it isn't, and it's rather the default that is in use.
>
> Overall, to me the messages are fine as they are (except -n that doesn't
> belong there), I don't see compelling reason to hide information from
> the user, and thus I won't propose patch that gets rid of one of them.

Nevertheless, as others are in favor of unification, I've merged these
two messages in the v2 version of the patch, which see.

Thanks,
-- Sergey Organov