threads / patch / 60706

patch, 3 partsallow disabling the automatic hint in advise_if_enabled()

Subject: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()

## tl;dr

36 messages between Jan 9, 2024 and Jan 20, 2024. Diffs are folded; open one to read it.

replies: 35people: 5as markdown or json

Rubén Justo· Jan 9, 2024, 15:25 UTC · lore

Using advise_if_enabled() to display an advice will automatically include instructions on how to disable the advice, along with the main advice:

	hint: use --reapply-cherry-picks to include skipped commits
	hint: Disable this message with "git config advice.skippedCherryPicks false"

This may become distracting or "noisy" over time, while the user may still want to receive the main advice.

Let's have a switch to allow disabling this automatic advice.
Rubén Justo (3):
  t/test-tool: usage description
  t/test-tool: handle -c <name>=<value> arguments
  advice: allow disabling the automatic hint in advise_if_enabled()
 advice.c             |  3 ++-
 advice.h             |  3 ++-
 t/helper/test-tool.c | 15 +++++++++++++--
 t/t0018-advice.sh    |  8 ++++++++
 4 files changed, 25 insertions(+), 4 deletions(-)
-- 
2.42.0
Rubén Justo· Jan 9, 2024, 15:29 UTC · re: Rubén Justo · lore

[PATCH 1/3] t/test-tool: usage description

Even though this is an internal tool, let's keep the usage description correct and well organized.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/helper/test-tool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/helper/test-tool.c +2 −2
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..d9f57c20db 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -5,7 +5,7 @@
 #include "parse-options.h"
 
 static const char * const test_tool_usage[] = {
-	"test-tool [-C <directory>] <command [<arguments>...]]",
+	"test-tool [-C <directory>] <command> [<arguments>...]]",
 	NULL
 };
 
@@ -100,7 +100,7 @@ static NORETURN void die_usage(void)
 {
 	size_t i;
 
-	fprintf(stderr, "usage: test-tool <toolname> [args]\n");
+	fprintf(stderr, "usage: %s\n", test_tool_usage[0]);
 	for (i = 0; i < ARRAY_SIZE(cmds); i++)
 		fprintf(stderr, "  %s\n", cmds[i].name);
 	exit(128);

-- 
2.42.0
Junio C Hamano· Jan 9, 2024, 18:19 UTC · re: Rubén Justo · lore

Re: [PATCH 1/3] t/test-tool: usage description

> Subject: Re: [PATCH 1/3] t/test-tool: usage description

Good eyes to spot the missing close-angle-bracket. I'll add some missing verb, e.g. "fix usage string", while queuing.

I would not bother replacing the fprintf() format string in the same patch. Hits from

    $ git grep '"usage:' t/helper

indicates that far less than half (3 among 12) reuses usage_str[] for this purpose. Making these "usage:" strings come from a unified API (perhaps parse_options() family of functions have something more appropriate than ad-hoc use of fprintf()? I didn't check) might be a welcome change but that is clearly outside the scope of the mark-up fix, and I do not see touching only this one that still uses fprintf() advances toward such a goal.

t/helper/test-chmtime.c: fprintf(stderr, "usage: %s %s\n", argv[0], usage_str); t/helper/test-delta.c: fprintf(stderr, "usage: %s\n", usage_str); t/helper/test-windows-named-pipe.c: fprintf(stderr, "usage: %s %s\n", argv[0], usage_string);

t/helper/test-advise.c: die("usage: %s <advice>", argv[0]); t/helper/test-csprng.c: fprintf(stderr, "usage: %s [<size>]\n", argv[0]); t/helper/test-genrandom.c: fprintf(stderr, "usage: %s <seed_string> [<size>]\n", argv[0]); t/helper/test-genzeros.c: fprintf(stderr, "usage: %s [<count>]\n", argv[0]); t/helper/test-hash-speed.c: die("usage: test-tool hash-speed algo_name"); t/helper/test-mergesort.c: fprintf(stderr, "usage: test-tool mergesort generate <distribution> <mode> <n> <m>\n"); t/helper/test-strcmp-offset.c: die("usage: %s <string1> <string2>", argv[0]); t/helper/test-tool.c: fprintf(stderr, "usage: test-tool <toolname> [args]\n");

Show 30 quoted lines
> Even though this is an internal tool, let's keep the usage description
> correct and well organized.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  t/helper/test-tool.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 37ba996539..d9f57c20db 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -5,7 +5,7 @@
>  #include "parse-options.h"
>  
>  static const char * const test_tool_usage[] = {
> -	"test-tool [-C <directory>] <command [<arguments>...]]",
> +	"test-tool [-C <directory>] <command> [<arguments>...]]",
>  	NULL
>  };
>  
> @@ -100,7 +100,7 @@ static NORETURN void die_usage(void)
>  {
>  	size_t i;
>  
> -	fprintf(stderr, "usage: test-tool <toolname> [args]\n");
> +	fprintf(stderr, "usage: %s\n", test_tool_usage[0]);
>  	for (i = 0; i < ARRAY_SIZE(cmds); i++)
>  		fprintf(stderr, "  %s\n", cmds[i].name);
>  	exit(128);
Rubén Justo· Jan 9, 2024, 15:29 UTC · re: Rubén Justo · lore

[PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments

Soon we're going to need to pass configuration values to a command in test-tool.

Let's teach test-tool to take config values via command line arguments.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/helper/test-tool.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Show changes to t/helper/test-tool.c +12 −1
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index d9f57c20db..7eba4ec9ab 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -3,9 +3,10 @@
 #include "test-tool-utils.h"
 #include "trace2.h"
 #include "parse-options.h"
+#include "config.h"
 
 static const char * const test_tool_usage[] = {
-	"test-tool [-C <directory>] <command> [<arguments>...]]",
+	"test-tool [-C <directory>] [-c <name>=<value>] <command> [<arguments>...]",
 	NULL
 };
 
@@ -106,6 +107,13 @@ static NORETURN void die_usage(void)
 	exit(128);
 }
 
+static int parse_config_option(const struct option *opt, const char *arg,
+			       int unset)
+{
+	git_config_push_parameter(arg);
+	return 0;
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	int i;
@@ -113,6 +121,9 @@ int cmd_main(int argc, const char **argv)
 	struct option options[] = {
 		OPT_STRING('C', NULL, &working_directory, "directory",
 			   "change the working directory"),
+		OPT_CALLBACK('c', NULL, NULL, "<name>=<value>",
+			   "pass a configuration parameter to the command",
+			   parse_config_option),
 		OPT_END()
 	};
 

-- 
2.42.0
Junio C Hamano· Jan 9, 2024, 18:19 UTC · re: Rubén Justo · lore

Re: [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments

Rubén Justo <rjusto@gmail.com> writes:
Show 9 quoted lines
> Soon we're going to need to pass configuration values to a command in
> test-tool.
>
> Let's teach test-tool to take config values via command line arguments.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  t/helper/test-tool.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
Nice.
Show 40 quoted lines
>
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index d9f57c20db..7eba4ec9ab 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -3,9 +3,10 @@
>  #include "test-tool-utils.h"
>  #include "trace2.h"
>  #include "parse-options.h"
> +#include "config.h"
>  
>  static const char * const test_tool_usage[] = {
> -	"test-tool [-C <directory>] <command> [<arguments>...]]",
> +	"test-tool [-C <directory>] [-c <name>=<value>] <command> [<arguments>...]",
>  	NULL
>  };
>  
> @@ -106,6 +107,13 @@ static NORETURN void die_usage(void)
>  	exit(128);
>  }
>  
> +static int parse_config_option(const struct option *opt, const char *arg,
> +			       int unset)
> +{
> +	git_config_push_parameter(arg);
> +	return 0;
> +}
> +
>  int cmd_main(int argc, const char **argv)
>  {
>  	int i;
> @@ -113,6 +121,9 @@ int cmd_main(int argc, const char **argv)
>  	struct option options[] = {
>  		OPT_STRING('C', NULL, &working_directory, "directory",
>  			   "change the working directory"),
> +		OPT_CALLBACK('c', NULL, NULL, "<name>=<value>",
> +			   "pass a configuration parameter to the command",
> +			   parse_config_option),
>  		OPT_END()
>  	};
Taylor Blau· Jan 9, 2024, 18:20 UTC · re: Rubén Justo · lore

Re: [PATCH 2/3] t/test-tool: handle -c <name>=<value> arguments

On Tue, Jan 09, 2024 at 04:29:57PM +0100, Rubén Justo wrote:
> Soon we're going to need to pass configuration values to a command in
> test-tool.
>
> Let's teach test-tool to take config values via command line arguments.

I wasn't expecting a step like this to appear in this series. I don't have strong feelings about it, especially since test-tool helpers already understand $GIT_DIR/config when they rely on library code which implicitly reads configuration.

But it does seem odd to have test-tool invocations that intimately depend on a particular set of configuration values. At the very least, this step seems to encourage passing finely tuned configuration values to test-tool helpers, which I am not sure is a good idea.

Your patch message suggests that this will be useful in the following patch, which makes sense. But I wonder if it would be easier to avoid the test-tool entirely and call some Git command in a state that we expect to generate advice. Then we can test its output with various values of advice.adviceOff.

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  t/helper/test-tool.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
The patch itself looks reasonable, though.

Thanks, Taylor

Rubén Justo· Jan 9, 2024, 15:30 UTC · re: Rubén Justo · lore

[PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

Using advise_if_enabled() to display an advice will automatically include instructions on how to disable the advice, along with the main advice:

	hint: use --reapply-cherry-picks to include skipped commits
	hint: Disable this message with "git config advice.skippedCherryPicks false"

This can become distracting or noisy over time, while the user may still want to receive the main advice.

Let's have a switch to allow disabling this automatic advice.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 advice.c          | 3 ++-
 advice.h          | 3 ++-
 t/t0018-advice.sh | 8 ++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)
Show changes to 3 files +12 −2

advice.c, advice.h, t/t0018-advice.sh

diff --git a/advice.c b/advice.c
index 50c79443ba..fa203f8806 100644
--- a/advice.c
+++ b/advice.c
@@ -79,6 +79,7 @@ static struct {
 	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
 	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
+	[ADVICE_ADVICE_OFF]				= { "adviceOff", 1 },
 };
 
 static const char turn_off_instructions[] =
@@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,
 
 	strbuf_vaddf(&buf, advice, params);
 
-	if (display_instructions)
+	if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))
 		strbuf_addf(&buf, turn_off_instructions, key);
 
 	for (cp = buf.buf; *cp; cp = np) {
diff --git a/advice.h b/advice.h
index 2affbe1426..1f2eef034e 100644
--- a/advice.h
+++ b/advice.h
@@ -10,7 +10,7 @@ struct string_list;
  * Add the new config variable to Documentation/config/advice.txt.
  * Call advise_if_enabled to print your advice.
  */
- enum advice_type {
+enum advice_type {
 	ADVICE_ADD_EMBEDDED_REPO,
 	ADVICE_ADD_EMPTY_PATHSPEC,
 	ADVICE_ADD_IGNORED_FILE,
@@ -50,6 +50,7 @@ struct string_list;
 	ADVICE_WAITING_FOR_EDITOR,
 	ADVICE_SKIPPED_CHERRY_PICKS,
 	ADVICE_WORKTREE_ADD_ORPHAN,
+	ADVICE_ADVICE_OFF,
 };
 
 int git_default_advice_config(const char *var, const char *value);
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index c13057a4ca..0b6a8b4a10 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to
 	test_must_be_empty actual
 '
 
+test_expect_success 'advice without the instructions to disable it' '
+	cat >expect <<-\EOF &&
+	hint: This is a piece of advice
+	EOF
+	test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.42.0
Junio C Hamano· Jan 9, 2024, 18:23 UTC · re: Rubén Justo · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

Rubén Justo <rjusto@gmail.com> writes:
Show 76 quoted lines
> Using advise_if_enabled() to display an advice will automatically
> include instructions on how to disable the advice, along with the
> main advice:
>
> 	hint: use --reapply-cherry-picks to include skipped commits
> 	hint: Disable this message with "git config advice.skippedCherryPicks false"
>
> This can become distracting or noisy over time, while the user may
> still want to receive the main advice.
>
> Let's have a switch to allow disabling this automatic advice.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  advice.c          | 3 ++-
>  advice.h          | 3 ++-
>  t/t0018-advice.sh | 8 ++++++++
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index 50c79443ba..fa203f8806 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -79,6 +79,7 @@ static struct {
>  	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
>  	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
>  	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
> +	[ADVICE_ADVICE_OFF]				= { "adviceOff", 1 },
>  };
>  
>  static const char turn_off_instructions[] =
> @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,
>  
>  	strbuf_vaddf(&buf, advice, params);
>  
> -	if (display_instructions)
> +	if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))
>  		strbuf_addf(&buf, turn_off_instructions, key);
>  
>  	for (cp = buf.buf; *cp; cp = np) {
> diff --git a/advice.h b/advice.h
> index 2affbe1426..1f2eef034e 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -10,7 +10,7 @@ struct string_list;
>   * Add the new config variable to Documentation/config/advice.txt.
>   * Call advise_if_enabled to print your advice.
>   */
> - enum advice_type {
> +enum advice_type {
>  	ADVICE_ADD_EMBEDDED_REPO,
>  	ADVICE_ADD_EMPTY_PATHSPEC,
>  	ADVICE_ADD_IGNORED_FILE,
> @@ -50,6 +50,7 @@ struct string_list;
>  	ADVICE_WAITING_FOR_EDITOR,
>  	ADVICE_SKIPPED_CHERRY_PICKS,
>  	ADVICE_WORKTREE_ADD_ORPHAN,
> +	ADVICE_ADVICE_OFF,
>  };
>  
>  int git_default_advice_config(const char *var, const char *value);
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index c13057a4ca..0b6a8b4a10 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to
>  	test_must_be_empty actual
>  '
>  
> +test_expect_success 'advice without the instructions to disable it' '
> +	cat >expect <<-\EOF &&
> +	hint: This is a piece of advice
> +	EOF
> +	test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual &&
> +	test_cmp expect actual
> +'

This is testing the right thing but in a "showing off a shiny new toy" way. We want to make sure we will catch regressions in the future by testing with a bit more conditions perturbed. For example, with the new "-c var=val" mechanism, we could

  * set advice.nestedtag to off (which would disable the whole
    advice)
  * set advice.adviceoff to on (which should be the same as not
    setting it explicitly at all).

to test different combinations that we were unable to test before [2/3] invented the mechanism.

Thanks.
Taylor Blau· Jan 9, 2024, 18:27 UTC · re: Rubén Justo · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
Show 9 quoted lines
> Using advise_if_enabled() to display an advice will automatically
> include instructions on how to disable the advice, along with the
> main advice:
>
> 	hint: use --reapply-cherry-picks to include skipped commits
> 	hint: Disable this message with "git config advice.skippedCherryPicks false"
>
> This can become distracting or noisy over time, while the user may
> still want to receive the main advice.

Presumably for more trivial pieces of advice, a user may want to immediately disable those hints in the future more quickly after first receiving the advice, in which case this feature may not be as useful for them.

But for trickier pieces of advice, I agree completely with your reasoning and think that something like this makes sense.

Show 18 quoted lines
> Let's have a switch to allow disabling this automatic advice.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  advice.c          | 3 ++-
>  advice.h          | 3 ++-
>  t/t0018-advice.sh | 8 ++++++++
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index 50c79443ba..fa203f8806 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -79,6 +79,7 @@ static struct {
>  	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
>  	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
>  	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
> +	[ADVICE_ADVICE_OFF]				= { "adviceOff", 1 },

The name seems to imply that setting `advice.adviceOff=true` would cause Git to suppress the turn-off instructions. But...

Show 10 quoted lines
>  };
>
>  static const char turn_off_instructions[] =
> @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,
>
>  	strbuf_vaddf(&buf, advice, params);
>
> -	if (display_instructions)
> +	if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))
>  		strbuf_addf(&buf, turn_off_instructions, key);

...it looks like the opposite is true. I guess the "adviceOff" part of this new configuration option suggests "show me advice on how to turn off advice of xyz kind in the future".

Perhaps a clearer alternative might be "advice.showDisableInstructions" or something? I don't know, coming up with a direct/clear name of this configuration is challenging for me.

Show 38 quoted lines
>
>  	for (cp = buf.buf; *cp; cp = np) {
> diff --git a/advice.h b/advice.h
> index 2affbe1426..1f2eef034e 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -10,7 +10,7 @@ struct string_list;
>   * Add the new config variable to Documentation/config/advice.txt.
>   * Call advise_if_enabled to print your advice.
>   */
> - enum advice_type {
> +enum advice_type {
>  	ADVICE_ADD_EMBEDDED_REPO,
>  	ADVICE_ADD_EMPTY_PATHSPEC,
>  	ADVICE_ADD_IGNORED_FILE,
> @@ -50,6 +50,7 @@ struct string_list;
>  	ADVICE_WAITING_FOR_EDITOR,
>  	ADVICE_SKIPPED_CHERRY_PICKS,
>  	ADVICE_WORKTREE_ADD_ORPHAN,
> +	ADVICE_ADVICE_OFF,
>  };
>
>  int git_default_advice_config(const char *var, const char *value);
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index c13057a4ca..0b6a8b4a10 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to
>  	test_must_be_empty actual
>  '
>
> +test_expect_success 'advice without the instructions to disable it' '
> +	cat >expect <<-\EOF &&
> +	hint: This is a piece of advice
> +	EOF
> +	test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual &&
> +	test_cmp expect actual
> +'

Looking at this test, I wonder why we don't imitate the existing style of:

    test_config advice.adviceOff false &&
    test-tool advise "This is a piece of advice" 2>actual &&
    test_cmp expect actual

instead of teaching the test-tool helpers how to interpret `-c` arguments. Doing so would allow us to drop the first couple of patches in this series and simplify things a bit.

Thanks, Taylor

Junio C Hamano· Jan 9, 2024, 19:57 UTC · re: Taylor Blau · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

Taylor Blau <me@ttaylorr.com> writes:
Show 10 quoted lines
> Looking at this test, I wonder why we don't imitate the existing style
> of:
>
>     test_config advice.adviceOff false &&
>     test-tool advise "This is a piece of advice" 2>actual &&
>     test_cmp expect actual
>
> instead of teaching the test-tool helpers how to interpret `-c`
> arguments. Doing so would allow us to drop the first couple of patches
> in this series and simplify things a bit.

Thanks for a dose of sanity. I too got a bit too excited by a shiny new toy ;-) Reusing the existing mechanism does make sense.

Rubén Justo· Jan 10, 2024, 12:11 UTC · re: Taylor Blau · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 09-ene-2024 13:27:37, Taylor Blau wrote:
Show 22 quoted lines
> > +	[ADVICE_ADVICE_OFF]				= { "adviceOff", 1 },
> 
> The name seems to imply that setting `advice.adviceOff=true` would cause
> Git to suppress the turn-off instructions. But...
> 
> >  };
> >
> >  static const char turn_off_instructions[] =
> > @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,
> >
> >  	strbuf_vaddf(&buf, advice, params);
> >
> > -	if (display_instructions)
> > +	if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))
> >  		strbuf_addf(&buf, turn_off_instructions, key);
> 
> ...it looks like the opposite is true. I guess the "adviceOff" part of
> this new configuration option suggests "show me advice on how to turn
> off advice of xyz kind in the future".
> 
> Perhaps a clearer alternative might be "advice.showDisableInstructions"
> or something?

Yeah. We can then use ADVICE_SHOW_DISABLE_INSTRUCTIONS, which is also a better name than the current ADVICE_ADVICE_OFF. Thanks.

I'll reroll also with a description of it in Documentation/config/advice.txt, as Jeff has pointed out.

> I don't know, coming up with a direct/clear name of this
> configuration is challenging for me.

Well ... I'm not going to show the previous names I've been considering for this ;-)

