Re: [PATCH] advice: refactor advise API

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

On Tue, Feb 11, 2020 at 9:37 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Feb 10, 2020 at 05:04:09AM +0000, Heba Waly via GitGitGadget wrote:
>
> >     The advice API is currently a little bit confusing to call. quoting from
> >     [1]:
> >
> >     When introducing a new advice message, you would
> >
> >      * come up with advice.frotz configuration variable
> >
> >      * define and declare advice_frotz global variable that defaults to
> >        true
> >
> >      * sprinkle calls like this:
> >
> >       if (advice_frotz)
> >         advise(_("helpful message about frotz"));
> >
> >     A new approach was suggested in [1] which this patch is based upon.
>
> I agree that the current procedure is a bit painful, and I think this is
> a step in the right direction. But...
>
> >     After this patch the plan is to migrate the rest of the advise calls to
> >     advise_ng and then finally remove advise() and rename advise_ng() to
> >     advise()
>
> ...this step may not be possible, for a few reasons:
>
>   1. Some of the sites do more than just advise(). E.g., branch.c checks
>      the flag and calls both error() and advise().
>
>   2. Some callers may have to do work to generate the arguments. If I
>      have:
>
>        advise("advice.foo", "some data: %s", generate_data());
>
>      then we'll call generate_data() even if we'll throw away the result
>      in the end.
>
> Similarly, some users of advice_* variables do not call advise() at all
> (some call die(), some like builtin/rm.c stuff the result in a strbuf,
> and I don't even know what's going on with wt_status.hints. :)
>
> So I think you may need to phase it in a bit more, like:
>
>   a. introduce want_advice() which decides whether or not to show the
>      advice based on a config key. I'd also suggest making the "advice."
>      part of the key implicit, just to make life easier for the callers.
>

yes, I agree.

>   b. introduce advise_key() which uses want_advice() and advise() under
>      the hood to do what your advise_ng() is doing here.
>
>   c. convert simple patterns of:
>
>        if (advice_foo)
>           advise("bar");
>
>      into:
>
>        advise_key("foo", "bar");
>
>      and drop advice_foo where possible.
>
>   d. handle more complex cases one-by-one. For example, with something
>      like:
>
>        if (advice_foo)
>          die("bar");
>
>      we probably want:
>
>        if (want_advice("foo"))
>          die("bar");
>
>      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.
>

All make sense to me, thanks for the feedback.

> -Peff

Heba