| Message ID | 875xy76qe1.fsf@osv.gnss.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clean: improve -n and -f implementation and documentation | expand |
Putting my documentation/translator hat: Le 29/02/2024 à 20:07, Sergey Organov a écrit : > What -n actually does in addition to its documented behavior is > ignoring of configuration variable clean.requireForce, that makes > sense provided -n prevents files removal anyway. > > So, first, document this in the manual, and then modify implementation > to make this more explicit in the code. > > Improved implementation also stops to share single internal variable > 'force' between command-line -f option and configuration variable > clean.requireForce, resulting in more clear logic. > > The error messages now do not mention -n as well, as it seems > unnecessary and does not reflect clarified implementation. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > Documentation/git-clean.txt | 2 ++ > builtin/clean.c | 26 +++++++++++++------------- > 2 files changed, 15 insertions(+), 13 deletions(-) > > diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt > index 69331e3f05a1..662eebb85207 100644 > --- a/Documentation/git-clean.txt > +++ b/Documentation/git-clean.txt > @@ -49,6 +49,8 @@ OPTIONS > -n:: > --dry-run:: > Don't actually remove anything, just show what would be done. > + Configuration variable clean.requireForce is ignored, as > + nothing will be deleted anyway. Please use backticks for options, configuration and environment names: `clean.requireForce` > > -q:: > --quiet:: > diff --git a/builtin/clean.c b/builtin/clean.c > index d90766cad3a0..fcc50d08ee9b 100644 > --- a/builtin/clean.c > +++ b/builtin/clean.c > @@ -25,7 +25,7 @@ > #include "help.h" > #include "prompt.h" > > -static int force = -1; /* unset */ > +static int require_force = -1; /* unset */ > static int interactive; > static struct string_list del_list = STRING_LIST_INIT_DUP; > static unsigned int colopts; > @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value, > } > > if (!strcmp(var, "clean.requireforce")) { > - force = !git_config_bool(var, value); > + require_force = git_config_bool(var, value); > return 0; > } > > @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix) > { > int i, res; > int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0; > - int ignored_only = 0, config_set = 0, errors = 0, gone = 1; > + int ignored_only = 0, force = 0, errors = 0, gone = 1; > int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT; > struct strbuf abs_path = STRBUF_INIT; > struct dir_struct dir = DIR_INIT; > @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix) > }; > > git_config(git_clean_config, NULL); > - if (force < 0) > - force = 0; > - else > - config_set = 1; > > argc = parse_options(argc, argv, prefix, options, builtin_clean_usage, > 0); > > - if (!interactive && !dry_run && !force) { > - if (config_set) > - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; " > + /* Dry run won't remove anything, so requiring force makes no sense */ > + if(dry_run) > + require_force = 0; > + > + if (!force && !interactive) { > + if (require_force > 0) > + die(_("clean.requireForce set to true and neither -f, nor -i given; " > + "refusing to clean")); > + else if (require_force < 0) > + die(_("clean.requireForce defaults to true and neither -f, nor -i given; " > "refusing to clean")); > - else > - die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;" > - " refusing to clean")); > } > The last two cases can be coalesced into a single case (the last one), because the difference in the messages does not bring more information to the user. > if (force > 1) > > base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
Jean-Noël Avila <avila.jn@gmail.com> writes: > Putting my documentation/translator hat: > > Le 29/02/2024 à 20:07, Sergey Organov a écrit : >> What -n actually does in addition to its documented behavior is >> ignoring of configuration variable clean.requireForce, that makes >> sense provided -n prevents files removal anyway. >> >> So, first, document this in the manual, and then modify implementation >> to make this more explicit in the code. >> >> Improved implementation also stops to share single internal variable >> 'force' between command-line -f option and configuration variable >> clean.requireForce, resulting in more clear logic. >> >> The error messages now do not mention -n as well, as it seems >> unnecessary and does not reflect clarified implementation. >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> Documentation/git-clean.txt | 2 ++ >> builtin/clean.c | 26 +++++++++++++------------- >> 2 files changed, 15 insertions(+), 13 deletions(-) >> >> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt >> index 69331e3f05a1..662eebb85207 100644 >> --- a/Documentation/git-clean.txt >> +++ b/Documentation/git-clean.txt >> @@ -49,6 +49,8 @@ OPTIONS >> -n:: >> --dry-run:: >> Don't actually remove anything, just show what would be done. >> + Configuration variable clean.requireForce is ignored, as >> + nothing will be deleted anyway. > > Please use backticks for options, configuration and environment names: > `clean.requireForce` I did consider this. However, existing text already has exactly this one unquoted, so I just did the same. Hopefully it will be fixed altogether later, or are you positive I better resend the patch with quotes? >> >> -q:: >> --quiet:: >> diff --git a/builtin/clean.c b/builtin/clean.c >> index d90766cad3a0..fcc50d08ee9b 100644 >> --- a/builtin/clean.c >> +++ b/builtin/clean.c >> @@ -25,7 +25,7 @@ >> #include "help.h" >> #include "prompt.h" >> >> -static int force = -1; /* unset */ >> +static int require_force = -1; /* unset */ >> static int interactive; >> static struct string_list del_list = STRING_LIST_INIT_DUP; >> static unsigned int colopts; >> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value, >> } >> >> if (!strcmp(var, "clean.requireforce")) { >> - force = !git_config_bool(var, value); >> + require_force = git_config_bool(var, value); >> return 0; >> } >> >> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix) >> { >> int i, res; >> int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0; >> - int ignored_only = 0, config_set = 0, errors = 0, gone = 1; >> + int ignored_only = 0, force = 0, errors = 0, gone = 1; >> int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT; >> struct strbuf abs_path = STRBUF_INIT; >> struct dir_struct dir = DIR_INIT; >> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix) >> }; >> >> git_config(git_clean_config, NULL); >> - if (force < 0) >> - force = 0; >> - else >> - config_set = 1; >> >> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage, >> 0); >> >> - if (!interactive && !dry_run && !force) { >> - if (config_set) >> - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; " >> + /* Dry run won't remove anything, so requiring force makes no sense */ >> + if(dry_run) >> + require_force = 0; >> + >> + if (!force && !interactive) { >> + if (require_force > 0) >> + die(_("clean.requireForce set to true and neither -f, nor -i given; " >> + "refusing to clean")); >> + else if (require_force < 0) >> + die(_("clean.requireForce defaults to true and neither -f, nor -i given; " >> "refusing to clean")); >> - else >> - die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;" >> - " refusing to clean")); >> } >> > > The last two cases can be coalesced into a single case (the last one), > because the difference in the messages does not bring more information > to the user. Did you misread the patch? There are only 2 cases here, the last (third) one is marked with '-' (removed). Too easy to misread this, I'd say. New code is: if (require_force > 0) die(_("clean.requireForce set to true and neither -f, nor -i given; " "refusing to clean")); else if (require_force < 0) die(_("clean.requireForce defaults to true and neither -f, nor -i given; " and is basically unchanged from the original, except reference to '-n' has been removed. Btw, is now comma needed after -f, and isn't it better to substitute ':' for ';'? Thank you for review! -- Sergey Organov
On Fri, Mar 1, 2024, at 15:34, Sergey Organov wrote: >> Please use backticks for options, configuration and environment names: >> `clean.requireForce` > > I did consider this. However, existing text already has exactly this one > unquoted, so I just did the same. Hopefully it will be fixed altogether > later, or are you positive I better resend the patch with quotes? Sometimes I see widespread changes (like formatting many files) get rejected because it is considered _churn_. Not fixing this in this series and then maybe someone else fixing it later seems like churn as well. Isn’t it better to fix it while you are changing the text?
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > On Fri, Mar 1, 2024, at 15:34, Sergey Organov wrote: >>> Please use backticks for options, configuration and environment names: >>> `clean.requireForce` >> >> I did consider this. However, existing text already has exactly this one >> unquoted, so I just did the same. Hopefully it will be fixed altogether >> later, or are you positive I better resend the patch with quotes? > > Sometimes I see widespread changes (like formatting many files) get > rejected because it is considered _churn_. Not fixing this in this > series and then maybe someone else fixing it later seems like churn as > well. Isn’t it better to fix it while you are changing the text? Any one of these is fine: (1) add the new paragraph with mark-up consistent with existing text (which is what Sergey did). (2) add the new paragraph with correct mark-up, making the document less consistent overall. (3) have one patch to fix broken mark-up of existing text, followed by another patch to add the new paragraph with correct mark-up. If you take one of the first two, it would be a very good idea to have a comment in the proposed log message to note the need for later clean-up. Without being written down anywhere, your discovery and the brain cycles you spent while deciding what to do will be wasted, which is not what you want. Thanks, both.
Jean-Noël Avila <avila.jn@gmail.com> writes: >> + /* Dry run won't remove anything, so requiring force makes no sense */ >> + if(dry_run) >> + require_force = 0; Style. "if (dry_run)". Getting rid of "config_set", which was an extra variable that kept track of where "force" came from, does make the logic cleaner, I guess. What we want to happen is that one of -i/-n/-f is required when clean.requireForce is *not* unset (i.e. 0 <= require_force). >> + if (!force && !interactive) { The require-force takes effect only when neither force or interactive is given, so the new code structure puts the above obvious conditional around "do we complain due to requireForce?" logic. Sensible. >> + if (require_force > 0) >> + die(_("clean.requireForce set to true and neither -f, nor -i given; " >> + "refusing to clean")); If it is explicitly set, we get this message. And ... >> + else if (require_force < 0) >> + die(_("clean.requireForce defaults to true and neither -f, nor -i given; " >> "refusing to clean")); ... if it is set due to default (in other words, if it is not unset), we get this message. As you said, I do not think it matters too much either way to the end-users where the truth setting of clean.requireForce came from, either due to the default or the user explicitly configuring. So unifying to a single message may be helpful to both readers and translators. clean.requireForce is true; unless interactive, -f is required might be a bit shorter and more to the point. > The last two cases can be coalesced into a single case (the last one), > because the difference in the messages does not bring more information > to the user. Yeah. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Getting rid of "config_set", which was an extra variable that kept > track of where "force" came from, does make the logic cleaner, I > guess. What we want to happen is that one of -i/-n/-f is required > when clean.requireForce is *not* unset (i.e. 0 <= require_force). Oh, noes. require_force is unset explicitly it would be 0, so this should have read (i.e. require_force != 0). Sorry for a thinko.
Junio C Hamano <gitster@pobox.com> writes: > Jean-Noël Avila <avila.jn@gmail.com> writes: > >>> + /* Dry run won't remove anything, so requiring force makes no sense */ >>> + if(dry_run) >>> + require_force = 0; > > Style. "if (dry_run)". Ooops! > > Getting rid of "config_set", which was an extra variable that kept > track of where "force" came from, does make the logic cleaner, I > guess. What we want to happen is that one of -i/-n/-f is required > when clean.requireForce is *not* unset (i.e. 0 <= require_force). > >>> + if (!force && !interactive) { > > The require-force takes effect only when neither force or > interactive is given, so the new code structure puts the above > obvious conditional around "do we complain due to requireForce?" > logic. Sensible. > >>> + if (require_force > 0) >>> + die(_("clean.requireForce set to true and neither -f, nor -i given; " >>> + "refusing to clean")); > > If it is explicitly set, we get this message. And ... > >>> + else if (require_force < 0) >>> + die(_("clean.requireForce defaults to true and neither -f, nor -i given; " >>> "refusing to clean")); > > ... if it is set due to default (in other words, if it is not unset), we > get this message. > > As you said, I do not think it matters too much either way to the > end-users where the truth setting of clean.requireForce came from, > either due to the default or the user explicitly configuring. So > unifying to a single message may be helpful to both readers and > translators. > > clean.requireForce is true; unless interactive, -f is required > > might be a bit shorter and more to the point. Dunno, I tried to keep changes to the bare sensible minimum, especially to avoid possible controversy. I'd leave this for somebody else to decide upon and patch, if they feel like it. >> The last two cases can be coalesced into a single case (the last one), >> because the difference in the messages does not bring more information >> to the user. > > Yeah. "The last two cases" sounds like there are more of them, and there is none, so to me it sounded like patch misread. Maybe I'm wrong. Thanks for the review! -- Sergey Organov
Sergey Organov <sorganov@gmail.com> writes: > What -n actually does in addition to its documented behavior is > ignoring of configuration variable clean.requireForce, that makes > sense provided -n prevents files removal anyway. There is another thing I noticed. This part to get rid of "config_set" does make sense. > git_config(git_clean_config, NULL); > - if (force < 0) > - force = 0; > - else > - config_set = 1; We used to think "force" variable is the master switch to do anything , and requireForce configuration was a way to flip its default to 0 (so that you need to set it to 1 again from the command line). This separates "force" (which can only given via the command line) and "require_force" (which controls when the "force" is used) and makes the logic simpler. > argc = parse_options(argc, argv, prefix, options, builtin_clean_usage, > 0); However. > - if (!interactive && !dry_run && !force) { > - if (config_set) > - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; " > + /* Dry run won't remove anything, so requiring force makes no sense */ > + if(dry_run) > + require_force = 0; I am not sure if this is making things inconsistent. Dry run will be harmless, and we can be lenient and not require force. But below, we do not require force when going interactive, either. So we could instead add if (dry_run || interactive) require_force = 0; above, drop the "&& !interactive" from the guard for the clean.requireForce block. Or we can go the opposite way. We do not have to tweak require_force at all based on other conditions. Instead we can update the guard below to check "!force && !interactive && !dry_run" before entering the clean.requireForce block, no? But the code after this patch makes me feel that it is somewhere in the middle between these two optimum places. Another thing. Stepping back and thinking _why_ the code can treat dry_run and interactive the same way (either to make them drop require_force above, or neither of them contributes to the value of require_force), if we are dropping "you didn't give me --dry-run" in the error message below, we should also drop "you didn't give me --interactive, either" as well, when complaining about the lack of "--force". One possible objection I can think of against doing so is that it might not be so obvious why "interactive" does not have to require "force" (even though it is clearly obvious to me). But if that were the objection, then to somebody else "dry-run does not have to require force" may equally not be so obvious (at least it wasn't so obvious to me during the last round of this discussion). So I can live without the "drop 'nor -i'" part I suggested in the above. We would not drop "nor -i" and add "nor --dry-run" back to the message instead. So from that angle, the message after this patch makes me feel that it is somewhere in the middle between two more sensible places. > + if (!force && !interactive) { > + if (require_force > 0) > + die(_("clean.requireForce set to true and neither -f, nor -i given; " > + "refusing to clean")); > + else if (require_force < 0) > + die(_("clean.requireForce defaults to true and neither -f, nor -i given; " > "refusing to clean")); > - else > - die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;" > - " refusing to clean")); > } > > if (force > 1) > > base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote: > Jean-Noël Avila <avila.jn@gmail.com> writes: > > > Putting my documentation/translator hat: > > > > Le 29/02/2024 à 20:07, Sergey Organov a écrit : > >> What -n actually does in addition to its documented behavior is > >> ignoring of configuration variable clean.requireForce, that makes > >> sense provided -n prevents files removal anyway. > >> > >> So, first, document this in the manual, and then modify implementation > >> to make this more explicit in the code. > >> > >> Improved implementation also stops to share single internal variable > >> 'force' between command-line -f option and configuration variable > >> clean.requireForce, resulting in more clear logic. > >> > >> The error messages now do not mention -n as well, as it seems > >> unnecessary and does not reflect clarified implementation. > >> > >> Signed-off-by: Sergey Organov <sorganov@gmail.com> > >> --- > >> Documentation/git-clean.txt | 2 ++ > >> builtin/clean.c | 26 +++++++++++++------------- > >> 2 files changed, 15 insertions(+), 13 deletions(-) > >> > >> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt > >> index 69331e3f05a1..662eebb85207 100644 > >> --- a/Documentation/git-clean.txt > >> +++ b/Documentation/git-clean.txt > >> @@ -49,6 +49,8 @@ OPTIONS > >> -n:: > >> --dry-run:: > >> Don't actually remove anything, just show what would be done. > >> + Configuration variable clean.requireForce is ignored, as > >> + nothing will be deleted anyway. > > > > Please use backticks for options, configuration and environment names: > > `clean.requireForce` > > I did consider this. However, existing text already has exactly this one > unquoted, so I just did the same. Hopefully it will be fixed altogether > later, or are you positive I better resend the patch with quotes? > > >> > >> -q:: > >> --quiet:: > >> diff --git a/builtin/clean.c b/builtin/clean.c > >> index d90766cad3a0..fcc50d08ee9b 100644 > >> --- a/builtin/clean.c > >> +++ b/builtin/clean.c > >> @@ -25,7 +25,7 @@ > >> #include "help.h" > >> #include "prompt.h" > >> > >> -static int force = -1; /* unset */ > >> +static int require_force = -1; /* unset */ > >> static int interactive; > >> static struct string_list del_list = STRING_LIST_INIT_DUP; > >> static unsigned int colopts; > >> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value, > >> } > >> > >> if (!strcmp(var, "clean.requireforce")) { > >> - force = !git_config_bool(var, value); > >> + require_force = git_config_bool(var, value); > >> return 0; > >> } > >> > >> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix) > >> { > >> int i, res; > >> int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0; > >> - int ignored_only = 0, config_set = 0, errors = 0, gone = 1; > >> + int ignored_only = 0, force = 0, errors = 0, gone = 1; > >> int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT; > >> struct strbuf abs_path = STRBUF_INIT; > >> struct dir_struct dir = DIR_INIT; > >> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix) > >> }; > >> > >> git_config(git_clean_config, NULL); > >> - if (force < 0) > >> - force = 0; > >> - else > >> - config_set = 1; > >> > >> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage, > >> 0); > >> > >> - if (!interactive && !dry_run && !force) { > >> - if (config_set) > >> - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; " > >> + /* Dry run won't remove anything, so requiring force makes no sense */ > >> + if(dry_run) > >> + require_force = 0; > >> + > >> + if (!force && !interactive) { > >> + if (require_force > 0) > >> + die(_("clean.requireForce set to true and neither -f, nor -i given; " > >> + "refusing to clean")); > >> + else if (require_force < 0) > >> + die(_("clean.requireForce defaults to true and neither -f, nor -i given; " > >> "refusing to clean")); > >> - else > >> - die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;" > >> - " refusing to clean")); > >> } > >> > > > > The last two cases can be coalesced into a single case (the last one), > > because the difference in the messages does not bring more information > > to the user. > > Did you misread the patch? There are only 2 cases here, the last (third) > one is marked with '-' (removed). Too easy to misread this, I'd say. New > code is: > > if (require_force > 0) > die(_("clean.requireForce set to true and neither -f, nor -i given; " > "refusing to clean")); > else if (require_force < 0) > die(_("clean.requireForce defaults to true and neither -f, nor -i given; " > > and is basically unchanged from the original, except reference to '-n' has been > removed. Btw, is now comma needed after -f, and isn't it better to > substitute ':' for ';'? > > Thank you for review! > > -- Sergey Organov > > Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that specifying that this is the default or not is really useful. If the configuration was set to true, it is was a no-op. If set to false, no message will appear.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> What -n actually does in addition to its documented behavior is >> ignoring of configuration variable clean.requireForce, that makes >> sense provided -n prevents files removal anyway. > > There is another thing I noticed. > > This part to get rid of "config_set" does make sense. > >> git_config(git_clean_config, NULL); >> - if (force < 0) >> - force = 0; >> - else >> - config_set = 1; > > We used to think "force" variable is the master switch to do > anything , and requireForce configuration was a way to flip its > default to 0 (so that you need to set it to 1 again from the command > line). This separates "force" (which can only given via the command > line) and "require_force" (which controls when the "force" is used) > and makes the logic simpler. > >> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage, >> 0); > > However. > >> - if (!interactive && !dry_run && !force) { >> - if (config_set) >> - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; " >> + /* Dry run won't remove anything, so requiring force makes no sense */ >> + if(dry_run) >> + require_force = 0; > > I am not sure if this is making things inconsistent. I believe things rather got more consistent, see below. > > Dry run will be harmless, and we can be lenient and not require > force. But below, we do not require force when going interactive, > either. Except, unlike dry-run, interactive is not harmless, similar to -f. > So we could instead add > > if (dry_run || interactive) > require_force = 0; > > above, drop the "&& !interactive" from the guard for the > clean.requireForce block. That'd be less consistent, as dry-run is harmless, whereas neither force nor interactive are. > Or we can go the opposite way. We do not have to tweak > require_force at all based on other conditions. Instead we can > update the guard below to check "!force && !interactive && !dry_run" > before entering the clean.requireForce block, no? No, we do need to tweak require_force, as another if() that is inside and produces error message does in fact check for require_force being either negative or positive, i.e., non-zero. > > But the code after this patch makes me feel that it is somewhere in > the middle between these two optimum places. I believe it's rather right in the spot. I left '-i' to stay with '-f', as it was before the patch, as both are very distinct (even if in different manner) when compared to '-n', so now only '-n' is now treated separately. The very idea of dry-run is that it is orthogonal to any other behavior, so if I were designing it, I'd left bailing-out without -f or -i in place even if -n were given, to show what exactly would happen without -n. With new code it'd be as simple as removing "if (dry_run) require_force = 0" line that introduces the original dependency. > > Another thing. Stepping back and thinking _why_ the code can treat > dry_run and interactive the same way (either to make them drop > require_force above, or neither of them contributes to the value of > require_force), if we are dropping "you didn't give me --dry-run" in > the error message below, we should also drop "you didn't give me > --interactive, either" as well, when complaining about the lack of > "--force". In fact, the new code rather keep treating -f and -i somewhat similarly, rather than -i and -n, intentionally. That said, if somebody is going to re-consider -f vs -i issue, they now have more cleaner code that doesn't involve -n anymore. > One possible objection I can think of against doing so is that it > might not be so obvious why "interactive" does not have to require > "force" (even though it is clearly obvious to me). But if that were > the objection, then to somebody else "dry-run does not have to > require force" may equally not be so obvious (at least it wasn't so > obvious to me during the last round of this discussion). I'm not sure about interactive not requiring force, and I intentionally avoided this issue in the patch in question, though I think the patch makes it easier to reason about -i vs -f in the future by removing -n handling from the picture. > > So I can live without the "drop 'nor -i'" part I suggested in the > above. We would not drop "nor -i" and add "nor --dry-run" back to > the message instead. I'm afraid we can't meaningfully keep -n (--dry-run) in the messages. As it stands, having -n there was a mistake right from the beginning. Please consider the original message, but without -i and -f, for the sake of the argument: "clean.requireForce set to true and -n is not given; refusing to clean" to me it sounds like nonsense, as it suggests that if were given -n, we'd perform cleanup, that is simply false as no cleanup is ever performed once -n is there. Adding -i and -f back to the message somewhat blurs the problem, yet -n still does not belong there. > So from that angle, the message after this patch makes me feel that > it is somewhere in the middle between two more sensible places. I don't think so, see above. I rather believe that even if everything else in the patch were denied, the -n should be removed from the error message, so I did exactly that, and only that (i.e., didn't merge 2 messages into one). Thanks, -- Sergey Organov
Jean-Noël AVILA <avila.jn@gmail.com> writes: > On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote: >> Jean-Noël Avila <avila.jn@gmail.com> writes: >> >> > Putting my documentation/translator hat: >> > >> > Le 29/02/2024 à 20:07, Sergey Organov a écrit : >> >> What -n actually does in addition to its documented behavior is >> >> ignoring of configuration variable clean.requireForce, that makes >> >> sense provided -n prevents files removal anyway. >> >> >> >> So, first, document this in the manual, and then modify implementation >> >> to make this more explicit in the code. >> >> >> >> Improved implementation also stops to share single internal variable >> >> 'force' between command-line -f option and configuration variable >> >> clean.requireForce, resulting in more clear logic. >> >> >> >> The error messages now do not mention -n as well, as it seems >> >> unnecessary and does not reflect clarified implementation. >> >> >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> >> --- >> >> Documentation/git-clean.txt | 2 ++ >> >> builtin/clean.c | 26 +++++++++++++------------- >> >> 2 files changed, 15 insertions(+), 13 deletions(-) >> >> >> >> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt >> >> index 69331e3f05a1..662eebb85207 100644 >> >> --- a/Documentation/git-clean.txt >> >> +++ b/Documentation/git-clean.txt >> >> @@ -49,6 +49,8 @@ OPTIONS >> >> -n:: >> >> --dry-run:: >> >> Don't actually remove anything, just show what would be done. >> >> + Configuration variable clean.requireForce is ignored, as >> >> + nothing will be deleted anyway. >> > >> > Please use backticks for options, configuration and environment names: >> > `clean.requireForce` >> >> I did consider this. However, existing text already has exactly this one >> unquoted, so I just did the same. Hopefully it will be fixed altogether >> later, or are you positive I better resend the patch with quotes? >> >> >> >> >> -q:: >> >> --quiet:: >> >> diff --git a/builtin/clean.c b/builtin/clean.c >> >> index d90766cad3a0..fcc50d08ee9b 100644 >> >> --- a/builtin/clean.c >> >> +++ b/builtin/clean.c >> >> @@ -25,7 +25,7 @@ >> >> #include "help.h" >> >> #include "prompt.h" >> >> >> >> -static int force = -1; /* unset */ >> >> +static int require_force = -1; /* unset */ >> >> static int interactive; >> >> static struct string_list del_list = STRING_LIST_INIT_DUP; >> >> static unsigned int colopts; >> >> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const > char *value, >> >> } >> >> >> >> if (!strcmp(var, "clean.requireforce")) { >> >> - force = !git_config_bool(var, value); >> >> + require_force = git_config_bool(var, value); >> >> return 0; >> >> } >> >> >> >> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char > *prefix) >> >> { >> >> int i, res; >> >> int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0; >> >> - int ignored_only = 0, config_set = 0, errors = 0, gone = 1; >> >> + int ignored_only = 0, force = 0, errors = 0, gone = 1; >> >> int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT; >> >> struct strbuf abs_path = STRBUF_INIT; >> >> struct dir_struct dir = DIR_INIT; >> >> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const > char *prefix) >> >> }; >> >> >> >> git_config(git_clean_config, NULL); >> >> - if (force < 0) >> >> - force = 0; >> >> - else >> >> - config_set = 1; >> >> >> >> argc = parse_options(argc, argv, prefix, options, > builtin_clean_usage, >> >> 0); >> >> >> >> - if (!interactive && !dry_run && !force) { >> >> - if (config_set) >> >> - die(_("clean.requireForce set to true and > neither -i, -n, nor -f given; " >> >> + /* Dry run won't remove anything, so requiring force makes no > sense */ >> >> + if(dry_run) >> >> + require_force = 0; >> >> + >> >> + if (!force && !interactive) { >> >> + if (require_force > 0) >> >> + die(_("clean.requireForce set to true and > neither -f, nor -i given; " >> >> + "refusing to clean")); >> >> + else if (require_force < 0) >> >> + die(_("clean.requireForce defaults to true > and neither -f, nor -i given; " >> >> "refusing to clean")); >> >> - else >> >> - die(_("clean.requireForce defaults to true > and neither -i, -n, nor -f given;" >> >> - " refusing to clean")); >> >> } >> >> >> > >> > The last two cases can be coalesced into a single case (the last one), >> > because the difference in the messages does not bring more information >> > to the user. >> >> Did you misread the patch? There are only 2 cases here, the last (third) >> one is marked with '-' (removed). Too easy to misread this, I'd say. New >> code is: >> >> if (require_force > 0) >> die(_("clean.requireForce set to true and > neither -f, nor -i given; " >> "refusing to clean")); >> else if (require_force < 0) >> die(_("clean.requireForce defaults to true > and neither -f, nor -i given; " >> >> and is basically unchanged from the original, except reference to '-n' has > been >> removed. Btw, is now comma needed after -f, and isn't it better to >> substitute ':' for ';'? >> >> Thank you for review! >> >> -- Sergey Organov >> >> > > Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that > specifying that this is the default or not is really useful. If the > configuration was set to true, it is was a no-op. If set to false, no > message will appear. I'm not sure either, and as it's not the topic of this particular patch, I'd like to delegate the decision on the issue.
Sergey Organov <sorganov@gmail.com> writes: >> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that >> specifying that this is the default or not is really useful. If the >> configuration was set to true, it is was a no-op. If set to false, no >> message will appear. > > I'm not sure either, and as it's not the topic of this particular patch, > I'd like to delegate the decision on the issue. It is very much spot on the topic of simplifying and clarifying the code to unify these remaining two messages into a single one. And involving the --interactive that allows users a chance to rethink and refrain from removing some to the equation would also be worth doing in the same topic, even though it might not fit your immediate agenda of crusade against --dry-run.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that >>> specifying that this is the default or not is really useful. If the >>> configuration was set to true, it is was a no-op. If set to false, no >>> message will appear. >> >> I'm not sure either, and as it's not the topic of this particular patch, >> I'd like to delegate the decision on the issue. > > It is very much spot on the topic of simplifying and clarifying the > code to unify these remaining two messages into a single one. I'm inclined to be more against merging than for it, as for me it'd be confusing to be told that a configuration variable is set to true when I didn't set it, nor there is any way to figure where it is set, because in fact it isn't, and it's rather the default that is in use. Overall, to me the messages are fine as they are (except -n that doesn't belong there), I don't see compelling reason to hide information from the user, and thus I won't propose patch that gets rid of one of them. > And involving the --interactive that allows users a chance to > rethink and refrain from removing some to the equation would also be > worth doing in the same topic, Worth doing what? I'm afraid I lost the plot here, as --interactive still looks fine to me. > even though it might not fit your immediate agenda of crusade against > --dry-run. I'm hopefully crusading for --dry-run, not against, trying to get rid of the cause of the original confusion that started -n/-f controversy. Thanks, -- Sergey Organov
Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that >>>> specifying that this is the default or not is really useful. If the >>>> configuration was set to true, it is was a no-op. If set to false, no >>>> message will appear. >>> >>> I'm not sure either, and as it's not the topic of this particular patch, >>> I'd like to delegate the decision on the issue. >> >> It is very much spot on the topic of simplifying and clarifying the >> code to unify these remaining two messages into a single one. > > I'm inclined to be more against merging than for it, as for me it'd be > confusing to be told that a configuration variable is set to true when I > didn't set it, nor there is any way to figure where it is set, because > in fact it isn't, and it's rather the default that is in use. > > Overall, to me the messages are fine as they are (except -n that doesn't > belong there), I don't see compelling reason to hide information from > the user, and thus I won't propose patch that gets rid of one of them. Nevertheless, as others are in favor of unification, I've merged these two messages in the v2 version of the patch, which see. Thanks, -- Sergey Organov
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt index 69331e3f05a1..662eebb85207 100644 --- a/Documentation/git-clean.txt +++ b/Documentation/git-clean.txt @@ -49,6 +49,8 @@ OPTIONS -n:: --dry-run:: Don't actually remove anything, just show what would be done. + Configuration variable clean.requireForce is ignored, as + nothing will be deleted anyway. -q:: --quiet:: diff --git a/builtin/clean.c b/builtin/clean.c index d90766cad3a0..fcc50d08ee9b 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -25,7 +25,7 @@ #include "help.h" #include "prompt.h" -static int force = -1; /* unset */ +static int require_force = -1; /* unset */ static int interactive; static struct string_list del_list = STRING_LIST_INIT_DUP; static unsigned int colopts; @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value, } if (!strcmp(var, "clean.requireforce")) { - force = !git_config_bool(var, value); + require_force = git_config_bool(var, value); return 0; } @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix) { int i, res; int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0; - int ignored_only = 0, config_set = 0, errors = 0, gone = 1; + int ignored_only = 0, force = 0, errors = 0, gone = 1; int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT; struct strbuf abs_path = STRBUF_INIT; struct dir_struct dir = DIR_INIT; @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix) }; git_config(git_clean_config, NULL); - if (force < 0) - force = 0; - else - config_set = 1; argc = parse_options(argc, argv, prefix, options, builtin_clean_usage, 0); - if (!interactive && !dry_run && !force) { - if (config_set) - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; " + /* Dry run won't remove anything, so requiring force makes no sense */ + if(dry_run) + require_force = 0; + + if (!force && !interactive) { + if (require_force > 0) + die(_("clean.requireForce set to true and neither -f, nor -i given; " + "refusing to clean")); + else if (require_force < 0) + die(_("clean.requireForce defaults to true and neither -f, nor -i given; " "refusing to clean")); - else - die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;" - " refusing to clean")); } if (force > 1)
What -n actually does in addition to its documented behavior is ignoring of configuration variable clean.requireForce, that makes sense provided -n prevents files removal anyway. So, first, document this in the manual, and then modify implementation to make this more explicit in the code. Improved implementation also stops to share single internal variable 'force' between command-line -f option and configuration variable clean.requireForce, resulting in more clear logic. The error messages now do not mention -n as well, as it seems unnecessary and does not reflect clarified implementation. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- Documentation/git-clean.txt | 2 ++ builtin/clean.c | 26 +++++++++++++------------- 2 files changed, 15 insertions(+), 13 deletions(-) base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d