Re: [PATCH v3 1/2] advice: revamp advise API

[Emily Shaffer <emilyshaffer@google.com>]

On Wed, Feb 19, 2020 at 08:34:00PM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
> 
> Currently it's very easy for the advice library's callers to miss
> checking the visibility step before printing an advice. Also, it makes
> more sense for this step to be handled by the advice library.
> 
> Add a new advise_if_enabled function that checks the visibility of
> advice messages before printing.
> 
> Add a new helper advise_enabled to check the visibility of the advice
> if the caller needs to carry out complicated processing based on that
> value.
> 
> A list of config variables 'advice_config_keys' is added to be used by
> list_config_advices() instead of 'advice_config[]' because we'll get
> rid of 'advice_config[]' and the global variables once we migrate all
> the callers to use the new APIs.
> 
> Also change the advise call in tag library from advise() to
> advise_if_enabled() to construct an example of the usage of the new
> API.
> 
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
>  Makefile               |  1 +
>  advice.c               | 92 ++++++++++++++++++++++++++++++++++++++++--
>  advice.h               | 53 +++++++++++++++++++++++-
>  builtin/tag.c          |  4 +-
>  t/helper/test-advise.c | 16 ++++++++
>  t/helper/test-tool.c   |  1 +
>  t/helper/test-tool.h   |  1 +
>  t/t0018-advice.sh      | 28 +++++++++++++
>  t/t7004-tag.sh         |  1 +
>  9 files changed, 190 insertions(+), 7 deletions(-)
>  create mode 100644 t/helper/test-advise.c
>  create mode 100755 t/t0018-advice.sh
> 
> diff --git a/Makefile b/Makefile
> index 09f98b777ca..ed923a3e818 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -695,6 +695,7 @@ X =
>  
>  PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
>  
> +TEST_BUILTINS_OBJS += test-advise.o
>  TEST_BUILTINS_OBJS += test-chmtime.o
>  TEST_BUILTINS_OBJS += test-config.o
>  TEST_BUILTINS_OBJS += test-ctype.o
> diff --git a/advice.c b/advice.c
> index 249c60dcf32..345319005ac 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -29,7 +29,6 @@ int advice_ignored_hook = 1;
>  int advice_waiting_for_editor = 1;
>  int advice_graft_file_deprecated = 1;
>  int advice_checkout_ambiguous_remote_branch_name = 1;
> -int advice_nested_tag = 1;
>  int advice_submodule_alternate_error_strategy_die = 1;
>  
>  static int advice_use_color = -1;
> @@ -89,13 +88,46 @@ static struct {
>  	{ "waitingForEditor", &advice_waiting_for_editor },
>  	{ "graftFileDeprecated", &advice_graft_file_deprecated },
>  	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
> -	{ "nestedTag", &advice_nested_tag },
>  	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
>  
>  	/* make this an alias for backward compatibility */
>  	{ "pushNonFastForward", &advice_push_update_rejected }
>  };
>  
> +static const char *advice_config_keys[] = {
> +	[FETCH_SHOW_FORCED_UPDATES]		 = "fetchShowForcedUpdates",
> +	[PUSH_UPDATE_REJECTED]			 = "pushUpdateRejected",
> +	/* make this an alias for backward compatibility */
> +	[PUSH_UPDATE_REJECTED_ALIAS]		 = "pushNonFastForward",
> +
> +	[PUSH_NON_FF_CURRENT]			 = "pushNonFFCurrent",
> +	[PUSH_NON_FF_MATCHING]			 = "pushNonFFMatching",
> +	[PUSH_ALREADY_EXISTS]			 = "pushAlreadyExists",
> +	[PUSH_FETCH_FIRST]			 = "pushFetchFirst",
> +	[PUSH_NEEDS_FORCE]			 = "pushNeedsForce",
> +	[PUSH_UNQUALIFIED_REF_NAME]		 = "pushUnqualifiedRefName",
> +	[STATUS_HINTS]				 = "statusHints",
> +	[STATUS_U_OPTION]			 = "statusUoption",
> +	[STATUS_AHEAD_BEHIND_WARNING]		 = "statusAheadBehindWarning",
> +	[COMMIT_BEFORE_MERGE]			 = "commitBeforeMerge",
> +	[RESET_QUIET_WARNING]			 = "resetQuiet",
> +	[RESOLVE_CONFLICT]			 = "resolveConflict",
> +	[SEQUENCER_IN_USE]			 = "sequencerInUse",
> +	[IMPLICIT_IDENTITY]			 = "implicitIdentity",
> +	[DETACHED_HEAD]				 = "detachedHead",
> +	[SET_UPSTREAM_FAILURE]			 = "setupStreamFailure",
> +	[OBJECT_NAME_WARNING]			 = "objectNameWarning",
> +	[AMWORKDIR]				 = "amWorkDir",
> +	[RM_HINTS]				 = "rmHints",
> +	[ADD_EMBEDDED_REPO]			 = "addEmbeddedRepo",
> +	[IGNORED_HOOK]				 = "ignoredHook",
> +	[WAITING_FOR_EDITOR] 			 = "waitingForEditor",
> +	[GRAFT_FILE_DEPRECATED]			 = "graftFileDeprecated",
> +	[CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME]	 = "checkoutAmbiguousRemoteBranchName",
> +	[NESTED_TAG]				 = "nestedTag",
> +	[SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = "submoduleAlternateErrorStrategyDie"
> +};
> +
>  void advise(const char *advice, ...)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> @@ -118,6 +150,58 @@ void advise(const char *advice, ...)
>  	strbuf_release(&buf);
>  }
>  
> +static int get_config_value(enum advice_type type)
> +{
> +	int value = 1;
> +	char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);

