git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] stash: added safety flag for stash clear subcommand
@ 2022-05-15 21:18 Nadav Goldstein via GitGitGadget
  2022-05-16  3:17 ` Junio C Hamano
  2022-05-23  6:12 ` [PATCH v2 0/2] stash clear: " Nadav Goldstein via GitGitGadget
  0 siblings, 2 replies; 12+ messages in thread
From: Nadav Goldstein via GitGitGadget @ 2022-05-15 21:18 UTC (permalink / raw)
  To: git; +Cc: Nadav Goldstein, Nadav Goldstein

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] stash: added safety flag for stash clear subcommand
  2022-05-15 21:18 [PATCH] stash: added safety flag for stash clear subcommand Nadav Goldstein via GitGitGadget
@ 2022-05-16  3:17 ` Junio C Hamano
  2022-05-23  6:12 ` [PATCH v2 0/2] stash clear: " Nadav Goldstein via GitGitGadget
  1 sibling, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2022-05-16  3:17 UTC (permalink / raw)
  To: Nadav Goldstein via GitGitGadget; +Cc: git, Nadav Goldstein, Nadav Goldstein

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 0/2] stash clear: added safety flag for stash clear subcommand
  2022-05-15 21:18 [PATCH] stash: added safety flag for stash clear subcommand Nadav Goldstein via GitGitGadget
  2022-05-16  3:17 ` Junio C Hamano
@ 2022-05-23  6:12 ` Nadav Goldstein via GitGitGadget
  2022-05-23  6:12   ` [PATCH v2 1/2] add-menu: added add-menu to lib objects Nadav Goldstein via GitGitGadget
                     ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Nadav Goldstein via GitGitGadget @ 2022-05-23  6:12 UTC (permalink / raw)
  To: git; +Cc: Nadav Goldstein

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/2] add-menu: added add-menu to lib objects
  2022-05-23  6:12 ` [PATCH v2 0/2] stash clear: " Nadav Goldstein via GitGitGadget
@ 2022-05-23  6:12   ` Nadav Goldstein via GitGitGadget
  2022-05-23 20:03     ` Derrick Stolee
  2022-05-23  6:12   ` [PATCH v2 2/2] clean: refector to the interactive part of clean Nadav Goldstein via GitGitGadget
  2022-05-23 19:33   ` [PATCH v2 0/2] stash clear: added safety flag for stash clear subcommand Derrick Stolee
  2 siblings, 1 reply; 12+ messages in thread
From: Nadav Goldstein via GitGitGadget @ 2022-05-23  6:12 UTC (permalink / raw)
  To: git; +Cc: Nadav Goldstein, Nadav Goldstein

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 2/2] clean: refector to the interactive part of clean
  2022-05-23  6:12 ` [PATCH v2 0/2] stash clear: " Nadav Goldstein via GitGitGadget
  2022-05-23  6:12   ` [PATCH v2 1/2] add-menu: added add-menu to lib objects Nadav Goldstein via GitGitGadget
@ 2022-05-23  6:12   ` Nadav Goldstein via GitGitGadget
  2022-05-23 19:45     ` Derrick Stolee
  2022-05-23 19:33   ` [PATCH v2 0/2] stash clear: added safety flag for stash clear subcommand Derrick Stolee
  2 siblings, 1 reply; 12+ messages in thread
From: Nadav Goldstein via GitGitGadget @ 2022-05-23  6:12 UTC (permalink / raw)
  To: git; +Cc: Nadav Goldstein, Nadav Goldstein

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] stash clear: added safety flag for stash clear subcommand
  2022-05-23  6:12 ` [PATCH v2 0/2] stash clear: " Nadav Goldstein via GitGitGadget
  2022-05-23  6:12   ` [PATCH v2 1/2] add-menu: added add-menu to lib objects Nadav Goldstein via GitGitGadget
  2022-05-23  6:12   ` [PATCH v2 2/2] clean: refector to the interactive part of clean Nadav Goldstein via GitGitGadget
