Jean-Noël AVILA [off-list ref] writes:

On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote:

quoted

Jean-Noël Avila [off-list ref] writes:

quoted

Putting my documentation/translator hat:

Le 29/02/2024 à 20:07, Sergey Organov a écrit :

quoted

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 <redacted>
---
 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? 

quoted

quoted

 
 -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,

quoted

quoted

quoted

 	}
 
 	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)

quoted

quoted

quoted

 {
 	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)

quoted

quoted

quoted

 	};
 
 	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,

quoted

quoted

quoted

 			     0);
 
-	if (!interactive && !dry_run && !force) {
-		if (config_set)
-			die(_("clean.requireForce set to true and 

neither -i, -n, nor -f given; "

quoted

quoted

quoted

+	/* Dry run won't remove anything, so requiring force makes no 

sense */

quoted

quoted

quoted

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

quoted

quoted

quoted

+				  "refusing to clean"));
+		else if (require_force < 0)
+			die(_("clean.requireForce defaults to true 

and neither -f, nor -i given; "

quoted

quoted

quoted

 				  "refusing to clean"));
-		else
-			die(_("clean.requireForce defaults to true 

and neither -i, -n, nor -f given;"

quoted

quoted

quoted

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

quoted

				  "refusing to clean"));
		else if (require_force < 0)
			die(_("clean.requireForce defaults to true 

and neither -f, nor -i given; "

quoted

and is basically unchanged from the original, except reference to '-n' has 

been

quoted

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.