Re: [PATCH v2] clean: improve -n and -f implementation and documentation

Re: [PATCH v2] clean: improve -n and -f implementation and documentation

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Changes since v1:
>
>  * Fixed style of the if() statement
>
>  * Merged two error messages into one
>
>  * clean.requireForce description changed accordingly

Excellent.

> diff --git a/builtin/clean.c b/builtin/clean.c
> index d90766cad3a0..41502dcb0dde 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -25,7 +25,7 @@
>  #include "help.h"
>  #include "prompt.h"
>  
> -static int force = -1; /* unset */
> +static int require_force = -1; /* unset */
>  static int interactive;
>  static struct string_list del_list = STRING_LIST_INIT_DUP;
>  static unsigned int colopts;
> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
>  	}
>  
>  	if (!strcmp(var, "clean.requireforce")) {
> -		force = !git_config_bool(var, value);
> +		require_force = git_config_bool(var, value);
>  		return 0;
>  	}
>  
> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  {
>  	int i, res;
>  	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
> -	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
> +	int ignored_only = 0, force = 0, errors = 0, gone = 1;
>  	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
>  	struct strbuf abs_path = STRBUF_INIT;
>  	struct dir_struct dir = DIR_INIT;
> @@ -946,22 +946,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	};
>  
>  	git_config(git_clean_config, NULL);
> -	if (force < 0)
> -		force = 0;
> -	else
> -		config_set = 1;

The above changes are a significant improvement.  Instead of a
single "force" variable whose meaning is fuzzy, we now have
"require_force" to track the config setting, and "force" to indicate
the "--force" option.  THis makes the code's intent much clearer.

>  	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>  			     0);
>  
> -	if (!interactive && !dry_run && !force) {
> -		if (config_set)
> -			die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
> -				  "refusing to clean"));
> -		else
> -			die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;"

And thanks to that, the above trick with an extra variable "config_set",
which smells highly a round-about way, can be simplified.

> +	/* Dry run won't remove anything, so requiring force makes no sense */
> +	if (dry_run)
> +		require_force = 0;
> +	if (require_force != 0 && !force && !interactive)

However, the above logic could be improved.  The behaviour we have,
for a user who does *not* explicitly disable config.requireForce,
is, that when clean.requireForce is not set to 0, we would fail
unless one of these is in effect: -f, -n, -i.  Even though using
either -n or -i makes it unnecessary to use -f *exactly* the same
way, the above treats dry_run and interactive separately with two if
statements, which is suboptimal as a "code/logic clean-up".

The reason for the behaviour can be explained this way:

 * "git clean" (with neither -i nor -n.  The user wants the default
   mode that has no built-in protection will be stopped without -f.

 * "git clean -n".  The user wants the dry-run mode that has its own
   protection, i.e. being always no-op to the files, so there is no
   need to fail here for the lack of "-f".

 * "git clean --interactive".  The user wants the interactive mode
   that has its own protection, i.e. giving the end-user a chance to
   say "oh, I didn't mean to remove these files, 'q'uit from this
   mistake", so there is no need to fail here for the lack of "-f".

> +		die(_("clean.requireForce is true and neither -f nor -i given:"
>  				  " refusing to clean"));

The message is certainly cleaner compared to the previous round, but
this also can be improved.  Stepping back a bit and thinking who are
the target audience of this message.  The only users who see this
message are running "git clean" in its default (unprotected) mode,
and they wanted to "clean" for real.  If they wanted to do dry-run,
they would have said "-n" themselves, and that is why we can safely
omit mention of "-n" we had in the original message.

These users did not want to run the interractive clean, either---if
they wanted to go interractive, they would have said "-i"
themselves.  So we do not need to mention "-i" either for exactly
the same logic.

Based on the above observation,

I'll send a follow-up patch to clean up the code around here (both
implementation and documentation), taking '-i' into account as well.

[PATCH 1/1] clean: further clean-up of implementation around "--force"

[Junio C Hamano]

