[PATCH v1 0/4] advice: remove usage of `advice_*` global variables

[PATCH v1 0/4] advice: remove usage of `advice_*` global variables

[Ben Boeckel]

Hi,

When looking at global variable usage for my `branch.default*` settings,
I found the `advice_` variables which were simple enough to resolve.

--Ben

---

Ben Boeckel (4):
  advice: add a function to set the value of an advice type
  advice: add enum variants for missing advice variables
  advice: remove uses of global `advice_` variables
  advice: remove static global variables for advice tracking

 advice.c                    | 88 ++++---------------------------------
 advice.h                    | 38 +++-------------
 branch.c                    |  2 +-
 builtin/add.c               |  8 ++--
 builtin/am.c                |  2 +-
 builtin/checkout.c          |  6 +--
 builtin/clone.c             |  2 +-
 builtin/commit.c            |  4 +-
 builtin/fetch.c             |  2 +-
 builtin/merge.c             |  4 +-
 builtin/push.c              | 12 ++---
 builtin/replace.c           |  2 +-
 builtin/reset.c             |  2 +-
 builtin/rm.c                |  2 +-
 builtin/submodule--helper.c |  2 +-
 commit.c                    |  2 +-
 editor.c                    |  2 +-
 notes-merge.c               |  2 +-
 object-name.c               |  2 +-
 remote.c                    | 12 ++---
 run-command.c               |  2 +-
 sequencer.c                 |  8 ++--
 unpack-trees.c              | 18 ++++----
 wt-status.c                 |  6 +--
 24 files changed, 68 insertions(+), 162 deletions(-)


base-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687
-- 
2.31.1

[PATCH v1 1/4] advice: add a function to set the value of an advice type

[Ben Boeckel]

This is setup for removing the `advice` global variables.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 advice.c | 5 +++++
 advice.h | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/advice.c b/advice.c
index 0b9c89c48a..fd58631dc1 100644
--- a/advice.c
+++ b/advice.c
@@ -187,6 +187,11 @@ int advice_enabled(enum advice_type type)
 	}
 }
 
+void advice_set(enum advice_type type, int value)
+{
+	advice_setting[type].enabled = value;
+}
+
 void advise_if_enabled(enum advice_type type, const char *advice, ...)
 {
 	va_list params;
diff --git a/advice.h b/advice.h
index bd26c385d0..74425a9f1a 100644
--- a/advice.h
+++ b/advice.h
@@ -87,6 +87,11 @@ void advise(const char *advice, ...);
  */
 int advice_enabled(enum advice_type type);
 
+/**
+ * Enable or disable advice of a certain kind.
+ */
+void advice_set(enum advice_type type, int value);
+
 /**
  * Checks the visibility of the advice before printing.
  */
-- 
2.31.1

Re: [PATCH v1 1/4] advice: add a function to set the value of an advice type

[Johannes Schindelin]

Hi Ben,

On Fri, 30 Jul 2021, Ben Boeckel wrote:

> This is setup for removing the `advice` global variables.

The idea to separate out this patch from the rest was probably to make
things easier to review.

However, without any callers of this function, I find it pretty hard to
review it.

My recommendation would be to move the addition of all the callers of this
function from 3/4 here (obviously without removing the assignments of the
global variables, that removal should stay with 3/4).

Ciao,
Dscho

>
> Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
> ---
>  advice.c | 5 +++++
>  advice.h | 5 +++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/advice.c b/advice.c
> index 0b9c89c48a..fd58631dc1 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -187,6 +187,11 @@ int advice_enabled(enum advice_type type)
>  	}
>  }
>
> +void advice_set(enum advice_type type, int value)
> +{
> +	advice_setting[type].enabled = value;
> +}
> +
>  void advise_if_enabled(enum advice_type type, const char *advice, ...)
>  {
>  	va_list params;
> diff --git a/advice.h b/advice.h
> index bd26c385d0..74425a9f1a 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -87,6 +87,11 @@ void advise(const char *advice, ...);
>   */
>  int advice_enabled(enum advice_type type);
>
> +/**
> + * Enable or disable advice of a certain kind.
> + */
> +void advice_set(enum advice_type type, int value);
> +
>  /**
>   * Checks the visibility of the advice before printing.
>   */
> --
> 2.31.1
>
>

[PATCH v1 2/4] advice: add enum variants for missing advice variables

[Ben Boeckel]

These were missed in their addition in 887a0fd573 (add: change advice
config variables used by the add API, 2020-02-06). All other global
variable settings have entries already.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 advice.c | 2 ++
 advice.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/advice.c b/advice.c
index fd58631dc1..fd9634aa4f 100644
--- a/advice.c
+++ b/advice.c
@@ -106,6 +106,8 @@ static struct {
 	int enabled;
 } advice_setting[] = {
 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
+	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
+	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile", 1 },
 	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
 	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName", 1 },
 	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge", 1 },
diff --git a/advice.h b/advice.h
index 74425a9f1a..101c4054b7 100644
--- a/advice.h
+++ b/advice.h
@@ -45,6 +45,8 @@ extern int advice_add_empty_pathspec;
  */
  enum advice_type {
 	ADVICE_ADD_EMBEDDED_REPO,
+	ADVICE_ADD_EMPTY_PATHSPEC,
+	ADVICE_ADD_IGNORED_FILE,
 	ADVICE_AM_WORK_DIR,
 	ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
 	ADVICE_COMMIT_BEFORE_MERGE,
-- 
2.31.1

Re: [PATCH v1 2/4] advice: add enum variants for missing advice variables

[Johannes Schindelin]

Hi Ben,

On Fri, 30 Jul 2021, Ben Boeckel wrote:

> These were missed in their addition in 887a0fd573 (add: change advice
> config variables used by the add API, 2020-02-06). All other global
> variable settings have entries already.

It took quite a bit of reading and looking through the `git log` history
to piece together what is going on here, and I wish the commit message
would have explained this better.

A big puzzlement came from the claim that "These were missed" is not only
missing a noun that clarifies what "These" are meant to be, but also from
the fact that `git grep advice_setting 887a0fd573` comes up empty. Which
suggests to me that nothing was missed there, but the problem lies with
`hw/advise-ng`, merged via c4a09cc9ccb (Merge branch 'hw/advise-ng',
2020-03-25), is based on v2.25.0, but was only merged after v2.26.0, which
contains daef1b300b0 (Merge branch 'hw/advice-add-nothing', 2020-02-14).

In other words, the addition of the two entries `addEmptyPathspec` and
`addIgnoredFile` happened in a diverging branch from the addition of the
`advice_setting` array, and the problem lies with the merge of the latter
into a branch that already had merged the former.

It would have helped me to read something along these lines:

	In daef1b300b0 (Merge branch 'hw/advice-add-nothing', 2020-02-14),
	two advice settings were introduced into the `advice_config`
	array.

	Subsequently, c4a09cc9ccb (Merge branch 'hw/advise-ng',
	2020-03-25) started to deprecate `advice_config` in favor of a new
	array, `advice_setting`.

	However, the latter branch did not include the former branch, and
	therefore `advice_setting` is missing the two entries added by the
	`hw/advice-add-nothing` branch.

	These are currently the only entries in `advice_config` missing
	from `advice_setting`.

FWIW I manually verified that last paragraph's claim.

The diff itself looks correct to me.

Ciao,
Dscho

Re: [PATCH v1 2/4] advice: add enum variants for missing advice variables

[Ben Boeckel]

On Mon, Aug 02, 2021 at 23:52:54 +0200, Johannes Schindelin wrote:
> On Fri, 30 Jul 2021, Ben Boeckel wrote:
> > These were missed in their addition in 887a0fd573 (add: change advice
> > config variables used by the add API, 2020-02-06). All other global
> > variable settings have entries already.
> 
> It took quite a bit of reading and looking through the `git log` history
> to piece together what is going on here, and I wish the commit message
> would have explained this better.
> 
> A big puzzlement came from the claim that "These were missed" is not only
> missing a noun that clarifies what "These" are meant to be, but also from
> the fact that `git grep advice_setting 887a0fd573` comes up empty. Which
> suggests to me that nothing was missed there, but the problem lies with
> `hw/advise-ng`, merged via c4a09cc9ccb (Merge branch 'hw/advise-ng',
> 2020-03-25), is based on v2.25.0, but was only merged after v2.26.0, which
> contains daef1b300b0 (Merge branch 'hw/advice-add-nothing', 2020-02-14).

Ah, I missed the logical merge conflict that was in effect here. I'll
add this to the commit message.

> In other words, the addition of the two entries `addEmptyPathspec` and
> `addIgnoredFile` happened in a diverging branch from the addition of the
> `advice_setting` array, and the problem lies with the merge of the latter
> into a branch that already had merged the former.
> 
> It would have helped me to read something along these lines:
> 
> 	In daef1b300b0 (Merge branch 'hw/advice-add-nothing', 2020-02-14),
> 	two advice settings were introduced into the `advice_config`
> 	array.
> 
> 	Subsequently, c4a09cc9ccb (Merge branch 'hw/advise-ng',
> 	2020-03-25) started to deprecate `advice_config` in favor of a new
> 	array, `advice_setting`.
> 
> 	However, the latter branch did not include the former branch, and
> 	therefore `advice_setting` is missing the two entries added by the
> 	`hw/advice-add-nothing` branch.
> 
> 	These are currently the only entries in `advice_config` missing
> 	from `advice_setting`.
> 
> FWIW I manually verified that last paragraph's claim.

I did as well :) ("All other global variable settings have entries
already."). I also verified the reverse, but that was going to be moot
with the other patches anyways. But having it called out as a separate
paragraph sounds better.

Thanks,

--Ben

[PATCH v1 3/4] advice: remove uses of global `advice_` variables

[Ben Boeckel]

There are now function APIs to access this information, so the global
variables are no longer needed to communicate their values.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 advice.c                    |  4 ++--
 branch.c                    |  2 +-
 builtin/add.c               |  8 ++++----
 builtin/am.c                |  2 +-
 builtin/checkout.c          |  6 +++---
 builtin/clone.c             |  2 +-
 builtin/commit.c            |  4 ++--
 builtin/fetch.c             |  2 +-
 builtin/merge.c             |  4 ++--
 builtin/push.c              | 12 ++++++------
 builtin/replace.c           |  2 +-
 builtin/reset.c             |  2 +-
 builtin/rm.c                |  2 +-
 builtin/submodule--helper.c |  2 +-
 commit.c                    |  2 +-
 editor.c                    |  2 +-
 notes-merge.c               |  2 +-
 object-name.c               |  2 +-
 remote.c                    | 12 ++++++------
 run-command.c               |  2 +-
 sequencer.c                 |  8 ++++----
 unpack-trees.c              | 18 +++++++++---------
 wt-status.c                 |  6 +++---
 23 files changed, 54 insertions(+), 54 deletions(-)

diff --git a/advice.c b/advice.c
index fd9634aa4f..ee4edc5e28 100644
--- a/advice.c
+++ b/advice.c
@@ -269,7 +269,7 @@ int error_resolve_conflict(const char *me)
 		error(_("It is not possible to %s because you have unmerged files."),
 			me);
 
-	if (advice_resolve_conflict)
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 		/*
 		 * Message used both when 'git commit' fails and when
 		 * other commands doing a merge do.
@@ -288,7 +288,7 @@ void NORETURN die_resolve_conflict(const char *me)
 void NORETURN die_conclude_merge(void)
 {
 	error(_("You have not concluded your merge (MERGE_HEAD exists)."));
-	if (advice_resolve_conflict)
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 		advise(_("Please, commit your changes before merging."));
 	die(_("Exiting because of unfinished merge."));
 }
diff --git a/branch.c b/branch.c
index 7a88a4861e..07a46430b3 100644
--- a/branch.c
+++ b/branch.c
@@ -271,7 +271,7 @@ void create_branch(struct repository *r,
 	real_ref = NULL;
 	if (get_oid_mb(start_name, &oid)) {
 		if (explicit_tracking) {
-			if (advice_set_upstream_failure) {
+			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
 				error(_(upstream_missing), start_name);
 				advise(_(upstream_advice));
 				exit(1);
diff --git a/builtin/add.c b/builtin/add.c
index 09e684585d..3d1fd64294 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -430,10 +430,10 @@ static void check_embedded_repo(const char *path)
 	strbuf_strip_suffix(&name, "/");
 
 	warning(_("adding embedded git repository: %s"), name.buf);
-	if (advice_add_embedded_repo) {
+	if (advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {
 		advise(embedded_advice, name.buf, name.buf);
 		/* there may be multiple entries; advise only once */
-		advice_add_embedded_repo = 0;
+		advice_set(ADVICE_ADD_EMBEDDED_REPO, 0);
 	}
 
 	strbuf_release(&name);
@@ -447,7 +447,7 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		if (advice_add_ignored_file)
+		if (advice_enabled(ADVICE_ADD_IGNORED_FILE))
 			advise(_("Use -f if you really want to add them.\n"
 				"Turn this message off by running\n"
 				"\"git config advice.addIgnoredFile false\""));
@@ -553,7 +553,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		if (advice_add_empty_pathspec)
+		if (advice_enabled(ADVICE_ADD_EMPTY_PATHSPEC))
 			advise( _("Maybe you wanted to say 'git add .'?\n"
 				"Turn this message off by running\n"
 				"\"git config advice.addEmptyPathspec false\""));
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81..040435de8f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1819,7 +1819,7 @@ static void am_run(struct am_state *state, int resume)
 			printf_ln(_("Patch failed at %s %.*s"), msgnum(state),
 				linelen(state->msg), state->msg);
 
-			if (advice_amworkdir)
+			if (advice_enabled(ADVICE_AM_WORK_DIR))
 				advise(_("Use 'git am --show-current-patch=diff' to see the failed patch"));
 
 			die_user_resolve(state);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f4cd7747d3..6d696cf304 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -923,7 +923,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);
 		if (!opts->quiet) {
 			if (old_branch_info->path &&
-			    advice_detached_head && !opts->force_detach)
+			    advice_enabled(ADVICE_DETACHED_HEAD) && !opts->force_detach)
 				detach_advice(new_branch_info->name);
 			describe_detached_head(_("HEAD is now at"), new_branch_info->commit);
 		}
@@ -1016,7 +1016,7 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)
 		sb.buf);
 	strbuf_release(&sb);
 
-	if (advice_detached_head)
+	if (advice_enabled(ADVICE_DETACHED_HEAD))
 		fprintf(stderr,
 			Q_(
 			/* The singular version */
@@ -1187,7 +1187,7 @@ static const char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_checkout_ambiguous_remote_branch_name) {
+	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
 		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
 			     "you can do so by fully qualifying the name with the --track option:\n"
 			     "\n"
diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c..c1603aa82b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -786,7 +786,7 @@ static int checkout(int submodule_progress)
 		return 0;
 	}
 	if (!strcmp(head, "HEAD")) {
-		if (advice_detached_head)
+		if (advice_enabled(ADVICE_DETACHED_HEAD))
 			detach_advice(oid_to_hex(&oid));
 		FREE_AND_NULL(head);
 	} else {
diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43..9566b030af 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -203,7 +203,7 @@ static void status_init_config(struct wt_status *s, config_fn_t fn)
 	init_diff_ui_defaults();
 	git_config(fn, s);
 	determine_whence(s);
-	s->hints = advice_status_hints; /* must come after git_config() */
+	s->hints = advice_enabled(ADVICE_STATUS_HINTS); /* must come after git_config() */
 }
 
 static void rollback_index_files(void)
@@ -1026,7 +1026,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	if (!committable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
-		s->hints = advice_status_hints;
+		s->hints = advice_enabled(ADVICE_STATUS_HINTS);
 		s->display_comment_prefix = old_display_comment_prefix;
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 25740c13df..2501e1d10d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1229,7 +1229,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		      " 'git remote prune %s' to remove any old, conflicting "
 		      "branches"), remote_name);
 
-	if (advice_fetch_show_forced_updates) {
+	if (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {
 		if (!fetch_show_forced_updates) {
 			warning(_(warn_show_forced_updates));
 		} else if (forced_updates_ms > FORCED_UPDATES_DELAY_WARNING_IN_MS) {
diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f5..2efb16626c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1367,14 +1367,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * There is no unmerged entry, don't advise 'git
 		 * add/rm <file>', just 'git commit'.
 		 */
-		if (advice_resolve_conflict)
+		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 			die(_("You have not concluded your merge (MERGE_HEAD exists).\n"
 				  "Please, commit your changes before you merge."));
 		else
 			die(_("You have not concluded your merge (MERGE_HEAD exists)."));
 	}
 	if (ref_exists("CHERRY_PICK_HEAD")) {
-		if (advice_resolve_conflict)
+		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 			die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n"
 			    "Please, commit your changes before you merge."));
 		else
diff --git a/builtin/push.c b/builtin/push.c
index e8b10a9b7e..4b026ce6c6 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -289,42 +289,42 @@ static const char message_advice_ref_needs_update[] =
 
 static void advise_pull_before_push(void)
 {
-	if (!advice_push_non_ff_current || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NON_FF_CURRENT) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_pull_before_push));
 }
 
 static void advise_checkout_pull_push(void)
 {
-	if (!advice_push_non_ff_matching || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NON_FF_MATCHING) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_checkout_pull_push));
 }
 
 static void advise_ref_already_exists(void)
 {
-	if (!advice_push_already_exists || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_ALREADY_EXISTS) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_already_exists));
 }
 
 static void advise_ref_fetch_first(void)
 {
-	if (!advice_push_fetch_first || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_FETCH_FIRST) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_fetch_first));
 }
 
 static void advise_ref_needs_force(void)
 {
-	if (!advice_push_needs_force || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NEEDS_FORCE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_needs_force));
 }
 
 static void advise_ref_needs_update(void)
 {
-	if (!advice_push_ref_needs_update || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_REF_NEEDS_UPDATE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_needs_update));
 }
diff --git a/builtin/replace.c b/builtin/replace.c
index cd48765911..1340878021 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -507,7 +507,7 @@ static int convert_graft_file(int force)
 	if (!fp)
 		return -1;
 
-	advice_graft_file_deprecated = 0;
+	advice_set(ADVICE_GRAFT_FILE_DEPRECATED, 0);
 	while (strbuf_getline(&buf, fp) != EOF) {
 		if (*buf.buf == '#')
 			continue;
diff --git a/builtin/reset.c b/builtin/reset.c
index 43e855cb88..51c9e2f43f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -412,7 +412,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_reset_quiet_warning && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
 					printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset.  You can\n"
 						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
 						"to make this the default.\n"), t_delta_in_ms / 1000.0);
diff --git a/builtin/rm.c b/builtin/rm.c
index 8a24c715e0..3b44b807e5 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -55,7 +55,7 @@ static void print_error_files(struct string_list *files_list,
 			strbuf_addf(&err_msg,
 				    "\n    %s",
 				    files_list->items[i].string);
-		if (advice_rm_hints)
+		if (advice_enabled(ADVICE_RM_HINTS))
 			strbuf_addstr(&err_msg, hints_msg);
 		*errs = error("%s", err_msg.buf);
 		strbuf_release(&err_msg);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f73963ad67..e5a5dc6903 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1747,7 +1747,7 @@ static int add_possible_reference_from_superproject(
 		} else {
 			switch (sas->error_mode) {
 			case SUBMODULE_ALTERNATE_ERROR_DIE:
-				if (advice_submodule_alternate_error_strategy_die)
+				if (advice_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE))
 					advise(_(alternate_error_advice));
 				die(_("submodule '%s' cannot add alternate: %s"),
 				    sas->submodule_name, err.buf);
diff --git a/commit.c b/commit.c
index 143f472c0f..17df05dce3 100644
--- a/commit.c
+++ b/commit.c
@@ -190,7 +190,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
-	if (advice_graft_file_deprecated)
+	if (advice_enabled(ADVICE_GRAFT_FILE_DEPRECATED))
 		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
 			 "and will be removed in a future Git version.\n"
 			 "\n"
diff --git a/editor.c b/editor.c
index 6303ae0ab0..fdd3eeafa9 100644
--- a/editor.c
+++ b/editor.c
@@ -58,7 +58,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
 		int ret, sig;
-		int print_waiting_for_editor = advice_waiting_for_editor && isatty(2);
+		int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);
 
 		if (print_waiting_for_editor) {
 			/*
diff --git a/notes-merge.c b/notes-merge.c
index 46c1f7c7f1..b4a3a903e8 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -273,7 +273,7 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
 		 */
 		if (file_exists(git_path(NOTES_MERGE_WORKTREE)) &&
 		    !is_empty_dir(git_path(NOTES_MERGE_WORKTREE))) {
-			if (advice_resolve_conflict)
+			if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 				die(_("You have not concluded your previous "
 				    "notes merge (%s exists).\nPlease, use "
 				    "'git notes merge --commit' or 'git notes "
diff --git a/object-name.c b/object-name.c
index 64202de60b..23758190ed 100644
--- a/object-name.c
+++ b/object-name.c
@@ -812,7 +812,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 			refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref, 0);
 			if (refs_found > 0) {
 				warning(warn_msg, len, str);
-				if (advice_object_name_warning)
+				if (advice_enabled(ADVICE_OBJECT_NAME_WARNING))
 					fprintf(stderr, "%s\n", _(object_name_msg));
 			}
 			free(real_ref);
diff --git a/remote.c b/remote.c
index dfb863d808..4ef53a6e30 100644
--- a/remote.c
+++ b/remote.c
@@ -1111,7 +1111,7 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 		"Neither worked, so we gave up. You must fully qualify the ref."),
 	      dst_value, matched_src_name);
 
-	if (!advice_push_unqualified_ref_name)
+	if (!advice_enabled(ADVICE_PUSH_UNQUALIFIED_REF_NAME))
 		return;
 
 	if (get_oid(matched_src_name, &oid))
@@ -2118,7 +2118,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 		strbuf_addf(sb,
 			_("Your branch is based on '%s', but the upstream is gone.\n"),
 			base);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git branch --unset-upstream\" to fixup)\n"));
 	} else if (!sti) {
@@ -2129,7 +2129,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 		strbuf_addf(sb,
 			    _("Your branch and '%s' refer to different commits.\n"),
 			    base);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
 				    "git status --ahead-behind");
 	} else if (!theirs) {
@@ -2138,7 +2138,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			   "Your branch is ahead of '%s' by %d commits.\n",
 			   ours),
 			base, ours);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git push\" to publish your local commits)\n"));
 	} else if (!ours) {
@@ -2149,7 +2149,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			       "and can be fast-forwarded.\n",
 			   theirs),
 			base, theirs);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git pull\" to update your local branch)\n"));
 	} else {
@@ -2162,7 +2162,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			       "respectively.\n",
 			   ours + theirs),
 			base, ours, theirs);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git pull\" to merge the remote branch into yours)\n"));
 	}
