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

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

[PATCH v2 0/2] stash clear: added safety flag for stash clear subcommand

[Nadav Goldstein via GitGitGadget]

PLAN: Add 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.

Changes since v1:

Removed temporarily the interactive flag from stash. introduced add-menu lib
to the project, which is simply the extracted code that responsible for
presenting the menu in the clean command, and made the clean command use it.

This change was made to allow stash to use interactive as well, with the
same style as git clean.

Before I continue the development, I would like to know what the community
think about this refactor.

Thanks!

Nadav Goldstein (2):
  add-menu: added add-menu to lib objects
  clean: refector to the interactive part of clean

 Makefile        |   1 +
 add-menu.c      | 353 +++++++++++++++++++++++++++++++++++++++++
 add-menu.h      |  55 +++++++
 builtin/clean.c | 413 ++----------------------------------------------
 4 files changed, 425 insertions(+), 397 deletions(-)
 create mode 100644 add-menu.c
 create mode 100644 add-menu.h


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

Range-diff vs v1:

 1:  5396a67b0ff < -:  ----------- stash: added safety flag for stash clear subcommand
 -:  ----------- > 1:  13bc75a2b05 add-menu: added add-menu to lib objects
 -:  ----------- > 2:  7271a285d18 clean: refector to the interactive part of clean

-- 
gitgitgadget

[PATCH v2 1/2] add-menu: added add-menu to lib objects

[Nadav Goldstein via GitGitGadget]

From: Nadav Goldstein <nadav.goldstein96@gmail.com>

added to the lib objects the add menu module which is
simply extracted functions from clear.c
(which in the next commit will be removed and instead
clear will use the outsourced functions).

Signed-off-by: Nadav Goldstein <nadav.goldstein96@gmail.com>
---
 Makefile   |   1 +
 add-menu.c | 339 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 add-menu.h |  51 ++++++++
 3 files changed, 391 insertions(+)
 create mode 100644 add-menu.c
 create mode 100644 add-menu.h

diff --git a/Makefile b/Makefile
index f8bccfab5e9..60ed7a5e1e0 100644
--- a/Makefile
+++ b/Makefile
@@ -871,6 +871,7 @@ LIB_H = $(FOUND_H_SOURCES)
 
 LIB_OBJS += abspath.o
 LIB_OBJS += add-interactive.o
+LIB_OBJS += add-menu.o
 LIB_OBJS += add-patch.o
 LIB_OBJS += advice.o
 LIB_OBJS += alias.o