@ 2022-05-23 19:33   ` Derrick Stolee
  2 siblings, 0 replies; 12+ messages in thread
From: Derrick Stolee @ 2022-05-23 19:33 UTC (permalink / raw)
  To: Nadav Goldstein via GitGitGadget, git; +Cc: Nadav Goldstein

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] clean: refector to the interactive part of clean
  2022-05-23  6:12   ` [PATCH v2 2/2] clean: refector to the interactive part of clean Nadav Goldstein via GitGitGadget
@ 2022-05-23 19:45     ` Derrick Stolee
  0 siblings, 0 replies; 12+ messages in thread
From: Derrick Stolee @ 2022-05-23 19:45 UTC (permalink / raw)
  To: Nadav Goldstein via GitGitGadget, git; +Cc: Nadav Goldstein

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] add-menu: added add-menu to lib objects
  2022-05-23  6:12   ` [PATCH v2 1/2] add-menu: added add-menu to lib objects Nadav Goldstein via GitGitGadget
@ 2022-05-23 20:03     ` Derrick Stolee
  2022-05-23 20:35       ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Derrick Stolee @ 2022-05-23 20:03 UTC (permalink / raw)
  To: Nadav Goldstein via GitGitGadget, git; +Cc: Nadav Goldstein

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] add-menu: added add-menu to lib objects
  2022-05-23 20:03     ` Derrick Stolee
@ 2022-05-23 20:35       ` Junio C Hamano
  0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2022-05-23 20:35 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Nadav Goldstein via GitGitGadget, git, Nadav Goldstein

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] stash: added safety flag for stash clear subcommand
@ 2022-05-18 21:05 Nadav Goldstein
  0 siblings, 0 replies; 12+ messages in thread
From: Nadav Goldstein @ 2022-05-18 21:05 UTC (permalink / raw)
  To: gitster; +Cc: git, gitgitgadget, nadav.goldstein96, nadav.goldstein

I completely agree with everything you said :)

I will get to work on v2, thanks!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] stash: added safety flag for stash clear subcommand
  2022-05-18 18:53 [PATCH] stash: " Nadav Goldstein
@ 2022-05-18 20:51 ` Junio C Hamano
  0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2022-05-18 20:51 UTC (permalink / raw)
  To: Nadav Goldstein; +Cc: git, gitgitgadget, nadav.goldstein96

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] stash: added safety flag for stash clear subcommand
@ 2022-05-18 18:53 Nadav Goldstein
  2022-05-18 20:51 ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Nadav Goldstein @ 2022-05-18 18:53 UTC (permalink / raw)
  To: gitster; +Cc: git, gitgitgadget, nadav.goldstein96, 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?

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-05-23 20:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-15 21:18 [PATCH] stash: added safety flag for stash clear subcommand Nadav Goldstein via GitGitGadget
2022-05-16  3:17 ` Junio C Hamano
2022-05-23  6:12 ` [PATCH v2 0/2] stash clear: " Nadav Goldstein via GitGitGadget
2022-05-23  6:12   ` [PATCH v2 1/2] add-menu: added add-menu to lib objects Nadav Goldstein via GitGitGadget
2022-05-23 20:03     ` Derrick Stolee
2022-05-23 20:35       ` Junio C Hamano
2022-05-23  6:12   ` [PATCH v2 2/2] clean: refector to the interactive part of clean Nadav Goldstein via GitGitGadget
2022-05-23 19:45     ` Derrick Stolee
2022-05-23 19:33   ` [PATCH v2 0/2] stash clear: added safety flag for stash clear subcommand Derrick Stolee
2022-05-18 18:53 [PATCH] stash: " Nadav Goldstein
2022-05-18 20:51 ` Junio C Hamano
2022-05-18 21:05 Nadav Goldstein

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).