Re: [PATCH v2] Refactor recv_sideband()

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

Lukas Fleischer <lfleischer@lfos.de> writes:

Lukas Fleischer <lfleischer@lfos.de> writes:

> Improve the readability of recv_sideband() significantly by replacing

s/significantly //; "making it readable" is already a subjective
goodness criterion, and you do not have to make it sound even more
subjective.  Let the updated result convince the reader that it is
vastly more readable.

> Also, reorganize the overall control flow, remove some superfluous
> variables and replace a custom implementation of strpbrk() with a call
> to the standard C library function.

I find that calling the loop "a custom implementation" is a bit
unfair.  The original tried to avoid looking beyond "len", but in
the updated code because you have buf[len] = '\0' to terminate the
line, and because you pass LARGE_PACKET_MAX to packet_read() while
your buf[] allocates one more byte, you can use strpbrk() here
safely. Which would mean "a custom implementation" was done for a
reason.

But that is a minor point.

I'll omit the preimage lines from the following.

>  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;
>  
> +	strbuf_addf(&outbuf, "%s", PREFIX);

I highly suspect that you are better off without this line being
here.

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

Two "return"s we see above will leak outbuf.buf that holds PREFIX.

>  		case 2:
> +			b = buf + 1;
>  
> +			/*
> +			 * Append a suffix to each nonempty line to clear the
> +			 * end of the screen line.
> +			 */
> +			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);
>  				}
> +				xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
> +				strbuf_reset(&outbuf);
> +				strbuf_addf(&outbuf, "%s", PREFIX);

Instead of doing "we assume outbuf already has PREFIX when we add
contents from buf[]", the code structure would be better if you:

 * make outbuf.buf contain PREFIX at the beginning of this innermost
   loop; lose the reset/addf from here.

 * move strbuf_reset(&outbuf) at the end of the next if (*b) block
   to just before "continue;"

perhaps?

> +				b = brk + 1;
> +			}
>  
> +			if (*b) {
> +				xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
> +				/* Incomplete line, skip the next prefix. */
> +				strbuf_reset(&outbuf);
> +			}
>  			continue;
>  		case 1:
> +			write_or_die(out, buf + 1, len);
>  			continue;
>  		default:
>  			fprintf(stderr, "%s: protocol error: bad band #%d\n",