Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

From: Jeff King <peff@peff.net>
Date: Apr 30, 2024, 10:52 UTC
Message-ID: <20240430105256.GH1279403@coredump.intra.peff.net>
In-Reply-To: <xmqqfrv4ug3l.fsf@gitster.g>

On Mon, Apr 29, 2024 at 12:24:46PM -0700, Junio C Hamano wrote:

Show 19 quoted lines

> Rubén Justo <rjusto@gmail.com> writes:
> 
> > diff --git a/add-patch.c b/add-patch.c
> > index 0997d4af73..fc0eed4fd4 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)
> >  	va_list args;
> >  
> >  	va_start(args, fmt);
> > -	fputs(s->s.error_color, stderr);
> > -	vfprintf(stderr, fmt, args);
> > -	fputs(s->s.reset_color, stderr);
> > -	fputc('\n', stderr);
> > +	fputs(s->s.error_color, stdout);
> > +	vprintf(fmt, args);
> > +	puts(s->s.reset_color);
> 
> I like the attention of the detail here ;-).

Indeed. I had to read this several times to wonder why it was not a mistake to leave the first fputs() but use vprintf() and puts() for the other two (for those just reading, the answer is that puts() prints an extra newline, so we can only use it at the end). Which IMHO really just points to how inconsistent the stdio interfaces are, but there is nothing we can do about that. ;)

I am tempted to suggest that it borders on too-clever, and writing out "stdout" everywhere with vfprintf() and fputs() would be more obvious. But in a little self-contained function like this I don't know that it matters much.

-Peff