diff --git a/add-menu.c b/add-menu.c
new file mode 100644
index 00000000000..6a1c125d113
--- /dev/null
+++ b/add-menu.c
@@ -0,0 +1,339 @@
+#include <stdio.h>
+#include "builtin.h"
+#include "add-menu.h"
+#include "cache.h"
+#include "config.h"
+#include "dir.h"
+#include "parse-options.h"
+#include "string-list.h"
+#include "quote.h"
+#include "column.h"
+#include "color.h"
+#include "pathspec.h"
+#include "help.h"
+#include "prompt.h"
+
+static int clean_use_color = -1;
+static char clean_colors[][COLOR_MAXLEN] = {
+	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
+	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
+	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
+	[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,
+	[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,
+	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
+};
+
+
+static const char *clean_get_color(enum color_clean ix)
+{
+	if (want_color(clean_use_color))
+		return clean_colors[ix];
+	return "";
+}
+
+static int find_unique(const char *choice, struct menu_stuff *menu_stuff)
+{
+	struct menu_item *menu_item;
+	struct string_list_item *string_list_item;
+	int i, len, found = 0;
+
+	len = strlen(choice);
+	switch (menu_stuff->type) {
+	default:
+		die("Bad type of menu_stuff when parse choice");
+	case MENU_STUFF_TYPE_MENU_ITEM:
+
+		menu_item = (struct menu_item *)menu_stuff->stuff;
+		for (i = 0; i < menu_stuff->nr; i++, menu_item++) {
+			if (len == 1 && *choice == menu_item->hotkey) {
+				found = i + 1;
+				break;
+			}
+			if (!strncasecmp(choice, menu_item->title, len)) {
+				if (found) {
+					if (len == 1) {
+						/* continue for hotkey matching */
+						found = -1;
+					} else {
+						found = 0;
+						break;
+					}
+				} else {
+					found = i + 1;
+				}
+			}
+		}
+		break;
+	case MENU_STUFF_TYPE_STRING_LIST:
+		string_list_item = ((struct string_list *)menu_stuff->stuff)->items;
+		for (i = 0; i < menu_stuff->nr; i++, string_list_item++) {
+			if (!strncasecmp(choice, string_list_item->string, len)) {
+				if (found) {
+					found = 0;
+					break;
+				}
+				found = i + 1;
+			}
+		}
+		break;
+	}
+	return found;
+}
+
+static int parse_choice(struct menu_stuff *menu_stuff,
+			int is_single,
+			struct strbuf input,
+			int **chosen)
+{
+	struct strbuf **choice_list, **ptr;
+	int nr = 0;
+	int i;
+
+	if (is_single) {
+		choice_list = strbuf_split_max(&input, '\n', 0);
+	} else {
+		char *p = input.buf;
+		do {
+			if (*p == ',')
+				*p = ' ';
+		} while (*p++);
+		choice_list = strbuf_split_max(&input, ' ', 0);
+	}
+
+	for (ptr = choice_list; *ptr; ptr++) {
+		char *p;
+		int choose = 1;
+		int bottom = 0, top = 0;
+		int is_range, is_number;
+
+		strbuf_trim(*ptr);
+		if (!(*ptr)->len)
+			continue;
+
+		/* Input that begins with '-'; unchoose */
+		if (*(*ptr)->buf == '-') {
+			choose = 0;
+			strbuf_remove((*ptr), 0, 1);
+		}
+
+		is_range = 0;
+		is_number = 1;
+		for (p = (*ptr)->buf; *p; p++) {
+			if ('-' == *p) {
+				if (!is_range) {
+					is_range = 1;
+					is_number = 0;
+				} else {
+					is_number = 0;
+					is_range = 0;
+					break;
+				}
+			} else if (!isdigit(*p)) {
+				is_number = 0;
+				is_range = 0;
+				break;
+			}
+		}
+
+		if (is_number) {
+			bottom = atoi((*ptr)->buf);
+			top = bottom;
+		} else if (is_range) {
+			bottom = atoi((*ptr)->buf);
+			/* a range can be specified like 5-7 or 5- */
+			if (!*(strchr((*ptr)->buf, '-') + 1))
+				top = menu_stuff->nr;
+			else
+				top = atoi(strchr((*ptr)->buf, '-') + 1);
+		} else if (!strcmp((*ptr)->buf, "*")) {
+			bottom = 1;
+			top = menu_stuff->nr;
+		} else {
+			bottom = find_unique((*ptr)->buf, menu_stuff);
+			top = bottom;
+		}
+
+		if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||
+		    (is_single && bottom != top)) {
+			clean_print_color(CLEAN_COLOR_ERROR);
+			printf(_("Huh (%s)?\n"), (*ptr)->buf);
+			clean_print_color(CLEAN_COLOR_RESET);
+			continue;
+		}
+
+		for (i = bottom; i <= top; i++)
+			(*chosen)[i-1] = choose;
+	}
+
+	strbuf_list_free(choice_list);
+
+	for (i = 0; i < menu_stuff->nr; i++)
+		nr += (*chosen)[i];
+	return nr;
+}
+
+static void pretty_print_menus(struct string_list *menu_list)
+{
+	unsigned int local_colopts = 0;
+	struct column_options copts;
+
+	local_colopts = COL_ENABLED | COL_ROW;
+	memset(&copts, 0, sizeof(copts));
+	copts.indent = "  ";
+	copts.padding = 2;
+	print_columns(menu_list, local_colopts, &copts);
+}
+
+/*
+ * display menu stuff with number prefix and hotkey highlight
+ */
+static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)
+{
+	struct string_list menu_list = STRING_LIST_INIT_DUP;
+	struct strbuf menu = STRBUF_INIT;
+	struct menu_item *menu_item;
+	struct string_list_item *string_list_item;
+	int i;
+
+	switch (stuff->type) {
+	default:
+		die("Bad type of menu_stuff when print menu");
+	case MENU_STUFF_TYPE_MENU_ITEM:
+		menu_item = (struct menu_item *)stuff->stuff;
+		for (i = 0; i < stuff->nr; i++, menu_item++) {
+			const char *p;
+			int highlighted = 0;
+
+			p = menu_item->title;
+			if ((*chosen)[i] < 0)
+				(*chosen)[i] = menu_item->selected ? 1 : 0;
+			strbuf_addf(&menu, "%s%2d: ", (*chosen)[i] ? "*" : " ", i+1);
+			for (; *p; p++) {
+				if (!highlighted && *p == menu_item->hotkey) {
+					strbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_PROMPT));
+					strbuf_addch(&menu, *p);
+					strbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_RESET));
+					highlighted = 1;
+				} else {
+					strbuf_addch(&menu, *p);
+				}
+			}
+			string_list_append(&menu_list, menu.buf);
+			strbuf_reset(&menu);
+		}
+		break;
+	case MENU_STUFF_TYPE_STRING_LIST:
+		i = 0;
+		for_each_string_list_item(string_list_item, (struct string_list *)stuff->stuff) {
+			if ((*chosen)[i] < 0)
+				(*chosen)[i] = 0;
+			strbuf_addf(&menu, "%s%2d: %s",
+				    (*chosen)[i] ? "*" : " ", i+1, string_list_item->string);
+			string_list_append(&menu_list, menu.buf);
+			strbuf_reset(&menu);
+			i++;
+		}
+		break;
+	}
+
+	pretty_print_menus(&menu_list);
+
+	strbuf_release(&menu);
+	string_list_clear(&menu_list, 0);
+}
+
+int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int))
+{
+	struct strbuf choice = STRBUF_INIT;
+	int *chosen, *result;
+	int nr = 0;
+	int eof = 0;
+	int i;
+
+	ALLOC_ARRAY(chosen, stuff->nr);
+	/* set chosen as uninitialized */
+	for (i = 0; i < stuff->nr; i++)
+		chosen[i] = -1;
+
+	for (;;) {
+		if (opts->header) {
+			printf_ln("%s%s%s",
+				  clean_get_color(CLEAN_COLOR_HEADER),
+				  _(opts->header),
+				  clean_get_color(CLEAN_COLOR_RESET));
+		}
+
+		/* chosen will be initialized by print_highlight_menu_stuff */
+		print_highlight_menu_stuff(stuff, &chosen);
+
+		if (opts->flags & MENU_OPTS_LIST_ONLY)
+			break;
+
+		if (opts->prompt) {
+			printf("%s%s%s%s",
+			       clean_get_color(CLEAN_COLOR_PROMPT),
+			       _(opts->prompt),
+			       opts->flags & MENU_OPTS_SINGLETON ? "> " : ">> ",
+			       clean_get_color(CLEAN_COLOR_RESET));
+		}
+
+		if (git_read_line_interactively(&choice) == EOF) {
+			eof = 1;
+			break;
+		}
+
+		/* help for prompt */
+		if (!strcmp(choice.buf, "?")) {
+			prompt_help_cmd(opts->flags & MENU_OPTS_SINGLETON);
+			continue;
+		}
+
+		/* for a multiple-choice menu, press ENTER (empty) will return back */
+		if (!(opts->flags & MENU_OPTS_SINGLETON) && !choice.len)
+			break;
+
+		nr = parse_choice(stuff,
+				  opts->flags & MENU_OPTS_SINGLETON,
+				  choice,
+				  &chosen);
+
+		if (opts->flags & MENU_OPTS_SINGLETON) {
+			if (nr)
+				break;
+		} else if (opts->flags & MENU_OPTS_IMMEDIATE) {
+			break;
+		}
+	}
+
+	if (eof) {
+		result = xmalloc(sizeof(int));
+		*result = EOF;
+	} else {
+		int j = 0;
+
+		/*
+		 * recalculate nr, if return back from menu directly with
+		 * default selections.
+		 */
+		if (!nr) {
+			for (i = 0; i < stuff->nr; i++)
+				nr += chosen[i];
+		}
+
+		CALLOC_ARRAY(result, st_add(nr, 1));
+		for (i = 0; i < stuff->nr && j < nr; i++) {
+			if (chosen[i])
+				result[j++] = i;
+		}
+		result[j] = EOF;
+	}
+
+	free(chosen);
+	strbuf_release(&choice);
+	return result;
+}
+
+void clean_print_color(enum color_clean ix)
+{
+	printf("%s", clean_get_color(ix));
+}
\ No newline at end of file
diff --git a/add-menu.h b/add-menu.h
new file mode 100644
index 00000000000..52e5ccb1800
--- /dev/null
+++ b/add-menu.h
@@ -0,0 +1,51 @@
+#define MENU_OPTS_SINGLETON		01
+#define MENU_OPTS_IMMEDIATE		02
+#define MENU_OPTS_LIST_ONLY		04
+
+enum color_clean {
+	CLEAN_COLOR_RESET = 0,
+	CLEAN_COLOR_PLAIN = 1,
+	CLEAN_COLOR_PROMPT = 2,
+	CLEAN_COLOR_HEADER = 3,
+	CLEAN_COLOR_HELP = 4,
+	CLEAN_COLOR_ERROR = 5
+};
+
+struct menu_opts {
+	const char *header;
+	const char *prompt;
+	int flags;
+};
+
+struct menu_item {
+	char hotkey;
+	const char *title;
+	int selected;
+	int (*fn)(void);
+};
+
+enum menu_stuff_type {
+	MENU_STUFF_TYPE_STRING_LIST = 1,
+	MENU_STUFF_TYPE_MENU_ITEM
+};
+
+struct menu_stuff {
+	enum menu_stuff_type type;
+	int nr;
+	void *stuff;
+};
+
+void clean_print_color(enum color_clean ix);
+
+/*
+ * Implement a git-add-interactive compatible UI, which is borrowed
+ * from git-add--interactive.perl.
+ *
+ * Return value:
+ *
+ *   - Return an array of integers
+ *   - , and it is up to you to free the allocated memory.
+ *   - The array ends with EOF.
+ *   - If user pressed CTRL-D (i.e. EOF), no selection returned.
+ */
+int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int));
\ No newline at end of file
-- 
gitgitgadget

Re: [PATCH v2 1/2] add-menu: added add-menu to lib objects

[Derrick Stolee]

On 5/23/22 2:12 AM, Nadav Goldstein via GitGitGadget wrote:
> From: Nadav Goldstein <nadav.goldstein96@gmail.com>
> 
> added to the lib objects the add menu module which is
> simply extracted functions from clear.c
> (which in the next commit will be removed and instead
> clear will use the outsourced functions).

