Merge branch 'hw/advise-ng'

Revamping of the advise API to allow more systematic enumeration of
advice knobs in the future.

* hw/advise-ng:
  tag: use new advice API to check visibility
  advice: revamp advise API
  advice: change "setupStreamFailure" to "setUpstreamFailure"
  advice: extract vadvise() from advise()

9 files changed, 200 insertions, 12 deletions

diff --git a/Makefile b/Makefile
index 9804a0758b..a5961113d8 100644
--- a/Makefile
+++ b/Makefile
@@ -695,6 +695,7 @@ X =
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
+TEST_BUILTINS_OBJS += test-advise.o
 TEST_BUILTINS_OBJS += test-chmtime.o
 TEST_BUILTINS_OBJS += test-config.o
 TEST_BUILTINS_OBJS += test-ctype.o
diff --git a/advice.c b/advice.c
index 97f3f981b4..f0a3d32d20 100644
--- a/advice.c
+++ b/advice.c
@@ -29,7 +29,6 @@ int advice_ignored_hook = 1;
 int advice_waiting_for_editor = 1;
 int advice_graft_file_deprecated = 1;
 int advice_checkout_ambiguous_remote_branch_name = 1;
-int advice_nested_tag = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
 int advice_add_ignored_file = 1;
 int advice_add_empty_pathspec = 1;
@@ -82,7 +81,7 @@ static struct {
 	{ "sequencerInUse", &advice_sequencer_in_use },
 	{ "implicitIdentity", &advice_implicit_identity },
 	{ "detachedHead", &advice_detached_head },
-	{ "setupStreamFailure", &advice_set_upstream_failure },
+	{ "setUpstreamFailure", &advice_set_upstream_failure },
 	{ "objectNameWarning", &advice_object_name_warning },
 	{ "amWorkDir", &advice_amworkdir },
 	{ "rmHints", &advice_rm_hints },
@@ -91,7 +90,6 @@ static struct {
 	{ "waitingForEditor", &advice_waiting_for_editor },
 	{ "graftFileDeprecated", &advice_graft_file_deprecated },
 	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
-	{ "nestedTag", &advice_nested_tag },
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
 	{ "addIgnoredFile", &advice_add_ignored_file },
 	{ "addEmptyPathspec", &advice_add_empty_pathspec },
@@ -100,15 +98,58 @@ static struct {
 	{ "pushNonFastForward", &advice_push_update_rejected }
 };
 
-void advise(const char *advice, ...)
+static struct {
+	const char *key;
+	int enabled;
+} advice_setting[] = {
+	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 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_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates", 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 },
+
+	/* make this an alias for backward compatibility */
+	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward", 1 },
+
+	[ADVICE_PUSH_NON_FF_CURRENT]			= { "pushNonFFCurrent", 1 },
+	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching", 1 },
+	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName", 1 },
+	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected", 1 },
+	[ADVICE_RESET_QUIET_WARNING]			= { "resetQuiet", 1 },
+	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict", 1 },
+	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
+	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },
+	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure", 1 },
+	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning", 1 },
+	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
+	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
+	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
+	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
+};
+
+static const char turn_off_instructions[] =
+N_("\n"
+   "Disable this message with \"git config advice.%s false\"");
+
+static void vadvise(const char *advice, int display_instructions,
+		    const char *key, va_list params)
 {
 	struct strbuf buf = STRBUF_INIT;
-	va_list params;
 	const char *cp, *np;
 
-	va_start(params, advice);
 	strbuf_vaddf(&buf, advice, params);
-	va_end(params);
+
+	if (display_instructions)
+		strbuf_addf(&buf, turn_off_instructions, key);
 
 	for (cp = buf.buf; *cp; cp = np) {
 		np = strchrnul(cp, '\n');
@@ -122,6 +163,37 @@ void advise(const char *advice, ...)
 	strbuf_release(&buf);
 }
 
+void advise(const char *advice, ...)
+{
+	va_list params;
+	va_start(params, advice);
+	vadvise(advice, 0, "", params);
+	va_end(params);
+}
+
+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;
+	}
+}
+
+void advise_if_enabled(enum advice_type type, const char *advice, ...)
+{
+	va_list params;
+
+	if (!advice_enabled(type))
+		return;
+
+	va_start(params, advice);
+	vadvise(advice, 1, advice_setting[type].key, params);
+	va_end(params);
+}
+
 int git_default_advice_config(const char *var, const char *value)
 {
 	const char *k, *slot_name;
@@ -148,6 +220,13 @@ int git_default_advice_config(const char *var, const char *value)
 		if (strcasecmp(k, advice_config[i].name))
 			continue;
 		*advice_config[i].preference = git_config_bool(var, value);
+		break;
+	}
+
+	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);
 		return 0;
 	}
 
@@ -158,8 +237,8 @@ void list_config_advices(struct string_list *list, const char *prefix)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(advice_config); i++)
-		list_config_item(list, prefix, advice_config[i].name);
+	for (i = 0; i < ARRAY_SIZE(advice_setting); i++)
+		list_config_item(list, prefix, advice_setting[i].key);
 }
 
 int error_resolve_conflict(const char *me)
diff --git a/advice.h b/advice.h
index 0e6e58d9f8..16f2c11642 100644
--- a/advice.h
+++ b/advice.h
@@ -29,14 +29,64 @@ extern int advice_ignored_hook;
 extern int advice_waiting_for_editor;
 extern int advice_graft_file_deprecated;
 extern int advice_checkout_ambiguous_remote_branch_name;
