Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre <nico@fluxnic.net>]

On Tue, 28 Jun 2016, Junio C Hamano wrote:

> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> >> The basic structure of the code (without the "SQUASH" we discussed)
> >> looks like this:
> >> 
> >> 	strbuf_addf(&outbuf, "%s", PREFIX);
> >> 	while (retval == 0) {
> >> 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> >> 		...
> >> 		band = buf[0] & 0xff;
> >> 		switch (band) {
> >> 		case 3:
> >> 			... /* emergency exit */
> >> 		case 2:
> >> 			while ((brk = strpbrk(b, "\n\r"))) {
> >> 				int linelen = brk - b;
> >> 
> >> 				if (linelen > 0) {
> >> 					strbuf_addf(&outbuf, "%.*s%s%c",
> >> 						    linelen, b, suffix, *brk);
> >> 				} else {
> >> 					strbuf_addf(&outbuf, "%c", *brk);
> >> 				}
> >> 				fprintf(stderr, "%.*s", (int)outbuf.len,
> >> 					outbuf.buf);
> >> 				strbuf_reset(&outbuf);
> >> 				strbuf_addf(&outbuf, "%s", PREFIX);
> >> 				b = brk + 1;
> >> 			}
> >> 			if (*b)
> >> 				strbuf_addf(&outbuf, "%s", b);
> >> 			break;
> >> 		...
> >> 		}
> >> 	}
> >> 
> >> 	if (outbuf.len > 0)
> >> 		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> >>  ...
> > That won't work. If at this point there is the beginning of a partial 
> > line queued in the buffer from the previous round waiting to see its 
> > line break, you just deleted the beginning of that line.
> 
> Ahh, OK, a single logical line split into two and sent in two
> packets--the first one would not result in any output (just does "if
> (*b) strbuf_addf(...)" to buffer it) and then the second one finally
> finds a LF.  Yeah, that won't work if we cleared.
> 
> But then my observation still holds, no?

I think it does... but I'm no longer sure of what you meant.

To make it clearer, here's a patch on top of pu that fixes all the 
issues I think are remaining. All tests pass now.

diff --git a/sideband.c b/sideband.c
index 36a032f..0e6c6df 100644
--- a/sideband.c
+++ b/sideband.c
@@ -23,10 +23,8 @@ int recv_sideband(const char *me, int in_stream, int out)
 	const char *term, *suffix;
 	char buf[LARGE_PACKET_MAX + 1];
 	struct strbuf outbuf = STRBUF_INIT;
-	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;
@@ -34,14 +32,15 @@ int recv_sideband(const char *me, int in_stream, int out)
 		suffix = DUMB_SUFFIX;
 
 	while (!retval) {
+		const char *b, *brk;
 		int band, len;
 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
 			strbuf_addf(&outbuf,
-				    "\n%s: protocol error: no band designator\n",
-				    me);
+				    "%s%s: protocol error: no band designator",
+				    outbuf.len ? "\n" : "", me);
 			retval = SIDEBAND_PROTOCOL_ERROR;
 			break;
 		}
@@ -50,7 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out)
 		len--;
 		switch (band) {
 		case 3:
-			strbuf_addf(&outbuf, "\n%s%s\n", PREFIX, buf + 1);
+			strbuf_addf(&outbuf, "%s%s%s", outbuf.len ? "\n" : "",
+				    PREFIX, buf + 1);
 			retval = SIDEBAND_REMOTE_ERROR;
 			break;
 		case 2:
@@ -70,6 +70,8 @@ int recv_sideband(const char *me, int in_stream, int out)
 			while ((brk = strpbrk(b, "\n\r"))) {
 				int linelen = brk - b;
 
+				if (!outbuf.len)
+					strbuf_addf(&outbuf, "%s", PREFIX);
 				if (linelen > 0) {
 					strbuf_addf(&outbuf, "%.*s%s%c",
 						    linelen, b, suffix, *brk);
@@ -78,27 +80,29 @@ int recv_sideband(const char *me, int in_stream, int out)
 				}
 				fwrite(outbuf.buf, 1, outbuf.len, stderr);
 				strbuf_reset(&outbuf);
-				strbuf_addf(&outbuf, "%s", PREFIX);
 
 				b = brk + 1;
 			}
 
 			if (*b)
-				strbuf_addf(&outbuf, "%s", b);
+				strbuf_addf(&outbuf, "%s%s",
+					    outbuf.len ? "" : PREFIX, b);
 			break;
 		case 1:
 			write_or_die(out, buf + 1, len);
 			break;
 		default:
-			strbuf_addf(&outbuf, "\n%s: protocol error: bad band #%d\n",
-				me, band);
+			strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
+				    outbuf.len ? "\n" : "", me, band);
 			retval = SIDEBAND_PROTOCOL_ERROR;
 			break;
 		}
 	}
 
-	if (outbuf.len)
+	if (outbuf.len) {
+		strbuf_addf(&outbuf, "\n");
 		fwrite(outbuf.buf, 1, outbuf.len, stderr);
+	}
 	strbuf_release(&outbuf);
 	return retval;
 }