Please rewrite according to Git style. (Mentioned in
my reply to patch 2 with more detail.)

> diff --git a/add-menu.c b/add-menu.c

I think I said something in another place that was a
bit incorrect: I think of "git add -p" as interactive
add, but really it's "git add -i" that is the
equivalent. The "git add -i" menu is very similar to
the "git clean -i" menu, as it is said in comments.

Thus, perhaps the best thing to do would be to unify
the two implementations, if at all possible. The one
in builtin/clean.c is from 2013 while the one in
add-interactive.c is much more recent.

The biggest test of your new API is whether it can
support _both_ of these existing interactive menus
before adding one to 'git stash'.

> new file mode 100644
> index 00000000000..6a1c125d113
> --- /dev/null
> +++ b/add-menu.c
> @@ -0,0 +1,339 @@
> +#include <stdio.h>

In the Git project, the first include should either be
"cache.h" or "git-compat-utils.h". For this API,
git-compat-utils.h should suffice, since there should
not be anything from the Git data model that actually
matters here.

> +#include "builtin.h"
> +#include "add-menu.h"
> +#include "cache.h"
> +#include "config.h"
> +#include "dir.h"
> +#include "parse-options.h"
> +#include "string-list.h"
> +#include "quote.h"
> +#include "column.h"
> +#include "color.h"
> +#include "pathspec.h"
> +#include "help.h"
> +#include "prompt.h"

I doubt that these are all required. Please check to
see what you are using from each of these includes and
remove as necessary.

> +static const char *clean_get_color(enum color_clean ix)
> +{
> +	if (want_color(clean_use_color))
> +		return clean_colors[ix];
> +	return "";
> +}

Please update the method names to not care about clean.
This is especially true in the public API in the *.h file.

> +void clean_print_color(enum color_clean ix)
> +{
> +	printf("%s", clean_get_color(ix));
> +}
> \ No newline at end of file

nit: please make sure the file ends with exactly one newline.

One problem with this approach of adding the code to this
new *.c file and then later removing the code from clean is
that we lose 'git blame' or 'git log -L' history across the
move. It's much harder to detect copies than to detect moved
lines of code.

I don't have a good solution in mind right now, but it's
worth thinking about.

> diff --git a/add-menu.h b/add-menu.h
> new file mode 100644
> index 00000000000..52e5ccb1800
> --- /dev/null
> +++ b/add-menu.h
> @@ -0,0 +1,51 @@

Don't forget the #ifndef __ADD_MENU__/#define __ADD_MENU__
trick to avoid multiple declarations of these values.

> +int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int));
> \ No newline at end of file

nit: newline here, too.

Thanks,
-Stolee

Re: [PATCH v2 1/2] add-menu: added add-menu to lib objects

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> The biggest test of your new API is whether it can
> support _both_ of these existing interactive menus
> before adding one to 'git stash'.

A great piece of advice.

> One problem with this approach of adding the code to this
> new *.c file and then later removing the code from clean is
> that we lose 'git blame' or 'git log -L' history across the
> move. It's much harder to detect copies than to detect moved
> lines of code.
>
> I don't have a good solution in mind right now, but it's
> worth thinking about.

I actually do ;-)

One single preliminary patch can rename the helper functions, update
the direct reference to the color configuration table into passing a
parameter to the table, doing the same to the menu that defines the
shorthand, help text, and implementation of the command, and any
other necessary adjustment.  Or if these tasks are turns out to be
too large to do in a single commit, they can be done in steps.  This
preliminary refactoring patch (or a series of patches) can be done
while everything is still in builtin/clean.c (or we could start by
refactoring the one in add-interactive.c instead).

Then the second patch in the series would move the refactored
library-ish code into the new interactive-menu.c file, and add the
interactive-menu.h header for users to include.  The first such user
will be builtin/clean.c (or if we started from add-interactive.c,
then that one).  "blame", "log" and "diff --color-moved" would all
notice that an already refactored block of code was lifted from one
source file and moved to the new place in this step.

The third patch in the series would convert add-interactive.c (or if
we started from it, then builtin/clean.c) to be the second user of
the API.  There might need a few preliminary steps in the file to be
converted before it happens to match the function signatures, etc.

After that, "git stash" will have its own interactive mode that uses
the new API from day one.

[PATCH v2 2/2] clean: refector to the interactive part of clean

[Nadav Goldstein via GitGitGadget]

From: Nadav Goldstein <nadav.goldstein96@gmail.com>

moved the code that responsible for presenting the menu options
(interactive flag) to be handles by the add-menu lib that was added
in previous commit.

Signed-off-by: Nadav Goldstein <nadav.goldstein96@gmail.com>
---
 add-menu.c      |  78 +++++----
 add-menu.h      |   8 +-
 builtin/clean.c | 413 ++----------------------------------------------
 3 files changed, 68 insertions(+), 431 deletions(-)

diff --git a/add-menu.c b/add-menu.c
index 6a1c125d113..becf4956917 100644
--- a/add-menu.c
+++ b/add-menu.c
@@ -13,24 +13,18 @@
 #include "help.h"
 #include "prompt.h"
 
-static int clean_use_color = -1;
-static char clean_colors[][COLOR_MAXLEN] = {
-	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
-	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
-	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
-	[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,
-	[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,
-	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
-};
-
-
-static const char *clean_get_color(enum color_clean ix)
+static const char *clean_get_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color)
 {
-	if (want_color(clean_use_color))
-		return clean_colors[ix];
+	if (want_color(*clean_use_color))
+		return (*clean_colors)[ix];
 	return "";
 }
 
+void clean_print_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color)
+{
+	printf("%s", clean_get_color(ix, clean_colors, clean_use_color));
+}
+
 static int find_unique(const char *choice, struct menu_stuff *menu_stuff)
 {
 	struct menu_item *menu_item;
@@ -80,10 +74,33 @@ static int find_unique(const char *choice, struct menu_stuff *menu_stuff)
 	return found;
 }
 
+/*
+ * Parse user input, and return choice(s) for menu (menu_stuff).
+ *
+ * Input
+ *     (for single choice)
+ *         1          - select a numbered item
+ *         foo        - select item based on menu title
+ *                    - (empty) select nothing
+ *
+ *     (for multiple choice)
+ *         1          - select a single item
+ *         3-5        - select a range of items
+ *         2-3,6-9    - select multiple ranges
+ *         foo        - select item based on menu title
+ *         -...       - unselect specified items
+ *         *          - choose all items
+ *                    - (empty) finish selecting
+ *
+ * The parse result will be saved in array **chosen, and
+ * return number of total selections.
+ */
 static int parse_choice(struct menu_stuff *menu_stuff,
 			int is_single,
 			struct strbuf input,
-			int **chosen)
+			int **chosen,
+			clean_color_settings *clean_colors,
+			int *clean_use_color)
 {
 	struct strbuf **choice_list, **ptr;
 	int nr = 0;
@@ -155,9 +172,9 @@ static int parse_choice(struct menu_stuff *menu_stuff,
 
 		if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||
 		    (is_single && bottom != top)) {
-			clean_print_color(CLEAN_COLOR_ERROR);
+			clean_print_color(CLEAN_COLOR_ERROR, clean_colors, clean_use_color);
 			printf(_("Huh (%s)?\n"), (*ptr)->buf);
-			clean_print_color(CLEAN_COLOR_RESET);
+			clean_print_color(CLEAN_COLOR_RESET, clean_colors, clean_use_color);
 			continue;
 		}
 
@@ -187,7 +204,7 @@ static void pretty_print_menus(struct string_list *menu_list)
 /*
  * display menu stuff with number prefix and hotkey highlight
  */
-static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)
+static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen, clean_color_settings *clean_colors, int *clean_use_color)
 {
 	struct string_list menu_list = STRING_LIST_INIT_DUP;
 	struct strbuf menu = STRBUF_INIT;
@@ -210,9 +227,9 @@ static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)
 			strbuf_addf(&menu, "%s%2d: ", (*chosen)[i] ? "*" : " ", i+1);
 			for (; *p; p++) {
 				if (!highlighted && *p == menu_item->hotkey) {
-					strbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_PROMPT));
+					strbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_PROMPT, clean_colors, clean_use_color));
 					strbuf_addch(&menu, *p);
