On Tue, Feb 11, 2020 at 9:37 AM Jeff King <peff@xxxxxxxx> 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