diff --git a/run-command.c b/run-command.c
index f72e72cce7..28e3eb5732 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1336,7 +1336,7 @@ const char *find_hook(const char *name)
 			err = errno;
 #endif
 
-		if (err == EACCES && advice_ignored_hook) {
+		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
 			static struct string_list advise_given = STRING_LIST_INIT_DUP;
 
 			if (!string_list_lookup(&advise_given, name)) {
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38..e0b4c86c1a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -486,7 +486,7 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
 	error(_("your local changes would be overwritten by %s."),
 		_(action_name(opts)));
 
-	if (advice_commit_before_merge)
+	if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE))
 		advise(_("commit your changes or stash them to proceed."));
 	return -1;
 }
@@ -1293,7 +1293,7 @@ void print_commit_summary(struct repository *r,
 	if (!committer_ident_sufficiently_given()) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
-		if (advice_implicit_identity) {
+		if (advice_enabled(ADVICE_IMPLICIT_IDENTITY)) {
 			strbuf_addch(&format, '\n');
 			strbuf_addstr(&format, implicit_ident_advice());
 		}
@@ -3041,7 +3041,7 @@ static int create_seq_dir(struct repository *r)
 	}
 	if (in_progress_error) {
 		error("%s", in_progress_error);
-		if (advice_sequencer_in_use)
+		if (advice_enabled(ADVICE_SEQUENCER_IN_USE))
 			advise(in_progress_advice,
 				advise_skip ? "--skip | " : "");
 		return -1;
@@ -3245,7 +3245,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
 give_advice:
 	error(_("there is nothing to skip"));
 
-	if (advice_resolve_conflict) {
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT)) {
 		advise(_("have you committed already?\n"
 			 "try \"git %s --continue\""),
 			 action == REPLAY_REVERT ? "revert" : "cherry-pick");
diff --git a/unpack-trees.c b/unpack-trees.c
index f88a69f8e7..eb5015623b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -111,17 +111,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 	strvec_init(&opts->msgs_to_free);
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
 			  "Please commit your changes or stash them before you switch branches.")
 		      : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by merge:\n%%s"
 			  "Please commit your changes or stash them before you merge.")
 		      : _("Your local changes to the following files would be overwritten by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by %s:\n%%s"
 			  "Please commit your changes or stash them before you %s.")
 		      : _("Your local changes to the following files would be overwritten by %s:\n%%s");
@@ -132,17 +132,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		_("Updating the following directories would lose untracked files in them:\n%s");
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by checkout:\n%%s"
 			  "Please move or remove them before you switch branches.")
 		      : _("The following untracked working tree files would be removed by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by merge:\n%%s"
 			  "Please move or remove them before you merge.")
 		      : _("The following untracked working tree files would be removed by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by %s:\n%%s"
 			  "Please move or remove them before you %s.")
 		      : _("The following untracked working tree files would be removed by %s:\n%%s");
@@ -150,17 +150,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		strvec_pushf(&opts->msgs_to_free, msg, cmd, cmd);
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by checkout:\n%%s"
 			  "Please move or remove them before you switch branches.")
 		      : _("The following untracked working tree files would be overwritten by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by merge:\n%%s"
 			  "Please move or remove them before you merge.")
 		      : _("The following untracked working tree files would be overwritten by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by %s:\n%%s"
 			  "Please move or remove them before you %s.")
 		      : _("The following untracked working tree files would be overwritten by %s:\n%%s");
diff --git a/wt-status.c b/wt-status.c
index c0dbf96749..1850251d8c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -736,7 +736,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
 
 	dir_clear(&dir);
 
-	if (advice_status_u_option)
+	if (advice_enabled(ADVICE_STATUS_U_OPTION))
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
@@ -1107,7 +1107,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
 	if (!format_tracking_info(branch, &sb, s->ahead_behind_flags))
 		return;
 
