stash: added safety flag for stash clear subcommand

Commit Message

Nadav Goldstein May 15, 2022, 9:18 p.m. UTC

From: Nadav Goldstein <[e-mail]>

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).

Signed-off-by: Nadav Goldstein <[e-mail]>
---
    stash clear: added safety flag for stash clear subcommand
    
    Added a flag to git stash clear (-i|--interactive), which prompts the
    user to confirm he indeed wants to clear the stash, otherwise it will
    abort.
    
    I found it useful as a frequent stash clear user which means my terminal
    always have it in recent commands, which could mean accidental erase of
    work. This flag ensures accidental fires won't clear hard work :)
    
    I also thought it would be better to do it opt in, to not change the way
    the command works in prod, only add to it.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1232%2Fnadav96%2Fclear-stash-prompt-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1232/nadav96/clear-stash-prompt-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1232

 Documentation/git-stash.txt |  9 +++++++--
 builtin/stash.c             | 21 +++++++++++++++++++--
 2 files changed, 26 insertions(+), 4 deletions(-)


base-commit: e8005e4871f130c4e402ddca2032c111252f070a

Comments

Junio C Hamano May 16, 2022, 3:17 a.m. UTC | #1

"Nadav Goldstein via GitGitGadget" <[e-mail]> writes:

> From: Nadav Goldstein <[e-mail]>
>
> 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.

Patch

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]
 'git stash' create [<message>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 
@@ -127,7 +127,7 @@  the stash entry is applied on top of the commit that was HEAD at the
 time `git stash` was run, it restores the originally stashed state
 with no conflicts.
 
-clear::
+clear [-i|--interactive]::
 	Remove all the stash entries. Note that those entries will then
 	be subject to pruning, and may be impossible to recover (see
 	'Examples' below for a possible strategy).
@@ -160,6 +160,11 @@  OPTIONS
 All ignored and untracked files are also stashed and then cleaned
 up with `git clean`.
 
+-i::
+--interactive::
+	This option is only valid for clear command, when applied, will request
+	a confirmation from the user before proceeding to clear the stash.
+
 -u::
 --include-untracked::
 --no-include-untracked::
diff --git a/builtin/stash.c b/builtin/stash.c
index 0c7b6a95882..b012d24ef38 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -26,7 +26,7 @@  static const char * const git_stash_usage[] = {
 	N_("git stash drop [-q|--quiet] [<stash>]"),
 	N_("git stash ( pop | apply ) [--index] [-q|--quiet] [<stash>]"),
 	N_("git stash branch <branchname> [<stash>]"),
-	"git stash clear",
+	"git stash clear [-i|--interactive]",
 	N_("git stash [push [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\n"
 	   "          [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n"
 	   "          [--pathspec-from-file=<file> [--pathspec-file-nul]]\n"
@@ -67,7 +67,7 @@  static const char * const git_stash_branch_usage[] = {
 };
 
 static const char * const git_stash_clear_usage[] = {
-	"git stash clear",
+	"git stash clear [-i|--interactive]",
 	NULL
 };
 
@@ -215,7 +215,10 @@  static int do_clear_stash(void)
 
 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()
 	};
 
@@ -226,7 +229,21 @@  static int clear_stash(int argc, const char **argv, const char *prefix)
 	if (argc)
 		return error(_("git stash clear with arguments is "
 			       "unimplemented"));
+	if (is_prompt == 1) {
+		char code[2];
+		printf("Are you sure you want to clear your stash? [y/N]: ");
+		if (fgets(code, 2, stdin) != NULL) {
+			if (code[0] == 'y' || code[0] == 'Y') {
+				printf_ln(_("Clearing stash"));
+				return do_clear_stash();
+			}
+			else {
+				printf_ln(_("Aborting clear"));
+			}
+		}
 
+		return 0;
+	}
 	return do_clear_stash();
 }