Lukas Fleischer <lfleischer@xxxxxxx> writes:
Lukas Fleischer <lfleischer@xxxxxxx> 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",
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html