-extern int advice_nested_tag;
 extern int advice_submodule_alternate_error_strategy_die;
 extern int advice_add_ignored_file;
 extern int advice_add_empty_pathspec;
 
+/*
+ * To add a new advice, you need to:
+ * Define a new advice_type.
+ * Add a new entry to advice_setting array.
+ * Add the new config variable to Documentation/config/advice.txt.
+ * Call advise_if_enabled to print your advice.
+ */
+ enum advice_type {
+	ADVICE_ADD_EMBEDDED_REPO,
+	ADVICE_AM_WORK_DIR,
+	ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
+	ADVICE_COMMIT_BEFORE_MERGE,
+	ADVICE_DETACHED_HEAD,
+	ADVICE_FETCH_SHOW_FORCED_UPDATES,
+	ADVICE_GRAFT_FILE_DEPRECATED,
+	ADVICE_IGNORED_HOOK,
+	ADVICE_IMPLICIT_IDENTITY,
+	ADVICE_NESTED_TAG,
+	ADVICE_OBJECT_NAME_WARNING,
+	ADVICE_PUSH_ALREADY_EXISTS,
+	ADVICE_PUSH_FETCH_FIRST,
+	ADVICE_PUSH_NEEDS_FORCE,
+	ADVICE_PUSH_NON_FF_CURRENT,
+	ADVICE_PUSH_NON_FF_MATCHING,
+	ADVICE_PUSH_UNQUALIFIED_REF_NAME,
+	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
+	ADVICE_PUSH_UPDATE_REJECTED,
+	ADVICE_RESET_QUIET_WARNING,
+	ADVICE_RESOLVE_CONFLICT,
+	ADVICE_RM_HINTS,
+	ADVICE_SEQUENCER_IN_USE,
+	ADVICE_SET_UPSTREAM_FAILURE,
+	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
+	ADVICE_STATUS_HINTS,
+	ADVICE_STATUS_U_OPTION,
+	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
+	ADVICE_WAITING_FOR_EDITOR,
+};
+
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
 void advise(const char *advice, ...);
+
+/**
+ * Checks if advice type is enabled (can be printed to the user).
+ * Should be called before advise().
+ */
+int advice_enabled(enum advice_type type);
+
+/**
+ * Checks the visibility of the advice before printing.
+ */
+void advise_if_enabled(enum advice_type type, const char *advice, ...);
+
 int error_resolve_conflict(const char *me);
 void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
diff --git a/builtin/tag.c b/builtin/tag.c
index e0a4c25382..cc30d346f5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -231,8 +231,9 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 	if (type <= OBJ_NONE)
 		die(_("bad object type."));
 
-	if (type == OBJ_TAG && advice_nested_tag)
-		advise(_(message_advice_nested_tag), tag, object_ref);
+	if (type == OBJ_TAG)
+		advise_if_enabled(ADVICE_NESTED_TAG, _(message_advice_nested_tag),
+				  tag, object_ref);
 
 	strbuf_addf(&header,
 		    "object %s\n"
diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
new file mode 100644
index 0000000000..38cdc2884e
--- /dev/null
+++ b/t/helper/test-advise.c
@@ -0,0 +1,22 @@
+#include "test-tool.h"
+#include "cache.h"
+#include "advice.h"
+#include "config.h"
+
+int cmd__advise_if_enabled(int argc, const char **argv)
+{
+	if (!argv[1])
+	die("usage: %s <advice>", argv[0]);
+
+	setup_git_directory();
+	git_config(git_default_config, NULL);
+
+	/*
+	 * Any advice type can be used for testing, but NESTED_TAG was
+	 * selected here and in t0018 where this command is being
+	 * executed.
+	 */
+	advise_if_enabled(ADVICE_NESTED_TAG, argv[1]);
+
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index c9a232d238..31eedcd241 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -14,6 +14,7 @@ struct test_cmd {
 };
 
 static struct test_cmd cmds[] = {
+	{ "advise", cmd__advise_if_enabled },
 	{ "chmtime", cmd__chmtime },
 	{ "config", cmd__config },
 	{ "ctype", cmd__ctype },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index c8549fd87f..4eb5e6609e 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -4,6 +4,7 @@
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "git-compat-util.h"
 
+int cmd__advise_if_enabled(int argc, const char **argv);
 int cmd__chmtime(int argc, const char **argv);
 int cmd__config(int argc, const char **argv);
 int cmd__ctype(int argc, const char **argv);
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
new file mode 100755
index 0000000000..e03554d2f3
--- /dev/null
+++ b/t/t0018-advice.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='Test advise_if_enabled functionality'
+
+. ./test-lib.sh
+
+test_expect_success 'advice should be printed when config variable is unset' '
+	cat >expect <<-\EOF &&
+	hint: This is a piece of advice
+	hint: Disable this message with "git config advice.nestedTag false"
+	EOF
+	test-tool advise "This is a piece of advice" 2>actual &&
+	test_i18ncmp expect actual
+'
+
+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 &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'advice should not be printed when config variable is set to false' '
+	test_config advice.nestedTag false &&
+	test-tool advise "This is a piece of advice" 2>actual &&
+	test_must_be_empty actual
+'
+
+test_done
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 6db92bd3ba..74b637deb2 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1726,6 +1726,7 @@ test_expect_success 'recursive tagging should give advice' '
 	hint: already a tag. If you meant to tag the object that it points to, use:
 	hint: |
 	hint: 	git tag -f nested annotated-v4.0^{}
+	hint: Disable this message with "git config advice.nestedTag false"
 	EOF
 	git tag -m nested nested annotated-v4.0 2>actual &&
 	test_i18ncmp expect actual