-	if (advice_status_ahead_behind_warning &&
+	if (advice_enabled(ADVICE_STATUS_AHEAD_BEHIND_WARNING) &&
 	    s->ahead_behind_flags == AHEAD_BEHIND_FULL) {
 		uint64_t t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
 		if (t_delta_in_ms > AB_DELAY_WARNING_IN_MS) {
@@ -1786,7 +1786,7 @@ static void wt_longstatus_print(struct wt_status *s)
 		wt_longstatus_print_other(s, &s->untracked, _("Untracked files"), "add");
 		if (s->show_ignored_mode)
 			wt_longstatus_print_other(s, &s->ignored, _("Ignored files"), "add -f");
-		if (advice_status_u_option && 2000 < s->untracked_in_ms) {
+		if (advice_enabled(ADVICE_STATUS_U_OPTION) && 2000 < s->untracked_in_ms) {
 			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 					 _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n"
-- 
2.31.1

Re: [PATCH v1 3/4] advice: remove uses of global `advice_` variables

[Johannes Schindelin]

Hi Ben,

On Fri, 30 Jul 2021, Ben Boeckel wrote:

> There are now function APIs to access this information, so the global
> variables are no longer needed to communicate their values.

This commit message implies that the reader remembers that `hw/advise-ng`
introduced a new advice API with the express intent to eventually replace
those global `advice_*` variables.

However, it is not the responsibility of the reader to remember that. It
is the responsibility of the commit message to describe this (so that the
reader can either remember it, or learn about it in the first place).

Now, as for the diff, I can guess just how tedious all of the
semi-repetitive `advice_*` -> `advice_enabled(ADVICE_*)` changes were from
trying to verify that they are all correct.

Big thank you for that!

However, even reading through the diff the second time, I managed to miss
the subtlety that there were two `advice_set()` calls strewn in.

As I pointed out in my review of patch 1/4, I would much prefer to have
the addition of those callers in 1/4 along with the introduction of said
function.

However, now that I write this, I would like to correct my advice (pun
intended) from 1/4 to leave the removal of the assignment of the global
`advice_*` variables here: It would make much more sense to move them to
patch 4/4.

Thanks,
Dscho

>
> Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
> ---
>  advice.c                    |  4 ++--
>  branch.c                    |  2 +-
>  builtin/add.c               |  8 ++++----
>  builtin/am.c                |  2 +-
>  builtin/checkout.c          |  6 +++---
>  builtin/clone.c             |  2 +-
>  builtin/commit.c            |  4 ++--
>  builtin/fetch.c             |  2 +-
>  builtin/merge.c             |  4 ++--
>  builtin/push.c              | 12 ++++++------
>  builtin/replace.c           |  2 +-
>  builtin/reset.c             |  2 +-
>  builtin/rm.c                |  2 +-
>  builtin/submodule--helper.c |  2 +-
>  commit.c                    |  2 +-
>  editor.c                    |  2 +-
>  notes-merge.c               |  2 +-
>  object-name.c               |  2 +-
>  remote.c                    | 12 ++++++------
>  run-command.c               |  2 +-
>  sequencer.c                 |  8 ++++----
>  unpack-trees.c              | 18 +++++++++---------
>  wt-status.c                 |  6 +++---
>  23 files changed, 54 insertions(+), 54 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index fd9634aa4f..ee4edc5e28 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -269,7 +269,7 @@ int error_resolve_conflict(const char *me)
>  		error(_("It is not possible to %s because you have unmerged files."),
>  			me);
>
> -	if (advice_resolve_conflict)
> +	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
>  		/*
>  		 * Message used both when 'git commit' fails and when
>  		 * other commands doing a merge do.
> @@ -288,7 +288,7 @@ void NORETURN die_resolve_conflict(const char *me)
>  void NORETURN die_conclude_merge(void)
>  {
>  	error(_("You have not concluded your merge (MERGE_HEAD exists)."));
> -	if (advice_resolve_conflict)
> +	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
>  		advise(_("Please, commit your changes before merging."));
>  	die(_("Exiting because of unfinished merge."));
>  }
> diff --git a/branch.c b/branch.c
> index 7a88a4861e..07a46430b3 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -271,7 +271,7 @@ void create_branch(struct repository *r,
>  	real_ref = NULL;
>  	if (get_oid_mb(start_name, &oid)) {
>  		if (explicit_tracking) {
> -			if (advice_set_upstream_failure) {
> +			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
>  				error(_(upstream_missing), start_name);
>  				advise(_(upstream_advice));
>  				exit(1);
> diff --git a/builtin/add.c b/builtin/add.c
> index 09e684585d..3d1fd64294 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -430,10 +430,10 @@ static void check_embedded_repo(const char *path)
>  	strbuf_strip_suffix(&name, "/");
>
>  	warning(_("adding embedded git repository: %s"), name.buf);
> -	if (advice_add_embedded_repo) {
> +	if (advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {
>  		advise(embedded_advice, name.buf, name.buf);
>  		/* there may be multiple entries; advise only once */
> -		advice_add_embedded_repo = 0;
> +		advice_set(ADVICE_ADD_EMBEDDED_REPO, 0);
>  	}
>
>  	strbuf_release(&name);
> @@ -447,7 +447,7 @@ static int add_files(struct dir_struct *dir, int flags)
>  		fprintf(stderr, _(ignore_error));
>  		for (i = 0; i < dir->ignored_nr; i++)
>  			fprintf(stderr, "%s\n", dir->ignored[i]->name);
> -		if (advice_add_ignored_file)
> +		if (advice_enabled(ADVICE_ADD_IGNORED_FILE))
>  			advise(_("Use -f if you really want to add them.\n"
>  				"Turn this message off by running\n"
>  				"\"git config advice.addIgnoredFile false\""));
> @@ -553,7 +553,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>
>  	if (require_pathspec && pathspec.nr == 0) {
>  		fprintf(stderr, _("Nothing specified, nothing added.\n"));
> -		if (advice_add_empty_pathspec)
> +		if (advice_enabled(ADVICE_ADD_EMPTY_PATHSPEC))
>  			advise( _("Maybe you wanted to say 'git add .'?\n"
>  				"Turn this message off by running\n"
>  				"\"git config advice.addEmptyPathspec false\""));
> diff --git a/builtin/am.c b/builtin/am.c
> index 0b2d886c81..040435de8f 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1819,7 +1819,7 @@ static void am_run(struct am_state *state, int resume)
>  			printf_ln(_("Patch failed at %s %.*s"), msgnum(state),
>  				linelen(state->msg), state->msg);
>
> -			if (advice_amworkdir)
> +			if (advice_enabled(ADVICE_AM_WORK_DIR))
>  				advise(_("Use 'git am --show-current-patch=diff' to see the failed patch"));
>
>  			die_user_resolve(state);
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f4cd7747d3..6d696cf304 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -923,7 +923,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);
>  		if (!opts->quiet) {
>  			if (old_branch_info->path &&
> -			    advice_detached_head && !opts->force_detach)
> +			    advice_enabled(ADVICE_DETACHED_HEAD) && !opts->force_detach)
>  				detach_advice(new_branch_info->name);
>  			describe_detached_head(_("HEAD is now at"), new_branch_info->commit);
>  		}
> @@ -1016,7 +1016,7 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)
>  		sb.buf);
>  	strbuf_release(&sb);
>
> -	if (advice_detached_head)
> +	if (advice_enabled(ADVICE_DETACHED_HEAD))
>  		fprintf(stderr,
>  			Q_(
>  			/* The singular version */
> @@ -1187,7 +1187,7 @@ static const char *parse_remote_branch(const char *arg,
>  	}
>
>  	if (!remote && num_matches > 1) {
> -	    if (advice_checkout_ambiguous_remote_branch_name) {
> +	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
>  		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
>  			     "you can do so by fully qualifying the name with the --track option:\n"
>  			     "\n"
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 66fe66679c..c1603aa82b 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -786,7 +786,7 @@ static int checkout(int submodule_progress)
>  		return 0;
>  	}
>  	if (!strcmp(head, "HEAD")) {
> -		if (advice_detached_head)
> +		if (advice_enabled(ADVICE_DETACHED_HEAD))
>  			detach_advice(oid_to_hex(&oid));
>  		FREE_AND_NULL(head);
>  	} else {
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 190d215d43..9566b030af 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -203,7 +203,7 @@ static void status_init_config(struct wt_status *s, config_fn_t fn)
>  	init_diff_ui_defaults();
>  	git_config(fn, s);
>  	determine_whence(s);
> -	s->hints = advice_status_hints; /* must come after git_config() */
> +	s->hints = advice_enabled(ADVICE_STATUS_HINTS); /* must come after git_config() */
>  }
>
>  static void rollback_index_files(void)
> @@ -1026,7 +1026,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	 */
>  	if (!committable && whence != FROM_MERGE && !allow_empty &&
>  	    !(amend && is_a_merge(current_head))) {
> -		s->hints = advice_status_hints;
> +		s->hints = advice_enabled(ADVICE_STATUS_HINTS);
>  		s->display_comment_prefix = old_display_comment_prefix;
>  		run_status(stdout, index_file, prefix, 0, s);
>  		if (amend)
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 25740c13df..2501e1d10d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1229,7 +1229,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  		      " 'git remote prune %s' to remove any old, conflicting "
>  		      "branches"), remote_name);
>
> -	if (advice_fetch_show_forced_updates) {
> +	if (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {
>  		if (!fetch_show_forced_updates) {
>  			warning(_(warn_show_forced_updates));
>  		} else if (forced_updates_ms > FORCED_UPDATES_DELAY_WARNING_IN_MS) {
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a8a843b1f5..2efb16626c 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1367,14 +1367,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		 * There is no unmerged entry, don't advise 'git
>  		 * add/rm <file>', just 'git commit'.
>  		 */
> -		if (advice_resolve_conflict)
> +		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
>  			die(_("You have not concluded your merge (MERGE_HEAD exists).\n"
>  				  "Please, commit your changes before you merge."));
>  		else
>  			die(_("You have not concluded your merge (MERGE_HEAD exists)."));
>  	}
>  	if (ref_exists("CHERRY_PICK_HEAD")) {
> -		if (advice_resolve_conflict)
> +		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
>  			die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n"
>  			    "Please, commit your changes before you merge."));
>  		else
> diff --git a/builtin/push.c b/builtin/push.c
> index e8b10a9b7e..4b026ce6c6 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -289,42 +289,42 @@ static const char message_advice_ref_needs_update[] =
>
>  static void advise_pull_before_push(void)
>  {
> -	if (!advice_push_non_ff_current || !advice_push_update_rejected)
> +	if (!advice_enabled(ADVICE_PUSH_NON_FF_CURRENT) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
>  		return;
>  	advise(_(message_advice_pull_before_push));
>  }
>
>  static void advise_checkout_pull_push(void)
>  {
> -	if (!advice_push_non_ff_matching || !advice_push_update_rejected)
> +	if (!advice_enabled(ADVICE_PUSH_NON_FF_MATCHING) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
>  		return;
>  	advise(_(message_advice_checkout_pull_push));
>  }
>
>  static void advise_ref_already_exists(void)
>  {
> -	if (!advice_push_already_exists || !advice_push_update_rejected)
> +	if (!advice_enabled(ADVICE_PUSH_ALREADY_EXISTS) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
>  		return;
>  	advise(_(message_advice_ref_already_exists));
>  }
>
>  static void advise_ref_fetch_first(void)
>  {
> -	if (!advice_push_fetch_first || !advice_push_update_rejected)
> +	if (!advice_enabled(ADVICE_PUSH_FETCH_FIRST) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
>  		return;
>  	advise(_(message_advice_ref_fetch_first));
>  }
>
>  static void advise_ref_needs_force(void)
>  {
> -	if (!advice_push_needs_force || !advice_push_update_rejected)
> +	if (!advice_enabled(ADVICE_PUSH_NEEDS_FORCE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
>  		return;
>  	advise(_(message_advice_ref_needs_force));
>  }
>
>  static void advise_ref_needs_update(void)
>  {
> -	if (!advice_push_ref_needs_update || !advice_push_update_rejected)
> +	if (!advice_enabled(ADVICE_PUSH_REF_NEEDS_UPDATE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
>  		return;
>  	advise(_(message_advice_ref_needs_update));
>  }
> diff --git a/builtin/replace.c b/builtin/replace.c
> index cd48765911..1340878021 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -507,7 +507,7 @@ static int convert_graft_file(int force)
>  	if (!fp)
>  		return -1;
>
> -	advice_graft_file_deprecated = 0;
> +	advice_set(ADVICE_GRAFT_FILE_DEPRECATED, 0);
>  	while (strbuf_getline(&buf, fp) != EOF) {
>  		if (*buf.buf == '#')
>  			continue;
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 43e855cb88..51c9e2f43f 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -412,7 +412,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  				refresh_index(&the_index, flags, NULL, NULL,
>  					      _("Unstaged changes after reset:"));
>  				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
> -				if (advice_reset_quiet_warning && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
> +				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
>  					printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset.  You can\n"
>  						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
>  						"to make this the default.\n"), t_delta_in_ms / 1000.0);
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 8a24c715e0..3b44b807e5 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -55,7 +55,7 @@ static void print_error_files(struct string_list *files_list,
>  			strbuf_addf(&err_msg,
>  				    "\n    %s",
>  				    files_list->items[i].string);
> -		if (advice_rm_hints)
> +		if (advice_enabled(ADVICE_RM_HINTS))
>  			strbuf_addstr(&err_msg, hints_msg);
>  		*errs = error("%s", err_msg.buf);
>  		strbuf_release(&err_msg);
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f73963ad67..e5a5dc6903 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1747,7 +1747,7 @@ static int add_possible_reference_from_superproject(
>  		} else {
>  			switch (sas->error_mode) {
>  			case SUBMODULE_ALTERNATE_ERROR_DIE:
> -				if (advice_submodule_alternate_error_strategy_die)
> +				if (advice_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE))
>  					advise(_(alternate_error_advice));
>  				die(_("submodule '%s' cannot add alternate: %s"),
>  				    sas->submodule_name, err.buf);
> diff --git a/commit.c b/commit.c
> index 143f472c0f..17df05dce3 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -190,7 +190,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)
>  	struct strbuf buf = STRBUF_INIT;
>  	if (!fp)
>  		return -1;
> -	if (advice_graft_file_deprecated)
> +	if (advice_enabled(ADVICE_GRAFT_FILE_DEPRECATED))
>  		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
>  			 "and will be removed in a future Git version.\n"
>  			 "\n"
> diff --git a/editor.c b/editor.c
> index 6303ae0ab0..fdd3eeafa9 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -58,7 +58,7 @@ static int launch_specified_editor(const char *editor, const char *path,
>  		const char *args[] = { editor, NULL, NULL };
>  		struct child_process p = CHILD_PROCESS_INIT;
>  		int ret, sig;
> -		int print_waiting_for_editor = advice_waiting_for_editor && isatty(2);
> +		int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);
>
>  		if (print_waiting_for_editor) {
>  			/*
> diff --git a/notes-merge.c b/notes-merge.c
> index 46c1f7c7f1..b4a3a903e8 100644
> --- a/notes-merge.c
> +++ b/notes-merge.c
> @@ -273,7 +273,7 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
>  		 */
>  		if (file_exists(git_path(NOTES_MERGE_WORKTREE)) &&
>  		    !is_empty_dir(git_path(NOTES_MERGE_WORKTREE))) {
> -			if (advice_resolve_conflict)
> +			if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
>  				die(_("You have not concluded your previous "
>  				    "notes merge (%s exists).\nPlease, use "
>  				    "'git notes merge --commit' or 'git notes "
> diff --git a/object-name.c b/object-name.c
> index 64202de60b..23758190ed 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -812,7 +812,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
>  			refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref, 0);
>  			if (refs_found > 0) {
>  				warning(warn_msg, len, str);
> -				if (advice_object_name_warning)
> +				if (advice_enabled(ADVICE_OBJECT_NAME_WARNING))
>  					fprintf(stderr, "%s\n", _(object_name_msg));
>  			}
>  			free(real_ref);
> diff --git a/remote.c b/remote.c
> index dfb863d808..4ef53a6e30 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1111,7 +1111,7 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
>  		"Neither worked, so we gave up. You must fully qualify the ref."),
>  	      dst_value, matched_src_name);
>
> -	if (!advice_push_unqualified_ref_name)
> +	if (!advice_enabled(ADVICE_PUSH_UNQUALIFIED_REF_NAME))
>  		return;
>
>  	if (get_oid(matched_src_name, &oid))
> @@ -2118,7 +2118,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
>  		strbuf_addf(sb,
>  			_("Your branch is based on '%s', but the upstream is gone.\n"),
>  			base);
> -		if (advice_status_hints)
> +		if (advice_enabled(ADVICE_STATUS_HINTS))
>  			strbuf_addstr(sb,
>  				_("  (use \"git branch --unset-upstream\" to fixup)\n"));
>  	} else if (!sti) {
> @@ -2129,7 +2129,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
>  		strbuf_addf(sb,
>  			    _("Your branch and '%s' refer to different commits.\n"),
>  			    base);
> -		if (advice_status_hints)
> +		if (advice_enabled(ADVICE_STATUS_HINTS))
>  			strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
>  				    "git status --ahead-behind");
>  	} else if (!theirs) {
> @@ -2138,7 +2138,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
>  			   "Your branch is ahead of '%s' by %d commits.\n",
>  			   ours),
>  			base, ours);
> -		if (advice_status_hints)
> +		if (advice_enabled(ADVICE_STATUS_HINTS))
>  			strbuf_addstr(sb,
>  				_("  (use \"git push\" to publish your local commits)\n"));
>  	} else if (!ours) {
> @@ -2149,7 +2149,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
>  			       "and can be fast-forwarded.\n",
>  			   theirs),
>  			base, theirs);
> -		if (advice_status_hints)
> +		if (advice_enabled(ADVICE_STATUS_HINTS))
>  			strbuf_addstr(sb,
>  				_("  (use \"git pull\" to update your local branch)\n"));
>  	} else {
> @@ -2162,7 +2162,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
>  			       "respectively.\n",
>  			   ours + theirs),
>  			base, ours, theirs);
> -		if (advice_status_hints)
> +		if (advice_enabled(ADVICE_STATUS_HINTS))
>  			strbuf_addstr(sb,
>  				_("  (use \"git pull\" to merge the remote branch into yours)\n"));
>  	}
> diff --git a/run-command.c b/run-command.c
> index f72e72cce7..28e3eb5732 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1336,7 +1336,7 @@ const char *find_hook(const char *name)
>  			err = errno;
>  #endif
>
> -		if (err == EACCES && advice_ignored_hook) {
> +		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
>  			static struct string_list advise_given = STRING_LIST_INIT_DUP;
>
>  			if (!string_list_lookup(&advise_given, name)) {
> diff --git a/sequencer.c b/sequencer.c
> index 0bec01cf38..e0b4c86c1a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -486,7 +486,7 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
>  	error(_("your local changes would be overwritten by %s."),
>  		_(action_name(opts)));
>
> -	if (advice_commit_before_merge)
> +	if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE))
>  		advise(_("commit your changes or stash them to proceed."));
>  	return -1;
>  }
> @@ -1293,7 +1293,7 @@ void print_commit_summary(struct repository *r,
>  	if (!committer_ident_sufficiently_given()) {
>  		strbuf_addstr(&format, "\n Committer: ");
>  		strbuf_addbuf_percentquote(&format, &committer_ident);
> -		if (advice_implicit_identity) {
> +		if (advice_enabled(ADVICE_IMPLICIT_IDENTITY)) {
>  			strbuf_addch(&format, '\n');
>  			strbuf_addstr(&format, implicit_ident_advice());
>  		}
> @@ -3041,7 +3041,7 @@ static int create_seq_dir(struct repository *r)
>  	}
>  	if (in_progress_error) {
>  		error("%s", in_progress_error);
> -		if (advice_sequencer_in_use)
> +		if (advice_enabled(ADVICE_SEQUENCER_IN_USE))
>  			advise(in_progress_advice,
>  				advise_skip ? "--skip | " : "");
>  		return -1;
> @@ -3245,7 +3245,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
>  give_advice:
>  	error(_("there is nothing to skip"));
>
> -	if (advice_resolve_conflict) {
> +	if (advice_enabled(ADVICE_RESOLVE_CONFLICT)) {
>  		advise(_("have you committed already?\n"
>  			 "try \"git %s --continue\""),
>  			 action == REPLAY_REVERT ? "revert" : "cherry-pick");
> diff --git a/unpack-trees.c b/unpack-trees.c
> index f88a69f8e7..eb5015623b 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -111,17 +111,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>  	strvec_init(&opts->msgs_to_free);
>
>  	if (!strcmp(cmd, "checkout"))
> -		msg = advice_commit_before_merge
> +		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>  		      ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
>  			  "Please commit your changes or stash them before you switch branches.")
>  		      : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
>  	else if (!strcmp(cmd, "merge"))
> -		msg = advice_commit_before_merge
> +		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>  		      ? _("Your local changes to the following files would be overwritten by merge:\n%%s"
>  			  "Please commit your changes or stash them before you merge.")
>  		      : _("Your local changes to the following files would be overwritten by merge:\n%%s");
>  	else
> -		msg = advice_commit_before_merge
> +		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>  		      ? _("Your local changes to the following files would be overwritten by %s:\n%%s"
>  			  "Please commit your changes or stash them before you %s.")
>  		      : _("Your local changes to the following files would be overwritten by %s:\n%%s");
> @@ -132,17 +132,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>  		_("Updating the following directories would lose untracked files in them:\n%s");
>
>  	if (!strcmp(cmd, "checkout"))
> -		msg = advice_commit_before_merge
> +		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>  		      ? _("The following untracked working tree files would be removed by checkout:\n%%s"
>  			  "Please move or remove them before you switch branches.")
>  		      : _("The following untracked working tree files would be removed by checkout:\n%%s");
>  	else if (!strcmp(cmd, "merge"))
> -		msg = advice_commit_before_merge
> +		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>  		      ? _("The following untracked working tree files would be removed by merge:\n%%s"
>  			  "Please move or remove them before you merge.")
>  		      : _("The following untracked working tree files would be removed by merge:\n%%s");
>  	else
> -		msg = advice_commit_before_merge
> +		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>  		      ? _("The following untracked working tree files would be removed by %s:\n%%s"
>  			  "Please move or remove them before you %s.")
>  		      : _("The following untracked working tree files would be removed by %s:\n%%s");
> @@ -150,17 +150,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>  		strvec_pushf(&opts->msgs_to_free, msg, cmd, cmd);
>
>  	if (!strcmp(cmd, "checkout"))
> -		msg = advice_commit_before_merge
> +		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>  		      ? _("The following untracked working tree files would be overwritten by checkout:\n%%s"
>  			  "Please move or remove them before you switch branches.")
>  		      : _("The following untracked working tree files would be overwritten by checkout:\n%%s");
>  	else if (!strcmp(cmd, "merge"))
> -		msg = advice_commit_before_merge
> +		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>  		      ? _("The following untracked working tree files would be overwritten by merge:\n%%s"
>  			  "Please move or remove them before you merge.")
>  		      : _("The following untracked working tree files would be overwritten by merge:\n%%s");
>  	else
> -		msg = advice_commit_before_merge
> +		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>  		      ? _("The following untracked working tree files would be overwritten by %s:\n%%s"
>  			  "Please move or remove them before you %s.")
>  		      : _("The following untracked working tree files would be overwritten by %s:\n%%s");
> diff --git a/wt-status.c b/wt-status.c
> index c0dbf96749..1850251d8c 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -736,7 +736,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
>
>  	dir_clear(&dir);
>
> -	if (advice_status_u_option)
> +	if (advice_enabled(ADVICE_STATUS_U_OPTION))
>  		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
>  }
>
> @@ -1107,7 +1107,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
>  	if (!format_tracking_info(branch, &sb, s->ahead_behind_flags))
>  		return;
>
> -	if (advice_status_ahead_behind_warning &&
> +	if (advice_enabled(ADVICE_STATUS_AHEAD_BEHIND_WARNING) &&
>  	    s->ahead_behind_flags == AHEAD_BEHIND_FULL) {
>  		uint64_t t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
>  		if (t_delta_in_ms > AB_DELAY_WARNING_IN_MS) {
> @@ -1786,7 +1786,7 @@ static void wt_longstatus_print(struct wt_status *s)
>  		wt_longstatus_print_other(s, &s->untracked, _("Untracked files"), "add");
>  		if (s->show_ignored_mode)
>  			wt_longstatus_print_other(s, &s->ignored, _("Ignored files"), "add -f");
> -		if (advice_status_u_option && 2000 < s->untracked_in_ms) {
> +		if (advice_enabled(ADVICE_STATUS_U_OPTION) && 2000 < s->untracked_in_ms) {
>  			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
>  			status_printf_ln(s, GIT_COLOR_NORMAL,
>  					 _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n"
> --
> 2.31.1
>
>

Re: [PATCH v1 3/4] advice: remove uses of global `advice_` variables

[Ben Boeckel]

On Tue, Aug 03, 2021 at 00:06:48 +0200, Johannes Schindelin wrote:
> On Fri, 30 Jul 2021, Ben Boeckel wrote:
> > There are now function APIs to access this information, so the global
> > variables are no longer needed to communicate their values.
> 
> This commit message implies that the reader remembers that `hw/advise-ng`
> introduced a new advice API with the express intent to eventually replace
> those global `advice_*` variables.
> 
> However, it is not the responsibility of the reader to remember that. It
> is the responsibility of the commit message to describe this (so that the
> reader can either remember it, or learn about it in the first place).

That makes sense, thanks. I guess I'm used to projects where I'm at
least peripherally aware of most of what's going on, but that's because
I work on cross-cutting concerns on them for the most part (build
systems, CI, software process).

> Now, as for the diff, I can guess just how tedious all of the
> semi-repetitive `advice_*` -> `advice_enabled(ADVICE_*)` changes were from
> trying to verify that they are all correct.

One of the times a case-insensitive word diff rendering would be handy.
(Then letting the compiler verify that the new side actually works.)

> Big thank you for that!

You can thank this Vim macro which made the tedious bits trivial:

    iadvice_enabled(<Esc>wgUwea)<Esc>]q

> However, even reading through the diff the second time, I managed to miss
> the subtlety that there were two `advice_set()` calls strewn in.
> 
> As I pointed out in my review of patch 1/4, I would much prefer to have
> the addition of those callers in 1/4 along with the introduction of said
> function.
> 
> However, now that I write this, I would like to correct my advice (pun
> intended) from 1/4 to leave the removal of the assignment of the global
> `advice_*` variables here: It would make much more sense to move them to
> patch 4/4.

Sounds good. I'll still keep it as a separate patch, but just have one
for the read-only side, then a new patch which adds the write API and
transforms the 2 write instances. The final patch can then stay the
same. In short:

  - 2/4 (read-only bits) -> v2 1/4
  - 3/4 -> v2 2/4
  - 1/4 + 3/4 (write bits) -> v2 3/4
  - 4/4 -> mostly the same

Thanks,

--Ben

[PATCH v1 4/4] advice: remove static global variables for advice tracking

[Ben Boeckel]

All of the preferences are now tracked as part of the `advice_setting`
array and all consumers of the global variables have been removed, so
the parallel tracking through `advice_config` is no longer necessary.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 advice.c | 77 --------------------------------------------------------
 advice.h | 31 -----------------------
 2 files changed, 108 deletions(-)

diff --git a/advice.c b/advice.c
index ee4edc5e28..6ba47f3f5e 100644
--- a/advice.c
+++ b/advice.c
@@ -4,37 +4,6 @@
 #include "help.h"
 #include "string-list.h"
 
-int advice_fetch_show_forced_updates = 1;
-int advice_push_update_rejected = 1;
-int advice_push_non_ff_current = 1;
-int advice_push_non_ff_matching = 1;
-int advice_push_already_exists = 1;
-int advice_push_fetch_first = 1;
-int advice_push_needs_force = 1;
-int advice_push_unqualified_ref_name = 1;
-int advice_push_ref_needs_update = 1;
-int advice_status_hints = 1;
-int advice_status_u_option = 1;
-int advice_status_ahead_behind_warning = 1;
-int advice_commit_before_merge = 1;
-int advice_reset_quiet_warning = 1;
-int advice_resolve_conflict = 1;
-int advice_sequencer_in_use = 1;
-int advice_implicit_identity = 1;
-int advice_detached_head = 1;
-int advice_set_upstream_failure = 1;
-int advice_object_name_warning = 1;
-int advice_amworkdir = 1;
-int advice_rm_hints = 1;
-int advice_add_embedded_repo = 1;
-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_submodule_alternate_error_strategy_die = 1;
-int advice_add_ignored_file = 1;
-int advice_add_empty_pathspec = 1;
-
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
@@ -62,45 +31,6 @@ static const char *advise_get_color(enum color_advice ix)
 	return "";
 }
 
-static struct {
-	const char *name;
-	int *preference;
-} advice_config[] = {
-	{ "fetchShowForcedUpdates", &advice_fetch_show_forced_updates },
-	{ "pushUpdateRejected", &advice_push_update_rejected },
-	{ "pushNonFFCurrent", &advice_push_non_ff_current },
-	{ "pushNonFFMatching", &advice_push_non_ff_matching },
-	{ "pushAlreadyExists", &advice_push_already_exists },
-	{ "pushFetchFirst", &advice_push_fetch_first },
-	{ "pushNeedsForce", &advice_push_needs_force },
-	{ "pushUnqualifiedRefName", &advice_push_unqualified_ref_name },
-	{ "pushRefNeedsUpdate", &advice_push_ref_needs_update },
-	{ "statusHints", &advice_status_hints },
-	{ "statusUoption", &advice_status_u_option },
-	{ "statusAheadBehindWarning", &advice_status_ahead_behind_warning },
-	{ "commitBeforeMerge", &advice_commit_before_merge },
-	{ "resetQuiet", &advice_reset_quiet_warning },
-	{ "resolveConflict", &advice_resolve_conflict },
-	{ "sequencerInUse", &advice_sequencer_in_use },
-	{ "implicitIdentity", &advice_implicit_identity },
-	{ "detachedHead", &advice_detached_head },
-	{ "setUpstreamFailure", &advice_set_upstream_failure },
-	{ "objectNameWarning", &advice_object_name_warning },
-	{ "amWorkDir", &advice_amworkdir },
-	{ "rmHints", &advice_rm_hints },
-	{ "addEmbeddedRepo", &advice_add_embedded_repo },
-	{ "ignoredHook", &advice_ignored_hook },
-	{ "waitingForEditor", &advice_waiting_for_editor },
-	{ "graftFileDeprecated", &advice_graft_file_deprecated },
-	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
-	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
-	{ "addIgnoredFile", &advice_add_ignored_file },
-	{ "addEmptyPathspec", &advice_add_empty_pathspec },
-
-	/* make this an alias for backward compatibility */
-	{ "pushNonFastForward", &advice_push_update_rejected }
-};
-
 static struct {
 	const char *key;
 	int enabled;
@@ -228,13 +158,6 @@ int git_default_advice_config(const char *var, const char *value)
 	if (!skip_prefix(var, "advice.", &k))
 		return 0;
 
-	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
-		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;
diff --git a/advice.h b/advice.h
index 101c4054b7..17ee5d3633 100644
--- a/advice.h
+++ b/advice.h
@@ -5,37 +5,6 @@
 
 struct string_list;
 
-extern int advice_fetch_show_forced_updates;
-extern int advice_push_update_rejected;
-extern int advice_push_non_ff_current;
-extern int advice_push_non_ff_matching;
-extern int advice_push_already_exists;
-extern int advice_push_fetch_first;
-extern int advice_push_needs_force;
-extern int advice_push_unqualified_ref_name;
-extern int advice_push_ref_needs_update;
-extern int advice_status_hints;
-extern int advice_status_u_option;
-extern int advice_status_ahead_behind_warning;
-extern int advice_commit_before_merge;
-extern int advice_reset_quiet_warning;
-extern int advice_resolve_conflict;
-extern int advice_sequencer_in_use;
-extern int advice_implicit_identity;
-extern int advice_detached_head;
-extern int advice_set_upstream_failure;
-extern int advice_object_name_warning;
-extern int advice_amworkdir;
-extern int advice_rm_hints;
-extern int advice_add_embedded_repo;
-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_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.
-- 
2.31.1

Re: [PATCH v1 4/4] advice: remove static global variables for advice tracking

[Johannes Schindelin]

Hi Ben,

On Fri, 30 Jul 2021, Ben Boeckel wrote:

> All of the preferences are now tracked as part of the `advice_setting`
> array and all consumers of the global variables have been removed, so
> the parallel tracking through `advice_config` is no longer necessary.

Maybe add "This concludes the move from the old advice API to the new one
introduced via c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25)"?

At least this reader would have found that helpful.

And as I mentioned in the review of patch 3/4, the removal of the
explicit assignment to the `advice_*` global variables should be moved
here.

Ciao,
Dscho

>
> Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
> ---
>  advice.c | 77 --------------------------------------------------------
>  advice.h | 31 -----------------------
>  2 files changed, 108 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index ee4edc5e28..6ba47f3f5e 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -4,37 +4,6 @@
>  #include "help.h"
>  #include "string-list.h"
>
> -int advice_fetch_show_forced_updates = 1;
> -int advice_push_update_rejected = 1;
> -int advice_push_non_ff_current = 1;
> -int advice_push_non_ff_matching = 1;
> -int advice_push_already_exists = 1;
> -int advice_push_fetch_first = 1;
> -int advice_push_needs_force = 1;
> -int advice_push_unqualified_ref_name = 1;
> -int advice_push_ref_needs_update = 1;
> -int advice_status_hints = 1;
> -int advice_status_u_option = 1;
> -int advice_status_ahead_behind_warning = 1;
> -int advice_commit_before_merge = 1;
> -int advice_reset_quiet_warning = 1;
> -int advice_resolve_conflict = 1;
> -int advice_sequencer_in_use = 1;
> -int advice_implicit_identity = 1;
> -int advice_detached_head = 1;
> -int advice_set_upstream_failure = 1;
> -int advice_object_name_warning = 1;
> -int advice_amworkdir = 1;
> -int advice_rm_hints = 1;
> -int advice_add_embedded_repo = 1;
> -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_submodule_alternate_error_strategy_die = 1;
> -int advice_add_ignored_file = 1;
> -int advice_add_empty_pathspec = 1;
> -
>  static int advice_use_color = -1;
>  static char advice_colors[][COLOR_MAXLEN] = {
>  	GIT_COLOR_RESET,
> @@ -62,45 +31,6 @@ static const char *advise_get_color(enum color_advice ix)
>  	return "";
>  }
>
> -static struct {
> -	const char *name;
> -	int *preference;
> -} advice_config[] = {
> -	{ "fetchShowForcedUpdates", &advice_fetch_show_forced_updates },
> -	{ "pushUpdateRejected", &advice_push_update_rejected },
> -	{ "pushNonFFCurrent", &advice_push_non_ff_current },
> -	{ "pushNonFFMatching", &advice_push_non_ff_matching },
> -	{ "pushAlreadyExists", &advice_push_already_exists },
> -	{ "pushFetchFirst", &advice_push_fetch_first },
> -	{ "pushNeedsForce", &advice_push_needs_force },
> -	{ "pushUnqualifiedRefName", &advice_push_unqualified_ref_name },
> -	{ "pushRefNeedsUpdate", &advice_push_ref_needs_update },
> -	{ "statusHints", &advice_status_hints },
> -	{ "statusUoption", &advice_status_u_option },
> -	{ "statusAheadBehindWarning", &advice_status_ahead_behind_warning },
> -	{ "commitBeforeMerge", &advice_commit_before_merge },
> -	{ "resetQuiet", &advice_reset_quiet_warning },
> -	{ "resolveConflict", &advice_resolve_conflict },
> -	{ "sequencerInUse", &advice_sequencer_in_use },
> -	{ "implicitIdentity", &advice_implicit_identity },
> -	{ "detachedHead", &advice_detached_head },
> -	{ "setUpstreamFailure", &advice_set_upstream_failure },
> -	{ "objectNameWarning", &advice_object_name_warning },
> -	{ "amWorkDir", &advice_amworkdir },
> -	{ "rmHints", &advice_rm_hints },
> -	{ "addEmbeddedRepo", &advice_add_embedded_repo },
> -	{ "ignoredHook", &advice_ignored_hook },
> -	{ "waitingForEditor", &advice_waiting_for_editor },
> -	{ "graftFileDeprecated", &advice_graft_file_deprecated },
> -	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
> -	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
> -	{ "addIgnoredFile", &advice_add_ignored_file },
> -	{ "addEmptyPathspec", &advice_add_empty_pathspec },
> -
> -	/* make this an alias for backward compatibility */
> -	{ "pushNonFastForward", &advice_push_update_rejected }
> -};
> -
>  static struct {
>  	const char *key;
>  	int enabled;
> @@ -228,13 +158,6 @@ int git_default_advice_config(const char *var, const char *value)
>  	if (!skip_prefix(var, "advice.", &k))
>  		return 0;
>
> -	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
> -		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;
> diff --git a/advice.h b/advice.h
> index 101c4054b7..17ee5d3633 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -5,37 +5,6 @@
>
>  struct string_list;
>
> -extern int advice_fetch_show_forced_updates;
> -extern int advice_push_update_rejected;
> -extern int advice_push_non_ff_current;
> -extern int advice_push_non_ff_matching;
> -extern int advice_push_already_exists;
> -extern int advice_push_fetch_first;
> -extern int advice_push_needs_force;
> -extern int advice_push_unqualified_ref_name;
> -extern int advice_push_ref_needs_update;
> -extern int advice_status_hints;
> -extern int advice_status_u_option;
> -extern int advice_status_ahead_behind_warning;
> -extern int advice_commit_before_merge;
> -extern int advice_reset_quiet_warning;
> -extern int advice_resolve_conflict;
> -extern int advice_sequencer_in_use;
> -extern int advice_implicit_identity;
> -extern int advice_detached_head;
> -extern int advice_set_upstream_failure;
> -extern int advice_object_name_warning;
> -extern int advice_amworkdir;
> -extern int advice_rm_hints;
> -extern int advice_add_embedded_repo;
> -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_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.
> --
> 2.31.1
>
>

Re: [PATCH v1 4/4] advice: remove static global variables for advice tracking

[Ben Boeckel]

On Tue, Aug 03, 2021 at 00:09:25 +0200, Johannes Schindelin wrote:
> On Fri, 30 Jul 2021, Ben Boeckel wrote:
> > All of the preferences are now tracked as part of the `advice_setting`
> > array and all consumers of the global variables have been removed, so
> > the parallel tracking through `advice_config` is no longer necessary.
> 
> Maybe add "This concludes the move from the old advice API to the new one
> introduced via c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25)"?
> 
> At least this reader would have found that helpful.

Agreed.

> And as I mentioned in the review of patch 3/4, the removal of the
> explicit assignment to the `advice_*` global variables should be moved
> here.

I addressed this in the 3/4 thread.

Thanks,

--Ben

Re: [PATCH v1 0/4] advice: remove usage of `advice_*` global variables

[Johannes Schindelin]

Hi Ben,

On Fri, 30 Jul 2021, Ben Boeckel wrote:

> When looking at global variable usage for my `branch.default*` settings,
> I found the `advice_` variables which were simple enough to resolve.

Even better, it concludes the journey started in c4a09cc9ccb (Merge branch
'hw/advise-ng', 2020-03-25).

I reviewed the entire series and left a few comments I believe to be
constructive.

Since patch 2/4 resolves a problem introduced by merging divergent changes
(one adding `advice_settings`, the other adding two entries to
`advice_config`), an obvious concern with this patch series is: How can we
guarantee that we're not introducing a similar problem when removing
`advice_config`? A future branch could easily add entries to that array,
and a merge of this here topic could potentially forget to add those
entries to `advice_settings`.

However, such a future merge would always cause merge (add/remove)
conflicts in the `advice_config` array, i.e. it will be much easier to
notice such a divergence, and hence it will be much more likely that the
`advice_setting` array will be adjusted accordingly in such a hypothetical
merge.

Ciao,
Dscho

Re: [PATCH v1 0/4] advice: remove usage of `advice_*` global variables

[Ben Boeckel]

On Tue, Aug 03, 2021 at 00:15:06 +0200, Johannes Schindelin wrote:
> On Fri, 30 Jul 2021, Ben Boeckel wrote:
> > When looking at global variable usage for my `branch.default*` settings,
> > I found the `advice_` variables which were simple enough to resolve.
> 
> Even better, it concludes the journey started in c4a09cc9ccb (Merge branch
> 'hw/advise-ng', 2020-03-25).
> 
> I reviewed the entire series and left a few comments I believe to be
> constructive.

Thanks, they've been helpful. I'll work on updating my topic in the
coming days.

> Since patch 2/4 resolves a problem introduced by merging divergent changes
> (one adding `advice_settings`, the other adding two entries to
> `advice_config`), an obvious concern with this patch series is: How can we
> guarantee that we're not introducing a similar problem when removing
> `advice_config`? A future branch could easily add entries to that array,
> and a merge of this here topic could potentially forget to add those
> entries to `advice_settings`.
> 
> However, such a future merge would always cause merge (add/remove)
> conflicts in the `advice_config` array, i.e. it will be much easier to
> notice such a divergence, and hence it will be much more likely that the
> `advice_setting` array will be adjusted accordingly in such a hypothetical

Yes, Git should be able to notice a conflict of this nature.

Thanks,

--Ben

[PATCH v2 0/4] advice: remove usage of `advice_*` global variables

[Ben Boeckel]

Hi,

When looking at global variable usage for my `branch.default*` settings,
I found the `advice_` variables which were simple enough to resolve.

This concludes the journey started in c4a09cc9ccb (Merge branch
'hw/advise-ng', 2020-03-25) to update the advice configuration API to
help avoid bad coding patterns in usage of the `advice_` global
variables.

New `advice_*` variables also in flight will conflict with this
patchset, but the conflict resolution is trivial (if not at all
automatic).

Thanks,

--Ben

---
v1 -> v2:
  - improve commit messages to mention the history of the current state
  - split the API updates between readers and writers of `advice_*` into
    separate commits
  - reorder commits to better show the progression of the replacement

Ben Boeckel (4):
  advice: add enum variants for missing advice variables
  advice: remove read uses of global `advice_` variables
  advice: add `advice_set` to update advice settings at runtime
  advice: remove static global variables for advice tracking

 advice.c                    | 88 ++++---------------------------------
 advice.h                    | 38 +++-------------
 branch.c                    |  2 +-
 builtin/add.c               |  8 ++--
 builtin/am.c                |  2 +-
 builtin/checkout.c          |  6 +--
 builtin/clone.c             |  2 +-
 builtin/commit.c            |  4 +-
 builtin/fetch.c             |  2 +-
 builtin/merge.c             |  4 +-
 builtin/push.c              | 12 ++---
 builtin/replace.c           |  2 +-
 builtin/reset.c             |  2 +-
 builtin/rm.c                |  2 +-
 builtin/submodule--helper.c |  2 +-
 commit.c                    |  2 +-
 editor.c                    |  2 +-
 notes-merge.c               |  2 +-
 object-name.c               |  2 +-
 remote.c                    | 12 ++---
 run-command.c               |  2 +-
 sequencer.c                 |  8 ++--
 unpack-trees.c              | 18 ++++----
 wt-status.c                 |  6 +--
 24 files changed, 68 insertions(+), 162 deletions(-)


base-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687
-- 
2.31.1

[PATCH v2 1/4] advice: add enum variants for missing advice variables

[Ben Boeckel]

In daef1b300b0 (Merge branch 'hw/advice-add-nothing', 2020-02-14), two
advice settings were introduced into the `advice_config` array.

Subsequently, c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25)
started to deprecate `advice_config` in favor of a new array,
`advice_setting`.

However, the latter branch did not include the former branch, and
therefore `advice_setting` is missing the two entries added by the
`hw/advice-add-nothing` branch.

These are currently the only entries in `advice_config` missing from
`advice_setting`.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 advice.c | 2 ++
 advice.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/advice.c b/advice.c
index 0b9c89c48a..6da51be63c 100644
--- a/advice.c
+++ b/advice.c
@@ -106,6 +106,8 @@ static struct {
 	int enabled;
 } advice_setting[] = {
 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
+	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
+	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile", 1 },
 	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
 	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName", 1 },
 	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge", 1 },
diff --git a/advice.h b/advice.h
index bd26c385d0..95489ab4c6 100644
--- a/advice.h
+++ b/advice.h
@@ -45,6 +45,8 @@ extern int advice_add_empty_pathspec;
  */
  enum advice_type {
 	ADVICE_ADD_EMBEDDED_REPO,
+	ADVICE_ADD_EMPTY_PATHSPEC,
+	ADVICE_ADD_IGNORED_FILE,
 	ADVICE_AM_WORK_DIR,
 	ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
 	ADVICE_COMMIT_BEFORE_MERGE,
-- 
2.31.1

[PATCH v2 2/4] advice: remove read uses of global `advice_` variables

[Ben Boeckel]

In c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25), a new API for
accessing advice variables was introduced and deprecated `advice_config`
in favor of a new array, `advice_setting`.

This patch ports all uses which read the status of the global `advice_`
variables over to the new `advice_enabled` API.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 advice.c                    |  4 ++--
 branch.c                    |  2 +-
 builtin/add.c               |  6 +++---
 builtin/am.c                |  2 +-
 builtin/checkout.c          |  6 +++---
 builtin/clone.c             |  2 +-
 builtin/commit.c            |  4 ++--
 builtin/fetch.c             |  2 +-
 builtin/merge.c             |  4 ++--
 builtin/push.c              | 12 ++++++------
 builtin/reset.c             |  2 +-
 builtin/rm.c                |  2 +-
 builtin/submodule--helper.c |  2 +-
 commit.c                    |  2 +-
 editor.c                    |  2 +-
 notes-merge.c               |  2 +-
 object-name.c               |  2 +-
 remote.c                    | 12 ++++++------
 run-command.c               |  2 +-
 sequencer.c                 |  8 ++++----
 unpack-trees.c              | 18 +++++++++---------
 wt-status.c                 |  6 +++---
 22 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/advice.c b/advice.c
index 6da51be63c..0cf3cda26a 100644
--- a/advice.c
+++ b/advice.c
@@ -264,7 +264,7 @@ int error_resolve_conflict(const char *me)
 		error(_("It is not possible to %s because you have unmerged files."),
 			me);
 
-	if (advice_resolve_conflict)
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 		/*
 		 * Message used both when 'git commit' fails and when
 		 * other commands doing a merge do.
@@ -283,7 +283,7 @@ void NORETURN die_resolve_conflict(const char *me)
 void NORETURN die_conclude_merge(void)
 {
 	error(_("You have not concluded your merge (MERGE_HEAD exists)."));
-	if (advice_resolve_conflict)
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 		advise(_("Please, commit your changes before merging."));
 	die(_("Exiting because of unfinished merge."));
 }
diff --git a/branch.c b/branch.c
index 7a88a4861e..07a46430b3 100644
--- a/branch.c
+++ b/branch.c
@@ -271,7 +271,7 @@ void create_branch(struct repository *r,
 	real_ref = NULL;
 	if (get_oid_mb(start_name, &oid)) {
 		if (explicit_tracking) {
-			if (advice_set_upstream_failure) {
+			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
 				error(_(upstream_missing), start_name);
 				advise(_(upstream_advice));
 				exit(1);
diff --git a/builtin/add.c b/builtin/add.c
index 09e684585d..a903306300 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -430,7 +430,7 @@ static void check_embedded_repo(const char *path)
 	strbuf_strip_suffix(&name, "/");
 
 	warning(_("adding embedded git repository: %s"), name.buf);
-	if (advice_add_embedded_repo) {
+	if (advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {
 		advise(embedded_advice, name.buf, name.buf);
 		/* there may be multiple entries; advise only once */
 		advice_add_embedded_repo = 0;
@@ -447,7 +447,7 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		if (advice_add_ignored_file)
+		if (advice_enabled(ADVICE_ADD_IGNORED_FILE))
 			advise(_("Use -f if you really want to add them.\n"
 				"Turn this message off by running\n"
 				"\"git config advice.addIgnoredFile false\""));
@@ -553,7 +553,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		if (advice_add_empty_pathspec)
+		if (advice_enabled(ADVICE_ADD_EMPTY_PATHSPEC))
 			advise( _("Maybe you wanted to say 'git add .'?\n"
 				"Turn this message off by running\n"
 				"\"git config advice.addEmptyPathspec false\""));
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81..040435de8f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1819,7 +1819,7 @@ static void am_run(struct am_state *state, int resume)
 			printf_ln(_("Patch failed at %s %.*s"), msgnum(state),
 				linelen(state->msg), state->msg);
 
-			if (advice_amworkdir)
+			if (advice_enabled(ADVICE_AM_WORK_DIR))
 				advise(_("Use 'git am --show-current-patch=diff' to see the failed patch"));
 
 			die_user_resolve(state);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f4cd7747d3..6d696cf304 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -923,7 +923,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);
 		if (!opts->quiet) {
 			if (old_branch_info->path &&
-			    advice_detached_head && !opts->force_detach)
+			    advice_enabled(ADVICE_DETACHED_HEAD) && !opts->force_detach)
 				detach_advice(new_branch_info->name);
 			describe_detached_head(_("HEAD is now at"), new_branch_info->commit);
 		}
@@ -1016,7 +1016,7 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)
 		sb.buf);
 	strbuf_release(&sb);
 
-	if (advice_detached_head)
+	if (advice_enabled(ADVICE_DETACHED_HEAD))
 		fprintf(stderr,
 			Q_(
 			/* The singular version */
@@ -1187,7 +1187,7 @@ static const char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_checkout_ambiguous_remote_branch_name) {
+	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
 		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
 			     "you can do so by fully qualifying the name with the --track option:\n"
 			     "\n"
diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c..c1603aa82b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -786,7 +786,7 @@ static int checkout(int submodule_progress)
 		return 0;
 	}
 	if (!strcmp(head, "HEAD")) {
-		if (advice_detached_head)
+		if (advice_enabled(ADVICE_DETACHED_HEAD))
 			detach_advice(oid_to_hex(&oid));
 		FREE_AND_NULL(head);
 	} else {
diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43..9566b030af 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -203,7 +203,7 @@ static void status_init_config(struct wt_status *s, config_fn_t fn)
 	init_diff_ui_defaults();
 	git_config(fn, s);
 	determine_whence(s);
-	s->hints = advice_status_hints; /* must come after git_config() */
+	s->hints = advice_enabled(ADVICE_STATUS_HINTS); /* must come after git_config() */
 }
 
 static void rollback_index_files(void)
@@ -1026,7 +1026,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	if (!committable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
-		s->hints = advice_status_hints;
+		s->hints = advice_enabled(ADVICE_STATUS_HINTS);
 		s->display_comment_prefix = old_display_comment_prefix;
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 25740c13df..2501e1d10d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1229,7 +1229,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		      " 'git remote prune %s' to remove any old, conflicting "
 		      "branches"), remote_name);
 
-	if (advice_fetch_show_forced_updates) {
+	if (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {
 		if (!fetch_show_forced_updates) {
 			warning(_(warn_show_forced_updates));
 		} else if (forced_updates_ms > FORCED_UPDATES_DELAY_WARNING_IN_MS) {
diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f5..2efb16626c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1367,14 +1367,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * There is no unmerged entry, don't advise 'git
 		 * add/rm <file>', just 'git commit'.
 		 */
-		if (advice_resolve_conflict)
+		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 			die(_("You have not concluded your merge (MERGE_HEAD exists).\n"
 				  "Please, commit your changes before you merge."));
 		else
 			die(_("You have not concluded your merge (MERGE_HEAD exists)."));
 	}
 	if (ref_exists("CHERRY_PICK_HEAD")) {
-		if (advice_resolve_conflict)
+		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 			die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n"
 			    "Please, commit your changes before you merge."));
 		else
diff --git a/builtin/push.c b/builtin/push.c
index e8b10a9b7e..4b026ce6c6 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -289,42 +289,42 @@ static const char message_advice_ref_needs_update[] =
 
 static void advise_pull_before_push(void)
 {
-	if (!advice_push_non_ff_current || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NON_FF_CURRENT) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_pull_before_push));
 }
 
 static void advise_checkout_pull_push(void)
 {
-	if (!advice_push_non_ff_matching || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NON_FF_MATCHING) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_checkout_pull_push));
 }
 
 static void advise_ref_already_exists(void)
 {
-	if (!advice_push_already_exists || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_ALREADY_EXISTS) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_already_exists));
 }
 
 static void advise_ref_fetch_first(void)
 {
-	if (!advice_push_fetch_first || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_FETCH_FIRST) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_fetch_first));
 }
 
 static void advise_ref_needs_force(void)
 {
-	if (!advice_push_needs_force || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NEEDS_FORCE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_needs_force));
 }
 
 static void advise_ref_needs_update(void)
 {
-	if (!advice_push_ref_needs_update || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_REF_NEEDS_UPDATE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_needs_update));
 }
diff --git a/builtin/reset.c b/builtin/reset.c
index 43e855cb88..51c9e2f43f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -412,7 +412,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_reset_quiet_warning && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
 					printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset.  You can\n"
 						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
 						"to make this the default.\n"), t_delta_in_ms / 1000.0);
