From: Heba Waly <heba.waly@gmail.com>
Date: Fri, 21 Feb 2020 00:34:50 GMT
Subject: Re: [PATCH v3 2/2] advice: extract vadvise() from advise()
Message-ID: <CACg5j27MDAq6dHD02SUZ-vyiYePUf=B1vJrrjxoj0RbLS=qEng@mail.gmail.com>
In-Reply-To: <20200220014205.GB2447@google.com>

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