git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Heba Waly via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Heba Waly <heba.waly@gmail.com>
Subject: Re: [PATCH] advice: refactor advise API
Date: Mon, 10 Feb 2020 15:37:25 -0500	[thread overview]
Message-ID: <20200210203725.GA620581@coredump.intra.peff.net> (raw)
In-Reply-To: <pull.548.git.1581311049547.gitgitgadget@gmail.com>

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.

  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.

-Peff

  parent reply	other threads:[~2020-02-10 20:37 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 [this message]
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
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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200210203725.GA620581@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=heba.waly@gmail.com \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).