Same comment as your other call for xstrfmt. I think you need to manage
the output.
> +	git_config_get_bool(key, &value);
> +	return value;
> +}
> +
> +int advice_enabled(enum advice_type type)
> +{
> +	switch(type) {
> +	case PUSH_UPDATE_REJECTED:
> +		return get_config_value(PUSH_UPDATE_REJECTED) &&
> +		       get_config_value(PUSH_UPDATE_REJECTED_ALIAS);
> +	default:
> +		return get_config_value(type);
> +	}
> +}
> +
> +static const char turn_off_instructions[] =
> +N_("\n"
> +   "Disable this message with \"git config %s false\"");
> +
> +void advise_if_enabled(enum advice_type type, const char *advice, ...)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);

Hmm, doesn't this leak?

On the surface it looks like xstrfmt can save you a strbuf allocation,
but if you check in strbuf.c, it actually allocates and detaches a
strbuf for you anyways. I'd argue that it's easier to tell whether
you're leaking a strbuf than the result of this call, so you might as
well do it that way.

> +	va_list params;
> +	const char *cp, *np;
> +	
> +	if(!advice_enabled(type))
> +		return;
> +
> +	va_start(params, advice);
> +	strbuf_vaddf(&buf, advice, params);
> +	va_end(params);
> +
> +	strbuf_addf(&buf, turn_off_instructions, key);
> +	
> +	for (cp = buf.buf; *cp; cp = np) {
> +		np = strchrnul(cp, '\n');
> +		fprintf(stderr,	_("%shint: %.*s%s\n"),
> +			advise_get_color(ADVICE_COLOR_HINT),
> +			(int)(np - cp), cp,
> +			advise_get_color(ADVICE_COLOR_RESET));
> +		if (*np)
> +			np++;
> +	}

Hm. This seems like something the project would use if strbuf knew how
to do it. Something like strbuf_prefix_lines(strbuf, prefix, delimiter)
and strbuf_suffix_lines(strbuf, suffix, delimiter)? I think there are
other types of output which need to prepend each line like this?

