Re: [PATCH v3] Introduced force flag to the git stash clear subcommand.

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

"Nadav Goldstein via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Nadav Goldstein <nadav.goldstein96@gmail.com>
>
> stash clean subcommand now support the force flag, along
> with the configuration var stash.requireforce, that if
> set to true, will make git stash clear fail unless supplied
> with force flag.

Documentation/SubmittingPatches gives many helpful hints on how to
write log messages for the project.

> @@ -208,6 +208,14 @@ to learn how to operate the `--patch` mode.
>  The `--patch` option implies `--keep-index`.  You can use
>  `--no-keep-index` to override this.
>  
> +-f::
> +--force::
> +	This option is only valid for `clear` command

Missing full-stop?

> ++
> +If the Git configuration variable stash.requireForce is set

Drop "Git" perhaps?  I haven't seen any other place that says "Git
configuration variable X" when talking about a single variable (it
probably is OK to call those defined in a Git configuration file
collectively as "Git configuration variables", though).

> +to true, 'git stash clear' will refuse to remove all the stash 
> +entries unless given -f.

I am not sure how much value users would get by requiring "--force",
though.  I know this was (partly) modeled after "git clean", but
over there, when the required "--force" is not given, the user would
give "--dry-run" (or "-n"), and the user will see what would be
removed if the user gave "--force".  If missing "--force" made "git
stash clear" show the stash entries that would be lost, then after
seeing an error message, it would be easier for the user to decide
if their next move should be to re-run the command with "--force",
or there are some precious entries and the user is not ready to do
"stash clear".

But just refusing to run without giving any other information will
just train the user to give "git stash clear --force" without
thinking, because getting "because you did not give the required
--force option, I am not doing anything" is only annoying without
giving any useful information.

> diff --git a/builtin/stash.c b/builtin/stash.c
> index a7e17ffe384..d037bc4f69c 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -53,7 +53,7 @@
>  #define BUILTIN_STASH_CREATE_USAGE \
>  	N_("git stash create [<message>]")
>  #define BUILTIN_STASH_CLEAR_USAGE \
> -	"git stash clear"
> +	"git stash clear [-f | --force]"
>  
>  static const char * const git_stash_usage[] = {
>  	BUILTIN_STASH_LIST_USAGE,
> @@ -122,6 +122,7 @@ static const char * const git_stash_save_usage[] = {
>  
>  static const char ref_stash[] = "refs/stash";
>  static struct strbuf stash_index_path = STRBUF_INIT;
> +static int clear_require_force = 0;

Do not explicitly initialize globals to 0 or NULL; let BSS take care
of the zero initialization, instead.

> @@ -246,7 +247,9 @@ static int do_clear_stash(void)
>  
>  static int clear_stash(int argc, const char **argv, const char *prefix)
>  {
> +	int force = 0;
>  	struct option options[] = {
> +		OPT__FORCE(&force, N_("force"), PARSE_OPT_NOCOMPLETE),
>  		OPT_END()
>  	};

As this topic focuses on "git stash clear", this is OK (and the
description in the documentation that says that "force" is currently
supported only by "clear" is also fine), but is "clear" the only
destructive subcommand and no other subcommand will want to learn
the "--force" for similar safety in the future?  The answer to this
question matters because ...

> @@ -258,6 +261,9 @@ static int clear_stash(int argc, const char **argv, const char *prefix)
>  		return error(_("git stash clear with arguments is "
>  			       "unimplemented"));
>  
> +	if (!force && clear_require_force)
> +		return error(_("fatal: stash.requireForce set to true and -f was not given; refusing to clear stash"));
> +
>  	return do_clear_stash();
>  }
>  
> @@ -851,6 +857,10 @@ static int git_stash_config(const char *var, const char *value, void *cb)
>  		show_include_untracked = git_config_bool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp(var, "stash.requireforce")) {

... the naming of this variable, facing the end users, does not
limit itself to "clear" at all.  It gives an impression that setting
this will require "--force" for all other subcommands that would
support it.  However ...

> +		clear_require_force = git_config_bool(var, value);

... inside the code, the variable is named in such a way that it is
only about the "clear" subcommand and nothing else.

I suspect that the end-user facing "stash.requireforce" should be
renamed to make it clear that it is about "stash clear" subcommand,
and not everywhere in "stash" requires "--force".  Nobody wants to
keep saying "git stash save --force" ;-)

> +		return 0;
> +	}

Thanks.