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

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