> +	strbuf_release(&buf);
> +
> +}
> +
>  int git_default_advice_config(const char *var, const char *value)
>  {
>  	const char *k, *slot_name;
> @@ -154,8 +238,8 @@ void list_config_advices(struct string_list *list, const char *prefix)
>  {
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(advice_config); i++)
> -		list_config_item(list, prefix, advice_config[i].name);
> +	for (i = 0; i < ARRAY_SIZE(advice_config_keys); i++)
> +		list_config_item(list, prefix, advice_config_keys[i]);
>  }
>  
>  int error_resolve_conflict(const char *me)
> diff --git a/advice.h b/advice.h
> index b706780614d..c8be662c4b1 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -29,12 +29,63 @@ extern int advice_ignored_hook;
>  extern int advice_waiting_for_editor;
>  extern int advice_graft_file_deprecated;
>  extern int advice_checkout_ambiguous_remote_branch_name;
> -extern int advice_nested_tag;
>  extern int advice_submodule_alternate_error_strategy_die;
>  
> +/**
> + To add a new advice, you need to:
> + - Define an advice_type.
> + - Add a new entry to advice_config_keys list.
> + - Add the new config variable to Documentation/config/advice.txt.
> + - Call advise_if_enabled to print your advice.
> + */
> +enum advice_type {
> +	FETCH_SHOW_FORCED_UPDATES = 0,
> +	PUSH_UPDATE_REJECTED = 1,
> +	PUSH_UPDATE_REJECTED_ALIAS = 2,
> +	PUSH_NON_FF_CURRENT = 3,
> +	PUSH_NON_FF_MATCHING = 4,
> +	PUSH_ALREADY_EXISTS = 5,
> +	PUSH_FETCH_FIRST = 6,
> +	PUSH_NEEDS_FORCE = 7,
> +	PUSH_UNQUALIFIED_REF_NAME = 8,
> +	STATUS_HINTS = 9,
> +	STATUS_U_OPTION = 10,
> +	STATUS_AHEAD_BEHIND_WARNING = 11,
> +	COMMIT_BEFORE_MERGE = 12,
> +	RESET_QUIET_WARNING = 13,
> +	RESOLVE_CONFLICT = 14,
> +	SEQUENCER_IN_USE = 15,
> +	IMPLICIT_IDENTITY = 16,
> +	DETACHED_HEAD = 17,
> +	SET_UPSTREAM_FAILURE = 18,
> +	OBJECT_NAME_WARNING = 19,
> +	AMWORKDIR = 20,
> +	RM_HINTS = 21,
> +	ADD_EMBEDDED_REPO = 22,
> +	IGNORED_HOOK = 23,
> +	WAITING_FOR_EDITOR = 24,
> +	GRAFT_FILE_DEPRECATED = 25,
> +	CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME = 26,
> +	NESTED_TAG = 27,
> +	SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE = 28,
> +};

Hmm. I wanted to say, "Our codebase uses ALL_CAPS or snake_case in enums
so you could use lowers if you wanted" - but based on 'git grep -A4
"^enum"' it's actually pretty unusual to see enums with lower-case
members. Dang :)

> +
> +
>  int git_default_advice_config(const char *var, const char *value);
>  __attribute__((format (printf, 1, 2)))
>  void advise(const char *advice, ...);
> +
> +/**
> + Checks if advice type is enabled (can be printed to the user).
> + Should be called before advise().
> + */
> +int advice_enabled(enum advice_type type);
> +
> +/**
> + Checks the visibility of the advice before printing.
> + */
> +void advise_if_enabled(enum advice_type type, const char *advice, ...);
> +
>  int error_resolve_conflict(const char *me);
>  void NORETURN die_resolve_conflict(const char *me);
>  void NORETURN die_conclude_merge(void);
> diff --git a/builtin/tag.c b/builtin/tag.c
> index e0a4c253828..247d9075e19 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -231,8 +231,8 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>  	if (type <= OBJ_NONE)
>  		die(_("bad object type."));
>  
> -	if (type == OBJ_TAG && advice_nested_tag)
> -		advise(_(message_advice_nested_tag), tag, object_ref);
> +	if (type == OBJ_TAG)
> +		advise_if_enabled(NESTED_TAG, _(message_advice_nested_tag), tag, object_ref);

