Re: [PATCH v4] Refactor recv_sideband()
From: Nicolas Pitre <nico@fluxnic.net>
Date: 2016-06-28 18:29:05
Possibly related (same subject, not in this thread)
- 2016-07-07 · Re: [PATCH v4] Refactor recv_sideband() · Nicolas Pitre <nico@fluxnic.net>
- 2016-07-06 · Re: [PATCH v4] Refactor recv_sideband() · Junio C Hamano <hidden>
- 2016-07-05 · Re: [PATCH v4] Refactor recv_sideband() · Nicolas Pitre <nico@fluxnic.net>
- 2016-07-01 · Re: [PATCH v4] Refactor recv_sideband() · Junio C Hamano <hidden>
- 2016-06-30 · Re: [PATCH v4] Refactor recv_sideband() · Lukas Fleischer <hidden>
On Tue, 28 Jun 2016, Junio C Hamano wrote:
Nicolas Pitre [off-list ref] writes:quoted
quoted
When we exit the loop because we set retval to a non-zero value, should we still drain the outbuf?I would think so. Anything that the remote sent before any error should be printed nevertheless. The clue for the error might be in the pending buffer. However in this case the actual error printout and the pending buffer will appear reversed. So what I'd suggest is actually something like this: if (len < 1) { strbuf_addf(&outbuf, "\n%s: protocol error: no band designator\n", me); retval = SIDEBAND_PROTOCOL_ERROR; break; And so on for the other error cases.Makes sense. Here is what I have as a "SQUASH" on top of Lukas's change to be queued on 'pu'.
Looks good to me. Acked-by: Nicolas Pitre <redacted>
It appears that a few tests get their expectations broken, with or without this "SQUASH" change, though X-<.
Without this, the error and remaining buffer would be reversed as mentioned previously. With this, the order is restored, but a newline is added to unterminated lines whereas the error was simply appended to the output before Lukas' patch. In any case the new behavior is probably better and I'd simply adjust the test expectations.
quoted hunk ↗ jump to hunk
sideband.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)diff --git a/sideband.c b/sideband.c index 226a8c2..082dfc6 100644 --- a/sideband.c +++ b/sideband.c@@ -33,13 +33,15 @@ int recv_sideband(const char *me, int in_stream, int out) else suffix = DUMB_SUFFIX; - while (retval == 0) { + while (!retval) { int band, len; len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0); if (len == 0) break; if (len < 1) { - fprintf(stderr, "%s: protocol error: no band designator\n", me); + strbuf_addf(&outbuf, + "\n%s: protocol error: no band designator\n", + me); retval = SIDEBAND_PROTOCOL_ERROR; break; }@@ -48,7 +50,7 @@ int recv_sideband(const char *me, int in_stream, int out) len--; switch (band) { case 3: - fprintf(stderr, "%s%s\n", PREFIX, buf + 1); + strbuf_addf(&outbuf, "\n%s%s\n", PREFIX, buf + 1); retval = SIDEBAND_REMOTE_ERROR; break; case 2:@@ -58,13 +60,12 @@ int recv_sideband(const char *me, int in_stream, int out) * Append a suffix to each nonempty line to clear the * end of the screen line. * - * The output is accumulated in a buffer and each line - * is printed to stderr using fprintf() with a single - * conversion specifier. This is a "best effort" - * approach to supporting both inter-process atomicity - * (single conversion specifiers are likely to end up - * in single atomic write() system calls) and the ANSI - * control code emulation under Windows. + * The output is accumulated in a buffer and + * each line is printed to stderr using + * fwrite(3). This is a "best effort" + * approach to support inter-process atomicity + * (single fwrite(3) call is likely to end up + * in single atomic write() system calls). */ while ((brk = strpbrk(b, "\n\r"))) { int linelen = brk - b;@@ -75,8 +76,7 @@ int recv_sideband(const char *me, int in_stream, int out) } else { strbuf_addf(&outbuf, "%c", *brk); } - fprintf(stderr, "%.*s", (int)outbuf.len, - outbuf.buf); + fwrite(outbuf.buf, 1, outbuf.len, stderr); strbuf_reset(&outbuf); strbuf_addf(&outbuf, "%s", PREFIX);@@ -90,15 +90,15 @@ int recv_sideband(const char *me, int in_stream, int out) write_or_die(out, buf + 1, len); break; default: - fprintf(stderr, "%s: protocol error: bad band #%d\n", + strbuf_addf(&outbuf, "\n%s: protocol error: bad band #%d\n", me, band); retval = SIDEBAND_PROTOCOL_ERROR; break; } } - if (outbuf.len > 0) - fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf); + if (outbuf.len) + fwrite(outbuf.buf, 1, outbuf.len, stderr); strbuf_release(&outbuf); return retval; } --To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html