Re: [PATCH v5 2/3] advice: revamp advise API

>
> "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?