Show 46 quoted lines
> 
> >
> >  	for (cp = buf.buf; *cp; cp = np) {
> > diff --git a/advice.h b/advice.h
> > index 2affbe1426..1f2eef034e 100644
> > --- a/advice.h
> > +++ b/advice.h
> > @@ -10,7 +10,7 @@ struct string_list;
> >   * Add the new config variable to Documentation/config/advice.txt.
> >   * Call advise_if_enabled to print your advice.
> >   */
> > - enum advice_type {
> > +enum advice_type {
> >  	ADVICE_ADD_EMBEDDED_REPO,
> >  	ADVICE_ADD_EMPTY_PATHSPEC,
> >  	ADVICE_ADD_IGNORED_FILE,
> > @@ -50,6 +50,7 @@ struct string_list;
> >  	ADVICE_WAITING_FOR_EDITOR,
> >  	ADVICE_SKIPPED_CHERRY_PICKS,
> >  	ADVICE_WORKTREE_ADD_ORPHAN,
> > +	ADVICE_ADVICE_OFF,
> >  };
> >
> >  int git_default_advice_config(const char *var, const char *value);
> > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> > index c13057a4ca..0b6a8b4a10 100755
> > --- a/t/t0018-advice.sh
> > +++ b/t/t0018-advice.sh
> > @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to
> >  	test_must_be_empty actual
> >  '
> >
> > +test_expect_success 'advice without the instructions to disable it' '
> > +	cat >expect <<-\EOF &&
> > +	hint: This is a piece of advice
> > +	EOF
> > +	test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual &&
> > +	test_cmp expect actual
> > +'
> 
> Looking at this test, I wonder why we don't imitate the existing style
> of:
> 
>     test_config advice.adviceOff false &&
>     test-tool advise "This is a piece of advice" 2>actual &&
>     test_cmp expect actual
As a reference, implicitly, that is:
    git config advice.adviceOff false &&
    test_when_finished "git config --unset-all advice.adviceOff" &&
    test-tool advise "This is a piece of advice" 2>actual &&
    test_cmp expect actual
I think the proposed test is better and understandable enough.

As a matter of fact, when I was thinking how to write the test I was expecting to have a working "-c" in test-tool, as if we have a (not expected) "git-advise".

So ...
> 
> instead of teaching the test-tool helpers how to interpret `-c`
> arguments. Doing so would allow us to drop the first couple of patches
> in this series and simplify things a bit.

... IMHO the first two patches allows us to write simpler and more intuitive tests based on test-tool.

I hope this argument persuades you, but I am not against your proposal.
> 
> Thanks,
> Taylor
Thank you for reviewing this series.
Jeff King· Jan 10, 2024, 11:02 UTC · re: Rubén Justo · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
Show 11 quoted lines
> Using advise_if_enabled() to display an advice will automatically
> include instructions on how to disable the advice, along with the
> main advice:
> 
> 	hint: use --reapply-cherry-picks to include skipped commits
> 	hint: Disable this message with "git config advice.skippedCherryPicks false"
> 
> This can become distracting or noisy over time, while the user may
> still want to receive the main advice.
> 
> Let's have a switch to allow disabling this automatic advice.

If I'm reading your patch correctly, this is a single option that controls the extra line for _all_ advice messages. But I'd have expected this to be something you'd want to set on a per-message basis. Here's my thinking.

The original idea for advice messages was that they might be verbose and annoying, but if you had one that showed up a lot you'd choose to shut it up individually. But you wouldn't do so for _all_ messages, because you might benefit from seeing others (including new ones that get added). The "Disable this..." part was added later to help you easily know which config option to tweak.

The expectation was that you'd fall into one of two categories:
  1. You don't see the message often enough to care, so you do nothing.
  2. You do find it annoying, so you disable this instance.
Your series proposes a third state:
  3. You find the actual hint useful, but the verbosity of "how to shut
     it up" is too much for you.

That make sense to me, along with being able to partially shut-up a message. But wouldn't you still need the "how to shut up" hint for _other_ messages, since it's customized for each situation?

E.g., suppose that after getting annoyed at advice.skippedCherryPicks, you run "git config advice.adviseOff false".

But now you run into another hint, like:
  $ git foo
  hint: you can use --empty-commits to deal with isn't as good as --xyzzy

and you want to disable it entirely. Which advice.* config option does so? You're stuck trying to find it in the manpage (which is tedious but also kind of tricky since you're now guessing which name goes with which message). You probably do want:

  hint: Disable this message with "git config advice.xyzzy false"

in that case (at which point you may decide to silence it completely or partially).

Which implies to me that the value of advice.* should be a tri-state to match the cases above: true, false, or a "minimal" / "quiet" mode (there might be a better name). And then you'd do:

  git config advice.skippedCherryPicks minimal

(or whatever it is called) to get the mode you want, without affecting advice.xyzzy.

>  advice.c          | 3 ++-
>  advice.h          | 3 ++-
>  t/t0018-advice.sh | 8 ++++++++
>  3 files changed, 12 insertions(+), 2 deletions(-)

Speaking of manpages, we'd presumably need an update to Documentation/config/advice.txt. :)

-Peff
Rubén Justo· Jan 10, 2024, 11:39 UTC · re: Jeff King · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 10-ene-2024 06:02:26, Jeff King wrote:
Show 67 quoted lines
> On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
> 
> > Using advise_if_enabled() to display an advice will automatically
> > include instructions on how to disable the advice, along with the
> > main advice:
> > 
> > 	hint: use --reapply-cherry-picks to include skipped commits
> > 	hint: Disable this message with "git config advice.skippedCherryPicks false"
> > 
> > This can become distracting or noisy over time, while the user may
> > still want to receive the main advice.
> > 
> > Let's have a switch to allow disabling this automatic advice.
> 
> If I'm reading your patch correctly, this is a single option that
> controls the extra line for _all_ advice messages. But I'd have expected
> this to be something you'd want to set on a per-message basis. Here's my
> thinking.
> 
> The original idea for advice messages was that they might be verbose and
> annoying, but if you had one that showed up a lot you'd choose to shut
> it up individually. But you wouldn't do so for _all_ messages, because
> you might benefit from seeing others (including new ones that get
> added). The "Disable this..." part was added later to help you easily
> know which config option to tweak.
> 
> The expectation was that you'd fall into one of two categories:
> 
>   1. You don't see the message often enough to care, so you do nothing.
> 
>   2. You do find it annoying, so you disable this instance.
> 
> Your series proposes a third state:
> 
>   3. You find the actual hint useful, but the verbosity of "how to shut
>      it up" is too much for you.
> 
> That make sense to me, along with being able to partially shut-up a
> message. But wouldn't you still need the "how to shut up" hint for
> _other_ messages, since it's customized for each situation?
> 
> E.g., suppose that after getting annoyed at advice.skippedCherryPicks,
> you run "git config advice.adviseOff false".
> 
> But now you run into another hint, like:
> 
>   $ git foo
>   hint: you can use --empty-commits to deal with isn't as good as --xyzzy
> 
> and you want to disable it entirely. Which advice.* config option does
> so? You're stuck trying to find it in the manpage (which is tedious but
> also kind of tricky since you're now guessing which name goes with which
> message). You probably do want:
> 
>   hint: Disable this message with "git config advice.xyzzy false"
> 
> in that case (at which point you may decide to silence it completely or
> partially).
> 
> Which implies to me that the value of advice.* should be a tri-state to
> match the cases above: true, false, or a "minimal" / "quiet" mode (there
> might be a better name). And then you'd do:
> 
>   git config advice.skippedCherryPicks minimal
> 
> (or whatever it is called) to get the mode you want, without affecting
> advice.xyzzy.

