diff mbox series

[2/2] advice: add "all" option to disable all hints

Message ID 20240424035857.84583-3-james@jamesliu.io (mailing list archive)
State Superseded
Headers show
Series advice: add "all" option to disable all hints | expand

Commit Message

James Liu April 24, 2024, 3:58 a.m. UTC 
Advice hints must be disabled individually by setting the relevant
advice.* variables to false in the Git configuration. For server-side
usages of Git where hints aren't necessary, it can be cumbersome to
maintain configuration to disable all advice hints. This is especially
the case if/when new advice hints are added.

Add a new "all" advice variable which acts as a toggle for all advice
types. When this is being used, individual advice hints can be enabled
by setting their respective configs to true.

Signed-off-by: James Liu <james@jamesliu.io>
---
 Documentation/config/advice.txt |  5 +++++
 advice.c                        |  8 ++++++++
 advice.h                        |  1 +
 t/t0018-advice.sh               | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+)

Comments

Patrick Steinhardt April 24, 2024, 5:29 a.m. UTC | #1 
On Wed, Apr 24, 2024 at 01:58:57PM +1000, James Liu wrote:
[snip]
> --- a/advice.c
> +++ b/advice.c
> @@ -43,6 +43,7 @@ static struct {
>  	const char *key;
>  	enum advice_level level;
>  } advice_setting[] = {
> +	[ADVICE_ALL]					= { "all" },
>  	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
>  	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
>  	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
> @@ -132,6 +133,13 @@ int advice_enabled(enum advice_type type)
>  		return enabled &&
>  		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
>  
> +	/*
> +	 * We still allow for specific advice hints to be enabled if
> +	 * advice.all == false.
> +	 */
> +	if (advice_setting[ADVICE_ALL].level == ADVICE_LEVEL_DISABLED)
> +		return advice_setting[type].level == ADVICE_LEVEL_ENABLED;

Makes sense. When the advice is unset we don't need to handle it at all,
and if it is enabled we basically want to behave the same as before.

>  	return enabled;
>  }
>  
> diff --git a/advice.h b/advice.h
> index c8d29f97f3..b5ac99a645 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -11,6 +11,7 @@ struct string_list;
>   * Call advise_if_enabled to print your advice.
>   */
>  enum advice_type {
> +	ADVICE_ALL,
>  	ADVICE_ADD_EMBEDDED_REPO,
>  	ADVICE_ADD_EMPTY_PATHSPEC,
>  	ADVICE_ADD_IGNORED_FILE,
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index 8010796e1f..19318cc9bb 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -53,4 +53,37 @@ test_expect_success 'advice should be printed when advice.pushUpdateRejected is
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'advice should not be printed when advice.all is set to false' '
> +	test_config advice.all false &&
> +	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
> +	test_must_be_empty actual
> +'
> +
> +test_expect_success 'advice should not be printed for pushUpdateRejected when advice.all is set to false' '
> +	test_config advice.all false &&
> +	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
> +	test_must_be_empty actual
> +'
> +
> +test_expect_success 'advice should be printed when advice.all is set to false, but specific advice is set to true' '
> +	cat >expect <<-\EOF &&
> +	hint: This is a piece of advice
> +	EOF
> +	test_config advice.all false &&
> +	test_config advice.nestedTag true &&
> +	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'advice should be printed when advice.all is set to false, but advice.pushUpdateRejected and its alias are set to true' '
> +	cat >expect <<-\EOF &&
> +	hint: This is a piece of advice
> +	EOF
> +	test_config advice.all false &&
> +	test_config advice.pushUpdateRejected true &&
> +	test_config advice.pushNonFastForward true &&
> +	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
> +	test_cmp expect actual
> +'
>  test_done

Do we actually have to enable both advices here? If not, then this
should probably be two separate tests that check whether each of the
keys does its thing.

Patrick
diff mbox series

Patch

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 0e35ae5240..0516a23b6b 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -5,6 +5,11 @@  advice.*::
 	that you do not need the help message by setting these to `false`:
 +
 --
+	all::
+		Convenience option that allows all advice hints to be
+		disabled when set to false. When false, individual
+		advice hints can be enabled by setting them to true.
+		Setting this to true is a no-op.
 	addEmbeddedRepo::
 		Shown when the user accidentally adds one
 		git repo inside of another.
diff --git a/advice.c b/advice.c
index 75111191ad..9f0860f143 100644
--- a/advice.c
+++ b/advice.c
@@ -43,6 +43,7 @@  static struct {
 	const char *key;
 	enum advice_level level;
 } advice_setting[] = {
+	[ADVICE_ALL]					= { "all" },
 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
 	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
 	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
@@ -132,6 +133,13 @@  int advice_enabled(enum advice_type type)
 		return enabled &&
 		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
 
+	/*
+	 * We still allow for specific advice hints to be enabled if
+	 * advice.all == false.
+	 */
+	if (advice_setting[ADVICE_ALL].level == ADVICE_LEVEL_DISABLED)
+		return advice_setting[type].level == ADVICE_LEVEL_ENABLED;
+
 	return enabled;
 }
 
diff --git a/advice.h b/advice.h
index c8d29f97f3..b5ac99a645 100644
--- a/advice.h
+++ b/advice.h
@@ -11,6 +11,7 @@  struct string_list;
  * Call advise_if_enabled to print your advice.
  */
 enum advice_type {
+	ADVICE_ALL,
 	ADVICE_ADD_EMBEDDED_REPO,
 	ADVICE_ADD_EMPTY_PATHSPEC,
 	ADVICE_ADD_IGNORED_FILE,
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index 8010796e1f..19318cc9bb 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -53,4 +53,37 @@  test_expect_success 'advice should be printed when advice.pushUpdateRejected is
 	test_cmp expect actual
 '
 
+test_expect_success 'advice should not be printed when advice.all is set to false' '
+	test_config advice.all false &&
+	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'advice should not be printed for pushUpdateRejected when advice.all is set to false' '
+	test_config advice.all false &&
+	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'advice should be printed when advice.all is set to false, but specific advice is set to true' '
+	cat >expect <<-\EOF &&
+	hint: This is a piece of advice
+	EOF
+	test_config advice.all false &&
+	test_config advice.nestedTag true &&
+	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'advice should be printed when advice.all is set to false, but advice.pushUpdateRejected and its alias are set to true' '
+	cat >expect <<-\EOF &&
+	hint: This is a piece of advice
+	EOF
+	test_config advice.all false &&
+	test_config advice.pushUpdateRejected true &&
+	test_config advice.pushNonFastForward true &&
+	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
+	test_cmp expect actual
+'
+
 test_done