From: Junio C Hamano <gitster@pobox.com>
To: "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Heba Waly <heba.waly@gmail.com>
Subject: Re: [PATCH v5 2/3] advice: revamp advise API
Date: Tue, 25 Feb 2020 09:40:28 -0800 [thread overview]
Message-ID: <xmqqsgiymupf.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <b7f10d060a41c1ef3d25e4c07be3747c7902b997.1582628141.git.gitgitgadget@gmail.com> (Heba Waly via GitGitGadget's message of "Tue, 25 Feb 2020 10:55:39 +0000")
"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
>
> 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?
> +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.
> +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?
next prev parent reply other threads:[~2020-02-25 17:40 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-10 5:04 [PATCH] advice: refactor advise API Heba Waly via GitGitGadget
2020-02-10 14:38 ` Derrick Stolee
2020-02-10 19:30 ` Junio C Hamano
2020-02-10 19:42 ` Taylor Blau
2020-02-10 22:29 ` Emily Shaffer
2020-02-11 0:08 ` Heba Waly
2020-02-12 20:57 ` Taylor Blau
2020-02-10 23:56 ` Heba Waly
2020-02-11 2:39 ` Derrick Stolee
2020-02-10 20:37 ` Jeff King
2020-02-10 22:55 ` Emily Shaffer
2020-02-11 2:35 ` Heba Waly
2020-02-11 19:49 ` Jeff King
2020-02-11 19:51 ` Jeff King
2020-02-11 2:20 ` Heba Waly
2020-02-10 22:46 ` Junio C Hamano
2020-02-11 2:01 ` Heba Waly
2020-02-11 6:08 ` Junio C Hamano
2020-02-16 21:39 ` [PATCH v2 0/2] [RFC][Outreachy] " Heba Waly via GitGitGadget
2020-02-16 21:39 ` [PATCH v2 1/2] " Heba Waly via GitGitGadget
2020-02-17 3:28 ` Junio C Hamano
2020-02-17 10:03 ` Heba Waly
2020-02-19 9:59 ` Heba Waly
2020-02-16 21:39 ` [PATCH v2 2/2] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-17 0:09 ` [PATCH v2 0/2] [RFC][Outreachy] advice: refactor advise API Junio C Hamano
2020-02-19 20:33 ` [PATCH v3 0/2] [Outreachy] advice: revamp " Heba Waly via GitGitGadget
2020-02-19 20:34 ` [PATCH v3 1/2] " Heba Waly via GitGitGadget
2020-02-20 1:37 ` Emily Shaffer
2020-02-21 0:31 ` Heba Waly
2020-02-19 20:34 ` [PATCH v3 2/2] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-20 1:42 ` Emily Shaffer
2020-02-21 0:34 ` Heba Waly
2020-02-24 15:13 ` [PATCH v4 0/3] [Outreachy] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-24 15:13 ` [PATCH v4 1/3] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-24 22:04 ` Emily Shaffer
2020-02-24 15:13 ` [PATCH v4 2/3] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-24 22:05 ` Junio C Hamano
2020-02-24 22:11 ` Eric Sunshine
2020-02-24 23:51 ` Heba Waly
2020-02-24 23:49 ` Heba Waly
2020-02-24 23:45 ` Emily Shaffer
2020-02-24 15:13 ` [PATCH v4 3/3] tag: use new advice API to check visibility Heba Waly via GitGitGadget
2020-02-24 22:07 ` Junio C Hamano
2020-02-24 23:46 ` Emily Shaffer
2020-02-25 10:55 ` [PATCH v5 0/3] [Outreachy] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-25 10:55 ` [PATCH v5 1/3] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-25 10:55 ` [PATCH v5 2/3] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-25 17:40 ` Junio C Hamano [this message]
2020-02-25 19:56 ` Emily Shaffer
2020-02-25 20:09 ` Junio C Hamano
2020-02-25 20:35 ` Junio C Hamano
2020-02-25 21:19 ` Heba Waly
2020-02-25 22:02 ` Junio C Hamano
2020-02-26 0:37 ` Heba Waly
2020-02-26 3:03 ` Junio C Hamano
2020-02-26 20:28 ` Heba Waly
2020-02-26 20:44 ` Junio C Hamano
2020-02-26 21:48 ` Jonathan Tan
2020-02-25 10:55 ` [PATCH v5 3/3] tag: use new advice API to check visibility Heba Waly via GitGitGadget
2020-02-25 17:48 ` Junio C Hamano
2020-02-27 4:35 ` [PATCH v6 0/4] [Outreachy] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-27 4:35 ` [PATCH v6 1/4] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-27 4:35 ` [PATCH v6 2/4] advice: change "setupStreamFailure" to "setUpstreamFailure" Heba Waly via GitGitGadget
2020-02-27 17:38 ` Junio C Hamano
2020-02-27 4:35 ` [PATCH v6 3/4] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-27 20:49 ` Junio C Hamano
2020-02-29 0:58 ` Heba Waly
2020-02-27 4:35 ` [PATCH v6 4/4] tag: use new advice API to check visibility Heba Waly via GitGitGadget
2020-03-02 20:01 ` [PATCH v7 0/4] [Outreachy] advice: revamp advise API Heba Waly via GitGitGadget
2020-03-02 20:01 ` [PATCH v7 1/4] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-03-02 20:01 ` [PATCH v7 2/4] advice: change "setupStreamFailure" to "setUpstreamFailure" Heba Waly via GitGitGadget
2020-03-02 20:01 ` [PATCH v7 3/4] advice: revamp advise API Heba Waly via GitGitGadget
2020-03-02 21:03 ` Junio C Hamano
2020-03-03 14:15 ` Junio C Hamano
2020-03-04 3:22 ` Heba Waly
2020-03-02 20:02 ` [PATCH v7 4/4] tag: use new advice API to check visibility Heba Waly via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqsgiymupf.fsf@gitster-ct.c.googlers.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=heba.waly@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).