From: Heba Waly <heba.waly@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Heba Waly via GitGitGadget <gitgitgadget@gmail.com>,
Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH] advice: refactor advise API
Date: Tue, 11 Feb 2020 15:20:15 +1300 [thread overview]
Message-ID: <CACg5j276+BnuTe+1eMQTALPL-Ngfy=N5udyz=EQ++29=_2R9aQ@mail.gmail.com> (raw)
In-Reply-To: <20200210203725.GA620581@coredump.intra.peff.net>
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
next prev parent reply other threads:[~2020-02-11 2:20 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-10 5:04 [PATCH] advice: refactor advise API Heba Waly via GitGitGadget
2020-02-10 14:38 ` Derrick Stolee
2020-02-10 19:30 ` Junio C Hamano
2020-02-10 19:42 ` Taylor Blau
2020-02-10 22:29 ` Emily Shaffer
2020-02-11 0:08 ` Heba Waly
2020-02-12 20:57 ` Taylor Blau
2020-02-10 23:56 ` Heba Waly
2020-02-11 2:39 ` Derrick Stolee
2020-02-10 20:37 ` Jeff King
2020-02-10 22:55 ` Emily Shaffer
2020-02-11 2:35 ` Heba Waly
2020-02-11 19:49 ` Jeff King
2020-02-11 19:51 ` Jeff King
2020-02-11 2:20 ` Heba Waly [this message]
2020-02-10 22:46 ` Junio C Hamano
2020-02-11 2:01 ` Heba Waly
2020-02-11 6:08 ` Junio C Hamano
2020-02-16 21:39 ` [PATCH v2 0/2] [RFC][Outreachy] " Heba Waly via GitGitGadget
2020-02-16 21:39 ` [PATCH v2 1/2] " Heba Waly via GitGitGadget
2020-02-17 3:28 ` Junio C Hamano
2020-02-17 10:03 ` Heba Waly
2020-02-19 9:59 ` Heba Waly
2020-02-16 21:39 ` [PATCH v2 2/2] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-17 0:09 ` [PATCH v2 0/2] [RFC][Outreachy] advice: refactor advise API Junio C Hamano
2020-02-19 20:33 ` [PATCH v3 0/2] [Outreachy] advice: revamp " Heba Waly via GitGitGadget
2020-02-19 20:34 ` [PATCH v3 1/2] " Heba Waly via GitGitGadget
2020-02-20 1:37 ` Emily Shaffer
2020-02-21 0:31 ` Heba Waly
2020-02-19 20:34 ` [PATCH v3 2/2] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-20 1:42 ` Emily Shaffer
2020-02-21 0:34 ` Heba Waly
2020-02-24 15:13 ` [PATCH v4 0/3] [Outreachy] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-24 15:13 ` [PATCH v4 1/3] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-24 22:04 ` Emily Shaffer
2020-02-24 15:13 ` [PATCH v4 2/3] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-24 22:05 ` Junio C Hamano
2020-02-24 22:11 ` Eric Sunshine
2020-02-24 23:51 ` Heba Waly
2020-02-24 23:49 ` Heba Waly
2020-02-24 23:45 ` Emily Shaffer
2020-02-24 15:13 ` [PATCH v4 3/3] tag: use new advice API to check visibility Heba Waly via GitGitGadget
2020-02-24 22:07 ` Junio C Hamano
2020-02-24 23:46 ` Emily Shaffer
2020-02-25 10:55 ` [PATCH v5 0/3] [Outreachy] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-25 10:55 ` [PATCH v5 1/3] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-25 10:55 ` [PATCH v5 2/3] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-25 17:40 ` Junio C Hamano
2020-02-25 19:56 ` Emily Shaffer
2020-02-25 20:09 ` Junio C Hamano
2020-02-25 20:35 ` Junio C Hamano
2020-02-25 21:19 ` Heba Waly
2020-02-25 22:02 ` Junio C Hamano
2020-02-26 0:37 ` Heba Waly
2020-02-26 3:03 ` Junio C Hamano
2020-02-26 20:28 ` Heba Waly
2020-02-26 20:44 ` Junio C Hamano
2020-02-26 21:48 ` Jonathan Tan
2020-02-25 10:55 ` [PATCH v5 3/3] tag: use new advice API to check visibility Heba Waly via GitGitGadget
2020-02-25 17:48 ` Junio C Hamano
2020-02-27 4:35 ` [PATCH v6 0/4] [Outreachy] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-27 4:35 ` [PATCH v6 1/4] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-27 4:35 ` [PATCH v6 2/4] advice: change "setupStreamFailure" to "setUpstreamFailure" Heba Waly via GitGitGadget
2020-02-27 17:38 ` Junio C Hamano
2020-02-27 4:35 ` [PATCH v6 3/4] advice: revamp advise API Heba Waly via GitGitGadget
2020-02-27 20:49 ` Junio C Hamano
2020-02-29 0:58 ` Heba Waly
2020-02-27 4:35 ` [PATCH v6 4/4] tag: use new advice API to check visibility Heba Waly via GitGitGadget
2020-03-02 20:01 ` [PATCH v7 0/4] [Outreachy] advice: revamp advise API Heba Waly via GitGitGadget
2020-03-02 20:01 ` [PATCH v7 1/4] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-03-02 20:01 ` [PATCH v7 2/4] advice: change "setupStreamFailure" to "setUpstreamFailure" Heba Waly via GitGitGadget
2020-03-02 20:01 ` [PATCH v7 3/4] advice: revamp advise API Heba Waly via GitGitGadget
2020-03-02 21:03 ` Junio C Hamano
2020-03-03 14:15 ` Junio C Hamano
2020-03-04 3:22 ` Heba Waly
2020-03-02 20:02 ` [PATCH v7 4/4] tag: use new advice API to check visibility Heba Waly via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACg5j276+BnuTe+1eMQTALPL-Ngfy=N5udyz=EQ++29=_2R9aQ@mail.gmail.com' \
--to=heba.waly@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).