Re: [PATCH v5 2/3] advice: revamp advise API
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 25, 2020, 17:40 UTC
- Message-ID
- <xmqqsgiymupf.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <b7f10d060a41c1ef3d25e4c07be3747c7902b997.1582628141.git.gitgitgadget@gmail.com>
"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 18 quoted lines
> 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.
This is for step [3/3], isn't it? I'll discard this paragraph.
Show 106 quoted lines
>
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
> Makefile | 1 +
> advice.c | 86 ++++++++++++++++++++++++++++++++++++++++--
> advice.h | 52 +++++++++++++++++++++++++
> t/helper/test-advise.c | 19 ++++++++++
> t/helper/test-tool.c | 1 +
> t/helper/test-tool.h | 1 +
> t/t0018-advice.sh | 32 ++++++++++++++++
> 7 files changed, 188 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 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 fd836332dad..5c2068b8f8a 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -96,13 +96,56 @@ static struct {
> { "pushNonFastForward", &advice_push_update_rejected }
> };
>
> -static void vadvise(const char *advice, va_list params)
> +static const char *advice_config_keys[] = {
> + [ADD_EMBEDDED_REPO] = "addEmbeddedRepo",
> + [AMWORKDIR] = "amWorkDir",
> + [CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = "checkoutAmbiguousRemoteBranchName",
> + [COMMIT_BEFORE_MERGE] = "commitBeforeMerge",
> + [DETACHED_HEAD] = "detachedHead",
> + [FETCH_SHOW_FORCED_UPDATES] = "fetchShowForcedUpdates",
> + [GRAFT_FILE_DEPRECATED] = "graftFileDeprecated",
> + [IGNORED_HOOK] = "ignoredHook",
> + [IMPLICIT_IDENTITY] = "implicitIdentity",
> + [NESTED_TAG] = "nestedTag",
> + [OBJECT_NAME_WARNING] = "objectNameWarning",
> + [PUSH_ALREADY_EXISTS] = "pushAlreadyExists",
> + [PUSH_FETCH_FIRST] = "pushFetchFirst",
> + [PUSH_NEEDS_FORCE] = "pushNeedsForce",
> +
> + /* make this an alias for backward compatibility */
> + [PUSH_UPDATE_REJECTED_ALIAS] = "pushNonFastForward",
> +
> + [PUSH_NON_FF_CURRENT] = "pushNonFFCurrent",
> + [PUSH_NON_FF_MATCHING] = "pushNonFFMatching",
> + [PUSH_UNQUALIFIED_REF_NAME] = "pushUnqualifiedRefName",
> + [PUSH_UPDATE_REJECTED] = "pushUpdateRejected",
> + [RESET_QUIET_WARNING] = "resetQuiet",
> + [RESOLVE_CONFLICT] = "resolveConflict",
> + [RM_HINTS] = "rmHints",
> + [SEQUENCER_IN_USE] = "sequencerInUse",
> + [SET_UPSTREAM_FAILURE] = "setupStreamFailure",
> + [STATUS_AHEAD_BEHIND_WARNING] = "statusAheadBehindWarning",
> + [STATUS_HINTS] = "statusHints",
> + [STATUS_U_OPTION] = "statusUoption",
> + [SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = "submoduleAlternateErrorStrategyDie",
> + [WAITING_FOR_EDITOR] = "waitingForEditor",
> +};
> +
> +static const char turn_off_instructions[] =
> +N_("\n"
> + "Disable this message with \"git config %s false\"");
> +
> +static void vadvise(const char *advice, int display_instructions,
> + 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,8 +162,43 @@ void advise(const char *advice, ...)
> {
> va_list params;
> va_start(params, advice);
> - vadvise(advice, params);
> + vadvise(advice, 0, "", params);
> + va_end(params);
> +}
> +
> +static int get_config_value(enum advice_type type)
> +{
> + int value = 1;
> + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);
> +
> + git_config_get_bool(key, &value);
> + free(key);
> + return value;
> +}So, in this hypothetical but quite realistic example:
if (advice_enabled(ADVICE_FOO)) {
char *foo = expensive_preparation();
advice_if_enabled(ADVICE_FOO, "use of %s is discouraged", foo);
}we end up formulating the "advice.*" key twice and ask git_config_get_bool() about the same key twice?
Show 10 quoted lines
> +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);
> + }
> +}Also, as "enum advice_type" will be part of the public API, and there is little type checking for enums, we shouldn't be naming them randomly like these---we'd at least want to use a common prefix, like "ADVICE_", in front of them. Those who are focused only on advice subsystem may feel that names like PUSH_UPDATE_REJECTED are sufficiently clear, but within the context of the whole system, there is no cue that these UPCASED_WORDS identifiers belong to the advice subsystem or somewhere else.
Show 7 quoted lines
> +void advise_if_enabled(enum advice_type type, const char *advice, ...)
> +{
> + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);
> + va_list params;
> +
> + if (!advice_enabled(type))
> + return;Oh, no, make the number of calls to xstrfmr() three times, not twice, as I said in the previous example.
I wonder if it would make the implementation better to do these:
- Rename advice_config_keys[] to advice_setting[] that does not imply it is only about the keys;
- This table will know, for each enum advice_type, which configuration variable enables it, *and* if it is enabled.
i.e.
static struct {
const char *config_key;
int disabled;
} advice_setting[] = {
[ADVICE_ADD_EMBEDED_REPO] = { "addEmbeddedRepo" },
[ADVICE_AM_WORK_DIR] = { "amWorkDir" },
...
[ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" },
};Side Note: you have AMWORKDIR that is unreadable. If the config
name uses camelCase by convention, the UPCASED_WORDS
should be separated with underscore at the same word
boundary.Then, upon the first call to advice_enabled(), call git_config() with a callback like
static int populate_advice_settings(const char *var, const char *value, void *cb)
{
int advice_type;
const char *name;if (!skip_prefix(var, "advice.", &name)) return 0; advice_type = find_advice_type_by_name(advice_setting, name); if (advice_type < 0) return 0; /* unknown advice.* variable */ /* advice.foo=false means advice.foo is disabled */ advice_setting[advice_type].disabled = !git_config_bool(var, value); }
only once. Your get_config_value() would then become a mere lookup in advice_setting[] array, e.g.
int advice_enabled(unsigned advice_type)
{
static int initialized; if (!initialized) {
initialized = 1;
git_config(populate_advice_settings, NULL);
}
if (ARRAY_SIZE(advice_setting) <= advice_type)
BUG("OOB advice type requested???");
return !advice_setting[advice_type].disabled;
}with your "push-update-rejected has two names" twist added.
Hmm?