We clarified how clean.requireForce interacts with the --dry-run
option in the previous commit, both in the implementation and in the
documentation.  Even when "git clean" (without other options) is
required to be used with "--force" (i.e. either clean.requireForce
is unset, or explicitly set to true) to protect end-users from
casual invocation of the command by mistake, "--dry-run" does not
require "--force" to be used, because it is already its own
protection mechanism by being a no-op to the working tree files.

The previous commit, however, missed another clean-up opportunity
around the same area.  Just like in the "--dry-run" mode, the
command in the "--interactive" mode does not require "--force",
either.  This is because by going interactive and giving the end
user one more step to confirm, the mode itself is serving as its own
protection mechanism.

Let's take things one step further, unify the code that defines
interaction between `--force` and these two other options.  Just
like we added explanation for the reason why "--dry-run" does not
honor `clean.requireForce`, add the same explanation for
"--interactive".  Finally, add some tests to show the interaction
between "--force" and "--interactive" (we already have tests that
show interaction between "--force" and "--dry-run").

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config/clean.txt | 2 +-
 Documentation/git-clean.txt    | 4 +++-
 builtin/clean.c                | 9 ++-------
 t/t7300-clean.sh               | 6 ++++++
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/Documentation/config/clean.txt b/Documentation/config/clean.txt
index b19ca210f3..c0188ead4e 100644
--- a/Documentation/config/clean.txt
+++ b/Documentation/config/clean.txt
@@ -1,3 +1,3 @@
 clean.requireForce::
 	A boolean to make git-clean refuse to delete files unless -f
-	or -i is given. Defaults to true.
+	is given. Defaults to true.
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 662eebb852..082d033438 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -37,7 +37,7 @@ OPTIONS
 --force::
 	If the Git configuration variable clean.requireForce is not set
 	to false, 'git clean' will refuse to delete files or directories
-	unless given -f or -i.  Git will refuse to modify untracked
+	unless given -f.  Git will refuse to modify untracked
 	nested git repositories (directories with a .git subdirectory)
 	unless a second -f is given.
 
@@ -45,6 +45,8 @@ OPTIONS
 --interactive::
 	Show what would be done and clean files interactively. See
 	``Interactive mode'' for details.
+	Configuration variable clean.requireForce is ignored, as
+	this mode gives its own safety protection by going interactive.
 
 -n::
 --dry-run::
diff --git a/builtin/clean.c b/builtin/clean.c
index 41502dcb0d..29efe84153 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -950,13 +950,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
 			     0);
 
-	/* Dry run won't remove anything, so requiring force makes no sense */
-	if (dry_run)
-		require_force = 0;
-
-	if (require_force != 0 && !force && !interactive)
-		die(_("clean.requireForce is true and neither -f nor -i given:"
-				  " refusing to clean"));
+	if (require_force != 0 && !force && !interactive && !dry_run)
+		die(_("clean.requireForce is true and -f not given: refusing to clean"));
 
 	if (force > 1)
 		rm_flags = 0;
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 611b3dd3ae..1f7201eb60 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -407,6 +407,12 @@ test_expect_success 'clean.requireForce and -f' '
 
 '
 
+test_expect_success 'clean.requireForce and --interactive' '
+	git clean --interactive </dev/null >output 2>error &&
+	test_grep ! "requireForce is true and" error &&
+	test_grep "\*\*\* Commands \*\*\*" output
+'
+
 test_expect_success 'core.excludesfile' '
 
 	echo excludes >excludes &&
-- 
2.44.0-84-gb387623c12

Re: [PATCH 1/1] clean: further clean-up of implementation around "--force"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> @@ -950,13 +950,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>  			     0);
>  
> -	/* Dry run won't remove anything, so requiring force makes no sense */
> -	if (dry_run)
> -		require_force = 0;
> -
> -	if (require_force != 0 && !force && !interactive)
> -		die(_("clean.requireForce is true and neither -f nor -i given:"
> -				  " refusing to clean"));
> +	if (require_force != 0 && !force && !interactive && !dry_run)
> +		die(_("clean.requireForce is true and -f not given: refusing to clean"));
>  
>  	if (force > 1)
>  		rm_flags = 0;