diff --git a/builtin/rm.c b/builtin/rm.c
index 8a24c715e0..3b44b807e5 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -55,7 +55,7 @@ static void print_error_files(struct string_list *files_list,
 			strbuf_addf(&err_msg,
 				    "\n    %s",
 				    files_list->items[i].string);
-		if (advice_rm_hints)
+		if (advice_enabled(ADVICE_RM_HINTS))
 			strbuf_addstr(&err_msg, hints_msg);
 		*errs = error("%s", err_msg.buf);
 		strbuf_release(&err_msg);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f73963ad67..e5a5dc6903 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1747,7 +1747,7 @@ static int add_possible_reference_from_superproject(
 		} else {
 			switch (sas->error_mode) {
 			case SUBMODULE_ALTERNATE_ERROR_DIE:
-				if (advice_submodule_alternate_error_strategy_die)
+				if (advice_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE))
 					advise(_(alternate_error_advice));
 				die(_("submodule '%s' cannot add alternate: %s"),
 				    sas->submodule_name, err.buf);
diff --git a/commit.c b/commit.c
index 143f472c0f..17df05dce3 100644
--- a/commit.c
+++ b/commit.c
@@ -190,7 +190,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
-	if (advice_graft_file_deprecated)
+	if (advice_enabled(ADVICE_GRAFT_FILE_DEPRECATED))
 		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
 			 "and will be removed in a future Git version.\n"
 			 "\n"
diff --git a/editor.c b/editor.c
index 6303ae0ab0..fdd3eeafa9 100644
--- a/editor.c
+++ b/editor.c
@@ -58,7 +58,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
 		int ret, sig;
-		int print_waiting_for_editor = advice_waiting_for_editor && isatty(2);
+		int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);
 
 		if (print_waiting_for_editor) {
 			/*
diff --git a/notes-merge.c b/notes-merge.c
index 46c1f7c7f1..b4a3a903e8 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -273,7 +273,7 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
 		 */
 		if (file_exists(git_path(NOTES_MERGE_WORKTREE)) &&
 		    !is_empty_dir(git_path(NOTES_MERGE_WORKTREE))) {
-			if (advice_resolve_conflict)
+			if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 				die(_("You have not concluded your previous "
 				    "notes merge (%s exists).\nPlease, use "
 				    "'git notes merge --commit' or 'git notes "
diff --git a/object-name.c b/object-name.c
index 64202de60b..23758190ed 100644
--- a/object-name.c
+++ b/object-name.c
@@ -812,7 +812,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 			refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref, 0);
 			if (refs_found > 0) {
 				warning(warn_msg, len, str);
-				if (advice_object_name_warning)
+				if (advice_enabled(ADVICE_OBJECT_NAME_WARNING))
 					fprintf(stderr, "%s\n", _(object_name_msg));
 			}
 			free(real_ref);
diff --git a/remote.c b/remote.c
index dfb863d808..4ef53a6e30 100644
--- a/remote.c
+++ b/remote.c
@@ -1111,7 +1111,7 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 		"Neither worked, so we gave up. You must fully qualify the ref."),
 	      dst_value, matched_src_name);
 
-	if (!advice_push_unqualified_ref_name)
+	if (!advice_enabled(ADVICE_PUSH_UNQUALIFIED_REF_NAME))
 		return;
 
 	if (get_oid(matched_src_name, &oid))
@@ -2118,7 +2118,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 		strbuf_addf(sb,
 			_("Your branch is based on '%s', but the upstream is gone.\n"),
 			base);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git branch --unset-upstream\" to fixup)\n"));
 	} else if (!sti) {
@@ -2129,7 +2129,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 		strbuf_addf(sb,
 			    _("Your branch and '%s' refer to different commits.\n"),
 			    base);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
 				    "git status --ahead-behind");
 	} else if (!theirs) {
@@ -2138,7 +2138,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			   "Your branch is ahead of '%s' by %d commits.\n",
 			   ours),
 			base, ours);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git push\" to publish your local commits)\n"));
 	} else if (!ours) {
@@ -2149,7 +2149,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			       "and can be fast-forwarded.\n",
 			   theirs),
 			base, theirs);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git pull\" to update your local branch)\n"));
 	} else {
@@ -2162,7 +2162,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			       "respectively.\n",
 			   ours + theirs),
 			base, ours, theirs);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git pull\" to merge the remote branch into yours)\n"));
 	}
diff --git a/run-command.c b/run-command.c
index f72e72cce7..28e3eb5732 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1336,7 +1336,7 @@ const char *find_hook(const char *name)
 			err = errno;
 #endif
 
-		if (err == EACCES && advice_ignored_hook) {
+		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
 			static struct string_list advise_given = STRING_LIST_INIT_DUP;
 
 			if (!string_list_lookup(&advise_given, name)) {
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38..e0b4c86c1a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -486,7 +486,7 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
 	error(_("your local changes would be overwritten by %s."),
 		_(action_name(opts)));
 
-	if (advice_commit_before_merge)
+	if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE))
 		advise(_("commit your changes or stash them to proceed."));
 	return -1;
 }
@@ -1293,7 +1293,7 @@ void print_commit_summary(struct repository *r,
 	if (!committer_ident_sufficiently_given()) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
-		if (advice_implicit_identity) {
+		if (advice_enabled(ADVICE_IMPLICIT_IDENTITY)) {
 			strbuf_addch(&format, '\n');
 			strbuf_addstr(&format, implicit_ident_advice());
 		}
@@ -3041,7 +3041,7 @@ static int create_seq_dir(struct repository *r)
 	}
 	if (in_progress_error) {
 		error("%s", in_progress_error);
-		if (advice_sequencer_in_use)
+		if (advice_enabled(ADVICE_SEQUENCER_IN_USE))
 			advise(in_progress_advice,
 				advise_skip ? "--skip | " : "");
 		return -1;
@@ -3245,7 +3245,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
 give_advice:
 	error(_("there is nothing to skip"));
 
-	if (advice_resolve_conflict) {
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT)) {
 		advise(_("have you committed already?\n"
 			 "try \"git %s --continue\""),
 			 action == REPLAY_REVERT ? "revert" : "cherry-pick");
diff --git a/unpack-trees.c b/unpack-trees.c
index f88a69f8e7..eb5015623b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -111,17 +111,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 	strvec_init(&opts->msgs_to_free);
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
 			  "Please commit your changes or stash them before you switch branches.")
 		      : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by merge:\n%%s"
 			  "Please commit your changes or stash them before you merge.")
 		      : _("Your local changes to the following files would be overwritten by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by %s:\n%%s"
 			  "Please commit your changes or stash them before you %s.")
 		      : _("Your local changes to the following files would be overwritten by %s:\n%%s");
@@ -132,17 +132,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		_("Updating the following directories would lose untracked files in them:\n%s");
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by checkout:\n%%s"
 			  "Please move or remove them before you switch branches.")
 		      : _("The following untracked working tree files would be removed by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by merge:\n%%s"
 			  "Please move or remove them before you merge.")
 		      : _("The following untracked working tree files would be removed by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by %s:\n%%s"
 			  "Please move or remove them before you %s.")
 		      : _("The following untracked working tree files would be removed by %s:\n%%s");
@@ -150,17 +150,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		strvec_pushf(&opts->msgs_to_free, msg, cmd, cmd);
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by checkout:\n%%s"
 			  "Please move or remove them before you switch branches.")
 		      : _("The following untracked working tree files would be overwritten by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by merge:\n%%s"
 			  "Please move or remove them before you merge.")
 		      : _("The following untracked working tree files would be overwritten by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by %s:\n%%s"
 			  "Please move or remove them before you %s.")
 		      : _("The following untracked working tree files would be overwritten by %s:\n%%s");
diff --git a/wt-status.c b/wt-status.c
index c0dbf96749..1850251d8c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -736,7 +736,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
 
 	dir_clear(&dir);
 
-	if (advice_status_u_option)
+	if (advice_enabled(ADVICE_STATUS_U_OPTION))
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
@@ -1107,7 +1107,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
 	if (!format_tracking_info(branch, &sb, s->ahead_behind_flags))
 		return;
 
