On Tue, 28 Jun 2016, Junio C Hamano wrote:
> 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?
I would think so. Anything that the remote sent before any error should
be printed nevertheless. The clue for the error might be in the pending
buffer.
However in this case the actual error printout and the pending buffer
will appear reversed.
So what I'd suggest is actually something like this:
if (len < 1) {
strbuf_addf(&outbuf, "\n%s: protocol error: no band
designator\n", me);
retval = SIDEBAND_PROTOCOL_ERROR;
break;
And so on for the other error cases.
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
