diff mbox series

advice: allow disabling the automatic hint in advise_if_enabled()

Message ID c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com (mailing list archive)
State Superseded
Headers show
Series advice: allow disabling the automatic hint in advise_if_enabled() | expand

Commit Message

Rubén Justo Jan. 12, 2024, 10:05 a.m. UTC 
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(-)


base-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d

Comments

Junio C Hamano Jan. 12, 2024, 10:19 p.m. UTC | #1 
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::
Jeff King Jan. 13, 2024, 7:38 a.m. UTC | #2 
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
Rubén Justo Jan. 15, 2024, 11:24 a.m. UTC | #3 
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.
Junio C Hamano Jan. 16, 2024, 4:56 a.m. UTC | #4 
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.
diff mbox series

Patch

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