-	if (advice_status_ahead_behind_warning &&
+	if (advice_enabled(ADVICE_STATUS_AHEAD_BEHIND_WARNING) &&
 	    s->ahead_behind_flags == AHEAD_BEHIND_FULL) {
 		uint64_t t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
 		if (t_delta_in_ms > AB_DELAY_WARNING_IN_MS) {
@@ -1786,7 +1786,7 @@ static void wt_longstatus_print(struct wt_status *s)
 		wt_longstatus_print_other(s, &s->untracked, _("Untracked files"), "add");
 		if (s->show_ignored_mode)
 			wt_longstatus_print_other(s, &s->ignored, _("Ignored files"), "add -f");
-		if (advice_status_u_option && 2000 < s->untracked_in_ms) {
+		if (advice_enabled(ADVICE_STATUS_U_OPTION) && 2000 < s->untracked_in_ms) {
 			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 					 _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n"
-- 
2.31.1

[PATCH v2 3/4] advice: add `advice_set` to update advice settings at runtime

[Ben Boeckel]

There are two uses of global `advice_` variables that write to them.
Replace these uses with a new API that writes to the internal table so
that the global variables may be removed completely.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 advice.c          | 5 +++++
 advice.h          | 5 +++++
 builtin/add.c     | 2 +-
 builtin/replace.c | 2 +-
 4 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/advice.c b/advice.c
index 0cf3cda26a..ee4edc5e28 100644
--- a/advice.c
+++ b/advice.c
@@ -189,6 +189,11 @@ int advice_enabled(enum advice_type type)
 	}
 }
 
+void advice_set(enum advice_type type, int value)
+{
+	advice_setting[type].enabled = value;
+}
+
 void advise_if_enabled(enum advice_type type, const char *advice, ...)
 {
 	va_list params;
diff --git a/advice.h b/advice.h
index 95489ab4c6..101c4054b7 100644
--- a/advice.h
+++ b/advice.h
@@ -89,6 +89,11 @@ void advise(const char *advice, ...);
  */
 int advice_enabled(enum advice_type type);
 
+/**
+ * Enable or disable advice of a certain kind.
+ */
+void advice_set(enum advice_type type, int value);
+
 /**
  * Checks the visibility of the advice before printing.
  */
diff --git a/builtin/add.c b/builtin/add.c
index a903306300..3d1fd64294 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -433,7 +433,7 @@ static void check_embedded_repo(const char *path)
 	if (advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {
 		advise(embedded_advice, name.buf, name.buf);
 		/* there may be multiple entries; advise only once */
-		advice_add_embedded_repo = 0;
+		advice_set(ADVICE_ADD_EMBEDDED_REPO, 0);
 	}
 
 	strbuf_release(&name);
diff --git a/builtin/replace.c b/builtin/replace.c
index cd48765911..1340878021 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -507,7 +507,7 @@ static int convert_graft_file(int force)
 	if (!fp)
 		return -1;
 
-	advice_graft_file_deprecated = 0;
+	advice_set(ADVICE_GRAFT_FILE_DEPRECATED, 0);
 	while (strbuf_getline(&buf, fp) != EOF) {
 		if (*buf.buf == '#')
 			continue;
-- 
2.31.1

[PATCH v2 4/4] advice: remove static global variables for advice tracking

[Ben Boeckel]

All of the preferences are now tracked as part of the `advice_setting`
array and all consumers of the global variables have been removed, so
the parallel tracking through `advice_config` is no longer necessary.

This concludes the move from the old advice API to the new one
introduced via c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25).

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 advice.c | 77 --------------------------------------------------------
 advice.h | 31 -----------------------
 2 files changed, 108 deletions(-)

diff --git a/advice.c b/advice.c
index ee4edc5e28..6ba47f3f5e 100644
--- a/advice.c
+++ b/advice.c
@@ -4,37 +4,6 @@
 #include "help.h"
 #include "string-list.h"
 
-int advice_fetch_show_forced_updates = 1;
-int advice_push_update_rejected = 1;
-int advice_push_non_ff_current = 1;
-int advice_push_non_ff_matching = 1;
-int advice_push_already_exists = 1;
-int advice_push_fetch_first = 1;
-int advice_push_needs_force = 1;
-int advice_push_unqualified_ref_name = 1;
-int advice_push_ref_needs_update = 1;
-int advice_status_hints = 1;
-int advice_status_u_option = 1;
-int advice_status_ahead_behind_warning = 1;
-int advice_commit_before_merge = 1;
-int advice_reset_quiet_warning = 1;
-int advice_resolve_conflict = 1;
-int advice_sequencer_in_use = 1;
-int advice_implicit_identity = 1;
-int advice_detached_head = 1;
-int advice_set_upstream_failure = 1;
-int advice_object_name_warning = 1;
-int advice_amworkdir = 1;
-int advice_rm_hints = 1;
-int advice_add_embedded_repo = 1;
-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_submodule_alternate_error_strategy_die = 1;
-int advice_add_ignored_file = 1;
-int advice_add_empty_pathspec = 1;
-
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
@@ -62,45 +31,6 @@ static const char *advise_get_color(enum color_advice ix)
 	return "";
 }
 
-static struct {
-	const char *name;
-	int *preference;
-} advice_config[] = {
-	{ "fetchShowForcedUpdates", &advice_fetch_show_forced_updates },
-	{ "pushUpdateRejected", &advice_push_update_rejected },
-	{ "pushNonFFCurrent", &advice_push_non_ff_current },
-	{ "pushNonFFMatching", &advice_push_non_ff_matching },
-	{ "pushAlreadyExists", &advice_push_already_exists },
-	{ "pushFetchFirst", &advice_push_fetch_first },
-	{ "pushNeedsForce", &advice_push_needs_force },
-	{ "pushUnqualifiedRefName", &advice_push_unqualified_ref_name },
-	{ "pushRefNeedsUpdate", &advice_push_ref_needs_update },
-	{ "statusHints", &advice_status_hints },
-	{ "statusUoption", &advice_status_u_option },
-	{ "statusAheadBehindWarning", &advice_status_ahead_behind_warning },
-	{ "commitBeforeMerge", &advice_commit_before_merge },
-	{ "resetQuiet", &advice_reset_quiet_warning },
-	{ "resolveConflict", &advice_resolve_conflict },
-	{ "sequencerInUse", &advice_sequencer_in_use },
-	{ "implicitIdentity", &advice_implicit_identity },
-	{ "detachedHead", &advice_detached_head },
-	{ "setUpstreamFailure", &advice_set_upstream_failure },
-	{ "objectNameWarning", &advice_object_name_warning },
-	{ "amWorkDir", &advice_amworkdir },
-	{ "rmHints", &advice_rm_hints },
-	{ "addEmbeddedRepo", &advice_add_embedded_repo },
-	{ "ignoredHook", &advice_ignored_hook },
-	{ "waitingForEditor", &advice_waiting_for_editor },
-	{ "graftFileDeprecated", &advice_graft_file_deprecated },
-	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
-	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
-	{ "addIgnoredFile", &advice_add_ignored_file },
-	{ "addEmptyPathspec", &advice_add_empty_pathspec },
-
-	/* make this an alias for backward compatibility */
-	{ "pushNonFastForward", &advice_push_update_rejected }
-};
-
 static struct {
 	const char *key;
 	int enabled;
@@ -228,13 +158,6 @@ int git_default_advice_config(const char *var, const char *value)
 	if (!skip_prefix(var, "advice.", &k))
 		return 0;
 
-	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
-		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;
diff --git a/advice.h b/advice.h
index 101c4054b7..17ee5d3633 100644
--- a/advice.h
+++ b/advice.h
@@ -5,37 +5,6 @@
 
 struct string_list;
 
-extern int advice_fetch_show_forced_updates;
-extern int advice_push_update_rejected;
-extern int advice_push_non_ff_current;
-extern int advice_push_non_ff_matching;
-extern int advice_push_already_exists;
-extern int advice_push_fetch_first;
-extern int advice_push_needs_force;
-extern int advice_push_unqualified_ref_name;
-extern int advice_push_ref_needs_update;
-extern int advice_status_hints;
-extern int advice_status_u_option;
-extern int advice_status_ahead_behind_warning;
-extern int advice_commit_before_merge;
-extern int advice_reset_quiet_warning;
-extern int advice_resolve_conflict;
-extern int advice_sequencer_in_use;
-extern int advice_implicit_identity;
-extern int advice_detached_head;
-extern int advice_set_upstream_failure;
-extern int advice_object_name_warning;
-extern int advice_amworkdir;
-extern int advice_rm_hints;
-extern int advice_add_embedded_repo;
-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_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.
-- 
2.31.1

[RFC PATCH v3 0/4] advice: remove usage of `advice_*` global variables

[Ævar Arnfjörð Bjarmason]

While reviewing
https://lore.kernel.org/git/20210805230321.532218-1-mathstuf@gmail.com/
today I started fixing it up locally. So in liue of inline commentary
here's the fixups I had.

There's mid-series breakages in the v2, because two variables are
moved over to the new API, with their users left at the old API until
the end. This fixes that up by migrating most of the variables, and
then incrementally dealing with the two remaining cases.

In one case we don't want to emit a warning twice, so a scope-local
static variable seems better than the new advice_set(), and in the
case of the graft advise() it seemed better to keep it global, but
move it over to commit.[ch].

We thus do away entirely with the need for the new advice_s

Ben Boeckel (2):
  advice: add enum variants for missing advice variables
  advice: remove read uses of most global `advice_` variables

Ævar Arnfjörð Bjarmason (2):
  advice: remove use of global advice_add_embedded_repo
  advice: move advice.graftFileDeprecated squashing to commit.[ch]

 advice.c                    | 83 ++-----------------------------------
 advice.h                    | 33 +--------------
 branch.c                    |  2 +-
 builtin/add.c               | 11 ++---
 builtin/am.c                |  2 +-
 builtin/checkout.c          |  6 +--
 builtin/clone.c             |  2 +-
 builtin/commit.c            |  4 +-
 builtin/fetch.c             |  2 +-
 builtin/merge.c             |  4 +-
 builtin/push.c              | 12 +++---
 builtin/replace.c           |  2 +-
 builtin/reset.c             |  2 +-
 builtin/rm.c                |  2 +-
 builtin/submodule--helper.c |  2 +-
 commit.c                    |  4 +-
 commit.h                    |  1 +
 editor.c                    |  2 +-
 notes-merge.c               |  2 +-
 object-name.c               |  2 +-
 remote.c                    | 12 +++---
 run-command.c               |  2 +-
 sequencer.c                 |  8 ++--
 unpack-trees.c              | 18 ++++----
 wt-status.c                 |  6 +--
 25 files changed, 63 insertions(+), 163 deletions(-)

Range-diff against v2:
1:  97743eb4dc ! 1:  5f934bb083 advice: add enum variants for missing advice variables
    @@ Commit message
         `advice_setting`.
     
         Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## advice.c ##
     @@ advice.c: static struct {
2:  bc6311304a ! 2:  eefcafcf8f advice: remove read uses of global `advice_` variables
    @@ Metadata
     Author: Ben Boeckel <mathstuf@gmail.com>
     
      ## Commit message ##
    -    advice: remove read uses of global `advice_` variables
    +    advice: remove read uses of most global `advice_` variables
     
         In c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25), a new API for
         accessing advice variables was introduced and deprecated `advice_config`
         in favor of a new array, `advice_setting`.
     
    -    This patch ports all uses which read the status of the global `advice_`
    -    variables over to the new `advice_enabled` API.
    +    This patch ports all but two uses which read the status of the global
    +    `advice_` variables over to the new `advice_enabled` API. We'll deal
    +    with advice_add_embedded_repo and advice_graft_file_deprecated
    +    seperately.
     
         Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## advice.c ##
    +@@
    + #include "help.h"
    + #include "string-list.h"
    + 
    +-int advice_fetch_show_forced_updates = 1;
    +-int advice_push_update_rejected = 1;
    +-int advice_push_non_ff_current = 1;
    +-int advice_push_non_ff_matching = 1;
    +-int advice_push_already_exists = 1;
    +-int advice_push_fetch_first = 1;
    +-int advice_push_needs_force = 1;
    +-int advice_push_unqualified_ref_name = 1;
    +-int advice_push_ref_needs_update = 1;
    +-int advice_status_hints = 1;
    +-int advice_status_u_option = 1;
    +-int advice_status_ahead_behind_warning = 1;
    +-int advice_commit_before_merge = 1;
    +-int advice_reset_quiet_warning = 1;
    +-int advice_resolve_conflict = 1;
    +-int advice_sequencer_in_use = 1;
    +-int advice_implicit_identity = 1;
    +-int advice_detached_head = 1;
    +-int advice_set_upstream_failure = 1;
    +-int advice_object_name_warning = 1;
    +-int advice_amworkdir = 1;
    +-int advice_rm_hints = 1;
    + int advice_add_embedded_repo = 1;
    +-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_submodule_alternate_error_strategy_die = 1;
    +-int advice_add_ignored_file = 1;
    +-int advice_add_empty_pathspec = 1;
    + 
    + static int advice_use_color = -1;
    + static char advice_colors[][COLOR_MAXLEN] = {
    +@@ advice.c: static struct {
    + 	const char *name;
    + 	int *preference;
    + } advice_config[] = {
    +-	{ "fetchShowForcedUpdates", &advice_fetch_show_forced_updates },
    +-	{ "pushUpdateRejected", &advice_push_update_rejected },
    +-	{ "pushNonFFCurrent", &advice_push_non_ff_current },
    +-	{ "pushNonFFMatching", &advice_push_non_ff_matching },
    +-	{ "pushAlreadyExists", &advice_push_already_exists },
    +-	{ "pushFetchFirst", &advice_push_fetch_first },
    +-	{ "pushNeedsForce", &advice_push_needs_force },
    +-	{ "pushUnqualifiedRefName", &advice_push_unqualified_ref_name },
    +-	{ "pushRefNeedsUpdate", &advice_push_ref_needs_update },
    +-	{ "statusHints", &advice_status_hints },
    +-	{ "statusUoption", &advice_status_u_option },
    +-	{ "statusAheadBehindWarning", &advice_status_ahead_behind_warning },
    +-	{ "commitBeforeMerge", &advice_commit_before_merge },
    +-	{ "resetQuiet", &advice_reset_quiet_warning },
    +-	{ "resolveConflict", &advice_resolve_conflict },
    +-	{ "sequencerInUse", &advice_sequencer_in_use },
    +-	{ "implicitIdentity", &advice_implicit_identity },
    +-	{ "detachedHead", &advice_detached_head },
    +-	{ "setUpstreamFailure", &advice_set_upstream_failure },
    +-	{ "objectNameWarning", &advice_object_name_warning },
    +-	{ "amWorkDir", &advice_amworkdir },
    +-	{ "rmHints", &advice_rm_hints },
    + 	{ "addEmbeddedRepo", &advice_add_embedded_repo },
    +-	{ "ignoredHook", &advice_ignored_hook },
    +-	{ "waitingForEditor", &advice_waiting_for_editor },
    + 	{ "graftFileDeprecated", &advice_graft_file_deprecated },
    +-	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
    +-	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
    +-	{ "addIgnoredFile", &advice_add_ignored_file },
    +-	{ "addEmptyPathspec", &advice_add_empty_pathspec },
    +-
    +-	/* make this an alias for backward compatibility */
    +-	{ "pushNonFastForward", &advice_push_update_rejected }
    + };
    + 
    + static struct {
     @@ advice.c: int error_resolve_conflict(const char *me)
      		error(_("It is not possible to %s because you have unmerged files."),
      			me);
    @@ advice.c: void NORETURN die_resolve_conflict(const char *me)
      	die(_("Exiting because of unfinished merge."));
      }
     
    + ## advice.h ##
    +@@
    + 
    + struct string_list;
    + 
    +-extern int advice_fetch_show_forced_updates;
    +-extern int advice_push_update_rejected;
    +-extern int advice_push_non_ff_current;
    +-extern int advice_push_non_ff_matching;
    +-extern int advice_push_already_exists;
    +-extern int advice_push_fetch_first;
    +-extern int advice_push_needs_force;
    +-extern int advice_push_unqualified_ref_name;
    +-extern int advice_push_ref_needs_update;
    +-extern int advice_status_hints;
    +-extern int advice_status_u_option;
    +-extern int advice_status_ahead_behind_warning;
    +-extern int advice_commit_before_merge;
    +-extern int advice_reset_quiet_warning;
    +-extern int advice_resolve_conflict;
    +-extern int advice_sequencer_in_use;
    +-extern int advice_implicit_identity;
    +-extern int advice_detached_head;
    +-extern int advice_set_upstream_failure;
    +-extern int advice_object_name_warning;
    +-extern int advice_amworkdir;
    +-extern int advice_rm_hints;
    + extern int advice_add_embedded_repo;
    +-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_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:
    +
      ## branch.c ##
     @@ branch.c: void create_branch(struct repository *r,
      	real_ref = NULL;
    @@ branch.c: void create_branch(struct repository *r,
      				exit(1);
     
      ## builtin/add.c ##
    -@@ builtin/add.c: static void check_embedded_repo(const char *path)
    - 	strbuf_strip_suffix(&name, "/");
    - 
    - 	warning(_("adding embedded git repository: %s"), name.buf);
    --	if (advice_add_embedded_repo) {
    -+	if (advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {
    - 		advise(embedded_advice, name.buf, name.buf);
    - 		/* there may be multiple entries; advise only once */
    - 		advice_add_embedded_repo = 0;
     @@ builtin/add.c: static int add_files(struct dir_struct *dir, int flags)
      		fprintf(stderr, _(ignore_error));
      		for (i = 0; i < dir->ignored_nr; i++)
    @@ builtin/submodule--helper.c: static int add_possible_reference_from_superproject
      				die(_("submodule '%s' cannot add alternate: %s"),
      				    sas->submodule_name, err.buf);
     
    - ## commit.c ##
    -@@ commit.c: static int read_graft_file(struct repository *r, const char *graft_file)
    - 	struct strbuf buf = STRBUF_INIT;
    - 	if (!fp)
    - 		return -1;
    --	if (advice_graft_file_deprecated)
    -+	if (advice_enabled(ADVICE_GRAFT_FILE_DEPRECATED))
    - 		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
    - 			 "and will be removed in a future Git version.\n"
    - 			 "\n"
    -
      ## editor.c ##
     @@ editor.c: static int launch_specified_editor(const char *editor, const char *path,
      		const char *args[] = { editor, NULL, NULL };
3:  c044f6a6d7 < -:  ---------- advice: add `advice_set` to update advice settings at runtime
-:  ---------- > 3:  02613d0f30 advice: remove use of global advice_add_embedded_repo
4:  644709115f ! 4:  fe6f6328f9 advice: remove static global variables for advice tracking
    @@
      ## Metadata ##
    -Author: Ben Boeckel <mathstuf@gmail.com>
    +Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    advice: remove static global variables for advice tracking
    +    advice: move advice.graftFileDeprecated squashing to commit.[ch]
     
    -    All of the preferences are now tracked as part of the `advice_setting`
    -    array and all consumers of the global variables have been removed, so
    -    the parallel tracking through `advice_config` is no longer necessary.
    +    Move the squashing of the advice.graftFileDeprecated advice over to an
    +    external variable in commit.[ch], allowing advice() to purely use the
    +    new-style API of invoking advice() with an enum.
     
    -    This concludes the move from the old advice API to the new one
    -    introduced via c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25).
    +    See 8821e90a09a (advice: don't pointlessly suggest
    +    --convert-graft-file, 2018-11-27) for why quieting this advice was
    +    needed. It's more straightforward to move this code to commit.[ch] and
    +    use it builtin/replace.c, than to go through the indirection of
    +    advice.[ch].
     
    -    Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
    +    Because this was the last advice_config variable we can remove that
    +    old facility from advice.c.
    +
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## advice.c ##
     @@
      #include "help.h"
      #include "string-list.h"
      
    --int advice_fetch_show_forced_updates = 1;
    --int advice_push_update_rejected = 1;
    --int advice_push_non_ff_current = 1;
    --int advice_push_non_ff_matching = 1;
    --int advice_push_already_exists = 1;
    --int advice_push_fetch_first = 1;
    --int advice_push_needs_force = 1;
    --int advice_push_unqualified_ref_name = 1;
    --int advice_push_ref_needs_update = 1;
    --int advice_status_hints = 1;
    --int advice_status_u_option = 1;
    --int advice_status_ahead_behind_warning = 1;
    --int advice_commit_before_merge = 1;
    --int advice_reset_quiet_warning = 1;
    --int advice_resolve_conflict = 1;
    --int advice_sequencer_in_use = 1;
    --int advice_implicit_identity = 1;
    --int advice_detached_head = 1;
    --int advice_set_upstream_failure = 1;
    --int advice_object_name_warning = 1;
    --int advice_amworkdir = 1;
    --int advice_rm_hints = 1;
    --int advice_add_embedded_repo = 1;
    --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_submodule_alternate_error_strategy_die = 1;
    --int advice_add_ignored_file = 1;
    --int advice_add_empty_pathspec = 1;
     -
      static int advice_use_color = -1;
      static char advice_colors[][COLOR_MAXLEN] = {
    @@ advice.c: static const char *advise_get_color(enum color_advice ix)
     -	const char *name;
     -	int *preference;
     -} advice_config[] = {
    --	{ "fetchShowForcedUpdates", &advice_fetch_show_forced_updates },
    --	{ "pushUpdateRejected", &advice_push_update_rejected },
    --	{ "pushNonFFCurrent", &advice_push_non_ff_current },
    --	{ "pushNonFFMatching", &advice_push_non_ff_matching },
    --	{ "pushAlreadyExists", &advice_push_already_exists },
    --	{ "pushFetchFirst", &advice_push_fetch_first },
    --	{ "pushNeedsForce", &advice_push_needs_force },
    --	{ "pushUnqualifiedRefName", &advice_push_unqualified_ref_name },
    --	{ "pushRefNeedsUpdate", &advice_push_ref_needs_update },
    --	{ "statusHints", &advice_status_hints },
    --	{ "statusUoption", &advice_status_u_option },
    --	{ "statusAheadBehindWarning", &advice_status_ahead_behind_warning },
    --	{ "commitBeforeMerge", &advice_commit_before_merge },
    --	{ "resetQuiet", &advice_reset_quiet_warning },
    --	{ "resolveConflict", &advice_resolve_conflict },
    --	{ "sequencerInUse", &advice_sequencer_in_use },
    --	{ "implicitIdentity", &advice_implicit_identity },
    --	{ "detachedHead", &advice_detached_head },
    --	{ "setUpstreamFailure", &advice_set_upstream_failure },
    --	{ "objectNameWarning", &advice_object_name_warning },
    --	{ "amWorkDir", &advice_amworkdir },
    --	{ "rmHints", &advice_rm_hints },
    --	{ "addEmbeddedRepo", &advice_add_embedded_repo },
    --	{ "ignoredHook", &advice_ignored_hook },
    --	{ "waitingForEditor", &advice_waiting_for_editor },
     -	{ "graftFileDeprecated", &advice_graft_file_deprecated },
    --	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
    --	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
    --	{ "addIgnoredFile", &advice_add_ignored_file },
    --	{ "addEmptyPathspec", &advice_add_empty_pathspec },
    --
    --	/* make this an alias for backward compatibility */
    --	{ "pushNonFastForward", &advice_push_update_rejected }
     -};
     -
      static struct {
    @@ advice.h
      
      struct string_list;
      
    --extern int advice_fetch_show_forced_updates;
    --extern int advice_push_update_rejected;
    --extern int advice_push_non_ff_current;
    --extern int advice_push_non_ff_matching;
    --extern int advice_push_already_exists;
    --extern int advice_push_fetch_first;
    --extern int advice_push_needs_force;
    --extern int advice_push_unqualified_ref_name;
    --extern int advice_push_ref_needs_update;
    --extern int advice_status_hints;
    --extern int advice_status_u_option;
    --extern int advice_status_ahead_behind_warning;
    --extern int advice_commit_before_merge;
    --extern int advice_reset_quiet_warning;
    --extern int advice_resolve_conflict;
    --extern int advice_sequencer_in_use;
    --extern int advice_implicit_identity;
    --extern int advice_detached_head;
    --extern int advice_set_upstream_failure;
    --extern int advice_object_name_warning;
    --extern int advice_amworkdir;
    --extern int advice_rm_hints;
    --extern int advice_add_embedded_repo;
    --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_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.
    +
    + ## builtin/replace.c ##
    +@@ builtin/replace.c: static int convert_graft_file(int force)
    + 	if (!fp)
    + 		return -1;
    + 
    +-	advice_graft_file_deprecated = 0;
    ++	no_graft_file_deprecated_advice = 1;
    + 	while (strbuf_getline(&buf, fp) != EOF) {
    + 		if (*buf.buf == '#')
    + 			continue;
    +
    + ## commit.c ##
    +@@
    + static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
    + 
    + int save_commit_buffer = 1;
    ++int no_graft_file_deprecated_advice;
    + 
    + const char *commit_type = "commit";
    + 
    +@@ commit.c: static int read_graft_file(struct repository *r, const char *graft_file)
    + 	struct strbuf buf = STRBUF_INIT;
    + 	if (!fp)
    + 		return -1;
    +-	if (advice_graft_file_deprecated)
    ++	if (!no_graft_file_deprecated_advice &&
    ++	    advice_enabled(ADVICE_GRAFT_FILE_DEPRECATED))
    + 		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
    + 			 "and will be removed in a future Git version.\n"
    + 			 "\n"
    +
    + ## commit.h ##
    +@@ commit.h: struct commit {
    + };
    + 
    + extern int save_commit_buffer;
    ++extern int no_graft_file_deprecated_advice;
    + extern const char *commit_type;
    + 
    + /* While we can decorate any object with a name, it's only used for commits.. */
-- 
2.33.0.rc0.680.gf07173a897a

[RFC PATCH v3 1/4] advice: add enum variants for missing advice variables

[Ævar Arnfjörð Bjarmason]

From: Ben Boeckel <mathstuf@gmail.com>

In daef1b300b0 (Merge branch 'hw/advice-add-nothing', 2020-02-14), two
advice settings were introduced into the `advice_config` array.

Subsequently, c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25)
started to deprecate `advice_config` in favor of a new array,
`advice_setting`.

However, the latter branch did not include the former branch, and
therefore `advice_setting` is missing the two entries added by the
`hw/advice-add-nothing` branch.

These are currently the only entries in `advice_config` missing from
`advice_setting`.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.c | 2 ++
 advice.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/advice.c b/advice.c
index 0b9c89c48a..6da51be63c 100644
--- a/advice.c
+++ b/advice.c
@@ -106,6 +106,8 @@ static struct {
 	int enabled;
 } advice_setting[] = {
 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
+	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
+	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile", 1 },
 	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
 	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName", 1 },
 	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge", 1 },
diff --git a/advice.h b/advice.h
index 9f8ffc7354..68629b5ba1 100644
--- a/advice.h
+++ b/advice.h
@@ -45,6 +45,8 @@ extern int advice_add_empty_pathspec;
  */
  enum advice_type {
 	ADVICE_ADD_EMBEDDED_REPO,
+	ADVICE_ADD_EMPTY_PATHSPEC,
+	ADVICE_ADD_IGNORED_FILE,
 	ADVICE_AM_WORK_DIR,
 	ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
 	ADVICE_COMMIT_BEFORE_MERGE,
-- 
2.33.0.rc0.680.gf07173a897a

[RFC PATCH v3 2/4] advice: remove read uses of most global `advice_` variables

[Ævar Arnfjörð Bjarmason]

From: Ben Boeckel <mathstuf@gmail.com>

In c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25), a new API for
accessing advice variables was introduced and deprecated `advice_config`
in favor of a new array, `advice_setting`.

This patch ports all but two uses which read the status of the global
`advice_` variables over to the new `advice_enabled` API. We'll deal
with advice_add_embedded_repo and advice_graft_file_deprecated
seperately.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.c                    | 63 ++-----------------------------------
 advice.h                    | 28 -----------------
 branch.c                    |  2 +-
 builtin/add.c               |  4 +--
 builtin/am.c                |  2 +-
 builtin/checkout.c          |  6 ++--
 builtin/clone.c             |  2 +-
 builtin/commit.c            |  4 +--
 builtin/fetch.c             |  2 +-
 builtin/merge.c             |  4 +--
 builtin/push.c              | 12 +++----
 builtin/reset.c             |  2 +-
 builtin/rm.c                |  2 +-
 builtin/submodule--helper.c |  2 +-
 editor.c                    |  2 +-
 notes-merge.c               |  2 +-
 object-name.c               |  2 +-
 remote.c                    | 12 +++----
 run-command.c               |  2 +-
 sequencer.c                 |  8 ++---
 unpack-trees.c              | 18 +++++------
 wt-status.c                 |  6 ++--
 22 files changed, 50 insertions(+), 137 deletions(-)

diff --git a/advice.c b/advice.c
index 6da51be63c..b18833bc80 100644
--- a/advice.c
+++ b/advice.c
@@ -4,36 +4,8 @@
 #include "help.h"
 #include "string-list.h"
 
-int advice_fetch_show_forced_updates = 1;
-int advice_push_update_rejected = 1;
-int advice_push_non_ff_current = 1;
-int advice_push_non_ff_matching = 1;
-int advice_push_already_exists = 1;
-int advice_push_fetch_first = 1;
-int advice_push_needs_force = 1;
-int advice_push_unqualified_ref_name = 1;
-int advice_push_ref_needs_update = 1;
-int advice_status_hints = 1;
-int advice_status_u_option = 1;
-int advice_status_ahead_behind_warning = 1;
-int advice_commit_before_merge = 1;
-int advice_reset_quiet_warning = 1;
-int advice_resolve_conflict = 1;
-int advice_sequencer_in_use = 1;
-int advice_implicit_identity = 1;
-int advice_detached_head = 1;
-int advice_set_upstream_failure = 1;
-int advice_object_name_warning = 1;
-int advice_amworkdir = 1;
-int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
-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_submodule_alternate_error_strategy_die = 1;
-int advice_add_ignored_file = 1;
-int advice_add_empty_pathspec = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -66,39 +38,8 @@ static struct {
 	const char *name;
 	int *preference;
 } advice_config[] = {
-	{ "fetchShowForcedUpdates", &advice_fetch_show_forced_updates },
-	{ "pushUpdateRejected", &advice_push_update_rejected },
-	{ "pushNonFFCurrent", &advice_push_non_ff_current },
-	{ "pushNonFFMatching", &advice_push_non_ff_matching },
-	{ "pushAlreadyExists", &advice_push_already_exists },
-	{ "pushFetchFirst", &advice_push_fetch_first },
-	{ "pushNeedsForce", &advice_push_needs_force },
-	{ "pushUnqualifiedRefName", &advice_push_unqualified_ref_name },
-	{ "pushRefNeedsUpdate", &advice_push_ref_needs_update },
-	{ "statusHints", &advice_status_hints },
-	{ "statusUoption", &advice_status_u_option },
-	{ "statusAheadBehindWarning", &advice_status_ahead_behind_warning },
-	{ "commitBeforeMerge", &advice_commit_before_merge },
-	{ "resetQuiet", &advice_reset_quiet_warning },
-	{ "resolveConflict", &advice_resolve_conflict },
-	{ "sequencerInUse", &advice_sequencer_in_use },
-	{ "implicitIdentity", &advice_implicit_identity },
-	{ "detachedHead", &advice_detached_head },
-	{ "setUpstreamFailure", &advice_set_upstream_failure },
-	{ "objectNameWarning", &advice_object_name_warning },
-	{ "amWorkDir", &advice_amworkdir },
-	{ "rmHints", &advice_rm_hints },
 	{ "addEmbeddedRepo", &advice_add_embedded_repo },
-	{ "ignoredHook", &advice_ignored_hook },
-	{ "waitingForEditor", &advice_waiting_for_editor },
 	{ "graftFileDeprecated", &advice_graft_file_deprecated },
-	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
-	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
-	{ "addIgnoredFile", &advice_add_ignored_file },
-	{ "addEmptyPathspec", &advice_add_empty_pathspec },
-
-	/* make this an alias for backward compatibility */
-	{ "pushNonFastForward", &advice_push_update_rejected }
 };
 
 static struct {
@@ -264,7 +205,7 @@ int error_resolve_conflict(const char *me)
 		error(_("It is not possible to %s because you have unmerged files."),
 			me);
 
-	if (advice_resolve_conflict)
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 		/*
 		 * Message used both when 'git commit' fails and when
 		 * other commands doing a merge do.
@@ -283,7 +224,7 @@ void NORETURN die_resolve_conflict(const char *me)
 void NORETURN die_conclude_merge(void)
 {
 	error(_("You have not concluded your merge (MERGE_HEAD exists)."));
-	if (advice_resolve_conflict)
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 		advise(_("Please, commit your changes before merging."));
 	die(_("Exiting because of unfinished merge."));
 }
diff --git a/advice.h b/advice.h
index 68629b5ba1..ed51db0f05 100644
--- a/advice.h
+++ b/advice.h
@@ -5,36 +5,8 @@
 
 struct string_list;
 
-extern int advice_fetch_show_forced_updates;
-extern int advice_push_update_rejected;
-extern int advice_push_non_ff_current;
-extern int advice_push_non_ff_matching;
-extern int advice_push_already_exists;
-extern int advice_push_fetch_first;
-extern int advice_push_needs_force;
-extern int advice_push_unqualified_ref_name;
-extern int advice_push_ref_needs_update;
-extern int advice_status_hints;
-extern int advice_status_u_option;
-extern int advice_status_ahead_behind_warning;
-extern int advice_commit_before_merge;
-extern int advice_reset_quiet_warning;
-extern int advice_resolve_conflict;
-extern int advice_sequencer_in_use;
-extern int advice_implicit_identity;
-extern int advice_detached_head;
-extern int advice_set_upstream_failure;
-extern int advice_object_name_warning;
-extern int advice_amworkdir;
-extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
-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_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:
diff --git a/branch.c b/branch.c
index 7a88a4861e..07a46430b3 100644
--- a/branch.c
+++ b/branch.c
@@ -271,7 +271,7 @@ void create_branch(struct repository *r,
 	real_ref = NULL;
 	if (get_oid_mb(start_name, &oid)) {
 		if (explicit_tracking) {
-			if (advice_set_upstream_failure) {
+			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
 				error(_(upstream_missing), start_name);
 				advise(_(upstream_advice));
 				exit(1);
diff --git a/builtin/add.c b/builtin/add.c
index 09e684585d..cf29b302d4 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -447,7 +447,7 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		if (advice_add_ignored_file)
+		if (advice_enabled(ADVICE_ADD_IGNORED_FILE))
 			advise(_("Use -f if you really want to add them.\n"
 				"Turn this message off by running\n"
 				"\"git config advice.addIgnoredFile false\""));
@@ -553,7 +553,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		if (advice_add_empty_pathspec)
+		if (advice_enabled(ADVICE_ADD_EMPTY_PATHSPEC))
 			advise( _("Maybe you wanted to say 'git add .'?\n"
 				"Turn this message off by running\n"
 				"\"git config advice.addEmptyPathspec false\""));
diff --git a/builtin/am.c b/builtin/am.c
index 0c2ad96b70..ff7dd33fcd 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1820,7 +1820,7 @@ static void am_run(struct am_state *state, int resume)
 			printf_ln(_("Patch failed at %s %.*s"), msgnum(state),
 				linelen(state->msg), state->msg);
 
-			if (advice_amworkdir)
+			if (advice_enabled(ADVICE_AM_WORK_DIR))
 				advise(_("Use 'git am --show-current-patch=diff' to see the failed patch"));
 
 			die_user_resolve(state);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b5d477919a..1c6307d456 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -918,7 +918,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);
 		if (!opts->quiet) {
 			if (old_branch_info->path &&
-			    advice_detached_head && !opts->force_detach)
+			    advice_enabled(ADVICE_DETACHED_HEAD) && !opts->force_detach)
 				detach_advice(new_branch_info->name);
 			describe_detached_head(_("HEAD is now at"), new_branch_info->commit);
 		}
@@ -1011,7 +1011,7 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)
 		sb.buf);
 	strbuf_release(&sb);
 
-	if (advice_detached_head)
+	if (advice_enabled(ADVICE_DETACHED_HEAD))
 		fprintf(stderr,
 			Q_(
 			/* The singular version */
@@ -1182,7 +1182,7 @@ static const char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_checkout_ambiguous_remote_branch_name) {
+	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
 		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
 			     "you can do so by fully qualifying the name with the --track option:\n"
 			     "\n"
diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c..c1603aa82b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -786,7 +786,7 @@ static int checkout(int submodule_progress)
 		return 0;
 	}
 	if (!strcmp(head, "HEAD")) {
-		if (advice_detached_head)
+		if (advice_enabled(ADVICE_DETACHED_HEAD))
 			detach_advice(oid_to_hex(&oid));
 		FREE_AND_NULL(head);
 	} else {
diff --git a/builtin/commit.c b/builtin/commit.c
index 243c626307..df8bcc27ae 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -203,7 +203,7 @@ static void status_init_config(struct wt_status *s, config_fn_t fn)
 	init_diff_ui_defaults();
 	git_config(fn, s);
 	determine_whence(s);
-	s->hints = advice_status_hints; /* must come after git_config() */
+	s->hints = advice_enabled(ADVICE_STATUS_HINTS); /* must come after git_config() */
 }
 
 static void rollback_index_files(void)
@@ -1033,7 +1033,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	if (!committable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
-		s->hints = advice_status_hints;
+		s->hints = advice_enabled(ADVICE_STATUS_HINTS);
 		s->display_comment_prefix = old_display_comment_prefix;
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 25740c13df..2501e1d10d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1229,7 +1229,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		      " 'git remote prune %s' to remove any old, conflicting "
 		      "branches"), remote_name);
 
-	if (advice_fetch_show_forced_updates) {
+	if (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {
 		if (!fetch_show_forced_updates) {
 			warning(_(warn_show_forced_updates));
 		} else if (forced_updates_ms > FORCED_UPDATES_DELAY_WARNING_IN_MS) {
diff --git a/builtin/merge.c b/builtin/merge.c
index 22f23990b3..1b3d98d5fd 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1368,14 +1368,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * There is no unmerged entry, don't advise 'git
 		 * add/rm <file>', just 'git commit'.
 		 */
-		if (advice_resolve_conflict)
+		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 			die(_("You have not concluded your merge (MERGE_HEAD exists).\n"
 				  "Please, commit your changes before you merge."));
 		else
 			die(_("You have not concluded your merge (MERGE_HEAD exists)."));
 	}
 	if (ref_exists("CHERRY_PICK_HEAD")) {
-		if (advice_resolve_conflict)
+		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 			die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n"
 			    "Please, commit your changes before you merge."));
 		else
diff --git a/builtin/push.c b/builtin/push.c
index e8b10a9b7e..4b026ce6c6 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -289,42 +289,42 @@ static const char message_advice_ref_needs_update[] =
 
 static void advise_pull_before_push(void)
 {
-	if (!advice_push_non_ff_current || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NON_FF_CURRENT) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_pull_before_push));
 }
 
 static void advise_checkout_pull_push(void)
 {
-	if (!advice_push_non_ff_matching || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NON_FF_MATCHING) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_checkout_pull_push));
 }
 
 static void advise_ref_already_exists(void)
 {
-	if (!advice_push_already_exists || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_ALREADY_EXISTS) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_already_exists));
 }
 
 static void advise_ref_fetch_first(void)
 {
-	if (!advice_push_fetch_first || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_FETCH_FIRST) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_fetch_first));
 }
 
 static void advise_ref_needs_force(void)
 {
-	if (!advice_push_needs_force || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NEEDS_FORCE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_needs_force));
 }
 
 static void advise_ref_needs_update(void)
 {
-	if (!advice_push_ref_needs_update || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_REF_NEEDS_UPDATE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_needs_update));
 }
diff --git a/builtin/reset.c b/builtin/reset.c
index 43e855cb88..51c9e2f43f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -412,7 +412,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_reset_quiet_warning && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
 					printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset.  You can\n"
 						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
 						"to make this the default.\n"), t_delta_in_ms / 1000.0);
diff --git a/builtin/rm.c b/builtin/rm.c
index 8a24c715e0..3b44b807e5 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -55,7 +55,7 @@ static void print_error_files(struct string_list *files_list,
 			strbuf_addf(&err_msg,
 				    "\n    %s",
 				    files_list->items[i].string);
-		if (advice_rm_hints)
+		if (advice_enabled(ADVICE_RM_HINTS))
 			strbuf_addstr(&err_msg, hints_msg);
 		*errs = error("%s", err_msg.buf);
 		strbuf_release(&err_msg);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e4..4da9781b99 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1724,7 +1724,7 @@ static int add_possible_reference_from_superproject(
 		} else {
 			switch (sas->error_mode) {
 			case SUBMODULE_ALTERNATE_ERROR_DIE:
-				if (advice_submodule_alternate_error_strategy_die)
+				if (advice_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE))
 					advise(_(alternate_error_advice));
 				die(_("submodule '%s' cannot add alternate: %s"),
 				    sas->submodule_name, err.buf);
diff --git a/editor.c b/editor.c
index 6303ae0ab0..fdd3eeafa9 100644
--- a/editor.c
+++ b/editor.c
@@ -58,7 +58,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
 		int ret, sig;
-		int print_waiting_for_editor = advice_waiting_for_editor && isatty(2);
+		int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);
 
 		if (print_waiting_for_editor) {
 			/*
diff --git a/notes-merge.c b/notes-merge.c
index 46c1f7c7f1..b4a3a903e8 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -273,7 +273,7 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
 		 */
 		if (file_exists(git_path(NOTES_MERGE_WORKTREE)) &&
 		    !is_empty_dir(git_path(NOTES_MERGE_WORKTREE))) {
-			if (advice_resolve_conflict)
+			if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 				die(_("You have not concluded your previous "
 				    "notes merge (%s exists).\nPlease, use "
 				    "'git notes merge --commit' or 'git notes "
diff --git a/object-name.c b/object-name.c
index 3263c19457..fdff4601b2 100644
--- a/object-name.c
+++ b/object-name.c
@@ -806,7 +806,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 			refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref, 0);
 			if (refs_found > 0) {
 				warning(warn_msg, len, str);
-				if (advice_object_name_warning)
+				if (advice_enabled(ADVICE_OBJECT_NAME_WARNING))
 					fprintf(stderr, "%s\n", _(object_name_msg));
 			}
 			free(real_ref);
diff --git a/remote.c b/remote.c
index dfb863d808..4ef53a6e30 100644
--- a/remote.c
+++ b/remote.c
@@ -1111,7 +1111,7 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 		"Neither worked, so we gave up. You must fully qualify the ref."),
 	      dst_value, matched_src_name);
 
-	if (!advice_push_unqualified_ref_name)
+	if (!advice_enabled(ADVICE_PUSH_UNQUALIFIED_REF_NAME))
 		return;
 
 	if (get_oid(matched_src_name, &oid))
@@ -2118,7 +2118,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 		strbuf_addf(sb,
 			_("Your branch is based on '%s', but the upstream is gone.\n"),
 			base);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git branch --unset-upstream\" to fixup)\n"));
 	} else if (!sti) {
@@ -2129,7 +2129,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 		strbuf_addf(sb,
 			    _("Your branch and '%s' refer to different commits.\n"),
 			    base);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
 				    "git status --ahead-behind");
 	} else if (!theirs) {
@@ -2138,7 +2138,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			   "Your branch is ahead of '%s' by %d commits.\n",
 			   ours),
 			base, ours);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git push\" to publish your local commits)\n"));
 	} else if (!ours) {
@@ -2149,7 +2149,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			       "and can be fast-forwarded.\n",
 			   theirs),
 			base, theirs);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git pull\" to update your local branch)\n"));
 	} else {
@@ -2162,7 +2162,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			       "respectively.\n",
 			   ours + theirs),
 			base, ours, theirs);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git pull\" to merge the remote branch into yours)\n"));
 	}
