Re: [PATCH v2] Refactor recv_sideband()

From: Lukas Fleischer <lfleischer@lfos.de>
Date: Jun 28, 2016, 04:01 UTC
Message-ID: <146708647010.14433.5732305874810138274@typhoon>
In-Reply-To: <alpine.LFD.2.20.1606271640230.2550@knanqh.ubzr>

On Mon, 27 Jun 2016 at 22:47:59, Nicolas Pitre wrote:

Show 28 quoted lines

> On Mon, 27 Jun 2016, Lukas Fleischer wrote:
> 
> > On Mon, 27 Jun 2016 at 19:50:13, Junio C Hamano wrote:
> > > Jeff King <peff@peff.net> writes:
> > > 
> > > > On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:
> > > >
> > > >> It's just you used xwrite() there that introduced a different issue.
> > > >> Wouldn't replacing it with fwrite(stderr) without changing anything
> > > >> else solve that?
> > 
> > I do not see how using fwrite() buys us anything. Neither fwrite() nor
> > fputs() nor fprintf() guarantee to call write() only once. Each of these
> > three functions is buffered when printing to stdout and unbuffered when
> > printing to stderr.
> 
> You are right.  However, in practice:
> 
> - fprintf(stderr, "%s", buffer) is likely to call write() only once 
>   given there is only one string specifier, and
> 
> - On Windows the ANSI escape sequences are interpreted by fprintf() and 
>   not by write() nor by the actual display console code. Insane but such 
>   is life sometimes.
> 
> So the point is simply to replace your call to write() by a call to 
> fprintf(..., "%*s", ...) in your patch which should provide the same 
> end result as before.

Well, this is essentially what I tried to make clear in my previous email. In practice, each of the following lines should work:

    fwrite(outbuf.buf, 1, outbuf.len, stderr);
    fputs(outbuf.buf, stderr);
    fprintf("%s", outbuf.buf, stderr);
    fprintf("%.*s", outbuf.len, outbuf.buf, stderr);

The first version is probably to most "efficient" one and I personally find the fputs() line to be the one that is easiest to read. However, I think it does not make sense to start another bikeshedding discussion at this point. I will make a defensive choice and use fprintf() with "%.*s" since that is what we used before, so it is tested well enough.

Given the amount of discussion required to get this right, I also strongly believe this code deserves a comment with a short explanation on why things are done this way...