An obvious alternative way to clean-up the logic is to do this
instead:

	if (dry_run || interactive))
		require_force = 0;
 	if (require_force != 0 && !force)
		die(_("clean.requireForce is true and ..."));

But as I wrote, the most important improvement done by Sergey's
patch was to remove the dual meaning of the "force" variable so that
it indicates if the "--force" option was given and nothing else,
while the "require_force" variable indicates if clean.requireForce
was given and nothing else.  From that point of view, the
conditional tweaking done to require_force in the above alternative
makes the code worse, relative to Sergey's patch, and certainly to
its follow up, my patch about "--interactive".

Re: [PATCH 1/1] clean: further clean-up of implementation around "--force"

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> We clarified how clean.requireForce interacts with the --dry-run
> option in the previous commit, both in the implementation and in the
> documentation.  Even when "git clean" (without other options) is
> required to be used with "--force" (i.e. either clean.requireForce
> is unset, or explicitly set to true) to protect end-users from
> casual invocation of the command by mistake, "--dry-run" does not
> require "--force" to be used, because it is already its own
> protection mechanism by being a no-op to the working tree files.
>
> The previous commit, however, missed another clean-up opportunity
> around the same area.  Just like in the "--dry-run" mode, the
> command in the "--interactive" mode does not require "--force",
> either.  This is because by going interactive and giving the end
> user one more step to confirm, the mode itself is serving as its own
> protection mechanism.
>
> Let's take things one step further, unify the code that defines
> interaction between `--force` and these two other options.  Just
> like we added explanation for the reason why "--dry-run" does not
> honor `clean.requireForce`, add the same explanation for
> "--interactive".  Finally, add some tests to show the interaction
> between "--force" and "--interactive" (we already have tests that
> show interaction between "--force" and "--dry-run").

Looks fine to me, including the patch itself.

Thanks,
-- Sergey Organov

Re: [PATCH v2] clean: improve -n and -f implementation and documentation

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Changes since v1:
>>
>>  * Fixed style of the if() statement
>>
>>  * Merged two error messages into one
>>
>>  * clean.requireForce description changed accordingly
>
> Excellent.
>
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index d90766cad3a0..41502dcb0dde 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -25,7 +25,7 @@
>>  #include "help.h"
>>  #include "prompt.h"
>>
>> -static int force = -1; /* unset */
>> +static int require_force = -1; /* unset */
>>  static int interactive;
>>  static struct string_list del_list = STRING_LIST_INIT_DUP;
>>  static unsigned int colopts;
>> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
>>  	}
>>
>>  	if (!strcmp(var, "clean.requireforce")) {
>> -		force = !git_config_bool(var, value);
>> +		require_force = git_config_bool(var, value);
>>  		return 0;
>>  	}
>>
>> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>>  {
>>  	int i, res;
>>  	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
>> -	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
>> +	int ignored_only = 0, force = 0, errors = 0, gone = 1;
>>  	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
>>  	struct strbuf abs_path = STRBUF_INIT;
>>  	struct dir_struct dir = DIR_INIT;
>> @@ -946,22 +946,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>>  	};
>>
>>  	git_config(git_clean_config, NULL);
>> -	if (force < 0)
>> -		force = 0;
>> -	else
>> -		config_set = 1;
>
> The above changes are a significant improvement.  Instead of a
> single "force" variable whose meaning is fuzzy, we now have
> "require_force" to track the config setting, and "force" to indicate
> the "--force" option.  THis makes the code's intent much clearer.
>
>>  	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>>  			     0);
>>
>> -	if (!interactive && !dry_run && !force) {
>> -		if (config_set)
>> -			die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
>> -				  "refusing to clean"));
>> -		else
>> -			die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;"
>
> And thanks to that, the above trick with an extra variable "config_set",
> which smells highly a round-about way, can be simplified.
>
>> +	/* Dry run won't remove anything, so requiring force makes no sense */
>> +	if (dry_run)
>> +		require_force = 0;
>> +	if (require_force != 0 && !force && !interactive)
>
> However, the above logic could be improved.  The behaviour we have,
> for a user who does *not* explicitly disable config.requireForce,
> is, that when clean.requireForce is not set to 0, we would fail
> unless one of these is in effect: -f, -n, -i.  Even though using
> either -n or -i makes it unnecessary to use -f *exactly* the same
> way, the above treats dry_run and interactive separately with two if
> statements, which is suboptimal as a "code/logic clean-up".