Your reasoning is sensible, but I'm not sure if we want that level of granularity. My guts doesn't feel it, though I'm not opposed.

In the message before yours in this thread, Junio proposed a similar approach, and I've been thinking about it. Let me answer to your comments from that message too.

Show 8 quoted lines
> 
> >  advice.c          | 3 ++-
> >  advice.h          | 3 ++-
> >  t/t0018-advice.sh | 8 ++++++++
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> Speaking of manpages, we'd presumably need an update to
> Documentation/config/advice.txt. :)

This has made me jump first to this message in the thread ... I missed this. Thank you!

> 
> -Peff
Dragan Simic· Jan 10, 2024, 14:18 UTC · re: Jeff King · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 2024-01-10 12:02, Jeff King wrote:
Show 29 quoted lines
> On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
> 
>> Using advise_if_enabled() to display an advice will automatically
>> include instructions on how to disable the advice, along with the
>> main advice:
>> 
>> 	hint: use --reapply-cherry-picks to include skipped commits
>> 	hint: Disable this message with "git config advice.skippedCherryPicks 
>> false"
>> 
>> This can become distracting or noisy over time, while the user may
>> still want to receive the main advice.
>> 
>> Let's have a switch to allow disabling this automatic advice.
> 
> If I'm reading your patch correctly, this is a single option that
> controls the extra line for _all_ advice messages. But I'd have 
> expected
> this to be something you'd want to set on a per-message basis. Here's 
> my
> thinking.
> 
> The original idea for advice messages was that they might be verbose 
> and
> annoying, but if you had one that showed up a lot you'd choose to shut
> it up individually. But you wouldn't do so for _all_ messages, because
> you might benefit from seeing others (including new ones that get
> added). The "Disable this..." part was added later to help you easily
> know which config option to tweak.

Just to chime in and support this behavior of the advice messages. Basically, you don't want to have them all disabled at the same time, but to have per-message enable/disable granularity. I'd guess that some of the messages are quite usable even to highly experienced users, and encountering newly added messages is also very useful. Thus, having them all disabled wouldn't be the best idea.

Show 54 quoted lines
> The expectation was that you'd fall into one of two categories:
> 
>   1. You don't see the message often enough to care, so you do nothing.
> 
>   2. You do find it annoying, so you disable this instance.
> 
> Your series proposes a third state:
> 
>   3. You find the actual hint useful, but the verbosity of "how to shut
>      it up" is too much for you.
> 
> That make sense to me, along with being able to partially shut-up a
> message. But wouldn't you still need the "how to shut up" hint for
> _other_ messages, since it's customized for each situation?
> 
> E.g., suppose that after getting annoyed at advice.skippedCherryPicks,
> you run "git config advice.adviseOff false".
> 
> But now you run into another hint, like:
> 
>   $ git foo
>   hint: you can use --empty-commits to deal with isn't as good as 
> --xyzzy
> 
> and you want to disable it entirely. Which advice.* config option does
> so? You're stuck trying to find it in the manpage (which is tedious but
> also kind of tricky since you're now guessing which name goes with 
> which
> message). You probably do want:
> 
>   hint: Disable this message with "git config advice.xyzzy false"
> 
> in that case (at which point you may decide to silence it completely or
> partially).
> 
> Which implies to me that the value of advice.* should be a tri-state to
> match the cases above: true, false, or a "minimal" / "quiet" mode 
> (there
> might be a better name). And then you'd do:
> 
>   git config advice.skippedCherryPicks minimal
> 
> (or whatever it is called) to get the mode you want, without affecting
> advice.xyzzy.
> 
>>  advice.c          | 3 ++-
>>  advice.h          | 3 ++-
>>  t/t0018-advice.sh | 8 ++++++++
>>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> Speaking of manpages, we'd presumably need an update to
> Documentation/config/advice.txt. :)
> 
> -Peff
Rubén Justo· Jan 10, 2024, 14:32 UTC · re: Dragan Simic · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 10-ene-2024 15:18:17, Dragan Simic wrote:
> On 2024-01-10 12:02, Jeff King wrote:
Show 6 quoted lines
> Just to chime in and support this behavior of the advice messages.
> Basically, you don't want to have them all disabled at the same time, but to
> have per-message enable/disable granularity.  I'd guess that some of the
> messages are quite usable even to highly experienced users, and encountering
> newly added messages is also very useful.  Thus, having them all disabled
> wouldn't be the best idea.
Totally agree.

This series is about disabling _the part in the advice about how to disable the advice_, but not the proper advice.

Maybe the name ADVICE_ADVICE_OFF has caused confusion.  Sorry if so.
Dragan Simic· Jan 10, 2024, 14:44 UTC · re: Rubén Justo · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 2024-01-10 15:32, Rubén Justo wrote:
Show 20 quoted lines
> On 10-ene-2024 15:18:17, Dragan Simic wrote:
>> On 2024-01-10 12:02, Jeff King wrote:
> 
>> Just to chime in and support this behavior of the advice messages.
>> Basically, you don't want to have them all disabled at the same time, 
>> but to
>> have per-message enable/disable granularity.  I'd guess that some of 
>> the
>> messages are quite usable even to highly experienced users, and 
>> encountering
>> newly added messages is also very useful.  Thus, having them all 
>> disabled
>> wouldn't be the best idea.
> 
> Totally agree.
> 
> This series is about disabling _the part in the advice about how to
> disable the advice_, but not the proper advice.
> 
> Maybe the name ADVICE_ADVICE_OFF has caused confusion.  Sorry if so.

