Re: [PATCH v3] Refactor recv_sideband()

[Nicolas Pitre <nico@fluxnic.net>]

On Wed, 22 Jun 2016, Lukas Fleischer wrote:

> Before this patch, we used character buffer manipulations to split
> messages from the sideband at line breaks and insert "remote: " at the
> beginning of each line, using the packet size to determine the end of a
> message. However, since it is safe to assume that diagnostic messages
> from the sideband never contain NUL characters, we can also
> NUL-terminate the buffer, use strpbrk() for splitting lines and use
> format strings to insert the prefix.
> 
> A static strbuf is used for constructing the output which is then
> printed using a single write() call, such that the atomicity of the
> output is preserved. See 9ac13ec (atomic write for sideband remote
> messages, 2006-10-11) for details.
> 
> Helped-by: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>

The patch is buggy.

Once patched, the code looks like this:

        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);
                        b = brk + 1;
                }
                if (*b) {
                        xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
                        /* Incomplete line, skip the next prefix. */
                        strbuf_reset(&outbuf);
                }
                continue;

You are probably missing a strbuf_addf() before the last xwrite().


Nicolas