I wonder do you mean:

	/* Dry run won't remove anything, so requiring force makes no
	* sense. Interactive has its own means of protection, so don't
	* require force as well */
	if (dry_run || interactive)
		require_force = 0;

	if (require_force != 0 && !force)
                die_();

that looks fine to me, as it puts 'force' flag and corresponding
configuration into one if(), whereas both exceptions are put into
another. OTOH, having:

     if (require_force != 0 && !force && !interactive && !dry_run)
                die_();

mixture looks less appealing to me, though I won't fight against it
either.

>
> The reason for the behaviour can be explained this way:
>
>  * "git clean" (with neither -i nor -n.  The user wants the default
>    mode that has no built-in protection will be stopped without -f.
>
>  * "git clean -n".  The user wants the dry-run mode that has its own
>    protection, i.e. being always no-op to the files, so there is no
>    need to fail here for the lack of "-f".
>
>  * "git clean --interactive".  The user wants the interactive mode
>    that has its own protection, i.e. giving the end-user a chance to
>    say "oh, I didn't mean to remove these files, 'q'uit from this
>    mistake", so there is no need to fail here for the lack of "-f".

Well, if we remove -i from error message as well, then yes, this makes
sense.

>
>> +		die(_("clean.requireForce is true and neither -f nor -i given:"
>>  				  " refusing to clean"));
>
> The message is certainly cleaner compared to the previous round, but
> this also can be improved.  Stepping back a bit and thinking who are
> the target audience of this message.  The only users who see this
> message are running "git clean" in its default (unprotected) mode,
> and they wanted to "clean" for real.  If they wanted to do dry-run,
> they would have said "-n" themselves, and that is why we can safely
> omit mention of "-n" we had in the original message.
>
> These users did not want to run the interractive clean, either---if
> they wanted to go interractive, they would have said "-i"
> themselves.  So we do not need to mention "-i" either for exactly
> the same logic.

I then suggest to consider to remove mention of -i from
clean.requireForce description as well.

>
> Based on the above observation,
>
> I'll send a follow-up patch to clean up the code around here (both
> implementation and documentation), taking '-i' into account as well.

Fine, thanks!

-- Sergey Organov

Re: [PATCH v2] clean: improve -n and -f implementation and documentation

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> I wonder do you mean:
> 
> 	/* Dry run won't remove anything, so requiring force makes no
> 	* sense. Interactive has its own means of protection, so don't
> 	* require force as well */
> 	if (dry_run || interactive)
> 		require_force = 0;
>
> 	if (require_force != 0 && !force)
>                 die_();
> ...

That is explained in a few messages after this one, so I'll wait
until you read them all before responding ;-).

Thanks.

Re: [PATCH v2] clean: improve -n and -f implementation and documentation

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> I wonder do you mean:
>> 
>> 	/* Dry run won't remove anything, so requiring force makes no
>> 	* sense. Interactive has its own means of protection, so don't
>> 	* require force as well */
>> 	if (dry_run || interactive)
>> 		require_force = 0;
>>
>> 	if (require_force != 0 && !force)
>>                 die_();
>> ...
>
> That is explained in a few messages after this one, so I'll wait
> until you read them all before responding ;-).

Ah, yeah, got it now! So no further response is needed.

Thanks,
-- Sergey Organov

Re: [PATCH v2] clean: improve -n and -f implementation and documentation

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