diff --git a/run-command.c b/run-command.c
index f72e72cce7..28e3eb5732 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1336,7 +1336,7 @@ const char *find_hook(const char *name)
 			err = errno;
 #endif
 
-		if (err == EACCES && advice_ignored_hook) {
+		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
 			static struct string_list advise_given = STRING_LIST_INIT_DUP;
 
 			if (!string_list_lookup(&advise_given, name)) {
diff --git a/sequencer.c b/sequencer.c
index 7f07cd00f3..ab63eede45 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -486,7 +486,7 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
 	error(_("your local changes would be overwritten by %s."),
 		_(action_name(opts)));
 
-	if (advice_commit_before_merge)
+	if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE))
 		advise(_("commit your changes or stash them to proceed."));
 	return -1;
 }
@@ -1293,7 +1293,7 @@ void print_commit_summary(struct repository *r,
 	if (!committer_ident_sufficiently_given()) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
-		if (advice_implicit_identity) {
+		if (advice_enabled(ADVICE_IMPLICIT_IDENTITY)) {
 			strbuf_addch(&format, '\n');
 			strbuf_addstr(&format, implicit_ident_advice());
 		}
@@ -3041,7 +3041,7 @@ static int create_seq_dir(struct repository *r)
 	}
 	if (in_progress_error) {
 		error("%s", in_progress_error);
-		if (advice_sequencer_in_use)
+		if (advice_enabled(ADVICE_SEQUENCER_IN_USE))
 			advise(in_progress_advice,
 				advise_skip ? "--skip | " : "");
 		return -1;
@@ -3245,7 +3245,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
 give_advice:
 	error(_("there is nothing to skip"));
 
-	if (advice_resolve_conflict) {
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT)) {
 		advise(_("have you committed already?\n"
 			 "try \"git %s --continue\""),
 			 action == REPLAY_REVERT ? "revert" : "cherry-pick");
diff --git a/unpack-trees.c b/unpack-trees.c
index 5786645f31..d9907faf7b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -111,17 +111,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 	strvec_init(&opts->msgs_to_free);
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
 			  "Please commit your changes or stash them before you switch branches.")
 		      : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by merge:\n%%s"
 			  "Please commit your changes or stash them before you merge.")
 		      : _("Your local changes to the following files would be overwritten by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by %s:\n%%s"
 			  "Please commit your changes or stash them before you %s.")
 		      : _("Your local changes to the following files would be overwritten by %s:\n%%s");