No worries. Regarding disabling the advices for disabling the advice messages, maybe it would be better to have only one configuration knob for that purpose, e.g. "core.verboseAdvice", as a boolean knob. That way, fishing for the right knob for some advice message wouldn't turn itself into an issue, and the users would be able to turn the additional "advices about advices" on and off easily, to be able to see what knobs actually turn off the advice messages that have become annoying enough to them.

Junio C Hamano· Jan 10, 2024, 16:22 UTC · re: Dragan Simic · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

Dragan Simic <dsimic@manjaro.org> writes:
> No worries.  Regarding disabling the advices for disabling the advice
> messages, maybe it would be better to have only one configuration knob
> for that purpose, e.g. "core.verboseAdvice", as a boolean knob.

I am not sure if you understood Peff's example that illustrates why it is a bad thing, as ...

> That
> way, fishing for the right knob for some advice message wouldn't turn
> itself into an issue,

... this is exactly what a single core.verboseAdvice knob that squelches the "how to disable this particular advice message" part from the message is problematic. Before you see and familialize yourself with all advice messages, you may see one piece of advice X and find it useful to keep, feeling no need to turn it off. If you use that single knob to squelch the part to tell you how to turn advice X off. But what happens when you hit another unrelated advice Y then? Because your core.verboseAdvice is a single big red button, the message does not say which advice.* variable to tweak for this particular advice message Y.

Dragan Simic· Jan 10, 2024, 17:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 2024-01-10 17:22, Junio C Hamano wrote:
Show 23 quoted lines
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> No worries.  Regarding disabling the advices for disabling the advice
>> messages, maybe it would be better to have only one configuration knob
>> for that purpose, e.g. "core.verboseAdvice", as a boolean knob.
> 
> I am not sure if you understood Peff's example that illustrates why
> it is a bad thing, as ...
> 
>> That
>> way, fishing for the right knob for some advice message wouldn't turn
>> itself into an issue,
> 
> ... this is exactly what a single core.verboseAdvice knob that
> squelches the "how to disable this particular advice message" part
> from the message is problematic.  Before you see and familialize
> yourself with all advice messages, you may see one piece of advice X
> and find it useful to keep, feeling no need to turn it off.  If you
> use that single knob to squelch the part to tell you how to turn
> advice X off.  But what happens when you hit another unrelated
> advice Y then?  Because your core.verboseAdvice is a single big red
> button, the message does not say which advice.* variable to tweak
> for this particular advice message Y.

Makes sense, but please allow me to explain how I envisioned the whole thing with the single, big core.verboseAdvice configuration knob:

1) You use git and find some advice useful, so you decide to keep it 
displayed.
    However, the additional advice about turning the advice off becomes 
annoying
    a bit, or better said, it becomes redundant because the main advice 
stays.
2) As a result, you simply set core.verboseAdvice to false and voila, 
the
    redundant additional advice disappears.  Seems perfect!  Of course, 
it
    isn't perfect, as the next point will clearly show.
3) You keep using git, and some advice becomes no longer needed, maybe 
even
    one of the advices that you previously used to find useful, or you 
find
    some newly added advice a bit annoying and, as a result, not needed.  
But,
    what do you do, where's that helpful additional advice?
4) As a careful git user that remembers important things, you go back to 
your
    git configuration file and set core.verboseAdvice to true, and the 
additional
    advices are back, telling you how to disable the unwanted advice.
5) After you disable the unwanted advice, you set core.verboseAdvice 
back to
    false and keep it that way until the next redundant advice pops up.

However, I do see that this approach has its downsides, for example the need for the unwanted advice to be displayed again together with the additional advice, by executing the appropriate git commands, after the above-described point #4.

Let's see what it would look like with the granular, per-advice on/off knobs:

1) You use git and find some advice useful, so you decide to keep it 
displayed.
    However, the additional advice about turning the advice off becomes 
annoying
    a bit, or better said, it becomes redundant because the main advice 
stays.
2) As a result, you follow the additional advice and set the specific 
knob to
    false, and voila, the redundant additional advice disappears.  Of 
course,
    it once again isn't perfect, as the next point will clearly show.
3) You keep using git, and one of the advices that you previously used 
to find
    useful becomes no longer needed.  But, what do you do, where's that 
helpful
    additional advice telling you how to turn the advice off?
4) Now you need to dig though the manual pages, or to re-enable the 
additional
    advices in your git configuration file, perhaps all of them at once, 
while
    keeping a backup of your original settings, to restore it later.  
Then, you
    again need to wait until the original advice gets displayed.
5) The additional advice is finally back, after some time passes or 
after
    specifically reproducing the exact git commands, telling you how to 
disable
    the unwanted advice.  Of course, you follow the advice and set the 
right
    knob in your git configuration file.
5) After you disable the unwanted advice, you restore the git 
configuration
    backup that you made earlier (you did that, right?), taking care not 
to
    override the newly made changes that disabled the unwanted advice.

Quite frankly, the second approach, although more granular, seems much more complicated and more error-prone to me.

Of course, please let me know if I'm missing something obvious.
Jeff King· Jan 11, 2024, 08:04 UTC · re: Dragan Simic · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On Wed, Jan 10, 2024 at 06:45:49PM +0100, Dragan Simic wrote:
Show 13 quoted lines
> 4) As a careful git user that remembers important things, you go back
> to your git configuration file and set core.verboseAdvice to true, and
> the additional advices are back, telling you how to disable the
> unwanted advice.
> 
> 5) After you disable the unwanted advice, you set core.verboseAdvice
> back to false and keep it that way until the next redundant advice
> pops up.
> 
> However, I do see that this approach has its downsides, for example
> the need for the unwanted advice to be displayed again together with
> the additional advice, by executing the appropriate git commands,
> after the above-described point #4.

Right, the need to re-trigger the advice is the problematic part here, I think. In some cases it is easy. But in others, especially commands which mutate the repo state (like the empty-commit rebase that started this thread), you may need to do a lot of work to recreate the situation.

Show 22 quoted lines
> Let's see what it would look like with the granular, per-advice on/off
> knobs:
> 
> 1) You use git and find some advice useful, so you decide to keep it
> displayed.  However, the additional advice about turning the advice
> off becomes annoying a bit, or better said, it becomes redundant
> because the main advice stays.
> 
> 2) As a result, you follow the additional advice and set the specific
> knob to false, and voila, the redundant additional advice disappears.
> Of course, it once again isn't perfect, as the next point will clearly
> show.
> 
> 3) You keep using git, and one of the advices that you previously used
> to find useful becomes no longer needed.  But, what do you do, where's
> that helpful additional advice telling you how to turn the advice off?
> 
> 4) Now you need to dig though the manual pages, or to re-enable the
> additional advices in your git configuration file, perhaps all of them
> at once, while keeping a backup of your original settings, to restore
> it later.  Then, you again need to wait until the original advice gets
> displayed.

These steps (3) and (4) seem unlikely to me. These are by definition messages you have seen before and decided to configure specifically (to "always", and not just "off"). So you probably only have a handful (if even more than one) of them in your config file.

Whereas for the case I am worried about, you are exposed to a _new_ message that you haven't seen before (and is maybe even new to Git!), from the much-larger pool of "all advice messages Git knows about".

It's possible we could implement both mechanisms and let people choose which one they want, depending on their preferences. It's not very much code. I just hesitate to make things more complex than they need to be.

-Peff
Dragan Simic· Jan 18, 2024, 06:15 UTC · re: Jeff King · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 2024-01-11 09:04, Jeff King wrote:
Show 22 quoted lines
> On Wed, Jan 10, 2024 at 06:45:49PM +0100, Dragan Simic wrote:
> 
>> 4) As a careful git user that remembers important things, you go back
>> to your git configuration file and set core.verboseAdvice to true, and
>> the additional advices are back, telling you how to disable the
>> unwanted advice.
>> 
>> 5) After you disable the unwanted advice, you set core.verboseAdvice
>> back to false and keep it that way until the next redundant advice
>> pops up.
>> 
>> However, I do see that this approach has its downsides, for example
>> the need for the unwanted advice to be displayed again together with
>> the additional advice, by executing the appropriate git commands,
>> after the above-described point #4.
> 
> Right, the need to re-trigger the advice is the problematic part here, 
> I
> think. In some cases it is easy. But in others, especially commands
> which mutate the repo state (like the empty-commit rebase that started
> this thread), you may need to do a lot of work to recreate the
> situation.
I apologize for my delayed response.

Yes, recreating some situations may simply require an unacceptable amount of work and time, making it pretty much infeasible in practice.

Show 27 quoted lines
>> Let's see what it would look like with the granular, per-advice on/off
>> knobs:
>> 
>> 1) You use git and find some advice useful, so you decide to keep it
>> displayed.  However, the additional advice about turning the advice
>> off becomes annoying a bit, or better said, it becomes redundant
>> because the main advice stays.
>> 
>> 2) As a result, you follow the additional advice and set the specific
>> knob to false, and voila, the redundant additional advice disappears.
>> Of course, it once again isn't perfect, as the next point will clearly
>> show.
>> 
>> 3) You keep using git, and one of the advices that you previously used
>> to find useful becomes no longer needed.  But, what do you do, where's
>> that helpful additional advice telling you how to turn the advice off?
>> 
>> 4) Now you need to dig though the manual pages, or to re-enable the
>> additional advices in your git configuration file, perhaps all of them
>> at once, while keeping a backup of your original settings, to restore
>> it later.  Then, you again need to wait until the original advice gets
>> displayed.
> 
> These steps (3) and (4) seem unlikely to me. These are by definition
> messages you have seen before and decided to configure specifically (to
> "always", and not just "off"). So you probably only have a handful (if
> even more than one) of them in your config file.

Yes, the number of such messages shouldn't, in general, get out of hand over time. Though, different users may do it differently.

Show 7 quoted lines
> Whereas for the case I am worried about, you are exposed to a _new_
> message that you haven't seen before (and is maybe even new to Git!),
> from the much-larger pool of "all advice messages Git knows about".
> 
> It's possible we could implement both mechanisms and let people choose
> which one they want, depending on their preferences. It's not very much
> code. I just hesitate to make things more complex than they need to be.

Perhaps having both options available could be a good thing. Though, adding quite a few knobs may end up confusing the users, so we should make sure to document it well.

Junio C Hamano· Jan 18, 2024, 18:26 UTC · re: Dragan Simic · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

Dragan Simic <dsimic@manjaro.org> writes:
> Perhaps having both options available could be a good thing.  Though,
> adding quite a few knobs may end up confusing the users, so we should
> make sure to document it well.

I agree there is a need for documentation, but we are not increasing the number of knobs, are we?

Dragan Simic· Jan 18, 2024, 18:53 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 2024-01-18 19:26, Junio C Hamano wrote:
Show 8 quoted lines
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> Perhaps having both options available could be a good thing.  Though,
>> adding quite a few knobs may end up confusing the users, so we should
>> make sure to document it well.
> 
> I agree there is a need for documentation, but we are not increasing
> the number of knobs, are we?

Perhaps there would be one more knob if we end up deciding to implement support for both approaches, as previously discussed.

