Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> It's just that if you take the latter, then the conditional after
> the loop exits (i.e. the last transmission was an incomplete line)
> cannot be "is outbuf empty?", as your base state is "has PREFIX and
> can never be empty".  I was working back from that if statement.

Let's try this again.  How does this look?

In this version:

 - "outbuf" is where we keep the (possibly partial) data collected
   to be eventually shown;

 - output of pending (possibly partial) data is handled by a helper
   function drain().  It is responsible for prepending of the
   PREFIX, which is treated purely as a cosmetic thing.  It also is
   responsible for completing an incomplete line at the end of the
   transmission (e.g. flushing of the buffered input upon reception of
   the emergency exit packet).

 - locally generated errors go directly to fprintf(stderr),
   bypassing outbuf (hence drain()).

 sideband.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/sideband.c b/sideband.c
index 226a8c2..6873137 100644
--- a/sideband.c
+++ b/sideband.c
@@ -18,6 +18,16 @@
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
+static void drain(struct strbuf *outbuf)
+{
+	if (!outbuf->len)
+		return;
+	strbuf_splice(outbuf, 0, 0, PREFIX, strlen(PREFIX));
+	strbuf_complete_line(outbuf);
+	fwrite(outbuf->buf, 1, outbuf->len, stderr);
+	strbuf_reset(outbuf);
+}
+
 int recv_sideband(const char *me, int in_stream, int out)
 {
 	const char *term, *suffix;
@@ -26,20 +36,21 @@ int recv_sideband(const char *me, int in_stream, int out)
 	const char *b, *brk;
 	int retval = 0;
 
-	strbuf_addf(&outbuf, "%s", PREFIX);
 	term = getenv("TERM");
 	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
 	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);
+			drain(&outbuf);
+			fprintf(stderr, "%s: protocol error: no band designator\n",
+				me);
 			retval = SIDEBAND_PROTOCOL_ERROR;
 			break;
 		}
@@ -48,7 +59,8 @@ int recv_sideband(const char *me, int in_stream, int out)
 		len--;
 		switch (band) {
 		case 3:
-			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
+			drain(&outbuf);
+			strbuf_addf(&outbuf, "%s\n", buf + 1);
 			retval = SIDEBAND_REMOTE_ERROR;
 			break;
 		case 2:
@@ -58,13 +70,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,11 +86,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);
-				strbuf_reset(&outbuf);
-				strbuf_addf(&outbuf, "%s", PREFIX);
-
+				drain(&outbuf);
 				b = brk + 1;
 			}
 
@@ -90,6 +97,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 			write_or_die(out, buf + 1, len);
 			break;
 		default:
+			drain(&outbuf);
 			fprintf(stderr, "%s: protocol error: bad band #%d\n",
 				me, band);
 			retval = SIDEBAND_PROTOCOL_ERROR;
@@ -97,8 +105,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 		}
 	}
 
-	if (outbuf.len > 0)
-		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
+	drain(&outbuf);
 	strbuf_release(&outbuf);
 	return retval;
 }