From: Emily Shaffer <emilyshaffer@google.com>
Date: Thu, 20 Feb 2020 01:42:05 GMT
Subject: Re: [PATCH v3 2/2] advice: extract vadvise() from advise()
Message-ID: <20200220014205.GB2447@google.com>
In-Reply-To: <a2a145c705e2751d4ced9cc71e62d5c560adb6e6.1582144442.git.gitgitgadget@gmail.com>

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