{"thread":{"id":"57884","subject":"[PATCH] stash: added safety flag for stash clear subcommand","startedAt":"2022-05-15T21:19:05Z","lastAt":"2023-06-20T21:42:55Z","messageCount":15,"participants":["Nadav Goldstein via GitGitGadget","Junio C Hamano","Derrick Stolee","Nadav Goldstein","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455303","messageId":"pull.1232.git.1652649537647.gitgitgadget@gmail.com","threadId":"57884","inReplyTo":null,"subject":"[PATCH] stash: added safety flag for stash clear subcommand","fromName":"Nadav Goldstein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-15T21:18:57Z","receivedAt":"2022-05-15T21:19:05Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"From: Nadav Goldstein \n\nIntroduced a flag (git stash clear -i) when when used,\ninstead of silently clearing the stash, will present a\nconfirmation to the user regarding the action which he's\nabout to perform. Took the inspiration from rm -rf -i flag.\nThis flag also outputs extra logs (abort/success) to let\nthe user know more \"interactively\" what is happening with\nthe stash (hence the flag name).\n\nSigned-off-by: Nadav Goldstein \n---\n stash clear: added safety flag for stash clear subcommand\n \n Added a flag to git stash clear (-i|--interactive), which prompts the\n user to confirm he indeed wants to clear the stash, otherwise it will\n abort.\n \n I found it useful as a frequent stash clear user which means my terminal\n always have it in recent commands, which could mean accidental erase of\n work. This flag ensures accidental fires won't clear hard work :)\n \n I also thought it would be better to do it opt in, to not change the way\n the command works in prod, only add to it.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1232%2Fnadav96%2Fclear-stash-prompt-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1232/nadav96/clear-stash-prompt-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1232\n\n Documentation/git-stash.txt | 9 +++++++--\n builtin/stash.c | 21 +++++++++++++++++++--\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\nindex 6e15f475257..a7ab5379779 100644\n--- a/Documentation/git-stash.txt\n+++ b/Documentation/git-stash.txt\n@@ -17,7 +17,7 @@ SYNOPSIS\n \t [-u|--include-untracked] [-a|--all] [-m|--message ]\n \t [--pathspec-from-file= [--pathspec-file-nul]]\n \t [--] [...]]\n-'git stash' clear\n+'git stash' clear [-i|--interactive]\n 'git stash' create []\n 'git stash' store [-m|--message ] [-q|--quiet] \n \n@@ -127,7 +127,7 @@ the stash entry is applied on top of the commit that was HEAD at the\n time `git stash` was run, it restores the originally stashed state\n with no conflicts.\n \n-clear::\n+clear [-i|--interactive]::\n \tRemove all the stash entries. Note that those entries will then\n \tbe subject to pruning, and may be impossible to recover (see\n \t'Examples' below for a possible strategy).\n@@ -160,6 +160,11 @@ OPTIONS\n All ignored and untracked files are also stashed and then cleaned\n up with `git clean`.\n \n+-i::\n+--interactive::\n+\tThis option is only valid for clear command, when applied, will request\n+\ta confirmation from the user before proceeding to clear the stash.\n+\n -u::\n --include-untracked::\n --no-include-untracked::\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 0c7b6a95882..b012d24ef38 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -26,7 +26,7 @@ static const char * const git_stash_usage[] = {\n \tN_(\"git stash drop [-q|--quiet] []\"),\n \tN_(\"git stash ( pop | apply ) [--index] [-q|--quiet] []\"),\n \tN_(\"git stash branch []\"),\n-\t\"git stash clear\",\n+\t\"git stash clear [-i|--interactive]\",\n \tN_(\"git stash [push [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n \t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n \t \" [--pathspec-from-file= [--pathspec-file-nul]]\\n\"\n@@ -67,7 +67,7 @@ static const char * const git_stash_branch_usage[] = {\n };\n \n static const char * const git_stash_clear_usage[] = {\n-\t\"git stash clear\",\n+\t\"git stash clear [-i|--interactive]\",\n \tNULL\n };\n \n@@ -215,7 +215,10 @@ static int do_clear_stash(void)\n \n static int clear_stash(int argc, const char **argv, const char *prefix)\n {\n+\tint is_prompt;\n \tstruct option options[] = {\n+\t\tOPT_BOOL('i', \"interactive\", &is_prompt,\n+\t\t\t N_(\"confirm clearing stash\")),\n \t\tOPT_END()\n \t};\n \n@@ -226,7 +229,21 @@ static int clear_stash(int argc, const char **argv, const char *prefix)\n \tif (argc)\n \t\treturn error(_(\"git stash clear with arguments is \"\n \t\t\t \"unimplemented\"));\n+\tif (is_prompt == 1) {\n+\t\tchar code[2];\n+\t\tprintf(\"Are you sure you want to clear your stash? [y/N]: \");\n+\t\tif (fgets(code, 2, stdin) != NULL) {\n+\t\t\tif (code[0] == 'y' || code[0] == 'Y') {\n+\t\t\t\tprintf_ln(_(\"Clearing stash\"));\n+\t\t\t\treturn do_clear_stash();\n+\t\t\t}\n+\t\t\telse {\n+\t\t\t\tprintf_ln(_(\"Aborting clear\"));\n+\t\t\t}\n+\t\t}\n \n+\t\treturn 0;\n+\t}\n \treturn do_clear_stash();\n }\n \n\nbase-commit: e8005e4871f130c4e402ddca2032c111252f070a\n-- \ngitgitgadget\n"},{"id":"455304","messageId":"xmqqtu9q5fpr.fsf@gitster.g","threadId":"57884","inReplyTo":"pull.1232.git.1652649537647.gitgitgadget@gmail.com","subject":"Re: [PATCH] stash: added safety flag for stash clear subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-16T03:17:04Z","receivedAt":"2022-05-16T03:17:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nadav Goldstein via GitGitGadget\" writes:\n\n> From: Nadav Goldstein \n>\n> Introduced a flag (git stash clear -i) when when used,\n> instead of silently clearing the stash, will present a\n> confirmation to the user regarding the action which he's\n> about to perform. Took the inspiration from rm -rf -i flag.\n> This flag also outputs extra logs (abort/success) to let\n> the user know more \"interactively\" what is happening with\n> the stash (hence the flag name).\n\nDocumentation/SubmittingPatches gives many helpful hints on how to\nwrite log messages for the project.\n\nIf this were truly \"interactive\" modelled after \"rm -i\", I would\nimagine that it would ask a question for each stash entry and allow\nthe user to selectively drop some entries while keeping others.\n\nBut that is not how the option behaves, as far as I can see. It\nonly asks a single y/N question, without even knowing how many stash\nentries there are. Your user may say \"Yes\" and the command may then\nfind no stash entries that need to be cleared after all.\n\nIt is not taking any inspiration from \"rm -i\" at all.\n\nI wonder if it is easier to model the \"safety\" after \"git clean\"\ninstead. The \"git clean\" command by default does not do any removal\nand you are required to say \"clean -f\" or \"clean -n\", unless the\n\"clean.requireforce\" configuration variable is set to false. The\nresulting \"git stash clear [--force|--dry-run]\" would be fairly easy\nto understand to new users, because \"git clean [--force|--dry-run]\"\nalready behaves in a similar way.\n\nOne caveat is that \"git clean\" by default requires \"--force\" on the\ncommand line to do any damage, and it is OK because it was like so\nfor a long time. But \"git stash clear\" has been with us for a very\nlong time without such requirement, so if you suddenly start\nrequiring \"--force\" and defaulting to \"--dry-run\", existing users\nmay be annoyed that they need to say\n\n\t$ git config --global stashClear.requireforce no\n\nonce to get back the original behaviour.\n\nI personally think that such a one-time annoyance forced on all the\nexisting users may probably be worth it for added safety, but I am\nnot the only user of Git, so... ;-)\n\nIn any case, my recommendation would be\n\n * add \"--force\" and \"--dry-run\" to \"git stash clear\".\n\n * add stashClear.requireForce that defaults to \"false\".\n\n * document the above. mention the setting in ReleaseNotes.\n\n * monitor places like stackoverflow to see if folks give advice\n \"set stashClear.requireForce to true for safety\" to newbies\n often. It would give us an excuse to proceed to the next step,\n i.e. propose to switch the default of stashClear.requireForce to\n \"true\".\n\n * The true \"interactive\" that lets your users pick and choose what\n to discard can come later as another improved form of additional\n safety.\n\nThe rest are minor comments that have already been outdated by all\nofhte above ;-)\n\n> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\n> index 6e15f475257..a7ab5379779 100644\n> --- a/Documentation/git-stash.txt\n> +++ b/Documentation/git-stash.txt\n> @@ -17,7 +17,7 @@ SYNOPSIS\n> \t [-u|--include-untracked] [-a|--all] [-m|--message ]\n> \t [--pathspec-from-file= [--pathspec-file-nul]]\n> \t [--] [...]]\n> -'git stash' clear\n> +'git stash' clear [-i|--interactive]\n\nAs I already said, calling the behaviour implemented by the patch\n\"interactive\" is misleading; users will expect to be able to pick\nand choose which entry to discard.\n\n> static int clear_stash(int argc, const char **argv, const char *prefix)\n> {\n> +\tint is_prompt;\n> \tstruct option options[] = {\n> +\t\tOPT_BOOL('i', \"interactive\", &is_prompt,\n> +\t\t\t N_(\"confirm clearing stash\")),\n> \t\tOPT_END()\n> \t};\n\n\"IS_prompt\" is a strange phrasing to call the option. \n\nneeds_prompt (we need to prompt the user before proceeding),\nor do_prompt (literally, \"do the prompting\"), would be more\nunderstanding, though.\n"},{"id":"455745","messageId":"pull.1232.v2.git.1653286345.gitgitgadget@gmail.com","threadId":"57884","inReplyTo":"pull.1232.git.1652649537647.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] stash clear: added safety flag for stash clear subcommand","fromName":"Nadav Goldstein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-23T06:12:23Z","receivedAt":"2022-05-23T06:12:57Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"PLAN: Add a flag to git stash clear (-i|--interactive), which prompts the\nuser to confirm he indeed wants to clear the stash, otherwise it will abort.\n\nI found it useful as a frequent stash clear user which means my terminal\nalways have it in recent commands, which could mean accidental erase of\nwork. This flag ensures accidental fires won't clear hard work :)\n\nI also thought it would be better to do it opt in, to not change the way the\ncommand works in prod, only add to it.\n\nChanges since v1:\n\nRemoved temporarily the interactive flag from stash. introduced add-menu lib\nto the project, which is simply the extracted code that responsible for\npresenting the menu in the clean command, and made the clean command use it.\n\nThis change was made to allow stash to use interactive as well, with the\nsame style as git clean.\n\nBefore I continue the development, I would like to know what the community\nthink about this refactor.\n\nThanks!\n\nNadav Goldstein (2):\n add-menu: added add-menu to lib objects\n clean: refector to the interactive part of clean\n\n Makefile | 1 +\n add-menu.c | 353 +++++++++++++++++++++++++++++++++++++++++\n add-menu.h | 55 +++++++\n builtin/clean.c | 413 ++----------------------------------------------\n 4 files changed, 425 insertions(+), 397 deletions(-)\n create mode 100644 add-menu.c\n create mode 100644 add-menu.h\n\n\nbase-commit: e8005e4871f130c4e402ddca2032c111252f070a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1232%2Fnadav96%2Fclear-stash-prompt-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1232/nadav96/clear-stash-prompt-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1232\n\nRange-diff vs v1:\n\n 1: 5396a67b0ff < -: ----------- stash: added safety flag for stash clear subcommand\n -: ----------- > 1: 13bc75a2b05 add-menu: added add-menu to lib objects\n -: ----------- > 2: 7271a285d18 clean: refector to the interactive part of clean\n\n-- \ngitgitgadget\n"},{"id":"455746","messageId":"13bc75a2b0510f55e9a73852838b3b11683f13a2.1653286345.git.gitgitgadget@gmail.com","threadId":"57884","inReplyTo":"pull.1232.v2.git.1653286345.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] add-menu: added add-menu to lib objects","fromName":"Nadav Goldstein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-23T06:12:24Z","receivedAt":"2022-05-23T06:13:16Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"From: Nadav Goldstein \n\nadded to the lib objects the add menu module which is\nsimply extracted functions from clear.c\n(which in the next commit will be removed and instead\nclear will use the outsourced functions).\n\nSigned-off-by: Nadav Goldstein \n---\n Makefile | 1 +\n add-menu.c | 339 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n add-menu.h | 51 ++++++++\n 3 files changed, 391 insertions(+)\n create mode 100644 add-menu.c\n create mode 100644 add-menu.h\n\ndiff --git a/Makefile b/Makefile\nindex f8bccfab5e9..60ed7a5e1e0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -871,6 +871,7 @@ LIB_H = $(FOUND_H_SOURCES)\n \n LIB_OBJS += abspath.o\n LIB_OBJS += add-interactive.o\n+LIB_OBJS += add-menu.o\n LIB_OBJS += add-patch.o\n LIB_OBJS += advice.o\n LIB_OBJS += alias.o\ndiff --git a/add-menu.c b/add-menu.c\nnew file mode 100644\nindex 00000000000..6a1c125d113\n--- /dev/null\n+++ b/add-menu.c\n@@ -0,0 +1,339 @@\n+#include \n+#include \"builtin.h\"\n+#include \"add-menu.h\"\n+#include \"cache.h\"\n+#include \"config.h\"\n+#include \"dir.h\"\n+#include \"parse-options.h\"\n+#include \"string-list.h\"\n+#include \"quote.h\"\n+#include \"column.h\"\n+#include \"color.h\"\n+#include \"pathspec.h\"\n+#include \"help.h\"\n+#include \"prompt.h\"\n+\n+static int clean_use_color = -1;\n+static char clean_colors[][COLOR_MAXLEN] = {\n+\t[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,\n+\t[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,\n+\t[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,\n+\t[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,\n+\t[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,\n+\t[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,\n+};\n+\n+\n+static const char *clean_get_color(enum color_clean ix)\n+{\n+\tif (want_color(clean_use_color))\n+\t\treturn clean_colors[ix];\n+\treturn \"\";\n+}\n+\n+static int find_unique(const char *choice, struct menu_stuff *menu_stuff)\n+{\n+\tstruct menu_item *menu_item;\n+\tstruct string_list_item *string_list_item;\n+\tint i, len, found = 0;\n+\n+\tlen = strlen(choice);\n+\tswitch (menu_stuff->type) {\n+\tdefault:\n+\t\tdie(\"Bad type of menu_stuff when parse choice\");\n+\tcase MENU_STUFF_TYPE_MENU_ITEM:\n+\n+\t\tmenu_item = (struct menu_item *)menu_stuff->stuff;\n+\t\tfor (i = 0; i < menu_stuff->nr; i++, menu_item++) {\n+\t\t\tif (len == 1 && *choice == menu_item->hotkey) {\n+\t\t\t\tfound = i + 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (!strncasecmp(choice, menu_item->title, len)) {\n+\t\t\t\tif (found) {\n+\t\t\t\t\tif (len == 1) {\n+\t\t\t\t\t\t/* continue for hotkey matching */\n+\t\t\t\t\t\tfound = -1;\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\tfound = 0;\n+\t\t\t\t\t\tbreak;\n+\t\t\t\t\t}\n+\t\t\t\t} else {\n+\t\t\t\t\tfound = i + 1;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\t\tbreak;\n+\tcase MENU_STUFF_TYPE_STRING_LIST:\n+\t\tstring_list_item = ((struct string_list *)menu_stuff->stuff)->items;\n+\t\tfor (i = 0; i < menu_stuff->nr; i++, string_list_item++) {\n+\t\t\tif (!strncasecmp(choice, string_list_item->string, len)) {\n+\t\t\t\tif (found) {\n+\t\t\t\t\tfound = 0;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\tfound = i + 1;\n+\t\t\t}\n+\t\t}\n+\t\tbreak;\n+\t}\n+\treturn found;\n+}\n+\n+static int parse_choice(struct menu_stuff *menu_stuff,\n+\t\t\tint is_single,\n+\t\t\tstruct strbuf input,\n+\t\t\tint **chosen)\n+{\n+\tstruct strbuf **choice_list, **ptr;\n+\tint nr = 0;\n+\tint i;\n+\n+\tif (is_single) {\n+\t\tchoice_list = strbuf_split_max(&input, '\\n', 0);\n+\t} else {\n+\t\tchar *p = input.buf;\n+\t\tdo {\n+\t\t\tif (*p == ',')\n+\t\t\t\t*p = ' ';\n+\t\t} while (*p++);\n+\t\tchoice_list = strbuf_split_max(&input, ' ', 0);\n+\t}\n+\n+\tfor (ptr = choice_list; *ptr; ptr++) {\n+\t\tchar *p;\n+\t\tint choose = 1;\n+\t\tint bottom = 0, top = 0;\n+\t\tint is_range, is_number;\n+\n+\t\tstrbuf_trim(*ptr);\n+\t\tif (!(*ptr)->len)\n+\t\t\tcontinue;\n+\n+\t\t/* Input that begins with '-'; unchoose */\n+\t\tif (*(*ptr)->buf == '-') {\n+\t\t\tchoose = 0;\n+\t\t\tstrbuf_remove((*ptr), 0, 1);\n+\t\t}\n+\n+\t\tis_range = 0;\n+\t\tis_number = 1;\n+\t\tfor (p = (*ptr)->buf; *p; p++) {\n+\t\t\tif ('-' == *p) {\n+\t\t\t\tif (!is_range) {\n+\t\t\t\t\tis_range = 1;\n+\t\t\t\t\tis_number = 0;\n+\t\t\t\t} else {\n+\t\t\t\t\tis_number = 0;\n+\t\t\t\t\tis_range = 0;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t} else if (!isdigit(*p)) {\n+\t\t\t\tis_number = 0;\n+\t\t\t\tis_range = 0;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (is_number) {\n+\t\t\tbottom = atoi((*ptr)->buf);\n+\t\t\ttop = bottom;\n+\t\t} else if (is_range) {\n+\t\t\tbottom = atoi((*ptr)->buf);\n+\t\t\t/* a range can be specified like 5-7 or 5- */\n+\t\t\tif (!*(strchr((*ptr)->buf, '-') + 1))\n+\t\t\t\ttop = menu_stuff->nr;\n+\t\t\telse\n+\t\t\t\ttop = atoi(strchr((*ptr)->buf, '-') + 1);\n+\t\t} else if (!strcmp((*ptr)->buf, \"*\")) {\n+\t\t\tbottom = 1;\n+\t\t\ttop = menu_stuff->nr;\n+\t\t} else {\n+\t\t\tbottom = find_unique((*ptr)->buf, menu_stuff);\n+\t\t\ttop = bottom;\n+\t\t}\n+\n+\t\tif (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||\n+\t\t (is_single && bottom != top)) {\n+\t\t\tclean_print_color(CLEAN_COLOR_ERROR);\n+\t\t\tprintf(_(\"Huh (%s)?\\n\"), (*ptr)->buf);\n+\t\t\tclean_print_color(CLEAN_COLOR_RESET);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tfor (i = bottom; i <= top; i++)\n+\t\t\t(*chosen)[i-1] = choose;\n+\t}\n+\n+\tstrbuf_list_free(choice_list);\n+\n+\tfor (i = 0; i < menu_stuff->nr; i++)\n+\t\tnr += (*chosen)[i];\n+\treturn nr;\n+}\n+\n+static void pretty_print_menus(struct string_list *menu_list)\n+{\n+\tunsigned int local_colopts = 0;\n+\tstruct column_options copts;\n+\n+\tlocal_colopts = COL_ENABLED | COL_ROW;\n+\tmemset(&copts, 0, sizeof(copts));\n+\tcopts.indent = \" \";\n+\tcopts.padding = 2;\n+\tprint_columns(menu_list, local_colopts, &copts);\n+}\n+\n+/*\n+ * display menu stuff with number prefix and hotkey highlight\n+ */\n+static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)\n+{\n+\tstruct string_list menu_list = STRING_LIST_INIT_DUP;\n+\tstruct strbuf menu = STRBUF_INIT;\n+\tstruct menu_item *menu_item;\n+\tstruct string_list_item *string_list_item;\n+\tint i;\n+\n+\tswitch (stuff->type) {\n+\tdefault:\n+\t\tdie(\"Bad type of menu_stuff when print menu\");\n+\tcase MENU_STUFF_TYPE_MENU_ITEM:\n+\t\tmenu_item = (struct menu_item *)stuff->stuff;\n+\t\tfor (i = 0; i < stuff->nr; i++, menu_item++) {\n+\t\t\tconst char *p;\n+\t\t\tint highlighted = 0;\n+\n+\t\t\tp = menu_item->title;\n+\t\t\tif ((*chosen)[i] < 0)\n+\t\t\t\t(*chosen)[i] = menu_item->selected ? 1 : 0;\n+\t\t\tstrbuf_addf(&menu, \"%s%2d: \", (*chosen)[i] ? \"*\" : \" \", i+1);\n+\t\t\tfor (; *p; p++) {\n+\t\t\t\tif (!highlighted && *p == menu_item->hotkey) {\n+\t\t\t\t\tstrbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_PROMPT));\n+\t\t\t\t\tstrbuf_addch(&menu, *p);\n+\t\t\t\t\tstrbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_RESET));\n+\t\t\t\t\thighlighted = 1;\n+\t\t\t\t} else {\n+\t\t\t\t\tstrbuf_addch(&menu, *p);\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tstring_list_append(&menu_list, menu.buf);\n+\t\t\tstrbuf_reset(&menu);\n+\t\t}\n+\t\tbreak;\n+\tcase MENU_STUFF_TYPE_STRING_LIST:\n+\t\ti = 0;\n+\t\tfor_each_string_list_item(string_list_item, (struct string_list *)stuff->stuff) {\n+\t\t\tif ((*chosen)[i] < 0)\n+\t\t\t\t(*chosen)[i] = 0;\n+\t\t\tstrbuf_addf(&menu, \"%s%2d: %s\",\n+\t\t\t\t (*chosen)[i] ? \"*\" : \" \", i+1, string_list_item->string);\n+\t\t\tstring_list_append(&menu_list, menu.buf);\n+\t\t\tstrbuf_reset(&menu);\n+\t\t\ti++;\n+\t\t}\n+\t\tbreak;\n+\t}\n+\n+\tpretty_print_menus(&menu_list);\n+\n+\tstrbuf_release(&menu);\n+\tstring_list_clear(&menu_list, 0);\n+}\n+\n+int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int))\n+{\n+\tstruct strbuf choice = STRBUF_INIT;\n+\tint *chosen, *result;\n+\tint nr = 0;\n+\tint eof = 0;\n+\tint i;\n+\n+\tALLOC_ARRAY(chosen, stuff->nr);\n+\t/* set chosen as uninitialized */\n+\tfor (i = 0; i < stuff->nr; i++)\n+\t\tchosen[i] = -1;\n+\n+\tfor (;;) {\n+\t\tif (opts->header) {\n+\t\t\tprintf_ln(\"%s%s%s\",\n+\t\t\t\t clean_get_color(CLEAN_COLOR_HEADER),\n+\t\t\t\t _(opts->header),\n+\t\t\t\t clean_get_color(CLEAN_COLOR_RESET));\n+\t\t}\n+\n+\t\t/* chosen will be initialized by print_highlight_menu_stuff */\n+\t\tprint_highlight_menu_stuff(stuff, &chosen);\n+\n+\t\tif (opts->flags & MENU_OPTS_LIST_ONLY)\n+\t\t\tbreak;\n+\n+\t\tif (opts->prompt) {\n+\t\t\tprintf(\"%s%s%s%s\",\n+\t\t\t clean_get_color(CLEAN_COLOR_PROMPT),\n+\t\t\t _(opts->prompt),\n+\t\t\t opts->flags & MENU_OPTS_SINGLETON ? \"> \" : \">> \",\n+\t\t\t clean_get_color(CLEAN_COLOR_RESET));\n+\t\t}\n+\n+\t\tif (git_read_line_interactively(&choice) == EOF) {\n+\t\t\teof = 1;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\t/* help for prompt */\n+\t\tif (!strcmp(choice.buf, \"?\")) {\n+\t\t\tprompt_help_cmd(opts->flags & MENU_OPTS_SINGLETON);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* for a multiple-choice menu, press ENTER (empty) will return back */\n+\t\tif (!(opts->flags & MENU_OPTS_SINGLETON) && !choice.len)\n+\t\t\tbreak;\n+\n+\t\tnr = parse_choice(stuff,\n+\t\t\t\t opts->flags & MENU_OPTS_SINGLETON,\n+\t\t\t\t choice,\n+\t\t\t\t &chosen);\n+\n+\t\tif (opts->flags & MENU_OPTS_SINGLETON) {\n+\t\t\tif (nr)\n+\t\t\t\tbreak;\n+\t\t} else if (opts->flags & MENU_OPTS_IMMEDIATE) {\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (eof) {\n+\t\tresult = xmalloc(sizeof(int));\n+\t\t*result = EOF;\n+\t} else {\n+\t\tint j = 0;\n+\n+\t\t/*\n+\t\t * recalculate nr, if return back from menu directly with\n+\t\t * default selections.\n+\t\t */\n+\t\tif (!nr) {\n+\t\t\tfor (i = 0; i < stuff->nr; i++)\n+\t\t\t\tnr += chosen[i];\n+\t\t}\n+\n+\t\tCALLOC_ARRAY(result, st_add(nr, 1));\n+\t\tfor (i = 0; i < stuff->nr && j < nr; i++) {\n+\t\t\tif (chosen[i])\n+\t\t\t\tresult[j++] = i;\n+\t\t}\n+\t\tresult[j] = EOF;\n+\t}\n+\n+\tfree(chosen);\n+\tstrbuf_release(&choice);\n+\treturn result;\n+}\n+\n+void clean_print_color(enum color_clean ix)\n+{\n+\tprintf(\"%s\", clean_get_color(ix));\n+}\n\\ No newline at end of file\ndiff --git a/add-menu.h b/add-menu.h\nnew file mode 100644\nindex 00000000000..52e5ccb1800\n--- /dev/null\n+++ b/add-menu.h\n@@ -0,0 +1,51 @@\n+#define MENU_OPTS_SINGLETON\t\t01\n+#define MENU_OPTS_IMMEDIATE\t\t02\n+#define MENU_OPTS_LIST_ONLY\t\t04\n+\n+enum color_clean {\n+\tCLEAN_COLOR_RESET = 0,\n+\tCLEAN_COLOR_PLAIN = 1,\n+\tCLEAN_COLOR_PROMPT = 2,\n+\tCLEAN_COLOR_HEADER = 3,\n+\tCLEAN_COLOR_HELP = 4,\n+\tCLEAN_COLOR_ERROR = 5\n+};\n+\n+struct menu_opts {\n+\tconst char *header;\n+\tconst char *prompt;\n+\tint flags;\n+};\n+\n+struct menu_item {\n+\tchar hotkey;\n+\tconst char *title;\n+\tint selected;\n+\tint (*fn)(void);\n+};\n+\n+enum menu_stuff_type {\n+\tMENU_STUFF_TYPE_STRING_LIST = 1,\n+\tMENU_STUFF_TYPE_MENU_ITEM\n+};\n+\n+struct menu_stuff {\n+\tenum menu_stuff_type type;\n+\tint nr;\n+\tvoid *stuff;\n+};\n+\n+void clean_print_color(enum color_clean ix);\n+\n+/*\n+ * Implement a git-add-interactive compatible UI, which is borrowed\n+ * from git-add--interactive.perl.\n+ *\n+ * Return value:\n+ *\n+ * - Return an array of integers\n+ * - , and it is up to you to free the allocated memory.\n+ * - The array ends with EOF.\n+ * - If user pressed CTRL-D (i.e. EOF), no selection returned.\n+ */\n+int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int));\n\\ No newline at end of file\n-- \ngitgitgadget\n\n"},{"id":"455747","messageId":"7271a285d18604bd77251d586201d31eba1287da.1653286345.git.gitgitgadget@gmail.com","threadId":"57884","inReplyTo":"pull.1232.v2.git.1653286345.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] clean: refector to the interactive part of clean","fromName":"Nadav Goldstein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-23T06:12:25Z","receivedAt":"2022-05-23T06:13:23Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"From: Nadav Goldstein \n\nmoved the code that responsible for presenting the menu options\n(interactive flag) to be handles by the add-menu lib that was added\nin previous commit.\n\nSigned-off-by: Nadav Goldstein \n---\n add-menu.c | 78 +++++----\n add-menu.h | 8 +-\n builtin/clean.c | 413 ++----------------------------------------------\n 3 files changed, 68 insertions(+), 431 deletions(-)\n\ndiff --git a/add-menu.c b/add-menu.c\nindex 6a1c125d113..becf4956917 100644\n--- a/add-menu.c\n+++ b/add-menu.c\n@@ -13,24 +13,18 @@\n #include \"help.h\"\n #include \"prompt.h\"\n \n-static int clean_use_color = -1;\n-static char clean_colors[][COLOR_MAXLEN] = {\n-\t[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,\n-\t[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,\n-\t[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,\n-\t[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,\n-\t[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,\n-\t[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,\n-};\n-\n-\n-static const char *clean_get_color(enum color_clean ix)\n+static const char *clean_get_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color)\n {\n-\tif (want_color(clean_use_color))\n-\t\treturn clean_colors[ix];\n+\tif (want_color(*clean_use_color))\n+\t\treturn (*clean_colors)[ix];\n \treturn \"\";\n }\n \n+void clean_print_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color)\n+{\n+\tprintf(\"%s\", clean_get_color(ix, clean_colors, clean_use_color));\n+}\n+\n static int find_unique(const char *choice, struct menu_stuff *menu_stuff)\n {\n \tstruct menu_item *menu_item;\n@@ -80,10 +74,33 @@ static int find_unique(const char *choice, struct menu_stuff *menu_stuff)\n \treturn found;\n }\n \n+/*\n+ * Parse user input, and return choice(s) for menu (menu_stuff).\n+ *\n+ * Input\n+ * (for single choice)\n+ * 1 - select a numbered item\n+ * foo - select item based on menu title\n+ * - (empty) select nothing\n+ *\n+ * (for multiple choice)\n+ * 1 - select a single item\n+ * 3-5 - select a range of items\n+ * 2-3,6-9 - select multiple ranges\n+ * foo - select item based on menu title\n+ * -... - unselect specified items\n+ * * - choose all items\n+ * - (empty) finish selecting\n+ *\n+ * The parse result will be saved in array **chosen, and\n+ * return number of total selections.\n+ */\n static int parse_choice(struct menu_stuff *menu_stuff,\n \t\t\tint is_single,\n \t\t\tstruct strbuf input,\n-\t\t\tint **chosen)\n+\t\t\tint **chosen,\n+\t\t\tclean_color_settings *clean_colors,\n+\t\t\tint *clean_use_color)\n {\n \tstruct strbuf **choice_list, **ptr;\n \tint nr = 0;\n@@ -155,9 +172,9 @@ static int parse_choice(struct menu_stuff *menu_stuff,\n \n \t\tif (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||\n \t\t (is_single && bottom != top)) {\n-\t\t\tclean_print_color(CLEAN_COLOR_ERROR);\n+\t\t\tclean_print_color(CLEAN_COLOR_ERROR, clean_colors, clean_use_color);\n \t\t\tprintf(_(\"Huh (%s)?\\n\"), (*ptr)->buf);\n-\t\t\tclean_print_color(CLEAN_COLOR_RESET);\n+\t\t\tclean_print_color(CLEAN_COLOR_RESET, clean_colors, clean_use_color);\n \t\t\tcontinue;\n \t\t}\n \n@@ -187,7 +204,7 @@ static void pretty_print_menus(struct string_list *menu_list)\n /*\n * display menu stuff with number prefix and hotkey highlight\n */\n-static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)\n+static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen, clean_color_settings *clean_colors, int *clean_use_color)\n {\n \tstruct string_list menu_list = STRING_LIST_INIT_DUP;\n \tstruct strbuf menu = STRBUF_INIT;\n@@ -210,9 +227,9 @@ static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)\n \t\t\tstrbuf_addf(&menu, \"%s%2d: \", (*chosen)[i] ? \"*\" : \" \", i+1);\n \t\t\tfor (; *p; p++) {\n \t\t\t\tif (!highlighted && *p == menu_item->hotkey) {\n-\t\t\t\t\tstrbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_PROMPT));\n+\t\t\t\t\tstrbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_PROMPT, clean_colors, clean_use_color));\n \t\t\t\t\tstrbuf_addch(&menu, *p);\n-\t\t\t\t\tstrbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_RESET));\n+\t\t\t\t\tstrbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_RESET, clean_colors, clean_use_color));\n \t\t\t\t\thighlighted = 1;\n \t\t\t\t} else {\n \t\t\t\t\tstrbuf_addch(&menu, *p);\n@@ -242,7 +259,7 @@ static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)\n \tstring_list_clear(&menu_list, 0);\n }\n \n-int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int))\n+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))\n {\n \tstruct strbuf choice = STRBUF_INIT;\n \tint *chosen, *result;\n@@ -258,23 +275,23 @@ int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*pr\n \tfor (;;) {\n \t\tif (opts->header) {\n \t\t\tprintf_ln(\"%s%s%s\",\n-\t\t\t\t clean_get_color(CLEAN_COLOR_HEADER),\n+\t\t\t\t clean_get_color(CLEAN_COLOR_HEADER, clean_colors, clean_use_color),\n \t\t\t\t _(opts->header),\n-\t\t\t\t clean_get_color(CLEAN_COLOR_RESET));\n+\t\t\t\t clean_get_color(CLEAN_COLOR_RESET, clean_colors, clean_use_color));\n \t\t}\n \n \t\t/* chosen will be initialized by print_highlight_menu_stuff */\n-\t\tprint_highlight_menu_stuff(stuff, &chosen);\n+\t\tprint_highlight_menu_stuff(stuff, &chosen, clean_colors, clean_use_color);\n \n \t\tif (opts->flags & MENU_OPTS_LIST_ONLY)\n \t\t\tbreak;\n \n \t\tif (opts->prompt) {\n \t\t\tprintf(\"%s%s%s%s\",\n-\t\t\t clean_get_color(CLEAN_COLOR_PROMPT),\n+\t\t\t clean_get_color(CLEAN_COLOR_PROMPT, clean_colors, clean_use_color),\n \t\t\t _(opts->prompt),\n \t\t\t opts->flags & MENU_OPTS_SINGLETON ? \"> \" : \">> \",\n-\t\t\t clean_get_color(CLEAN_COLOR_RESET));\n+\t\t\t clean_get_color(CLEAN_COLOR_RESET, clean_colors, clean_use_color));\n \t\t}\n \n \t\tif (git_read_line_interactively(&choice) == EOF) {\n@@ -295,7 +312,9 @@ int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*pr\n \t\tnr = parse_choice(stuff,\n \t\t\t\t opts->flags & MENU_OPTS_SINGLETON,\n \t\t\t\t choice,\n-\t\t\t\t &chosen);\n+\t\t\t\t &chosen,\n+\t\t\t\t clean_colors,\n+\t\t\t\t clean_use_color);\n \n \t\tif (opts->flags & MENU_OPTS_SINGLETON) {\n \t\t\tif (nr)\n@@ -331,9 +350,4 @@ int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*pr\n \tfree(chosen);\n \tstrbuf_release(&choice);\n \treturn result;\n-}\n-\n-void clean_print_color(enum color_clean ix)\n-{\n-\tprintf(\"%s\", clean_get_color(ix));\n }\n\\ No newline at end of file\ndiff --git a/add-menu.h b/add-menu.h\nindex 52e5ccb1800..64f1cbdab9f 100644\n--- a/add-menu.h\n+++ b/add-menu.h\n@@ -1,3 +1,7 @@\n+#include \"color.h\"\n+\n+typedef char clean_color_settings[][COLOR_MAXLEN];\n+\n #define MENU_OPTS_SINGLETON\t\t01\n #define MENU_OPTS_IMMEDIATE\t\t02\n #define MENU_OPTS_LIST_ONLY\t\t04\n@@ -35,7 +39,7 @@ struct menu_stuff {\n \tvoid *stuff;\n };\n \n-void clean_print_color(enum color_clean ix);\n+void clean_print_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color);\n \n /*\n * Implement a git-add-interactive compatible UI, which is borrowed\n@@ -48,4 +52,4 @@ void clean_print_color(enum color_clean ix);\n * - The array ends with EOF.\n * - If user pressed CTRL-D (i.e. EOF), no selection returned.\n */\n-int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int));\n\\ No newline at end of file\n+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));\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 5466636e666..ef220869851 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -19,6 +19,7 @@\n #include \"pathspec.h\"\n #include \"help.h\"\n #include \"prompt.h\"\n+#include \"add-menu.h\"\n \n static int force = -1; /* unset */\n static int interactive;\n@@ -39,14 +40,6 @@ static const char *msg_warn_lstat_failed = N_(\"could not lstat %s\\n\");\n static const char *msg_skip_cwd = N_(\"Refusing to remove current working directory\\n\");\n static const char *msg_would_skip_cwd = N_(\"Would refuse to remove current working directory\\n\");\n \n-enum color_clean {\n-\tCLEAN_COLOR_RESET = 0,\n-\tCLEAN_COLOR_PLAIN = 1,\n-\tCLEAN_COLOR_PROMPT = 2,\n-\tCLEAN_COLOR_HEADER = 3,\n-\tCLEAN_COLOR_HELP = 4,\n-\tCLEAN_COLOR_ERROR = 5\n-};\n \n static const char *color_interactive_slots[] = {\n \t[CLEAN_COLOR_ERROR] = \"error\",\n@@ -58,7 +51,7 @@ static const char *color_interactive_slots[] = {\n };\n \n static int clean_use_color = -1;\n-static char clean_colors[][COLOR_MAXLEN] = {\n+static clean_color_settings clean_colors = {\n \t[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,\n \t[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,\n \t[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,\n@@ -67,36 +60,8 @@ static char clean_colors[][COLOR_MAXLEN] = {\n \t[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,\n };\n \n-#define MENU_OPTS_SINGLETON\t\t01\n-#define MENU_OPTS_IMMEDIATE\t\t02\n-#define MENU_OPTS_LIST_ONLY\t\t04\n-\n-struct menu_opts {\n-\tconst char *header;\n-\tconst char *prompt;\n-\tint flags;\n-};\n-\n #define MENU_RETURN_NO_LOOP\t\t10\n \n-struct menu_item {\n-\tchar hotkey;\n-\tconst char *title;\n-\tint selected;\n-\tint (*fn)(void);\n-};\n-\n-enum menu_stuff_type {\n-\tMENU_STUFF_TYPE_STRING_LIST = 1,\n-\tMENU_STUFF_TYPE_MENU_ITEM\n-};\n-\n-struct menu_stuff {\n-\tenum menu_stuff_type type;\n-\tint nr;\n-\tvoid *stuff;\n-};\n-\n define_list_config_array(color_interactive_slots);\n \n static int git_clean_config(const char *var, const char *value, void *cb)\n@@ -130,18 +95,6 @@ static int git_clean_config(const char *var, const char *value, void *cb)\n \treturn git_color_default_config(var, value, cb);\n }\n \n-static const char *clean_get_color(enum color_clean ix)\n-{\n-\tif (want_color(clean_use_color))\n-\t\treturn clean_colors[ix];\n-\treturn \"\";\n-}\n-\n-static void clean_print_color(enum color_clean ix)\n-{\n-\tprintf(\"%s\", clean_get_color(ix));\n-}\n-\n static int exclude_cb(const struct option *opt, const char *arg, int unset)\n {\n \tstruct string_list *exclude_list = opt->value;\n@@ -307,21 +260,9 @@ static void pretty_print_dels(void)\n \tstring_list_clear(&list, 0);\n }\n \n-static void pretty_print_menus(struct string_list *menu_list)\n-{\n-\tunsigned int local_colopts = 0;\n-\tstruct column_options copts;\n-\n-\tlocal_colopts = COL_ENABLED | COL_ROW;\n-\tmemset(&copts, 0, sizeof(copts));\n-\tcopts.indent = \" \";\n-\tcopts.padding = 2;\n-\tprint_columns(menu_list, local_colopts, &copts);\n-}\n-\n static void prompt_help_cmd(int singleton)\n {\n-\tclean_print_color(CLEAN_COLOR_HELP);\n+\tclean_print_color(CLEAN_COLOR_HELP, &clean_colors, &clean_use_color);\n \tprintf(singleton ?\n \t\t _(\"Prompt help:\\n\"\n \t\t \"1 - select a numbered item\\n\"\n@@ -335,329 +276,7 @@ static void prompt_help_cmd(int singleton)\n \t\t \"-... - unselect specified items\\n\"\n \t\t \"* - choose all items\\n\"\n \t\t \" - (empty) finish selecting\\n\"));\n-\tclean_print_color(CLEAN_COLOR_RESET);\n-}\n-\n-/*\n- * display menu stuff with number prefix and hotkey highlight\n- */\n-static void print_highlight_menu_stuff(struct menu_stuff *stuff, int **chosen)\n-{\n-\tstruct string_list menu_list = STRING_LIST_INIT_DUP;\n-\tstruct strbuf menu = STRBUF_INIT;\n-\tstruct menu_item *menu_item;\n-\tstruct string_list_item *string_list_item;\n-\tint i;\n-\n-\tswitch (stuff->type) {\n-\tdefault:\n-\t\tdie(\"Bad type of menu_stuff when print menu\");\n-\tcase MENU_STUFF_TYPE_MENU_ITEM:\n-\t\tmenu_item = (struct menu_item *)stuff->stuff;\n-\t\tfor (i = 0; i < stuff->nr; i++, menu_item++) {\n-\t\t\tconst char *p;\n-\t\t\tint highlighted = 0;\n-\n-\t\t\tp = menu_item->title;\n-\t\t\tif ((*chosen)[i] < 0)\n-\t\t\t\t(*chosen)[i] = menu_item->selected ? 1 : 0;\n-\t\t\tstrbuf_addf(&menu, \"%s%2d: \", (*chosen)[i] ? \"*\" : \" \", i+1);\n-\t\t\tfor (; *p; p++) {\n-\t\t\t\tif (!highlighted && *p == menu_item->hotkey) {\n-\t\t\t\t\tstrbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_PROMPT));\n-\t\t\t\t\tstrbuf_addch(&menu, *p);\n-\t\t\t\t\tstrbuf_addstr(&menu, clean_get_color(CLEAN_COLOR_RESET));\n-\t\t\t\t\thighlighted = 1;\n-\t\t\t\t} else {\n-\t\t\t\t\tstrbuf_addch(&menu, *p);\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\tstring_list_append(&menu_list, menu.buf);\n-\t\t\tstrbuf_reset(&menu);\n-\t\t}\n-\t\tbreak;\n-\tcase MENU_STUFF_TYPE_STRING_LIST:\n-\t\ti = 0;\n-\t\tfor_each_string_list_item(string_list_item, (struct string_list *)stuff->stuff) {\n-\t\t\tif ((*chosen)[i] < 0)\n-\t\t\t\t(*chosen)[i] = 0;\n-\t\t\tstrbuf_addf(&menu, \"%s%2d: %s\",\n-\t\t\t\t (*chosen)[i] ? \"*\" : \" \", i+1, string_list_item->string);\n-\t\t\tstring_list_append(&menu_list, menu.buf);\n-\t\t\tstrbuf_reset(&menu);\n-\t\t\ti++;\n-\t\t}\n-\t\tbreak;\n-\t}\n-\n-\tpretty_print_menus(&menu_list);\n-\n-\tstrbuf_release(&menu);\n-\tstring_list_clear(&menu_list, 0);\n-}\n-\n-static int find_unique(const char *choice, struct menu_stuff *menu_stuff)\n-{\n-\tstruct menu_item *menu_item;\n-\tstruct string_list_item *string_list_item;\n-\tint i, len, found = 0;\n-\n-\tlen = strlen(choice);\n-\tswitch (menu_stuff->type) {\n-\tdefault:\n-\t\tdie(\"Bad type of menu_stuff when parse choice\");\n-\tcase MENU_STUFF_TYPE_MENU_ITEM:\n-\n-\t\tmenu_item = (struct menu_item *)menu_stuff->stuff;\n-\t\tfor (i = 0; i < menu_stuff->nr; i++, menu_item++) {\n-\t\t\tif (len == 1 && *choice == menu_item->hotkey) {\n-\t\t\t\tfound = i + 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (!strncasecmp(choice, menu_item->title, len)) {\n-\t\t\t\tif (found) {\n-\t\t\t\t\tif (len == 1) {\n-\t\t\t\t\t\t/* continue for hotkey matching */\n-\t\t\t\t\t\tfound = -1;\n-\t\t\t\t\t} else {\n-\t\t\t\t\t\tfound = 0;\n-\t\t\t\t\t\tbreak;\n-\t\t\t\t\t}\n-\t\t\t\t} else {\n-\t\t\t\t\tfound = i + 1;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n-\t\tbreak;\n-\tcase MENU_STUFF_TYPE_STRING_LIST:\n-\t\tstring_list_item = ((struct string_list *)menu_stuff->stuff)->items;\n-\t\tfor (i = 0; i < menu_stuff->nr; i++, string_list_item++) {\n-\t\t\tif (!strncasecmp(choice, string_list_item->string, len)) {\n-\t\t\t\tif (found) {\n-\t\t\t\t\tfound = 0;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t\tfound = i + 1;\n-\t\t\t}\n-\t\t}\n-\t\tbreak;\n-\t}\n-\treturn found;\n-}\n-\n-/*\n- * Parse user input, and return choice(s) for menu (menu_stuff).\n- *\n- * Input\n- * (for single choice)\n- * 1 - select a numbered item\n- * foo - select item based on menu title\n- * - (empty) select nothing\n- *\n- * (for multiple choice)\n- * 1 - select a single item\n- * 3-5 - select a range of items\n- * 2-3,6-9 - select multiple ranges\n- * foo - select item based on menu title\n- * -... - unselect specified items\n- * * - choose all items\n- * - (empty) finish selecting\n- *\n- * The parse result will be saved in array **chosen, and\n- * return number of total selections.\n- */\n-static int parse_choice(struct menu_stuff *menu_stuff,\n-\t\t\tint is_single,\n-\t\t\tstruct strbuf input,\n-\t\t\tint **chosen)\n-{\n-\tstruct strbuf **choice_list, **ptr;\n-\tint nr = 0;\n-\tint i;\n-\n-\tif (is_single) {\n-\t\tchoice_list = strbuf_split_max(&input, '\\n', 0);\n-\t} else {\n-\t\tchar *p = input.buf;\n-\t\tdo {\n-\t\t\tif (*p == ',')\n-\t\t\t\t*p = ' ';\n-\t\t} while (*p++);\n-\t\tchoice_list = strbuf_split_max(&input, ' ', 0);\n-\t}\n-\n-\tfor (ptr = choice_list; *ptr; ptr++) {\n-\t\tchar *p;\n-\t\tint choose = 1;\n-\t\tint bottom = 0, top = 0;\n-\t\tint is_range, is_number;\n-\n-\t\tstrbuf_trim(*ptr);\n-\t\tif (!(*ptr)->len)\n-\t\t\tcontinue;\n-\n-\t\t/* Input that begins with '-'; unchoose */\n-\t\tif (*(*ptr)->buf == '-') {\n-\t\t\tchoose = 0;\n-\t\t\tstrbuf_remove((*ptr), 0, 1);\n-\t\t}\n-\n-\t\tis_range = 0;\n-\t\tis_number = 1;\n-\t\tfor (p = (*ptr)->buf; *p; p++) {\n-\t\t\tif ('-' == *p) {\n-\t\t\t\tif (!is_range) {\n-\t\t\t\t\tis_range = 1;\n-\t\t\t\t\tis_number = 0;\n-\t\t\t\t} else {\n-\t\t\t\t\tis_number = 0;\n-\t\t\t\t\tis_range = 0;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t} else if (!isdigit(*p)) {\n-\t\t\t\tis_number = 0;\n-\t\t\t\tis_range = 0;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t}\n-\n-\t\tif (is_number) {\n-\t\t\tbottom = atoi((*ptr)->buf);\n-\t\t\ttop = bottom;\n-\t\t} else if (is_range) {\n-\t\t\tbottom = atoi((*ptr)->buf);\n-\t\t\t/* a range can be specified like 5-7 or 5- */\n-\t\t\tif (!*(strchr((*ptr)->buf, '-') + 1))\n-\t\t\t\ttop = menu_stuff->nr;\n-\t\t\telse\n-\t\t\t\ttop = atoi(strchr((*ptr)->buf, '-') + 1);\n-\t\t} else if (!strcmp((*ptr)->buf, \"*\")) {\n-\t\t\tbottom = 1;\n-\t\t\ttop = menu_stuff->nr;\n-\t\t} else {\n-\t\t\tbottom = find_unique((*ptr)->buf, menu_stuff);\n-\t\t\ttop = bottom;\n-\t\t}\n-\n-\t\tif (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||\n-\t\t (is_single && bottom != top)) {\n-\t\t\tclean_print_color(CLEAN_COLOR_ERROR);\n-\t\t\tprintf(_(\"Huh (%s)?\\n\"), (*ptr)->buf);\n-\t\t\tclean_print_color(CLEAN_COLOR_RESET);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tfor (i = bottom; i <= top; i++)\n-\t\t\t(*chosen)[i-1] = choose;\n-\t}\n-\n-\tstrbuf_list_free(choice_list);\n-\n-\tfor (i = 0; i < menu_stuff->nr; i++)\n-\t\tnr += (*chosen)[i];\n-\treturn nr;\n-}\n-\n-/*\n- * Implement a git-add-interactive compatible UI, which is borrowed\n- * from git-add--interactive.perl.\n- *\n- * Return value:\n- *\n- * - Return an array of integers\n- * - , and it is up to you to free the allocated memory.\n- * - The array ends with EOF.\n- * - If user pressed CTRL-D (i.e. EOF), no selection returned.\n- */\n-static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)\n-{\n-\tstruct strbuf choice = STRBUF_INIT;\n-\tint *chosen, *result;\n-\tint nr = 0;\n-\tint eof = 0;\n-\tint i;\n-\n-\tALLOC_ARRAY(chosen, stuff->nr);\n-\t/* set chosen as uninitialized */\n-\tfor (i = 0; i < stuff->nr; i++)\n-\t\tchosen[i] = -1;\n-\n-\tfor (;;) {\n-\t\tif (opts->header) {\n-\t\t\tprintf_ln(\"%s%s%s\",\n-\t\t\t\t clean_get_color(CLEAN_COLOR_HEADER),\n-\t\t\t\t _(opts->header),\n-\t\t\t\t clean_get_color(CLEAN_COLOR_RESET));\n-\t\t}\n-\n-\t\t/* chosen will be initialized by print_highlight_menu_stuff */\n-\t\tprint_highlight_menu_stuff(stuff, &chosen);\n-\n-\t\tif (opts->flags & MENU_OPTS_LIST_ONLY)\n-\t\t\tbreak;\n-\n-\t\tif (opts->prompt) {\n-\t\t\tprintf(\"%s%s%s%s\",\n-\t\t\t clean_get_color(CLEAN_COLOR_PROMPT),\n-\t\t\t _(opts->prompt),\n-\t\t\t opts->flags & MENU_OPTS_SINGLETON ? \"> \" : \">> \",\n-\t\t\t clean_get_color(CLEAN_COLOR_RESET));\n-\t\t}\n-\n-\t\tif (git_read_line_interactively(&choice) == EOF) {\n-\t\t\teof = 1;\n-\t\t\tbreak;\n-\t\t}\n-\n-\t\t/* help for prompt */\n-\t\tif (!strcmp(choice.buf, \"?\")) {\n-\t\t\tprompt_help_cmd(opts->flags & MENU_OPTS_SINGLETON);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* for a multiple-choice menu, press ENTER (empty) will return back */\n-\t\tif (!(opts->flags & MENU_OPTS_SINGLETON) && !choice.len)\n-\t\t\tbreak;\n-\n-\t\tnr = parse_choice(stuff,\n-\t\t\t\t opts->flags & MENU_OPTS_SINGLETON,\n-\t\t\t\t choice,\n-\t\t\t\t &chosen);\n-\n-\t\tif (opts->flags & MENU_OPTS_SINGLETON) {\n-\t\t\tif (nr)\n-\t\t\t\tbreak;\n-\t\t} else if (opts->flags & MENU_OPTS_IMMEDIATE) {\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\tif (eof) {\n-\t\tresult = xmalloc(sizeof(int));\n-\t\t*result = EOF;\n-\t} else {\n-\t\tint j = 0;\n-\n-\t\t/*\n-\t\t * recalculate nr, if return back from menu directly with\n-\t\t * default selections.\n-\t\t */\n-\t\tif (!nr) {\n-\t\t\tfor (i = 0; i < stuff->nr; i++)\n-\t\t\t\tnr += chosen[i];\n-\t\t}\n-\n-\t\tCALLOC_ARRAY(result, st_add(nr, 1));\n-\t\tfor (i = 0; i < stuff->nr && j < nr; i++) {\n-\t\t\tif (chosen[i])\n-\t\t\t\tresult[j++] = i;\n-\t\t}\n-\t\tresult[j] = EOF;\n-\t}\n-\n-\tfree(chosen);\n-\tstrbuf_release(&choice);\n-\treturn result;\n+\tclean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);\n }\n \n static int clean_cmd(void)\n@@ -681,9 +300,9 @@ static int filter_by_patterns_cmd(void)\n \t\tif (changed)\n \t\t\tpretty_print_dels();\n \n-\t\tclean_print_color(CLEAN_COLOR_PROMPT);\n+\t\tclean_print_color(CLEAN_COLOR_PROMPT, &clean_colors, &clean_use_color);\n \t\tprintf(_(\"Input ignore patterns>> \"));\n-\t\tclean_print_color(CLEAN_COLOR_RESET);\n+\t\tclean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);\n \t\tif (git_read_line_interactively(&confirm) == EOF)\n \t\t\tputchar('\\n');\n \n@@ -715,9 +334,9 @@ static int filter_by_patterns_cmd(void)\n \t\tif (changed) {\n \t\t\tstring_list_remove_empty_items(&del_list, 0);\n \t\t} else {\n-\t\t\tclean_print_color(CLEAN_COLOR_ERROR);\n+\t\t\tclean_print_color(CLEAN_COLOR_ERROR, &clean_colors, &clean_use_color);\n \t\t\tprintf_ln(_(\"WARNING: Cannot find items matched by: %s\"), confirm.buf);\n-\t\t\tclean_print_color(CLEAN_COLOR_RESET);\n+\t\t\tclean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);\n \t\t}\n \n \t\tstrbuf_list_free(ignore_list);\n@@ -744,7 +363,7 @@ static int select_by_numbers_cmd(void)\n \tmenu_stuff.stuff = &del_list;\n \tmenu_stuff.nr = del_list.nr;\n \n-\tchosen = list_and_choose(&menu_opts, &menu_stuff);\n+\tchosen = list_and_choose(&menu_opts, &menu_stuff, &clean_colors, &clean_use_color, prompt_help_cmd);\n \titems = del_list.items;\n \tfor (i = 0, j = 0; i < del_list.nr; i++) {\n \t\tif (i < chosen[j]) {\n@@ -807,7 +426,7 @@ static int quit_cmd(void)\n \n static int help_cmd(void)\n {\n-\tclean_print_color(CLEAN_COLOR_HELP);\n+\tclean_print_color(CLEAN_COLOR_HELP, &clean_colors, &clean_use_color);\n \tprintf_ln(_(\n \t\t \"clean - start cleaning\\n\"\n \t\t \"filter by pattern - exclude items from deletion\\n\"\n@@ -817,7 +436,7 @@ static int help_cmd(void)\n \t\t \"help - this screen\\n\"\n \t\t \"? - help for prompt selection\"\n \t\t ));\n-\tclean_print_color(CLEAN_COLOR_RESET);\n+\tclean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);\n \treturn 0;\n }\n \n@@ -844,15 +463,15 @@ static void interactive_main_loop(void)\n \t\tmenu_stuff.stuff = menus;\n \t\tmenu_stuff.nr = sizeof(menus) / sizeof(struct menu_item);\n \n-\t\tclean_print_color(CLEAN_COLOR_HEADER);\n+\t\tclean_print_color(CLEAN_COLOR_HEADER, &clean_colors, &clean_use_color);\n \t\tprintf_ln(Q_(\"Would remove the following item:\",\n \t\t\t \"Would remove the following items:\",\n \t\t\t del_list.nr));\n-\t\tclean_print_color(CLEAN_COLOR_RESET);\n+\t\tclean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);\n \n \t\tpretty_print_dels();\n \n-\t\tchosen = list_and_choose(&menu_opts, &menu_stuff);\n+\t\tchosen = list_and_choose(&menu_opts, &menu_stuff, &clean_colors, &clean_use_color, prompt_help_cmd);\n \n \t\tif (*chosen != EOF) {\n \t\t\tint ret;\n@@ -860,9 +479,9 @@ static void interactive_main_loop(void)\n \t\t\tif (ret != MENU_RETURN_NO_LOOP) {\n \t\t\t\tFREE_AND_NULL(chosen);\n \t\t\t\tif (!del_list.nr) {\n-\t\t\t\t\tclean_print_color(CLEAN_COLOR_ERROR);\n+\t\t\t\t\tclean_print_color(CLEAN_COLOR_ERROR, &clean_colors, &clean_use_color);\n \t\t\t\t\tprintf_ln(_(\"No more files to clean, exiting.\"));\n-\t\t\t\t\tclean_print_color(CLEAN_COLOR_RESET);\n+\t\t\t\t\tclean_print_color(CLEAN_COLOR_RESET, &clean_colors, &clean_use_color);\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\tcontinue;\n-- \ngitgitgadget\n"},{"id":"455811","messageId":"c795733a-fcf2-9ca3-3f31-b347b191f725@github.com","threadId":"57884","inReplyTo":"pull.1232.v2.git.1653286345.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] stash clear: added safety flag for stash clear subcommand","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-23T19:33:21Z","receivedAt":"2022-05-23T19:43:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/23/22 2:12 AM, Nadav Goldstein via GitGitGadget wrote:\n> PLAN: Add a flag to git stash clear (-i|--interactive), which prompts the\n> user to confirm he indeed wants to clear the stash, otherwise it will abort.\n> \n> I found it useful as a frequent stash clear user which means my terminal\n> always have it in recent commands, which could mean accidental erase of\n> work. This flag ensures accidental fires won't clear hard work :)\n> \n> I also thought it would be better to do it opt in, to not change the way the\n> command works in prod, only add to it.\n> \n> Changes since v1:\n\nIt looks like you completely changed your approach here. It's perfectly\nfine to use the same PR and keep the Git mailing list thread intact.\n\nHowever, it would be good to keep the cover letter and title up-to-date\nwith what you are submitting. (Now that you have multiple patches, GGG\nwill split the PR Title and Description into your cover letter message.)\n\nWhat you actually implement is a refactor to make 'git clean's\ninteractive menu be implemented in libgit.a.\n\n> Removed temporarily the interactive flag from stash. introduced add-menu lib\n> to the project, which is simply the extracted code that responsible for\n> presenting the menu in the clean command, and made the clean command use it.\n> \n> This change was made to allow stash to use interactive as well, with the\n> same style as git clean.\n\nIt is interesting that you'd model 'git stash -i' after 'git clean -i'\ninstead of something like 'git add -p'. It would be interesting to see\nyour intended 'git stash' menu based on this API before we commit to\nthe extraction.\n\nThanks,\n-Stolee\n"},{"id":"455813","messageId":"a28047a1-4db2-1638-8b7e-62fcf0b57983@github.com","threadId":"57884","inReplyTo":"7271a285d18604bd77251d586201d31eba1287da.1653286345.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] clean: refector to the interactive part of clean","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-23T19:45:39Z","receivedAt":"2022-05-23T19:45:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/23/22 2:12 AM, Nadav Goldstein via GitGitGadget wrote:\n> From: Nadav Goldstein \n> \n> moved the code that responsible for presenting the menu options\n> (interactive flag) to be handles by the add-menu lib that was added\n> in previous commit.\n\nPlease see Documentation/SubmittingPatches, specifically the\n\"present tense\" section [1]. \n\n[1] https://github.com/git/git/blob/f9b95943b68b6b8ca5a6072f50a08411c6449b55/Documentation/SubmittingPatches#L174-L179\n\n> -static int clean_use_color = -1;\n> -static char clean_colors[][COLOR_MAXLEN] = {\n> -\t[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,\n> -\t[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,\n> -\t[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,\n> -\t[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,\n> -\t[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,\n> -\t[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,\n> -};\n> -\n> -\n> -static const char *clean_get_color(enum color_clean ix)\n> +static const char *clean_get_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color)\n\nI'm surprised to see changes to add-menu.c in this patch. I\nwould expect that add-menu.c was written in its correct form\nin the first patch and then this patch could focus entirely\non deleting matching code from builtin/clean.c and calling\nthe API exposed in add-menu.h.\n\nPerhaps this was just an interactive rebase issue? Fixed up\nthe wrong commit? That happens to me a lot.\n\nThere are also a lot of places that refer to \"clean\" when\nyou want this API to be abstracted away from 'git clean'.\n\n> \\ No newline at end of file\n> diff --git a/add-menu.h b/add-menu.h\n> index 52e5ccb1800..64f1cbdab9f 100644\n> --- a/add-menu.h\n> +++ b/add-menu.h\n> @@ -1,3 +1,7 @@\n> +#include \"color.h\"\n> +\n> +typedef char clean_color_settings[][COLOR_MAXLEN];\n\nThis typedef also shouldn't reference 'git clean'.\n\n> +\n> #define MENU_OPTS_SINGLETON\t\t01\n> #define MENU_OPTS_IMMEDIATE\t\t02\n> #define MENU_OPTS_LIST_ONLY\t\t04\n> @@ -35,7 +39,7 @@ struct menu_stuff {\n> \tvoid *stuff;\n> };\n> \n> -void clean_print_color(enum color_clean ix);\n> +void clean_print_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color);\n> \n> /*\n> * Implement a git-add-interactive compatible UI, which is borrowed\n> @@ -48,4 +52,4 @@ void clean_print_color(enum color_clean ix);\n> * - The array ends with EOF.\n> * - If user pressed CTRL-D (i.e. EOF), no selection returned.\n> */\n> -int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int));\n> \\ No newline at end of file\n> +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));\n\nThere are a lot of instances where your lines are much too\nwide. Documentation/CodingGuidelines has a lot of style\nrequirements, including trying to stay to an 80-character\nwidth. There are a lot of examples in the codebase that\nshow how to break a method prototype across multiple lines\nwith tasteful vertical alignment.\n\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 5466636e666..ef220869851 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -19,6 +19,7 @@\n> #include \"pathspec.h\"\n> #include \"help.h\"\n> #include \"prompt.h\"\n> +#include \"add-menu.h\"\n> \n> static int force = -1; /* unset */\n> static int interactive;\n> @@ -39,14 +40,6 @@ static const char *msg_warn_lstat_failed = N_(\"could not lstat %s\\n\");\n> static const char *msg_skip_cwd = N_(\"Refusing to remove current working directory\\n\");\n> static const char *msg_would_skip_cwd = N_(\"Would refuse to remove current working directory\\n\");\n> \n> -enum color_clean {\n> -\tCLEAN_COLOR_RESET = 0,\n> -\tCLEAN_COLOR_PLAIN = 1,\n> -\tCLEAN_COLOR_PROMPT = 2,\n> -\tCLEAN_COLOR_HEADER = 3,\n> -\tCLEAN_COLOR_HELP = 4,\n> -\tCLEAN_COLOR_ERROR = 5\n> -};\n\nThis removal doesn't seem valuable. I think this color set\nshould remain in the builtin, especially because it's being\nused further down.\n\nAlternatively, the names can be renamed to \"MENU_COLOR_...\"\nto apply to all commands in the add-menu.h API.\n\n> static const char *color_interactive_slots[] = {\n> \t[CLEAN_COLOR_ERROR] = \"error\",\n> @@ -58,7 +51,7 @@ static const char *color_interactive_slots[] = {\n> };\n> \n> static int clean_use_color = -1;\n> -static char clean_colors[][COLOR_MAXLEN] = {\n> +static clean_color_settings clean_colors = {\n> \t[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,\n> \t[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,\n> \t[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,\n> @@ -67,36 +60,8 @@ static char clean_colors[][COLOR_MAXLEN] = {\n> \t[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,\n> };\n> \n> -#define MENU_OPTS_SINGLETON\t\t01\n> -#define MENU_OPTS_IMMEDIATE\t\t02\n> -#define MENU_OPTS_LIST_ONLY\t\t04\n> -\n> -struct menu_opts {\n> -\tconst char *header;\n> -\tconst char *prompt;\n> -\tint flags;\n> -};\n> -\n\nGenerally, the remainder of this patch is primarily\ndeletions. Although, we could make it be completely\ndeletions if the API method names are changed (not to\nstart with \"clean_\" and then all these calls are\nmodified in one go:\n\n> static void prompt_help_cmd(int singleton)\n> {\n> -\tclean_print_color(CLEAN_COLOR_HELP);\n> +\tclean_print_color(CLEAN_COLOR_HELP, &clean_colors, &clean_use_color);\n\nHere, we would have something like \"menu_print_color()\"\ninstead. To avoid adding too much deletion noise when\nmaking these important changes, we can add MAYBE_UNUSED\nto all of the static methods that become unreachable.\n\nAfter a patch that does that refactor, a diff that only\ndeletes lines (does not add any) would be very easy to\nverify.\n\nThanks,\n-Stolee\n"},{"id":"455814","messageId":"8d483e1d-1865-c475-cbe3-78fa1f7f8bfb@github.com","threadId":"57884","inReplyTo":"13bc75a2b0510f55e9a73852838b3b11683f13a2.1653286345.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] add-menu: added add-menu to lib objects","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-23T20:03:34Z","receivedAt":"2022-05-23T20:03:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/23/22 2:12 AM, Nadav Goldstein via GitGitGadget wrote:\n> From: Nadav Goldstein \n> \n> added to the lib objects the add menu module which is\n> simply extracted functions from clear.c\n> (which in the next commit will be removed and instead\n> clear will use the outsourced functions).\n\nPlease rewrite according to Git style. (Mentioned in\nmy reply to patch 2 with more detail.)\n\n> diff --git a/add-menu.c b/add-menu.c\n\nI think I said something in another place that was a\nbit incorrect: I think of \"git add -p\" as interactive\nadd, but really it's \"git add -i\" that is the\nequivalent. The \"git add -i\" menu is very similar to\nthe \"git clean -i\" menu, as it is said in comments.\n\nThus, perhaps the best thing to do would be to unify\nthe two implementations, if at all possible. The one\nin builtin/clean.c is from 2013 while the one in\nadd-interactive.c is much more recent.\n\nThe biggest test of your new API is whether it can\nsupport _both_ of these existing interactive menus\nbefore adding one to 'git stash'.\n\n> new file mode 100644\n> index 00000000000..6a1c125d113\n> --- /dev/null\n> +++ b/add-menu.c\n> @@ -0,0 +1,339 @@\n> +#include \n\nIn the Git project, the first include should either be\n\"cache.h\" or \"git-compat-utils.h\". For this API,\ngit-compat-utils.h should suffice, since there should\nnot be anything from the Git data model that actually\nmatters here.\n\n> +#include \"builtin.h\"\n> +#include \"add-menu.h\"\n> +#include \"cache.h\"\n> +#include \"config.h\"\n> +#include \"dir.h\"\n> +#include \"parse-options.h\"\n> +#include \"string-list.h\"\n> +#include \"quote.h\"\n> +#include \"column.h\"\n> +#include \"color.h\"\n> +#include \"pathspec.h\"\n> +#include \"help.h\"\n> +#include \"prompt.h\"\n\nI doubt that these are all required. Please check to\nsee what you are using from each of these includes and\nremove as necessary.\n\n> +static const char *clean_get_color(enum color_clean ix)\n> +{\n> +\tif (want_color(clean_use_color))\n> +\t\treturn clean_colors[ix];\n> +\treturn \"\";\n> +}\n\nPlease update the method names to not care about clean.\nThis is especially true in the public API in the *.h file.\n\n> +void clean_print_color(enum color_clean ix)\n> +{\n> +\tprintf(\"%s\", clean_get_color(ix));\n> +}\n> \\ No newline at end of file\n\nnit: please make sure the file ends with exactly one newline.\n\nOne problem with this approach of adding the code to this\nnew *.c file and then later removing the code from clean is\nthat we lose 'git blame' or 'git log -L' history across the\nmove. It's much harder to detect copies than to detect moved\nlines of code.\n\nI don't have a good solution in mind right now, but it's\nworth thinking about.\n\n> diff --git a/add-menu.h b/add-menu.h\n> new file mode 100644\n> index 00000000000..52e5ccb1800\n> --- /dev/null\n> +++ b/add-menu.h\n> @@ -0,0 +1,51 @@\n\nDon't forget the #ifndef __ADD_MENU__/#define __ADD_MENU__\ntrick to avoid multiple declarations of these values.\n\n> +int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int));\n> \\ No newline at end of file\n\nnit: newline here, too.\n\nThanks,\n-Stolee\n"},{"id":"455849","messageId":"xmqq1qwkq96f.fsf@gitster.g","threadId":"57884","inReplyTo":"8d483e1d-1865-c475-cbe3-78fa1f7f8bfb@github.com","subject":"Re: [PATCH v2 1/2] add-menu: added add-menu to lib objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-23T20:35:20Z","receivedAt":"2022-05-23T20:35:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> The biggest test of your new API is whether it can\n> support _both_ of these existing interactive menus\n> before adding one to 'git stash'.\n\nA great piece of advice.\n\n> One problem with this approach of adding the code to this\n> new *.c file and then later removing the code from clean is\n> that we lose 'git blame' or 'git log -L' history across the\n> move. It's much harder to detect copies than to detect moved\n> lines of code.\n>\n> I don't have a good solution in mind right now, but it's\n> worth thinking about.\n\nI actually do ;-)\n\nOne single preliminary patch can rename the helper functions, update\nthe direct reference to the color configuration table into passing a\nparameter to the table, doing the same to the menu that defines the\nshorthand, help text, and implementation of the command, and any\nother necessary adjustment. Or if these tasks are turns out to be\ntoo large to do in a single commit, they can be done in steps. This\npreliminary refactoring patch (or a series of patches) can be done\nwhile everything is still in builtin/clean.c (or we could start by\nrefactoring the one in add-interactive.c instead).\n\nThen the second patch in the series would move the refactored\nlibrary-ish code into the new interactive-menu.c file, and add the\ninteractive-menu.h header for users to include. The first such user\nwill be builtin/clean.c (or if we started from add-interactive.c,\nthen that one). \"blame\", \"log\" and \"diff --color-moved\" would all\nnotice that an already refactored block of code was lifted from one\nsource file and moved to the new place in this step.\n\nThe third patch in the series would convert add-interactive.c (or if\nwe started from it, then builtin/clean.c) to be the second user of\nthe API. There might need a few preliminary steps in the file to be\nconverted before it happens to match the function signatures, etc.\n\nAfter that, \"git stash\" will have its own interactive mode that uses\nthe new API from day one.\n"},{"id":"478535","messageId":"pull.1232.v3.git.1687219414844.gitgitgadget@gmail.com","threadId":"57884","inReplyTo":"pull.1232.v2.git.1653286345.gitgitgadget@gmail.com","subject":"[PATCH v3] Introduced force flag to the git stash clear subcommand.","fromName":"Nadav Goldstein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-20T00:03:34Z","receivedAt":"2023-06-20T00:03:41Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"From: Nadav Goldstein \n\nstash clean subcommand now support the force flag, along\nwith the configuration var stash.requireforce, that if\nset to true, will make git stash clear fail unless supplied\nwith force flag.\n\nSigned-off-by: Nadav Goldstein \n---\n stash clear: added safety flag for stash clear subcommand\n \n This patch started to solve the issue of easy trigger of git stash\n clear. I first went with using an interactive (-i) flag to stash clear,\n but following the conversations I had here I understood that it was a\n misleading flag and not a good direction for implementation.\n \n So in this version of the patch (v3), I went with Junio proposal to\n introduce force flag to the clean subcommand.\n \n Thanks!\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1232%2Fnadav96%2Fclear-stash-prompt-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1232/nadav96/clear-stash-prompt-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1232\n\nRange-diff vs v2:\n\n 1: 13bc75a2b05 < -: ----------- add-menu: added add-menu to lib objects\n 2: 7271a285d18 < -: ----------- clean: refector to the interactive part of clean\n -: ----------- > 1: 6150ec27b5a Introduced force flag to the git stash clear subcommand.\n\n\n Documentation/git-stash.txt | 12 ++++++++++--\n builtin/stash.c | 12 +++++++++++-\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\nindex f4bb6114d91..e95410d507e 100644\n--- a/Documentation/git-stash.txt\n+++ b/Documentation/git-stash.txt\n@@ -20,7 +20,7 @@ SYNOPSIS\n \t [--] [...]]\n 'git stash' save [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]\n \t [-u | --include-untracked] [-a | --all] []\n-'git stash' clear\n+'git stash' clear [-f | --force]\n 'git stash' create []\n 'git stash' store [(-m | --message) ] [-q | --quiet] \n \n@@ -130,7 +130,7 @@ the stash entry is applied on top of the commit that was HEAD at the\n time `git stash` was run, it restores the originally stashed state\n with no conflicts.\n \n-clear::\n+clear [-f|--force]::\n \tRemove all the stash entries. Note that those entries will then\n \tbe subject to pruning, and may be impossible to recover (see\n \t'Examples' below for a possible strategy).\n@@ -208,6 +208,14 @@ to learn how to operate the `--patch` mode.\n The `--patch` option implies `--keep-index`. You can use\n `--no-keep-index` to override this.\n \n+-f::\n+--force::\n+\tThis option is only valid for `clear` command\n++\n+If the Git configuration variable stash.requireForce is set\n+to true, 'git stash clear' will refuse to remove all the stash \n+entries unless given -f.\n+\n -S::\n --staged::\n \tThis option is only valid for `push` and `save` commands.\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex a7e17ffe384..d037bc4f69c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -53,7 +53,7 @@\n #define BUILTIN_STASH_CREATE_USAGE \\\n \tN_(\"git stash create []\")\n #define BUILTIN_STASH_CLEAR_USAGE \\\n-\t\"git stash clear\"\n+\t\"git stash clear [-f | --force]\"\n \n static const char * const git_stash_usage[] = {\n \tBUILTIN_STASH_LIST_USAGE,\n@@ -122,6 +122,7 @@ static const char * const git_stash_save_usage[] = {\n \n static const char ref_stash[] = \"refs/stash\";\n static struct strbuf stash_index_path = STRBUF_INIT;\n+static int clear_require_force = 0;\n \n /*\n * w_commit is set to the commit containing the working tree\n@@ -246,7 +247,9 @@ static int do_clear_stash(void)\n \n static int clear_stash(int argc, const char **argv, const char *prefix)\n {\n+\tint force = 0;\n \tstruct option options[] = {\n+\t\tOPT__FORCE(&force, N_(\"force\"), PARSE_OPT_NOCOMPLETE),\n \t\tOPT_END()\n \t};\n \n@@ -258,6 +261,9 @@ static int clear_stash(int argc, const char **argv, const char *prefix)\n \t\treturn error(_(\"git stash clear with arguments is \"\n \t\t\t \"unimplemented\"));\n \n+\tif (!force && clear_require_force)\n+\t\treturn error(_(\"fatal: stash.requireForce set to true and -f was not given; refusing to clear stash\"));\n+\n \treturn do_clear_stash();\n }\n \n@@ -851,6 +857,10 @@ static int git_stash_config(const char *var, const char *value, void *cb)\n \t\tshow_include_untracked = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"stash.requireforce\")) {\n+\t\tclear_require_force = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \treturn git_diff_basic_config(var, value, cb);\n }\n \n\nbase-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d\n-- \ngitgitgadget\n"},{"id":"478536","messageId":"xmqqy1keodjj.fsf@gitster.g","threadId":"57884","inReplyTo":"pull.1232.v3.git.1687219414844.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] Introduced force flag to the git stash clear subcommand.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-20T06:25:52Z","receivedAt":"2023-06-20T06:25:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nadav Goldstein via GitGitGadget\" writes:\n\n> From: Nadav Goldstein \n>\n> stash clean subcommand now support the force flag, along\n> with the configuration var stash.requireforce, that if\n> set to true, will make git stash clear fail unless supplied\n> with force flag.\n\nDocumentation/SubmittingPatches gives many helpful hints on how to\nwrite log messages for the project.\n\n> @@ -208,6 +208,14 @@ to learn how to operate the `--patch` mode.\n> The `--patch` option implies `--keep-index`. You can use\n> `--no-keep-index` to override this.\n> \n> +-f::\n> +--force::\n> +\tThis option is only valid for `clear` command\n\nMissing full-stop?\n\n> ++\n> +If the Git configuration variable stash.requireForce is set\n\nDrop \"Git\" perhaps? I haven't seen any other place that says \"Git\nconfiguration variable X\" when talking about a single variable (it\nprobably is OK to call those defined in a Git configuration file\ncollectively as \"Git configuration variables\", though).\n\n> +to true, 'git stash clear' will refuse to remove all the stash \n> +entries unless given -f.\n\nI am not sure how much value users would get by requiring \"--force\",\nthough. I know this was (partly) modeled after \"git clean\", but\nover there, when the required \"--force\" is not given, the user would\ngive \"--dry-run\" (or \"-n\"), and the user will see what would be\nremoved if the user gave \"--force\". If missing \"--force\" made \"git\nstash clear\" show the stash entries that would be lost, then after\nseeing an error message, it would be easier for the user to decide\nif their next move should be to re-run the command with \"--force\",\nor there are some precious entries and the user is not ready to do\n\"stash clear\".\n\nBut just refusing to run without giving any other information will\njust train the user to give \"git stash clear --force\" without\nthinking, because getting \"because you did not give the required\n--force option, I am not doing anything\" is only annoying without\ngiving any useful information.\n\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index a7e17ffe384..d037bc4f69c 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -53,7 +53,7 @@\n> #define BUILTIN_STASH_CREATE_USAGE \\\n> \tN_(\"git stash create []\")\n> #define BUILTIN_STASH_CLEAR_USAGE \\\n> -\t\"git stash clear\"\n> +\t\"git stash clear [-f | --force]\"\n> \n> static const char * const git_stash_usage[] = {\n> \tBUILTIN_STASH_LIST_USAGE,\n> @@ -122,6 +122,7 @@ static const char * const git_stash_save_usage[] = {\n> \n> static const char ref_stash[] = \"refs/stash\";\n> static struct strbuf stash_index_path = STRBUF_INIT;\n> +static int clear_require_force = 0;\n\nDo not explicitly initialize globals to 0 or NULL; let BSS take care\nof the zero initialization, instead.\n\n> @@ -246,7 +247,9 @@ static int do_clear_stash(void)\n> \n> static int clear_stash(int argc, const char **argv, const char *prefix)\n> {\n> +\tint force = 0;\n> \tstruct option options[] = {\n> +\t\tOPT__FORCE(&force, N_(\"force\"), PARSE_OPT_NOCOMPLETE),\n> \t\tOPT_END()\n> \t};\n\nAs this topic focuses on \"git stash clear\", this is OK (and the\ndescription in the documentation that says that \"force\" is currently\nsupported only by \"clear\" is also fine), but is \"clear\" the only\ndestructive subcommand and no other subcommand will want to learn\nthe \"--force\" for similar safety in the future? The answer to this\nquestion matters because ...\n\n> @@ -258,6 +261,9 @@ static int clear_stash(int argc, const char **argv, const char *prefix)\n> \t\treturn error(_(\"git stash clear with arguments is \"\n> \t\t\t \"unimplemented\"));\n> \n> +\tif (!force && clear_require_force)\n> +\t\treturn error(_(\"fatal: stash.requireForce set to true and -f was not given; refusing to clear stash\"));\n> +\n> \treturn do_clear_stash();\n> }\n> \n> @@ -851,6 +857,10 @@ static int git_stash_config(const char *var, const char *value, void *cb)\n> \t\tshow_include_untracked = git_config_bool(var, value);\n> \t\treturn 0;\n> \t}\n> +\tif (!strcmp(var, \"stash.requireforce\")) {\n\n... the naming of this variable, facing the end users, does not\nlimit itself to \"clear\" at all. It gives an impression that setting\nthis will require \"--force\" for all other subcommands that would\nsupport it. However ...\n\n> +\t\tclear_require_force = git_config_bool(var, value);\n\n... inside the code, the variable is named in such a way that it is\nonly about the \"clear\" subcommand and nothing else.\n\nI suspect that the end-user facing \"stash.requireforce\" should be\nrenamed to make it clear that it is about \"stash clear\" subcommand,\nand not everywhere in \"stash\" requires \"--force\". Nobody wants to\nkeep saying \"git stash save --force\" ;-)\n\n> +\t\treturn 0;\n> +\t}\n\nThanks.\n"},{"id":"478591","messageId":"1540e884-08c7-922e-1fd9-65616268c1c9@gmail.com","threadId":"57884","inReplyTo":"xmqqy1keodjj.fsf@gitster.g","subject":"Re: [PATCH v3] Introduced force flag to the git stash clear subcommand.","fromName":"Nadav Goldstein","fromEmail":"nadav.goldstein96@gmail.com","sentAt":"2023-06-20T19:54:53Z","receivedAt":"2023-06-20T19:55:11Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"> I am not sure how much value users would get by requiring \"--force\",\n> though. I know this was (partly) modeled after \"git clean\", but\n> over there, when the required \"--force\" is not given, the user would\n> give \"--dry-run\" (or \"-n\"), and the user will see what would be\n> removed if the user gave \"--force\". If missing \"--force\" made \"git\n> stash clear\" show the stash entries that would be lost, then after\n> seeing an error message, it would be easier for the user to decide\n> if their next move should be to re-run the command with \"--force\",\n> or there are some precious entries and the user is not ready to do\n> \"stash clear\".\n>\n> But just refusing to run without giving any other information will\n> just train the user to give \"git stash clear --force\" without\n> thinking, because getting \"because you did not give the required\n> --force option, I am not doing anything\" is only annoying without\n> giving any useful information.\n\n\nI see, but isn't the same argument apply for git clean? if not adding \nthe force flag, the same message as I wrote appear in git clean (I \ncopied it from there), and it will exit without any other information, \nhence given your argument, running git clean is also not very useful.\n\n\nOne can argue that git clean --dry-run == git stash list\n\n\nI suggested in the beginning of this thread to ask the user if he is \nsure he want to proceed (default to no), and only if he wrote y/yes \nproceed with the action (and force will just do it, or requireforce=false).\n\n\nThe reason I suggested it is because when running git stash clear, it \nwill remain in the user recent commands, and when the user will navigate \nthrough the commands history in the terminal, he might accidentally fire \ngit stash clear, and this confirmation will be another safeguard against \nthis mistake.\n\n\nMaybe it will be useful for git clean as well for the same reasons.\nAlso when the user types git clean, I argue he wanted to clean or he did \nit by mistake, and In both scenarios I don't see why making git clean \njust fail will be useful.\n\n\nSo what do you think? Maybe we should present in both clean and clear a \nconfirmation message? (only if requireforce=true)\n\n\nWhat do you think?\n\n"},{"id":"478596","messageId":"xmqqpm5plv41.fsf@gitster.g","threadId":"57884","inReplyTo":"1540e884-08c7-922e-1fd9-65616268c1c9@gmail.com","subject":"Re: [PATCH v3] Introduced force flag to the git stash clear subcommand.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-20T20:46:54Z","receivedAt":"2023-06-20T20:47:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nadav Goldstein writes:\n\n> I see, but isn't the same argument apply for git clean? if not adding\n> the force flag, the same message as I wrote appear in git clean (I\n> copied it from there), and it will exit without any other information,\n> hence given your argument, running git clean is also not very useful.\n\nThe thing is that \"git clean\" by default forces people to choose\nbetween \"-f\" and \"-n\" to force people to understand the issue. And\nonce they understand the issue, they'd learn to run \"clean -n\"\nfirst, which lets them see what would be removed, before they run\n\"clean -f\". Does your \"stash clear\" work the same way? I do not\nthink so.\n\nIf there is \"stash clear --dry-run\" that runs \"stash list\", it might\nbe similar, but not similar enough. I wonder if \"stash clear\", when\nstashClear.requireForce is set to true and unless \"--force\" is\ngiven, should do \"stash list\" and then error out. I dunno.\n\n"},{"id":"478597","messageId":"CAPig+cTXUKTWYhLpu-zRf4DFhHq_BnLZFznEc3pk-qiqcS0_CA@mail.gmail.com","threadId":"57884","inReplyTo":"1540e884-08c7-922e-1fd9-65616268c1c9@gmail.com","subject":"Re: [PATCH v3] Introduced force flag to the git stash clear subcommand.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-06-20T21:01:27Z","receivedAt":"2023-06-20T21:01:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 20, 2023 at 4:05 PM Nadav Goldstein\n wrote:\n> > I am not sure how much value users would get by requiring \"--force\",\n> > though. I know this was (partly) modeled after \"git clean\", but\n> > over there, when the required \"--force\" is not given, the user would\n> > give \"--dry-run\" (or \"-n\"), and the user will see what would be\n> > removed if the user gave \"--force\". If missing \"--force\" made \"git\n> > stash clear\" show the stash entries that would be lost, then after\n> > seeing an error message, it would be easier for the user to decide\n> > if their next move should be to re-run the command with \"--force\",\n> > or there are some precious entries and the user is not ready to do\n> > \"stash clear\".\n> >\n> > But just refusing to run without giving any other information will\n> > just train the user to give \"git stash clear --force\" without\n> > thinking, because getting \"because you did not give the required\n> > --force option, I am not doing anything\" is only annoying without\n> > giving any useful information.\n>\n> I see, but isn't the same argument apply for git clean? if not adding\n> the force flag, the same message as I wrote appear in git clean (I\n> copied it from there), and it will exit without any other information,\n> hence given your argument, running git clean is also not very useful.\n\nFor what it's worth, I had the same reaction as Junio upon reading\nthis patch; specifically, that it will train users to type \"git stash\nclear --force\" mechanically without thinking, thus won't be much of a\nsafeguard.\n\n> I suggested in the beginning of this thread to ask the user if he is\n> sure he want to proceed (default to no), and only if he wrote y/yes\n> proceed with the action (and force will just do it, or requireforce=false).\n>\n> The reason I suggested it is because when running git stash clear, it\n> will remain in the user recent commands, and when the user will navigate\n> through the commands history in the terminal, he might accidentally fire\n> git stash clear, and this confirmation will be another safeguard against\n> this mistake.\n>\n> Maybe it will be useful for git clean as well for the same reasons.\n> Also when the user types git clean, I argue he wanted to clean or he did\n> it by mistake, and In both scenarios I don't see why making git clean\n> just fail will be useful.\n\n\"git clean\" is in a rather different (and more severe) boat since file\ndeletion is irrevocable, whereas a stash thrown away by \"git stash\nclear\" (or \"git stash drop\") can be recovered (at least until it gets\ngarbage-collected). So, rather than adding a --force option or an\ninteractive \"yes/no\" prompt, perhaps a better approach would be to\nhave \"git stash clear\" (and \"git stash drop\") print out advice\nexplaining to the user how to recover the dropped stash(es), much like\n\"git switch\" or \"git checkout\" prints advice explaining how to recover\ncommits left dangling on a detached head.\n"},{"id":"478602","messageId":"9e1e38af-9bc5-2ec7-f3ad-d3325969b484@gmail.com","threadId":"57884","inReplyTo":"CAPig+cTXUKTWYhLpu-zRf4DFhHq_BnLZFznEc3pk-qiqcS0_CA@mail.gmail.com","subject":"Re: [PATCH v3] Introduced force flag to the git stash clear subcommand.","fromName":"Nadav Goldstein","fromEmail":"nadav.goldstein96@gmail.com","sentAt":"2023-06-20T21:42:47Z","receivedAt":"2023-06-20T21:42:55Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"I see both of your points, and I agree adding the flag will only make \nusers type the flag without thinking.\n\n\nBut I still don't understand why do we need git clean without any flags.\n\n\nThe only users that will run git clean are new users that don't know you \nneed to run it with -f or experienced users that set clean.requireforce \n= false.\n\n\nMoreover, we can also assume that every user that have \nclean.requireforce = true (the default), and ran git clean, did so by \nmistake/intended to clean, So why don't we offer him interactive way to \nunderstand the consequences and confirm his action? (and explain about \nclean.requireforce like we currently do).\nBy doing it this way, the change will not effect experienced users \nbecause they either run git clean -f when they need to clean or they set \nclean.requireforce = false, and then the change will not apply to them.\n\n\nThis argument is also for stash clear.\n\n\nThanks.\n\nOn 21/06/2023 0:01, Eric Sunshine wrote:\n> On Tue, Jun 20, 2023 at 4:05 PM Nadav Goldstein\n> wrote:\n>>> I am not sure how much value users would get by requiring \"--force\",\n>>> though. I know this was (partly) modeled after \"git clean\", but\n>>> over there, when the required \"--force\" is not given, the user would\n>>> give \"--dry-run\" (or \"-n\"), and the user will see what would be\n>>> removed if the user gave \"--force\". If missing \"--force\" made \"git\n>>> stash clear\" show the stash entries that would be lost, then after\n>>> seeing an error message, it would be easier for the user to decide\n>>> if their next move should be to re-run the command with \"--force\",\n>>> or there are some precious entries and the user is not ready to do\n>>> \"stash clear\".\n>>>\n>>> But just refusing to run without giving any other information will\n>>> just train the user to give \"git stash clear --force\" without\n>>> thinking, because getting \"because you did not give the required\n>>> --force option, I am not doing anything\" is only annoying without\n>>> giving any useful information.\n>> I see, but isn't the same argument apply for git clean? if not adding\n>> the force flag, the same message as I wrote appear in git clean (I\n>> copied it from there), and it will exit without any other information,\n>> hence given your argument, running git clean is also not very useful.\n> For what it's worth, I had the same reaction as Junio upon reading\n> this patch; specifically, that it will train users to type \"git stash\n> clear --force\" mechanically without thinking, thus won't be much of a\n> safeguard.\n>\n>> I suggested in the beginning of this thread to ask the user if he is\n>> sure he want to proceed (default to no), and only if he wrote y/yes\n>> proceed with the action (and force will just do it, or requireforce=false).\n>>\n>> The reason I suggested it is because when running git stash clear, it\n>> will remain in the user recent commands, and when the user will navigate\n>> through the commands history in the terminal, he might accidentally fire\n>> git stash clear, and this confirmation will be another safeguard against\n>> this mistake.\n>>\n>> Maybe it will be useful for git clean as well for the same reasons.\n>> Also when the user types git clean, I argue he wanted to clean or he did\n>> it by mistake, and In both scenarios I don't see why making git clean\n>> just fail will be useful.\n> \"git clean\" is in a rather different (and more severe) boat since file\n> deletion is irrevocable, whereas a stash thrown away by \"git stash\n> clear\" (or \"git stash drop\") can be recovered (at least until it gets\n> garbage-collected). So, rather than adding a --force option or an\n> interactive \"yes/no\" prompt, perhaps a better approach would be to\n> have \"git stash clear\" (and \"git stash drop\") print out advice\n> explaining to the user how to recover the dropped stash(es), much like\n> \"git switch\" or \"git checkout\" prints advice explaining how to recover\n> commits left dangling on a detached head.\n>\n>\n"}]}