Re: [PATCH] stash: added safety flag for stash clear subcommand

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

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

> From: Nadav Goldstein <nadav.goldstein@blazepod.co>
>
> Introduced a flag (git stash clear -i) when when used,
> instead of silently clearing the stash, will present a
> confirmation to the user regarding the action which he's
> about to perform. Took the inspiration from rm -rf -i flag.
> This flag also outputs extra logs (abort/success) to let
> the user know more "interactively" what is happening with
> the stash (hence the flag name).

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

If this were truly "interactive" modelled after "rm -i", I would
imagine that it would ask a question for each stash entry and allow
the user to selectively drop some entries while keeping others.

But that is not how the option behaves, as far as I can see.  It
only asks a single y/N question, without even knowing how many stash
entries there are.  Your user may say "Yes" and the command may then
find no stash entries that need to be cleared after all.

It is not taking any inspiration from "rm -i" at all.

I wonder if it is easier to model the "safety" after "git clean"
instead.  The "git clean" command by default does not do any removal
and you are required to say "clean -f" or "clean -n", unless the
"clean.requireforce" configuration variable is set to false.  The
resulting "git stash clear [--force|--dry-run]" would be fairly easy
to understand to new users, because "git clean [--force|--dry-run]"
already behaves in a similar way.

One caveat is that "git clean" by default requires "--force" on the
command line to do any damage, and it is OK because it was like so
for a long time.  But "git stash clear" has been with us for a very
long time without such requirement, so if you suddenly start
requiring "--force" and defaulting to "--dry-run", existing users
may be annoyed that they need to say

	$ git config --global stashClear.requireforce no

once to get back the original behaviour.

I personally think that such a one-time annoyance forced on all the
existing users may probably be worth it for added safety, but I am
not the only user of Git, so... ;-)

In any case, my recommendation would be

 * add "--force" and "--dry-run" to "git stash clear".

 * add stashClear.requireForce that defaults to "false".

 * document the above. mention the setting in ReleaseNotes.

 * monitor places like stackoverflow to see if folks give advice
   "set stashClear.requireForce to true for safety" to newbies
   often.  It would give us an excuse to proceed to the next step,
   i.e. propose to switch the default of stashClear.requireForce to
   "true".

 * The true "interactive" that lets your users pick and choose what
   to discard can come later as another improved form of additional
   safety.

The rest are minor comments that have already been outdated by all
ofhte above ;-)

> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 6e15f475257..a7ab5379779 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -17,7 +17,7 @@ SYNOPSIS
>  	     [-u|--include-untracked] [-a|--all] [-m|--message <message>]
>  	     [--pathspec-from-file=<file> [--pathspec-file-nul]]
>  	     [--] [<pathspec>...]]
> -'git stash' clear
> +'git stash' clear [-i|--interactive]

As I already said, calling the behaviour implemented by the patch
"interactive" is misleading; users will expect to be able to pick
and choose which entry to discard.

>  static int clear_stash(int argc, const char **argv, const char *prefix)
>  {
> +	int is_prompt;
>  	struct option options[] = {
> +		OPT_BOOL('i', "interactive", &is_prompt,
> +			 N_("confirm clearing stash")),
>  		OPT_END()
>  	};

"IS_prompt" is a strange phrasing to call the option.  

needs_prompt (we need to prompt the user before proceeding),
or do_prompt (literally, "do the prompting"), would be more
understanding, though.