{"thread":{"id":"61042","subject":"Re: [PATCH v2] clean: improve -n and -f implementation and documentation","startedAt":"2024-03-03T22:06:11Z","lastAt":"2024-03-04T20:19:15Z","messageCount":9,"participants":["Junio C Hamano","Sergey Organov"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"489823","messageId":"20240303220600.2491792-1-gitster@pobox.com","threadId":"61042","inReplyTo":"7le6ziqzb.fsf_-_@osv.gnss.ru","subject":"Re: [PATCH v2] clean: improve -n and -f implementation and documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-03T22:05:59Z","receivedAt":"2024-03-03T22:06:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Changes 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\nExcellent.\n\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 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\nThe above changes are a significant improvement.  Instead of a\nsingle \"force\" variable whose meaning is fuzzy, we now have\n\"require_force\" to track the config setting, and \"force\" to indicate\nthe \"--force\" option.  THis makes the code's intent much clearer.\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\nAnd thanks to that, the above trick with an extra variable \"config_set\",\nwhich smells highly a round-about way, can be simplified.\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> +\tif (require_force != 0 && !force && !interactive)\n\nHowever, the above logic could be improved.  The behaviour we have,\nfor a user who does *not* explicitly disable config.requireForce,\nis, that when clean.requireForce is not set to 0, we would fail\nunless one of these is in effect: -f, -n, -i.  Even though using\neither -n or -i makes it unnecessary to use -f *exactly* the same\nway, the above treats dry_run and interactive separately with two if\nstatements, which is suboptimal as a \"code/logic clean-up\".\n\nThe reason for the behaviour can be explained this way:\n\n * \"git clean\" (with neither -i nor -n.  The user wants the default\n   mode that has no built-in protection will be stopped without -f.\n\n * \"git clean -n\".  The user wants the dry-run mode that has its own\n   protection, i.e. being always no-op to the files, so there is no\n   need to fail here for the lack of \"-f\".\n\n * \"git clean --interactive\".  The user wants the interactive mode\n   that has its own protection, i.e. giving the end-user a chance to\n   say \"oh, I didn't mean to remove these files, 'q'uit from this\n   mistake\", so there is no need to fail here for the lack of \"-f\".\n\n> +\t\tdie(_(\"clean.requireForce is true and neither -f nor -i given:\"\n>  \t\t\t\t  \" refusing to clean\"));\n\nThe message is certainly cleaner compared to the previous round, but\nthis also can be improved.  Stepping back a bit and thinking who are\nthe target audience of this message.  The only users who see this\nmessage are running \"git clean\" in its default (unprotected) mode,\nand they wanted to \"clean\" for real.  If they wanted to do dry-run,\nthey would have said \"-n\" themselves, and that is why we can safely\nomit mention of \"-n\" we had in the original message.\n\nThese users did not want to run the interractive clean, either---if\nthey wanted to go interractive, they would have said \"-i\"\nthemselves.  So we do not need to mention \"-i\" either for exactly\nthe same logic.\n\nBased on the above observation,\n\nI'll send a follow-up patch to clean up the code around here (both\nimplementation and documentation), taking '-i' into account as well.\n"},{"id":"489824","messageId":"20240303220600.2491792-2-gitster@pobox.com","threadId":"61042","inReplyTo":"20240303220600.2491792-1-gitster@pobox.com","subject":"[PATCH 1/1] clean: further clean-up of implementation around \"--force\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-03T22:06:00Z","receivedAt":"2024-03-03T22:06:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We clarified how clean.requireForce interacts with the --dry-run\noption in the previous commit, both in the implementation and in the\ndocumentation.  Even when \"git clean\" (without other options) is\nrequired to be used with \"--force\" (i.e. either clean.requireForce\nis unset, or explicitly set to true) to protect end-users from\ncasual invocation of the command by mistake, \"--dry-run\" does not\nrequire \"--force\" to be used, because it is already its own\nprotection mechanism by being a no-op to the working tree files.\n\nThe previous commit, however, missed another clean-up opportunity\naround the same area.  Just like in the \"--dry-run\" mode, the\ncommand in the \"--interactive\" mode does not require \"--force\",\neither.  This is because by going interactive and giving the end\nuser one more step to confirm, the mode itself is serving as its own\nprotection mechanism.\n\nLet's take things one step further, unify the code that defines\ninteraction between `--force` and these two other options.  Just\nlike we added explanation for the reason why \"--dry-run\" does not\nhonor `clean.requireForce`, add the same explanation for\n\"--interactive\".  Finally, add some tests to show the interaction\nbetween \"--force\" and \"--interactive\" (we already have tests that\nshow interaction between \"--force\" and \"--dry-run\").\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/config/clean.txt | 2 +-\n Documentation/git-clean.txt    | 4 +++-\n builtin/clean.c                | 9 ++-------\n t/t7300-clean.sh               | 6 ++++++\n 4 files changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config/clean.txt b/Documentation/config/clean.txt\nindex b19ca210f3..c0188ead4e 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 refuse to delete files unless -f\n-\tor -i is given. Defaults to true.\n+\tis given. Defaults to true.\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex 662eebb852..082d033438 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -37,7 +37,7 @@ OPTIONS\n --force::\n \tIf the Git configuration variable clean.requireForce is not set\n \tto false, 'git clean' will refuse to delete files or directories\n-\tunless given -f or -i.  Git will refuse to modify untracked\n+\tunless given -f.  Git will refuse to modify untracked\n \tnested git repositories (directories with a .git subdirectory)\n \tunless a second -f is given.\n \n@@ -45,6 +45,8 @@ OPTIONS\n --interactive::\n \tShow what would be done and clean files interactively. See\n \t``Interactive mode'' for details.\n+\tConfiguration variable clean.requireForce is ignored, as\n+\tthis mode gives its own safety protection by going interactive.\n \n -n::\n --dry-run::\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 41502dcb0d..29efe84153 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -950,13 +950,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n \t\t\t     0);\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-\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+\tif (require_force != 0 && !force && !interactive && !dry_run)\n+\t\tdie(_(\"clean.requireForce is true and -f not given: refusing to clean\"));\n \n \tif (force > 1)\n \t\trm_flags = 0;\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 611b3dd3ae..1f7201eb60 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -407,6 +407,12 @@ test_expect_success 'clean.requireForce and -f' '\n \n '\n \n+test_expect_success 'clean.requireForce and --interactive' '\n+\tgit clean --interactive </dev/null >output 2>error &&\n+\ttest_grep ! \"requireForce is true and\" error &&\n+\ttest_grep \"\\*\\*\\* Commands \\*\\*\\*\" output\n+'\n+\n test_expect_success 'core.excludesfile' '\n \n \techo excludes >excludes &&\n-- \n2.44.0-84-gb387623c12\n\n"},{"id":"489825","messageId":"xmqq5xy3vu0x.fsf@gitster.g","threadId":"61042","inReplyTo":"20240303220600.2491792-2-gitster@pobox.com","subject":"Re: [PATCH 1/1] clean: further clean-up of implementation around \"--force\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-03T22:18:38Z","receivedAt":"2024-03-03T22:18:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> @@ -950,13 +950,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n>  \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n>  \t\t\t     0);\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> -\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> +\tif (require_force != 0 && !force && !interactive && !dry_run)\n> +\t\tdie(_(\"clean.requireForce is true and -f not given: refusing to clean\"));\n>  \n>  \tif (force > 1)\n>  \t\trm_flags = 0;\n\nAn obvious alternative way to clean-up the logic is to do this\ninstead:\n\n\tif (dry_run || interactive))\n\t\trequire_force = 0;\n \tif (require_force != 0 && !force)\n\t\tdie(_(\"clean.requireForce is true and ...\"));\n\nBut as I wrote, the most important improvement done by Sergey's\npatch was to remove the dual meaning of the \"force\" variable so that\nit indicates if the \"--force\" option was given and nothing else,\nwhile the \"require_force\" variable indicates if clean.requireForce\nwas given and nothing else.  From that point of view, the\nconditional tweaking done to require_force in the above alternative\nmakes the code worse, relative to Sergey's patch, and certainly to\nits follow up, my patch about \"--interactive\".\n"},{"id":"489910","messageId":"87h6hl96z7.fsf@osv.gnss.ru","threadId":"61042","inReplyTo":"20240303220600.2491792-1-gitster@pobox.com","subject":"Re: [PATCH v2] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-04T18:39:40Z","receivedAt":"2024-03-04T18:39:44Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Changes 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> Excellent.\n>\n>> diff --git a/builtin/clean.c b/builtin/clean.c\n>> index 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> The above changes are a significant improvement.  Instead of a\n> single \"force\" variable whose meaning is fuzzy, we now have\n> \"require_force\" to track the config setting, and \"force\" to indicate\n> the \"--force\" option.  THis makes the code's intent much clearer.\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>\n> And thanks to that, the above trick with an extra variable \"config_set\",\n> which smells highly a round-about way, can be simplified.\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>> +\tif (require_force != 0 && !force && !interactive)\n>\n> However, the above logic could be improved.  The behaviour we have,\n> for a user who does *not* explicitly disable config.requireForce,\n> is, that when clean.requireForce is not set to 0, we would fail\n> unless one of these is in effect: -f, -n, -i.  Even though using\n> either -n or -i makes it unnecessary to use -f *exactly* the same\n> way, the above treats dry_run and interactive separately with two if\n> statements, which is suboptimal as a \"code/logic clean-up\".\n\nI wonder do you mean:\n\n\t/* Dry run won't remove anything, so requiring force makes no\n\t* sense. Interactive has its own means of protection, so don't\n\t* require force as well */\n\tif (dry_run || interactive)\n\t\trequire_force = 0;\n\n\tif (require_force != 0 && !force)\n                die_();\n\nthat looks fine to me, as it puts 'force' flag and corresponding\nconfiguration into one if(), whereas both exceptions are put into\nanother. OTOH, having:\n\n     if (require_force != 0 && !force && !interactive && !dry_run)\n                die_();\n\nmixture looks less appealing to me, though I won't fight against it\neither.\n\n>\n> The reason for the behaviour can be explained this way:\n>\n>  * \"git clean\" (with neither -i nor -n.  The user wants the default\n>    mode that has no built-in protection will be stopped without -f.\n>\n>  * \"git clean -n\".  The user wants the dry-run mode that has its own\n>    protection, i.e. being always no-op to the files, so there is no\n>    need to fail here for the lack of \"-f\".\n>\n>  * \"git clean --interactive\".  The user wants the interactive mode\n>    that has its own protection, i.e. giving the end-user a chance to\n>    say \"oh, I didn't mean to remove these files, 'q'uit from this\n>    mistake\", so there is no need to fail here for the lack of \"-f\".\n\nWell, if we remove -i from error message as well, then yes, this makes\nsense.\n\n>\n>> +\t\tdie(_(\"clean.requireForce is true and neither -f nor -i given:\"\n>>  \t\t\t\t  \" refusing to clean\"));\n>\n> The message is certainly cleaner compared to the previous round, but\n> this also can be improved.  Stepping back a bit and thinking who are\n> the target audience of this message.  The only users who see this\n> message are running \"git clean\" in its default (unprotected) mode,\n> and they wanted to \"clean\" for real.  If they wanted to do dry-run,\n> they would have said \"-n\" themselves, and that is why we can safely\n> omit mention of \"-n\" we had in the original message.\n>\n> These users did not want to run the interractive clean, either---if\n> they wanted to go interractive, they would have said \"-i\"\n> themselves.  So we do not need to mention \"-i\" either for exactly\n> the same logic.\n\nI then suggest to consider to remove mention of -i from\nclean.requireForce description as well.\n\n>\n> Based on the above observation,\n>\n> I'll send a follow-up patch to clean up the code around here (both\n> implementation and documentation), taking '-i' into account as well.\n\nFine, thanks!\n\n-- Sergey Organov\n"},{"id":"489911","messageId":"xmqqa5ndq1op.fsf@gitster.g","threadId":"61042","inReplyTo":"87h6hl96z7.fsf@osv.gnss.ru","subject":"Re: [PATCH v2] clean: improve -n and -f implementation and documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T18:41:58Z","receivedAt":"2024-03-04T18:42:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> I wonder do you mean:\n> \n> \t/* Dry run won't remove anything, so requiring force makes no\n> \t* sense. Interactive has its own means of protection, so don't\n> \t* require force as well */\n> \tif (dry_run || interactive)\n> \t\trequire_force = 0;\n>\n> \tif (require_force != 0 && !force)\n>                 die_();\n> ...\n\nThat is explained in a few messages after this one, so I'll wait\nuntil you read them all before responding ;-).\n\nThanks.\n"},{"id":"489912","messageId":"87cys996nf.fsf@osv.gnss.ru","threadId":"61042","inReplyTo":"20240303220600.2491792-2-gitster@pobox.com","subject":"Re: [PATCH 1/1] clean: further clean-up of implementation around \"--force\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-04T18:46:44Z","receivedAt":"2024-03-04T18:46:48Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> We clarified how clean.requireForce interacts with the --dry-run\n> option in the previous commit, both in the implementation and in the\n> documentation.  Even when \"git clean\" (without other options) is\n> required to be used with \"--force\" (i.e. either clean.requireForce\n> is unset, or explicitly set to true) to protect end-users from\n> casual invocation of the command by mistake, \"--dry-run\" does not\n> require \"--force\" to be used, because it is already its own\n> protection mechanism by being a no-op to the working tree files.\n>\n> The previous commit, however, missed another clean-up opportunity\n> around the same area.  Just like in the \"--dry-run\" mode, the\n> command in the \"--interactive\" mode does not require \"--force\",\n> either.  This is because by going interactive and giving the end\n> user one more step to confirm, the mode itself is serving as its own\n> protection mechanism.\n>\n> Let's take things one step further, unify the code that defines\n> interaction between `--force` and these two other options.  Just\n> like we added explanation for the reason why \"--dry-run\" does not\n> honor `clean.requireForce`, add the same explanation for\n> \"--interactive\".  Finally, add some tests to show the interaction\n> between \"--force\" and \"--interactive\" (we already have tests that\n> show interaction between \"--force\" and \"--dry-run\").\n\nLooks fine to me, including the patch itself.\n\nThanks,\n-- Sergey Organov\n"},{"id":"489914","messageId":"878r2x96kd.fsf@osv.gnss.ru","threadId":"61042","inReplyTo":"xmqqa5ndq1op.fsf@gitster.g","subject":"Re: [PATCH v2] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-04T18:48:34Z","receivedAt":"2024-03-04T18:48:37Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> I wonder do you mean:\n>> \n>> \t/* Dry run won't remove anything, so requiring force makes no\n>> \t* sense. Interactive has its own means of protection, so don't\n>> \t* require force as well */\n>> \tif (dry_run || interactive)\n>> \t\trequire_force = 0;\n>>\n>> \tif (require_force != 0 && !force)\n>>                 die_();\n>> ...\n>\n> That is explained in a few messages after this one, so I'll wait\n> until you read them all before responding ;-).\n\nAh, yeah, got it now! So no further response is needed.\n\nThanks,\n-- Sergey Organov\n"},{"id":"489916","messageId":"xmqqo7btom4u.fsf@gitster.g","threadId":"61042","inReplyTo":"87h6hl96z7.fsf@osv.gnss.ru","subject":"Re: [PATCH v2] clean: improve -n and -f implementation and documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T19:03:13Z","receivedAt":"2024-03-04T19:03:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n>> The reason for the behaviour can be explained this way:\n>>\n>>  * \"git clean\" (with neither -i nor -n.  The user wants the default\n>>    mode that has no built-in protection will be stopped without -f.\n>>\n>>  * \"git clean -n\".  The user wants the dry-run mode that has its own\n>>    protection, i.e. being always no-op to the files, so there is no\n>>    need to fail here for the lack of \"-f\".\n>>\n>>  * \"git clean --interactive\".  The user wants the interactive mode\n>>    that has its own protection, i.e. giving the end-user a chance to\n>>    say \"oh, I didn't mean to remove these files, 'q'uit from this\n>>    mistake\", so there is no need to fail here for the lack of \"-f\".\n>\n> Well, if we remove -i from error message as well, then yes, this makes\n> sense.\n> ...\n> I then suggest to consider to remove mention of -i from\n> clean.requireForce description as well.\n\nThe follow-up patch you just reviewed in the other thread does\nexactly that.\n\nThis is a tangent, but before finalizing the version that complains\n\"clean.requireForce is in effect and you did not give me -f\" without\nmentioning \"-i\" or \"-n\", I asked gemini.google.com to proofread the\npatch and and one of its suggestion was to use this:\n\n    \"clean.requireForce is true.  Use -f to override, or consider\n    using -n (dry-run) or -i (interactive) for a safer workflow.\"\n\nas a possibly cleaner message.  It is the opposite of what both of\nus concluded to be good in this exchange, but in some sense, it does\nsound more helpful to end users, which I somehow found amusing.\n\n\n"},{"id":"489917","messageId":"87sf157nsv.fsf@osv.gnss.ru","threadId":"61042","inReplyTo":"xmqqo7btom4u.fsf@gitster.g","subject":"Re: [PATCH v2] clean: improve -n and -f implementation and documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2024-03-04T20:19:12Z","receivedAt":"2024-03-04T20:19:15Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>>> The reason for the behaviour can be explained this way:\n>>>\n>>>  * \"git clean\" (with neither -i nor -n.  The user wants the default\n>>>    mode that has no built-in protection will be stopped without -f.\n>>>\n>>>  * \"git clean -n\".  The user wants the dry-run mode that has its own\n>>>    protection, i.e. being always no-op to the files, so there is no\n>>>    need to fail here for the lack of \"-f\".\n>>>\n>>>  * \"git clean --interactive\".  The user wants the interactive mode\n>>>    that has its own protection, i.e. giving the end-user a chance to\n>>>    say \"oh, I didn't mean to remove these files, 'q'uit from this\n>>>    mistake\", so there is no need to fail here for the lack of \"-f\".\n>>\n>> Well, if we remove -i from error message as well, then yes, this makes\n>> sense.\n>> ...\n>> I then suggest to consider to remove mention of -i from\n>> clean.requireForce description as well.\n>\n> The follow-up patch you just reviewed in the other thread does\n> exactly that.\n\nYeah, the follow-up patch somehow didn't thread correctly with original\ndiscussion, so I've noticed it only after I wrote the above, and the\npatch is fine indeed.\n\n>\n> This is a tangent, but before finalizing the version that complains\n> \"clean.requireForce is in effect and you did not give me -f\" without\n> mentioning \"-i\" or \"-n\", I asked gemini.google.com to proofread the\n> patch and and one of its suggestion was to use this:\n>\n>     \"clean.requireForce is true.  Use -f to override, or consider\n>     using -n (dry-run) or -i (interactive) for a safer workflow.\"\n>\n> as a possibly cleaner message.  It is the opposite of what both of\n> us concluded to be good in this exchange, but in some sense, it does\n> sound more helpful to end users, which I somehow found amusing.\n\nThe added advice looks fine to me, as it explicitly separates -f from\nthe other ways of using \"git clean\". However, starting phrase with\n\"clean.requireForce is true\" sounds strange. I'd rather say:\n\n   \"Refusing to remove files: use -f to force removal. Alternatively,\n   consider using -n (dry-run) or -i (interactive) for a safer workflow.\n   Set clean.requireForce to false to get rid of this message\"\n\nHere we first state what has happened, and then mention solutions in\nmost-probable-first order.\n\nHowever, if I were gemini, I'd probably start from noticing that no\nerror message is required at all unless there is something to delete in\nthe first place. I.e., the error should probably occur not here, but\nrather at every attempt to delete, and then explanation should be given\nlater, e.g.:\n\n  Refusing to remove FILE1\n  Refusing to remove FILE2\n\n  No files were removed: use -f to force removal. Alternatively,\n  consider using -n (dry-run) or -i (interactive) for a safer workflow.\n\n  Set clean.requireForce to false to disable this protection.\n\nWith this, user effectively gets functionality similar to \"git clean -n\"\nby default.\n\nJust saying.\n\nThanks,\n-- Sergey Organov\n"}]}