@@ -132,17 +132,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		_("Updating the following directories would lose untracked files in them:\n%s");
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by checkout:\n%%s"
 			  "Please move or remove them before you switch branches.")
 		      : _("The following untracked working tree files would be removed by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by merge:\n%%s"
 			  "Please move or remove them before you merge.")
 		      : _("The following untracked working tree files would be removed by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by %s:\n%%s"
 			  "Please move or remove them before you %s.")
 		      : _("The following untracked working tree files would be removed by %s:\n%%s");
@@ -150,17 +150,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		strvec_pushf(&opts->msgs_to_free, msg, cmd, cmd);
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by checkout:\n%%s"
 			  "Please move or remove them before you switch branches.")
 		      : _("The following untracked working tree files would be overwritten by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by merge:\n%%s"
 			  "Please move or remove them before you merge.")
 		      : _("The following untracked working tree files would be overwritten by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by %s:\n%%s"
 			  "Please move or remove them before you %s.")
 		      : _("The following untracked working tree files would be overwritten by %s:\n%%s");
diff --git a/wt-status.c b/wt-status.c
index eaed30eafb..e4f29b2b4c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -787,7 +787,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
 
 	dir_clear(&dir);
 
-	if (advice_status_u_option)
+	if (advice_enabled(ADVICE_STATUS_U_OPTION))
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
@@ -1158,7 +1158,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
 	if (!format_tracking_info(branch, &sb, s->ahead_behind_flags))
 		return;
 
-	if (advice_status_ahead_behind_warning &&
+	if (advice_enabled(ADVICE_STATUS_AHEAD_BEHIND_WARNING) &&
 	    s->ahead_behind_flags == AHEAD_BEHIND_FULL) {
 		uint64_t t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
 		if (t_delta_in_ms > AB_DELAY_WARNING_IN_MS) {
@@ -1845,7 +1845,7 @@ static void wt_longstatus_print(struct wt_status *s)
 		wt_longstatus_print_other(s, &s->untracked, _("Untracked files"), "add");
 		if (s->show_ignored_mode)
 			wt_longstatus_print_other(s, &s->ignored, _("Ignored files"), "add -f");
-		if (advice_status_u_option && 2000 < s->untracked_in_ms) {
+		if (advice_enabled(ADVICE_STATUS_U_OPTION) && 2000 < s->untracked_in_ms) {
 			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 					 _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n"
-- 
2.33.0.rc0.680.gf07173a897a

Re: [RFC PATCH v3 2/4] advice: remove read uses of most global `advice_` variables

[Eric Sunshine]

/On Fri, Aug 6, 2021 at 3:13 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> In c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25), a new API for
> accessing advice variables was introduced and deprecated `advice_config`
> in favor of a new array, `advice_setting`.
>
> This patch ports all but two uses which read the status of the global
> `advice_` variables over to the new `advice_enabled` API. We'll deal
> with advice_add_embedded_repo and advice_graft_file_deprecated
> seperately.

s/seperately/separately/

> Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

[RFC PATCH v3 3/4] advice: remove use of global advice_add_embedded_repo

[Ævar Arnfjörð Bjarmason]

The external use of this variable was added in 532139940c9 (add: warn
when adding an embedded repository, 2017-06-14), for the use-case it's
more straightforward to track whether we've adviced in
check_embedded_repo() than setting the global variable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.c      | 2 --
 advice.h      | 1 -
 builtin/add.c | 7 ++++---
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/advice.c b/advice.c
index b18833bc80..41cfea82d0 100644
--- a/advice.c
+++ b/advice.c
@@ -4,7 +4,6 @@
 #include "help.h"
 #include "string-list.h"
 
-int advice_add_embedded_repo = 1;
 int advice_graft_file_deprecated = 1;
 
 static int advice_use_color = -1;
@@ -38,7 +37,6 @@ static struct {
 	const char *name;
 	int *preference;
 } advice_config[] = {
-	{ "addEmbeddedRepo", &advice_add_embedded_repo },
 	{ "graftFileDeprecated", &advice_graft_file_deprecated },
 };
 
diff --git a/advice.h b/advice.h
index ed51db0f05..4b754f4c62 100644
--- a/advice.h
+++ b/advice.h
@@ -5,7 +5,6 @@
 
 struct string_list;
 
-extern int advice_add_embedded_repo;
 extern int advice_graft_file_deprecated;
 
 /*
diff --git a/builtin/add.c b/builtin/add.c
index cf29b302d4..8a5dd29f3f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -419,6 +419,7 @@ static const char embedded_advice[] = N_(
 static void check_embedded_repo(const char *path)
 {
 	struct strbuf name = STRBUF_INIT;
+	static int adviced_on_embedded_repo = 0;
 
 	if (!warn_on_embedded_repo)
 		return;
@@ -430,10 +431,10 @@ static void check_embedded_repo(const char *path)
 	strbuf_strip_suffix(&name, "/");
 
 	warning(_("adding embedded git repository: %s"), name.buf);
-	if (advice_add_embedded_repo) {
+	if (!adviced_on_embedded_repo &&
+	    advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {
 		advise(embedded_advice, name.buf, name.buf);
-		/* there may be multiple entries; advise only once */
-		advice_add_embedded_repo = 0;
+		adviced_on_embedded_repo = 1;
 	}
 
 	strbuf_release(&name);
-- 
2.33.0.rc0.680.gf07173a897a

Re: [RFC PATCH v3 3/4] advice: remove use of global advice_add_embedded_repo

[Eric Sunshine]

On Fri, Aug 6, 2021 at 3:13 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> The external use of this variable was added in 532139940c9 (add: warn
> when adding an embedded repository, 2017-06-14), for the use-case it's

s/, for/. For/

> more straightforward to track whether we've adviced in

s/adviced/advised/
...or better...
s/we've adviced/we've shown advice/

> check_embedded_repo() than setting the global variable.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

[RFC PATCH v3 4/4] advice: move advice.graftFileDeprecated squashing to commit.[ch]

[Ævar Arnfjörð Bjarmason]

Move the squashing of the advice.graftFileDeprecated advice over to an
external variable in commit.[ch], allowing advice() to purely use the
new-style API of invoking advice() with an enum.

See 8821e90a09a (advice: don't pointlessly suggest
--convert-graft-file, 2018-11-27) for why quieting this advice was
needed. It's more straightforward to move this code to commit.[ch] and
use it builtin/replace.c, than to go through the indirection of
advice.[ch].

Because this was the last advice_config variable we can remove that
old facility from advice.c.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.c          | 16 ----------------
 advice.h          |  2 --
 builtin/replace.c |  2 +-
 commit.c          |  4 +++-
 commit.h          |  1 +
 5 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/advice.c b/advice.c
index 41cfea82d0..e716ddebff 100644
--- a/advice.c
+++ b/advice.c
@@ -4,8 +4,6 @@
 #include "help.h"
 #include "string-list.h"
 
-int advice_graft_file_deprecated = 1;
-
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
@@ -33,13 +31,6 @@ static const char *advise_get_color(enum color_advice ix)
 	return "";
 }
 
-static struct {
-	const char *name;
-	int *preference;
-} advice_config[] = {
-	{ "graftFileDeprecated", &advice_graft_file_deprecated },
-};
-
 static struct {
 	const char *key;
 	int enabled;
@@ -162,13 +153,6 @@ int git_default_advice_config(const char *var, const char *value)
 	if (!skip_prefix(var, "advice.", &k))
 		return 0;
 
-	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
-		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;
diff --git a/advice.h b/advice.h
index 4b754f4c62..e380a0562e 100644
--- a/advice.h
+++ b/advice.h
@@ -5,8 +5,6 @@
 
 struct string_list;
 
-extern int advice_graft_file_deprecated;
-
 /*
  * To add a new advice, you need to:
  * Define a new advice_type.
diff --git a/builtin/replace.c b/builtin/replace.c
index cd48765911..946938d011 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -507,7 +507,7 @@ static int convert_graft_file(int force)
 	if (!fp)
 		return -1;
 
-	advice_graft_file_deprecated = 0;
+	no_graft_file_deprecated_advice = 1;
 	while (strbuf_getline(&buf, fp) != EOF) {
 		if (*buf.buf == '#')
 			continue;
diff --git a/commit.c b/commit.c
index 143f472c0f..551de4903c 100644
--- a/commit.c
+++ b/commit.c
@@ -25,6 +25,7 @@
 static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
 
 int save_commit_buffer = 1;
+int no_graft_file_deprecated_advice;
 
 const char *commit_type = "commit";
 
@@ -190,7 +191,8 @@ static int read_graft_file(struct repository *r, const char *graft_file)
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
-	if (advice_graft_file_deprecated)
+	if (!no_graft_file_deprecated_advice &&
+	    advice_enabled(ADVICE_GRAFT_FILE_DEPRECATED))
 		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
 			 "and will be removed in a future Git version.\n"
 			 "\n"
diff --git a/commit.h b/commit.h
index df42eb434f..3ea32766bc 100644
--- a/commit.h
+++ b/commit.h
@@ -41,6 +41,7 @@ struct commit {
 };
 
 extern int save_commit_buffer;
+extern int no_graft_file_deprecated_advice;
 extern const char *commit_type;
 
 /* While we can decorate any object with a name, it's only used for commits.. */
-- 
2.33.0.rc0.680.gf07173a897a

[PATCH v4 0/4] advice: remove usage of `advice_*` global variables

[Ævar Arnfjörð Bjarmason]

I reviewed Ben Boeckel's advice() patches in
https://lore.kernel.org/git/RFC-cover-v3-0.4-0000000000-20210806T191231Z-avarab@gmail.com
and submitted that review as an RFC. Since there's been no feedback
there from the author here's a non-RFC version.

The only change from v3 is the commit message typos/grammar fixes
pointed out by Eric Sunshine. Thanks!

Ben Boeckel (2):
  advice: add enum variants for missing advice variables
  advice: remove read uses of most global `advice_` variables

Ævar Arnfjörð Bjarmason (2):
  advice: remove use of global advice_add_embedded_repo
  advice: move advice.graftFileDeprecated squashing to commit.[ch]

 advice.c                    | 83 ++-----------------------------------
 advice.h                    | 33 +--------------
 branch.c                    |  2 +-
 builtin/add.c               | 11 ++---
 builtin/am.c                |  2 +-
 builtin/checkout.c          |  6 +--
 builtin/clone.c             |  2 +-
 builtin/commit.c            |  4 +-
 builtin/fetch.c             |  2 +-
 builtin/merge.c             |  4 +-
 builtin/push.c              | 12 +++---
 builtin/replace.c           |  2 +-
 builtin/reset.c             |  2 +-
 builtin/rm.c                |  2 +-
 builtin/submodule--helper.c |  2 +-
 commit.c                    |  4 +-
 commit.h                    |  1 +
 editor.c                    |  2 +-
 notes-merge.c               |  2 +-
 object-name.c               |  2 +-
 remote.c                    | 12 +++---
 run-command.c               |  2 +-
 sequencer.c                 |  8 ++--
 unpack-trees.c              | 18 ++++----
 wt-status.c                 |  6 +--
 25 files changed, 63 insertions(+), 163 deletions(-)

Range-diff against v3:
1:  5f934bb083b = 1:  4e977e9d5a1 advice: add enum variants for missing advice variables
2:  eefcafcf8f5 ! 2:  3869bda3b39 advice: remove read uses of most global `advice_` variables
    @@ Commit message
         This patch ports all but two uses which read the status of the global
         `advice_` variables over to the new `advice_enabled` API. We'll deal
         with advice_add_embedded_repo and advice_graft_file_deprecated
    -    seperately.
    +    separately.
     
         Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
3:  02613d0f307 ! 3:  e1018212e40 advice: remove use of global advice_add_embedded_repo
    @@ Commit message
         advice: remove use of global advice_add_embedded_repo
     
         The external use of this variable was added in 532139940c9 (add: warn
    -    when adding an embedded repository, 2017-06-14), for the use-case it's
    -    more straightforward to track whether we've adviced in
    +    when adding an embedded repository, 2017-06-14). For the use-case it's
    +    more straightforward to track whether we've shown advice in
         check_embedded_repo() than setting the global variable.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
4:  fe6f6328f9c = 4:  7f79fb282e5 advice: move advice.graftFileDeprecated squashing to commit.[ch]
-- 
2.33.0.663.gfcc3c7013a8

[PATCH v4 1/4] advice: add enum variants for missing advice variables

[Ævar Arnfjörð Bjarmason]

From: Ben Boeckel <mathstuf@gmail.com>

In daef1b300b0 (Merge branch 'hw/advice-add-nothing', 2020-02-14), two
advice settings were introduced into the `advice_config` array.

Subsequently, c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25)
started to deprecate `advice_config` in favor of a new array,
`advice_setting`.

However, the latter branch did not include the former branch, and
therefore `advice_setting` is missing the two entries added by the
`hw/advice-add-nothing` branch.

These are currently the only entries in `advice_config` missing from
`advice_setting`.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.c | 2 ++
 advice.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/advice.c b/advice.c
index 0b9c89c48ab..6da51be63c1 100644
--- a/advice.c
+++ b/advice.c
@@ -106,6 +106,8 @@ static struct {
 	int enabled;
 } advice_setting[] = {
 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
+	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
+	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile", 1 },
 	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
 	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName", 1 },
 	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge", 1 },
diff --git a/advice.h b/advice.h
index 9f8ffc73546..68629b5ba1d 100644
--- a/advice.h
+++ b/advice.h
@@ -45,6 +45,8 @@ extern int advice_add_empty_pathspec;
  */
  enum advice_type {
 	ADVICE_ADD_EMBEDDED_REPO,
+	ADVICE_ADD_EMPTY_PATHSPEC,
+	ADVICE_ADD_IGNORED_FILE,
 	ADVICE_AM_WORK_DIR,
 	ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
 	ADVICE_COMMIT_BEFORE_MERGE,
-- 
2.33.0.663.gfcc3c7013a8

[PATCH v4 2/4] advice: remove read uses of most global `advice_` variables

[Ævar Arnfjörð Bjarmason]

From: Ben Boeckel <mathstuf@gmail.com>

In c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25), a new API for
accessing advice variables was introduced and deprecated `advice_config`
in favor of a new array, `advice_setting`.

This patch ports all but two uses which read the status of the global
`advice_` variables over to the new `advice_enabled` API. We'll deal
with advice_add_embedded_repo and advice_graft_file_deprecated
separately.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.c                    | 63 ++-----------------------------------
 advice.h                    | 28 -----------------
 branch.c                    |  2 +-
 builtin/add.c               |  4 +--
 builtin/am.c                |  2 +-
 builtin/checkout.c          |  6 ++--
 builtin/clone.c             |  2 +-
 builtin/commit.c            |  4 +--
 builtin/fetch.c             |  2 +-
 builtin/merge.c             |  4 +--
 builtin/push.c              | 12 +++----
 builtin/reset.c             |  2 +-
 builtin/rm.c                |  2 +-
 builtin/submodule--helper.c |  2 +-
 editor.c                    |  2 +-
 notes-merge.c               |  2 +-
 object-name.c               |  2 +-
 remote.c                    | 12 +++----
 run-command.c               |  2 +-
 sequencer.c                 |  8 ++---
 unpack-trees.c              | 18 +++++------
 wt-status.c                 |  6 ++--
 22 files changed, 50 insertions(+), 137 deletions(-)

diff --git a/advice.c b/advice.c
index 6da51be63c1..b18833bc807 100644
--- a/advice.c
+++ b/advice.c
@@ -4,36 +4,8 @@
 #include "help.h"
 #include "string-list.h"
 
-int advice_fetch_show_forced_updates = 1;
-int advice_push_update_rejected = 1;
-int advice_push_non_ff_current = 1;
-int advice_push_non_ff_matching = 1;
-int advice_push_already_exists = 1;
-int advice_push_fetch_first = 1;
-int advice_push_needs_force = 1;
-int advice_push_unqualified_ref_name = 1;
-int advice_push_ref_needs_update = 1;
-int advice_status_hints = 1;
-int advice_status_u_option = 1;
-int advice_status_ahead_behind_warning = 1;
-int advice_commit_before_merge = 1;
-int advice_reset_quiet_warning = 1;
-int advice_resolve_conflict = 1;
-int advice_sequencer_in_use = 1;
-int advice_implicit_identity = 1;
-int advice_detached_head = 1;
-int advice_set_upstream_failure = 1;
-int advice_object_name_warning = 1;
-int advice_amworkdir = 1;
-int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
-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_submodule_alternate_error_strategy_die = 1;
-int advice_add_ignored_file = 1;
-int advice_add_empty_pathspec = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -66,39 +38,8 @@ static struct {
 	const char *name;
 	int *preference;
 } advice_config[] = {
-	{ "fetchShowForcedUpdates", &advice_fetch_show_forced_updates },
-	{ "pushUpdateRejected", &advice_push_update_rejected },
-	{ "pushNonFFCurrent", &advice_push_non_ff_current },
-	{ "pushNonFFMatching", &advice_push_non_ff_matching },
-	{ "pushAlreadyExists", &advice_push_already_exists },
-	{ "pushFetchFirst", &advice_push_fetch_first },
-	{ "pushNeedsForce", &advice_push_needs_force },
-	{ "pushUnqualifiedRefName", &advice_push_unqualified_ref_name },
-	{ "pushRefNeedsUpdate", &advice_push_ref_needs_update },
-	{ "statusHints", &advice_status_hints },
-	{ "statusUoption", &advice_status_u_option },
-	{ "statusAheadBehindWarning", &advice_status_ahead_behind_warning },
-	{ "commitBeforeMerge", &advice_commit_before_merge },
-	{ "resetQuiet", &advice_reset_quiet_warning },
-	{ "resolveConflict", &advice_resolve_conflict },
-	{ "sequencerInUse", &advice_sequencer_in_use },
-	{ "implicitIdentity", &advice_implicit_identity },
-	{ "detachedHead", &advice_detached_head },
-	{ "setUpstreamFailure", &advice_set_upstream_failure },
-	{ "objectNameWarning", &advice_object_name_warning },
-	{ "amWorkDir", &advice_amworkdir },
-	{ "rmHints", &advice_rm_hints },
 	{ "addEmbeddedRepo", &advice_add_embedded_repo },
-	{ "ignoredHook", &advice_ignored_hook },
-	{ "waitingForEditor", &advice_waiting_for_editor },
 	{ "graftFileDeprecated", &advice_graft_file_deprecated },
-	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
-	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
-	{ "addIgnoredFile", &advice_add_ignored_file },
-	{ "addEmptyPathspec", &advice_add_empty_pathspec },
-
-	/* make this an alias for backward compatibility */
-	{ "pushNonFastForward", &advice_push_update_rejected }
 };
 
 static struct {
@@ -264,7 +205,7 @@ int error_resolve_conflict(const char *me)
 		error(_("It is not possible to %s because you have unmerged files."),
 			me);
 
-	if (advice_resolve_conflict)
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 		/*
 		 * Message used both when 'git commit' fails and when
 		 * other commands doing a merge do.
@@ -283,7 +224,7 @@ void NORETURN die_resolve_conflict(const char *me)
 void NORETURN die_conclude_merge(void)
 {
 	error(_("You have not concluded your merge (MERGE_HEAD exists)."));
-	if (advice_resolve_conflict)
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 		advise(_("Please, commit your changes before merging."));
 	die(_("Exiting because of unfinished merge."));
 }
diff --git a/advice.h b/advice.h
index 68629b5ba1d..ed51db0f057 100644
--- a/advice.h
+++ b/advice.h
@@ -5,36 +5,8 @@
 
 struct string_list;
 
-extern int advice_fetch_show_forced_updates;
-extern int advice_push_update_rejected;
-extern int advice_push_non_ff_current;
-extern int advice_push_non_ff_matching;
-extern int advice_push_already_exists;
-extern int advice_push_fetch_first;
-extern int advice_push_needs_force;
-extern int advice_push_unqualified_ref_name;
-extern int advice_push_ref_needs_update;
-extern int advice_status_hints;
-extern int advice_status_u_option;
-extern int advice_status_ahead_behind_warning;
-extern int advice_commit_before_merge;
-extern int advice_reset_quiet_warning;
-extern int advice_resolve_conflict;
-extern int advice_sequencer_in_use;
-extern int advice_implicit_identity;
-extern int advice_detached_head;
-extern int advice_set_upstream_failure;
-extern int advice_object_name_warning;
-extern int advice_amworkdir;
-extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
-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_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:
diff --git a/branch.c b/branch.c
index 7a88a4861e7..07a46430b38 100644
--- a/branch.c
+++ b/branch.c
@@ -271,7 +271,7 @@ void create_branch(struct repository *r,
 	real_ref = NULL;
 	if (get_oid_mb(start_name, &oid)) {
 		if (explicit_tracking) {
-			if (advice_set_upstream_failure) {
+			if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
 				error(_(upstream_missing), start_name);
 				advise(_(upstream_advice));
 				exit(1);
diff --git a/builtin/add.c b/builtin/add.c
index 09e684585d9..cf29b302d44 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -447,7 +447,7 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		if (advice_add_ignored_file)
+		if (advice_enabled(ADVICE_ADD_IGNORED_FILE))
 			advise(_("Use -f if you really want to add them.\n"
 				"Turn this message off by running\n"
 				"\"git config advice.addIgnoredFile false\""));
@@ -553,7 +553,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		if (advice_add_empty_pathspec)
+		if (advice_enabled(ADVICE_ADD_EMPTY_PATHSPEC))
 			advise( _("Maybe you wanted to say 'git add .'?\n"
 				"Turn this message off by running\n"
 				"\"git config advice.addEmptyPathspec false\""));
diff --git a/builtin/am.c b/builtin/am.c
index 0c2ad96b70e..ff7dd33fcd4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1820,7 +1820,7 @@ static void am_run(struct am_state *state, int resume)
 			printf_ln(_("Patch failed at %s %.*s"), msgnum(state),
 				linelen(state->msg), state->msg);
 
-			if (advice_amworkdir)
+			if (advice_enabled(ADVICE_AM_WORK_DIR))
 				advise(_("Use 'git am --show-current-patch=diff' to see the failed patch"));
 
 			die_user_resolve(state);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b5d477919a7..1c6307d456b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -918,7 +918,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);
 		if (!opts->quiet) {
 			if (old_branch_info->path &&
-			    advice_detached_head && !opts->force_detach)
+			    advice_enabled(ADVICE_DETACHED_HEAD) && !opts->force_detach)
 				detach_advice(new_branch_info->name);
 			describe_detached_head(_("HEAD is now at"), new_branch_info->commit);
 		}
@@ -1011,7 +1011,7 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)
 		sb.buf);
 	strbuf_release(&sb);
 
-	if (advice_detached_head)
+	if (advice_enabled(ADVICE_DETACHED_HEAD))
 		fprintf(stderr,
 			Q_(
 			/* The singular version */
@@ -1182,7 +1182,7 @@ static const char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_checkout_ambiguous_remote_branch_name) {
+	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
 		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
 			     "you can do so by fully qualifying the name with the --track option:\n"
 			     "\n"
diff --git a/builtin/clone.c b/builtin/clone.c
index 66fe66679c8..c1603aa82ba 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -786,7 +786,7 @@ static int checkout(int submodule_progress)
 		return 0;
 	}
 	if (!strcmp(head, "HEAD")) {
-		if (advice_detached_head)
+		if (advice_enabled(ADVICE_DETACHED_HEAD))
 			detach_advice(oid_to_hex(&oid));
 		FREE_AND_NULL(head);
 	} else {
diff --git a/builtin/commit.c b/builtin/commit.c
index 243c626307c..df8bcc27ae9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -203,7 +203,7 @@ static void status_init_config(struct wt_status *s, config_fn_t fn)
 	init_diff_ui_defaults();
 	git_config(fn, s);
 	determine_whence(s);
-	s->hints = advice_status_hints; /* must come after git_config() */
+	s->hints = advice_enabled(ADVICE_STATUS_HINTS); /* must come after git_config() */
 }
 
 static void rollback_index_files(void)
@@ -1033,7 +1033,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	if (!committable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
-		s->hints = advice_status_hints;
+		s->hints = advice_enabled(ADVICE_STATUS_HINTS);
 		s->display_comment_prefix = old_display_comment_prefix;
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 25740c13df1..2501e1d10d9 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1229,7 +1229,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		      " 'git remote prune %s' to remove any old, conflicting "
 		      "branches"), remote_name);
 
-	if (advice_fetch_show_forced_updates) {
+	if (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {
 		if (!fetch_show_forced_updates) {
 			warning(_(warn_show_forced_updates));
 		} else if (forced_updates_ms > FORCED_UPDATES_DELAY_WARNING_IN_MS) {
diff --git a/builtin/merge.c b/builtin/merge.c
index 22f23990b37..1b3d98d5fd4 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1368,14 +1368,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * There is no unmerged entry, don't advise 'git
 		 * add/rm <file>', just 'git commit'.
 		 */
-		if (advice_resolve_conflict)
+		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 			die(_("You have not concluded your merge (MERGE_HEAD exists).\n"
 				  "Please, commit your changes before you merge."));
 		else
 			die(_("You have not concluded your merge (MERGE_HEAD exists)."));
 	}
 	if (ref_exists("CHERRY_PICK_HEAD")) {
-		if (advice_resolve_conflict)
+		if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 			die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n"
 			    "Please, commit your changes before you merge."));
 		else
diff --git a/builtin/push.c b/builtin/push.c
index e8b10a9b7ed..4b026ce6c6a 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -289,42 +289,42 @@ static const char message_advice_ref_needs_update[] =
 
 static void advise_pull_before_push(void)
 {
-	if (!advice_push_non_ff_current || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NON_FF_CURRENT) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_pull_before_push));
 }
 
 static void advise_checkout_pull_push(void)
 {
-	if (!advice_push_non_ff_matching || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NON_FF_MATCHING) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_checkout_pull_push));
 }
 
 static void advise_ref_already_exists(void)
 {
-	if (!advice_push_already_exists || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_ALREADY_EXISTS) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_already_exists));
 }
 
 static void advise_ref_fetch_first(void)
 {
-	if (!advice_push_fetch_first || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_FETCH_FIRST) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_fetch_first));
 }
 
 static void advise_ref_needs_force(void)
 {
-	if (!advice_push_needs_force || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_NEEDS_FORCE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_needs_force));
 }
 
 static void advise_ref_needs_update(void)
 {
-	if (!advice_push_ref_needs_update || !advice_push_update_rejected)
+	if (!advice_enabled(ADVICE_PUSH_REF_NEEDS_UPDATE) || !advice_enabled(ADVICE_PUSH_UPDATE_REJECTED))
 		return;
 	advise(_(message_advice_ref_needs_update));
 }
