threads / discuss / 60711
Subject: what should "git clean -n -f [-d] [-x] <pattern>" do?
Junio C HamanoI 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.
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Elijah NewrenRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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).
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Junio C HamanoRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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-tooand you can substitute "-n" with "-f" to actually perform it?
Anybody care to come up with a sensible migration plan?
Thanks.
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Junio C HamanoRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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.
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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.
Junio C HamanoRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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.
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Junio C HamanoRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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.
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Junio C HamanoRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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.
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Kristoffer HaugsbakkRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Jeff KingRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Sergey OrganovRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Jeff KingRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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
Junio C HamanoRe: what should "git clean -n -f [-d] [-x] <pattern>" do?
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.
Sergey Organov[PATCH] clean: improve -n and -f implementation and documentation
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
Jean-Noël AvilaRe: [PATCH] clean: improve -n and -f implementation and documentation
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
Sergey OrganovRe: [PATCH] clean: improve -n and -f implementation and documentation
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
Kristoffer HaugsbakkRe: [PATCH] clean: improve -n and -f implementation and documentation
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
Junio C HamanoRe: [PATCH] clean: improve -n and -f implementation and documentation
"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.
Jean-Noël AVILARe: [PATCH] clean: improve -n and -f implementation and documentation
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.
Sergey OrganovRe: [PATCH] clean: improve -n and -f implementation and documentation
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.
Junio C HamanoRe: [PATCH] clean: improve -n and -f implementation and documentation
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.
Sergey OrganovRe: [PATCH] clean: improve -n and -f implementation and documentation
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
Sergey OrganovRe: [PATCH] clean: improve -n and -f implementation and documentation
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
Junio C HamanoRe: [PATCH] clean: improve -n and -f implementation and documentation
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.
Junio C HamanoRe: [PATCH] clean: improve -n and -f implementation and documentation
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.
Sergey OrganovRe: [PATCH] clean: improve -n and -f implementation and documentation
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
Junio C HamanoRe: [PATCH] clean: improve -n and -f implementation and documentation
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: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1dSergey OrganovRe: [PATCH] clean: improve -n and -f implementation and documentation
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
Sergey Organov[PATCH v2] clean: improve -n and -f implementation and documentation
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