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

[Junio C Hamano <gitster@pobox.com>]

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.