From f668d9b7ca04da1d8adf0a146b48a46559471501 Mon Sep 17 00:00:00 2001 From: Heba Waly Date: Fri, 21 Feb 2020 14:20:13 +1300 Subject: [PATCH 1/4] advice: extract vadvise() from advise() In preparation for a new advice method, extract a version of advise() that uses an explict 'va_list' parameter. Call it from advise() for a functionally equivalent version. Signed-off-by: Derrick Stolee Signed-off-by: Heba Waly --- advice.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/advice.c b/advice.c index 249c60dcf32e24..fd836332dad756 100644 --- a/advice.c +++ b/advice.c @@ -96,15 +96,12 @@ static struct { { "pushNonFastForward", &advice_push_update_rejected } }; -void advise(const char *advice, ...) +static void vadvise(const char *advice, va_list params) { struct strbuf buf = STRBUF_INIT; - va_list params; const char *cp, *np; - va_start(params, advice); strbuf_vaddf(&buf, advice, params); - va_end(params); for (cp = buf.buf; *cp; cp = np) { np = strchrnul(cp, '\n'); @@ -118,6 +115,14 @@ void advise(const char *advice, ...) strbuf_release(&buf); } +void advise(const char *advice, ...) +{ + va_list params; + va_start(params, advice); + vadvise(advice, params); + va_end(params); +} + int git_default_advice_config(const char *var, const char *value) { const char *k, *slot_name; From 828b4aee19d09218beca6ad1d93006012be99a0a Mon Sep 17 00:00:00 2001 From: Heba Waly Date: Thu, 27 Feb 2020 15:22:10 +1300 Subject: [PATCH 2/4] advice: change "setupStreamFailure" to "setUpstreamFailure" fb6fbffbda (advice: keep config name in camelCase in advice_config[], 2018-05-26) changed the config names to camelCase, but one of the names wasn't changed correctly. Fix it. Signed-off-by: Heba Waly --- advice.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/advice.c b/advice.c index fd836332dad756..258cc9ba7af02b 100644 --- a/advice.c +++ b/advice.c @@ -80,7 +80,7 @@ static struct { { "sequencerInUse", &advice_sequencer_in_use }, { "implicitIdentity", &advice_implicit_identity }, { "detachedHead", &advice_detached_head }, - { "setupStreamFailure", &advice_set_upstream_failure }, + { "setUpstreamFailure", &advice_set_upstream_failure }, { "objectNameWarning", &advice_object_name_warning }, { "amWorkDir", &advice_amworkdir }, { "rmHints", &advice_rm_hints }, From a4673803eaf881f7caa25d97e432c0ff0d065060 Mon Sep 17 00:00:00 2001 From: Heba Waly Date: Thu, 6 Feb 2020 13:43:55 +1300 Subject: [PATCH 3/4] advice: revamp advise API 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 advice_settings is added to cache the config variables names and values, it's intended to replace advice_config[] and the global variables once we migrate all the callers to use the new APIs. Signed-off-by: Heba Waly --- Makefile | 1 + advice.c | 84 ++++++++++++++++++++++++++++++++++++++++-- advice.h | 51 +++++++++++++++++++++++++ t/helper/test-advise.c | 21 +++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0018-advice.sh | 32 ++++++++++++++++ 7 files changed, 187 insertions(+), 4 deletions(-) create mode 100644 t/helper/test-advise.c create mode 100755 t/t0018-advice.sh diff --git a/Makefile b/Makefile index 09f98b777cae1d..ed923a3e818c5e 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 258cc9ba7af02b..9814d6cdfd8b24 100644 --- a/advice.c +++ b/advice.c @@ -96,13 +96,59 @@ static struct { { "pushNonFastForward", &advice_push_update_rejected } }; -static void vadvise(const char *advice, va_list params) +static struct { + const char *key; + int enabled; +} advice_setting[] = { + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 }, + [ADVICE_AM_WORK_DIR] = { "amWorkDir", 1 }, + [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName", 1 }, + [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge", 1 }, + [ADVICE_DETACHED_HEAD] = { "detachedHead", 1 }, + [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates", 1 }, + [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 }, + [ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 }, + [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity", 1 }, + [ADVICE_NESTED_TAG] = { "nestedTag", 1 }, + [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning", 1 }, + [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists", 1 }, + [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst", 1 }, + [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce", 1 }, + + /* make this an alias for backward compatibility */ + [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward", 1 }, + + [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent", 1 }, + [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching", 1 }, + [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName", 1 }, + [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected", 1 }, + [ADVICE_RESET_QUIET_WARNING] = { "resetQuiet", 1 }, + [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict", 1 }, + [ADVICE_RM_HINTS] = { "rmHints", 1 }, + [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse", 1 }, + [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure", 1 }, + [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning", 1 }, + [ADVICE_STATUS_HINTS] = { "statusHints", 1 }, + [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 }, + [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 }, + [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, +}; + +static const char turn_off_instructions[] = +N_("\n" + "Disable this message with \"git config advice.%s false\""); + +static void vadvise(const char *advice, int display_instructions, + const char *key, va_list params) { struct strbuf buf = STRBUF_INIT; const char *cp, *np; strbuf_vaddf(&buf, advice, params); + if (display_instructions) + strbuf_addf(&buf, turn_off_instructions, key); + for (cp = buf.buf; *cp; cp = np) { np = strchrnul(cp, '\n'); fprintf(stderr, _("%shint: %.*s%s\n"), @@ -119,7 +165,30 @@ void advise(const char *advice, ...) { va_list params; va_start(params, advice); - vadvise(advice, params); + vadvise(advice, 0, "", params); + va_end(params); +} + +int advice_enabled(enum advice_type type) +{ + switch(type) { + case ADVICE_PUSH_UPDATE_REJECTED: + return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled && + advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled; + default: + return advice_setting[type].enabled; + } +} + +void advise_if_enabled(enum advice_type type, const char *advice, ...) +{ + va_list params; + + if (!advice_enabled(type)) + return; + + va_start(params, advice); + vadvise(advice, 1, advice_setting[type].key, params); va_end(params); } @@ -149,6 +218,13 @@ int git_default_advice_config(const char *var, const char *value) if (strcasecmp(k, advice_config[i].name)) continue; *advice_config[i].preference = git_config_bool(var, value); + break; + } + + for (i = 0; i < ARRAY_SIZE(advice_setting); i++) { + if (strcasecmp(k, advice_setting[i].key)) + continue; + advice_setting[i].enabled = git_config_bool(var, value); return 0; } @@ -159,8 +235,8 @@ void list_config_advices(struct string_list *list, const char *prefix) { int i; - for (i = 0; i < ARRAY_SIZE(advice_config); i++) - list_config_item(list, prefix, advice_config[i].name); + for (i = 0; i < ARRAY_SIZE(advice_setting); i++) + list_config_item(list, prefix, advice_setting[i].key); } int error_resolve_conflict(const char *me) diff --git a/advice.h b/advice.h index b706780614dd37..d72ab4a06c1b94 100644 --- a/advice.h +++ b/advice.h @@ -32,9 +32,60 @@ 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 a new advice_type. + * Add a new entry to advice_setting array. + * Add the new config variable to Documentation/config/advice.txt. + * Call advise_if_enabled to print your advice. + */ + enum advice_type { + ADVICE_ADD_EMBEDDED_REPO, + ADVICE_AM_WORK_DIR, + ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME, + ADVICE_COMMIT_BEFORE_MERGE, + ADVICE_DETACHED_HEAD, + ADVICE_FETCH_SHOW_FORCED_UPDATES, + ADVICE_GRAFT_FILE_DEPRECATED, + ADVICE_IGNORED_HOOK, + ADVICE_IMPLICIT_IDENTITY, + ADVICE_NESTED_TAG, + ADVICE_OBJECT_NAME_WARNING, + ADVICE_PUSH_ALREADY_EXISTS, + ADVICE_PUSH_FETCH_FIRST, + ADVICE_PUSH_NEEDS_FORCE, + ADVICE_PUSH_NON_FF_CURRENT, + ADVICE_PUSH_NON_FF_MATCHING, + ADVICE_PUSH_UNQUALIFIED_REF_NAME, + ADVICE_PUSH_UPDATE_REJECTED_ALIAS, + ADVICE_PUSH_UPDATE_REJECTED, + ADVICE_RESET_QUIET_WARNING, + ADVICE_RESOLVE_CONFLICT, + ADVICE_RM_HINTS, + ADVICE_SEQUENCER_IN_USE, + ADVICE_SET_UPSTREAM_FAILURE, + ADVICE_STATUS_AHEAD_BEHIND_WARNING, + ADVICE_STATUS_HINTS, + ADVICE_STATUS_U_OPTION, + ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE, + ADVICE_WAITING_FOR_EDITOR, +}; + int git_default_advice_config(const char *var, const char *value); __attribute__((format (printf, 1, 2))) void advise(const char *advice, ...); + +/** + * Checks if advice type is enabled (can be printed to the user). + * Should be called before advise(). + */ +int advice_enabled(enum advice_type type); + +/** + * Checks the visibility of the advice before printing. + */ +void advise_if_enabled(enum advice_type type, const char *advice, ...); + int error_resolve_conflict(const char *me); void NORETURN die_resolve_conflict(const char *me); void NORETURN die_conclude_merge(void); diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c new file mode 100644 index 00000000000000..cdafa413e378db --- /dev/null +++ b/t/helper/test-advise.c @@ -0,0 +1,21 @@ +#include "test-tool.h" +#include "cache.h" +#include "advice.h" +#include "config.h" + +int cmd__advise_if_enabled(int argc, const char **argv) +{ + if (!argv[1]) + die("usage: %s ", argv[0]); + + setup_git_directory(); + git_config(git_default_config, NULL); + + /* + Any advice type can be used for testing, but NESTED_TAG was selected + here and in t0018 where this command is being executed. + */ + advise_if_enabled(ADVICE_NESTED_TAG, argv[1]); + + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index f20989d4497b59..6977badc69095a 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 8ed2af71d1b238..ca5e33b842f51b 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 00000000000000..e03554d2f3457b --- /dev/null +++ b/t/t0018-advice.sh @@ -0,0 +1,32 @@ +#!/bin/sh + +test_description='Test advise_if_enabled functionality' + +. ./test-lib.sh + +test_expect_success 'advice should be printed when config variable is unset' ' + cat >expect <<-\EOF && + hint: This is a piece of advice + hint: Disable this message with "git config advice.nestedTag false" + EOF + test-tool advise "This is a piece of advice" 2>actual && + test_i18ncmp expect actual +' + +test_expect_success 'advice should be printed when config variable is set to true' ' + cat >expect <<-\EOF && + hint: This is a piece of advice + hint: Disable this message with "git config advice.nestedTag false" + EOF + test_config advice.nestedTag true && + test-tool advise "This is a piece of advice" 2>actual && + test_i18ncmp expect actual +' + +test_expect_success 'advice should not be printed when config variable is set to false' ' + test_config advice.nestedTag false && + test-tool advise "This is a piece of advice" 2>actual && + test_must_be_empty actual +' + +test_done From 3e213cb59b7237fda0f3cea125155e961b6a462e Mon Sep 17 00:00:00 2001 From: Heba Waly Date: Sat, 22 Feb 2020 03:38:58 +1300 Subject: [PATCH 4/4] tag: use new advice API to check visibility 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 --- advice.c | 2 -- advice.h | 1 - builtin/tag.c | 5 +++-- t/t7004-tag.sh | 1 + 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/advice.c b/advice.c index 9814d6cdfd8b24..ef4bef65d8ca8c 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,7 +88,6 @@ static struct { { "waitingForEditor", &advice_waiting_for_editor }, { "graftFileDeprecated", &advice_graft_file_deprecated }, { "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name }, - { "nestedTag", &advice_nested_tag }, { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die }, /* make this an alias for backward compatibility */ diff --git a/advice.h b/advice.h index d72ab4a06c1b94..77cbe5c6b362a7 100644 --- a/advice.h +++ b/advice.h @@ -29,7 +29,6 @@ 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; /* diff --git a/builtin/tag.c b/builtin/tag.c index e0a4c25382846f..cc30d346f5d5ba 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -231,8 +231,9 @@ static void create_tag(const struct object_id *object, const char *object_ref, if (type <= OBJ_NONE) die(_("bad object type.")); - if (type == OBJ_TAG && advice_nested_tag) - advise(_(message_advice_nested_tag), tag, object_ref); + if (type == OBJ_TAG) + advise_if_enabled(ADVICE_NESTED_TAG, _(message_advice_nested_tag), + tag, object_ref); strbuf_addf(&header, "object %s\n" diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 6db92bd3ba62db..74b637deb259a9 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