Re: [PATCH v4] Refactor recv_sideband()

From: Nicolas Pitre <nico@fluxnic.net>
Date: Jun 28, 2016, 20:36 UTC
Message-ID: <alpine.LFD.2.20.1606281629280.24439@knanqh.ubzr>
In-Reply-To: <xmqq60st853d.fsf@gitster.mtv.corp.google.com>

On Tue, 28 Jun 2016, Junio C Hamano wrote:

Show 36 quoted lines

> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> > Without this, the error and remaining buffer would be reversed as 
> > mentioned previously.  With this, the order is restored, but a newline 
> > is added to unterminated lines whereas the error was simply appended to 
> > the output before Lukas' patch.
> >
> > In any case the new behavior is probably better and I'd simply adjust 
> > the test expectations.
> 
> There is something else going on.  I cannot quite explain why I am
> getting this failure from t5401-update-hooks.sh, for example:
> 
>     --- expect      2016-06-28 19:46:24.564937075 +0000
>     +++ actual      2016-06-28 19:46:24.564937075 +0000
>     @@ -9,3 +9,4 @@
>      remote: STDERR post-receive
>      remote: STDOUT post-update
>      remote: STDERR post-update
>     +remote: To ./victim.git
>     not ok 12 - send-pack stderr contains hook messages
> 
> ... goes and looks what v2.9.0 produces, which ends like this:
> 
>     ...
>     remote: STDERR post-receive        
>     remote: STDOUT post-update        
>     remote: STDERR post-update        
>     To ./victim.git
>        e4822ab..2b65bd1  master -> master
>      ! [remote rejected] tofail -> tofail (hook declined)
> 
> The test checks if lines prefixed with "remote: " match the expected
> output, and the difference is an indication that the new code is
> showing an extra incomplete-line "remote: " before other parts of
> the code says "To ./victim.git" to report where the push is going.

Ah...  I think I know what's going on.

The leftover data in the strbuf is normally (when there is no errors) an unterminated line. So instead of doing:

-                       fprintf(stderr, "%s: protocol error: no band designator\n", me);
+                       strbuf_addf(&outbuf,
+                                   "\n%s: protocol error: no band designator\n",
+                                   me);

you could omit the final \n in the format string and:

-       if (outbuf.len > 0)
-               fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
+       if (outbuf.len)
+               fwrite(outbuf.buf, 1, outbuf.len, stderr);
        strbuf_release(&outbuf);

and here a \n could be added before writing out the buffer.

Nicolas