Junio C Hamano· Jan 18, 2024, 20:19 UTC · re: Dragan Simic · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

Dragan Simic <dsimic@manjaro.org> writes:
Show 11 quoted lines
> On 2024-01-18 19:26, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>> Perhaps having both options available could be a good thing.  Though,
>>> adding quite a few knobs may end up confusing the users, so we should
>>> make sure to document it well.
>> I agree there is a need for documentation, but we are not increasing
>> the number of knobs, are we?
>
> Perhaps there would be one more knob if we end up deciding to implement
> support for both approaches, as previously discussed.

But that would be just one knob (which I personally do not quite see the need for), not "quite a few knobs" as you said, no?

Dragan Simic· Jan 18, 2024, 20:50 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 2024-01-18 21:19, Junio C Hamano wrote:
Show 19 quoted lines
> Dragan Simic <dsimic@manjaro.org> writes:
>> On 2024-01-18 19:26, Junio C Hamano wrote:
>>> Dragan Simic <dsimic@manjaro.org> writes:
>>> 
>>>> Perhaps having both options available could be a good thing.  
>>>> Though,
>>>> adding quite a few knobs may end up confusing the users, so we 
>>>> should
>>>> make sure to document it well.
>>> 
>>> I agree there is a need for documentation, but we are not increasing
>>> the number of knobs, are we?
>> 
>> Perhaps there would be one more knob if we end up deciding to 
>> implement
>> support for both approaches, as previously discussed.
> 
> But that would be just one knob (which I personally do not quite see
> the need for), not "quite a few knobs" as you said, no?

I'm sorry for being imprecise. Regarding the need for that additional "global" knob, I think it can't hurt. Some people might even find it quite useful, and the good thing is that it wouldn't make the internal logic significantly more complicated.

Rubén Justo· Jan 20, 2024, 11:31 UTC · re: Dragan Simic · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 18-ene-2024 21:50:23, Dragan Simic wrote:
Show 22 quoted lines
> On 2024-01-18 21:19, Junio C Hamano wrote:
> > Dragan Simic <dsimic@manjaro.org> writes:
> > > On 2024-01-18 19:26, Junio C Hamano wrote:
> > > > Dragan Simic <dsimic@manjaro.org> writes:
> > > > 
> > > > > Perhaps having both options available could be a good thing.
> > > > > Though,
> > > > > adding quite a few knobs may end up confusing the users, so
> > > > > we should
> > > > > make sure to document it well.
> > > > 
> > > > I agree there is a need for documentation, but we are not increasing
> > > > the number of knobs, are we?
> > > 
> > > Perhaps there would be one more knob if we end up deciding to
> > > implement
> > > support for both approaches, as previously discussed.
> > 
> > But that would be just one knob (which I personally do not quite see
> > the need for), not "quite a few knobs" as you said, no?
> 
> I'm sorry for being imprecise.
Hi Dragan

My original motivation for starting this series has been fulfilled: Give the user an option to tell us not to include the disabling instructions alongside the advice.

The current consensus is: if the user explicitly enables visibility for an advice, we can stop including the instructions on how to disable its visibility.

As reference, in this thread two "global" options have been reviewed:
 - To take the disabling instructions as an advice in itself and as
   such, as with the rest, we can have a knob to disable it.  Affecting
   all advice messages.
 - A new knob to allow the user to set the default visibility for those
   advice messages without an explicit visibility set.  And so we can
   take on globally what we now take on locally;  if there is not an
   explicit visibility value but there is an explicit "default" value,
   we can stop showing the instruction on how to disable it.
> Regarding the need for that additional
> "global" knob, I think it can't hurt.  Some people might even find it
> quite useful

I don't quite understand what "global" knob you are referring to. But I share with you the feeling that a "global" knob might be useful.

The current iteration for this series looks like it will be merged to 'next' soon. In my opinion, it is a good stop for this series, and maybe we can explore that 'global' knob in a new one.

Thank you for your interest in making this series better.
Dragan Simic· Jan 20, 2024, 15:31 UTC · re: Rubén Justo · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

On 2024-01-20 12:31, Rubén Justo wrote:
Show 25 quoted lines
> On 18-ene-2024 21:50:23, Dragan Simic wrote:
>> On 2024-01-18 21:19, Junio C Hamano wrote:
>> > Dragan Simic <dsimic@manjaro.org> writes:
>> > > On 2024-01-18 19:26, Junio C Hamano wrote:
>> > > > Dragan Simic <dsimic@manjaro.org> writes:
>> > > >
>> > > > > Perhaps having both options available could be a good thing.
>> > > > > Though,
>> > > > > adding quite a few knobs may end up confusing the users, so
>> > > > > we should
>> > > > > make sure to document it well.
>> > > >
>> > > > I agree there is a need for documentation, but we are not increasing
>> > > > the number of knobs, are we?
>> > >
>> > > Perhaps there would be one more knob if we end up deciding to
>> > > implement
>> > > support for both approaches, as previously discussed.
>> >
>> > But that would be just one knob (which I personally do not quite see
>> > the need for), not "quite a few knobs" as you said, no?
>> 
>> I'm sorry for being imprecise.
> 
> Hi Dragan
Hello Rubén!
> My original motivation for starting this series has been fulfilled:
> Give the user an option to tell us not to include the disabling
> instructions alongside the advice.

I like the whole idea, because if someone finds some hint usable and decides to keep it displayed, displaying the additional hint about disabling the primary hint (i.e. the advice) simply becomes redundant.

> The current consensus is: if the user explicitly enables visibility for
> an advice, we can stop including the instructions on how to disable its
> visibility.
Totally agreed, simple and effective.
Show 18 quoted lines
> As reference, in this thread two "global" options have been reviewed:
> 
>  - To take the disabling instructions as an advice in itself and as
>    such, as with the rest, we can have a knob to disable it.  Affecting
>    all advice messages.
> 
>  - A new knob to allow the user to set the default visibility for those
>    advice messages without an explicit visibility set.  And so we can
>    take on globally what we now take on locally;  if there is not an
>    explicit visibility value but there is an explicit "default" value,
>    we can stop showing the instruction on how to disable it.
> 
>> Regarding the need for that additional "global" knob, I think it can't
>> hurt.  Some people might even find it quite useful
> 
> I don't quite understand what "global" knob you are referring to.  But 
> I
> share with you the feeling that a "global" knob might be useful.

The additional "global knob", at least the way I see it, would enable all advice messages, overriding all other configuration options that may be present. It would be like a "big switch" that makes all advices displayed, for the case in which someone decides to get rid of the hint that used to be useful to them so the advice was disabled, but the hint is no longer found to be useful to them. In such cases, having no advice displayed would mean that the user is unable to know easily what knob disables the no-longer-favored hint.

The reason for "forcing" all advices to be displayed would be to allow the advices to be displayed without the need to "fish" for the right knob to be turned in the configuration file. Though, it wouldn't be perfect from the usability standpoint, because recreating the right conditions for displaying some hint and the associated advice may rather easily be practically infeasible, as already discussed in this thread.

Of course, going through the man pages to find the right knob is always the best option for those who have the time, but having a "global knob", to help the users a bit, if possible, in general shouldn't hurt.

I hope it all makes more sense now. Please, let me know if further clarification is required.

Additionally, the way I envisioned it could also be combined with the first option for the "global knob" that you listed above, as an additional option for it to "force" the advices to be displayed, in addition to the ability to disable all advices.

> The current iteration for this series looks like it will be merged to
> 'next' soon.  In my opinion, it is a good stop for this series, and
> maybe we can explore that 'global' knob in a new one.
I agree, the "global knob" can be added later, if we decide so.
> Thank you for your interest in making this series better.
Thank you for your work on the patches!
Junio C Hamano· Jan 10, 2024, 16:14 UTC · re: Jeff King · lore

Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()

Jeff King <peff@peff.net> writes:
Show 26 quoted lines
> If I'm reading your patch correctly, this is a single option that
> controls the extra line for _all_ advice messages. But I'd have expected
> this to be something you'd want to set on a per-message basis. Here's my
> thinking.
>
> The original idea for advice messages was that they might be verbose and
> annoying, but if you had one that showed up a lot you'd choose to shut
> it up individually. But you wouldn't do so for _all_ messages, because
> you might benefit from seeing others (including new ones that get
> added). The "Disable this..." part was added later to help you easily
> know which config option to tweak.
>
> The expectation was that you'd fall into one of two categories:
>
>   1. You don't see the message often enough to care, so you do nothing.
>
>   2. You do find it annoying, so you disable this instance.
>
> Your series proposes a third state:
>
>   3. You find the actual hint useful, but the verbosity of "how to shut
>      it up" is too much for you.
>
> That make sense to me, along with being able to partially shut-up a
> message. But wouldn't you still need the "how to shut up" hint for
> _other_ messages, since it's customized for each situation?

Thanks for saying what I wanted to say in my one of the messages much clearly than I could. The above is exactly why I would be more sympathetic to "advice.foo = (yes/no/always)".

Taylor Blau· Jan 9, 2024, 18:28 UTC · re: Rubén Justo · lore

Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()

On Tue, Jan 09, 2024 at 04:25:32PM +0100, Rubén Justo wrote:
Show 11 quoted lines
> Using advise_if_enabled() to display an advice will automatically
> include instructions on how to disable the advice, along with the main
> advice:
>
> 	hint: use --reapply-cherry-picks to include skipped commits
> 	hint: Disable this message with "git config advice.skippedCherryPicks false"
>
> This may become distracting or "noisy" over time, while the user may
> still want to receive the main advice.
>
> Let's have a switch to allow disabling this automatic advice.

I reviewed this and had a couple of notes, mostly focused on what to call the new configuration option, and if we should be modifying the test-tool helpers to accept arbitrary configuration via command-line options.

I think that we could reasonably drop the first two patches by imitating the existing style of t0018 more closely, but I don't feel all that strongly about it.

So I would probably expect a reroll of this series, but if you disagree with my comments, I would not be sad to see this series merged as-is.

Thanks, Taylor

Junio C Hamano· Jan 9, 2024, 22:32 UTC · re: Taylor Blau · lore

Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()

Taylor Blau <me@ttaylorr.com> writes:
Show 21 quoted lines
> On Tue, Jan 09, 2024 at 04:25:32PM +0100, Rubén Justo wrote:
>> Using advise_if_enabled() to display an advice will automatically
>> include instructions on how to disable the advice, along with the main
>> advice:
>>
>> 	hint: use --reapply-cherry-picks to include skipped commits
>> 	hint: Disable this message with "git config advice.skippedCherryPicks false"
>>
>> This may become distracting or "noisy" over time, while the user may
>> still want to receive the main advice.
>>
>> Let's have a switch to allow disabling this automatic advice.
>
> I reviewed this and had a couple of notes, mostly focused on what to
> call the new configuration option, and if we should be modifying the
> test-tool helpers to accept arbitrary configuration via command-line
> options.
>
> I think that we could reasonably drop the first two patches by
> imitating the existing style of t0018 more closely, but I don't feel all
> that strongly about it.