diff --git a/builtin/reset.c b/builtin/reset.c
index 43e855cb887..51c9e2f43ff 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -412,7 +412,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_reset_quiet_warning && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
 					printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset.  You can\n"
 						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
 						"to make this the default.\n"), t_delta_in_ms / 1000.0);
diff --git a/builtin/rm.c b/builtin/rm.c
index 8a24c715e02..3b44b807e5f 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -55,7 +55,7 @@ static void print_error_files(struct string_list *files_list,
 			strbuf_addf(&err_msg,
 				    "\n    %s",
 				    files_list->items[i].string);
-		if (advice_rm_hints)
+		if (advice_enabled(ADVICE_RM_HINTS))
 			strbuf_addstr(&err_msg, hints_msg);
 		*errs = error("%s", err_msg.buf);
 		strbuf_release(&err_msg);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e45..4da9781b991 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1724,7 +1724,7 @@ static int add_possible_reference_from_superproject(
 		} else {
 			switch (sas->error_mode) {
 			case SUBMODULE_ALTERNATE_ERROR_DIE:
-				if (advice_submodule_alternate_error_strategy_die)
+				if (advice_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE))
 					advise(_(alternate_error_advice));
 				die(_("submodule '%s' cannot add alternate: %s"),
 				    sas->submodule_name, err.buf);
diff --git a/editor.c b/editor.c
index 6303ae0ab0d..fdd3eeafa94 100644
--- a/editor.c
+++ b/editor.c
@@ -58,7 +58,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
 		int ret, sig;
-		int print_waiting_for_editor = advice_waiting_for_editor && isatty(2);
+		int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);
 
 		if (print_waiting_for_editor) {
 			/*
diff --git a/notes-merge.c b/notes-merge.c
index 46c1f7c7f11..b4a3a903e86 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -273,7 +273,7 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
 		 */
 		if (file_exists(git_path(NOTES_MERGE_WORKTREE)) &&
 		    !is_empty_dir(git_path(NOTES_MERGE_WORKTREE))) {
-			if (advice_resolve_conflict)
+			if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 				die(_("You have not concluded your previous "
 				    "notes merge (%s exists).\nPlease, use "
 				    "'git notes merge --commit' or 'git notes "
diff --git a/object-name.c b/object-name.c
index 3263c19457f..fdff4601b2c 100644
--- a/object-name.c
+++ b/object-name.c
@@ -806,7 +806,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 			refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref, 0);
 			if (refs_found > 0) {
 				warning(warn_msg, len, str);
-				if (advice_object_name_warning)
+				if (advice_enabled(ADVICE_OBJECT_NAME_WARNING))
 					fprintf(stderr, "%s\n", _(object_name_msg));
 			}
 			free(real_ref);
diff --git a/remote.c b/remote.c
index dfb863d8083..4ef53a6e30b 100644
--- a/remote.c
+++ b/remote.c
@@ -1111,7 +1111,7 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 		"Neither worked, so we gave up. You must fully qualify the ref."),
 	      dst_value, matched_src_name);
 
-	if (!advice_push_unqualified_ref_name)
+	if (!advice_enabled(ADVICE_PUSH_UNQUALIFIED_REF_NAME))
 		return;
 
 	if (get_oid(matched_src_name, &oid))
@@ -2118,7 +2118,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 		strbuf_addf(sb,
 			_("Your branch is based on '%s', but the upstream is gone.\n"),
 			base);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git branch --unset-upstream\" to fixup)\n"));
 	} else if (!sti) {
@@ -2129,7 +2129,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 		strbuf_addf(sb,
 			    _("Your branch and '%s' refer to different commits.\n"),
 			    base);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
 				    "git status --ahead-behind");
 	} else if (!theirs) {
@@ -2138,7 +2138,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			   "Your branch is ahead of '%s' by %d commits.\n",
 			   ours),
 			base, ours);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git push\" to publish your local commits)\n"));
 	} else if (!ours) {
@@ -2149,7 +2149,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			       "and can be fast-forwarded.\n",
 			   theirs),
 			base, theirs);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git pull\" to update your local branch)\n"));
 	} else {
@@ -2162,7 +2162,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 			       "respectively.\n",
 			   ours + theirs),
 			base, ours, theirs);
-		if (advice_status_hints)
+		if (advice_enabled(ADVICE_STATUS_HINTS))
 			strbuf_addstr(sb,
 				_("  (use \"git pull\" to merge the remote branch into yours)\n"));
 	}
diff --git a/run-command.c b/run-command.c
index f72e72cce73..28e3eb57328 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1336,7 +1336,7 @@ const char *find_hook(const char *name)
 			err = errno;
 #endif
 
-		if (err == EACCES && advice_ignored_hook) {
+		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
 			static struct string_list advise_given = STRING_LIST_INIT_DUP;
 
 			if (!string_list_lookup(&advise_given, name)) {
diff --git a/sequencer.c b/sequencer.c
index 7f07cd00f3f..ab63eede45b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -486,7 +486,7 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
 	error(_("your local changes would be overwritten by %s."),
 		_(action_name(opts)));
 
-	if (advice_commit_before_merge)
+	if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE))
 		advise(_("commit your changes or stash them to proceed."));
 	return -1;
 }
@@ -1293,7 +1293,7 @@ void print_commit_summary(struct repository *r,
 	if (!committer_ident_sufficiently_given()) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
-		if (advice_implicit_identity) {
+		if (advice_enabled(ADVICE_IMPLICIT_IDENTITY)) {
 			strbuf_addch(&format, '\n');
 			strbuf_addstr(&format, implicit_ident_advice());
 		}
@@ -3041,7 +3041,7 @@ static int create_seq_dir(struct repository *r)
 	}
 	if (in_progress_error) {
 		error("%s", in_progress_error);
-		if (advice_sequencer_in_use)
+		if (advice_enabled(ADVICE_SEQUENCER_IN_USE))
 			advise(in_progress_advice,
 				advise_skip ? "--skip | " : "");
 		return -1;
@@ -3245,7 +3245,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
 give_advice:
 	error(_("there is nothing to skip"));
 
-	if (advice_resolve_conflict) {
+	if (advice_enabled(ADVICE_RESOLVE_CONFLICT)) {
 		advise(_("have you committed already?\n"
 			 "try \"git %s --continue\""),
 			 action == REPLAY_REVERT ? "revert" : "cherry-pick");
diff --git a/unpack-trees.c b/unpack-trees.c
index 5786645f315..d9907faf7b6 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -111,17 +111,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 	strvec_init(&opts->msgs_to_free);
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
 			  "Please commit your changes or stash them before you switch branches.")
 		      : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by merge:\n%%s"
 			  "Please commit your changes or stash them before you merge.")
 		      : _("Your local changes to the following files would be overwritten by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("Your local changes to the following files would be overwritten by %s:\n%%s"
 			  "Please commit your changes or stash them before you %s.")
 		      : _("Your local changes to the following files would be overwritten by %s:\n%%s");
@@ -132,17 +132,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		_("Updating the following directories would lose untracked files in them:\n%s");
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by checkout:\n%%s"
 			  "Please move or remove them before you switch branches.")
 		      : _("The following untracked working tree files would be removed by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by merge:\n%%s"
 			  "Please move or remove them before you merge.")
 		      : _("The following untracked working tree files would be removed by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by %s:\n%%s"
 			  "Please move or remove them before you %s.")
 		      : _("The following untracked working tree files would be removed by %s:\n%%s");
@@ -150,17 +150,17 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		strvec_pushf(&opts->msgs_to_free, msg, cmd, cmd);
 
 	if (!strcmp(cmd, "checkout"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by checkout:\n%%s"
 			  "Please move or remove them before you switch branches.")
 		      : _("The following untracked working tree files would be overwritten by checkout:\n%%s");
 	else if (!strcmp(cmd, "merge"))
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by merge:\n%%s"
 			  "Please move or remove them before you merge.")
 		      : _("The following untracked working tree files would be overwritten by merge:\n%%s");
 	else
-		msg = advice_commit_before_merge
+		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be overwritten by %s:\n%%s"
 			  "Please move or remove them before you %s.")
 		      : _("The following untracked working tree files would be overwritten by %s:\n%%s");
diff --git a/wt-status.c b/wt-status.c
index eaed30eafba..e4f29b2b4c9 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -787,7 +787,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
 
 	dir_clear(&dir);
 
-	if (advice_status_u_option)
+	if (advice_enabled(ADVICE_STATUS_U_OPTION))
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
@@ -1158,7 +1158,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
 	if (!format_tracking_info(branch, &sb, s->ahead_behind_flags))
 		return;
 
-	if (advice_status_ahead_behind_warning &&
+	if (advice_enabled(ADVICE_STATUS_AHEAD_BEHIND_WARNING) &&
 	    s->ahead_behind_flags == AHEAD_BEHIND_FULL) {
 		uint64_t t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
 		if (t_delta_in_ms > AB_DELAY_WARNING_IN_MS) {
@@ -1845,7 +1845,7 @@ static void wt_longstatus_print(struct wt_status *s)
 		wt_longstatus_print_other(s, &s->untracked, _("Untracked files"), "add");
 		if (s->show_ignored_mode)
 			wt_longstatus_print_other(s, &s->ignored, _("Ignored files"), "add -f");
-		if (advice_status_u_option && 2000 < s->untracked_in_ms) {
+		if (advice_enabled(ADVICE_STATUS_U_OPTION) && 2000 < s->untracked_in_ms) {
 			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 					 _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n"
-- 
2.33.0.663.gfcc3c7013a8

[PATCH v4 3/4] advice: remove use of global advice_add_embedded_repo

[Ævar Arnfjörð Bjarmason]

The external use of this variable was added in 532139940c9 (add: warn
when adding an embedded repository, 2017-06-14). For the use-case it's
more straightforward to track whether we've shown advice in
check_embedded_repo() than setting the global variable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.c      | 2 --
 advice.h      | 1 -
 builtin/add.c | 7 ++++---
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/advice.c b/advice.c
index b18833bc807..41cfea82d06 100644
--- a/advice.c
+++ b/advice.c
@@ -4,7 +4,6 @@
 #include "help.h"
 #include "string-list.h"
 
-int advice_add_embedded_repo = 1;
 int advice_graft_file_deprecated = 1;
 
 static int advice_use_color = -1;
@@ -38,7 +37,6 @@ static struct {
 	const char *name;
 	int *preference;
 } advice_config[] = {
-	{ "addEmbeddedRepo", &advice_add_embedded_repo },
 	{ "graftFileDeprecated", &advice_graft_file_deprecated },
 };
 
diff --git a/advice.h b/advice.h
index ed51db0f057..4b754f4c626 100644
--- a/advice.h
+++ b/advice.h
@@ -5,7 +5,6 @@
 
 struct string_list;
 
-extern int advice_add_embedded_repo;
 extern int advice_graft_file_deprecated;
 
 /*
diff --git a/builtin/add.c b/builtin/add.c
index cf29b302d44..8a5dd29f3f1 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -419,6 +419,7 @@ static const char embedded_advice[] = N_(
 static void check_embedded_repo(const char *path)
 {
 	struct strbuf name = STRBUF_INIT;
+	static int adviced_on_embedded_repo = 0;
 
 	if (!warn_on_embedded_repo)
 		return;
@@ -430,10 +431,10 @@ static void check_embedded_repo(const char *path)
 	strbuf_strip_suffix(&name, "/");
 
 	warning(_("adding embedded git repository: %s"), name.buf);
-	if (advice_add_embedded_repo) {
+	if (!adviced_on_embedded_repo &&
+	    advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {
 		advise(embedded_advice, name.buf, name.buf);
-		/* there may be multiple entries; advise only once */
-		advice_add_embedded_repo = 0;
+		adviced_on_embedded_repo = 1;
 	}
 
 	strbuf_release(&name);
-- 
2.33.0.663.gfcc3c7013a8

[PATCH v4 4/4] advice: move advice.graftFileDeprecated squashing to commit.[ch]

[Ævar Arnfjörð Bjarmason]

Move the squashing of the advice.graftFileDeprecated advice over to an
external variable in commit.[ch], allowing advice() to purely use the
new-style API of invoking advice() with an enum.

See 8821e90a09a (advice: don't pointlessly suggest
--convert-graft-file, 2018-11-27) for why quieting this advice was
needed. It's more straightforward to move this code to commit.[ch] and
use it builtin/replace.c, than to go through the indirection of
advice.[ch].

Because this was the last advice_config variable we can remove that
old facility from advice.c.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.c          | 16 ----------------
 advice.h          |  2 --
 builtin/replace.c |  2 +-
 commit.c          |  4 +++-
 commit.h          |  1 +
 5 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/advice.c b/advice.c
index 41cfea82d06..e716ddebffe 100644
--- a/advice.c
+++ b/advice.c
@@ -4,8 +4,6 @@
 #include "help.h"
 #include "string-list.h"
 
-int advice_graft_file_deprecated = 1;
-
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
@@ -33,13 +31,6 @@ static const char *advise_get_color(enum color_advice ix)
 	return "";
 }
 
-static struct {
-	const char *name;
-	int *preference;
-} advice_config[] = {
-	{ "graftFileDeprecated", &advice_graft_file_deprecated },
-};
-
 static struct {
 	const char *key;
 	int enabled;
@@ -162,13 +153,6 @@ int git_default_advice_config(const char *var, const char *value)
 	if (!skip_prefix(var, "advice.", &k))
 		return 0;
 
-	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
-		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;
diff --git a/advice.h b/advice.h
index 4b754f4c626..e380a0562e3 100644
--- a/advice.h
+++ b/advice.h
@@ -5,8 +5,6 @@
 
 struct string_list;
 
-extern int advice_graft_file_deprecated;
-
 /*
  * To add a new advice, you need to:
  * Define a new advice_type.
diff --git a/builtin/replace.c b/builtin/replace.c
index cd487659117..946938d011e 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -507,7 +507,7 @@ static int convert_graft_file(int force)
 	if (!fp)
 		return -1;
 
-	advice_graft_file_deprecated = 0;
+	no_graft_file_deprecated_advice = 1;
 	while (strbuf_getline(&buf, fp) != EOF) {
 		if (*buf.buf == '#')
 			continue;
diff --git a/commit.c b/commit.c
index 143f472c0f2..551de4903c1 100644
--- a/commit.c
+++ b/commit.c
@@ -25,6 +25,7 @@
 static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
 
 int save_commit_buffer = 1;
+int no_graft_file_deprecated_advice;
 
 const char *commit_type = "commit";
 
@@ -190,7 +191,8 @@ static int read_graft_file(struct repository *r, const char *graft_file)
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
-	if (advice_graft_file_deprecated)
+	if (!no_graft_file_deprecated_advice &&
+	    advice_enabled(ADVICE_GRAFT_FILE_DEPRECATED))
 		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
 			 "and will be removed in a future Git version.\n"
 			 "\n"
diff --git a/commit.h b/commit.h
index df42eb434f3..3ea32766bcb 100644
--- a/commit.h
+++ b/commit.h
@@ -41,6 +41,7 @@ struct commit {
 };
 
 extern int save_commit_buffer;
+extern int no_graft_file_deprecated_advice;
 extern const char *commit_type;
 
 /* While we can decorate any object with a name, it's only used for commits.. */
-- 
2.33.0.663.gfcc3c7013a8

Re: [PATCH v4 4/4] advice: move advice.graftFileDeprecated squashing to commit.[ch]

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> -static struct {
> -	const char *name;
> -	int *preference;
> -} advice_config[] = {
> -	{ "graftFileDeprecated", &advice_graft_file_deprecated },
> -};
> -

Nice.