Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 8, 2020, 19:05 UTC
- Message-ID
- <xmqqwoa122h1.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <CACg5j260h88bd=W_4EzAn7B0TiU02Y8BzKDQ7w3UJiHkhL60NQ@mail.gmail.com>
Heba Waly <heba.waly@gmail.com> writes:
Show 13 quoted lines
> On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote: >> >> advice seems simple on the surface, but every new piece of advice >> means having to add yet another configuration variable, writing more >> code, more tests, and more documentation > > This raises a question though, do we really need a new configuration > for every new advice? > So a user who's not interested in receiving advice will have to > disable every single advice config? It doesn't seem scalable to me. > I imagine a user will either want to enable or disable the advice > feature all together. Why don't we have only one `enable_advice` > configuration that controls all the advice messages?
The advice mechanism was a way to help new people learn the system by giving a bit of extra help messages that would become annoying once they learned that part of the system, so by default they are on, and can be turned off once they learn enough about the specific situation that gives one kind of advise. Hence, "[advice] !all" to decline any and all advice message, including anything that would be introduced in the future, is somewhat a foreign concept in that picture.
Having said that, I am not opposed to add support for such an overall "turn all off" (or on for that matter). Totally untested, but something along this line, perhaps? The idea is that
- the config keys may come in any order;
- once advice.all is set to either true or false, we set all the advice.* variables to the given value,
- for any other advice.* config, we interpret it only if we haven't seen advice.all
advice.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/advice.c b/advice.c index 098ac0abea..b9a8fe1360 100644 --- a/advice.c +++ b/advice.c @@ -3,6 +3,8 @@ #include "color.h" #include "help.h" +static int advice_all_seen = -1; /* not seen yet */ + int advice_fetch_show_forced_updates = 1; int advice_push_update_rejected = 1; int advice_push_non_ff_current = 1; @@ -142,13 +144,22 @@ int git_default_advice_config(const char *var, const char *value) if (!skip_prefix(var, "advice.", &k)) return 0; - for (i = 0; i < ARRAY_SIZE(advice_config); i++) { - if (strcasecmp(k, advice_config[i].name)) - continue; - *advice_config[i].preference = git_config_bool(var, value); + if (!strcmp(var, "advise.all")) { + advice_all_seen = git_config_bool(var, value); + for (i = 0; i < ARRAY_SIZE(advice_config); i++) + *advice_config[i].preference = advice_all_seen; return 0; } + if (advice_all_seen < 0) { + for (i = 0; i < ARRAY_SIZE(advice_config); i++) { + if (strcasecmp(k, advice_config[i].name)) + continue; + *advice_config[i].preference = git_config_bool(var, value); + return 0; + } + } + return 0; }