Even though I do not have a fundamental objection against test-tool learning "-c key=val", I do not see a strong need for the first two steps, either.

I actually have more problems with the primary change of this series (in addition to that configuration knob is probably misnamed, as you pointed out). How to disable the advice is an integral part of the end-user experience about the conditional advice system. The idea is to show it repeatedly, perhaps loud enough to be called "noisy", so that the user learns to follow the suggestion given by the hint. Until then, the user is expected to gradually learn to follow the suggestion more and more, seeing the advice message less and less often. If we rob the "how to disable THIS PARTICULAR message" and gave only this line:

>> 	hint: use --reapply-cherry-picks to include skipped commits

it would become impossible to find how to disable it, once the user get comfortable enough to pass --reapply-cherry-picks when it is appropriate to do so. I do not think there is no quick way other than grepping in the source to find that the user needs to disable the skippedCherryPicks message (no, you can look at the output from "git config --help" and find "skippedCherryPicks" if you know that is the symbol to be found, but there is nothing to link the above hint message to that particular help page entry).

I would understand if the proposed change were to change the "advice.<key>" configuration variable from a Boolean to a tristate (yes = default, no = disabled, always = give advice without instruction), though. IOW, the message might look like so:

    hint: use --reapply-cherry-picks to include skipped commits
    hint: setting advice.skippedCherryPicks to 'false' disables this message
    hint: and setting it to 'always' removes this additional instruction.

Then, those who find the hint always useful (because the particular mistake the hint is given against is something the user commits very rarely and the convenience of being reminded when it happens, which is rare, outweigh the burden of learning what is suggested) may want to set it to 'always' to accept that new choice.

But not the way the new feature is proposed.
Thanks.
Rubén Justo· Jan 10, 2024, 12:40 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()

On 09-ene-2024 14:32:20, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> that configuration knob is probably misnamed, as you
> pointed out).
Agree.  Maybe we have a better name: "showDisableInstructions".
Show 8 quoted lines
> I would understand if the proposed change were to change the
> "advice.<key>" configuration variable from a Boolean to a tristate
> (yes = default, no = disabled, always = give advice without
> instruction), though.  IOW, the message might look like so:
> 
>     hint: use --reapply-cherry-picks to include skipped commits
>     hint: setting advice.skippedCherryPicks to 'false' disables this message
>     hint: and setting it to 'always' removes this additional instruction.

That's an interesting twist ... and intuitive it seems, as Peff also came up with a similar approach.

But do we need, or want, that fine grain?

Using advise_if_enabled() allows us to display a hint while automatically adding the option to disable it, _explicitly_ (so far*), to the user.

But it happens that advise_if_enabled() itself has a hint to give.

My goal in this series is about giving the user the possibility to disable _that_ hint (in the hint).

The user choosing to disable that hint is telling us: "I know I can disable a hint, stop telling me so, please". So I don't think this opens up a new risk or problem finding how to disable a hint.

    $ git -c advice.showDisableInstructions=1 command_with_a_no_longer_welcomed_hint
Is there a need to make that "hint in the hint" customizable _per hint_?

That probably means that a new "make-it-always-for-all" may be desirable alongside the new tristate yes-no-always ...

I dunno.
    * perhaps this multi-value possibility could be a path
      to explore what Taylor outlined in another message in this thread:
      the possibility of a "one-shot" hint.
Rubén Justo· Jan 12, 2024, 10:05 UTC · re: Rubén Justo · lore

[PATCH] advice: allow disabling the automatic hint in advise_if_enabled()

Using advise_if_enabled() to display an advice will automatically include instructions on how to disable the advice, alongside the main advice:

	hint: use --reapply-cherry-picks to include skipped commits
	hint: Disable this message with "git config advice.skippedCherryPicks false"
To do so, we provide a knob which can be used to disable the advice.
But also to tell us the opposite: to show the advice.

Let's not include the deactivation instructions for an advice if the user explicitly sets its visibility.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

I hope this patch captures most of the comments from the previous iteration, mostly:

- Allow to disable the disabling instructions, per advice.
- No new test is needed, therefore the first two commits from the
  previous iteration are not needed here.
Thanks.
 advice.c          | 109 +++++++++++++++++++++++++---------------------
 t/t0018-advice.sh |   1 -
 2 files changed, 59 insertions(+), 51 deletions(-)
Show changes to 2 files +59 −51

advice.c, t/t0018-advice.sh

diff --git a/advice.c b/advice.c
index f6e4c2f302..8474966ce1 100644
--- a/advice.c
+++ b/advice.c
@@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
 	return "";
 }
 
+enum advice_level {
+	ADVICE_LEVEL_DEFAULT = 0,
+	ADVICE_LEVEL_DISABLED,
+	ADVICE_LEVEL_ENABLED,
+};
+
 static struct {
 	const char *key;
-	int enabled;
+	enum advice_level level;
 } advice_setting[] = {
-	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
-	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
-	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile", 1 },
-	[ADVICE_AMBIGUOUS_FETCH_REFSPEC]		= { "ambiguousFetchRefspec", 1 },
-	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
-	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName", 1 },
-	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge", 1 },
-	[ADVICE_DETACHED_HEAD]				= { "detachedHead", 1 },
-	[ADVICE_DIVERGING]				= { "diverging", 1 },
-	[ADVICE_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates", 1 },
-	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch", 1 },
-	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated", 1 },
-	[ADVICE_IGNORED_HOOK]				= { "ignoredHook", 1 },
-	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity", 1 },
-	[ADVICE_NESTED_TAG]				= { "nestedTag", 1 },
-	[ADVICE_OBJECT_NAME_WARNING]			= { "objectNameWarning", 1 },
-	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists", 1 },
-	[ADVICE_PUSH_FETCH_FIRST]			= { "pushFetchFirst", 1 },
-	[ADVICE_PUSH_NEEDS_FORCE]			= { "pushNeedsForce", 1 },
-	[ADVICE_PUSH_NON_FF_CURRENT]			= { "pushNonFFCurrent", 1 },
-	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching", 1 },
-	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate", 1 },
-	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName", 1 },
-	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected", 1 },
-	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward", 1 }, /* backwards compatibility */
-	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh", 1 },
-	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict", 1 },
-	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
-	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },
-	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure", 1 },
-	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks", 1 },
-	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning", 1 },
-	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
-	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
-	[ADVICE_SUBMODULES_NOT_UPDATED] 		= { "submodulesNotUpdated", 1 },
-	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
-	[ADVICE_SUGGEST_DETACHING_HEAD]			= { "suggestDetachingHead", 1 },
-	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
-	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
-	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
+	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
+	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
+	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
+	[ADVICE_AMBIGUOUS_FETCH_REFSPEC]		= { "ambiguousFetchRefspec" },
+	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir" },
+	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName" },
+	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge" },
+	[ADVICE_DETACHED_HEAD]				= { "detachedHead" },
+	[ADVICE_DIVERGING]				= { "diverging" },
+	[ADVICE_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates" },
+	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch" },
+	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated" },
+	[ADVICE_IGNORED_HOOK]				= { "ignoredHook" },
+	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity" },
+	[ADVICE_NESTED_TAG]				= { "nestedTag" },
+	[ADVICE_OBJECT_NAME_WARNING]			= { "objectNameWarning" },
+	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists" },
+	[ADVICE_PUSH_FETCH_FIRST]			= { "pushFetchFirst" },
+	[ADVICE_PUSH_NEEDS_FORCE]			= { "pushNeedsForce" },
+	[ADVICE_PUSH_NON_FF_CURRENT]			= { "pushNonFFCurrent" },
+	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching" },
+	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate" },
+	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName" },
+	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected" },
+	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward" }, /* backwards compatibility */
+	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
+	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
+	[ADVICE_RM_HINTS]				= { "rmHints" },
+	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
+	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
+	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
+	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning" },
+	[ADVICE_STATUS_HINTS]				= { "statusHints" },
+	[ADVICE_STATUS_U_OPTION]			= { "statusUoption" },
+	[ADVICE_SUBMODULES_NOT_UPDATED] 		= { "submodulesNotUpdated" },
+	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" },
+	[ADVICE_SUGGEST_DETACHING_HEAD]			= { "suggestDetachingHead" },
+	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath" },
+	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor" },
+	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan" },
 };
 
 static const char turn_off_instructions[] =
@@ -116,13 +122,13 @@ void advise(const char *advice, ...)
 
 int advice_enabled(enum advice_type type)
 {
-	switch(type) {
-	case ADVICE_PUSH_UPDATE_REJECTED:
-		return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
-		       advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
-	default:
-		return advice_setting[type].enabled;
-	}
+	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+
+	if (type == ADVICE_PUSH_UPDATE_REJECTED)
+		return enabled &&
+		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
+
+	return enabled;
 }
 
 void advise_if_enabled(enum advice_type type, const char *advice, ...)
@@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
 		return;
 
 	va_start(params, advice);
-	vadvise(advice, 1, advice_setting[type].key, params);
+	vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
+		advice_setting[type].key, params);
 	va_end(params);
 }
 
@@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
 	for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
 		if (strcasecmp(k, advice_setting[i].key))
 			continue;
-		advice_setting[i].enabled = git_config_bool(var, value);
+		advice_setting[i].level = git_config_bool(var, value)
+					  ? ADVICE_LEVEL_ENABLED
+					  : ADVICE_LEVEL_DISABLED;
 		return 0;
 	}
 
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index c13057a4ca..0dcfb760a2 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
 test_expect_success 'advice should be printed when config variable is set to true' '
 	cat >expect <<-\EOF &&
 	hint: This is a piece of advice
-	hint: Disable this message with "git config advice.nestedTag false"
 	EOF
 	test_config advice.nestedTag true &&
 	test-tool advise "This is a piece of advice" 2>actual &&

base-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d

-- 
2.42.0
Junio C Hamano· Jan 12, 2024, 22:19 UTC · re: Rubén Justo · lore

Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()

Rubén Justo <rjusto@gmail.com> writes:
Show 14 quoted lines
>  advice.c          | 109 +++++++++++++++++++++++++---------------------
>  t/t0018-advice.sh |   1 -
>  2 files changed, 59 insertions(+), 51 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index f6e4c2f302..8474966ce1 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
>  	return "";
>  }
>  
> +enum advice_level {
> +	ADVICE_LEVEL_DEFAULT = 0,

We may want to say "_NONE" not "_DEFAULT" to match the convention used elsewhere, but I have no strong preference. I do have a preference to see that, when we explicitly say "In this enum, there is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the definition, the places we use a variable of this enum type, we say

	if (!variable)
and not
	if (variable == ADVICE_LEVEL_DEFAULT)
Show 15 quoted lines
> +	ADVICE_LEVEL_DISABLED,
> +	ADVICE_LEVEL_ENABLED,
> +};
> +
>  static struct {
>  	const char *key;
> -	int enabled;
> +	enum advice_level level;
>  } advice_setting[] = {
> ...
> -	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
> +	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
> ...
> +	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan" },
>  };

It certainly becomes nicer to use the "unspecified is left to 0" convention like this.

