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

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

[Nadav Goldstein]

Thank you all for your detailed replies!

I thought deeply on the issues that you presented, and I wonder
whether modeling after git clean --dry-run accomplish the goal I aimed
to solve.
If we make "git stash clear" to by default not clear the stash and
just present the stashes to be dropped (and require -f to force
clearing the stash), wouldn't we essentially make a duplicate of "git
stash list"?

My goal is to not be afraid to have git stash clear in your terminal
history, while indeed performing the clearing of the whole stack when
fired.

I thought presenting a confirmation dialog before clearing would
suffice as a solution, but I like the idea of listing all of the
stashes to be dropped and requesting confirmation (and in the future
allow selection of specific stashes to be dropped as you suggested).

Regarding making it opt out instead of opt in, I am afraid there are
tools that use git stash clean automatically, and making the
confirmation opt-out (by default confirm) we can potentially break
them, don't you think it would be more gentle to the git community to
make it opt in? Of course one major downside is that users need to
"find" it so the adaptation will be much slower, but I feel it's for
the best.

What do you think? I do agree that --interactive is not a good name
and a bit misleading, can you help me think of a better name that will
be more of a suit? maybe -c|--confirm, and using
stashClear.requireConfirm? Or -p|--prompt?

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

[Junio C Hamano]

Nadav Goldstein <nadav.goldstein@blazepod.co> writes:

> If we make "git stash clear" to by default not clear the stash and
> just present the stashes to be dropped (and require -f to force
> clearing the stash), wouldn't we essentially make a duplicate of "git
> stash list"?

Correct, but that is not what I would imagine to be a parallel to
"git clean [-f|-n]" with clean.requireForce.  If you haven't played
with "git clean" with various settings of that variable, you should.
Its behaviour illustrates pretty well what I meant in the message
that prompted your response, to which I am responding to.  I would
imagine that "git stash clear [-f|-n]" with stashClear.requireForce
to be more like:

 * stashClear.requireForce that is not set is treated as if it is
   set to true;

 * "git stash clear" without -f or -n will die with a single line of
   message, telling the user to give either -f or -n, unless the
   configuration is set explicitly to 'false'.

It will be more like "confirmation" to those who run "git stash
clear", and then if they are certain, run "git stash clear -f".

Of course, "interactive" can be added like "git clean -i".

Just like "-i" defeats the refusal by "clear.requireForce" when
running "git clean", "git stash clear -i" would go interactive and
should be usable regardless of what value stashClear.requireForce
has.

> Regarding making it opt out instead of opt in, I am afraid there are
> tools that use git stash clean automatically, and making the
> confirmation opt-out (by default confirm) we can potentially break
> them, don't you think it would be more gentle to the git community to
> make it opt in?

It is usually a good idea to introduce such a disruptive feature
gradually by first making it opt-in, with a warning message to tell
users that the default will change in the future.  For this feature,
it may go like this:

 1. Phase one.

 * "git stash clear" learns "-f|--force" and "-n|--dry-run"
   options.

 * A configuration variable stashClear.requireForce is introduced.
   "git stash clear" starts paying attention to it.  When it is

    - set to true, "git stash clear" without "-f" or "-n" errors
      out.

    - set to false, "git stash clear" works just like today.  No
      extra messages, no extra errors.

    - unset, "git stash clear" gives a warning message to tell the
      users two things:

      - The stashClear.requireForce configuration variable exists
        and when it is set to true, you need to pass "-f" to make
        "git stash clear" actually clear.  When it is set to false,
        the stash is cleared just like before.  When it is unset,
        you will see this warning message, but the command clears
        the stash.

      - The default value of stashClear.requireForce will become
        true in a future version of Git, and users are encouraged to
        set their preference early by setting the configuration
        variable now.

      but otherwise it goes ahead and clears the stash.

 * Release notes talks about upcoming default flip for the variable.

 2. Phase two (probably several releases after Phase one).

 * The behaviour of "git stash clear" is changed so that when
   stashClear.requireForce is

    - set to true of false, it behaves identically as phase one.

    - unset, "git stash clear" gives a warning message to tell the
      users about the stashClear.requireForce variable just like in
      phase one (the description needs to be adjusted to say that
      the default value is now 'true'), and behaves as if the
      variable is set to 'true'.

 3. Phase three (probably several releases after Phase two).

 * Long after users have adjusted to the new "safer by default"
   behaviour, we shorten the error message given to those who do not
   have the variable configured, similar to the message "git clean"
   gives to those without clear.requireForce.

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

[Nadav Goldstein]

I completely agree with everything you said :)

I will get to work on v2, thanks!

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

[Nadav Goldstein via GitGitGadget]

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

Signed-off-by: Nadav Goldstein <nadav.goldstein@blazepod.co>
---
    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(-)

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();
 }
 

base-commit: e8005e4871f130c4e402ddca2032c111252f070a
-- 
gitgitgadget

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

[Junio C Hamano]

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