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

[Heba Waly <heba.waly@gmail.com>]

On Wed, Feb 26, 2020 at 6:40 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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.

Yes, should have been discarded.

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

No, in the above example, advise() should be called not advise_if_enabled().
As we discussed in the beginning of this thread.
https://public-inbox.org/git/xmqqa75py7u8.fsf@gitster-ct.c.googlers.com/

>
> > +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.
>

I agree.

> > +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.

I followed the original global variable name, which is
`advice_amworkdir`, we can change that.

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

I wasn't very happy about having to keep the list of config keys in
memory, but that was a good enough solution for now.
I also agree that there could be benefits for caching the values, as
you mentioned it will be less expensive than looking up from the
hashmap, but this array will grow with every new advice added to the
system. And this data is already loaded in the hashmap, so we are
duplicating it.
So are the benefits worth the duplication? I don't know.

Thanks,
Heba