Show 13 quoted lines
>  static const char turn_off_instructions[] =
> @@ -116,13 +122,13 @@ void advise(const char *advice, ...)
>  
>  int advice_enabled(enum advice_type type)
>  {
> -	switch(type) {
> -	case ADVICE_PUSH_UPDATE_REJECTED:
> -		return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
> -		       advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
> -	default:
> -		return advice_setting[type].enabled;
> -	}
> +	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
OK.
> +	if (type == ADVICE_PUSH_UPDATE_REJECTED)
> +		return enabled &&
> +		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
I like the textbook use of a simple recursion here ;-)
> +	return enabled;
>  }
Show 8 quoted lines
>  void advise_if_enabled(enum advice_type type, const char *advice, ...)
> @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
>  		return;
>  
>  	va_start(params, advice);
> -	vadvise(advice, 1, advice_setting[type].key, params);
> +	vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
> +		advice_setting[type].key, params);

OK. Once you configure this advice to be always shown, you no longer are using the _DEFAULT, so we pass 0 as the second parameter. Makes sense.

As I said, if we explicitly document that _DEFAULT's value is zero with "= 0" in the enum definition, which we also rely on the initialization of the advice_setting[] array, then it may probably be better to write it more like so:

	vadvice(advice, !advice_setting[type].level,
		advice_setting[type].key, params);

I wonder if it makes this caller simpler to update the return value of advice_enabled() to a tristate. Then we can do

	int enabled = advice_enabled(type);
	if (!enabled)
		return;
	va_start(...);
	vadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...);
	va_end(...);
but it does not make that much difference.
Show 11 quoted lines
>  	va_end(params);
>  }
>  
> @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
>  	for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
>  		if (strcasecmp(k, advice_setting[i].key))
>  			continue;
> -		advice_setting[i].enabled = git_config_bool(var, value);
> +		advice_setting[i].level = git_config_bool(var, value)
> +					  ? ADVICE_LEVEL_ENABLED
> +					  : ADVICE_LEVEL_DISABLED;

OK. We saw (var, value) in the configuration files we are parsing, so we find [i] that corresponds to the advice key and tweak the level.

This loop obviously is O(N*M) for the number of "advice.*" configuration variables N and the number of advice keys M, but that is not new to this patch---our expectation is that N will be small, even though M will grow over time.

Show 12 quoted lines
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index c13057a4ca..0dcfb760a2 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
>  test_expect_success 'advice should be printed when config variable is set to true' '
>  	cat >expect <<-\EOF &&
>  	hint: This is a piece of advice
> -	hint: Disable this message with "git config advice.nestedTag false"
>  	EOF
>  	test_config advice.nestedTag true &&
>  	test-tool advise "This is a piece of advice" 2>actual &&

OK. The commit changes behaviour for existing users who explicitly said "I want to see this advice" by configuring advice.* key to 'true', and it probably is a good thing, even though it may be a bit surprising.

One thing that may be missing is a documentation update. Something along this line to highlight that now 'true' has a bit different meaning from before (and as a consequence, we can no longer say "defaults to true").

 Documentation/config/advice.txt | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
Show changes to diff +7 −3
diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt
index 2737381a11..364a8268b6 100644
--- c/Documentation/config/advice.txt
+++ w/Documentation/config/advice.txt
@@ -1,7 +1,11 @@
 advice.*::
-	These variables control various optional help messages designed to
-	aid new users. All 'advice.*' variables default to 'true', and you
-	can tell Git that you do not need help by setting these to 'false':
+
+	These variables control various optional help messages
+	designed to aid new users.  When left unconfigured, Git will
+	give you the help message with an instruction on how to
+	squelch it.  When set to 'true', the help message is given,
+	but without hint on how to squelch the message.  You can
+	tell Git that you do not need help by setting these to 'false':
 +
 --
 	ambiguousFetchRefspec::
Jeff King· Jan 13, 2024, 07:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()

On Fri, Jan 12, 2024 at 02:19:32PM -0800, Junio C Hamano wrote:
Show 28 quoted lines
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >  advice.c          | 109 +++++++++++++++++++++++++---------------------
> >  t/t0018-advice.sh |   1 -
> >  2 files changed, 59 insertions(+), 51 deletions(-)
> >
> > diff --git a/advice.c b/advice.c
> > index f6e4c2f302..8474966ce1 100644
> > --- a/advice.c
> > +++ b/advice.c
> > @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
> >  	return "";
> >  }
> >  
> > +enum advice_level {
> > +	ADVICE_LEVEL_DEFAULT = 0,
> 
> We may want to say "_NONE" not "_DEFAULT" to match the convention
> used elsewhere, but I have no strong preference.  I do have a
> preference to see that, when we explicitly say "In this enum, there
> is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the
> definition, the places we use a variable of this enum type, we say
> 
> 	if (!variable)
> 
> and not
> 
> 	if (variable == ADVICE_LEVEL_DEFAULT)
This may be getting into the realm of bikeshedding, but... ;)

For a tri-state we often use "-1" for "not specified". That has the nice side effect in this case that "if (level)" shows the advice (that works because "unspecified" and "explicitly true" both show the advice. And then "if (level < 0)" is used for just the hint. But maybe that is too clever/fragile.

Of course that means that all of the initializers have to use "-1" explicitly. So zero-initialization is sometimes nice, too.

-Peff
Junio C Hamano· Jan 16, 2024, 04:56 UTC · re: Jeff King · lore

Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> For a tri-state we often use "-1" for "not specified". That has the nice
> side effect in this case that "if (level)" shows the advice (that works
> because "unspecified" and "explicitly true" both show the advice. And
> then "if (level < 0)" is used for just the hint. But maybe that is too
> clever/fragile.
>
> Of course that means that all of the initializers have to use "-1"
> explicitly. So zero-initialization is sometimes nice, too.
;-)  100% agreed.
Rubén Justo· Jan 15, 2024, 11:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()

On 12-ene-2024 14:19:32, Junio C Hamano wrote:
Show 19 quoted lines
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >  advice.c          | 109 +++++++++++++++++++++++++---------------------
> >  t/t0018-advice.sh |   1 -
> >  2 files changed, 59 insertions(+), 51 deletions(-)
> >
> > diff --git a/advice.c b/advice.c
> > index f6e4c2f302..8474966ce1 100644
> > --- a/advice.c
> > +++ b/advice.c
> > @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
> >  	return "";
> >  }
> >  
> > +enum advice_level {
> > +	ADVICE_LEVEL_DEFAULT = 0,
> 
> We may want to say "_NONE" not "_DEFAULT" to match the convention
> used elsewhere, but I have no strong preference.

The "_DEFAULT" name is rooted in the idea of having a default but configurable value. I'm not developing that idea in this series because I'm not sure if it's desirable. But, I'll leave a sketch here:

Show changes to advice.c +13 −2
diff --git a/advice.c b/advice.c
index 8474966ce1..1bb427a8d8 100644
--- a/advice.c
+++ b/advice.c
@@ -39,6 +39,8 @@ enum advice_level {
 	ADVICE_LEVEL_ENABLED,
 };
 
+static enum advice_level advice_level_default;
+
 static struct {
 	const char *key;
 	enum advice_level level;
@@ -122,7 +124,9 @@ void advise(const char *advice, ...)
 
 int advice_enabled(enum advice_type type)
 {
-	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+	int enabled = advice_setting[type].level
+		      ? advice_setting[type].level != ADVICE_LEVEL_DISABLED
+		      : advice_level_default != ADVICE_LEVEL_DISABLED;
 
 	if (type == ADVICE_PUSH_UPDATE_REJECTED)
 		return enabled &&
@@ -139,7 +143,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
 		return;
 
 	va_start(params, advice);
-	vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
+	vadvise(advice, !advice_setting[type].level && !advice_level_default,
 		advice_setting[type].key, params);
 	va_end(params);
 }
@@ -166,6 +170,13 @@ int git_default_advice_config(const char *var, const char *value)
 	if (!skip_prefix(var, "advice.", &k))
 		return 0;
 
+	if (!strcmp(var, "advice.default")) {
+		advice_level_default = git_config_bool(var, value)
+				       ? ADVICE_LEVEL_ENABLED
+				       : ADVICE_LEVEL_DISABLED;
+		return 0;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
 		if (strcasecmp(k, advice_setting[i].key))
 			continue;

The way it is now, "_NONE" is perfectly fine.  I'll use it.

> I do have a
> preference to see that, when we explicitly say "In this enum, there
> is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the
> definition, the places we use a variable of this enum type, we say
> 
> 	if (!variable)

OK

> 
> and not
> 
> 	if (variable == ADVICE_LEVEL_DEFAULT)
> 
> > +	ADVICE_LEVEL_DISABLED,
> > +	ADVICE_LEVEL_ENABLED,
> > +};
> > +
> >  static struct {
> >  	const char *key;
> > -	int enabled;
> > +	enum advice_level level;
> >  } advice_setting[] = {
> > ...
> > -	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
> > +	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
> > ...
> > +	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan" },
> >  };
> 
> It certainly becomes nicer to use the "unspecified is left to 0"
> convention like this.

Indeed, that's my feeling too.

> 
> >  static const char turn_off_instructions[] =
> > @@ -116,13 +122,13 @@ void advise(const char *advice, ...)
> >  
> >  int advice_enabled(enum advice_type type)
> >  {
> > -	switch(type) {
> > -	case ADVICE_PUSH_UPDATE_REJECTED:
> > -		return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
> > -		       advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
> > -	default:
> > -		return advice_setting[type].enabled;
> > -	}
> > +	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
> 
> OK.
> 
> > +	if (type == ADVICE_PUSH_UPDATE_REJECTED)
> > +		return enabled &&
> > +		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
> 
> I like the textbook use of a simple recursion here ;-)
> 
> > +	return enabled;
> >  }
> 
> >  void advise_if_enabled(enum advice_type type, const char *advice, ...)
> > @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
> >  		return;
> >  
> >  	va_start(params, advice);
> > -	vadvise(advice, 1, advice_setting[type].key, params);
> > +	vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
> > +		advice_setting[type].key, params);
> 
> OK.  Once you configure this advice to be always shown, you no
> longer are using the _DEFAULT, so we pass 0 as the second parameter.
> Makes sense.
> 
> As I said, if we explicitly document that _DEFAULT's value is zero
> with "= 0" in the enum definition, which we also rely on the
> initialization of the advice_setting[] array, then it may probably
> be better to write it more like so:
> 
> 	vadvice(advice, !advice_setting[type].level,
> 		advice_setting[type].key, params);

OK