-					strbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_RESET));
+					strbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_RESET, clean_colors, clean_use_color));
 					highlighted = 1;
 				} else {
 					strbuf_addch(&menu, *p);
@@ -242,7 +259,7 @@ static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)
 	string_list_clear(&menu_list, 0);
 }
 
-int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int))
+int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, clean_color_settings *clean_colors, int *clean_use_color, void (*prompt_help_cmd)(int))
 {
 	struct strbuf choice = STRBUF_INIT;
 	int *chosen, *result;
@@ -258,23 +275,23 @@ int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*pr
 	for (;;) {
 		if (opts->header) {
 			printf_ln("%s%s%s",
-				  clean_get_color(CLEAN_COLOR_HEADER),
+				  clean_get_color(CLEAN_COLOR_HEADER, clean_colors, clean_use_color),
 				  _(opts->header),
-				  clean_get_color(CLEAN_COLOR_RESET));
+				  clean_get_color(CLEAN_COLOR_RESET, clean_colors, clean_use_color));
 		}
 
 		/* chosen will be initialized by print_highlight_menu_stuff */
-		print_highlight_menu_stuff(stuff, &chosen);
+		print_highlight_menu_stuff(stuff, &chosen, clean_colors, clean_use_color);
 
 		if (opts->flags & MENU_OPTS_LIST_ONLY)
 			break;
 
 		if (opts->prompt) {
 			printf("%s%s%s%s",
-			       clean_get_color(CLEAN_COLOR_PROMPT),
+			       clean_get_color(CLEAN_COLOR_PROMPT, clean_colors, clean_use_color),
 			       _(opts->prompt),
 			       opts->flags & MENU_OPTS_SINGLETON ? "> " : ">> ",
-			       clean_get_color(CLEAN_COLOR_RESET));
+			       clean_get_color(CLEAN_COLOR_RESET, clean_colors, clean_use_color));
 		}
 
 		if (git_read_line_interactively(&choice) == EOF) {
@@ -295,7 +312,9 @@ int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*pr
 		nr = parse_choice(stuff,
 				  opts->flags & MENU_OPTS_SINGLETON,
 				  choice,
-				  &chosen);
+				  &chosen,
+				  clean_colors,
+				  clean_use_color);
 
 		if (opts->flags & MENU_OPTS_SINGLETON) {
 			if (nr)
@@ -331,9 +350,4 @@ int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*pr
 	free(chosen);
 	strbuf_release(&choice);
 	return result;
-}
-
-void clean_print_color(enum color_clean ix)
-{
-	printf("%s", clean_get_color(ix));
 }
\ No newline at end of file
diff --git a/add-menu.h b/add-menu.h
index 52e5ccb1800..64f1cbdab9f 100644
--- a/add-menu.h
+++ b/add-menu.h
@@ -1,3 +1,7 @@
+#include "color.h"
+
+typedef char clean_color_settings[][COLOR_MAXLEN];
+
 #define MENU_OPTS_SINGLETON		01
 #define MENU_OPTS_IMMEDIATE		02
 #define MENU_OPTS_LIST_ONLY		04
@@ -35,7 +39,7 @@ struct menu_stuff {
 	void *stuff;
 };
 
-void clean_print_color(enum color_clean ix);
+void clean_print_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color);
 
 /*
  * Implement a git-add-interactive compatible UI, which is borrowed
@@ -48,4 +52,4 @@ void clean_print_color(enum color_clean ix);
  *   - The array ends with EOF.
  *   - If user pressed CTRL-D (i.e. EOF), no selection returned.
  */
-int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int));
\ No newline at end of file
+int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, clean_color_settings *clean_colors, int *clean_use_color, void (*prompt_help_cmd)(int));
diff --git a/builtin/clean.c b/builtin/clean.c
index 5466636e666..ef220869851 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -19,6 +19,7 @@
 #include "pathspec.h"
 #include "help.h"
 #include "prompt.h"
+#include "add-menu.h"
 
 static int force = -1; /* unset */
 static int interactive;
@@ -39,14 +40,6 @@ static const char *msg_warn_lstat_failed = N_("could not lstat %s\n");
 static const char *msg_skip_cwd = N_("Refusing to remove current working directory\n");
 static const char *msg_would_skip_cwd = N_("Would refuse to remove current working directory\n");
 
