| Message ID | a2a145c705e2751d4ced9cc71e62d5c560adb6e6.1582144442.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | advice: refactor advise API | expand |
On Wed, Feb 19, 2020 at 08:34:01PM +0000, Heba Waly via GitGitGadget wrote: > From: Heba Waly <heba.waly@gmail.com> > > extract a version of advise() that uses an explict 'va_list' parameter. > Call it from advise() and advise_if_enabled() for a functionally > equivalent version. Hm, I'd put this patch before the advise_if_enabled() one, so that each commit makes sense by itself (rather than adding a bunch of code last patch only to remove it in this patch). > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > Signed-off-by: Heba Waly <heba.waly@gmail.com> > --- > - for (cp = buf.buf; *cp; cp = np) { > - np = strchrnul(cp, '\n'); > - fprintf(stderr, _("%shint: %.*s%s\n"), > - advise_get_color(ADVICE_COLOR_HINT), > - (int)(np - cp), cp, > - advise_get_color(ADVICE_COLOR_RESET)); > - if (*np) > - np++; > - } I see - this hunk that I commented on in the other review is actually duplicated from advise(). Hm, I still think it'd be useful to put this functionality into strbuf, but I guess since it's not new code you're adding there's not a lot of need to sweat about it. - Emily
On Thu, Feb 20, 2020 at 2:42 PM Emily Shaffer <emilyshaffer@google.com> wrote: > > On Wed, Feb 19, 2020 at 08:34:01PM +0000, Heba Waly via GitGitGadget wrote: > > From: Heba Waly <heba.waly@gmail.com> > > > > extract a version of advise() that uses an explict 'va_list' parameter. > > Call it from advise() and advise_if_enabled() for a functionally > > equivalent version. > > Hm, I'd put this patch before the advise_if_enabled() one, so that each > commit makes sense by itself (rather than adding a bunch of code last > patch only to remove it in this patch). > You're right, that was me avoiding the commits re-order conflicts but I'll give it a second try. > > > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > > Signed-off-by: Heba Waly <heba.waly@gmail.com> > > --- > > - for (cp = buf.buf; *cp; cp = np) { > > - np = strchrnul(cp, '\n'); > > - fprintf(stderr, _("%shint: %.*s%s\n"), > > - advise_get_color(ADVICE_COLOR_HINT), > > - (int)(np - cp), cp, > > - advise_get_color(ADVICE_COLOR_RESET)); > > - if (*np) > > - np++; > > - } > > I see - this hunk that I commented on in the other review is actually > duplicated from advise(). Hm, I still think it'd be useful to put this > functionality into strbuf, but I guess since it's not new code you're > adding there's not a lot of need to sweat about it. > Agree. > - Emily Thank you Emily Heba
diff --git a/advice.c b/advice.c index 345319005ac..0c144c69487 100644 --- a/advice.c +++ b/advice.c @@ -128,15 +128,20 @@ static const char *advice_config_keys[] = { [SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = "submoduleAlternateErrorStrategyDie" }; -void advise(const char *advice, ...) +static const char turn_off_instructions[] = +N_("\n" + "Disable this message with \"git config %s false\""); + +static void vadvise(const char *advice, va_list params, + int display_instructions, char *key) { struct strbuf buf = STRBUF_INIT; - va_list params; const char *cp, *np; - va_start(params, advice); strbuf_vaddf(&buf, advice, params); - va_end(params); + + if(display_instructions) + strbuf_addf(&buf, turn_off_instructions, key); for (cp = buf.buf; *cp; cp = np) { np = strchrnul(cp, '\n'); @@ -169,37 +174,25 @@ int advice_enabled(enum advice_type type) } } -static const char turn_off_instructions[] = -N_("\n" - "Disable this message with \"git config %s false\""); +void advise(const char *advice, ...) +{ + va_list params; + va_start(params, advice); + vadvise(advice, params, 0, ""); + va_end(params); +} void advise_if_enabled(enum advice_type type, const char *advice, ...) { - struct strbuf buf = STRBUF_INIT; - char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); va_list params; - const char *cp, *np; - + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); + if(!advice_enabled(type)) return; va_start(params, advice); - strbuf_vaddf(&buf, advice, params); + vadvise(advice, params, 1, key); va_end(params); - - strbuf_addf(&buf, turn_off_instructions, key); - - for (cp = buf.buf; *cp; cp = np) { - np = strchrnul(cp, '\n'); - fprintf(stderr, _("%shint: %.*s%s\n"), - advise_get_color(ADVICE_COLOR_HINT), - (int)(np - cp), cp, - advise_get_color(ADVICE_COLOR_RESET)); - if (*np) - np++; - } - strbuf_release(&buf); - } int git_default_advice_config(const char *var, const char *value)