{"thread":{"id":"60711","subject":"what should \"git clean -n -f [-d] [-x] \" do?","startedAt":"2024-01-09T20:20:51Z","lastAt":"2024-03-03T09:54:07Z","messageCount":38,"participants":["Junio C Hamano","Sergey Organov","Elijah Newren","Jeff King","Kristoffer Haugsbakk","Jean-Noël Avila","Jean-Noël AVILA"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"486471","messageId":"xmqq34v6gswv.fsf@gitster.g","threadId":"60711","inReplyTo":null,"subject":"what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T20:20:48Z","receivedAt":"2024-01-09T20:20:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I think the current code makes \"-n\" take precedence, and ignores\n\"-f\". Shouldn't it either\n\n (1) error out with \"-n and -f cannot be used together\", or\n (2) let \"-n\" and \"-f\" follow the usual \"last one wins\" rule?\n\nThe latter may be logically cleaner but it is a change that breaks\nbackward compatibility big time in a more dangerous direction, so it\nmay not be desirable in practice, with too big a downside for a too\nlittle gain.\n"},{"id":"486475","messageId":"877ckitb7m.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqq34v6gswv.fsf@gitster.g","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-09T22:04:45Z","receivedAt":"2024-01-09T22:04:50Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> I think the current code makes \"-n\" take precedence, and ignores\n> \"-f\".\n\nTo me it rather looks more like \"-n\" implies \"-f\", but then there is\n\"second -f\" rule that makes things even more interesting:\n\n \"Git will refuse to modify untracked nested git repositories\n (directories with a .git subdirectory) unless a second -f is given.\"\n\nHow do I figure what files will be deleted on\n\n git clean -f -f\n\nwhen \"-n\" behaves as you (or me) described? I.e., what\n\n git clean -f -f -n\n\nand\n\n git clean -f -n\n\nwill output?\n\n>\n> Shouldn't it either\n>\n> (1) error out with \"-n and -f cannot be used together\", or\n> (2) let \"-n\" and \"-f\" follow the usual \"last one wins\" rule?\n>\n> The latter may be logically cleaner but it is a change that breaks\n> backward compatibility big time in a more dangerous direction, so it\n> may not be desirable in practice, with too big a downside for a too\n> little gain.\n\nI agree (2) is too dangerous and surprising, and (1) is limiting: I\nbelieve the user should be able to see what will be done on\n\n git clean -f -f\n\nby simply adding \"-n\" to the command-line.\n\nSo I figure I'd rather prefer yet another option:\n\n(3) -n dry run: show what will be done once \"-n\" is removed.\n\nThis way, e.g.,\n\n git clean\n\nand\n\n git clean -n\n\nwill produce exactly the same output with default configuration:\n\n fatal: clean.requireForce defaults to true and neither -i, nor -f given; refusing to clean\n\nand one will need to say, e.g.:\n\n git clean -n -f\n\nto get the list of files to be deleted with \"git clean -f\".\n\nWith (3) \"-n\" becomes orthogonal to \"-f\", resulting in predictable and\nuseful behavior.\n\nBR,\n-- Sergey Organov\n\n"},{"id":"487033","messageId":"CABPp-BHUVLS4vB5maZzU5gS33ve6LkKgij+rc1bBZges6Xej-g@mail.gmail.com","threadId":"60711","inReplyTo":"xmqq34v6gswv.fsf@gitster.g","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-01-19T02:07:17Z","receivedAt":"2024-01-19T02:07:31Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jan 9, 2024 at 12:21 PM Junio C Hamano wrote:\n>\n> I think the current code makes \"-n\" take precedence, and ignores\n> \"-f\".\n\n:-(\n\n> Shouldn't it either\n>\n> (1) error out with \"-n and -f cannot be used together\", or\n> (2) let \"-n\" and \"-f\" follow the usual \"last one wins\" rule?\n\nI believe so.\n\n> The latter may be logically cleaner but it is a change that breaks\n> backward compatibility big time in a more dangerous direction, so it\n> may not be desirable in practice, with too big a downside for a too\n> little gain.\n\nYeah, I think (1) is the safer option, for now. We could potentially\ndo (1), then wait a long time, then switch to (2).\n"},{"id":"487259","messageId":"87a5ow9jb4.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"CABPp-BHUVLS4vB5maZzU5gS33ve6LkKgij+rc1bBZges6Xej-g@mail.gmail.com","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-23T15:10:55Z","receivedAt":"2024-01-23T15:10:59Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Elijah Newren writes:\n\n> On Tue, Jan 9, 2024 at 12:21 PM Junio C Hamano wrote:\n>>\n>> I think the current code makes \"-n\" take precedence, and ignores\n>> \"-f\".\n>\n> :-(\n>\n>> Shouldn't it either\n>>\n>> (1) error out with \"-n and -f cannot be used together\", or\n>> (2) let \"-n\" and \"-f\" follow the usual \"last one wins\" rule?\n>\n> I believe so.\n\nThen how does one figure what \"git clean -f -f\" will do without actually\ndoing it?\n\nPlease notice that -f -f is special according to the manual:\n\n \"Git will refuse to modify untracked nested git repositories\n (directories with a .git subdirectory) unless a second -f is given.\"\n\nI looks like neither (0), nor (1) nor (2) gives us any useful behavior\nin this case.\n\nI figure the best solution is to rather make -n orthogonal to -f, that\nwill solve the puzzle, and that is what actually expected from a \"dry\nrun\" option: don't change any behavior, except print actions instead of\nperforming them.\n\n-- Sergey Organov\n"},{"id":"487276","messageId":"xmqqsf2nnbkj.fsf@gitster.g","threadId":"60711","inReplyTo":"87a5ow9jb4.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-23T18:34:20Z","receivedAt":"2024-01-23T18:34:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Then how does one figure what \"git clean -f -f\" will do without actually\n> doing it?\n\nI think whoever came up with the bright idea of forcing twice\nsomehow does a totally different thing from forcing once should be\nshot, twice ;-) It does not mesh well with the idea behind the\nclean.requireForce setting to make you explicitly choose either '-f'\nor '-n' to express your intent.\n\nI wonder how feasible is it to deprecate that misfeature introduced\nwith a0f4afbe (clean: require double -f options to nuke nested git\nrepository and work tree, 2009-06-30) and migrate its users (which\nis marked as \"This is rarely what the user wants\") to a new option,\nsay, --nested-repo-too so that the \"dry-run\" version of the\ninvocations become\n\n git clean -n\n git clean -n --nested-repo-too\n\nand you can substitute \"-n\" with \"-f\" to actually perform it?\n\nAnybody care to come up with a sensible migration plan?\n\nThanks.\n"},{"id":"487309","messageId":"87plxr3zsr.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqqsf2nnbkj.fsf@gitster.g","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-24T08:23:32Z","receivedAt":"2024-01-24T08:23:36Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> Then how does one figure what \"git clean -f -f\" will do without actually\n>> doing it?\n>\n> I think whoever came up with the bright idea of forcing twice\n> somehow does a totally different thing from forcing once should be\n> shot, twice ;-) It does not mesh well with the idea behind the\n> clean.requireForce setting to make you explicitly choose either '-f'\n> or '-n' to express your intent.\n\nI agree, yet I see it as another deficiency, in addition to that of -n,\nand I used it as an example to emphasize the deficiency of -n.\n\n> I wonder how feasible is it to deprecate that misfeature introduced\n> with a0f4afbe (clean: require double -f options to nuke nested git\n> repository and work tree, 2009-06-30) and migrate its users (which\n> is marked as \"This is rarely what the user wants\") to a new option,\n> say, --nested-repo-too so that the \"dry-run\" version of the\n> invocations become\n>\n> git clean -n\n> git clean -n --nested-repo-too\n>\n> and you can substitute \"-n\" with \"-f\" to actually perform it?\n\nWhereas obsoleting second -f in favor of new --nested-repo might be a\ngood idea indeed, I believe it's still a mistake for \"dry run\" to\nsomehow interfere with -f, sorry.\n\nThanks,\n-- Sergey Organov\n"},{"id":"487338","messageId":"xmqqa5ouhckj.fsf@gitster.g","threadId":"60711","inReplyTo":"87plxr3zsr.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T17:21:32Z","receivedAt":"2024-01-24T17:21:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Whereas obsoleting second -f in favor of new --nested-repo might be a\n> good idea indeed, I believe it's still a mistake for \"dry run\" to\n> somehow interfere with -f, sorry.\n\nNo need to be sorry ;-)\n\nI actually think the true culprit of making this an odd-man-out is\nthat the use of \"-f\" in \"git clean\", especially with its use of the\nconfiguration variable clean.requireForce that defaults to true, is\nutterly non-standard.\n\nThe usual pattern of defining what \"-f\" does is that the \"git foo\"\ncommand without any options does its common thing but refuses to\nperform undesirable operations (e.g. \"git add .\" adds everything\nbut refrains from adding ignored paths). And \"git foo -f\" is a way\nto also perform what it commonly skips.\n\nIn contrast, with clean.requireForce that defaults to true, \"git\nclean\" does not do anything useful by default. Without such a\nsafety, \"git clean\" would be a way to clean expendable paths, and\n\"git clean -f\" might be to also clean precious paths. But it does\nnot work that way. It always requires \"-f\" to do anything. Worse\nyet, it is not even \"by default it acts as if -n is given and -f is\na way to countermand that implicit -n\". It is \"you must give me\neither -f (i.e. please do work) or -n (i.e. please show what you\nwould do) before I do anything\".\n\n $ git clean\n fatal: clean.requireForce defaults to true and neither -i, -n, nor -f given; refusing to clean\n\nGiven that, it is hard to argue that it would be a natural end-user\nexpectation that the command does something useful (i.e. show what\nwould be done) when it is given \"-f\" and \"-n\" at the same time.\nWhat makes this a rather nonsense UI is the fact that \"-f\" does not\nwork the way we would expect for this command.\n\n\n\n\n\n\n\n"},{"id":"487370","messageId":"87il3h72ym.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqqa5ouhckj.fsf@gitster.g","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-25T17:11:29Z","receivedAt":"2024-01-25T17:11:33Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> Whereas obsoleting second -f in favor of new --nested-repo might be a\n>> good idea indeed, I believe it's still a mistake for \"dry run\" to\n>> somehow interfere with -f, sorry.\n>\n> No need to be sorry ;-)\n>\n> I actually think the true culprit of making this an odd-man-out is\n> that the use of \"-f\" in \"git clean\", especially with its use of the\n> configuration variable clean.requireForce that defaults to true, is\n> utterly non-standard.\n>\n> The usual pattern of defining what \"-f\" does is that the \"git foo\"\n> command without any options does its common thing but refuses to\n> perform undesirable operations (e.g. \"git add .\" adds everything\n> but refrains from adding ignored paths). And \"git foo -f\" is a way\n> to also perform what it commonly skips.\n>\n> In contrast, with clean.requireForce that defaults to true, \"git\n> clean\" does not do anything useful by default. Without such a\n> safety, \"git clean\" would be a way to clean expendable paths, and\n> \"git clean -f\" might be to also clean precious paths. But it does\n> not work that way. It always requires \"-f\" to do anything. Worse\n> yet, it is not even \"by default it acts as if -n is given and -f is\n> a way to countermand that implicit -n\". It is \"you must give me\n> either -f (i.e. please do work) or -n (i.e. please show what you\n> would do) before I do anything\".\n>\n> $ git clean\n> fatal: clean.requireForce defaults to true and neither -i, -n, nor -f given; refusing to clean\n>\n> Given that, it is hard to argue that it would be a natural end-user\n> expectation that the command does something useful (i.e. show what\n> would be done) when it is given \"-f\" and \"-n\" at the same time.\n> What makes this a rather nonsense UI is the fact that \"-f\" does not\n> work the way we would expect for this command.\n\nI think we all agree that current UI is a kind of nonsense, but have\ndifferent views of the optimal target interface. My points are as\nfollowing:\n\n1. The fact that bare \"git clean\" only produces error by default is\nprobably a good thing, as removal of untracked files is unrecoverable\noperation in Git domain, so requiring -f by default is probably a good\nthing as well, provided the *only* operation that \"git clean\" performs\nis dangerous enough.\n\n2. The \"-n\" behavior is pure nonsense.\n\nSo, how do we fix (2)? Let's try mental experiment. Suppose there is no\n\"-n\" option for \"git clean\" and we are going to implement it. We start\nfrom:\n\n $ git clean\n fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean\n $ git clean -f\n removing \"a\"\n removing \"b\"\n $\n\nPlease notice that there is no \"-n\" in the error message as there is no\nsuch option yet in our experiment.\n\nNow we are going to introduce \"dry run\" option \"-n\". Most simple and\nobvious way to do it is to set internal flag \"dry_run\" and then at every\ninvocation of \"remove(file_name)\" put an if(dry_run) that will just\nprint(file_name) instead or removing it. Let's suppose we did just that.\nWe get this behavior:\n\n $ git clean -n\n fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean\n $ git clean -f -n\n would remove \"a\"\n would remove \"b\"\n $ git clean -f -f -n\n would remove \"a\"\n would remove \"b\"\n would remove \"sub/a\"\n $\n\nI see this as logical, clean, and straightforward behavior, meeting user\nexpectations for \"dry run\" option, so I suggest to do just that.\n\nThanks,\n-- Sergey Organov.\n"},{"id":"487371","messageId":"xmqq1qa5xq4n.fsf@gitster.g","threadId":"60711","inReplyTo":"87il3h72ym.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-25T17:46:32Z","receivedAt":"2024-01-25T17:46:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Now we are going to introduce \"dry run\" option \"-n\". Most simple and\n> obvious way to do it is to set internal flag \"dry_run\" and then at every\n> invocation of \"remove(file_name)\" put an if(dry_run) that will just\n> print(file_name) instead or removing it. Let's suppose we did just that.\n> We get this behavior:\n>\n> $ git clean -n\n> fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean\n> $ git clean -f -n\n> would remove \"a\"\n> would remove \"b\"\n> $ git clean -f -f -n\n> would remove \"a\"\n> would remove \"b\"\n> would remove \"sub/a\"\n> $\n>\n> I see this as logical, clean, and straightforward behavior, meeting user\n> expectations for \"dry run\" option, so I suggest to do just that.\n\nI think we are saying the same thing. If the original semantics\nwere \"you must force with -f to do anything useful\", instead of \"you\nmust choose either forcing with -f or not doing with -n\", then it\nwould have led to the above behaviour.\n\nThe thing is, it is way too late to change it that way without\nbreaking too many folks, and that is the problem.\n\n"},{"id":"487381","messageId":"87ede56tva.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqq1qa5xq4n.fsf@gitster.g","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-25T20:27:53Z","receivedAt":"2024-01-25T20:27:58Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> Now we are going to introduce \"dry run\" option \"-n\". Most simple and\n>> obvious way to do it is to set internal flag \"dry_run\" and then at every\n>> invocation of \"remove(file_name)\" put an if(dry_run) that will just\n>> print(file_name) instead or removing it. Let's suppose we did just that.\n>> We get this behavior:\n>>\n>> $ git clean -n\n>> fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean\n>> $ git clean -f -n\n>> would remove \"a\"\n>> would remove \"b\"\n>> $ git clean -f -f -n\n>> would remove \"a\"\n>> would remove \"b\"\n>> would remove \"sub/a\"\n>> $\n>>\n>> I see this as logical, clean, and straightforward behavior, meeting user\n>> expectations for \"dry run\" option, so I suggest to do just that.\n>\n> I think we are saying the same thing. If the original semantics\n> were \"you must force with -f to do anything useful\", instead of \"you\n> must choose either forcing with -f or not doing with -n\", then it\n> would have led to the above behaviour.\n>\n> The thing is, it is way too late to change it that way without\n> breaking too many folks, and that is the problem.\n\nIf we agree on the behavior above for sane \"dry run\", yet you worry\nabout backward compatibility so much to deny changing the behavior of\n\"-n\", then a way to go could be to introduce, say, \"-d\" for sane \"dry\nrun\", and obsolete \"-n\" while keeping it alone.\n\nThanks,\n-- Sergey Organov\n\n\n"},{"id":"487382","messageId":"87a5ot6tos.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"87ede56tva.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-25T20:31:47Z","receivedAt":"2024-01-25T20:31:51Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Sergey Organov writes:\n\n> Junio C Hamano writes:\n>\n>> Sergey Organov writes:\n>>\n>>> Now we are going to introduce \"dry run\" option \"-n\". Most simple and\n>>> obvious way to do it is to set internal flag \"dry_run\" and then at every\n>>> invocation of \"remove(file_name)\" put an if(dry_run) that will just\n>>> print(file_name) instead or removing it. Let's suppose we did just that.\n>>> We get this behavior:\n>>>\n>>> $ git clean -n\n>>> fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean\n>>> $ git clean -f -n\n>>> would remove \"a\"\n>>> would remove \"b\"\n>>> $ git clean -f -f -n\n>>> would remove \"a\"\n>>> would remove \"b\"\n>>> would remove \"sub/a\"\n>>> $\n>>>\n>>> I see this as logical, clean, and straightforward behavior, meeting user\n>>> expectations for \"dry run\" option, so I suggest to do just that.\n>>\n>> I think we are saying the same thing. If the original semantics\n>> were \"you must force with -f to do anything useful\", instead of \"you\n>> must choose either forcing with -f or not doing with -n\", then it\n>> would have led to the above behaviour.\n>>\n>> The thing is, it is way too late to change it that way without\n>> breaking too many folks, and that is the problem.\n>\n> If we agree on the behavior above for sane \"dry run\", yet you worry\n> about backward compatibility so much to deny changing the behavior of\n> \"-n\", then a way to go could be to introduce, say, \"-d\" for sane \"dry\n> run\", and obsolete \"-n\" while keeping it alone.\n\nExcept exactly \"-d\" is already taken, but you get the idea.\n\n-- Sergey Organov\n"},{"id":"487395","messageId":"xmqqzfwspmh0.fsf@gitster.g","threadId":"60711","inReplyTo":"87a5ot6tos.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-26T07:44:59Z","receivedAt":"2024-01-26T07:45:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Sergey Organov writes:\n>\n>> Junio C Hamano writes:\n>> ..\n>>> ... If the original semantics\n>>> were \"you must force with -f to do anything useful\", instead of \"you\n>>> must choose either forcing with -f or not doing with -n\", then it\n>>> would have led to the above behaviour.\n>> ...\n>> If we agree on the behavior above for sane \"dry run\"...\n\nNot so fast. I said \"if the original semantics were ... then it\nwould have led to the above behaviour\". As the original semantics\nwere not, that conclusion does not stand.\n\nThe \"-n\" option here were not added primarily as a dry-run option,\nand haven't been treated as such forever. As can be seen by the\n\"you must give either -f or -n option, and it is an error to give\nneither\" rule, from the end-user's point of view, it is a way to say\n\"between do-it (-f) and do-not-do-it (-n), I choose the latter for\nthis invocation\". And in that context, an attempt to make \"-f -f\"\nmean a stronger form of forcing than \"-f\" was a mistake, because it\nmakes your \"I want to see what happens if I tried that opration that\nrequires the stronger force\" request impossible.\n\nAnd there are two equally valid ways to deal with this misfeature.\n\nOne is to admit that \"-f -f\" was a mistake (which I already said),\nand a natural consequence of that admission is to introduce a more\nspecific \"in addition to what you do usually, this riskier operation\nis allowed\" option (e.g., --nested-repo). This leads to a design\nthat matches real world usage better, even if we did not have the\n\"how to ask dry-run?\" issue, because in the real world, when there\nare multiple \"risky\" things you may have to explicitly ask to\nenable, these things do not necessarily form a nice linear\n\"riskiness levels\" that you can express your risk tolerance with the\nnumber of \"-f\" options. When you need to add special protection for\na new case other than \"nested repo\", for example, the \"riskiness\nlevels\" design may need to place it above the \"nested repo\" level of\nriskiness and may require the user to give three \"-f\" options, but\nthat would make it impossible to protect against nuking of nested\nrepos while allowing only that newly added case. By having more\nspecific \"this particular risky operation is allowed\", \"-f\" can\nstill be \"between do-it and do-not-do-it, I choose the former\", and\nthe \"--nested-repo\" (and other options to allow specific risky\noperations we add in the future) would not have to have funny\ninteractions with \"-n\".\n\nThe other valid way is to treat the use of the \"riskiness levels\" to\nspecify what is forced still as a good idea. If one comes from that\nposition, the resulting UI would be consistent with what you have\nbeen advocating for. One or more \"-f\" will specify what kind of\nrisky stuff are allowed, and \"-n\" will say whether the operation\ngets carried out or merely shown what would happen if \"-n\" weren't\nthere.\n\nIt is just that I think \"riskiness levels\" I did in a0f4afbe (clean:\nrequire double -f options to nuke nested git repository and work\ntree, 2009-06-30) was an utter mistake, and that is why I feel very\nhesitant to agree with the design that still promotes it.\n"},{"id":"487404","messageId":"87ede4fg8s.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqqzfwspmh0.fsf@gitster.g","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-26T12:09:39Z","receivedAt":"2024-01-26T12:09:43Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> Sergey Organov writes:\n>>\n>>> Junio C Hamano writes:\n>>> ..\n>>>> ... If the original semantics\n>>>> were \"you must force with -f to do anything useful\", instead of \"you\n>>>> must choose either forcing with -f or not doing with -n\", then it\n>>>> would have led to the above behaviour.\n>>> ...\n>>> If we agree on the behavior above for sane \"dry run\"...\n>\n> Not so fast. I said \"if the original semantics were ... then it\n> would have led to the above behaviour\". As the original semantics\n> were not, that conclusion does not stand.\n\nOK, fine, then my point is that the original semantics if flawed.\n\n>\n> The \"-n\" option here were not added primarily as a dry-run option,\n> and haven't been treated as such forever. As can be seen by the\n> \"you must give either -f or -n option, and it is an error to give\n> neither\" rule, from the end-user's point of view, it is a way to say\n> \"between do-it (-f) and do-not-do-it (-n), I choose the latter for\n> this invocation\".\n\nYep, and in my opinion this is even more a mistake than \"-f -f\".\n\n> And in that context, an attempt to make \"-f -f\"\n> mean a stronger form of forcing than \"-f\" was a mistake, because it\n> makes your \"I want to see what happens if I tried that opration that\n> requires the stronger force\" request impossible.\n\nI believe this just emphasizes the original mistake of \"-n\" design\nmeaning something else than simple \"dry run\".\n\n>\n> And there are two equally valid ways to deal with this misfeature.\n\nI rather see two almost independent misfeatures here, so I believe both\nare to be addressed.\n\n>\n> One is to admit that \"-f -f\" was a mistake (which I already said),\n> and a natural consequence of that admission is to introduce a more\n> specific \"in addition to what you do usually, this riskier operation\n> is allowed\" option (e.g., --nested-repo).\n\nThis addresses one of the two deficiencies I see, yes.\n\n> This leads to a design that matches real world usage better, even if\n> we did not have the \"how to ask dry-run?\" issue, because in the real\n> world, when there are multiple \"risky\" things you may have to\n> explicitly ask to enable, these things do not necessarily form a nice\n> linear \"riskiness levels\" that you can express your risk tolerance\n> with the number of \"-f\" options. When you need to add special\n> protection for a new case other than \"nested repo\", for example, the\n> \"riskiness levels\" design may need to place it above the \"nested repo\"\n> level of riskiness and may require the user to give three \"-f\"\n> options, but that would make it impossible to protect against nuking\n> of nested repos while allowing only that newly added case. By having\n> more specific \"this particular risky operation is allowed\", \"-f\" can\n> still be \"between do-it and do-not-do-it, I choose the former\",\n\nYep, makes sense.\n\n> and the \"--nested-repo\" (and other options to allow specific risky\n> operations we add in the future) would not have to have funny\n> interactions with \"-n\".\n\nYep, but it still leaves \"-n\" being defective, as it for whatever reason\nsurprisingly clashes with \"-f\". I believe it shouldn't.\n\n> The other valid way is to treat the use of the \"riskiness levels\" to\n> specify what is forced still as a good idea. If one comes from that\n> position, the resulting UI would be consistent with what you have\n> been advocating for. One or more \"-f\" will specify what kind of\n> risky stuff are allowed, and \"-n\" will say whether the operation\n> gets carried out or merely shown what would happen if \"-n\" weren't\n> there.\n\nI'm not arguing in favor of \"-f -f\". My point is that even if you fix\n\"-f -f\", \"-n\" deficiency will still cry for fixing.\n\n>\n> It is just that I think \"riskiness levels\" I did in a0f4afbe (clean:\n> require double -f options to nuke nested git repository and work\n> tree, 2009-06-30) was an utter mistake, and that is why I feel very\n> hesitant to agree with the design that still promotes it.\n\nAgain, I'm not arguing in favor of \"-f -f\", I'm rather neutral about it.\n\nI'm still arguing in favor of fixing \"-n\", and I believe a fix is needed\nindependently from decision about \"-f -f\".\n\nThanks,\n-- Sergey Organov\n"},{"id":"487449","messageId":"xmqqzfwrjdul.fsf@gitster.g","threadId":"60711","inReplyTo":"87ede4fg8s.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-27T10:00:02Z","receivedAt":"2024-01-27T10:00:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> I'm still arguing in favor of fixing \"-n\", and I believe a fix is needed\n> independently from decision about \"-f -f\".\n\nEven though I do not personally like it, I do not think \"which\nbetween do-it (f) and do-not-do-it (n) do you want to use?\" is\nbroken. It sometimes irritates me to find \"git clean\" (without \"-f\"\nor \"-n\", and with clean.requireForce not disabled) complain, and I\npersonally think \"git clean\" when clean.requireForce is in effect\nand no \"-n\" or \"-f\" were given should pretend as if \"-n\" were given.\nI wish if it were \"without -n or -f, we pretend as if -n were given,\npossibly with a warning that says 'you need -f if you actually want\nto carry out these operations'\".\n\nBut that is a separate usability issue.\n\nWhat I find broken is that giving one 'f' and one 'n' in different\norder, i.e. \"-f -n\" and \"-n -f\", does not do what I expect. If you\nare choosing between do-it (f) and do-not-do-it (n), you ought to be\nable to rely on the usual last-one-wins rule. That I find broken.\n\nThe mistake[*] of \"-f -f\" is rather obvious, given that the other\n\"normal\" ways to tweak what is affected by the command are done as\n\"what else do we clean? directories (d)? ignored (x)?...\" options.\nWhen we add the upcoming \"precious\" bit support, we should make sure\nthat the way to trigger \"oh, by the way, please clobber those paths\nthat are marked precious, too\" is not by giving three '-f'. It\nwould make it impossible to ask for that without also removing\nnested repositories, which takes two '-f'.\n\n\n[Footnote]\n\n * To a lessor extent, the -v (verbose) option shares the same\n problem as \"-f -f\" here, in that its worldview is to assume that\n a single \"verbosity level\" is sufficient. Unlike the severity\n level thing, however, the user who wanted to see only messages\n about X but have to also see messages about Y and Z that are at\n the same or lessor verbosity level as X can filter out unwanted\n messages without causing a real harm.\n"},{"id":"487452","messageId":"87il3enc1i.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqqzfwrjdul.fsf@gitster.g","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-27T13:25:29Z","receivedAt":"2024-01-27T13:25:32Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> I'm still arguing in favor of fixing \"-n\", and I believe a fix is needed\n>> independently from decision about \"-f -f\".\n>\n> Even though I do not personally like it, I do not think \"which\n> between do-it (f) and do-not-do-it (n) do you want to use?\" is\n> broken.\n\nWell, you are right, but \"-n\" is not documented as \"do-not-do-it\" in the\nsense you use it here. \n\n> It sometimes irritates me to find \"git clean\" (without \"-f\"\n> or \"-n\", and with clean.requireForce not disabled) complain, and I\n> personally think \"git clean\" when clean.requireForce is in effect\n> and no \"-n\" or \"-f\" were given should pretend as if \"-n\" were given.\n> I wish if it were \"without -n or -f, we pretend as if -n were given,\n> possibly with a warning that says 'you need -f if you actually want\n> to carry out these operations'\".\n\nYep, then we'd not need \"-n\" that much, only if to cancel explicit \"-f\"\n(provided \"-f -f\" feature is removed.)\n\n>\n> But that is a separate usability issue.\n\nYep, and that'd be very different design. \n\n>\n> What I find broken is that giving one 'f' and one 'n' in different\n> order, i.e. \"-f -n\" and \"-n -f\", does not do what I expect. If you\n> are choosing between do-it (f) and do-not-do-it (n), you ought to be\n> able to rely on the usual last-one-wins rule. That I find broken.\n\nI fail to see where this expectation comes from, provided \"-n\" is not\ndocumented as anything opposed to \"-f\":\n\n -n, --dry-run\n Don’t actually remove anything, just show what would be done.\n\nThis is typical convenient description of \"dry run\", and current \"-n\"\nimplementation is rather close to the description, that I'd still\nrewrite to emphasize the primary goal of the --dry-run:\n\n -n, --dry-run\n Show what would be done, and don’t actually remove anything.\n\nWith these descriptions, the last thing that I'd expect is \"-n -f\"\nremoving my files.\n\nOverall, as I see it, we have buggy implementation of suitably\ndocumented \"--dry-run\" option, and the best course is to fix the\nbug, with no semantic changes to the option itself.\n\nThanks,\n-- Sergey Organov\n"},{"id":"487494","messageId":"87jzns7a8a.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqqzfwrjdul.fsf@gitster.g","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-29T09:35:49Z","receivedAt":"2024-01-29T09:35:52Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> I'm still arguing in favor of fixing \"-n\", and I believe a fix is needed\n>> independently from decision about \"-f -f\".\n>\n> Even though I do not personally like it, I do not think \"which\n> between do-it (f) and do-not-do-it (n) do you want to use?\" is\n> broken. It sometimes irritates me to find \"git clean\" (without \"-f\"\n> or \"-n\", and with clean.requireForce not disabled) complain, and I\n> personally think \"git clean\" when clean.requireForce is in effect\n> and no \"-n\" or \"-f\" were given should pretend as if \"-n\" were given.\n\nAs a note, I'd consider to get rid of 'clean.requireForce' anyway, as\nits default value provides safe reasonably behaving environment, and I\nfail to see why anybody would need to set it to 'false'.\n\nThanks,\n-- Sergey Organov\n"},{"id":"487536","messageId":"20240129182006.GC3765717@coredump.intra.peff.net","threadId":"60711","inReplyTo":"87jzns7a8a.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-29T18:20:06Z","receivedAt":"2024-01-29T18:20:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 29, 2024 at 12:35:49PM +0300, Sergey Organov wrote:\n\n> >> I'm still arguing in favor of fixing \"-n\", and I believe a fix is needed\n> >> independently from decision about \"-f -f\".\n> >\n> > Even though I do not personally like it, I do not think \"which\n> > between do-it (f) and do-not-do-it (n) do you want to use?\" is\n> > broken. It sometimes irritates me to find \"git clean\" (without \"-f\"\n> > or \"-n\", and with clean.requireForce not disabled) complain, and I\n> > personally think \"git clean\" when clean.requireForce is in effect\n> > and no \"-n\" or \"-f\" were given should pretend as if \"-n\" were given.\n> \n> As a note, I'd consider to get rid of 'clean.requireForce' anyway, as\n> its default value provides safe reasonably behaving environment, and I\n> fail to see why anybody would need to set it to 'false'.\n\nPlease don't. I set it to \"false\", because I find the default behavior a\npointless roadblock if you are already aware that \"git clean\" can be\ndestructive. Surely I can't be the only one.\n\n-Peff\n"},{"id":"487542","messageId":"7f97e5e4-c394-4403-94f1-6163fbd02e88@app.fastmail.com","threadId":"60711","inReplyTo":"87il3enc1i.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-29T19:40:42Z","receivedAt":"2024-01-29T19:41:04Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Sat, Jan 27, 2024, at 14:25, Sergey Organov wrote:\n> Junio C Hamano writes:\n>\n>> Sergey Organov writes:\n\nI agree with Sergey.\n\nLet’s suppose I’ve never used git-clean(1) (and I almost never use\nit). I read the man page to find out what it’s about. Oh, it removes\nfiles that I haven’t tracked. That sounds dangerous. But I see under\n`-n, --dry-run` that I can simulate what it would do:\n\n “ Don’t actually remove anything, just show what would be done.\n\nGreat, this is what I want. So this seems to mean to run `git clean` and\njust tell me what would happen. But now I’ve already read that it\nrequires `--force` in order to do anything. Which means that I don’t\nwant to just run:\n\n```\ngit clean --dry-run\n```\n\nSince I presume that would give me the “no `--force` provided”\nerror. Which means that I want to tack on `--force`:\n\n```\ngit clean --dry-run --force\n```\n\nNow I figure that this will run `git clean --force` but switch real\ndeletion with printing the filenames.[1]\n\nJunio wrote:\n\n> What I find broken is that giving one 'f' and one 'n' in different\n> order, i.e. \"-f -n\" and \"-n -f\", does not do what I expect. If you\n> are choosing between do-it (f) and do-not-do-it (n), you ought to be\n> able to rely on the usual last-one-wins rule. That I find broken.\n\nNow suppose I have noticed that some git(1) commands have these\n`--[no-]do-it` options. I know that I can leverage this to override a\nprevious option. And that is useful when I for example have an alias\nwith `--do-it` but for this invocation I want `--no-do-it`. I read about\n`--force` here but see that there is no `--no-force`. I then assume that\nthe only things that have to do with `--force` or not is that option and\nthe `requireForce` configuration variable.\n\nI’ve also seen `--force` in other git(1) commands. And they usually are\nabout some specific scenario rather than the whole command itself, since\ne.g. committing one too many times doesn’t really hurt. But I understand\nhow `--force` applies to all the useful work that git-clean(1) does\nbecause all the useful work is also destructive work. So this is what I\nexpect from these options in general:\n\n1. `--force`: require for the subset of actions that are potentially\n dangerous or may be unwanted in some way\n2. `--dry-run`: simulate the action (specifically print everything that\n would happen but don’t do anything to `.git`, to untracked files, or\n anything else)\n\nAnd I expect these two to be orthogonal. Because I might want—if the\noption is there—to simulate some `--force` (e.g. `git push --force`)\nwith a `--dry-run`. As in: what would be printed? I wouldn’t expect\n`--force` to override `--dry-run`.\n\n† 1: I’m never this careful in real life. But this is about deleting\n files without any (from Git) recovery so I guess some prudence is\n required in this case.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"487557","messageId":"87v87bx12j.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"20240129182006.GC3765717@coredump.intra.peff.net","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-29T21:49:08Z","receivedAt":"2024-01-29T21:49:12Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jan 29, 2024 at 12:35:49PM +0300, Sergey Organov wrote:\n>\n>> >> I'm still arguing in favor of fixing \"-n\", and I believe a fix is needed\n>> >> independently from decision about \"-f -f\".\n>> >\n>> > Even though I do not personally like it, I do not think \"which\n>> > between do-it (f) and do-not-do-it (n) do you want to use?\" is\n>> > broken. It sometimes irritates me to find \"git clean\" (without \"-f\"\n>> > or \"-n\", and with clean.requireForce not disabled) complain, and I\n>> > personally think \"git clean\" when clean.requireForce is in effect\n>> > and no \"-n\" or \"-f\" were given should pretend as if \"-n\" were given.\n>> \n>> As a note, I'd consider to get rid of 'clean.requireForce' anyway, as\n>> its default value provides safe reasonably behaving environment, and I\n>> fail to see why anybody would need to set it to 'false'.\n>\n> Please don't. I set it to \"false\", because I find the default behavior a\n> pointless roadblock if you are already aware that \"git clean\" can be\n> destructive. Surely I can't be the only one.\n\nWell, provided there is at least one person who finds it useful to set\nit to 'false', I withdraw my suggestion.\n\nThat said, did you consider to:\n\n $ git config --global alias.cl 'clean -f'\n\ninstead of\n\n $ git config --global clean.requireForce false\n\nI wonder?\n\nThanks,\n-- Sergey Organov\n"},{"id":"487583","messageId":"20240130054401.GA166761@coredump.intra.peff.net","threadId":"60711","inReplyTo":"87v87bx12j.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:44:01Z","receivedAt":"2024-01-30T05:44:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 30, 2024 at 12:49:08AM +0300, Sergey Organov wrote:\n\n> > Please don't. I set it to \"false\", because I find the default behavior a\n> > pointless roadblock if you are already aware that \"git clean\" can be\n> > destructive. Surely I can't be the only one.\n> \n> Well, provided there is at least one person who finds it useful to set\n> it to 'false', I withdraw my suggestion.\n> \n> That said, did you consider to:\n> \n> $ git config --global alias.cl 'clean -f'\n> \n> instead of\n> \n> $ git config --global clean.requireForce false\n> \n> I wonder?\n\nNot really, as when I originally set the config in 2007, it was just\nundoing the then-recent change to default clean.requireForce to true. I\nalready had muscle memory using \"git clean\" as it had worked\nhistorically from 2005-2007.\n\nI know that isn't necessarily relevant for new users today, but my point\nis mostly that we have clean.requireForce already and people would\nprobably be annoyed if we took it away. :)\n\n-Peff\n"},{"id":"487586","messageId":"xmqqv87bcqod.fsf@gitster.g","threadId":"60711","inReplyTo":"20240130054401.GA166761@coredump.intra.peff.net","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-30T05:53:54Z","receivedAt":"2024-01-30T05:54:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I know that isn't necessarily relevant for new users today, but my point\n> is mostly that we have clean.requireForce already and people would\n> probably be annoyed if we took it away. :)\n\nSounds quite sane and sensible position.\n\nMy favourite question Git Rev News may ask their interviewee is \"if\nthere were no existing users to worry about, what would you change\nin Git?\". I have many things in my mind I would change if we could,\nbut they remain only in my fantasy, because we have to care, and I\nhave to fight for, those existing users who are silent majority,\nsimply due to the age of the tool.\n"},{"id":"487640","messageId":"87plxhiri4.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"87il3enc1i.fsf@osv.gnss.ru","subject":"Re: what should \"git clean -n -f [-d] [-x] \" do?","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-01-31T13:04:03Z","receivedAt":"2024-01-31T13:04:06Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Sergey Organov writes:\n\n> Junio C Hamano writes:\n>\n>> Sergey Organov writes:\n>>\n>>> I'm still arguing in favor of fixing \"-n\", and I believe a fix is needed\n>>> independently from decision about \"-f -f\".\n>>\n>> Even though I do not personally like it, I do not think \"which\n>> between do-it (f) and do-not-do-it (n) do you want to use?\" is\n>> broken.\n>\n> Well, you are right, but \"-n\" is not documented as \"do-not-do-it\" in the\n> sense you use it here. \n>\n>> It sometimes irritates me to find \"git clean\" (without \"-f\"\n>> or \"-n\", and with clean.requireForce not disabled) complain, and I\n>> personally think \"git clean\" when clean.requireForce is in effect\n>> and no \"-n\" or \"-f\" were given should pretend as if \"-n\" were given.\n>> I wish if it were \"without -n or -f, we pretend as if -n were given,\n>> possibly with a warning that says 'you need -f if you actually want\n>> to carry out these operations'\".\n>\n> Yep, then we'd not need \"-n\" that much, only if to cancel explicit \"-f\"\n> (provided \"-f -f\" feature is removed.)\n>\n>>\n>> But that is a separate usability issue.\n>\n> Yep, and that'd be very different design. \n>\n>>\n>> What I find broken is that giving one 'f' and one 'n' in different\n>> order, i.e. \"-f -n\" and \"-n -f\", does not do what I expect. If you\n>> are choosing between do-it (f) and do-not-do-it (n), you ought to be\n>> able to rely on the usual last-one-wins rule. That I find broken.\n>\n> I fail to see where this expectation comes from, provided \"-n\" is not\n> documented as anything opposed to \"-f\":\n>\n> -n, --dry-run\n> Don’t actually remove anything, just show what would be done.\n>\n> This is typical convenient description of \"dry run\", and current \"-n\"\n> implementation is rather close to the description, that I'd still\n> rewrite to emphasize the primary goal of the --dry-run:\n>\n>\n> With these descriptions, the last thing that I'd expect is \"-n -f\"\n> removing my files.\n>\n> Overall, as I see it, we have buggy implementation of suitably\n> documented \"--dry-run\" option, and the best course is to fix the\n> bug, with no semantic changes to the option itself.\n\nOTOH, to preserve current actual behavior as much as possible, we can\nprobably first fix documentation like this:\n\n -n, --dry-run\n Show what would be done, and don’t actually remove anything.\n This sets 'clean.requireForce' to 'false' for the duration\n of this command execution.\n\nthat to me looks like a match for current observable behavior.\n\nThen we can fix '-n' implementation exactly according to this updated\nspecification, making '-n' really independent from '-f', yet keeping\npure \"git clean -n\" as well as \"git clean -f -n\", and \"git clean -n -f\"\nbackward compatible.\n\nAs a bonus, the above solution will also free our hands in [re]defining\n'-f -f' later, if needed.\n\nWDYT?\n\nThanks,\n-- Sergey Organov\n"},{"id":"489678","messageId":"875xy76qe1.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqq34v6gswv.fsf@gitster.g","subject":"[PATCH] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-02-29T19:07:18Z","receivedAt":"2024-02-29T19:07:21Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"What -n actually does in addition to its documented behavior is\nignoring of configuration variable clean.requireForce, that makes\nsense provided -n prevents files removal anyway.\n\nSo, first, document this in the manual, and then modify implementation\nto make this more explicit in the code.\n\nImproved implementation also stops to share single internal variable\n'force' between command-line -f option and configuration variable\nclean.requireForce, resulting in more clear logic.\n\nThe error messages now do not mention -n as well, as it seems\nunnecessary and does not reflect clarified implementation.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/git-clean.txt | 2 ++\n builtin/clean.c | 26 +++++++++++++-------------\n 2 files changed, 15 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex 69331e3f05a1..662eebb85207 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -49,6 +49,8 @@ OPTIONS\n -n::\n --dry-run::\n \tDon't actually remove anything, just show what would be done.\n+\tConfiguration variable clean.requireForce is ignored, as\n+\tnothing will be deleted anyway.\n \n -q::\n --quiet::\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d90766cad3a0..fcc50d08ee9b 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -25,7 +25,7 @@\n #include \"help.h\"\n #include \"prompt.h\"\n \n-static int force = -1; /* unset */\n+static int require_force = -1; /* unset */\n static int interactive;\n static struct string_list del_list = STRING_LIST_INIT_DUP;\n static unsigned int colopts;\n@@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"clean.requireforce\")) {\n-\t\tforce = !git_config_bool(var, value);\n+\t\trequire_force = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n {\n \tint i, res;\n \tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n-\tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n+\tint ignored_only = 0, force = 0, errors = 0, gone = 1;\n \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n \tstruct strbuf abs_path = STRBUF_INIT;\n \tstruct dir_struct dir = DIR_INIT;\n@@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t};\n \n \tgit_config(git_clean_config, NULL);\n-\tif (force < 0)\n-\t\tforce = 0;\n-\telse\n-\t\tconfig_set = 1;\n \n \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n \t\t\t 0);\n \n-\tif (!interactive && !dry_run && !force) {\n-\t\tif (config_set)\n-\t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\n+\t/* Dry run won't remove anything, so requiring force makes no sense */\n+\tif(dry_run)\n+\t\trequire_force = 0;\n+\n+\tif (!force && !interactive) {\n+\t\tif (require_force > 0)\n+\t\t\tdie(_(\"clean.requireForce set to true and neither -f, nor -i given; \"\n+\t\t\t\t \"refusing to clean\"));\n+\t\telse if (require_force < 0)\n+\t\t\tdie(_(\"clean.requireForce defaults to true and neither -f, nor -i given; \"\n \t\t\t\t \"refusing to clean\"));\n-\t\telse\n-\t\t\tdie(_(\"clean.requireForce defaults to true and neither -i, -n, nor -f given;\"\n-\t\t\t\t \" refusing to clean\"));\n \t}\n \n \tif (force > 1)\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n-- \n2.25.1\n\n"},{"id":"489738","messageId":"51a196c0-ea57-4ec5-99ea-c3f09cd90962@gmail.com","threadId":"60711","inReplyTo":"875xy76qe1.fsf@osv.gnss.ru","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Jean-Noël Avila","fromEmail":"avila.jn@gmail.com","sentAt":"2024-03-01T13:20:47Z","receivedAt":"2024-03-01T13:20:52Z","isPatch":true,"sender":{"key":"jn.avila@free.fr","avatar":"https://avatars.githubusercontent.com/u/156172?v=4"},"body":"Putting my documentation/translator hat:\n\nLe 29/02/2024 à 20:07, Sergey Organov a écrit :\n> What -n actually does in addition to its documented behavior is\n> ignoring of configuration variable clean.requireForce, that makes\n> sense provided -n prevents files removal anyway.\n> \n> So, first, document this in the manual, and then modify implementation\n> to make this more explicit in the code.\n> \n> Improved implementation also stops to share single internal variable\n> 'force' between command-line -f option and configuration variable\n> clean.requireForce, resulting in more clear logic.\n> \n> The error messages now do not mention -n as well, as it seems\n> unnecessary and does not reflect clarified implementation.\n> \n> Signed-off-by: Sergey Organov \n> ---\n> Documentation/git-clean.txt | 2 ++\n> builtin/clean.c | 26 +++++++++++++-------------\n> 2 files changed, 15 insertions(+), 13 deletions(-)\n> \n> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\n> index 69331e3f05a1..662eebb85207 100644\n> --- a/Documentation/git-clean.txt\n> +++ b/Documentation/git-clean.txt\n> @@ -49,6 +49,8 @@ OPTIONS\n> -n::\n> --dry-run::\n> \tDon't actually remove anything, just show what would be done.\n> +\tConfiguration variable clean.requireForce is ignored, as\n> +\tnothing will be deleted anyway.\n\nPlease use backticks for options, configuration and environment names:\n`clean.requireForce`\n> \n> -q::\n> --quiet::\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index d90766cad3a0..fcc50d08ee9b 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -25,7 +25,7 @@\n> #include \"help.h\"\n> #include \"prompt.h\"\n> \n> -static int force = -1; /* unset */\n> +static int require_force = -1; /* unset */\n> static int interactive;\n> static struct string_list del_list = STRING_LIST_INIT_DUP;\n> static unsigned int colopts;\n> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,\n> \t}\n> \n> \tif (!strcmp(var, \"clean.requireforce\")) {\n> -\t\tforce = !git_config_bool(var, value);\n> +\t\trequire_force = git_config_bool(var, value);\n> \t\treturn 0;\n> \t}\n> \n> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> {\n> \tint i, res;\n> \tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n> -\tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n> +\tint ignored_only = 0, force = 0, errors = 0, gone = 1;\n> \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n> \tstruct strbuf abs_path = STRBUF_INIT;\n> \tstruct dir_struct dir = DIR_INIT;\n> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> \t};\n> \n> \tgit_config(git_clean_config, NULL);\n> -\tif (force < 0)\n> -\t\tforce = 0;\n> -\telse\n> -\t\tconfig_set = 1;\n> \n> \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n> \t\t\t 0);\n> \n> -\tif (!interactive && !dry_run && !force) {\n> -\t\tif (config_set)\n> -\t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\n> +\t/* Dry run won't remove anything, so requiring force makes no sense */\n> +\tif(dry_run)\n> +\t\trequire_force = 0;\n> +\n> +\tif (!force && !interactive) {\n> +\t\tif (require_force > 0)\n> +\t\t\tdie(_(\"clean.requireForce set to true and neither -f, nor -i given; \"\n> +\t\t\t\t \"refusing to clean\"));\n> +\t\telse if (require_force < 0)\n> +\t\t\tdie(_(\"clean.requireForce defaults to true and neither -f, nor -i given; \"\n> \t\t\t\t \"refusing to clean\"));\n> -\t\telse\n> -\t\t\tdie(_(\"clean.requireForce defaults to true and neither -i, -n, nor -f given;\"\n> -\t\t\t\t \" refusing to clean\"));\n> \t}\n> \n\nThe last two cases can be coalesced into a single case (the last one),\nbecause the difference in the messages does not bring more information\nto the user.\n\n\n\n> \tif (force > 1)\n> \n> base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n\n"},{"id":"489739","messageId":"87frxam35f.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"51a196c0-ea57-4ec5-99ea-c3f09cd90962@gmail.com","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-01T14:34:52Z","receivedAt":"2024-03-01T14:34:55Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jean-Noël Avila writes:\n\n> Putting my documentation/translator hat:\n>\n> Le 29/02/2024 à 20:07, Sergey Organov a écrit :\n>> What -n actually does in addition to its documented behavior is\n>> ignoring of configuration variable clean.requireForce, that makes\n>> sense provided -n prevents files removal anyway.\n>> \n>> So, first, document this in the manual, and then modify implementation\n>> to make this more explicit in the code.\n>> \n>> Improved implementation also stops to share single internal variable\n>> 'force' between command-line -f option and configuration variable\n>> clean.requireForce, resulting in more clear logic.\n>> \n>> The error messages now do not mention -n as well, as it seems\n>> unnecessary and does not reflect clarified implementation.\n>> \n>> Signed-off-by: Sergey Organov \n>> ---\n>> Documentation/git-clean.txt | 2 ++\n>> builtin/clean.c | 26 +++++++++++++-------------\n>> 2 files changed, 15 insertions(+), 13 deletions(-)\n>> \n>> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\n>> index 69331e3f05a1..662eebb85207 100644\n>> --- a/Documentation/git-clean.txt\n>> +++ b/Documentation/git-clean.txt\n>> @@ -49,6 +49,8 @@ OPTIONS\n>> -n::\n>> --dry-run::\n>> \tDon't actually remove anything, just show what would be done.\n>> +\tConfiguration variable clean.requireForce is ignored, as\n>> +\tnothing will be deleted anyway.\n>\n> Please use backticks for options, configuration and environment names:\n> `clean.requireForce`\n\nI did consider this. However, existing text already has exactly this one\nunquoted, so I just did the same. Hopefully it will be fixed altogether\nlater, or are you positive I better resend the patch with quotes? \n\n>> \n>> -q::\n>> --quiet::\n>> diff --git a/builtin/clean.c b/builtin/clean.c\n>> index d90766cad3a0..fcc50d08ee9b 100644\n>> --- a/builtin/clean.c\n>> +++ b/builtin/clean.c\n>> @@ -25,7 +25,7 @@\n>> #include \"help.h\"\n>> #include \"prompt.h\"\n>> \n>> -static int force = -1; /* unset */\n>> +static int require_force = -1; /* unset */\n>> static int interactive;\n>> static struct string_list del_list = STRING_LIST_INIT_DUP;\n>> static unsigned int colopts;\n>> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,\n>> \t}\n>> \n>> \tif (!strcmp(var, \"clean.requireforce\")) {\n>> -\t\tforce = !git_config_bool(var, value);\n>> +\t\trequire_force = git_config_bool(var, value);\n>> \t\treturn 0;\n>> \t}\n>> \n>> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n>> {\n>> \tint i, res;\n>> \tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n>> -\tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n>> +\tint ignored_only = 0, force = 0, errors = 0, gone = 1;\n>> \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n>> \tstruct strbuf abs_path = STRBUF_INIT;\n>> \tstruct dir_struct dir = DIR_INIT;\n>> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n>> \t};\n>> \n>> \tgit_config(git_clean_config, NULL);\n>> -\tif (force < 0)\n>> -\t\tforce = 0;\n>> -\telse\n>> -\t\tconfig_set = 1;\n>> \n>> \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n>> \t\t\t 0);\n>> \n>> -\tif (!interactive && !dry_run && !force) {\n>> -\t\tif (config_set)\n>> -\t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\n>> +\t/* Dry run won't remove anything, so requiring force makes no sense */\n>> +\tif(dry_run)\n>> +\t\trequire_force = 0;\n>> +\n>> +\tif (!force && !interactive) {\n>> +\t\tif (require_force > 0)\n>> +\t\t\tdie(_(\"clean.requireForce set to true and neither -f, nor -i given; \"\n>> +\t\t\t\t \"refusing to clean\"));\n>> +\t\telse if (require_force < 0)\n>> +\t\t\tdie(_(\"clean.requireForce defaults to true and neither -f, nor -i given; \"\n>> \t\t\t\t \"refusing to clean\"));\n>> -\t\telse\n>> -\t\t\tdie(_(\"clean.requireForce defaults to true and neither -i, -n, nor -f given;\"\n>> -\t\t\t\t \" refusing to clean\"));\n>> \t}\n>> \n>\n> The last two cases can be coalesced into a single case (the last one),\n> because the difference in the messages does not bring more information\n> to the user.\n\nDid you misread the patch? There are only 2 cases here, the last (third)\none is marked with '-' (removed). Too easy to misread this, I'd say. New\ncode is:\n\n\t\tif (require_force > 0)\n\t\t\tdie(_(\"clean.requireForce set to true and neither -f, nor -i given; \"\n\t\t\t\t \"refusing to clean\"));\n\t\telse if (require_force < 0)\n\t\t\tdie(_(\"clean.requireForce defaults to true and neither -f, nor -i given; \"\n\nand is basically unchanged from the original, except reference to '-n' has been\nremoved. Btw, is now comma needed after -f, and isn't it better to\nsubstitute ':' for ';'?\n\nThank you for review!\n\n-- Sergey Organov\n\n"},{"id":"489741","messageId":"86ce3c89-4a58-42bf-a31a-96fa6b74e937@app.fastmail.com","threadId":"60711","inReplyTo":"87frxam35f.fsf@osv.gnss.ru","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-01T15:29:38Z","receivedAt":"2024-03-01T15:30:00Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Fri, Mar 1, 2024, at 15:34, Sergey Organov wrote:\n>> Please use backticks for options, configuration and environment names:\n>> `clean.requireForce`\n>\n> I did consider this. However, existing text already has exactly this one\n> unquoted, so I just did the same. Hopefully it will be fixed altogether\n> later, or are you positive I better resend the patch with quotes?\n\nSometimes I see widespread changes (like formatting many files) get\nrejected because it is considered _churn_. Not fixing this in this\nseries and then maybe someone else fixing it later seems like churn as\nwell. Isn’t it better to fix it while you are changing the text?\n\n-- \nKristoffer Haugsbakk\n"},{"id":"489748","messageId":"xmqqttlp6d2p.fsf@gitster.g","threadId":"60711","inReplyTo":"86ce3c89-4a58-42bf-a31a-96fa6b74e937@app.fastmail.com","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-01T18:07:10Z","receivedAt":"2024-03-01T18:07:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Fri, Mar 1, 2024, at 15:34, Sergey Organov wrote:\n>>> Please use backticks for options, configuration and environment names:\n>>> `clean.requireForce`\n>>\n>> I did consider this. However, existing text already has exactly this one\n>> unquoted, so I just did the same. Hopefully it will be fixed altogether\n>> later, or are you positive I better resend the patch with quotes?\n>\n> Sometimes I see widespread changes (like formatting many files) get\n> rejected because it is considered _churn_. Not fixing this in this\n> series and then maybe someone else fixing it later seems like churn as\n> well. Isn’t it better to fix it while you are changing the text?\n\nAny one of these is fine:\n\n (1) add the new paragraph with mark-up consistent with existing\n text (which is what Sergey did).\n\n (2) add the new paragraph with correct mark-up, making the document\n less consistent overall.\n\n (3) have one patch to fix broken mark-up of existing text, followed\n by another patch to add the new paragraph with correct mark-up.\n\nIf you take one of the first two, it would be a very good idea to\nhave a comment in the proposed log message to note the need for\nlater clean-up. Without being written down anywhere, your discovery\nand the brain cycles you spent while deciding what to do will be\nwasted, which is not what you want.\n\nThanks, both.\n"},{"id":"489749","messageId":"xmqqmsrh6d2f.fsf@gitster.g","threadId":"60711","inReplyTo":"51a196c0-ea57-4ec5-99ea-c3f09cd90962@gmail.com","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-01T18:07:20Z","receivedAt":"2024-03-01T18:07:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jean-Noël Avila writes:\n\n>> +\t/* Dry run won't remove anything, so requiring force makes no sense */\n>> +\tif(dry_run)\n>> +\t\trequire_force = 0;\n\nStyle. \"if (dry_run)\".\n\nGetting rid of \"config_set\", which was an extra variable that kept\ntrack of where \"force\" came from, does make the logic cleaner, I\nguess. What we want to happen is that one of -i/-n/-f is required\nwhen clean.requireForce is *not* unset (i.e. 0 <= require_force).\n\n>> +\tif (!force && !interactive) {\n\nThe require-force takes effect only when neither force or\ninteractive is given, so the new code structure puts the above\nobvious conditional around \"do we complain due to requireForce?\"\nlogic. Sensible.\n\n>> +\t\tif (require_force > 0)\n>> +\t\t\tdie(_(\"clean.requireForce set to true and neither -f, nor -i given; \"\n>> +\t\t\t\t \"refusing to clean\"));\n\nIf it is explicitly set, we get this message. And ...\n\n>> +\t\telse if (require_force < 0)\n>> +\t\t\tdie(_(\"clean.requireForce defaults to true and neither -f, nor -i given; \"\n>> \t\t\t\t \"refusing to clean\"));\n\n... if it is set due to default (in other words, if it is not unset), we\nget this message.\n\nAs you said, I do not think it matters too much either way to the\nend-users where the truth setting of clean.requireForce came from,\neither due to the default or the user explicitly configuring. So\nunifying to a single message may be helpful to both readers and\ntranslators.\n\n\tclean.requireForce is true; unless interactive, -f is required\n\nmight be a bit shorter and more to the point.\n\n> The last two cases can be coalesced into a single case (the last one),\n> because the difference in the messages does not bring more information\n> to the user.\n\nYeah.\n\nThanks.\n\n"},{"id":"489753","messageId":"xmqq7cil6bzy.fsf@gitster.g","threadId":"60711","inReplyTo":"xmqqmsrh6d2f.fsf@gitster.g","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-01T18:30:25Z","receivedAt":"2024-03-01T18:30:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Getting rid of \"config_set\", which was an extra variable that kept\n> track of where \"force\" came from, does make the logic cleaner, I\n> guess. What we want to happen is that one of -i/-n/-f is required\n> when clean.requireForce is *not* unset (i.e. 0 <= require_force).\n\nOh, noes. require_force is unset explicitly it would be 0, so this\nshould have read (i.e. require_force != 0). Sorry for a thinko.\n\n"},{"id":"489758","messageId":"878r31n3zj.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqqmsrh6d2f.fsf@gitster.g","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-01T19:31:28Z","receivedAt":"2024-03-01T19:31:32Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Jean-Noël Avila writes:\n>\n>>> +\t/* Dry run won't remove anything, so requiring force makes no sense */\n>>> +\tif(dry_run)\n>>> +\t\trequire_force = 0;\n>\n> Style. \"if (dry_run)\".\n\nOoops!\n\n>\n> Getting rid of \"config_set\", which was an extra variable that kept\n> track of where \"force\" came from, does make the logic cleaner, I\n> guess. What we want to happen is that one of -i/-n/-f is required\n> when clean.requireForce is *not* unset (i.e. 0 <= require_force).\n>\n>>> +\tif (!force && !interactive) {\n>\n> The require-force takes effect only when neither force or\n> interactive is given, so the new code structure puts the above\n> obvious conditional around \"do we complain due to requireForce?\"\n> logic. Sensible.\n>\n>>> +\t\tif (require_force > 0)\n>>> +\t\t\tdie(_(\"clean.requireForce set to true and neither -f, nor -i given; \"\n>>> +\t\t\t\t \"refusing to clean\"));\n>\n> If it is explicitly set, we get this message. And ...\n>\n>>> +\t\telse if (require_force < 0)\n>>> +\t\t\tdie(_(\"clean.requireForce defaults to true and neither -f, nor -i given; \"\n>>> \t\t\t\t \"refusing to clean\"));\n>\n> ... if it is set due to default (in other words, if it is not unset), we\n> get this message.\n>\n> As you said, I do not think it matters too much either way to the\n> end-users where the truth setting of clean.requireForce came from,\n> either due to the default or the user explicitly configuring. So\n> unifying to a single message may be helpful to both readers and\n> translators.\n>\n> \tclean.requireForce is true; unless interactive, -f is required\n>\n> might be a bit shorter and more to the point.\n\nDunno, I tried to keep changes to the bare sensible minimum, especially\nto avoid possible controversy. I'd leave this for somebody else to\ndecide upon and patch, if they feel like it.\n\n>> The last two cases can be coalesced into a single case (the last one),\n>> because the difference in the messages does not bring more information\n>> to the user.\n>\n> Yeah.\n\n\"The last two cases\" sounds like there are more of them, and there is\nnone, so to me it sounded like patch misread. Maybe I'm wrong.\n\nThanks for the review!\n\n-- Sergey Organov\n"},{"id":"489778","messageId":"xmqqv864zjbf.fsf@gitster.g","threadId":"60711","inReplyTo":"875xy76qe1.fsf@osv.gnss.ru","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-02T16:31:48Z","receivedAt":"2024-03-02T16:31:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> What -n actually does in addition to its documented behavior is\n> ignoring of configuration variable clean.requireForce, that makes\n> sense provided -n prevents files removal anyway.\n\nThere is another thing I noticed.\n\nThis part to get rid of \"config_set\" does make sense.\n\n> \tgit_config(git_clean_config, NULL);\n> -\tif (force < 0)\n> -\t\tforce = 0;\n> -\telse\n> -\t\tconfig_set = 1;\n\nWe used to think \"force\" variable is the master switch to do\nanything , and requireForce configuration was a way to flip its\ndefault to 0 (so that you need to set it to 1 again from the command\nline). This separates \"force\" (which can only given via the command\nline) and \"require_force\" (which controls when the \"force\" is used)\nand makes the logic simpler.\n\n> \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n> \t\t\t 0);\n\nHowever.\n\n> -\tif (!interactive && !dry_run && !force) {\n> -\t\tif (config_set)\n> -\t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\n> +\t/* Dry run won't remove anything, so requiring force makes no sense */\n> +\tif(dry_run)\n> +\t\trequire_force = 0;\n\nI am not sure if this is making things inconsistent.\n\nDry run will be harmless, and we can be lenient and not require\nforce. But below, we do not require force when going interactive,\neither. So we could instead add\n\n\tif (dry_run || interactive)\n\t\trequire_force = 0;\n\nabove, drop the \"&& !interactive\" from the guard for the\nclean.requireForce block.\n\nOr we can go the opposite way. We do not have to tweak\nrequire_force at all based on other conditions. Instead we can\nupdate the guard below to check \"!force && !interactive && !dry_run\"\nbefore entering the clean.requireForce block, no?\n\nBut the code after this patch makes me feel that it is somewhere in\nthe middle between these two optimum places.\n\nAnother thing. Stepping back and thinking _why_ the code can treat\ndry_run and interactive the same way (either to make them drop\nrequire_force above, or neither of them contributes to the value of\nrequire_force), if we are dropping \"you didn't give me --dry-run\" in\nthe error message below, we should also drop \"you didn't give me\n--interactive, either\" as well, when complaining about the lack of\n\"--force\".\n\nOne possible objection I can think of against doing so is that it\nmight not be so obvious why \"interactive\" does not have to require\n\"force\" (even though it is clearly obvious to me). But if that were\nthe objection, then to somebody else \"dry-run does not have to\nrequire force\" may equally not be so obvious (at least it wasn't so\nobvious to me during the last round of this discussion).\n\nSo I can live without the \"drop 'nor -i'\" part I suggested in the\nabove. We would not drop \"nor -i\" and add \"nor --dry-run\" back to\nthe message instead.\n\nSo from that angle, the message after this patch makes me feel that\nit is somewhere in the middle between two more sensible places.\n\n> +\tif (!force && !interactive) {\n> +\t\tif (require_force > 0)\n> +\t\t\tdie(_(\"clean.requireForce set to true and neither -f, nor -i given; \"\n> +\t\t\t\t \"refusing to clean\"));\n> +\t\telse if (require_force < 0)\n> +\t\t\tdie(_(\"clean.requireForce defaults to true and neither -f, nor -i given; \"\n> \t\t\t\t \"refusing to clean\"));\n> -\t\telse\n> -\t\t\tdie(_(\"clean.requireForce defaults to true and neither -i, -n, nor -f given;\"\n> -\t\t\t\t \" refusing to clean\"));\n> \t}\n> \n> \tif (force > 1)\n>\n> base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n"},{"id":"489791","messageId":"6033073.lOV4Wx5bFT@cayenne","threadId":"60711","inReplyTo":"87frxam35f.fsf@osv.gnss.ru","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Jean-Noël AVILA","fromEmail":"avila.jn@gmail.com","sentAt":"2024-03-02T19:47:55Z","receivedAt":"2024-03-02T19:57:24Z","isPatch":true,"sender":{"key":"jn.avila@free.fr","avatar":"https://avatars.githubusercontent.com/u/156172?v=4"},"body":"On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote:\n> Jean-Noël Avila writes:\n> \n> > Putting my documentation/translator hat:\n> >\n> > Le 29/02/2024 à 20:07, Sergey Organov a écrit :\n> >> What -n actually does in addition to its documented behavior is\n> >> ignoring of configuration variable clean.requireForce, that makes\n> >> sense provided -n prevents files removal anyway.\n> >> \n> >> So, first, document this in the manual, and then modify implementation\n> >> to make this more explicit in the code.\n> >> \n> >> Improved implementation also stops to share single internal variable\n> >> 'force' between command-line -f option and configuration variable\n> >> clean.requireForce, resulting in more clear logic.\n> >> \n> >> The error messages now do not mention -n as well, as it seems\n> >> unnecessary and does not reflect clarified implementation.\n> >> \n> >> Signed-off-by: Sergey Organov \n> >> ---\n> >> Documentation/git-clean.txt | 2 ++\n> >> builtin/clean.c | 26 +++++++++++++-------------\n> >> 2 files changed, 15 insertions(+), 13 deletions(-)\n> >> \n> >> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\n> >> index 69331e3f05a1..662eebb85207 100644\n> >> --- a/Documentation/git-clean.txt\n> >> +++ b/Documentation/git-clean.txt\n> >> @@ -49,6 +49,8 @@ OPTIONS\n> >> -n::\n> >> --dry-run::\n> >> \tDon't actually remove anything, just show what would be done.\n> >> +\tConfiguration variable clean.requireForce is ignored, as\n> >> +\tnothing will be deleted anyway.\n> >\n> > Please use backticks for options, configuration and environment names:\n> > `clean.requireForce`\n> \n> I did consider this. However, existing text already has exactly this one\n> unquoted, so I just did the same. Hopefully it will be fixed altogether\n> later, or are you positive I better resend the patch with quotes? \n> \n> >> \n> >> -q::\n> >> --quiet::\n> >> diff --git a/builtin/clean.c b/builtin/clean.c\n> >> index d90766cad3a0..fcc50d08ee9b 100644\n> >> --- a/builtin/clean.c\n> >> +++ b/builtin/clean.c\n> >> @@ -25,7 +25,7 @@\n> >> #include \"help.h\"\n> >> #include \"prompt.h\"\n> >> \n> >> -static int force = -1; /* unset */\n> >> +static int require_force = -1; /* unset */\n> >> static int interactive;\n> >> static struct string_list del_list = STRING_LIST_INIT_DUP;\n> >> static unsigned int colopts;\n> >> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const \nchar *value,\n> >> \t}\n> >> \n> >> \tif (!strcmp(var, \"clean.requireforce\")) {\n> >> -\t\tforce = !git_config_bool(var, value);\n> >> +\t\trequire_force = git_config_bool(var, value);\n> >> \t\treturn 0;\n> >> \t}\n> >> \n> >> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char \n*prefix)\n> >> {\n> >> \tint i, res;\n> >> \tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n> >> -\tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n> >> +\tint ignored_only = 0, force = 0, errors = 0, gone = 1;\n> >> \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n> >> \tstruct strbuf abs_path = STRBUF_INIT;\n> >> \tstruct dir_struct dir = DIR_INIT;\n> >> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const \nchar *prefix)\n> >> \t};\n> >> \n> >> \tgit_config(git_clean_config, NULL);\n> >> -\tif (force < 0)\n> >> -\t\tforce = 0;\n> >> -\telse\n> >> -\t\tconfig_set = 1;\n> >> \n> >> \targc = parse_options(argc, argv, prefix, options, \nbuiltin_clean_usage,\n> >> \t\t\t 0);\n> >> \n> >> -\tif (!interactive && !dry_run && !force) {\n> >> -\t\tif (config_set)\n> >> -\t\t\tdie(_(\"clean.requireForce set to true and \nneither -i, -n, nor -f given; \"\n> >> +\t/* Dry run won't remove anything, so requiring force makes no \nsense */\n> >> +\tif(dry_run)\n> >> +\t\trequire_force = 0;\n> >> +\n> >> +\tif (!force && !interactive) {\n> >> +\t\tif (require_force > 0)\n> >> +\t\t\tdie(_(\"clean.requireForce set to true and \nneither -f, nor -i given; \"\n> >> +\t\t\t\t \"refusing to clean\"));\n> >> +\t\telse if (require_force < 0)\n> >> +\t\t\tdie(_(\"clean.requireForce defaults to true \nand neither -f, nor -i given; \"\n> >> \t\t\t\t \"refusing to clean\"));\n> >> -\t\telse\n> >> -\t\t\tdie(_(\"clean.requireForce defaults to true \nand neither -i, -n, nor -f given;\"\n> >> -\t\t\t\t \" refusing to clean\"));\n> >> \t}\n> >> \n> >\n> > The last two cases can be coalesced into a single case (the last one),\n> > because the difference in the messages does not bring more information\n> > to the user.\n> \n> Did you misread the patch? There are only 2 cases here, the last (third)\n> one is marked with '-' (removed). Too easy to misread this, I'd say. New\n> code is:\n> \n> \t\tif (require_force > 0)\n> \t\t\tdie(_(\"clean.requireForce set to true and \nneither -f, nor -i given; \"\n> \t\t\t\t \"refusing to clean\"));\n> \t\telse if (require_force < 0)\n> \t\t\tdie(_(\"clean.requireForce defaults to true \nand neither -f, nor -i given; \"\n> \n> and is basically unchanged from the original, except reference to '-n' has \nbeen\n> removed. Btw, is now comma needed after -f, and isn't it better to\n> substitute ':' for ';'?\n> \n> Thank you for review!\n> \n> -- Sergey Organov\n> \n> \n\nOh, sorry, I misinterpreted the patch. But yet, I'm not sure that specifying \nthat this is the default or not is really useful. If the configuration was set \nto true, it is was a no-op. If set to false, no message will appear.\n\n\n\n\n"},{"id":"489794","messageId":"87wmqk8kw0.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqqv864zjbf.fsf@gitster.g","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-02T19:59:59Z","receivedAt":"2024-03-02T20:00:03Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> What -n actually does in addition to its documented behavior is\n>> ignoring of configuration variable clean.requireForce, that makes\n>> sense provided -n prevents files removal anyway.\n>\n> There is another thing I noticed.\n>\n> This part to get rid of \"config_set\" does make sense.\n>\n>> \tgit_config(git_clean_config, NULL);\n>> -\tif (force < 0)\n>> -\t\tforce = 0;\n>> -\telse\n>> -\t\tconfig_set = 1;\n>\n> We used to think \"force\" variable is the master switch to do\n> anything , and requireForce configuration was a way to flip its\n> default to 0 (so that you need to set it to 1 again from the command\n> line). This separates \"force\" (which can only given via the command\n> line) and \"require_force\" (which controls when the \"force\" is used)\n> and makes the logic simpler.\n>\n>> \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n>> \t\t\t 0);\n>\n> However.\n>\n>> -\tif (!interactive && !dry_run && !force) {\n>> -\t\tif (config_set)\n>> -\t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\n>> +\t/* Dry run won't remove anything, so requiring force makes no sense */\n>> +\tif(dry_run)\n>> +\t\trequire_force = 0;\n>\n> I am not sure if this is making things inconsistent.\n\nI believe things rather got more consistent, see below.\n\n>\n> Dry run will be harmless, and we can be lenient and not require\n> force. But below, we do not require force when going interactive,\n> either.\n\nExcept, unlike dry-run, interactive is not harmless, similar to -f.\n\n> So we could instead add\n>\n> \tif (dry_run || interactive)\n> \t\trequire_force = 0;\n>\n> above, drop the \"&& !interactive\" from the guard for the\n> clean.requireForce block.\n\nThat'd be less consistent, as dry-run is harmless, whereas neither force\nnor interactive are.\n\n> Or we can go the opposite way. We do not have to tweak\n> require_force at all based on other conditions. Instead we can\n> update the guard below to check \"!force && !interactive && !dry_run\"\n> before entering the clean.requireForce block, no?\n\nNo, we do need to tweak require_force, as another if() that is inside\nand produces error message does in fact check for require_force being\neither negative or positive, i.e., non-zero.\n\n>\n> But the code after this patch makes me feel that it is somewhere in\n> the middle between these two optimum places.\n\nI believe it's rather right in the spot. I left '-i' to stay with '-f',\nas it was before the patch, as both are very distinct (even if in\ndifferent manner) when compared to '-n', so now only '-n' is now treated\nseparately.\n\nThe very idea of dry-run is that it is orthogonal to any other behavior,\nso if I were designing it, I'd left bailing-out without -f or -i in\nplace even if -n were given, to show what exactly would happen without\n-n. With new code it'd be as simple as removing \"if (dry_run)\nrequire_force = 0\" line that introduces the original dependency.\n\n>\n> Another thing. Stepping back and thinking _why_ the code can treat\n> dry_run and interactive the same way (either to make them drop\n> require_force above, or neither of them contributes to the value of\n> require_force), if we are dropping \"you didn't give me --dry-run\" in\n> the error message below, we should also drop \"you didn't give me\n> --interactive, either\" as well, when complaining about the lack of\n> \"--force\".\n\nIn fact, the new code rather keep treating -f and -i somewhat similarly,\nrather than -i and -n, intentionally.\n\nThat said, if somebody is going to re-consider -f vs -i issue, they now\nhave more cleaner code that doesn't involve -n anymore.\n\n> One possible objection I can think of against doing so is that it\n> might not be so obvious why \"interactive\" does not have to require\n> \"force\" (even though it is clearly obvious to me). But if that were\n> the objection, then to somebody else \"dry-run does not have to\n> require force\" may equally not be so obvious (at least it wasn't so\n> obvious to me during the last round of this discussion).\n\nI'm not sure about interactive not requiring force, and I intentionally\navoided this issue in the patch in question, though I think the patch\nmakes it easier to reason about -i vs -f in the future by removing -n\nhandling from the picture.\n\n>\n> So I can live without the \"drop 'nor -i'\" part I suggested in the\n> above. We would not drop \"nor -i\" and add \"nor --dry-run\" back to\n> the message instead.\n\nI'm afraid we can't meaningfully keep -n (--dry-run) in the messages. As\nit stands, having -n there was a mistake right from the beginning.\nPlease consider the original message, but without -i and -f, for the\nsake of the argument:\n\n \"clean.requireForce set to true and -n is not given; refusing to clean\"\n\nto me it sounds like nonsense, as it suggests that if were given -n,\nwe'd perform cleanup, that is simply false as no cleanup is ever\nperformed once -n is there. Adding -i and -f back to the message\nsomewhat blurs the problem, yet -n still does not belong there.\n\n> So from that angle, the message after this patch makes me feel that\n> it is somewhere in the middle between two more sensible places.\n\nI don't think so, see above. I rather believe that even if everything\nelse in the patch were denied, the -n should be removed from the error\nmessage, so I did exactly that, and only that (i.e., didn't merge 2\nmessages into one).\n\nThanks,\n-- Sergey Organov\n"},{"id":"489796","messageId":"87r0gs8kgw.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"6033073.lOV4Wx5bFT@cayenne","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-02T20:09:03Z","receivedAt":"2024-03-02T20:09:07Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jean-Noël AVILA writes:\n\n> On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote:\n>> Jean-Noël Avila writes:\n>> \n>> > Putting my documentation/translator hat:\n>> >\n>> > Le 29/02/2024 à 20:07, Sergey Organov a écrit :\n>> >> What -n actually does in addition to its documented behavior is\n>> >> ignoring of configuration variable clean.requireForce, that makes\n>> >> sense provided -n prevents files removal anyway.\n>> >> \n>> >> So, first, document this in the manual, and then modify implementation\n>> >> to make this more explicit in the code.\n>> >> \n>> >> Improved implementation also stops to share single internal variable\n>> >> 'force' between command-line -f option and configuration variable\n>> >> clean.requireForce, resulting in more clear logic.\n>> >> \n>> >> The error messages now do not mention -n as well, as it seems\n>> >> unnecessary and does not reflect clarified implementation.\n>> >> \n>> >> Signed-off-by: Sergey Organov \n>> >> ---\n>> >> Documentation/git-clean.txt | 2 ++\n>> >> builtin/clean.c | 26 +++++++++++++-------------\n>> >> 2 files changed, 15 insertions(+), 13 deletions(-)\n>> >> \n>> >> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\n>> >> index 69331e3f05a1..662eebb85207 100644\n>> >> --- a/Documentation/git-clean.txt\n>> >> +++ b/Documentation/git-clean.txt\n>> >> @@ -49,6 +49,8 @@ OPTIONS\n>> >> -n::\n>> >> --dry-run::\n>> >> \tDon't actually remove anything, just show what would be done.\n>> >> +\tConfiguration variable clean.requireForce is ignored, as\n>> >> +\tnothing will be deleted anyway.\n>> >\n>> > Please use backticks for options, configuration and environment names:\n>> > `clean.requireForce`\n>> \n>> I did consider this. However, existing text already has exactly this one\n>> unquoted, so I just did the same. Hopefully it will be fixed altogether\n>> later, or are you positive I better resend the patch with quotes? \n>> \n>> >> \n>> >> -q::\n>> >> --quiet::\n>> >> diff --git a/builtin/clean.c b/builtin/clean.c\n>> >> index d90766cad3a0..fcc50d08ee9b 100644\n>> >> --- a/builtin/clean.c\n>> >> +++ b/builtin/clean.c\n>> >> @@ -25,7 +25,7 @@\n>> >> #include \"help.h\"\n>> >> #include \"prompt.h\"\n>> >> \n>> >> -static int force = -1; /* unset */\n>> >> +static int require_force = -1; /* unset */\n>> >> static int interactive;\n>> >> static struct string_list del_list = STRING_LIST_INIT_DUP;\n>> >> static unsigned int colopts;\n>> >> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const \n> char *value,\n>> >> \t}\n>> >> \n>> >> \tif (!strcmp(var, \"clean.requireforce\")) {\n>> >> -\t\tforce = !git_config_bool(var, value);\n>> >> +\t\trequire_force = git_config_bool(var, value);\n>> >> \t\treturn 0;\n>> >> \t}\n>> >> \n>> >> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char \n> *prefix)\n>> >> {\n>> >> \tint i, res;\n>> >> \tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n>> >> -\tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n>> >> +\tint ignored_only = 0, force = 0, errors = 0, gone = 1;\n>> >> \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n>> >> \tstruct strbuf abs_path = STRBUF_INIT;\n>> >> \tstruct dir_struct dir = DIR_INIT;\n>> >> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const \n> char *prefix)\n>> >> \t};\n>> >> \n>> >> \tgit_config(git_clean_config, NULL);\n>> >> -\tif (force < 0)\n>> >> -\t\tforce = 0;\n>> >> -\telse\n>> >> -\t\tconfig_set = 1;\n>> >> \n>> >> \targc = parse_options(argc, argv, prefix, options, \n> builtin_clean_usage,\n>> >> \t\t\t 0);\n>> >> \n>> >> -\tif (!interactive && !dry_run && !force) {\n>> >> -\t\tif (config_set)\n>> >> -\t\t\tdie(_(\"clean.requireForce set to true and \n> neither -i, -n, nor -f given; \"\n>> >> +\t/* Dry run won't remove anything, so requiring force makes no \n> sense */\n>> >> +\tif(dry_run)\n>> >> +\t\trequire_force = 0;\n>> >> +\n>> >> +\tif (!force && !interactive) {\n>> >> +\t\tif (require_force > 0)\n>> >> +\t\t\tdie(_(\"clean.requireForce set to true and \n> neither -f, nor -i given; \"\n>> >> +\t\t\t\t \"refusing to clean\"));\n>> >> +\t\telse if (require_force < 0)\n>> >> +\t\t\tdie(_(\"clean.requireForce defaults to true \n> and neither -f, nor -i given; \"\n>> >> \t\t\t\t \"refusing to clean\"));\n>> >> -\t\telse\n>> >> -\t\t\tdie(_(\"clean.requireForce defaults to true \n> and neither -i, -n, nor -f given;\"\n>> >> -\t\t\t\t \" refusing to clean\"));\n>> >> \t}\n>> >> \n>> >\n>> > The last two cases can be coalesced into a single case (the last one),\n>> > because the difference in the messages does not bring more information\n>> > to the user.\n>> \n>> Did you misread the patch? There are only 2 cases here, the last (third)\n>> one is marked with '-' (removed). Too easy to misread this, I'd say. New\n>> code is:\n>> \n>> \t\tif (require_force > 0)\n>> \t\t\tdie(_(\"clean.requireForce set to true and \n> neither -f, nor -i given; \"\n>> \t\t\t\t \"refusing to clean\"));\n>> \t\telse if (require_force < 0)\n>> \t\t\tdie(_(\"clean.requireForce defaults to true \n> and neither -f, nor -i given; \"\n>> \n>> and is basically unchanged from the original, except reference to '-n' has \n> been\n>> removed. Btw, is now comma needed after -f, and isn't it better to\n>> substitute ':' for ';'?\n>> \n>> Thank you for review!\n>> \n>> -- Sergey Organov\n>> \n>> \n>\n> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that\n> specifying that this is the default or not is really useful. If the\n> configuration was set to true, it is was a no-op. If set to false, no\n> message will appear.\n\nI'm not sure either, and as it's not the topic of this particular patch,\nI'd like to delegate the decision on the issue.\n"},{"id":"489797","messageId":"xmqqwmqkwdef.fsf@gitster.g","threadId":"60711","inReplyTo":"87r0gs8kgw.fsf@osv.gnss.ru","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-02T21:07:52Z","receivedAt":"2024-03-02T21:07:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that\n>> specifying that this is the default or not is really useful. If the\n>> configuration was set to true, it is was a no-op. If set to false, no\n>> message will appear.\n>\n> I'm not sure either, and as it's not the topic of this particular patch,\n> I'd like to delegate the decision on the issue.\n\nIt is very much spot on the topic of simplifying and clarifying the\ncode to unify these remaining two messages into a single one.\n\nAnd involving the --interactive that allows users a chance to\nrethink and refrain from removing some to the equation would also be\nworth doing in the same topic, even though it might not fit your\nimmediate agenda of crusade against --dry-run.\n"},{"id":"489800","messageId":"87bk7w8aae.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"xmqqwmqkwdef.fsf@gitster.g","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-02T23:48:57Z","receivedAt":"2024-03-02T23:49:00Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that\n>>> specifying that this is the default or not is really useful. If the\n>>> configuration was set to true, it is was a no-op. If set to false, no\n>>> message will appear.\n>>\n>> I'm not sure either, and as it's not the topic of this particular patch,\n>> I'd like to delegate the decision on the issue.\n>\n> It is very much spot on the topic of simplifying and clarifying the\n> code to unify these remaining two messages into a single one.\n\nI'm inclined to be more against merging than for it, as for me it'd be\nconfusing to be told that a configuration variable is set to true when I\ndidn't set it, nor there is any way to figure where it is set, because\nin fact it isn't, and it's rather the default that is in use.\n\nOverall, to me the messages are fine as they are (except -n that doesn't\nbelong there), I don't see compelling reason to hide information from\nthe user, and thus I won't propose patch that gets rid of one of them.\n\n> And involving the --interactive that allows users a chance to\n> rethink and refrain from removing some to the equation would also be\n> worth doing in the same topic,\n\nWorth doing what? I'm afraid I lost the plot here, as --interactive\nstill looks fine to me.\n\n> even though it might not fit your immediate agenda of crusade against\n> --dry-run.\n\nI'm hopefully crusading for --dry-run, not against, trying to get rid of\nthe cause of the original confusion that started -n/-f controversy.\n\nThanks,\n-- Sergey Organov\n"},{"id":"489803","messageId":"87le6ziqzb.fsf_-_@osv.gnss.ru","threadId":"60711","inReplyTo":"875xy76qe1.fsf@osv.gnss.ru","subject":"[PATCH v2] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-03T09:50:32Z","receivedAt":"2024-03-03T09:50:36Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"What -n actually does in addition to its documented behavior is\nignoring of configuration variable clean.requireForce, that makes\nsense provided -n prevents files removal anyway.\n\nSo, first, document this in the manual, and then modify implementation\nto make this more explicit in the code.\n\nImproved implementation also stops to share single internal variable\n'force' between command-line -f option and configuration variable\nclean.requireForce, resulting in more clear logic.\n\nTwo error messages with slightly different text depending on if\nclean.requireForce was explicitly set or not, are merged into a single\none.\n\nThe resulting error message now does not mention -n as well, as it\nneither matches intended clean.requireForce usage nor reflects\nclarified implementation.\n\nDocumentation of clean.requireForce is changed accordingly.\n\nSigned-off-by: Sergey Organov \n---\n\nChanges since v1:\n\n * Fixed style of the if() statement\n\n * Merged two error messages into one\n\n * clean.requireForce description changed accordingly\n\n Documentation/config/clean.txt | 4 ++--\n Documentation/git-clean.txt | 2 ++\n builtin/clean.c | 23 +++++++++--------------\n 3 files changed, 13 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config/clean.txt b/Documentation/config/clean.txt\nindex f05b9403b5ad..b19ca210f39b 100644\n--- a/Documentation/config/clean.txt\n+++ b/Documentation/config/clean.txt\n@@ -1,3 +1,3 @@\n clean.requireForce::\n-\tA boolean to make git-clean do nothing unless given -f,\n-\t-i, or -n. Defaults to true.\n+\tA boolean to make git-clean refuse to delete files unless -f\n+\tor -i is given. Defaults to true.\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex 69331e3f05a1..662eebb85207 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -49,6 +49,8 @@ OPTIONS\n -n::\n --dry-run::\n \tDon't actually remove anything, just show what would be done.\n+\tConfiguration variable clean.requireForce is ignored, as\n+\tnothing will be deleted anyway.\n \n -q::\n --quiet::\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d90766cad3a0..41502dcb0dde 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -25,7 +25,7 @@\n #include \"help.h\"\n #include \"prompt.h\"\n \n-static int force = -1; /* unset */\n+static int require_force = -1; /* unset */\n static int interactive;\n static struct string_list del_list = STRING_LIST_INIT_DUP;\n static unsigned int colopts;\n@@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"clean.requireforce\")) {\n-\t\tforce = !git_config_bool(var, value);\n+\t\trequire_force = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n {\n \tint i, res;\n \tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n-\tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n+\tint ignored_only = 0, force = 0, errors = 0, gone = 1;\n \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n \tstruct strbuf abs_path = STRBUF_INIT;\n \tstruct dir_struct dir = DIR_INIT;\n@@ -946,22 +946,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t};\n \n \tgit_config(git_clean_config, NULL);\n-\tif (force < 0)\n-\t\tforce = 0;\n-\telse\n-\t\tconfig_set = 1;\n \n \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n \t\t\t 0);\n \n-\tif (!interactive && !dry_run && !force) {\n-\t\tif (config_set)\n-\t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\n-\t\t\t\t \"refusing to clean\"));\n-\t\telse\n-\t\t\tdie(_(\"clean.requireForce defaults to true and neither -i, -n, nor -f given;\"\n+\t/* Dry run won't remove anything, so requiring force makes no sense */\n+\tif (dry_run)\n+\t\trequire_force = 0;\n+\n+\tif (require_force != 0 && !force && !interactive)\n+\t\tdie(_(\"clean.requireForce is true and neither -f nor -i given:\"\n \t\t\t\t \" refusing to clean\"));\n-\t}\n \n \tif (force > 1)\n \t\trm_flags = 0;\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n-- \n2.25.1\n\n"},{"id":"489804","messageId":"87h6hniqtf.fsf@osv.gnss.ru","threadId":"60711","inReplyTo":"87bk7w8aae.fsf@osv.gnss.ru","subject":"Re: [PATCH] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-03T09:54:04Z","receivedAt":"2024-03-03T09:54:07Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Sergey Organov writes:\n\n> Junio C Hamano writes:\n>\n>> Sergey Organov writes:\n>>\n>>>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that\n>>>> specifying that this is the default or not is really useful. If the\n>>>> configuration was set to true, it is was a no-op. If set to false, no\n>>>> message will appear.\n>>>\n>>> I'm not sure either, and as it's not the topic of this particular patch,\n>>> I'd like to delegate the decision on the issue.\n>>\n>> It is very much spot on the topic of simplifying and clarifying the\n>> code to unify these remaining two messages into a single one.\n>\n> I'm inclined to be more against merging than for it, as for me it'd be\n> confusing to be told that a configuration variable is set to true when I\n> didn't set it, nor there is any way to figure where it is set, because\n> in fact it isn't, and it's rather the default that is in use.\n>\n> Overall, to me the messages are fine as they are (except -n that doesn't\n> belong there), I don't see compelling reason to hide information from\n> the user, and thus I won't propose patch that gets rid of one of them.\n\nNevertheless, as others are in favor of unification, I've merged these\ntwo messages in the v2 version of the patch, which see.\n\nThanks,\n-- Sergey Organov\n"}]}