{"thread":{"id":"60706","subject":"[PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()","startedAt":"2024-01-09T15:25:40Z","lastAt":"2024-01-20T15:32:05Z","messageCount":36,"participants":["Rubén Justo","Junio C Hamano","Taylor Blau","Jeff King","Dragan Simic"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"486435","messageId":"7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com","threadId":"60706","inReplyTo":null,"subject":"[PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-09T15:25:32Z","receivedAt":"2024-01-09T15:25:40Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Using advise_if_enabled() to display an advice will automatically\ninclude instructions on how to disable the advice, along with the main\nadvice:\n\n\thint: use --reapply-cherry-picks to include skipped commits\n\thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n\nThis may become distracting or \"noisy\" over time, while the user may\nstill want to receive the main advice.\n\nLet's have a switch to allow disabling this automatic advice.\n\nRubén Justo (3):\n t/test-tool: usage description\n t/test-tool: handle -c = arguments\n advice: allow disabling the automatic hint in advise_if_enabled()\n\n advice.c | 3 ++-\n advice.h | 3 ++-\n t/helper/test-tool.c | 15 +++++++++++++--\n t/t0018-advice.sh | 8 ++++++++\n 4 files changed, 25 insertions(+), 4 deletions(-)\n\n-- \n2.42.0\n\n"},{"id":"486436","messageId":"ec57072d-0069-4d07-a695-b89436350568@gmail.com","threadId":"60706","inReplyTo":"7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com","subject":"[PATCH 1/3] t/test-tool: usage description","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-09T15:29:30Z","receivedAt":"2024-01-09T15:29:38Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Even though this is an internal tool, let's keep the usage description\ncorrect and well organized.\n\nSigned-off-by: Rubén Justo \n---\n t/helper/test-tool.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 37ba996539..d9f57c20db 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -5,7 +5,7 @@\n #include \"parse-options.h\"\n \n static const char * const test_tool_usage[] = {\n-\t\"test-tool [-C ] ...]]\",\n+\t\"test-tool [-C ] [...]]\",\n \tNULL\n };\n \n@@ -100,7 +100,7 @@ static NORETURN void die_usage(void)\n {\n \tsize_t i;\n \n-\tfprintf(stderr, \"usage: test-tool [args]\\n\");\n+\tfprintf(stderr, \"usage: %s\\n\", test_tool_usage[0]);\n \tfor (i = 0; i < ARRAY_SIZE(cmds); i++)\n \t\tfprintf(stderr, \" %s\\n\", cmds[i].name);\n \texit(128);\n-- \n2.42.0\n\n"},{"id":"486437","messageId":"6a4d6a56-ab6f-4557-a5a3-1713f57cbfc9@gmail.com","threadId":"60706","inReplyTo":"7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com","subject":"[PATCH 2/3] t/test-tool: handle -c = arguments","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-09T15:29:57Z","receivedAt":"2024-01-09T15:30:01Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Soon we're going to need to pass configuration values to a command in\ntest-tool.\n\nLet's teach test-tool to take config values via command line arguments.\n\nSigned-off-by: Rubén Justo \n---\n t/helper/test-tool.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex d9f57c20db..7eba4ec9ab 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -3,9 +3,10 @@\n #include \"test-tool-utils.h\"\n #include \"trace2.h\"\n #include \"parse-options.h\"\n+#include \"config.h\"\n \n static const char * const test_tool_usage[] = {\n-\t\"test-tool [-C ] [...]]\",\n+\t\"test-tool [-C ] [-c =] [...]\",\n \tNULL\n };\n \n@@ -106,6 +107,13 @@ static NORETURN void die_usage(void)\n \texit(128);\n }\n \n+static int parse_config_option(const struct option *opt, const char *arg,\n+\t\t\t int unset)\n+{\n+\tgit_config_push_parameter(arg);\n+\treturn 0;\n+}\n+\n int cmd_main(int argc, const char **argv)\n {\n \tint i;\n@@ -113,6 +121,9 @@ int cmd_main(int argc, const char **argv)\n \tstruct option options[] = {\n \t\tOPT_STRING('C', NULL, &working_directory, \"directory\",\n \t\t\t \"change the working directory\"),\n+\t\tOPT_CALLBACK('c', NULL, NULL, \"=\",\n+\t\t\t \"pass a configuration parameter to the command\",\n+\t\t\t parse_config_option),\n \t\tOPT_END()\n \t};\n \n-- \n2.42.0\n\n"},{"id":"486438","messageId":"d6099d78-43c6-4709-9121-11f84228cf91@gmail.com","threadId":"60706","inReplyTo":"7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com","subject":"[PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-09T15:30:16Z","receivedAt":"2024-01-09T15:30:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Using advise_if_enabled() to display an advice will automatically\ninclude instructions on how to disable the advice, along with the\nmain advice:\n\n\thint: use --reapply-cherry-picks to include skipped commits\n\thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n\nThis can become distracting or noisy over time, while the user may\nstill want to receive the main advice.\n\nLet's have a switch to allow disabling this automatic advice.\n\nSigned-off-by: Rubén Justo \n---\n advice.c | 3 ++-\n advice.h | 3 ++-\n t/t0018-advice.sh | 8 ++++++++\n 3 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 50c79443ba..fa203f8806 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -79,6 +79,7 @@ static struct {\n \t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\", 1 },\n \t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\", 1 },\n \t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\", 1 },\n+\t[ADVICE_ADVICE_OFF]\t\t\t\t= { \"adviceOff\", 1 },\n };\n \n static const char turn_off_instructions[] =\n@@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,\n \n \tstrbuf_vaddf(&buf, advice, params);\n \n-\tif (display_instructions)\n+\tif (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))\n \t\tstrbuf_addf(&buf, turn_off_instructions, key);\n \n \tfor (cp = buf.buf; *cp; cp = np) {\ndiff --git a/advice.h b/advice.h\nindex 2affbe1426..1f2eef034e 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -10,7 +10,7 @@ struct string_list;\n * Add the new config variable to Documentation/config/advice.txt.\n * Call advise_if_enabled to print your advice.\n */\n- enum advice_type {\n+enum advice_type {\n \tADVICE_ADD_EMBEDDED_REPO,\n \tADVICE_ADD_EMPTY_PATHSPEC,\n \tADVICE_ADD_IGNORED_FILE,\n@@ -50,6 +50,7 @@ struct string_list;\n \tADVICE_WAITING_FOR_EDITOR,\n \tADVICE_SKIPPED_CHERRY_PICKS,\n \tADVICE_WORKTREE_ADD_ORPHAN,\n+\tADVICE_ADVICE_OFF,\n };\n \n int git_default_advice_config(const char *var, const char *value);\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex c13057a4ca..0b6a8b4a10 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'advice without the instructions to disable it' '\n+\tcat >expect <<-\\EOF &&\n+\thint: This is a piece of advice\n+\tEOF\n+\ttest-tool -c advice.adviceOff=0 advise \"This is a piece of advice\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.42.0\n\n"},{"id":"486455","messageId":"xmqqy1cygyjp.fsf@gitster.g","threadId":"60706","inReplyTo":"ec57072d-0069-4d07-a695-b89436350568@gmail.com","subject":"Re: [PATCH 1/3] t/test-tool: usage description","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T18:19:06Z","receivedAt":"2024-01-09T18:19:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> Subject: Re: [PATCH 1/3] t/test-tool: usage description\n\nGood eyes to spot the missing close-angle-bracket. I'll add some\nmissing verb, e.g. \"fix usage string\", while queuing.\n\nI would not bother replacing the fprintf() format string in the same\npatch. Hits from\n\n $ git grep '\"usage:' t/helper\n\nindicates that far less than half (3 among 12) reuses usage_str[]\nfor this purpose. Making these \"usage:\" strings come from a unified\nAPI (perhaps parse_options() family of functions have something more\nappropriate than ad-hoc use of fprintf()? I didn't check) might be\na welcome change but that is clearly outside the scope of the\nmark-up fix, and I do not see touching only this one that still uses\nfprintf() advances toward such a goal.\n\nt/helper/test-chmtime.c:\tfprintf(stderr, \"usage: %s %s\\n\", argv[0], usage_str);\nt/helper/test-delta.c:\t\tfprintf(stderr, \"usage: %s\\n\", usage_str);\nt/helper/test-windows-named-pipe.c:\tfprintf(stderr, \"usage: %s %s\\n\", argv[0], usage_string);\n\n\nt/helper/test-advise.c:\t\tdie(\"usage: %s \", argv[0]);\nt/helper/test-csprng.c:\t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\nt/helper/test-genrandom.c:\t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\nt/helper/test-genzeros.c:\t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\nt/helper/test-hash-speed.c:\t\tdie(\"usage: test-tool hash-speed algo_name\");\nt/helper/test-mergesort.c:\tfprintf(stderr, \"usage: test-tool mergesort generate \\n\");\nt/helper/test-strcmp-offset.c:\t\tdie(\"usage: %s \", argv[0]);\nt/helper/test-tool.c:\tfprintf(stderr, \"usage: test-tool [args]\\n\");\n\n\n> Even though this is an internal tool, let's keep the usage description\n> correct and well organized.\n>\n> Signed-off-by: Rubén Justo \n> ---\n> t/helper/test-tool.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index 37ba996539..d9f57c20db 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -5,7 +5,7 @@\n> #include \"parse-options.h\"\n> \n> static const char * const test_tool_usage[] = {\n> -\t\"test-tool [-C ] ...]]\",\n> +\t\"test-tool [-C ] [...]]\",\n> \tNULL\n> };\n> \n> @@ -100,7 +100,7 @@ static NORETURN void die_usage(void)\n> {\n> \tsize_t i;\n> \n> -\tfprintf(stderr, \"usage: test-tool [args]\\n\");\n> +\tfprintf(stderr, \"usage: %s\\n\", test_tool_usage[0]);\n> \tfor (i = 0; i < ARRAY_SIZE(cmds); i++)\n> \t\tfprintf(stderr, \" %s\\n\", cmds[i].name);\n> \texit(128);\n"},{"id":"486456","messageId":"xmqqttnmgyj2.fsf@gitster.g","threadId":"60706","inReplyTo":"6a4d6a56-ab6f-4557-a5a3-1713f57cbfc9@gmail.com","subject":"Re: [PATCH 2/3] t/test-tool: handle -c = arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T18:19:29Z","receivedAt":"2024-01-09T18:19:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Soon we're going to need to pass configuration values to a command in\n> test-tool.\n>\n> Let's teach test-tool to take config values via command line arguments.\n>\n> Signed-off-by: Rubén Justo \n> ---\n> t/helper/test-tool.c | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n\nNice.\n\n>\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index d9f57c20db..7eba4ec9ab 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -3,9 +3,10 @@\n> #include \"test-tool-utils.h\"\n> #include \"trace2.h\"\n> #include \"parse-options.h\"\n> +#include \"config.h\"\n> \n> static const char * const test_tool_usage[] = {\n> -\t\"test-tool [-C ] [...]]\",\n> +\t\"test-tool [-C ] [-c =] [...]\",\n> \tNULL\n> };\n> \n> @@ -106,6 +107,13 @@ static NORETURN void die_usage(void)\n> \texit(128);\n> }\n> \n> +static int parse_config_option(const struct option *opt, const char *arg,\n> +\t\t\t int unset)\n> +{\n> +\tgit_config_push_parameter(arg);\n> +\treturn 0;\n> +}\n> +\n> int cmd_main(int argc, const char **argv)\n> {\n> \tint i;\n> @@ -113,6 +121,9 @@ int cmd_main(int argc, const char **argv)\n> \tstruct option options[] = {\n> \t\tOPT_STRING('C', NULL, &working_directory, \"directory\",\n> \t\t\t \"change the working directory\"),\n> +\t\tOPT_CALLBACK('c', NULL, NULL, \"=\",\n> +\t\t\t \"pass a configuration parameter to the command\",\n> +\t\t\t parse_config_option),\n> \t\tOPT_END()\n> \t};\n"},{"id":"486457","messageId":"ZZ2OhkDhq1yOp+q3@nand.local","threadId":"60706","inReplyTo":"6a4d6a56-ab6f-4557-a5a3-1713f57cbfc9@gmail.com","subject":"Re: [PATCH 2/3] t/test-tool: handle -c = arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-09T18:20:54Z","receivedAt":"2024-01-09T18:20:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 09, 2024 at 04:29:57PM +0100, Rubén Justo wrote:\n> Soon we're going to need to pass configuration values to a command in\n> test-tool.\n>\n> Let's teach test-tool to take config values via command line arguments.\n\nI wasn't expecting a step like this to appear in this series. I don't\nhave strong feelings about it, especially since test-tool helpers\nalready understand $GIT_DIR/config when they rely on library code which\nimplicitly reads configuration.\n\nBut it does seem odd to have test-tool invocations that intimately\ndepend on a particular set of configuration values. At the very least,\nthis step seems to encourage passing finely tuned configuration values\nto test-tool helpers, which I am not sure is a good idea.\n\nYour patch message suggests that this will be useful in the following\npatch, which makes sense. But I wonder if it would be easier to avoid\nthe test-tool entirely and call some Git command in a state that we\nexpect to generate advice. Then we can test its output with various\nvalues of advice.adviceOff.\n\n> Signed-off-by: Rubén Justo \n> ---\n> t/helper/test-tool.c | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n\nThe patch itself looks reasonable, though.\n\nThanks,\nTaylor\n"},{"id":"486458","messageId":"xmqqo7dugyd4.fsf@gitster.g","threadId":"60706","inReplyTo":"d6099d78-43c6-4709-9121-11f84228cf91@gmail.com","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T18:23:03Z","receivedAt":"2024-01-09T18:23:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Using advise_if_enabled() to display an advice will automatically\n> include instructions on how to disable the advice, along with the\n> main advice:\n>\n> \thint: use --reapply-cherry-picks to include skipped commits\n> \thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n>\n> This can become distracting or noisy over time, while the user may\n> still want to receive the main advice.\n>\n> Let's have a switch to allow disabling this automatic advice.\n>\n> Signed-off-by: Rubén Justo \n> ---\n> advice.c | 3 ++-\n> advice.h | 3 ++-\n> t/t0018-advice.sh | 8 ++++++++\n> 3 files changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/advice.c b/advice.c\n> index 50c79443ba..fa203f8806 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -79,6 +79,7 @@ static struct {\n> \t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\", 1 },\n> \t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\", 1 },\n> \t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\", 1 },\n> +\t[ADVICE_ADVICE_OFF]\t\t\t\t= { \"adviceOff\", 1 },\n> };\n> \n> static const char turn_off_instructions[] =\n> @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,\n> \n> \tstrbuf_vaddf(&buf, advice, params);\n> \n> -\tif (display_instructions)\n> +\tif (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))\n> \t\tstrbuf_addf(&buf, turn_off_instructions, key);\n> \n> \tfor (cp = buf.buf; *cp; cp = np) {\n> diff --git a/advice.h b/advice.h\n> index 2affbe1426..1f2eef034e 100644\n> --- a/advice.h\n> +++ b/advice.h\n> @@ -10,7 +10,7 @@ struct string_list;\n> * Add the new config variable to Documentation/config/advice.txt.\n> * Call advise_if_enabled to print your advice.\n> */\n> - enum advice_type {\n> +enum advice_type {\n> \tADVICE_ADD_EMBEDDED_REPO,\n> \tADVICE_ADD_EMPTY_PATHSPEC,\n> \tADVICE_ADD_IGNORED_FILE,\n> @@ -50,6 +50,7 @@ struct string_list;\n> \tADVICE_WAITING_FOR_EDITOR,\n> \tADVICE_SKIPPED_CHERRY_PICKS,\n> \tADVICE_WORKTREE_ADD_ORPHAN,\n> +\tADVICE_ADVICE_OFF,\n> };\n> \n> int git_default_advice_config(const char *var, const char *value);\n> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\n> index c13057a4ca..0b6a8b4a10 100755\n> --- a/t/t0018-advice.sh\n> +++ b/t/t0018-advice.sh\n> @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success 'advice without the instructions to disable it' '\n> +\tcat >expect <<-\\EOF &&\n> +\thint: This is a piece of advice\n> +\tEOF\n> +\ttest-tool -c advice.adviceOff=0 advise \"This is a piece of advice\" 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis is testing the right thing but in a \"showing off a shiny new\ntoy\" way. We want to make sure we will catch regressions in the\nfuture by testing with a bit more conditions perturbed. For\nexample, with the new \"-c var=val\" mechanism, we could\n\n * set advice.nestedtag to off (which would disable the whole\n advice)\n\n * set advice.adviceoff to on (which should be the same as not\n setting it explicitly at all).\n\nto test different combinations that we were unable to test before\n[2/3] invented the mechanism.\n\nThanks.\n"},{"id":"486459","messageId":"ZZ2QGYBBmK8cSYBD@nand.local","threadId":"60706","inReplyTo":"d6099d78-43c6-4709-9121-11f84228cf91@gmail.com","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-09T18:27:37Z","receivedAt":"2024-01-09T18:27:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:\n> Using advise_if_enabled() to display an advice will automatically\n> include instructions on how to disable the advice, along with the\n> main advice:\n>\n> \thint: use --reapply-cherry-picks to include skipped commits\n> \thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n>\n> This can become distracting or noisy over time, while the user may\n> still want to receive the main advice.\n\nPresumably for more trivial pieces of advice, a user may want to\nimmediately disable those hints in the future more quickly after first\nreceiving the advice, in which case this feature may not be as useful\nfor them.\n\nBut for trickier pieces of advice, I agree completely with your\nreasoning and think that something like this makes sense.\n\n> Let's have a switch to allow disabling this automatic advice.\n>\n> Signed-off-by: Rubén Justo \n> ---\n> advice.c | 3 ++-\n> advice.h | 3 ++-\n> t/t0018-advice.sh | 8 ++++++++\n> 3 files changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/advice.c b/advice.c\n> index 50c79443ba..fa203f8806 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -79,6 +79,7 @@ static struct {\n> \t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\", 1 },\n> \t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\", 1 },\n> \t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\", 1 },\n> +\t[ADVICE_ADVICE_OFF]\t\t\t\t= { \"adviceOff\", 1 },\n\nThe name seems to imply that setting `advice.adviceOff=true` would cause\nGit to suppress the turn-off instructions. But...\n\n> };\n>\n> static const char turn_off_instructions[] =\n> @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,\n>\n> \tstrbuf_vaddf(&buf, advice, params);\n>\n> -\tif (display_instructions)\n> +\tif (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))\n> \t\tstrbuf_addf(&buf, turn_off_instructions, key);\n\n...it looks like the opposite is true. I guess the \"adviceOff\" part of\nthis new configuration option suggests \"show me advice on how to turn\noff advice of xyz kind in the future\".\n\nPerhaps a clearer alternative might be \"advice.showDisableInstructions\"\nor something? I don't know, coming up with a direct/clear name of this\nconfiguration is challenging for me.\n\n>\n> \tfor (cp = buf.buf; *cp; cp = np) {\n> diff --git a/advice.h b/advice.h\n> index 2affbe1426..1f2eef034e 100644\n> --- a/advice.h\n> +++ b/advice.h\n> @@ -10,7 +10,7 @@ struct string_list;\n> * Add the new config variable to Documentation/config/advice.txt.\n> * Call advise_if_enabled to print your advice.\n> */\n> - enum advice_type {\n> +enum advice_type {\n> \tADVICE_ADD_EMBEDDED_REPO,\n> \tADVICE_ADD_EMPTY_PATHSPEC,\n> \tADVICE_ADD_IGNORED_FILE,\n> @@ -50,6 +50,7 @@ struct string_list;\n> \tADVICE_WAITING_FOR_EDITOR,\n> \tADVICE_SKIPPED_CHERRY_PICKS,\n> \tADVICE_WORKTREE_ADD_ORPHAN,\n> +\tADVICE_ADVICE_OFF,\n> };\n>\n> int git_default_advice_config(const char *var, const char *value);\n> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\n> index c13057a4ca..0b6a8b4a10 100755\n> --- a/t/t0018-advice.sh\n> +++ b/t/t0018-advice.sh\n> @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to\n> \ttest_must_be_empty actual\n> '\n>\n> +test_expect_success 'advice without the instructions to disable it' '\n> +\tcat >expect <<-\\EOF &&\n> +\thint: This is a piece of advice\n> +\tEOF\n> +\ttest-tool -c advice.adviceOff=0 advise \"This is a piece of advice\" 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n\nLooking at this test, I wonder why we don't imitate the existing style\nof:\n\n test_config advice.adviceOff false &&\n test-tool advise \"This is a piece of advice\" 2>actual &&\n test_cmp expect actual\n\ninstead of teaching the test-tool helpers how to interpret `-c`\narguments. Doing so would allow us to drop the first couple of patches\nin this series and simplify things a bit.\n\nThanks,\nTaylor\n"},{"id":"486460","messageId":"ZZ2QafUf/JxXYZU/@nand.local","threadId":"60706","inReplyTo":"7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com","subject":"Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-09T18:28:57Z","receivedAt":"2024-01-09T18:28:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 09, 2024 at 04:25:32PM +0100, Rubén Justo wrote:\n> Using advise_if_enabled() to display an advice will automatically\n> include instructions on how to disable the advice, along with the main\n> advice:\n>\n> \thint: use --reapply-cherry-picks to include skipped commits\n> \thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n>\n> This may become distracting or \"noisy\" over time, while the user may\n> still want to receive the main advice.\n>\n> Let's have a switch to allow disabling this automatic advice.\n\nI reviewed this and had a couple of notes, mostly focused on what to\ncall the new configuration option, and if we should be modifying the\ntest-tool helpers to accept arbitrary configuration via command-line\noptions.\n\nI think that we could reasonably drop the first two patches by\nimitating the existing style of t0018 more closely, but I don't feel all\nthat strongly about it.\n\nSo I would probably expect a reroll of this series, but if you disagree\nwith my comments, I would not be sad to see this series merged as-is.\n\nThanks,\nTaylor\n"},{"id":"486469","messageId":"xmqqjzoigtzg.fsf@gitster.g","threadId":"60706","inReplyTo":"ZZ2QGYBBmK8cSYBD@nand.local","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T19:57:39Z","receivedAt":"2024-01-09T19:57:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Looking at this test, I wonder why we don't imitate the existing style\n> of:\n>\n> test_config advice.adviceOff false &&\n> test-tool advise \"This is a piece of advice\" 2>actual &&\n> test_cmp expect actual\n>\n> instead of teaching the test-tool helpers how to interpret `-c`\n> arguments. Doing so would allow us to drop the first couple of patches\n> in this series and simplify things a bit.\n\nThanks for a dose of sanity. I too got a bit too excited by a shiny\nnew toy ;-) Reusing the existing mechanism does make sense.\n\n"},{"id":"486476","messageId":"xmqq8r4yf897.fsf@gitster.g","threadId":"60706","inReplyTo":"ZZ2QafUf/JxXYZU/@nand.local","subject":"Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T22:32:20Z","receivedAt":"2024-01-09T22:32:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Tue, Jan 09, 2024 at 04:25:32PM +0100, Rubén Justo wrote:\n>> Using advise_if_enabled() to display an advice will automatically\n>> include instructions on how to disable the advice, along with the main\n>> advice:\n>>\n>> \thint: use --reapply-cherry-picks to include skipped commits\n>> \thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n>>\n>> This may become distracting or \"noisy\" over time, while the user may\n>> still want to receive the main advice.\n>>\n>> Let's have a switch to allow disabling this automatic advice.\n>\n> I reviewed this and had a couple of notes, mostly focused on what to\n> call the new configuration option, and if we should be modifying the\n> test-tool helpers to accept arbitrary configuration via command-line\n> options.\n>\n> I think that we could reasonably drop the first two patches by\n> imitating the existing style of t0018 more closely, but I don't feel all\n> that strongly about it.\n\nEven though I do not have a fundamental objection against test-tool\nlearning \"-c key=val\", I do not see a strong need for the first two\nsteps, either.\n\nI actually have more problems with the primary change of this series\n(in addition to that configuration knob is probably misnamed, as you\npointed out). How to disable the advice is an integral part of the\nend-user experience about the conditional advice system. The idea\nis to show it repeatedly, perhaps loud enough to be called \"noisy\",\nso that the user learns to follow the suggestion given by the hint.\nUntil then, the user is expected to gradually learn to follow the\nsuggestion more and more, seeing the advice message less and less\noften. If we rob the \"how to disable THIS PARTICULAR message\" and\ngave only this line:\n\n>> \thint: use --reapply-cherry-picks to include skipped commits\n\nit would become impossible to find how to disable it, once the user\nget comfortable enough to pass --reapply-cherry-picks when it is\nappropriate to do so. I do not think there is no quick way other\nthan grepping in the source to find that the user needs to disable\nthe skippedCherryPicks message (no, you can look at the output from\n\"git config --help\" and find \"skippedCherryPicks\" if you know that\nis the symbol to be found, but there is nothing to link the above\nhint message to that particular help page entry).\n\nI would understand if the proposed change were to change the\n\"advice.\" configuration variable from a Boolean to a tristate\n(yes = default, no = disabled, always = give advice without\ninstruction), though. IOW, the message might look like so:\n\n hint: use --reapply-cherry-picks to include skipped commits\n hint: setting advice.skippedCherryPicks to 'false' disables this message\n hint: and setting it to 'always' removes this additional instruction.\n\nThen, those who find the hint always useful (because the particular\nmistake the hint is given against is something the user commits very\nrarely and the convenience of being reminded when it happens, which\nis rare, outweigh the burden of learning what is suggested) may want\nto set it to 'always' to accept that new choice.\n\nBut not the way the new feature is proposed.\n\nThanks.\n"},{"id":"486516","messageId":"20240110110226.GC16674@coredump.intra.peff.net","threadId":"60706","inReplyTo":"d6099d78-43c6-4709-9121-11f84228cf91@gmail.com","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-10T11:02:26Z","receivedAt":"2024-01-10T11:02:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:\n\n> Using advise_if_enabled() to display an advice will automatically\n> include instructions on how to disable the advice, along with the\n> main advice:\n> \n> \thint: use --reapply-cherry-picks to include skipped commits\n> \thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n> \n> This can become distracting or noisy over time, while the user may\n> still want to receive the main advice.\n> \n> Let's have a switch to allow disabling this automatic advice.\n\nIf I'm reading your patch correctly, this is a single option that\ncontrols the extra line for _all_ advice messages. But I'd have expected\nthis to be something you'd want to set on a per-message basis. Here's my\nthinking.\n\nThe original idea for advice messages was that they might be verbose and\nannoying, but if you had one that showed up a lot you'd choose to shut\nit up individually. But you wouldn't do so for _all_ messages, because\nyou might benefit from seeing others (including new ones that get\nadded). The \"Disable this...\" part was added later to help you easily\nknow which config option to tweak.\n\nThe expectation was that you'd fall into one of two categories:\n\n 1. You don't see the message often enough to care, so you do nothing.\n\n 2. You do find it annoying, so you disable this instance.\n\nYour series proposes a third state:\n\n 3. You find the actual hint useful, but the verbosity of \"how to shut\n it up\" is too much for you.\n\nThat make sense to me, along with being able to partially shut-up a\nmessage. But wouldn't you still need the \"how to shut up\" hint for\n_other_ messages, since it's customized for each situation?\n\nE.g., suppose that after getting annoyed at advice.skippedCherryPicks,\nyou run \"git config advice.adviseOff false\".\n\nBut now you run into another hint, like:\n\n $ git foo\n hint: you can use --empty-commits to deal with isn't as good as --xyzzy\n\nand you want to disable it entirely. Which advice.* config option does\nso? You're stuck trying to find it in the manpage (which is tedious but\nalso kind of tricky since you're now guessing which name goes with which\nmessage). You probably do want:\n\n hint: Disable this message with \"git config advice.xyzzy false\"\n\nin that case (at which point you may decide to silence it completely or\npartially).\n\nWhich implies to me that the value of advice.* should be a tri-state to\nmatch the cases above: true, false, or a \"minimal\" / \"quiet\" mode (there\nmight be a better name). And then you'd do:\n\n git config advice.skippedCherryPicks minimal\n\n(or whatever it is called) to get the mode you want, without affecting\nadvice.xyzzy.\n\n> advice.c | 3 ++-\n> advice.h | 3 ++-\n> t/t0018-advice.sh | 8 ++++++++\n> 3 files changed, 12 insertions(+), 2 deletions(-)\n\nSpeaking of manpages, we'd presumably need an update to\nDocumentation/config/advice.txt. :)\n\n-Peff\n"},{"id":"486520","messageId":"40325650-166e-4936-b2ca-542a6b1f9936@gmail.com","threadId":"60706","inReplyTo":"20240110110226.GC16674@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-10T11:39:15Z","receivedAt":"2024-01-10T11:39:23Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 10-ene-2024 06:02:26, Jeff King wrote:\n> On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:\n> \n> > Using advise_if_enabled() to display an advice will automatically\n> > include instructions on how to disable the advice, along with the\n> > main advice:\n> > \n> > \thint: use --reapply-cherry-picks to include skipped commits\n> > \thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n> > \n> > This can become distracting or noisy over time, while the user may\n> > still want to receive the main advice.\n> > \n> > Let's have a switch to allow disabling this automatic advice.\n> \n> If I'm reading your patch correctly, this is a single option that\n> controls the extra line for _all_ advice messages. But I'd have expected\n> this to be something you'd want to set on a per-message basis. Here's my\n> thinking.\n> \n> The original idea for advice messages was that they might be verbose and\n> annoying, but if you had one that showed up a lot you'd choose to shut\n> it up individually. But you wouldn't do so for _all_ messages, because\n> you might benefit from seeing others (including new ones that get\n> added). The \"Disable this...\" part was added later to help you easily\n> know which config option to tweak.\n> \n> The expectation was that you'd fall into one of two categories:\n> \n> 1. You don't see the message often enough to care, so you do nothing.\n> \n> 2. You do find it annoying, so you disable this instance.\n> \n> Your series proposes a third state:\n> \n> 3. You find the actual hint useful, but the verbosity of \"how to shut\n> it up\" is too much for you.\n> \n> That make sense to me, along with being able to partially shut-up a\n> message. But wouldn't you still need the \"how to shut up\" hint for\n> _other_ messages, since it's customized for each situation?\n> \n> E.g., suppose that after getting annoyed at advice.skippedCherryPicks,\n> you run \"git config advice.adviseOff false\".\n> \n> But now you run into another hint, like:\n> \n> $ git foo\n> hint: you can use --empty-commits to deal with isn't as good as --xyzzy\n> \n> and you want to disable it entirely. Which advice.* config option does\n> so? You're stuck trying to find it in the manpage (which is tedious but\n> also kind of tricky since you're now guessing which name goes with which\n> message). You probably do want:\n> \n> hint: Disable this message with \"git config advice.xyzzy false\"\n> \n> in that case (at which point you may decide to silence it completely or\n> partially).\n> \n> Which implies to me that the value of advice.* should be a tri-state to\n> match the cases above: true, false, or a \"minimal\" / \"quiet\" mode (there\n> might be a better name). And then you'd do:\n> \n> git config advice.skippedCherryPicks minimal\n> \n> (or whatever it is called) to get the mode you want, without affecting\n> advice.xyzzy.\n\nYour reasoning is sensible, but I'm not sure if we want that level of\ngranularity. My guts doesn't feel it, though I'm not opposed.\n\nIn the message before yours in this thread, Junio proposed a similar\napproach, and I've been thinking about it. Let me answer to your\ncomments from that message too.\n\n> \n> > advice.c | 3 ++-\n> > advice.h | 3 ++-\n> > t/t0018-advice.sh | 8 ++++++++\n> > 3 files changed, 12 insertions(+), 2 deletions(-)\n> \n> Speaking of manpages, we'd presumably need an update to\n> Documentation/config/advice.txt. :)\n\nThis has made me jump first to this message in the thread ... I missed\nthis. Thank you!\n\n> \n> -Peff\n"},{"id":"486523","messageId":"19bdce2f-a854-48a6-b865-32700971c34d@gmail.com","threadId":"60706","inReplyTo":"ZZ2QGYBBmK8cSYBD@nand.local","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-10T12:11:17Z","receivedAt":"2024-01-10T12:11:25Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 09-ene-2024 13:27:37, Taylor Blau wrote:\n\n> > +\t[ADVICE_ADVICE_OFF]\t\t\t\t= { \"adviceOff\", 1 },\n> \n> The name seems to imply that setting `advice.adviceOff=true` would cause\n> Git to suppress the turn-off instructions. But...\n> \n> > };\n> >\n> > static const char turn_off_instructions[] =\n> > @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,\n> >\n> > \tstrbuf_vaddf(&buf, advice, params);\n> >\n> > -\tif (display_instructions)\n> > +\tif (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))\n> > \t\tstrbuf_addf(&buf, turn_off_instructions, key);\n> \n> ...it looks like the opposite is true. I guess the \"adviceOff\" part of\n> this new configuration option suggests \"show me advice on how to turn\n> off advice of xyz kind in the future\".\n> \n> Perhaps a clearer alternative might be \"advice.showDisableInstructions\"\n> or something?\n\nYeah. We can then use ADVICE_SHOW_DISABLE_INSTRUCTIONS, which is also a\nbetter name than the current ADVICE_ADVICE_OFF. Thanks.\n\nI'll reroll also with a description of it in\nDocumentation/config/advice.txt, as Jeff has pointed out.\n\n> I don't know, coming up with a direct/clear name of this\n> configuration is challenging for me.\n\nWell ... I'm not going to show the previous names I've been considering\nfor this ;-)\n\n> \n> >\n> > \tfor (cp = buf.buf; *cp; cp = np) {\n> > diff --git a/advice.h b/advice.h\n> > index 2affbe1426..1f2eef034e 100644\n> > --- a/advice.h\n> > +++ b/advice.h\n> > @@ -10,7 +10,7 @@ struct string_list;\n> > * Add the new config variable to Documentation/config/advice.txt.\n> > * Call advise_if_enabled to print your advice.\n> > */\n> > - enum advice_type {\n> > +enum advice_type {\n> > \tADVICE_ADD_EMBEDDED_REPO,\n> > \tADVICE_ADD_EMPTY_PATHSPEC,\n> > \tADVICE_ADD_IGNORED_FILE,\n> > @@ -50,6 +50,7 @@ struct string_list;\n> > \tADVICE_WAITING_FOR_EDITOR,\n> > \tADVICE_SKIPPED_CHERRY_PICKS,\n> > \tADVICE_WORKTREE_ADD_ORPHAN,\n> > +\tADVICE_ADVICE_OFF,\n> > };\n> >\n> > int git_default_advice_config(const char *var, const char *value);\n> > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\n> > index c13057a4ca..0b6a8b4a10 100755\n> > --- a/t/t0018-advice.sh\n> > +++ b/t/t0018-advice.sh\n> > @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to\n> > \ttest_must_be_empty actual\n> > '\n> >\n> > +test_expect_success 'advice without the instructions to disable it' '\n> > +\tcat >expect <<-\\EOF &&\n> > +\thint: This is a piece of advice\n> > +\tEOF\n> > +\ttest-tool -c advice.adviceOff=0 advise \"This is a piece of advice\" 2>actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> Looking at this test, I wonder why we don't imitate the existing style\n> of:\n> \n> test_config advice.adviceOff false &&\n> test-tool advise \"This is a piece of advice\" 2>actual &&\n> test_cmp expect actual\n\nAs a reference, implicitly, that is:\n\n git config advice.adviceOff false &&\n test_when_finished \"git config --unset-all advice.adviceOff\" &&\n test-tool advise \"This is a piece of advice\" 2>actual &&\n test_cmp expect actual\n\nI think the proposed test is better and understandable enough.\n\nAs a matter of fact, when I was thinking how to write the test I was\nexpecting to have a working \"-c\" in test-tool, as if we have a (not\nexpected) \"git-advise\".\n\nSo ...\n\n> \n> instead of teaching the test-tool helpers how to interpret `-c`\n> arguments. Doing so would allow us to drop the first couple of patches\n> in this series and simplify things a bit.\n\n... IMHO the first two patches allows us to write simpler and more\nintuitive tests based on test-tool.\n\nI hope this argument persuades you, but I am not against your proposal.\n\n> \n> Thanks,\n> Taylor\n\nThank you for reviewing this series.\n"},{"id":"486524","messageId":"a3f7b606-afef-4cb5-a97c-caf07d3c0d4f@gmail.com","threadId":"60706","inReplyTo":"xmqq8r4yf897.fsf@gitster.g","subject":"Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-10T12:40:01Z","receivedAt":"2024-01-10T12:40:09Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 09-ene-2024 14:32:20, Junio C Hamano wrote:\n> Taylor Blau writes:\n\n> that configuration knob is probably misnamed, as you\n> pointed out).\n\nAgree. Maybe we have a better name: \"showDisableInstructions\".\n\n> I would understand if the proposed change were to change the\n> \"advice.\" configuration variable from a Boolean to a tristate\n> (yes = default, no = disabled, always = give advice without\n> instruction), though. IOW, the message might look like so:\n> \n> hint: use --reapply-cherry-picks to include skipped commits\n> hint: setting advice.skippedCherryPicks to 'false' disables this message\n> hint: and setting it to 'always' removes this additional instruction.\n\nThat's an interesting twist ... and intuitive it seems, as Peff also\ncame up with a similar approach.\n\nBut do we need, or want, that fine grain?\n\nUsing advise_if_enabled() allows us to display a hint while\nautomatically adding the option to disable it, _explicitly_ (so far*),\nto the user.\n\nBut it happens that advise_if_enabled() itself has a hint to give.\n\nMy goal in this series is about giving the user the possibility to\ndisable _that_ hint (in the hint).\n\nThe user choosing to disable that hint is telling us: \"I know I can\ndisable a hint, stop telling me so, please\". So I don't think\nthis opens up a new risk or problem finding how to disable a hint.\n\n $ git -c advice.showDisableInstructions=1 command_with_a_no_longer_welcomed_hint\n\nIs there a need to make that \"hint in the hint\" customizable _per hint_?\n\nThat probably means that a new \"make-it-always-for-all\" may be desirable\nalongside the new tristate yes-no-always ...\n\nI dunno.\n\n\n * perhaps this multi-value possibility could be a path\n to explore what Taylor outlined in another message in this thread:\n the possibility of a \"one-shot\" hint.\n"},{"id":"486527","messageId":"aaf59fc82ef3132ece8e1f70623570a2@manjaro.org","threadId":"60706","inReplyTo":"20240110110226.GC16674@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-10T14:18:17Z","receivedAt":"2024-01-10T14:18:19Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-01-10 12:02, Jeff King wrote:\n> On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:\n> \n>> Using advise_if_enabled() to display an advice will automatically\n>> include instructions on how to disable the advice, along with the\n>> main advice:\n>> \n>> \thint: use --reapply-cherry-picks to include skipped commits\n>> \thint: Disable this message with \"git config advice.skippedCherryPicks \n>> false\"\n>> \n>> This can become distracting or noisy over time, while the user may\n>> still want to receive the main advice.\n>> \n>> Let's have a switch to allow disabling this automatic advice.\n> \n> If I'm reading your patch correctly, this is a single option that\n> controls the extra line for _all_ advice messages. But I'd have \n> expected\n> this to be something you'd want to set on a per-message basis. Here's \n> my\n> thinking.\n> \n> The original idea for advice messages was that they might be verbose \n> and\n> annoying, but if you had one that showed up a lot you'd choose to shut\n> it up individually. But you wouldn't do so for _all_ messages, because\n> you might benefit from seeing others (including new ones that get\n> added). The \"Disable this...\" part was added later to help you easily\n> know which config option to tweak.\n\nJust to chime in and support this behavior of the advice messages. \nBasically, you don't want to have them all disabled at the same time, \nbut to have per-message enable/disable granularity. I'd guess that some \nof the messages are quite usable even to highly experienced users, and \nencountering newly added messages is also very useful. Thus, having \nthem all disabled wouldn't be the best idea.\n\n> The expectation was that you'd fall into one of two categories:\n> \n> 1. You don't see the message often enough to care, so you do nothing.\n> \n> 2. You do find it annoying, so you disable this instance.\n> \n> Your series proposes a third state:\n> \n> 3. You find the actual hint useful, but the verbosity of \"how to shut\n> it up\" is too much for you.\n> \n> That make sense to me, along with being able to partially shut-up a\n> message. But wouldn't you still need the \"how to shut up\" hint for\n> _other_ messages, since it's customized for each situation?\n> \n> E.g., suppose that after getting annoyed at advice.skippedCherryPicks,\n> you run \"git config advice.adviseOff false\".\n> \n> But now you run into another hint, like:\n> \n> $ git foo\n> hint: you can use --empty-commits to deal with isn't as good as \n> --xyzzy\n> \n> and you want to disable it entirely. Which advice.* config option does\n> so? You're stuck trying to find it in the manpage (which is tedious but\n> also kind of tricky since you're now guessing which name goes with \n> which\n> message). You probably do want:\n> \n> hint: Disable this message with \"git config advice.xyzzy false\"\n> \n> in that case (at which point you may decide to silence it completely or\n> partially).\n> \n> Which implies to me that the value of advice.* should be a tri-state to\n> match the cases above: true, false, or a \"minimal\" / \"quiet\" mode \n> (there\n> might be a better name). And then you'd do:\n> \n> git config advice.skippedCherryPicks minimal\n> \n> (or whatever it is called) to get the mode you want, without affecting\n> advice.xyzzy.\n> \n>> advice.c | 3 ++-\n>> advice.h | 3 ++-\n>> t/t0018-advice.sh | 8 ++++++++\n>> 3 files changed, 12 insertions(+), 2 deletions(-)\n> \n> Speaking of manpages, we'd presumably need an update to\n> Documentation/config/advice.txt. :)\n> \n> -Peff\n"},{"id":"486529","messageId":"97fdf6d4-1403-4fe9-b912-a85342a9fa56@gmail.com","threadId":"60706","inReplyTo":"aaf59fc82ef3132ece8e1f70623570a2@manjaro.org","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-10T14:32:28Z","receivedAt":"2024-01-10T14:32:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 10-ene-2024 15:18:17, Dragan Simic wrote:\n> On 2024-01-10 12:02, Jeff King wrote:\n\n> Just to chime in and support this behavior of the advice messages.\n> Basically, you don't want to have them all disabled at the same time, but to\n> have per-message enable/disable granularity. I'd guess that some of the\n> messages are quite usable even to highly experienced users, and encountering\n> newly added messages is also very useful. Thus, having them all disabled\n> wouldn't be the best idea.\n\nTotally agree.\n\nThis series is about disabling _the part in the advice about how to\ndisable the advice_, but not the proper advice.\n\nMaybe the name ADVICE_ADVICE_OFF has caused confusion. Sorry if so.\n"},{"id":"486531","messageId":"a97b03a305a7b8b95341b63af1de0271@manjaro.org","threadId":"60706","inReplyTo":"97fdf6d4-1403-4fe9-b912-a85342a9fa56@gmail.com","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-10T14:44:41Z","receivedAt":"2024-01-10T14:44:43Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-01-10 15:32, Rubén Justo wrote:\n> On 10-ene-2024 15:18:17, Dragan Simic wrote:\n>> On 2024-01-10 12:02, Jeff King wrote:\n> \n>> Just to chime in and support this behavior of the advice messages.\n>> Basically, you don't want to have them all disabled at the same time, \n>> but to\n>> have per-message enable/disable granularity. I'd guess that some of \n>> the\n>> messages are quite usable even to highly experienced users, and \n>> encountering\n>> newly added messages is also very useful. Thus, having them all \n>> disabled\n>> wouldn't be the best idea.\n> \n> Totally agree.\n> \n> This series is about disabling _the part in the advice about how to\n> disable the advice_, but not the proper advice.\n> \n> Maybe the name ADVICE_ADVICE_OFF has caused confusion. Sorry if so.\n\nNo worries. Regarding disabling the advices for disabling the advice \nmessages, maybe it would be better to have only one configuration knob \nfor that purpose, e.g. \"core.verboseAdvice\", as a boolean knob. That \nway, fishing for the right knob for some advice message wouldn't turn \nitself into an issue, and the users would be able to turn the additional \n\"advices about advices\" on and off easily, to be able to see what knobs \nactually turn off the advice messages that have become annoying enough \nto them.\n"},{"id":"486534","messageId":"xmqqo7dtdv3q.fsf@gitster.g","threadId":"60706","inReplyTo":"20240110110226.GC16674@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T16:14:01Z","receivedAt":"2024-01-10T16:14:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If I'm reading your patch correctly, this is a single option that\n> controls the extra line for _all_ advice messages. But I'd have expected\n> this to be something you'd want to set on a per-message basis. Here's my\n> thinking.\n>\n> The original idea for advice messages was that they might be verbose and\n> annoying, but if you had one that showed up a lot you'd choose to shut\n> it up individually. But you wouldn't do so for _all_ messages, because\n> you might benefit from seeing others (including new ones that get\n> added). The \"Disable this...\" part was added later to help you easily\n> know which config option to tweak.\n>\n> The expectation was that you'd fall into one of two categories:\n>\n> 1. You don't see the message often enough to care, so you do nothing.\n>\n> 2. You do find it annoying, so you disable this instance.\n>\n> Your series proposes a third state:\n>\n> 3. You find the actual hint useful, but the verbosity of \"how to shut\n> it up\" is too much for you.\n>\n> That make sense to me, along with being able to partially shut-up a\n> message. But wouldn't you still need the \"how to shut up\" hint for\n> _other_ messages, since it's customized for each situation?\n\nThanks for saying what I wanted to say in my one of the messages\nmuch clearly than I could. The above is exactly why I would be more\nsympathetic to \"advice.foo = (yes/no/always)\".\n"},{"id":"486535","messageId":"xmqqil41duov.fsf@gitster.g","threadId":"60706","inReplyTo":"a97b03a305a7b8b95341b63af1de0271@manjaro.org","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T16:22:56Z","receivedAt":"2024-01-10T16:22:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> No worries. Regarding disabling the advices for disabling the advice\n> messages, maybe it would be better to have only one configuration knob\n> for that purpose, e.g. \"core.verboseAdvice\", as a boolean knob.\n\nI am not sure if you understood Peff's example that illustrates why\nit is a bad thing, as ...\n\n> That\n> way, fishing for the right knob for some advice message wouldn't turn\n> itself into an issue,\n\n... this is exactly what a single core.verboseAdvice knob that\nsquelches the \"how to disable this particular advice message\" part\nfrom the message is problematic. Before you see and familialize\nyourself with all advice messages, you may see one piece of advice X\nand find it useful to keep, feeling no need to turn it off. If you\nuse that single knob to squelch the part to tell you how to turn\nadvice X off. But what happens when you hit another unrelated\nadvice Y then? Because your core.verboseAdvice is a single big red\nbutton, the message does not say which advice.* variable to tweak\nfor this particular advice message Y.\n"},{"id":"486545","messageId":"d6d72ec5431ad1ca775e6e4e9921f94c@manjaro.org","threadId":"60706","inReplyTo":"xmqqil41duov.fsf@gitster.g","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-10T17:45:49Z","receivedAt":"2024-01-10T17:45:57Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-01-10 17:22, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> No worries. Regarding disabling the advices for disabling the advice\n>> messages, maybe it would be better to have only one configuration knob\n>> for that purpose, e.g. \"core.verboseAdvice\", as a boolean knob.\n> \n> I am not sure if you understood Peff's example that illustrates why\n> it is a bad thing, as ...\n> \n>> That\n>> way, fishing for the right knob for some advice message wouldn't turn\n>> itself into an issue,\n> \n> ... this is exactly what a single core.verboseAdvice knob that\n> squelches the \"how to disable this particular advice message\" part\n> from the message is problematic. Before you see and familialize\n> yourself with all advice messages, you may see one piece of advice X\n> and find it useful to keep, feeling no need to turn it off. If you\n> use that single knob to squelch the part to tell you how to turn\n> advice X off. But what happens when you hit another unrelated\n> advice Y then? Because your core.verboseAdvice is a single big red\n> button, the message does not say which advice.* variable to tweak\n> for this particular advice message Y.\n\nMakes sense, but please allow me to explain how I envisioned the whole \nthing\nwith the single, big core.verboseAdvice configuration knob:\n\n1) You use git and find some advice useful, so you decide to keep it \ndisplayed.\n However, the additional advice about turning the advice off becomes \nannoying\n a bit, or better said, it becomes redundant because the main advice \nstays.\n\n2) As a result, you simply set core.verboseAdvice to false and voila, \nthe\n redundant additional advice disappears. Seems perfect! Of course, \nit\n isn't perfect, as the next point will clearly show.\n\n3) You keep using git, and some advice becomes no longer needed, maybe \neven\n one of the advices that you previously used to find useful, or you \nfind\n some newly added advice a bit annoying and, as a result, not needed. \nBut,\n what do you do, where's that helpful additional advice?\n\n4) As a careful git user that remembers important things, you go back to \nyour\n git configuration file and set core.verboseAdvice to true, and the \nadditional\n advices are back, telling you how to disable the unwanted advice.\n\n5) After you disable the unwanted advice, you set core.verboseAdvice \nback to\n false and keep it that way until the next redundant advice pops up.\n\nHowever, I do see that this approach has its downsides, for example the \nneed\nfor the unwanted advice to be displayed again together with the \nadditional advice,\nby executing the appropriate git commands, after the above-described \npoint #4.\n\nLet's see what it would look like with the granular, per-advice on/off \nknobs:\n\n1) You use git and find some advice useful, so you decide to keep it \ndisplayed.\n However, the additional advice about turning the advice off becomes \nannoying\n a bit, or better said, it becomes redundant because the main advice \nstays.\n\n2) As a result, you follow the additional advice and set the specific \nknob to\n false, and voila, the redundant additional advice disappears. Of \ncourse,\n it once again isn't perfect, as the next point will clearly show.\n\n3) You keep using git, and one of the advices that you previously used \nto find\n useful becomes no longer needed. But, what do you do, where's that \nhelpful\n additional advice telling you how to turn the advice off?\n\n4) Now you need to dig though the manual pages, or to re-enable the \nadditional\n advices in your git configuration file, perhaps all of them at once, \nwhile\n keeping a backup of your original settings, to restore it later. \nThen, you\n again need to wait until the original advice gets displayed.\n\n5) The additional advice is finally back, after some time passes or \nafter\n specifically reproducing the exact git commands, telling you how to \ndisable\n the unwanted advice. Of course, you follow the advice and set the \nright\n knob in your git configuration file.\n\n5) After you disable the unwanted advice, you restore the git \nconfiguration\n backup that you made earlier (you did that, right?), taking care not \nto\n override the newly made changes that disabled the unwanted advice.\n\nQuite frankly, the second approach, although more granular, seems much \nmore\ncomplicated and more error-prone to me.\n\nOf course, please let me know if I'm missing something obvious.\n"},{"id":"486607","messageId":"20240111080429.GG48154@coredump.intra.peff.net","threadId":"60706","inReplyTo":"d6d72ec5431ad1ca775e6e4e9921f94c@manjaro.org","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-11T08:04:29Z","receivedAt":"2024-01-11T08:04:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 10, 2024 at 06:45:49PM +0100, Dragan Simic wrote:\n\n> 4) As a careful git user that remembers important things, you go back\n> to your git configuration file and set core.verboseAdvice to true, and\n> the additional advices are back, telling you how to disable the\n> unwanted advice.\n> \n> 5) After you disable the unwanted advice, you set core.verboseAdvice\n> back to false and keep it that way until the next redundant advice\n> pops up.\n> \n> However, I do see that this approach has its downsides, for example\n> the need for the unwanted advice to be displayed again together with\n> the additional advice, by executing the appropriate git commands,\n> after the above-described point #4.\n\nRight, the need to re-trigger the advice is the problematic part here, I\nthink. In some cases it is easy. But in others, especially commands\nwhich mutate the repo state (like the empty-commit rebase that started\nthis thread), you may need to do a lot of work to recreate the\nsituation.\n\n> Let's see what it would look like with the granular, per-advice on/off\n> knobs:\n> \n> 1) You use git and find some advice useful, so you decide to keep it\n> displayed. However, the additional advice about turning the advice\n> off becomes annoying a bit, or better said, it becomes redundant\n> because the main advice stays.\n> \n> 2) As a result, you follow the additional advice and set the specific\n> knob to false, and voila, the redundant additional advice disappears.\n> Of course, it once again isn't perfect, as the next point will clearly\n> show.\n> \n> 3) You keep using git, and one of the advices that you previously used\n> to find useful becomes no longer needed. But, what do you do, where's\n> that helpful additional advice telling you how to turn the advice off?\n> \n> 4) Now you need to dig though the manual pages, or to re-enable the\n> additional advices in your git configuration file, perhaps all of them\n> at once, while keeping a backup of your original settings, to restore\n> it later. Then, you again need to wait until the original advice gets\n> displayed.\n\nThese steps (3) and (4) seem unlikely to me. These are by definition\nmessages you have seen before and decided to configure specifically (to\n\"always\", and not just \"off\"). So you probably only have a handful (if\neven more than one) of them in your config file.\n\nWhereas for the case I am worried about, you are exposed to a _new_\nmessage that you haven't seen before (and is maybe even new to Git!),\nfrom the much-larger pool of \"all advice messages Git knows about\".\n\nIt's possible we could implement both mechanisms and let people choose\nwhich one they want, depending on their preferences. It's not very much\ncode. I just hesitate to make things more complex than they need to be.\n\n-Peff\n"},{"id":"486702","messageId":"c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com","threadId":"60706","inReplyTo":"7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com","subject":"[PATCH] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-12T10:05:37Z","receivedAt":"2024-01-12T10:05:50Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Using advise_if_enabled() to display an advice will automatically\ninclude instructions on how to disable the advice, alongside the main\nadvice:\n\n\thint: use --reapply-cherry-picks to include skipped commits\n\thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n\nTo do so, we provide a knob which can be used to disable the advice.\n\nBut also to tell us the opposite: to show the advice.\n\nLet's not include the deactivation instructions for an advice if the\nuser explicitly sets its visibility.\n\nSigned-off-by: Rubén Justo \n---\n\nI hope this patch captures most of the comments from the previous\niteration, mostly:\n\n- Allow to disable the disabling instructions, per advice.\n- No new test is needed, therefore the first two commits from the\n previous iteration are not needed here.\n\nThanks.\n\n advice.c | 109 +++++++++++++++++++++++++---------------------\n t/t0018-advice.sh | 1 -\n 2 files changed, 59 insertions(+), 51 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex f6e4c2f302..8474966ce1 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)\n \treturn \"\";\n }\n \n+enum advice_level {\n+\tADVICE_LEVEL_DEFAULT = 0,\n+\tADVICE_LEVEL_DISABLED,\n+\tADVICE_LEVEL_ENABLED,\n+};\n+\n static struct {\n \tconst char *key;\n-\tint enabled;\n+\tenum advice_level level;\n } advice_setting[] = {\n-\t[ADVICE_ADD_EMBEDDED_REPO]\t\t\t= { \"addEmbeddedRepo\", 1 },\n-\t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n-\t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n-\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n-\t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n-\t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n-\t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n-\t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\n-\t[ADVICE_DIVERGING]\t\t\t\t= { \"diverging\", 1 },\n-\t[ADVICE_FETCH_SHOW_FORCED_UPDATES]\t\t= { \"fetchShowForcedUpdates\", 1 },\n-\t[ADVICE_FORCE_DELETE_BRANCH]\t\t\t= { \"forceDeleteBranch\", 1 },\n-\t[ADVICE_GRAFT_FILE_DEPRECATED]\t\t\t= { \"graftFileDeprecated\", 1 },\n-\t[ADVICE_IGNORED_HOOK]\t\t\t\t= { \"ignoredHook\", 1 },\n-\t[ADVICE_IMPLICIT_IDENTITY]\t\t\t= { \"implicitIdentity\", 1 },\n-\t[ADVICE_NESTED_TAG]\t\t\t\t= { \"nestedTag\", 1 },\n-\t[ADVICE_OBJECT_NAME_WARNING]\t\t\t= { \"objectNameWarning\", 1 },\n-\t[ADVICE_PUSH_ALREADY_EXISTS]\t\t\t= { \"pushAlreadyExists\", 1 },\n-\t[ADVICE_PUSH_FETCH_FIRST]\t\t\t= { \"pushFetchFirst\", 1 },\n-\t[ADVICE_PUSH_NEEDS_FORCE]\t\t\t= { \"pushNeedsForce\", 1 },\n-\t[ADVICE_PUSH_NON_FF_CURRENT]\t\t\t= { \"pushNonFFCurrent\", 1 },\n-\t[ADVICE_PUSH_NON_FF_MATCHING]\t\t\t= { \"pushNonFFMatching\", 1 },\n-\t[ADVICE_PUSH_REF_NEEDS_UPDATE]\t\t\t= { \"pushRefNeedsUpdate\", 1 },\n-\t[ADVICE_PUSH_UNQUALIFIED_REF_NAME]\t\t= { \"pushUnqualifiedRefName\", 1 },\n-\t[ADVICE_PUSH_UPDATE_REJECTED]\t\t\t= { \"pushUpdateRejected\", 1 },\n-\t[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]\t\t= { \"pushNonFastForward\", 1 }, /* backwards compatibility */\n-\t[ADVICE_RESET_NO_REFRESH_WARNING]\t\t= { \"resetNoRefresh\", 1 },\n-\t[ADVICE_RESOLVE_CONFLICT]\t\t\t= { \"resolveConflict\", 1 },\n-\t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\", 1 },\n-\t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\", 1 },\n-\t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\", 1 },\n-\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\", 1 },\n-\t[ADVICE_STATUS_AHEAD_BEHIND_WARNING]\t\t= { \"statusAheadBehindWarning\", 1 },\n-\t[ADVICE_STATUS_HINTS]\t\t\t\t= { \"statusHints\", 1 },\n-\t[ADVICE_STATUS_U_OPTION]\t\t\t= { \"statusUoption\", 1 },\n-\t[ADVICE_SUBMODULES_NOT_UPDATED] \t\t= { \"submodulesNotUpdated\", 1 },\n-\t[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { \"submoduleAlternateErrorStrategyDie\", 1 },\n-\t[ADVICE_SUGGEST_DETACHING_HEAD]\t\t\t= { \"suggestDetachingHead\", 1 },\n-\t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\", 1 },\n-\t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\", 1 },\n-\t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\", 1 },\n+\t[ADVICE_ADD_EMBEDDED_REPO]\t\t\t= { \"addEmbeddedRepo\" },\n+\t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\" },\n+\t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\" },\n+\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\" },\n+\t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\" },\n+\t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\" },\n+\t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\" },\n+\t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\" },\n+\t[ADVICE_DIVERGING]\t\t\t\t= { \"diverging\" },\n+\t[ADVICE_FETCH_SHOW_FORCED_UPDATES]\t\t= { \"fetchShowForcedUpdates\" },\n+\t[ADVICE_FORCE_DELETE_BRANCH]\t\t\t= { \"forceDeleteBranch\" },\n+\t[ADVICE_GRAFT_FILE_DEPRECATED]\t\t\t= { \"graftFileDeprecated\" },\n+\t[ADVICE_IGNORED_HOOK]\t\t\t\t= { \"ignoredHook\" },\n+\t[ADVICE_IMPLICIT_IDENTITY]\t\t\t= { \"implicitIdentity\" },\n+\t[ADVICE_NESTED_TAG]\t\t\t\t= { \"nestedTag\" },\n+\t[ADVICE_OBJECT_NAME_WARNING]\t\t\t= { \"objectNameWarning\" },\n+\t[ADVICE_PUSH_ALREADY_EXISTS]\t\t\t= { \"pushAlreadyExists\" },\n+\t[ADVICE_PUSH_FETCH_FIRST]\t\t\t= { \"pushFetchFirst\" },\n+\t[ADVICE_PUSH_NEEDS_FORCE]\t\t\t= { \"pushNeedsForce\" },\n+\t[ADVICE_PUSH_NON_FF_CURRENT]\t\t\t= { \"pushNonFFCurrent\" },\n+\t[ADVICE_PUSH_NON_FF_MATCHING]\t\t\t= { \"pushNonFFMatching\" },\n+\t[ADVICE_PUSH_REF_NEEDS_UPDATE]\t\t\t= { \"pushRefNeedsUpdate\" },\n+\t[ADVICE_PUSH_UNQUALIFIED_REF_NAME]\t\t= { \"pushUnqualifiedRefName\" },\n+\t[ADVICE_PUSH_UPDATE_REJECTED]\t\t\t= { \"pushUpdateRejected\" },\n+\t[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]\t\t= { \"pushNonFastForward\" }, /* backwards compatibility */\n+\t[ADVICE_RESET_NO_REFRESH_WARNING]\t\t= { \"resetNoRefresh\" },\n+\t[ADVICE_RESOLVE_CONFLICT]\t\t\t= { \"resolveConflict\" },\n+\t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\" },\n+\t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\" },\n+\t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\" },\n+\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\" },\n+\t[ADVICE_STATUS_AHEAD_BEHIND_WARNING]\t\t= { \"statusAheadBehindWarning\" },\n+\t[ADVICE_STATUS_HINTS]\t\t\t\t= { \"statusHints\" },\n+\t[ADVICE_STATUS_U_OPTION]\t\t\t= { \"statusUoption\" },\n+\t[ADVICE_SUBMODULES_NOT_UPDATED] \t\t= { \"submodulesNotUpdated\" },\n+\t[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { \"submoduleAlternateErrorStrategyDie\" },\n+\t[ADVICE_SUGGEST_DETACHING_HEAD]\t\t\t= { \"suggestDetachingHead\" },\n+\t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\" },\n+\t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\" },\n+\t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\" },\n };\n \n static const char turn_off_instructions[] =\n@@ -116,13 +122,13 @@ void advise(const char *advice, ...)\n \n int advice_enabled(enum advice_type type)\n {\n-\tswitch(type) {\n-\tcase ADVICE_PUSH_UPDATE_REJECTED:\n-\t\treturn advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&\n-\t\t advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;\n-\tdefault:\n-\t\treturn advice_setting[type].enabled;\n-\t}\n+\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n+\n+\tif (type == ADVICE_PUSH_UPDATE_REJECTED)\n+\t\treturn enabled &&\n+\t\t advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);\n+\n+\treturn enabled;\n }\n \n void advise_if_enabled(enum advice_type type, const char *advice, ...)\n@@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)\n \t\treturn;\n \n \tva_start(params, advice);\n-\tvadvise(advice, 1, advice_setting[type].key, params);\n+\tvadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,\n+\t\tadvice_setting[type].key, params);\n \tva_end(params);\n }\n \n@@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)\n \tfor (i = 0; i < ARRAY_SIZE(advice_setting); i++) {\n \t\tif (strcasecmp(k, advice_setting[i].key))\n \t\t\tcontinue;\n-\t\tadvice_setting[i].enabled = git_config_bool(var, value);\n+\t\tadvice_setting[i].level = git_config_bool(var, value)\n+\t\t\t\t\t ? ADVICE_LEVEL_ENABLED\n+\t\t\t\t\t : ADVICE_LEVEL_DISABLED;\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex c13057a4ca..0dcfb760a2 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '\n test_expect_success 'advice should be printed when config variable is set to true' '\n \tcat >expect <<-\\EOF &&\n \thint: This is a piece of advice\n-\thint: Disable this message with \"git config advice.nestedTag false\"\n \tEOF\n \ttest_config advice.nestedTag true &&\n \ttest-tool advise \"This is a piece of advice\" 2>actual &&\n\nbase-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d\n-- \n2.42.0\n\n"},{"id":"486743","messageId":"xmqqsf326vpn.fsf@gitster.g","threadId":"60706","inReplyTo":"c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com","subject":"Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T22:19:32Z","receivedAt":"2024-01-12T22:19:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> advice.c | 109 +++++++++++++++++++++++++---------------------\n> t/t0018-advice.sh | 1 -\n> 2 files changed, 59 insertions(+), 51 deletions(-)\n>\n> diff --git a/advice.c b/advice.c\n> index f6e4c2f302..8474966ce1 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)\n> \treturn \"\";\n> }\n> \n> +enum advice_level {\n> +\tADVICE_LEVEL_DEFAULT = 0,\n\nWe may want to say \"_NONE\" not \"_DEFAULT\" to match the convention\nused elsewhere, but I have no strong preference. I do have a\npreference to see that, when we explicitly say \"In this enum, there\nis ADVICE_LEVEL_DEFAULT and its value is zero\" with \"= 0\" in the\ndefinition, the places we use a variable of this enum type, we say\n\n\tif (!variable)\n\nand not\n\n\tif (variable == ADVICE_LEVEL_DEFAULT)\n\n> +\tADVICE_LEVEL_DISABLED,\n> +\tADVICE_LEVEL_ENABLED,\n> +};\n> +\n> static struct {\n> \tconst char *key;\n> -\tint enabled;\n> +\tenum advice_level level;\n> } advice_setting[] = {\n> ...\n> -\t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\", 1 },\n> +\t[ADVICE_ADD_EMBEDDED_REPO]\t\t\t= { \"addEmbeddedRepo\" },\n> ...\n> +\t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\" },\n> };\n\nIt certainly becomes nicer to use the \"unspecified is left to 0\"\nconvention like this.\n\n> static const char turn_off_instructions[] =\n> @@ -116,13 +122,13 @@ void advise(const char *advice, ...)\n> \n> int advice_enabled(enum advice_type type)\n> {\n> -\tswitch(type) {\n> -\tcase ADVICE_PUSH_UPDATE_REJECTED:\n> -\t\treturn advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&\n> -\t\t advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;\n> -\tdefault:\n> -\t\treturn advice_setting[type].enabled;\n> -\t}\n> +\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n\nOK.\n\n> +\tif (type == ADVICE_PUSH_UPDATE_REJECTED)\n> +\t\treturn enabled &&\n> +\t\t advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);\n\nI like the textbook use of a simple recursion here ;-)\n\n> +\treturn enabled;\n> }\n\n> void advise_if_enabled(enum advice_type type, const char *advice, ...)\n> @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)\n> \t\treturn;\n> \n> \tva_start(params, advice);\n> -\tvadvise(advice, 1, advice_setting[type].key, params);\n> +\tvadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,\n> +\t\tadvice_setting[type].key, params);\n\nOK. Once you configure this advice to be always shown, you no\nlonger are using the _DEFAULT, so we pass 0 as the second parameter.\nMakes sense.\n\nAs I said, if we explicitly document that _DEFAULT's value is zero\nwith \"= 0\" in the enum definition, which we also rely on the\ninitialization of the advice_setting[] array, then it may probably\nbe better to write it more like so:\n\n\tvadvice(advice, !advice_setting[type].level,\n\t\tadvice_setting[type].key, params);\n\nI wonder if it makes this caller simpler to update the return value\nof advice_enabled() to a tristate. Then we can do\n\n\tint enabled = advice_enabled(type);\n\n\tif (!enabled)\n\t\treturn;\n\tva_start(...);\n\tvadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...);\n\tva_end(...);\n\nbut it does not make that much difference.\n\n> \tva_end(params);\n> }\n> \n> @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)\n> \tfor (i = 0; i < ARRAY_SIZE(advice_setting); i++) {\n> \t\tif (strcasecmp(k, advice_setting[i].key))\n> \t\t\tcontinue;\n> -\t\tadvice_setting[i].enabled = git_config_bool(var, value);\n> +\t\tadvice_setting[i].level = git_config_bool(var, value)\n> +\t\t\t\t\t ? ADVICE_LEVEL_ENABLED\n> +\t\t\t\t\t : ADVICE_LEVEL_DISABLED;\n\nOK. We saw (var, value) in the configuration files we are parsing,\nso we find [i] that corresponds to the advice key and tweak the\nlevel.\n\nThis loop obviously is O(N*M) for the number of \"advice.*\"\nconfiguration variables N and the number of advice keys M, but that\nis not new to this patch---our expectation is that N will be small,\neven though M will grow over time.\n\n> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\n> index c13057a4ca..0dcfb760a2 100755\n> --- a/t/t0018-advice.sh\n> +++ b/t/t0018-advice.sh\n> @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '\n> test_expect_success 'advice should be printed when config variable is set to true' '\n> \tcat >expect <<-\\EOF &&\n> \thint: This is a piece of advice\n> -\thint: Disable this message with \"git config advice.nestedTag false\"\n> \tEOF\n> \ttest_config advice.nestedTag true &&\n> \ttest-tool advise \"This is a piece of advice\" 2>actual &&\n\nOK. The commit changes behaviour for existing users who explicitly\nsaid \"I want to see this advice\" by configuring advice.* key to 'true',\nand it probably is a good thing, even though it may be a bit surprising.\n\nOne thing that may be missing is a documentation update. Something\nalong this line to highlight that now 'true' has a bit different\nmeaning from before (and as a consequence, we can no longer say\n\"defaults to true\").\n\n Documentation/config/advice.txt | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt\nindex 2737381a11..364a8268b6 100644\n--- c/Documentation/config/advice.txt\n+++ w/Documentation/config/advice.txt\n@@ -1,7 +1,11 @@\n advice.*::\n-\tThese variables control various optional help messages designed to\n-\taid new users. All 'advice.*' variables default to 'true', and you\n-\tcan tell Git that you do not need help by setting these to 'false':\n+\n+\tThese variables control various optional help messages\n+\tdesigned to aid new users. When left unconfigured, Git will\n+\tgive you the help message with an instruction on how to\n+\tsquelch it. When set to 'true', the help message is given,\n+\tbut without hint on how to squelch the message. You can\n+\ttell Git that you do not need help by setting these to 'false':\n +\n --\n \tambiguousFetchRefspec::\n"},{"id":"486761","messageId":"20240113073828.GB657764@coredump.intra.peff.net","threadId":"60706","inReplyTo":"xmqqsf326vpn.fsf@gitster.g","subject":"Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-13T07:38:28Z","receivedAt":"2024-01-13T07:38:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2024 at 02:19:32PM -0800, Junio C Hamano wrote:\n\n> Rubén Justo writes:\n> \n> > advice.c | 109 +++++++++++++++++++++++++---------------------\n> > t/t0018-advice.sh | 1 -\n> > 2 files changed, 59 insertions(+), 51 deletions(-)\n> >\n> > diff --git a/advice.c b/advice.c\n> > index f6e4c2f302..8474966ce1 100644\n> > --- a/advice.c\n> > +++ b/advice.c\n> > @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)\n> > \treturn \"\";\n> > }\n> > \n> > +enum advice_level {\n> > +\tADVICE_LEVEL_DEFAULT = 0,\n> \n> We may want to say \"_NONE\" not \"_DEFAULT\" to match the convention\n> used elsewhere, but I have no strong preference. I do have a\n> preference to see that, when we explicitly say \"In this enum, there\n> is ADVICE_LEVEL_DEFAULT and its value is zero\" with \"= 0\" in the\n> definition, the places we use a variable of this enum type, we say\n> \n> \tif (!variable)\n> \n> and not\n> \n> \tif (variable == ADVICE_LEVEL_DEFAULT)\n\nThis may be getting into the realm of bikeshedding, but... ;)\n\nFor a tri-state we often use \"-1\" for \"not specified\". That has the nice\nside effect in this case that \"if (level)\" shows the advice (that works\nbecause \"unspecified\" and \"explicitly true\" both show the advice. And\nthen \"if (level < 0)\" is used for just the hint. But maybe that is too\nclever/fragile.\n\nOf course that means that all of the initializers have to use \"-1\"\nexplicitly. So zero-initialization is sometimes nice, too.\n\n-Peff\n"},{"id":"486804","messageId":"7cb6cae4-3de7-4c2f-8e4c-ea653b6b802f@gmail.com","threadId":"60706","inReplyTo":"xmqqsf326vpn.fsf@gitster.g","subject":"Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-15T11:24:30Z","receivedAt":"2024-01-15T11:24:43Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 12-ene-2024 14:19:32, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > advice.c | 109 +++++++++++++++++++++++++---------------------\n> > t/t0018-advice.sh | 1 -\n> > 2 files changed, 59 insertions(+), 51 deletions(-)\n> >\n> > diff --git a/advice.c b/advice.c\n> > index f6e4c2f302..8474966ce1 100644\n> > --- a/advice.c\n> > +++ b/advice.c\n> > @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)\n> > \treturn \"\";\n> > }\n> > \n> > +enum advice_level {\n> > +\tADVICE_LEVEL_DEFAULT = 0,\n> \n> We may want to say \"_NONE\" not \"_DEFAULT\" to match the convention\n> used elsewhere, but I have no strong preference.\n\nThe \"_DEFAULT\" name is rooted in the idea of having a default but\nconfigurable value. I'm not developing that idea in this series because\nI'm not sure if it's desirable. But, I'll leave a sketch here:\n\ndiff --git a/advice.c b/advice.c\nindex 8474966ce1..1bb427a8d8 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -39,6 +39,8 @@ enum advice_level {\n \tADVICE_LEVEL_ENABLED,\n };\n \n+static enum advice_level advice_level_default;\n+\n static struct {\n \tconst char *key;\n \tenum advice_level level;\n@@ -122,7 +124,9 @@ void advise(const char *advice, ...)\n \n int advice_enabled(enum advice_type type)\n {\n-\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n+\tint enabled = advice_setting[type].level\n+\t\t ? advice_setting[type].level != ADVICE_LEVEL_DISABLED\n+\t\t : advice_level_default != ADVICE_LEVEL_DISABLED;\n \n \tif (type == ADVICE_PUSH_UPDATE_REJECTED)\n \t\treturn enabled &&\n@@ -139,7 +143,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)\n \t\treturn;\n \n \tva_start(params, advice);\n-\tvadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,\n+\tvadvise(advice, !advice_setting[type].level && !advice_level_default,\n \t\tadvice_setting[type].key, params);\n \tva_end(params);\n }\n@@ -166,6 +170,13 @@ int git_default_advice_config(const char *var, const char *value)\n \tif (!skip_prefix(var, \"advice.\", &k))\n \t\treturn 0;\n \n+\tif (!strcmp(var, \"advice.default\")) {\n+\t\tadvice_level_default = git_config_bool(var, value)\n+\t\t\t\t ? ADVICE_LEVEL_ENABLED\n+\t\t\t\t : ADVICE_LEVEL_DISABLED;\n+\t\treturn 0;\n+\t}\n+\n \tfor (i = 0; i < ARRAY_SIZE(advice_setting); i++) {\n \t\tif (strcasecmp(k, advice_setting[i].key))\n \t\t\tcontinue;\n\nThe way it is now, \"_NONE\" is perfectly fine. I'll use it.\n\n> I do have a\n> preference to see that, when we explicitly say \"In this enum, there\n> is ADVICE_LEVEL_DEFAULT and its value is zero\" with \"= 0\" in the\n> definition, the places we use a variable of this enum type, we say\n> \n> \tif (!variable)\n\nOK\n\n> \n> and not\n> \n> \tif (variable == ADVICE_LEVEL_DEFAULT)\n> \n> > +\tADVICE_LEVEL_DISABLED,\n> > +\tADVICE_LEVEL_ENABLED,\n> > +};\n> > +\n> > static struct {\n> > \tconst char *key;\n> > -\tint enabled;\n> > +\tenum advice_level level;\n> > } advice_setting[] = {\n> > ...\n> > -\t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\", 1 },\n> > +\t[ADVICE_ADD_EMBEDDED_REPO]\t\t\t= { \"addEmbeddedRepo\" },\n> > ...\n> > +\t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\" },\n> > };\n> \n> It certainly becomes nicer to use the \"unspecified is left to 0\"\n> convention like this.\n\nIndeed, that's my feeling too.\n\n> \n> > static const char turn_off_instructions[] =\n> > @@ -116,13 +122,13 @@ void advise(const char *advice, ...)\n> > \n> > int advice_enabled(enum advice_type type)\n> > {\n> > -\tswitch(type) {\n> > -\tcase ADVICE_PUSH_UPDATE_REJECTED:\n> > -\t\treturn advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&\n> > -\t\t advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;\n> > -\tdefault:\n> > -\t\treturn advice_setting[type].enabled;\n> > -\t}\n> > +\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n> \n> OK.\n> \n> > +\tif (type == ADVICE_PUSH_UPDATE_REJECTED)\n> > +\t\treturn enabled &&\n> > +\t\t advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);\n> \n> I like the textbook use of a simple recursion here ;-)\n> \n> > +\treturn enabled;\n> > }\n> \n> > void advise_if_enabled(enum advice_type type, const char *advice, ...)\n> > @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)\n> > \t\treturn;\n> > \n> > \tva_start(params, advice);\n> > -\tvadvise(advice, 1, advice_setting[type].key, params);\n> > +\tvadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,\n> > +\t\tadvice_setting[type].key, params);\n> \n> OK. Once you configure this advice to be always shown, you no\n> longer are using the _DEFAULT, so we pass 0 as the second parameter.\n> Makes sense.\n> \n> As I said, if we explicitly document that _DEFAULT's value is zero\n> with \"= 0\" in the enum definition, which we also rely on the\n> initialization of the advice_setting[] array, then it may probably\n> be better to write it more like so:\n> \n> \tvadvice(advice, !advice_setting[type].level,\n> \t\tadvice_setting[type].key, params);\n\nOK\n\n> \n> I wonder if it makes this caller simpler to update the return value\n> of advice_enabled() to a tristate. Then we can do\n> \n> \tint enabled = advice_enabled(type);\n> \n> \tif (!enabled)\n> \t\treturn;\n> \tva_start(...);\n> \tvadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...);\n> \tva_end(...);\n> \n> but it does not make that much difference.\n> \n> > \tva_end(params);\n> > }\n> > \n> > @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)\n> > \tfor (i = 0; i < ARRAY_SIZE(advice_setting); i++) {\n> > \t\tif (strcasecmp(k, advice_setting[i].key))\n> > \t\t\tcontinue;\n> > -\t\tadvice_setting[i].enabled = git_config_bool(var, value);\n> > +\t\tadvice_setting[i].level = git_config_bool(var, value)\n> > +\t\t\t\t\t ? ADVICE_LEVEL_ENABLED\n> > +\t\t\t\t\t : ADVICE_LEVEL_DISABLED;\n> \n> OK. We saw (var, value) in the configuration files we are parsing,\n> so we find [i] that corresponds to the advice key and tweak the\n> level.\n> \n> This loop obviously is O(N*M) for the number of \"advice.*\"\n> configuration variables N and the number of advice keys M, but that\n> is not new to this patch---our expectation is that N will be small,\n> even though M will grow over time.\n\nI wonder if we can drop entirely this loop. Maybe a hashmap can be\nhelpful here. Of course, this is tangential to this series.\n\n> \n> > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\n> > index c13057a4ca..0dcfb760a2 100755\n> > --- a/t/t0018-advice.sh\n> > +++ b/t/t0018-advice.sh\n> > @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '\n> > test_expect_success 'advice should be printed when config variable is set to true' '\n> > \tcat >expect <<-\\EOF &&\n> > \thint: This is a piece of advice\n> > -\thint: Disable this message with \"git config advice.nestedTag false\"\n> > \tEOF\n> > \ttest_config advice.nestedTag true &&\n> > \ttest-tool advise \"This is a piece of advice\" 2>actual &&\n> \n> OK. The commit changes behaviour for existing users who explicitly\n> said \"I want to see this advice\" by configuring advice.* key to 'true',\n\nTechnically, when the user sets any value.\n\nMaybe in the future we transform the knob to have more than two states,\nbeyond yes/no. At that point, current rationality would still apply.\n\n> and it probably is a good thing, even though it may be a bit surprising.\n> \n> One thing that may be missing is a documentation update. Something\n> along this line to highlight that now 'true' has a bit different\n> meaning from before (and as a consequence, we can no longer say\n> \"defaults to true\").\n> \n> Documentation/config/advice.txt | 10 +++++++---\n> 1 file changed, 7 insertions(+), 3 deletions(-)\n> \n> diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt\n> index 2737381a11..364a8268b6 100644\n> --- c/Documentation/config/advice.txt\n> +++ w/Documentation/config/advice.txt\n> @@ -1,7 +1,11 @@\n> advice.*::\n> -\tThese variables control various optional help messages designed to\n> -\taid new users. All 'advice.*' variables default to 'true', and you\n> -\tcan tell Git that you do not need help by setting these to 'false':\n> +\n> +\tThese variables control various optional help messages\n> +\tdesigned to aid new users. When left unconfigured, Git will\n> +\tgive you the help message with an instruction on how to\n> +\tsquelch it. When set to 'true', the help message is given,\n> +\tbut without hint on how to squelch the message. You can\n> +\ttell Git that you do not need help by setting these to 'false':\n> +\n> --\n> \tambiguousFetchRefspec::\n\nOK. I'll add this.\n\nThanks.\n"},{"id":"486811","messageId":"6a842ef8-b390-4739-9eef-e867d55ed5ea@gmail.com","threadId":"60706","inReplyTo":"c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com","subject":"[PATCH v2] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-15T14:28:28Z","receivedAt":"2024-01-15T14:28:41Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Using advise_if_enabled() to display an advice will automatically\ninclude instructions on how to disable the advice, alongside the main\nadvice:\n\n\thint: use --reapply-cherry-picks to include skipped commits\n\thint: Disable this message with \"git config advice.skippedCherryPicks false\"\n\nTo do so, we provide a knob which can be used to disable the advice.\n\nBut also to tell us the opposite: to show the advice.\n\nLet's not include the deactivation instructions for an advice if the\nuser explicitly sets its visibility.\n\nSigned-off-by: Rubén Justo \n---\n\nThis must have been v3, but previous iteration was erroneously sent as a\nv1. Sorry.\n\nRange-diff against v1:\n1: d280195c7b ! 1: 0bee5d1bba advice: allow disabling the automatic hint in advise_if_enabled()\n @@ Commit message\n \n Signed-off-by: Rubén Justo \n \n + ## Documentation/config/advice.txt ##\n +@@\n + advice.*::\n + \tThese variables control various optional help messages designed to\n +-\taid new users. All 'advice.*' variables default to 'true', and you\n +-\tcan tell Git that you do not need help by setting these to 'false':\n ++\taid new users. When left unconfigured, Git will give the message\n ++\talongside instructions on how to squelch it. You can tell Git\n ++\tthat you do not need the help message by setting these to 'false':\n + +\n + --\n + \taddEmbeddedRepo::\n +\n ## advice.c ##\n @@ advice.c: static const char *advise_get_color(enum color_advice ix)\n \treturn \"\";\n }\n \n +enum advice_level {\n -+\tADVICE_LEVEL_DEFAULT = 0,\n ++\tADVICE_LEVEL_NONE = 0,\n +\tADVICE_LEVEL_DISABLED,\n +\tADVICE_LEVEL_ENABLED,\n +};\n @@ advice.c: void advise_if_enabled(enum advice_type type, const char *advice, ...)\n \n \tva_start(params, advice);\n -\tvadvise(advice, 1, advice_setting[type].key, params);\n -+\tvadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,\n -+\t\tadvice_setting[type].key, params);\n ++\tvadvise(advice, !advice_setting[type].level, advice_setting[type].key,\n ++\t\tparams);\n \tva_end(params);\n }\n \n\n Documentation/config/advice.txt | 5 +-\n advice.c | 109 +++++++++++++++++---------------\n t/t0018-advice.sh | 1 -\n 3 files changed, 62 insertions(+), 53 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 25c0917524..c7ea70f2e2 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -1,7 +1,8 @@\n advice.*::\n \tThese variables control various optional help messages designed to\n-\taid new users. All 'advice.*' variables default to 'true', and you\n-\tcan tell Git that you do not need help by setting these to 'false':\n+\taid new users. When left unconfigured, Git will give the message\n+\talongside instructions on how to squelch it. You can tell Git\n+\tthat you do not need the help message by setting these to 'false':\n +\n --\n \taddEmbeddedRepo::\ndiff --git a/advice.c b/advice.c\nindex f6e4c2f302..6e9098ff08 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)\n \treturn \"\";\n }\n \n+enum advice_level {\n+\tADVICE_LEVEL_NONE = 0,\n+\tADVICE_LEVEL_DISABLED,\n+\tADVICE_LEVEL_ENABLED,\n+};\n+\n static struct {\n \tconst char *key;\n-\tint enabled;\n+\tenum advice_level level;\n } advice_setting[] = {\n-\t[ADVICE_ADD_EMBEDDED_REPO]\t\t\t= { \"addEmbeddedRepo\", 1 },\n-\t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n-\t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n-\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n-\t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n-\t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n-\t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n-\t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\n-\t[ADVICE_DIVERGING]\t\t\t\t= { \"diverging\", 1 },\n-\t[ADVICE_FETCH_SHOW_FORCED_UPDATES]\t\t= { \"fetchShowForcedUpdates\", 1 },\n-\t[ADVICE_FORCE_DELETE_BRANCH]\t\t\t= { \"forceDeleteBranch\", 1 },\n-\t[ADVICE_GRAFT_FILE_DEPRECATED]\t\t\t= { \"graftFileDeprecated\", 1 },\n-\t[ADVICE_IGNORED_HOOK]\t\t\t\t= { \"ignoredHook\", 1 },\n-\t[ADVICE_IMPLICIT_IDENTITY]\t\t\t= { \"implicitIdentity\", 1 },\n-\t[ADVICE_NESTED_TAG]\t\t\t\t= { \"nestedTag\", 1 },\n-\t[ADVICE_OBJECT_NAME_WARNING]\t\t\t= { \"objectNameWarning\", 1 },\n-\t[ADVICE_PUSH_ALREADY_EXISTS]\t\t\t= { \"pushAlreadyExists\", 1 },\n-\t[ADVICE_PUSH_FETCH_FIRST]\t\t\t= { \"pushFetchFirst\", 1 },\n-\t[ADVICE_PUSH_NEEDS_FORCE]\t\t\t= { \"pushNeedsForce\", 1 },\n-\t[ADVICE_PUSH_NON_FF_CURRENT]\t\t\t= { \"pushNonFFCurrent\", 1 },\n-\t[ADVICE_PUSH_NON_FF_MATCHING]\t\t\t= { \"pushNonFFMatching\", 1 },\n-\t[ADVICE_PUSH_REF_NEEDS_UPDATE]\t\t\t= { \"pushRefNeedsUpdate\", 1 },\n-\t[ADVICE_PUSH_UNQUALIFIED_REF_NAME]\t\t= { \"pushUnqualifiedRefName\", 1 },\n-\t[ADVICE_PUSH_UPDATE_REJECTED]\t\t\t= { \"pushUpdateRejected\", 1 },\n-\t[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]\t\t= { \"pushNonFastForward\", 1 }, /* backwards compatibility */\n-\t[ADVICE_RESET_NO_REFRESH_WARNING]\t\t= { \"resetNoRefresh\", 1 },\n-\t[ADVICE_RESOLVE_CONFLICT]\t\t\t= { \"resolveConflict\", 1 },\n-\t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\", 1 },\n-\t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\", 1 },\n-\t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\", 1 },\n-\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\", 1 },\n-\t[ADVICE_STATUS_AHEAD_BEHIND_WARNING]\t\t= { \"statusAheadBehindWarning\", 1 },\n-\t[ADVICE_STATUS_HINTS]\t\t\t\t= { \"statusHints\", 1 },\n-\t[ADVICE_STATUS_U_OPTION]\t\t\t= { \"statusUoption\", 1 },\n-\t[ADVICE_SUBMODULES_NOT_UPDATED] \t\t= { \"submodulesNotUpdated\", 1 },\n-\t[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { \"submoduleAlternateErrorStrategyDie\", 1 },\n-\t[ADVICE_SUGGEST_DETACHING_HEAD]\t\t\t= { \"suggestDetachingHead\", 1 },\n-\t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\", 1 },\n-\t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\", 1 },\n-\t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\", 1 },\n+\t[ADVICE_ADD_EMBEDDED_REPO]\t\t\t= { \"addEmbeddedRepo\" },\n+\t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\" },\n+\t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\" },\n+\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\" },\n+\t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\" },\n+\t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\" },\n+\t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\" },\n+\t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\" },\n+\t[ADVICE_DIVERGING]\t\t\t\t= { \"diverging\" },\n+\t[ADVICE_FETCH_SHOW_FORCED_UPDATES]\t\t= { \"fetchShowForcedUpdates\" },\n+\t[ADVICE_FORCE_DELETE_BRANCH]\t\t\t= { \"forceDeleteBranch\" },\n+\t[ADVICE_GRAFT_FILE_DEPRECATED]\t\t\t= { \"graftFileDeprecated\" },\n+\t[ADVICE_IGNORED_HOOK]\t\t\t\t= { \"ignoredHook\" },\n+\t[ADVICE_IMPLICIT_IDENTITY]\t\t\t= { \"implicitIdentity\" },\n+\t[ADVICE_NESTED_TAG]\t\t\t\t= { \"nestedTag\" },\n+\t[ADVICE_OBJECT_NAME_WARNING]\t\t\t= { \"objectNameWarning\" },\n+\t[ADVICE_PUSH_ALREADY_EXISTS]\t\t\t= { \"pushAlreadyExists\" },\n+\t[ADVICE_PUSH_FETCH_FIRST]\t\t\t= { \"pushFetchFirst\" },\n+\t[ADVICE_PUSH_NEEDS_FORCE]\t\t\t= { \"pushNeedsForce\" },\n+\t[ADVICE_PUSH_NON_FF_CURRENT]\t\t\t= { \"pushNonFFCurrent\" },\n+\t[ADVICE_PUSH_NON_FF_MATCHING]\t\t\t= { \"pushNonFFMatching\" },\n+\t[ADVICE_PUSH_REF_NEEDS_UPDATE]\t\t\t= { \"pushRefNeedsUpdate\" },\n+\t[ADVICE_PUSH_UNQUALIFIED_REF_NAME]\t\t= { \"pushUnqualifiedRefName\" },\n+\t[ADVICE_PUSH_UPDATE_REJECTED]\t\t\t= { \"pushUpdateRejected\" },\n+\t[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]\t\t= { \"pushNonFastForward\" }, /* backwards compatibility */\n+\t[ADVICE_RESET_NO_REFRESH_WARNING]\t\t= { \"resetNoRefresh\" },\n+\t[ADVICE_RESOLVE_CONFLICT]\t\t\t= { \"resolveConflict\" },\n+\t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\" },\n+\t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\" },\n+\t[ADVICE_SET_UPSTREAM_FAILURE]\t\t\t= { \"setUpstreamFailure\" },\n+\t[ADVICE_SKIPPED_CHERRY_PICKS]\t\t\t= { \"skippedCherryPicks\" },\n+\t[ADVICE_STATUS_AHEAD_BEHIND_WARNING]\t\t= { \"statusAheadBehindWarning\" },\n+\t[ADVICE_STATUS_HINTS]\t\t\t\t= { \"statusHints\" },\n+\t[ADVICE_STATUS_U_OPTION]\t\t\t= { \"statusUoption\" },\n+\t[ADVICE_SUBMODULES_NOT_UPDATED] \t\t= { \"submodulesNotUpdated\" },\n+\t[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { \"submoduleAlternateErrorStrategyDie\" },\n+\t[ADVICE_SUGGEST_DETACHING_HEAD]\t\t\t= { \"suggestDetachingHead\" },\n+\t[ADVICE_UPDATE_SPARSE_PATH]\t\t\t= { \"updateSparsePath\" },\n+\t[ADVICE_WAITING_FOR_EDITOR]\t\t\t= { \"waitingForEditor\" },\n+\t[ADVICE_WORKTREE_ADD_ORPHAN]\t\t\t= { \"worktreeAddOrphan\" },\n };\n \n static const char turn_off_instructions[] =\n@@ -116,13 +122,13 @@ void advise(const char *advice, ...)\n \n int advice_enabled(enum advice_type type)\n {\n-\tswitch(type) {\n-\tcase ADVICE_PUSH_UPDATE_REJECTED:\n-\t\treturn advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&\n-\t\t advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;\n-\tdefault:\n-\t\treturn advice_setting[type].enabled;\n-\t}\n+\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n+\n+\tif (type == ADVICE_PUSH_UPDATE_REJECTED)\n+\t\treturn enabled &&\n+\t\t advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);\n+\n+\treturn enabled;\n }\n \n void advise_if_enabled(enum advice_type type, const char *advice, ...)\n@@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)\n \t\treturn;\n \n \tva_start(params, advice);\n-\tvadvise(advice, 1, advice_setting[type].key, params);\n+\tvadvise(advice, !advice_setting[type].level, advice_setting[type].key,\n+\t\tparams);\n \tva_end(params);\n }\n \n@@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)\n \tfor (i = 0; i < ARRAY_SIZE(advice_setting); i++) {\n \t\tif (strcasecmp(k, advice_setting[i].key))\n \t\t\tcontinue;\n-\t\tadvice_setting[i].enabled = git_config_bool(var, value);\n+\t\tadvice_setting[i].level = git_config_bool(var, value)\n+\t\t\t\t\t ? ADVICE_LEVEL_ENABLED\n+\t\t\t\t\t : ADVICE_LEVEL_DISABLED;\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex c13057a4ca..0dcfb760a2 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '\n test_expect_success 'advice should be printed when config variable is set to true' '\n \tcat >expect <<-\\EOF &&\n \thint: This is a piece of advice\n-\thint: Disable this message with \"git config advice.nestedTag false\"\n \tEOF\n \ttest_config advice.nestedTag true &&\n \ttest-tool advise \"This is a piece of advice\" 2>actual &&\n\nbase-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d\n-- \n2.42.0\n"},{"id":"486822","messageId":"xmqqttnd511n.fsf@gitster.g","threadId":"60706","inReplyTo":"20240113073828.GB657764@coredump.intra.peff.net","subject":"Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T04:56:20Z","receivedAt":"2024-01-16T04:56:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For a tri-state we often use \"-1\" for \"not specified\". That has the nice\n> side effect in this case that \"if (level)\" shows the advice (that works\n> because \"unspecified\" and \"explicitly true\" both show the advice. And\n> then \"if (level < 0)\" is used for just the hint. But maybe that is too\n> clever/fragile.\n>\n> Of course that means that all of the initializers have to use \"-1\"\n> explicitly. So zero-initialization is sometimes nice, too.\n\n;-) 100% agreed.\n"},{"id":"486955","messageId":"5f322b3e6053c27dda58ef1c1f025d11@manjaro.org","threadId":"60706","inReplyTo":"20240111080429.GG48154@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-18T06:15:32Z","receivedAt":"2024-01-18T06:15:35Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-01-11 09:04, Jeff King wrote:\n> On Wed, Jan 10, 2024 at 06:45:49PM +0100, Dragan Simic wrote:\n> \n>> 4) As a careful git user that remembers important things, you go back\n>> to your git configuration file and set core.verboseAdvice to true, and\n>> the additional advices are back, telling you how to disable the\n>> unwanted advice.\n>> \n>> 5) After you disable the unwanted advice, you set core.verboseAdvice\n>> back to false and keep it that way until the next redundant advice\n>> pops up.\n>> \n>> However, I do see that this approach has its downsides, for example\n>> the need for the unwanted advice to be displayed again together with\n>> the additional advice, by executing the appropriate git commands,\n>> after the above-described point #4.\n> \n> Right, the need to re-trigger the advice is the problematic part here, \n> I\n> think. In some cases it is easy. But in others, especially commands\n> which mutate the repo state (like the empty-commit rebase that started\n> this thread), you may need to do a lot of work to recreate the\n> situation.\n\nI apologize for my delayed response.\n\nYes, recreating some situations may simply require an unacceptable\namount of work and time, making it pretty much infeasible in practice.\n\n>> Let's see what it would look like with the granular, per-advice on/off\n>> knobs:\n>> \n>> 1) You use git and find some advice useful, so you decide to keep it\n>> displayed. However, the additional advice about turning the advice\n>> off becomes annoying a bit, or better said, it becomes redundant\n>> because the main advice stays.\n>> \n>> 2) As a result, you follow the additional advice and set the specific\n>> knob to false, and voila, the redundant additional advice disappears.\n>> Of course, it once again isn't perfect, as the next point will clearly\n>> show.\n>> \n>> 3) You keep using git, and one of the advices that you previously used\n>> to find useful becomes no longer needed. But, what do you do, where's\n>> that helpful additional advice telling you how to turn the advice off?\n>> \n>> 4) Now you need to dig though the manual pages, or to re-enable the\n>> additional advices in your git configuration file, perhaps all of them\n>> at once, while keeping a backup of your original settings, to restore\n>> it later. Then, you again need to wait until the original advice gets\n>> displayed.\n> \n> These steps (3) and (4) seem unlikely to me. These are by definition\n> messages you have seen before and decided to configure specifically (to\n> \"always\", and not just \"off\"). So you probably only have a handful (if\n> even more than one) of them in your config file.\n\nYes, the number of such messages shouldn't, in general, get out of hand\nover time. Though, different users may do it differently.\n\n> Whereas for the case I am worried about, you are exposed to a _new_\n> message that you haven't seen before (and is maybe even new to Git!),\n> from the much-larger pool of \"all advice messages Git knows about\".\n> \n> It's possible we could implement both mechanisms and let people choose\n> which one they want, depending on their preferences. It's not very much\n> code. I just hesitate to make things more complex than they need to be.\n\nPerhaps having both options available could be a good thing. Though,\nadding quite a few knobs may end up confusing the users, so we should\nmake sure to document it well.\n"},{"id":"486989","messageId":"xmqqil3qsdkv.fsf@gitster.g","threadId":"60706","inReplyTo":"5f322b3e6053c27dda58ef1c1f025d11@manjaro.org","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-18T18:26:08Z","receivedAt":"2024-01-18T18:26:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Perhaps having both options available could be a good thing. Though,\n> adding quite a few knobs may end up confusing the users, so we should\n> make sure to document it well.\n\nI agree there is a need for documentation, but we are not increasing\nthe number of knobs, are we?\n"},{"id":"486991","messageId":"bfded0571f133ffc1612fcbca1811a43@manjaro.org","threadId":"60706","inReplyTo":"xmqqil3qsdkv.fsf@gitster.g","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-18T18:53:34Z","receivedAt":"2024-01-18T18:53:37Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-01-18 19:26, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> Perhaps having both options available could be a good thing. Though,\n>> adding quite a few knobs may end up confusing the users, so we should\n>> make sure to document it well.\n> \n> I agree there is a need for documentation, but we are not increasing\n> the number of knobs, are we?\n\nPerhaps there would be one more knob if we end up deciding to implement\nsupport for both approaches, as previously discussed.\n"},{"id":"486997","messageId":"xmqqwms6pf72.fsf@gitster.g","threadId":"60706","inReplyTo":"bfded0571f133ffc1612fcbca1811a43@manjaro.org","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-18T20:19:29Z","receivedAt":"2024-01-18T20:19:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> On 2024-01-18 19:26, Junio C Hamano wrote:\n>> Dragan Simic writes:\n>> \n>>> Perhaps having both options available could be a good thing. Though,\n>>> adding quite a few knobs may end up confusing the users, so we should\n>>> make sure to document it well.\n>> I agree there is a need for documentation, but we are not increasing\n>> the number of knobs, are we?\n>\n> Perhaps there would be one more knob if we end up deciding to implement\n> support for both approaches, as previously discussed.\n\nBut that would be just one knob (which I personally do not quite see\nthe need for), not \"quite a few knobs\" as you said, no?\n"},{"id":"487005","messageId":"dd82bef87fd82f7c4bf7cbff5f873141@manjaro.org","threadId":"60706","inReplyTo":"xmqqwms6pf72.fsf@gitster.g","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-18T20:50:23Z","receivedAt":"2024-01-18T20:50:25Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-01-18 21:19, Junio C Hamano wrote:\n> Dragan Simic writes:\n>> On 2024-01-18 19:26, Junio C Hamano wrote:\n>>> Dragan Simic writes:\n>>> \n>>>> Perhaps having both options available could be a good thing. \n>>>> Though,\n>>>> adding quite a few knobs may end up confusing the users, so we \n>>>> should\n>>>> make sure to document it well.\n>>> \n>>> I agree there is a need for documentation, but we are not increasing\n>>> the number of knobs, are we?\n>> \n>> Perhaps there would be one more knob if we end up deciding to \n>> implement\n>> support for both approaches, as previously discussed.\n> \n> But that would be just one knob (which I personally do not quite see\n> the need for), not \"quite a few knobs\" as you said, no?\n\nI'm sorry for being imprecise. Regarding the need for that additional\n\"global\" knob, I think it can't hurt. Some people might even find it\nquite useful, and the good thing is that it wouldn't make the internal\nlogic significantly more complicated.\n"},{"id":"487133","messageId":"6a27be56-9d52-46bf-8136-5f1c42379fd1@gmail.com","threadId":"60706","inReplyTo":"dd82bef87fd82f7c4bf7cbff5f873141@manjaro.org","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-20T11:31:12Z","receivedAt":"2024-01-20T11:31:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 18-ene-2024 21:50:23, Dragan Simic wrote:\n> On 2024-01-18 21:19, Junio C Hamano wrote:\n> > Dragan Simic writes:\n> > > On 2024-01-18 19:26, Junio C Hamano wrote:\n> > > > Dragan Simic writes:\n> > > > \n> > > > > Perhaps having both options available could be a good thing.\n> > > > > Though,\n> > > > > adding quite a few knobs may end up confusing the users, so\n> > > > > we should\n> > > > > make sure to document it well.\n> > > > \n> > > > I agree there is a need for documentation, but we are not increasing\n> > > > the number of knobs, are we?\n> > > \n> > > Perhaps there would be one more knob if we end up deciding to\n> > > implement\n> > > support for both approaches, as previously discussed.\n> > \n> > But that would be just one knob (which I personally do not quite see\n> > the need for), not \"quite a few knobs\" as you said, no?\n> \n> I'm sorry for being imprecise.\n\nHi Dragan\n\nMy original motivation for starting this series has been fulfilled:\nGive the user an option to tell us not to include the disabling\ninstructions alongside the advice.\n\nThe current consensus is: if the user explicitly enables visibility for\nan advice, we can stop including the instructions on how to disable its\nvisibility.\n\nAs reference, in this thread two \"global\" options have been reviewed:\n\n - To take the disabling instructions as an advice in itself and as\n such, as with the rest, we can have a knob to disable it. Affecting\n all advice messages.\n\n - A new knob to allow the user to set the default visibility for those\n advice messages without an explicit visibility set. And so we can\n take on globally what we now take on locally; if there is not an\n explicit visibility value but there is an explicit \"default\" value,\n we can stop showing the instruction on how to disable it.\n\n> Regarding the need for that additional\n> \"global\" knob, I think it can't hurt. Some people might even find it\n> quite useful\n\nI don't quite understand what \"global\" knob you are referring to. But I\nshare with you the feeling that a \"global\" knob might be useful.\n\nThe current iteration for this series looks like it will be merged to\n'next' soon. In my opinion, it is a good stop for this series, and\nmaybe we can explore that 'global' knob in a new one.\n\nThank you for your interest in making this series better.\n"},{"id":"487137","messageId":"88e5dd52ad959577fdc522b6f0f72509@manjaro.org","threadId":"60706","inReplyTo":"6a27be56-9d52-46bf-8136-5f1c42379fd1@gmail.com","subject":"Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-20T15:31:56Z","receivedAt":"2024-01-20T15:32:05Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-01-20 12:31, Rubén Justo wrote:\n> On 18-ene-2024 21:50:23, Dragan Simic wrote:\n>> On 2024-01-18 21:19, Junio C Hamano wrote:\n>> > Dragan Simic writes:\n>> > > On 2024-01-18 19:26, Junio C Hamano wrote:\n>> > > > Dragan Simic writes:\n>> > > >\n>> > > > > Perhaps having both options available could be a good thing.\n>> > > > > Though,\n>> > > > > adding quite a few knobs may end up confusing the users, so\n>> > > > > we should\n>> > > > > make sure to document it well.\n>> > > >\n>> > > > I agree there is a need for documentation, but we are not increasing\n>> > > > the number of knobs, are we?\n>> > >\n>> > > Perhaps there would be one more knob if we end up deciding to\n>> > > implement\n>> > > support for both approaches, as previously discussed.\n>> >\n>> > But that would be just one knob (which I personally do not quite see\n>> > the need for), not \"quite a few knobs\" as you said, no?\n>> \n>> I'm sorry for being imprecise.\n> \n> Hi Dragan\n\nHello Rubén!\n\n> My original motivation for starting this series has been fulfilled:\n> Give the user an option to tell us not to include the disabling\n> instructions alongside the advice.\n\nI like the whole idea, because if someone finds some hint usable and\ndecides to keep it displayed, displaying the additional hint about\ndisabling the primary hint (i.e. the advice) simply becomes redundant.\n\n> The current consensus is: if the user explicitly enables visibility for\n> an advice, we can stop including the instructions on how to disable its\n> visibility.\n\nTotally agreed, simple and effective.\n\n> As reference, in this thread two \"global\" options have been reviewed:\n> \n> - To take the disabling instructions as an advice in itself and as\n> such, as with the rest, we can have a knob to disable it. Affecting\n> all advice messages.\n> \n> - A new knob to allow the user to set the default visibility for those\n> advice messages without an explicit visibility set. And so we can\n> take on globally what we now take on locally; if there is not an\n> explicit visibility value but there is an explicit \"default\" value,\n> we can stop showing the instruction on how to disable it.\n> \n>> Regarding the need for that additional \"global\" knob, I think it can't\n>> hurt. Some people might even find it quite useful\n> \n> I don't quite understand what \"global\" knob you are referring to. But \n> I\n> share with you the feeling that a \"global\" knob might be useful.\n\nThe additional \"global knob\", at least the way I see it, would enable \nall\nadvice messages, overriding all other configuration options that may be\npresent. It would be like a \"big switch\" that makes all advices \ndisplayed,\nfor the case in which someone decides to get rid of the hint that used \nto\nbe useful to them so the advice was disabled, but the hint is no longer\nfound to be useful to them. In such cases, having no advice displayed\nwould mean that the user is unable to know easily what knob disables the\nno-longer-favored hint.\n\nThe reason for \"forcing\" all advices to be displayed would be to allow\nthe advices to be displayed without the need to \"fish\" for the right \nknob\nto be turned in the configuration file. Though, it wouldn't be perfect\nfrom the usability standpoint, because recreating the right conditions\nfor displaying some hint and the associated advice may rather easily be\npractically infeasible, as already discussed in this thread.\n\nOf course, going through the man pages to find the right knob is always\nthe best option for those who have the time, but having a \"global knob\",\nto help the users a bit, if possible, in general shouldn't hurt.\n\nI hope it all makes more sense now. Please, let me know if further\nclarification is required.\n\nAdditionally, the way I envisioned it could also be combined with the\nfirst option for the \"global knob\" that you listed above, as an \nadditional\noption for it to \"force\" the advices to be displayed, in addition to the\nability to disable all advices.\n\n> The current iteration for this series looks like it will be merged to\n> 'next' soon. In my opinion, it is a good stop for this series, and\n> maybe we can explore that 'global' knob in a new one.\n\nI agree, the \"global knob\" can be added later, if we decide so.\n\n> Thank you for your interest in making this series better.\n\nThank you for your work on the patches!\n"}]}