Re: [PATCH v2] Refactor recv_sideband()

Jeff King [off-list ref] writes:

On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:

quoted

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 am having trouble actually seeing how the ANSI-emulation code gets
triggered, but the comment in color.h implies that it is only printf,
fprintf, and fputs that have the desired effect. So fwrite() may not be
sufficient, and we may need fprintf("%.*s", len, buf) or something.

I have no idea how, either X-<.  But you're probably right about the
magic being limited to the printf family of functions---I do recall
hearing something like that in the past.

On Mon, 27 Jun 2016 at 19:50:13, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:

quoted

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. I do not think there is any serious implementation
of any of those functions that performs segmented write() calls (but I
might be mistaken). According to POSIX, write() can take up to SSIZE_MAX
bytes which is guaranteed to be at least 32767 but is actually much
larger on most systems (2^32 - 1 here). It is very unlikely that this
limit will ever be reached by a single line of a diagnostic error
message.

Frankly, there is a small benefit to fwrite() because we already know
the string length from the strbuf and most fputs() implementations
probably do something equivalent to

    fwrite(s, 1, strlen(s), stream);

I can switch to using fwrite() instead of fputs() in v4 if you prefer
that.

quoted

I am having trouble actually seeing how the ANSI-emulation code gets
triggered, but the comment in color.h implies that it is only printf,
fprintf, and fputs that have the desired effect. So fwrite() may not be
sufficient, and we may need fprintf("%.*s", len, buf) or something.

I have no idea how, either X-<.  But you're probably right about the
magic being limited to the printf family of functions---I do recall
hearing something like that in the past.

I do not know anything about the emulation code as well but from a
cursory read of winansi_init(), it looks like there is some magic that
hooks into the stdout and stderr streams, redirects them to a named
pipe, then replaces ANSI control codes and actually prints to the
console from console_thread(). So it should work with any of the
stream-based functions but not with write(), puts(), etc.

On Mon, 27 Jun 2016, Lukas Fleischer wrote:

On Mon, 27 Jun 2016 at 19:50:13, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:

quoted

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.


Nicolas

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

On Mon, 27 Jun 2016, Lukas Fleischer wrote:

quoted

On Mon, 27 Jun 2016 at 19:50:13, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

On Mon, Jun 27, 2016 at 08:54:22AM -0700, Junio C Hamano wrote:

quoted

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...