-enum color_clean {
-	CLEAN_COLOR_RESET = 0,
-	CLEAN_COLOR_PLAIN = 1,
-	CLEAN_COLOR_PROMPT = 2,
-	CLEAN_COLOR_HEADER = 3,
-	CLEAN_COLOR_HELP = 4,
-	CLEAN_COLOR_ERROR = 5
-};
 
 static const char *color_interactive_slots[] = {
 	[CLEAN_COLOR_ERROR]  = "error",
@@ -58,7 +51,7 @@ static const char *color_interactive_slots[] = {
 };
 
 static int clean_use_color = -1;
-static char clean_colors[][COLOR_MAXLEN] = {
+static clean_color_settings clean_colors = {
 	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
 	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
 	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
@@ -67,36 +60,8 @@ static char clean_colors[][COLOR_MAXLEN] = {
 	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
 };
 
-#define MENU_OPTS_SINGLETON		01
-#define MENU_OPTS_IMMEDIATE		02
-#define MENU_OPTS_LIST_ONLY		04
-
-struct menu_opts {
-	const char *header;
-	const char *prompt;
-	int flags;
-};
-
 #define MENU_RETURN_NO_LOOP		10
 
-struct menu_item {
-	char hotkey;
-	const char *title;
-	int selected;
-	int (*fn)(void);
-};
-
-enum menu_stuff_type {
-	MENU_STUFF_TYPE_STRING_LIST = 1,
-	MENU_STUFF_TYPE_MENU_ITEM
-};
-
-struct menu_stuff {
-	enum menu_stuff_type type;
-	int nr;
-	void *stuff;
-};
-
 define_list_config_array(color_interactive_slots);
 
 static int git_clean_config(const char *var, const char *value, void *cb)
@@ -130,18 +95,6 @@ static int git_clean_config(const char *var, const char *value, void *cb)
 	return git_color_default_config(var, value, cb);
 }
 
-static const char *clean_get_color(enum color_clean ix)
-{
-	if (want_color(clean_use_color))
-		return clean_colors[ix];
-	return "";
-}
-
-static void clean_print_color(enum color_clean ix)
-{
-	printf("%s", clean_get_color(ix));
-}
-
 static int exclude_cb(const struct option *opt, const char *arg, int unset)
 {
 	struct string_list *exclude_list = opt->value;
@@ -307,21 +260,9 @@ static void pretty_print_dels(void)
 	string_list_clear(&list, 0);
 }
 
-static void pretty_print_menus(struct string_list *menu_list)
-{
-	unsigned int local_colopts = 0;
-	struct column_options copts;
-
-	local_colopts = COL_ENABLED | COL_ROW;
-	memset(&copts, 0, sizeof(copts));
-	copts.indent = "  ";
-	copts.padding = 2;
-	print_columns(menu_list, local_colopts, &copts);
-}
-
 static void prompt_help_cmd(int singleton)
 {
-	clean_print_color(CLEAN_COLOR_HELP);
+	clean_print_color(CLEAN_COLOR_HELP, &clean_colors, &clean_use_color);
 	printf(singleton ?
 		  _("Prompt help:\n"
 		    "1          - select a numbered item\n"
@@ -335,329 +276,7 @@ static void prompt_help_cmd(int singleton)
 		    "-...       - unselect specified items\n"
 		    "*          - choose all items\n"
 		    "           - (empty) finish selecting\n"));
-	clean_print_color(CLEAN_COLOR_RESET);
-}
-
-/*
- * display menu stuff with number prefix and hotkey highlight
- */
-static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)
-{
-	struct string_list menu_list = STRING_LIST_INIT_DUP;
-	struct strbuf menu = STRBUF_INIT;
-	struct menu_item *menu_item;
-	struct string_list_item *string_list_item;
-	int i;
-
-	switch (stuff->type) {
-	default:
-		die("Bad type of menu_stuff when print menu");
-	case MENU_STUFF_TYPE_MENU_ITEM:
-		menu_item = (struct menu_item *)stuff->stuff;
-		for (i = 0; i < stuff->nr; i++, menu_item++) {
-			const char *p;
-			int highlighted = 0;
-
-			p = menu_item->title;
-			if ((*chosen)[i] < 0)
-				(*chosen)[i] = menu_item->selected ? 1 : 0;
-			strbuf_addf(&menu, "%s%2d: ", (*chosen)[i] ? "*" : " ", i+1);
-			for (; *p; p++) {
-				if (!highlighted && *p == menu_item->hotkey) {
-					strbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_PROMPT));
-					strbuf_addch(&menu, *p);
-					strbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_RESET));
-					highlighted = 1;
-				} else {
-					strbuf_addch(&menu, *p);
-				}
-			}
-			string_list_append(&menu_list, menu.buf);
-			strbuf_reset(&menu);
-		}
-		break;
-	case MENU_STUFF_TYPE_STRING_LIST:
-		i = 0;
-		for_each_string_list_item(string_list_item, (struct string_list *)stuff->stuff) {
-			if ((*chosen)[i] < 0)
-				(*chosen)[i] = 0;
-			strbuf_addf(&menu, "%s%2d: %s",
-				    (*chosen)[i] ? "*" : " ", i+1, string_list_item->string);
-			string_list_append(&menu_list, menu.buf);
-			strbuf_reset(&menu);
-			i++;
-		}
-		break;
-	}
-
-	pretty_print_menus(&menu_list);
-
-	strbuf_release(&menu);
-	string_list_clear(&menu_list, 0);
-}
-
-static int find_unique(const char *choice, struct menu_stuff *menu_stuff)
-{
-	struct menu_item *menu_item;
-	struct string_list_item *string_list_item;
-	int i, len, found = 0;
-
-	len = strlen(choice);
-	switch (menu_stuff->type) {
-	default:
-		die("Bad type of menu_stuff when parse choice");
-	case MENU_STUFF_TYPE_MENU_ITEM:
-
-		menu_item = (struct menu_item *)menu_stuff->stuff;
-		for (i = 0; i < menu_stuff->nr; i++, menu_item++) {
-			if (len == 1 && *choice == menu_item->hotkey) {
-				found = i + 1;
-				break;
-			}
-			if (!strncasecmp(choice, menu_item->title, len)) {
-				if (found) {
-					if (len == 1) {
-						/* continue for hotkey matching */
-						found = -1;
-					} else {
-						found = 0;
-						break;
-					}
-				} else {
-					found = i + 1;
-				}
-			}
-		}
-		break;
-	case MENU_STUFF_TYPE_STRING_LIST:
-		string_list_item = ((struct string_list *)menu_stuff->stuff)->items;
-		for (i = 0; i < menu_stuff->nr; i++, string_list_item++) {
-			if (!strncasecmp(choice, string_list_item->string, len)) {
-				if (found) {
-					found = 0;
-					break;
-				}
-				found = i + 1;
-			}
-		}
-		break;
-	}
-	return found;
-}
-
-/*
- * Parse user input, and return choice(s) for menu (menu_stuff).
- *
- * Input
- *     (for single choice)
- *         1          - select a numbered item
- *         foo        - select item based on menu title
- *                    - (empty) select nothing
- *
- *     (for multiple choice)
- *         1          - select a single item
- *         3-5        - select a range of items
- *         2-3,6-9    - select multiple ranges
- *         foo        - select item based on menu title
- *         -...       - unselect specified items
- *         *          - choose all items
- *                    - (empty) finish selecting
- *
- * The parse result will be saved in array **chosen, and
- * return number of total selections.
- */
-static int parse_choice(struct menu_stuff *menu_stuff,
-			int is_single,
-			struct strbuf input,
-			int **chosen)
-{
-	struct strbuf **choice_list, **ptr;
-	int nr = 0;
-	int i;
-
-	if (is_single) {
-		choice_list = strbuf_split_max(&input, '\n', 0);
-	} else {
-		char *p = input.buf;
-		do {
-			if (*p == ',')
-				*p = ' ';
-		} while (*p++);
-		choice_list = strbuf_split_max(&input, ' ', 0);
-	}
-
-	for (ptr = choice_list; *ptr; ptr++) {
-		char *p;
-		int choose = 1;
-		int bottom = 0, top = 0;
-		int is_range, is_number;
-
-		strbuf_trim(*ptr);
-		if (!(*ptr)->len)
-			continue;
-
-		/* Input that begins with '-'; unchoose */
-		if (*(*ptr)->buf == '-') {
-			choose = 0;
-			strbuf_remove((*ptr), 0, 1);
-		}
-
-		is_range = 0;
-		is_number = 1;
-		for (p = (*ptr)->buf; *p; p++) {
-			if ('-' == *p) {
-				if (!is_range) {
-					is_range = 1;
-					is_number = 0;
-				} else {
-					is_number = 0;
-					is_range = 0;
-					break;
-				}
-			} else if (!isdigit(*p)) {
-				is_number = 0;
-				is_range = 0;
-				break;
-			}
-		}
-
-		if (is_number) {
-			bottom = atoi((*ptr)->buf);
-			top = bottom;
-		} else if (is_range) {
-			bottom = atoi((*ptr)->buf);
-			/* a range can be specified like 5-7 or 5- */
-			if (!*(strchr((*ptr)->buf, '-') + 1))
-				top = menu_stuff->nr;
-			else
-				top = atoi(strchr((*ptr)->buf, '-') + 1);
-		} else if (!strcmp((*ptr)->buf, "*")) {
-			bottom = 1;
-			top = menu_stuff->nr;
-		} else {
-			bottom = find_unique((*ptr)->buf, menu_stuff);
-			top = bottom;
-		}
-
-		if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||
-		    (is_single && bottom != top)) {
-			clean_print_color(CLEAN_COLOR_ERROR);
-			printf(_("Huh (%s)?\n"), (*ptr)->buf);
-			clean_print_color(CLEAN_COLOR_RESET);
-			continue;
-		}
-
-		for (i = bottom; i <= top; i++)
-			(*chosen)[i-1] = choose;
-	}
-
-	strbuf_list_free(choice_list);
-
-	for (i = 0; i < menu_stuff->nr; i++)
-		nr += (*chosen)[i];
-	return nr;
-}
-
-/*
- * Implement a git-add-interactive compatible UI, which is borrowed
- * from git-add--interactive.perl.
- *
- * Return value:
- *
- *   - Return an array of integers
- *   - , and it is up to you to free the allocated memory.
- *   - The array ends with EOF.
- *   - If user pressed CTRL-D (i.e. EOF), no selection returned.
- */
-static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)
-{
-	struct strbuf choice = STRBUF_INIT;
-	int *chosen, *result;
-	int nr = 0;
-	int eof = 0;
-	int i;
-
-	ALLOC_ARRAY(chosen, stuff->nr);
-	/* set chosen as uninitialized */
-	for (i = 0; i < stuff->nr; i++)
-		chosen[i] = -1;
-
-	for (;;) {
-		if (opts->header) {
-			printf_ln("%s%s%s",
-				  clean_get_color(CLEAN_COLOR_HEADER),
-				  _(opts->header),
-				  clean_get_color(CLEAN_COLOR_RESET));
-		}
-
-		/* chosen will be initialized by print_highlight_menu_stuff */
-		print_highlight_menu_stuff(stuff, &chosen);
-
-		if (opts->flags & MENU_OPTS_LIST_ONLY)
-			break;
-
-		if (opts->prompt) {
-			printf("%s%s%s%s",
-			       clean_get_color(CLEAN_COLOR_PROMPT),
-			       _(opts->prompt),
-			       opts->flags & MENU_OPTS_SINGLETON ? "> " : ">> ",
-			       clean_get_color(CLEAN_COLOR_RESET));
-		}
-
-		if (git_read_line_interactively(&choice) == EOF) {
-			eof = 1;
-			break;
-		}
-
-		/* help for prompt */
-		if (!strcmp(choice.buf, "?")) {
-			prompt_help_cmd(opts->flags & MENU_OPTS_SINGLETON);
-			continue;
-		}
-
-		/* for a multiple-choice menu, press ENTER (empty) will return back */
-		if (!(opts->flags & MENU_OPTS_SINGLETON) && !choice.len)
-			break;
-
-		nr = parse_choice(stuff,
-				  opts->flags & MENU_OPTS_SINGLETON,
-				  choice,
-				  &chosen);
-
-		if (opts->flags & MENU_OPTS_SINGLETON) {
-			if (nr)
-				break;
-		} else if (opts->flags & MENU_OPTS_IMMEDIATE) {
-			break;
-		}
-	}
-
-	if (eof) {
-		result = xmalloc(sizeof(int));
-		*result = EOF;
-	} else {
-		int j = 0;
-
-		/*
-		 * recalculate nr, if return back from menu directly with
-		 * default selections.
-		 */
-		if (!nr) {
-			for (i = 0; i < stuff->nr; i++)
-				nr += chosen[i];
-		}
-
-		CALLOC_ARRAY(result, st_add(nr, 1));
-		for (i = 0; i < stuff->nr && j < nr; i++) {
-			if (chosen[i])
-				result[j++] = i;
-		}
-		result[j] = EOF;
-	}
-
-	free(chosen);
-	strbuf_release(&choice);
-	return result;
+	clean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);
 }
 
 static int clean_cmd(void)