>> The reason for the behaviour can be explained this way:
>>
>>  * "git clean" (with neither -i nor -n.  The user wants the default
>>    mode that has no built-in protection will be stopped without -f.
>>
>>  * "git clean -n".  The user wants the dry-run mode that has its own
>>    protection, i.e. being always no-op to the files, so there is no
>>    need to fail here for the lack of "-f".
>>
>>  * "git clean --interactive".  The user wants the interactive mode
>>    that has its own protection, i.e. giving the end-user a chance to
>>    say "oh, I didn't mean to remove these files, 'q'uit from this
>>    mistake", so there is no need to fail here for the lack of "-f".
>
> Well, if we remove -i from error message as well, then yes, this makes
> sense.
> ...
> I then suggest to consider to remove mention of -i from
> clean.requireForce description as well.

The follow-up patch you just reviewed in the other thread does
exactly that.

This is a tangent, but before finalizing the version that complains
"clean.requireForce is in effect and you did not give me -f" without
mentioning "-i" or "-n", I asked gemini.google.com to proofread the
patch and and one of its suggestion was to use this:

    "clean.requireForce is true.  Use -f to override, or consider
    using -n (dry-run) or -i (interactive) for a safer workflow."

as a possibly cleaner message.  It is the opposite of what both of
us concluded to be good in this exchange, but in some sense, it does
sound more helpful to end users, which I somehow found amusing.

Re: [PATCH v2] clean: improve -n and -f implementation and documentation

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> The reason for the behaviour can be explained this way:
>>>
>>>  * "git clean" (with neither -i nor -n.  The user wants the default
>>>    mode that has no built-in protection will be stopped without -f.
>>>
>>>  * "git clean -n".  The user wants the dry-run mode that has its own
>>>    protection, i.e. being always no-op to the files, so there is no
>>>    need to fail here for the lack of "-f".
>>>
>>>  * "git clean --interactive".  The user wants the interactive mode
>>>    that has its own protection, i.e. giving the end-user a chance to
>>>    say "oh, I didn't mean to remove these files, 'q'uit from this
>>>    mistake", so there is no need to fail here for the lack of "-f".
>>
>> Well, if we remove -i from error message as well, then yes, this makes
>> sense.
>> ...
>> I then suggest to consider to remove mention of -i from
>> clean.requireForce description as well.
>
> The follow-up patch you just reviewed in the other thread does
> exactly that.

Yeah, the follow-up patch somehow didn't thread correctly with original
discussion, so I've noticed it only after I wrote the above, and the
patch is fine indeed.

>
> This is a tangent, but before finalizing the version that complains
> "clean.requireForce is in effect and you did not give me -f" without
> mentioning "-i" or "-n", I asked gemini.google.com to proofread the
> patch and and one of its suggestion was to use this:
>
>     "clean.requireForce is true.  Use -f to override, or consider
>     using -n (dry-run) or -i (interactive) for a safer workflow."
>
> as a possibly cleaner message.  It is the opposite of what both of
> us concluded to be good in this exchange, but in some sense, it does
> sound more helpful to end users, which I somehow found amusing.

The added advice looks fine to me, as it explicitly separates -f from
the other ways of using "git clean". However, starting phrase with
"clean.requireForce is true" sounds strange. I'd rather say:

   "Refusing to remove files: use -f to force removal. Alternatively,
   consider using -n (dry-run) or -i (interactive) for a safer workflow.
   Set clean.requireForce to false to get rid of this message"

Here we first state what has happened, and then mention solutions in
most-probable-first order.

However, if I were gemini, I'd probably start from noticing that no
error message is required at all unless there is something to delete in
the first place. I.e., the error should probably occur not here, but
rather at every attempt to delete, and then explanation should be given
later, e.g.:

  Refusing to remove FILE1
  Refusing to remove FILE2

  No files were removed: use -f to force removal. Alternatively,
  consider using -n (dry-run) or -i (interactive) for a safer workflow.

  Set clean.requireForce to false to disable this protection.

With this, user effectively gets functionality similar to "git clean -n"
by default.

Just saying.

Thanks,
-- Sergey Organov