Re: [PATCH v4] Refactor recv_sideband()

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

Nicolas Pitre <nico@fluxnic.net> writes:

> 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);

OK, because you no longer put PREFIX in outbuf, outbuf.len becomes
an easy way to tell if we have anything accumulated, and if there
already is something, we separate our message from it with a LF.

I like the simplicity and clarity of the logic.

> @@ -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);

Likewise for all other changes.

> -	if (outbuf.len)
> +	if (outbuf.len) {
> +		strbuf_addf(&outbuf, "\n");
>  		fwrite(outbuf.buf, 1, outbuf.len, stderr);
> +	}

... and this does make sense.

Lukas, can you see what is in 'pu' after I push out today's
integration result in several hours and tell us if you like the
result of the SQUASH??? change?

Thanks.