@@ -681,9 +300,9 @@ static int filter_by_patterns_cmd(void)
 		if (changed)
 			pretty_print_dels();
 
-		clean_print_color(CLEAN_COLOR_PROMPT);
+		clean_print_color(CLEAN_COLOR_PROMPT, &clean_colors, &clean_use_color);
 		printf(_("Input ignore patterns>> "));
-		clean_print_color(CLEAN_COLOR_RESET);
+		clean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);
 		if (git_read_line_interactively(&confirm) == EOF)
 			putchar('\n');
 
@@ -715,9 +334,9 @@ static int filter_by_patterns_cmd(void)
 		if (changed) {
 			string_list_remove_empty_items(&del_list, 0);
 		} else {
-			clean_print_color(CLEAN_COLOR_ERROR);
+			clean_print_color(CLEAN_COLOR_ERROR, &clean_colors, &clean_use_color);
 			printf_ln(_("WARNING: Cannot find items matched by: %s"), confirm.buf);
-			clean_print_color(CLEAN_COLOR_RESET);
+			clean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);
 		}
 
 		strbuf_list_free(ignore_list);
@@ -744,7 +363,7 @@ static int select_by_numbers_cmd(void)
 	menu_stuff.stuff = &del_list;
 	menu_stuff.nr = del_list.nr;
 
-	chosen = list_and_choose(&menu_opts, &menu_stuff);
+	chosen = list_and_choose(&menu_opts, &menu_stuff, &clean_colors, &clean_use_color, prompt_help_cmd);
 	items = del_list.items;
 	for (i = 0, j = 0; i < del_list.nr; i++) {
 		if (i < chosen[j]) {
@@ -807,7 +426,7 @@ static int quit_cmd(void)
 
 static int help_cmd(void)
 {
-	clean_print_color(CLEAN_COLOR_HELP);
+	clean_print_color(CLEAN_COLOR_HELP, &clean_colors, &clean_use_color);
 	printf_ln(_(
 		    "clean               - start cleaning\n"
 		    "filter by pattern   - exclude items from deletion\n"
@@ -817,7 +436,7 @@ static int help_cmd(void)
 		    "help                - this screen\n"
 		    "?                   - help for prompt selection"
 		   ));
-	clean_print_color(CLEAN_COLOR_RESET);
+	clean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);
 	return 0;
 }
 
@@ -844,15 +463,15 @@ static void interactive_main_loop(void)
 		menu_stuff.stuff = menus;
 		menu_stuff.nr = sizeof(menus) / sizeof(struct menu_item);
 
-		clean_print_color(CLEAN_COLOR_HEADER);
+		clean_print_color(CLEAN_COLOR_HEADER, &clean_colors, &clean_use_color);
 		printf_ln(Q_("Would remove the following item:",
 			     "Would remove the following items:",
 			     del_list.nr));
-		clean_print_color(CLEAN_COLOR_RESET);
+		clean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);
 
 		pretty_print_dels();
 
