Re: [PATCH v4] Refactor recv_sideband()

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 28, 2016, 17:24 UTC
Message-ID: <xmqq60st9qg5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqa8i59rph.fsf@gitster.mtv.corp.google.com>

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

Show 28 quoted lines

> With input from Dscho that recent Git-for-Windows does the right
> thing without limiting us to use only a subset of stdio, perhaps we
> would want to squash something like this in.
>
> diff --git a/sideband.c b/sideband.c
> index 226a8c2..72e2c5c 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -58,13 +58,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 suppor 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;

That may be good, but the remainder is crap. Sorry about the typo; s/output/outbuf/g is needed at least.

Show 19 quoted lines

> @@ -75,8 +74,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(output.buf, 1, output.len, stderr);
>  				strbuf_reset(&outbuf);
>  				strbuf_addf(&outbuf, "%s", PREFIX);
>  
> @@ -98,7 +96,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  	}
>  
>  	if (outbuf.len > 0)
> -		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> +		fwrite(output.buf, 1, output.len, stderr);
>  	strbuf_release(&outbuf);
>  	return retval;
>  }

But we would be better off using the real write(2), now Windows port does not limit us to stdio.

And then that made me stare at the patch even more. We still write some error messages to stderr in the updated code (without my crap SQUASH) inside "while (!retval)" loop:

	while (retval == 0) {
		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);
			retval = SIDEBAND_PROTOCOL_ERROR;
			break;
		}
		band = buf[0] & 0xff;
		buf[len] = '\0';
		len--;
		switch (band) {
		case 3:
			fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
			retval = SIDEBAND_REMOTE_ERROR;
			break;
		case 2:
			...
			while ((brk = strpbrk(b, "\n\r"))) {
				...
				write(2, outbuf.buf, outbuf.len);
				...
			}

			if (*b)
				strbuf_addf(&outbuf, "%s", b);
			break;
		case 1:
			write_or_die(out, buf + 1, len);
			break;
		default:
			fprintf(stderr, "%s: protocol error: bad band #%d\n",
				me, band);
			retval = SIDEBAND_PROTOCOL_ERROR;
			break;
		}
	}

	if (outbuf.len > 0)
		write(2, outbuf.buf, outbuf.len);

In general, mixing stdio and raw file descriptor access is not such a good idea, but these remaining calls to fprintf(stderr, ...) above are for error-exit codepath, so from that point of view, the above illustration may be acceptable, but there is still one niggle.

When we exit the loop because we set retval to a non-zero value, should we still drain the outbuf?

IOW, shouldn't the if statement after "while (!retval)" loop be more like this?

	if (!retval && outbuf.len)
        	write(2, outbuf.buf, outbuf,len);

Thanks.