> 
> I wonder if it makes this caller simpler to update the return value
> of advice_enabled() to a tristate.  Then we can do
> 
> 	int enabled = advice_enabled(type);
> 
> 	if (!enabled)
> 		return;
> 	va_start(...);
> 	vadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...);
> 	va_end(...);
> 
> but it does not make that much difference.
> 
> >  	va_end(params);
> >  }
> >  
> > @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
> >  	for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
> >  		if (strcasecmp(k, advice_setting[i].key))
> >  			continue;
> > -		advice_setting[i].enabled = git_config_bool(var, value);
> > +		advice_setting[i].level = git_config_bool(var, value)
> > +					  ? ADVICE_LEVEL_ENABLED
> > +					  : ADVICE_LEVEL_DISABLED;
> 
> OK.  We saw (var, value) in the configuration files we are parsing,
> so we find [i] that corresponds to the advice key and tweak the
> level.
> 
> This loop obviously is O(N*M) for the number of "advice.*"
> configuration variables N and the number of advice keys M, but that
> is not new to this patch---our expectation is that N will be small,
> even though M will grow over time.

I wonder if we can drop entirely this loop.  Maybe a hashmap can be
helpful here.  Of course, this is tangential to this series.

> 
> > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> > index c13057a4ca..0dcfb760a2 100755
> > --- a/t/t0018-advice.sh
> > +++ b/t/t0018-advice.sh
> > @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
> >  test_expect_success 'advice should be printed when config variable is set to true' '
> >  	cat >expect <<-\EOF &&
> >  	hint: This is a piece of advice
> > -	hint: Disable this message with "git config advice.nestedTag false"
> >  	EOF
> >  	test_config advice.nestedTag true &&
> >  	test-tool advise "This is a piece of advice" 2>actual &&
> 
> OK.  The commit changes behaviour for existing users who explicitly
> said "I want to see this advice" by configuring advice.* key to 'true',

Technically, when the user sets any value.

Maybe in the future we transform the knob to have more than two states,
beyond yes/no.  At that point, current rationality would still apply.

> and it probably is a good thing, even though it may be a bit surprising.
> 
> One thing that may be missing is a documentation update.  Something
> along this line to highlight that now 'true' has a bit different
> meaning from before (and as a consequence, we can no longer say
> "defaults to true").
> 
>  Documentation/config/advice.txt | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt
> index 2737381a11..364a8268b6 100644
> --- c/Documentation/config/advice.txt
> +++ w/Documentation/config/advice.txt
> @@ -1,7 +1,11 @@
>  advice.*::
> -	These variables control various optional help messages designed to
> -	aid new users. All 'advice.*' variables default to 'true', and you
> -	can tell Git that you do not need help by setting these to 'false':
> +
> +	These variables control various optional help messages
> +	designed to aid new users.  When left unconfigured, Git will
> +	give you the help message with an instruction on how to
> +	squelch it.  When set to 'true', the help message is given,
> +	but without hint on how to squelch the message.  You can
> +	tell Git that you do not need help by setting these to 'false':
>  +
>  --
>  	ambiguousFetchRefspec::

OK.  I'll add this.

Thanks.
Rubén Justo· Jan 15, 2024, 14:28 UTC · re: Rubén Justo · lore

[PATCH v2] advice: allow disabling the automatic hint in advise_if_enabled()

Using advise_if_enabled() to display an advice will automatically include instructions on how to disable the advice, alongside the main advice:

	hint: use --reapply-cherry-picks to include skipped commits
	hint: Disable this message with "git config advice.skippedCherryPicks false"
To do so, we provide a knob which can be used to disable the advice.
But also to tell us the opposite: to show the advice.

Let's not include the deactivation instructions for an advice if the user explicitly sets its visibility.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

This must have been v3, but previous iteration was erroneously sent as a v1. Sorry.

Range-diff against v1:
1:  d280195c7b ! 1:  0bee5d1bba advice: allow disabling the automatic hint in advise_if_enabled()
    @@ Commit message
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
    + ## Documentation/config/advice.txt ##
    +@@
    + advice.*::
    + 	These variables control various optional help messages designed to
    +-	aid new users. All 'advice.*' variables default to 'true', and you
    +-	can tell Git that you do not need help by setting these to 'false':
    ++	aid new users.  When left unconfigured, Git will give the message
    ++	alongside instructions on how to squelch it.  You can tell Git
    ++	that you do not need the help message by setting these to 'false':
    + +
    + --
    + 	addEmbeddedRepo::
    +
      ## advice.c ##
     @@ advice.c: static const char *advise_get_color(enum color_advice ix)
      	return "";
      }
      
     +enum advice_level {
    -+	ADVICE_LEVEL_DEFAULT = 0,
    ++	ADVICE_LEVEL_NONE = 0,
     +	ADVICE_LEVEL_DISABLED,
     +	ADVICE_LEVEL_ENABLED,
     +};
    @@ advice.c: void advise_if_enabled(enum advice_type type, const char *advice, ...)
      
      	va_start(params, advice);
     -	vadvise(advice, 1, advice_setting[type].key, params);
    -+	vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
    -+		advice_setting[type].key, params);
    ++	vadvise(advice, !advice_setting[type].level, advice_setting[type].key,
    ++		params);
      	va_end(params);
      }
      
 Documentation/config/advice.txt |   5 +-
 advice.c                        | 109 +++++++++++++++++---------------
 t/t0018-advice.sh               |   1 -
 3 files changed, 62 insertions(+), 53 deletions(-)
Show changes to 3 files +62 −53

Documentation/config/advice.txt, advice.c, t/t0018-advice.sh

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 25c0917524..c7ea70f2e2 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -1,7 +1,8 @@
 advice.*::
 	These variables control various optional help messages designed to
-	aid new users. All 'advice.*' variables default to 'true', and you
-	can tell Git that you do not need help by setting these to 'false':
+	aid new users.  When left unconfigured, Git will give the message
+	alongside instructions on how to squelch it.  You can tell Git
+	that you do not need the help message by setting these to 'false':
 +
 --
 	addEmbeddedRepo::
diff --git a/advice.c b/advice.c
index f6e4c2f302..6e9098ff08 100644
--- a/advice.c
+++ b/advice.c
@@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
 	return "";
 }
 
+enum advice_level {
+	ADVICE_LEVEL_NONE = 0,
+	ADVICE_LEVEL_DISABLED,
+	ADVICE_LEVEL_ENABLED,
+};
+
 static struct {
 	const char *key;
-	int enabled;
+	enum advice_level level;
 } advice_setting[] = {
-	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
-	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
-	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile", 1 },
-	[ADVICE_AMBIGUOUS_FETCH_REFSPEC]		= { "ambiguousFetchRefspec", 1 },
-	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
-	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName", 1 },
-	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge", 1 },
-	[ADVICE_DETACHED_HEAD]				= { "detachedHead", 1 },
-	[ADVICE_DIVERGING]				= { "diverging", 1 },
-	[ADVICE_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates", 1 },
-	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch", 1 },
-	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated", 1 },
-	[ADVICE_IGNORED_HOOK]				= { "ignoredHook", 1 },
-	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity", 1 },
-	[ADVICE_NESTED_TAG]				= { "nestedTag", 1 },
-	[ADVICE_OBJECT_NAME_WARNING]			= { "objectNameWarning", 1 },
-	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists", 1 },
-	[ADVICE_PUSH_FETCH_FIRST]			= { "pushFetchFirst", 1 },
-	[ADVICE_PUSH_NEEDS_FORCE]			= { "pushNeedsForce", 1 },
-	[ADVICE_PUSH_NON_FF_CURRENT]			= { "pushNonFFCurrent", 1 },
-	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching", 1 },
-	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate", 1 },
-	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName", 1 },
-	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected", 1 },
-	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward", 1 }, /* backwards compatibility */
-	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh", 1 },
-	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict", 1 },
-	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
-	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },
-	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure", 1 },
-	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks", 1 },
-	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning", 1 },
-	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
-	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
-	[ADVICE_SUBMODULES_NOT_UPDATED] 		= { "submodulesNotUpdated", 1 },
-	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
-	[ADVICE_SUGGEST_DETACHING_HEAD]			= { "suggestDetachingHead", 1 },
-	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
-	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
-	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
+	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
+	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
+	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
+	[ADVICE_AMBIGUOUS_FETCH_REFSPEC]		= { "ambiguousFetchRefspec" },
+	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir" },
+	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName" },
+	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge" },
+	[ADVICE_DETACHED_HEAD]				= { "detachedHead" },
+	[ADVICE_DIVERGING]				= { "diverging" },
+	[ADVICE_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates" },
+	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch" },
+	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated" },
+	[ADVICE_IGNORED_HOOK]				= { "ignoredHook" },
+	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity" },
+	[ADVICE_NESTED_TAG]				= { "nestedTag" },
+	[ADVICE_OBJECT_NAME_WARNING]			= { "objectNameWarning" },
+	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists" },
+	[ADVICE_PUSH_FETCH_FIRST]			= { "pushFetchFirst" },
+	[ADVICE_PUSH_NEEDS_FORCE]			= { "pushNeedsForce" },
+	[ADVICE_PUSH_NON_FF_CURRENT]			= { "pushNonFFCurrent" },
+	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching" },
+	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate" },
+	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName" },
+	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected" },
+	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward" }, /* backwards compatibility */
+	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
+	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
+	[ADVICE_RM_HINTS]				= { "rmHints" },
+	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
+	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
+	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
+	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning" },
+	[ADVICE_STATUS_HINTS]				= { "statusHints" },
+	[ADVICE_STATUS_U_OPTION]			= { "statusUoption" },
+	[ADVICE_SUBMODULES_NOT_UPDATED] 		= { "submodulesNotUpdated" },
+	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" },
+	[ADVICE_SUGGEST_DETACHING_HEAD]			= { "suggestDetachingHead" },
+	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath" },
+	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor" },
+	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan" },
 };
 
 static const char turn_off_instructions[] =
@@ -116,13 +122,13 @@ void advise(const char *advice, ...)
 
 int advice_enabled(enum advice_type type)
 {
-	switch(type) {
-	case ADVICE_PUSH_UPDATE_REJECTED:
-		return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
-		       advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
-	default:
-		return advice_setting[type].enabled;
-	}
+	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+
+	if (type == ADVICE_PUSH_UPDATE_REJECTED)
+		return enabled &&
+		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
+
+	return enabled;
 }
 
 void advise_if_enabled(enum advice_type type, const char *advice, ...)
@@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
 		return;
 
 	va_start(params, advice);
-	vadvise(advice, 1, advice_setting[type].key, params);
+	vadvise(advice, !advice_setting[type].level, advice_setting[type].key,
+		params);
 	va_end(params);
 }
 
@@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
 	for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
 		if (strcasecmp(k, advice_setting[i].key))
 			continue;
-		advice_setting[i].enabled = git_config_bool(var, value);
+		advice_setting[i].level = git_config_bool(var, value)
+					  ? ADVICE_LEVEL_ENABLED
+					  : ADVICE_LEVEL_DISABLED;
 		return 0;
 	}
 
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index c13057a4ca..0dcfb760a2 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
 test_expect_success 'advice should be printed when config variable is set to true' '
 	cat >expect <<-\EOF &&
 	hint: This is a piece of advice
-	hint: Disable this message with "git config advice.nestedTag false"
 	EOF
 	test_config advice.nestedTag true &&
 	test-tool advise "This is a piece of advice" 2>actual &&

base-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d

-- 
2.42.0

← back to recent threads