-		chosen = list_and_choose(&menu_opts, &menu_stuff);
+		chosen = list_and_choose(&menu_opts, &menu_stuff, &clean_colors, &clean_use_color, prompt_help_cmd);
 
 		if (*chosen != EOF) {
 			int ret;
@@ -860,9 +479,9 @@ static void interactive_main_loop(void)
 			if (ret != MENU_RETURN_NO_LOOP) {
 				FREE_AND_NULL(chosen);
 				if (!del_list.nr) {
-					clean_print_color(CLEAN_COLOR_ERROR);
+					clean_print_color(CLEAN_COLOR_ERROR, &clean_colors, &clean_use_color);
 					printf_ln(_("No more files to clean, exiting."));
-					clean_print_color(CLEAN_COLOR_RESET);
+					clean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);
 					break;
 				}
 				continue;
-- 
gitgitgadget

Re: [PATCH v2 2/2] clean: refector to the interactive part of clean

[Derrick Stolee]

On 5/23/22 2:12 AM, Nadav Goldstein via GitGitGadget wrote:
> From: Nadav Goldstein <nadav.goldstein96@gmail.com>
> 
> moved the code that responsible for presenting the menu options
> (interactive flag) to be handles by the add-menu lib that was added
> in previous commit.

Please see Documentation/SubmittingPatches, specifically the
"present tense" section [1]. 

[1] https://github.com/git/git/blob/f9b95943b68b6b8ca5a6072f50a08411c6449b55/Documentation/SubmittingPatches#L174-L179

> -static int clean_use_color = -1;
> -static char clean_colors[][COLOR_MAXLEN] = {
> -	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
> -	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
> -	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
> -	[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,
> -	[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,
> -	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
> -};
> -
> -
> -static const char *clean_get_color(enum color_clean ix)
> +static const char *clean_get_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color)

I'm surprised to see changes to add-menu.c in this patch. I
would expect that add-menu.c was written in its correct form
in the first patch and then this patch could focus entirely
on deleting matching code from builtin/clean.c and calling
the API exposed in add-menu.h.

Perhaps this was just an interactive rebase issue? Fixed up
the wrong commit? That happens to me a lot.

There are also a lot of places that refer to "clean" when
you want this API to be abstracted away from 'git clean'.

> \ No newline at end of file
> diff --git a/add-menu.h b/add-menu.h
> index 52e5ccb1800..64f1cbdab9f 100644
> --- a/add-menu.h
> +++ b/add-menu.h
> @@ -1,3 +1,7 @@
> +#include "color.h"
> +
> +typedef char clean_color_settings[][COLOR_MAXLEN];

This typedef also shouldn't reference 'git clean'.

> +
>  #define MENU_OPTS_SINGLETON		01
>  #define MENU_OPTS_IMMEDIATE		02
>  #define MENU_OPTS_LIST_ONLY		04
> @@ -35,7 +39,7 @@ struct menu_stuff {
>  	void *stuff;
>  };
>  
> -void clean_print_color(enum color_clean ix);
> +void clean_print_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color);
>  
>  /*
>   * Implement a git-add-interactive compatible UI, which is borrowed
> @@ -48,4 +52,4 @@ void clean_print_color(enum color_clean ix);
>   *   - The array ends with EOF.
>   *   - If user pressed CTRL-D (i.e. EOF), no selection returned.
>   */
> -int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int));
> \ No newline at end of file
> +int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, clean_color_settings *clean_colors, int *clean_use_color, void (*prompt_help_cmd)(int));

There are a lot of instances where your lines are much too
wide. Documentation/CodingGuidelines has a lot of style
requirements, including trying to stay to an 80-character
width. There are a lot of examples in the codebase that
show how to break a method prototype across multiple lines
with tasteful vertical alignment.

> diff --git a/builtin/clean.c b/builtin/clean.c
> index 5466636e666..ef220869851 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -19,6 +19,7 @@
>  #include "pathspec.h"
>  #include "help.h"
>  #include "prompt.h"
> +#include "add-menu.h"
>  
>  static int force = -1; /* unset */
>  static int interactive;
> @@ -39,14 +40,6 @@ static const char *msg_warn_lstat_failed = N_("could not lstat %s\n");
>  static const char *msg_skip_cwd = N_("Refusing to remove current working directory\n");
>  static const char *msg_would_skip_cwd = N_("Would refuse to remove current working directory\n");
>  
> -enum color_clean {
> -	CLEAN_COLOR_RESET = 0,
> -	CLEAN_COLOR_PLAIN = 1,
> -	CLEAN_COLOR_PROMPT = 2,
> -	CLEAN_COLOR_HEADER = 3,
> -	CLEAN_COLOR_HELP = 4,
> -	CLEAN_COLOR_ERROR = 5
> -};

This removal doesn't seem valuable. I think this color set
should remain in the builtin, especially because it's being
used further down.

Alternatively, the names can be renamed to "MENU_COLOR_..."
to apply to all commands in the add-menu.h API.

