Re: [PATCH] advice: refactor advise API

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

On Tue, Feb 11, 2020 at 11:55 AM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Mon, Feb 10, 2020 at 03:37:25PM -0500, Jeff King wrote:
> >
> >      instead. Using string literals is more accident-prone than
> >      variables (because the compiler doesn't notice if we misspell them)
> >      but I think is OK for cases where we just refer to the key once.
> >      For others (e.g., advice_commit_before_merge has 13 mentions),
> >      either keep the variable. Or alternatively make a wrapper like:
> >
> >        int want_advice_commit_before_merge(void)
> >        {
> >                return want_advice("commitbeforemerge");
> >        }
> >
> >      if we want to drop the existing mechanism to load all of the
> >      variables at the beginning.
>
> I tend to disagree on both counts. I'd personally rather see something
> like 'void advise_key(enum advice, char *format, ...)'.
>
> As I understand it, Heba wanted to avoid that pattern so that people
> adding a new advice didn't need to modify the advice library. However, I
> think there's value to having a centralized list of all possible advices
> (besides the documentation). The per-advice wrapper is harder to iterate
> than an enum, and will also result in a lot of samey code if we decide
> we want to use that pattern for more advices.
>

I think Peff is suggesting the wrapper for only those rare cases where
a single advice config variable is being checked several times around
the code base, but this doesn't mean we'll have many of them. In my
own opinion, I don't see the need for the list of advices, I think
it'll add unneeded complexity to the advice library and the
introduction of new advice messages. Mainly because I don't see a
scenario where we'd need to iterate through them, so I don't know ...

> (In fact, with git-bugreport I'm running into a lot of regret that hooks
> are invoked in the way Peff describes - 'find_hook("pre-commit")' -
> rather than with an enum naming the hook; it's very hard to check all
> possible hooks, and hard to examine the codebase and determine which
> hooks do and don't exist.)
>
> When Heba began to describe this project I had hoped for a final product
> like 'void show_advice(enum advice_config)' which looked up the
> appropriate string from the advice library instead of asking the caller
> to provide it, although seeing the need for varargs has demonstrated to
> me that that's not feasible :) But I think taking the advice config key
> as an argument is possibly too far the other direction. At that point,
> it starts to beg the question, "why isn't this function in config.h and
> called print_if_configured(cfgname, message, ...)?"
>
> Although, take this all with a grain of salt. I think I lean towards
> this much encapsulation after a sordid history with C++ and an
> enlightened C developer may not choose it ;)
>
>  - Emily