Re: [PATCH v2] Refactor recv_sideband()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 14, 2016, 21:25 UTC
- Message-ID
- <xmqqbn338nu1.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160614210038.31465-1-lfleischer@lfos.de>
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.
Show 8 quoted lines
> 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.
Show 17 quoted lines
> ...
> 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.
Show 19 quoted lines
> 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?
Show 14 quoted lines
> + 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",