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

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

[Rubén Justo]

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

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

[Rubén Justo]

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);
-- 
2.42.0

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

[Rubén Justo]

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(-)

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

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

[Rubén Justo]

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
+'
+
 test_done
-- 
2.42.0

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

[Junio C Hamano]

> 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");


> 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);

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

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

>
> 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()
>  	};

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

[Taylor Blau]

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

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

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

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

[Taylor Blau]

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.

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.

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

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

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

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

[Taylor Blau]

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.

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

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

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

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

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

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

[Jeff King]

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.

>  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

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

[Rubén Justo]

On 10-ene-2024 06:02:26, Jeff King wrote:
> 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.

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

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

[Rubén Justo]

On 09-ene-2024 13:27:37, Taylor Blau wrote:

> > +	[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 ;-)

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

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

[Rubén Justo]

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

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

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

[Dragan Simic]

On 2024-01-10 12:02, Jeff King wrote:
> 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.

> 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

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

[Rubén Justo]

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.

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

[Dragan Simic]

On 2024-01-10 15:32, Rubén Justo wrote:
> 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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Junio C Hamano]

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.

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

[Dragan Simic]

On 2024-01-10 17:22, Junio C Hamano wrote:
> 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.

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

[Jeff King]

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.

> 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

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

[Rubén Justo]

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(-)

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

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

[Junio C Hamano]

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)

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

>  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);

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.

> 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(-)

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

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

[Jeff King]

On Fri, Jan 12, 2024 at 02:19:32PM -0800, Junio C Hamano wrote:

> 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

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

[Rubén Justo]

On 12-ene-2024 14:19:32, Junio C Hamano wrote:
> 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:

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.

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

[Rubén Justo]

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(-)

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Dragan Simic]

On 2024-01-11 09:04, Jeff King wrote:
> 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.

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

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

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

[Junio C Hamano]

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?

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

[Dragan Simic]

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.

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

[Junio C Hamano]

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?

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

[Dragan Simic]

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

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

[Rubén Justo]

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

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.

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

[Dragan Simic]

On 2024-01-20 12:31, Rubén Justo wrote:
> 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.

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