>  static const char *color_interactive_slots[] = {
>  	[CLEAN_COLOR_ERROR]  = "error",
> @@ -58,7 +51,7 @@ static const char *color_interactive_slots[] = {
>  };
>  
>  static int clean_use_color = -1;
> -static char clean_colors[][COLOR_MAXLEN] = {
> +static clean_color_settings clean_colors = {
>  	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
>  	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
>  	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
> @@ -67,36 +60,8 @@ static char clean_colors[][COLOR_MAXLEN] = {
>  	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
>  };
>  
> -#define MENU_OPTS_SINGLETON		01
> -#define MENU_OPTS_IMMEDIATE		02
> -#define MENU_OPTS_LIST_ONLY		04
> -
> -struct menu_opts {
> -	const char *header;
> -	const char *prompt;
> -	int flags;
> -};
> -

Generally, the remainder of this patch is primarily
deletions. Although, we could make it be completely
deletions if the API method names are changed (not to
start with "clean_" and then all these calls are
modified in one go:

>  static void prompt_help_cmd(int singleton)
>  {
> -	clean_print_color(CLEAN_COLOR_HELP);
> +	clean_print_color(CLEAN_COLOR_HELP, &clean_colors, &clean_use_color);

Here, we would have something like "menu_print_color()"
instead. To avoid adding too much deletion noise when
making these important changes, we can add MAYBE_UNUSED
to all of the static methods that become unreachable.

After a patch that does that refactor, a diff that only
deletes lines (does not add any) would be very easy to
verify.

Thanks,
-Stolee

Re: [PATCH v2 0/2] stash clear: added safety flag for stash clear subcommand

[Derrick Stolee]

On 5/23/22 2:12 AM, Nadav Goldstein via GitGitGadget wrote:
> PLAN: Add 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.
> 
> Changes since v1:

It looks like you completely changed your approach here. It's perfectly
fine to use the same PR and keep the Git mailing list thread intact.

However, it would be good to keep the cover letter and title up-to-date
with what you are submitting. (Now that you have multiple patches, GGG
will split the PR Title and Description into your cover letter message.)

What you actually implement is a refactor to make 'git clean's
interactive menu be implemented in libgit.a.

> Removed temporarily the interactive flag from stash. introduced add-menu lib
> to the project, which is simply the extracted code that responsible for
> presenting the menu in the clean command, and made the clean command use it.
> 
> This change was made to allow stash to use interactive as well, with the
> same style as git clean.

It is interesting that you'd model 'git stash -i' after 'git clean -i'
instead of something like 'git add -p'. It would be interesting to see
your intended 'git stash' menu based on this API before we commit to
the extraction.

Thanks,
-Stolee

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

[Nadav Goldstein via GitGitGadget]

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.

Signed-off-by: Nadav Goldstein <nadav.goldstein96@gmail.com>
---
    stash clear: added safety flag for stash clear subcommand
    
    This patch started to solve the issue of easy trigger of git stash
    clear. I first went with using an interactive (-i) flag to stash clear,
    but following the conversations I had here I understood that it was a
    misleading flag and not a good direction for implementation.
    
    So in this version of the patch (v3), I went with Junio proposal to
    introduce force flag to the clean subcommand.
    
    Thanks!

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

Range-diff vs v2:

 1:  13bc75a2b05 < -:  ----------- add-menu: added add-menu to lib objects
 2:  7271a285d18 < -:  ----------- clean: refector to the interactive part of clean
 -:  ----------- > 1:  6150ec27b5a Introduced force flag to the git stash clear subcommand.


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

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index f4bb6114d91..e95410d507e 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -20,7 +20,7 @@ SYNOPSIS
 	     [--] [<pathspec>...]]
 'git stash' save [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]
 	     [-u | --include-untracked] [-a | --all] [<message>]
-'git stash' clear
+'git stash' clear [-f | --force]
 'git stash' create [<message>]
 'git stash' store [(-m | --message) <message>] [-q | --quiet] <commit>
 
@@ -130,7 +130,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 [-f|--force]::
 	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).
@@ -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
++
+If the Git configuration variable stash.requireForce is set
+to true, 'git stash clear' will refuse to remove all the stash 
+entries unless given -f.
+
 -S::
 --staged::
 	This option is only valid for `push` and `save` commands.
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;
 
 /*
  * w_commit is set to the commit containing the working tree
@@ -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()
 	};
 
@@ -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")) {
+		clear_require_force = git_config_bool(var, value);
+		return 0;
+	}
 	return git_diff_basic_config(var, value, cb);
 }
 

base-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d
-- 
gitgitgadget

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

[Junio C Hamano]

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

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

[Nadav Goldstein]

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


I see, but isn't the same argument apply for git clean? if not adding 
the force flag, the same message as I wrote appear in git clean (I 
copied it from there), and it will exit without any other information, 
hence given your argument, running git clean is also not very useful.


One can argue that git clean --dry-run == git stash list


I suggested in the beginning of this thread to ask the user if he is 
sure he want to proceed (default to no), and only if he wrote y/yes 
proceed with the action (and force will just do it, or requireforce=false).


The reason I suggested it is because when running git stash clear, it 
will remain in the user recent commands, and when the user will navigate 
through the commands history in the terminal, he might accidentally fire 
git stash clear, and this confirmation will be another safeguard against 
this mistake.


Maybe it will be useful for git clean as well for the same reasons.
Also when the user types git clean, I argue he wanted to clean or he did 
it by mistake, and In both scenarios I don't see why making git clean 
just fail will be useful.


So what do you think? Maybe we should present in both clean and clear a 
confirmation message? (only if requireforce=true)


What do you think?

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

[Junio C Hamano]

Nadav Goldstein <nadav.goldstein96@gmail.com> writes:

> I see, but isn't the same argument apply for git clean? if not adding
> the force flag, the same message as I wrote appear in git clean (I
> copied it from there), and it will exit without any other information,
> hence given your argument, running git clean is also not very useful.

The thing is that "git clean" by default forces people to choose
between "-f" and "-n" to force people to understand the issue.  And
once they understand the issue, they'd learn to run "clean -n"
first, which lets them see what would be removed, before they run
"clean -f".  Does your "stash clear" work the same way?  I do not
think so.

If there is "stash clear --dry-run" that runs "stash list", it might
be similar, but not similar enough.  I wonder if "stash clear", when
stashClear.requireForce is set to true and unless "--force" is
given, should do "stash list" and then error out.  I dunno.

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

[Eric Sunshine]

On Tue, Jun 20, 2023 at 4:05 PM Nadav Goldstein
<nadav.goldstein96@gmail.com> wrote:
> > 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.
>
> I see, but isn't the same argument apply for git clean? if not adding
> the force flag, the same message as I wrote appear in git clean (I
> copied it from there), and it will exit without any other information,
> hence given your argument, running git clean is also not very useful.

For what it's worth, I had the same reaction as Junio upon reading
this patch; specifically, that it will train users to type "git stash
clear --force" mechanically without thinking, thus won't be much of a
safeguard.

> I suggested in the beginning of this thread to ask the user if he is
> sure he want to proceed (default to no), and only if he wrote y/yes
> proceed with the action (and force will just do it, or requireforce=false).
>
> The reason I suggested it is because when running git stash clear, it
> will remain in the user recent commands, and when the user will navigate
> through the commands history in the terminal, he might accidentally fire
> git stash clear, and this confirmation will be another safeguard against
> this mistake.
>
> Maybe it will be useful for git clean as well for the same reasons.
> Also when the user types git clean, I argue he wanted to clean or he did
> it by mistake, and In both scenarios I don't see why making git clean
> just fail will be useful.

"git clean" is in a rather different (and more severe) boat since file
deletion is irrevocable, whereas a stash thrown away by "git stash
clear" (or "git stash drop") can be recovered (at least until it gets
garbage-collected). So, rather than adding a --force option or an
interactive "yes/no" prompt, perhaps a better approach would be to
have "git stash clear" (and "git stash drop") print out advice
explaining to the user how to recover the dropped stash(es), much like
"git switch" or "git checkout" prints advice explaining how to recover
commits left dangling on a detached head.

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

[Nadav Goldstein]

I see both of your points, and I agree adding the flag will only make 
users type the flag without thinking.


But I still don't understand why do we need git clean without any flags.


The only users that will run git clean are new users that don't know you 
need to run it with -f or experienced users that set clean.requireforce 
= false.


Moreover, we can also assume that every user that have 
clean.requireforce = true (the default), and ran git clean, did so by 
mistake/intended to clean, So why don't we offer him interactive way to 
understand the consequences and confirm his action? (and explain about 
clean.requireforce like we currently do).
By doing it this way, the change will not effect experienced users 
because they either run git clean -f when they need to clean or they set 
clean.requireforce = false, and then the change will not apply to them.


This argument is also for stash clear.


Thanks.

On 21/06/2023 0:01, Eric Sunshine wrote:
> On Tue, Jun 20, 2023 at 4:05 PM Nadav Goldstein
> <nadav.goldstein96@gmail.com> wrote:
>>> 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.
>> I see, but isn't the same argument apply for git clean? if not adding
>> the force flag, the same message as I wrote appear in git clean (I
>> copied it from there), and it will exit without any other information,
>> hence given your argument, running git clean is also not very useful.
> For what it's worth, I had the same reaction as Junio upon reading
> this patch; specifically, that it will train users to type "git stash
> clear --force" mechanically without thinking, thus won't be much of a
> safeguard.
>
>> I suggested in the beginning of this thread to ask the user if he is
>> sure he want to proceed (default to no), and only if he wrote y/yes
>> proceed with the action (and force will just do it, or requireforce=false).
>>
>> The reason I suggested it is because when running git stash clear, it
>> will remain in the user recent commands, and when the user will navigate
>> through the commands history in the terminal, he might accidentally fire
>> git stash clear, and this confirmation will be another safeguard against
>> this mistake.
>>
>> Maybe it will be useful for git clean as well for the same reasons.
>> Also when the user types git clean, I argue he wanted to clean or he did
>> it by mistake, and In both scenarios I don't see why making git clean
>> just fail will be useful.
> "git clean" is in a rather different (and more severe) boat since file
> deletion is irrevocable, whereas a stash thrown away by "git stash
> clear" (or "git stash drop") can be recovered (at least until it gets
> garbage-collected). So, rather than adding a --force option or an
> interactive "yes/no" prompt, perhaps a better approach would be to
> have "git stash clear" (and "git stash drop") print out advice
> explaining to the user how to recover the dropped stash(es), much like
> "git switch" or "git checkout" prints advice explaining how to recover
> commits left dangling on a detached head.
>
>