Hm, if it was me, I would have put this bit in its own commit. But I see
that it's a pretty tiny change...

>  
>  	strbuf_addf(&header,
>  		    "object %s\n"
> diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
> new file mode 100644
> index 00000000000..6d28c9cd5aa
> --- /dev/null
> +++ b/t/helper/test-advise.c
> @@ -0,0 +1,16 @@
> +#include "test-tool.h"
> +#include "cache.h"
> +#include "advice.h"
> +
> +int cmd__advise_if_enabled(int argc, const char **argv)
> +{
> +	if (!argv[1])
> +	die("usage: %s <advice>", argv[0]);
> +
> +	setup_git_directory();
> +
> +	//use any advice type for testing
I think this might be misleading - your t0018 does quite a few checks
explicitly for the NESTED_TAG advice. Maybe it's better to say something
like "Make sure this agrees with t0018"? Also nit, I've been told off a
few times for using //c++ style comments.
> +	advise_if_enabled(NESTED_TAG, argv[1]);
> +
> +	return 0;
> +}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index f20989d4497..6977badc690 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -14,6 +14,7 @@ struct test_cmd {
>  };
>  
>  static struct test_cmd cmds[] = {
> +	{ "advise", cmd__advise_if_enabled },
>  	{ "chmtime", cmd__chmtime },
>  	{ "config", cmd__config },
>  	{ "ctype", cmd__ctype },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 8ed2af71d1b..ca5e33b842f 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -4,6 +4,7 @@
>  #define USE_THE_INDEX_COMPATIBILITY_MACROS
>  #include "git-compat-util.h"
>  
> +int cmd__advise_if_enabled(int argc, const char **argv);
>  int cmd__chmtime(int argc, const char **argv);
>  int cmd__config(int argc, const char **argv);
>  int cmd__ctype(int argc, const char **argv);
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> new file mode 100755
> index 00000000000..f4cdb649d51
> --- /dev/null
> +++ b/t/t0018-advice.sh
> @@ -0,0 +1,28 @@
> +#!/bin/sh
> +
> +test_description='Test advise_if_enabled functionality'
> +
> +. ./test-lib.sh
> +
> +cat > expected <<EOF
> +hint: This is a piece of advice
> +hint: Disable this message with "git config advice.nestedTag false"
> +EOF
> +test_expect_success 'advise should be printed when config variable is unset' '
> +	test-tool advise "This is a piece of advice" 2>actual &&
> +	test_i18ncmp expected actual
> +'
> +
> +test_expect_success 'advise should be printed when config variable is set to true' '
> +	test_config advice.nestedTag true &&
> +	test-tool advise "This is a piece of advice" 2>actual &&
> +	test_i18ncmp expected actual
> +'
> +
> +test_expect_success 'advise should not be printed when config variable is set to false' '
> +	test_config advice.nestedTag false &&
> +	test-tool advise "This is a piece of advice" 2>actual &&
> +	test_must_be_empty actual
> +'
> +
> +test_done
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 6db92bd3ba6..74b637deb25 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1726,6 +1726,7 @@ test_expect_success 'recursive tagging should give advice' '
>  	hint: already a tag. If you meant to tag the object that it points to, use:
>  	hint: |
>  	hint: 	git tag -f nested annotated-v4.0^{}
> +	hint: Disable this message with "git config advice.nestedTag false"
>  	EOF
>  	git tag -m nested nested annotated-v4.0 2>actual &&
>  	test_i18ncmp expect actual
> -- 
> gitgitgadget
>