diff mbox series

clean: improve -n and -f implementation and documentation

Message ID 875xy76qe1.fsf@osv.gnss.ru (mailing list archive)
State Superseded
Headers show
Series clean: improve -n and -f implementation and documentation | expand

Commit Message

Sergey Organov Feb. 29, 2024, 7:07 p.m. UTC
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

Comments

Jean-Noël Avila March 1, 2024, 1:20 p.m. UTC | #1
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
Sergey Organov March 1, 2024, 2:34 p.m. UTC | #2
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
Kristoffer Haugsbakk March 1, 2024, 3:29 p.m. UTC | #3
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?
Junio C Hamano March 1, 2024, 6:07 p.m. UTC | #4
"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.
Junio C Hamano March 1, 2024, 6:07 p.m. UTC | #5
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 March 1, 2024, 6:30 p.m. UTC | #6
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.
Sergey Organov March 1, 2024, 7:31 p.m. UTC | #7
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
Junio C Hamano March 2, 2024, 4:31 p.m. UTC | #8
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
Jean-Noël Avila March 2, 2024, 7:47 p.m. UTC | #9
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.
Sergey Organov March 2, 2024, 7:59 p.m. UTC | #10
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
Sergey Organov March 2, 2024, 8:09 p.m. UTC | #11
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.
Junio C Hamano March 2, 2024, 9:07 p.m. UTC | #12
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.
Sergey Organov March 2, 2024, 11:48 p.m. UTC | #13
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 March 3, 2024, 9:54 a.m. UTC | #14
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 mbox series

Patch

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)