Re: [PATCH v4] Refactor recv_sideband()

[Nicolas Pitre <nico@fluxnic.net>]

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

> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> >> 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.
> 
> Unfortunately, that is not it.
> 
> The basic structure of the code (without the "SQUASH" we discussed)
> looks like this:
> 
> 	strbuf_addf(&outbuf, "%s", PREFIX);
> 	while (retval == 0) {
> 		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> 		...
> 		band = buf[0] & 0xff;
> 		switch (band) {
> 		case 3:
> 			... /* emergency exit */
> 		case 2:
> 			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);
> 				}
> 				fprintf(stderr, "%.*s", (int)outbuf.len,
> 					outbuf.buf);
> 				strbuf_reset(&outbuf);
> 				strbuf_addf(&outbuf, "%s", PREFIX);
> 				b = brk + 1;
> 			}
> 			if (*b)
> 				strbuf_addf(&outbuf, "%s", b);
> 			break;
> 		...
> 		}
> 	}
> 
> 	if (outbuf.len > 0)
> 		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);
> 
> Imagine we are reading from band #2 and we find a complete line.  We
> concatenate the payload up to the LF at the end of the line to the
> PREFIX we prepared outside the loop and emit it, and then we ASSUME
> that we further have something after strpbrk() and add PREFIX to the
> buffer, before going to the next line in the payload.
> 
> But there may not be anything after the LF.  outbuf.len is still
> counting the PREFIX and we end up showing it, without termination.

You're right.  Although my previous observations still apply.

> This takes us back to what I said in my review of an earlier round,
> in $gmane/297332, where I said:
> 
>     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?
> 
> I think the strbuf_addf(PREFIX) above the loop should be removed,
> and instead the code should use the PREFIX only when it decides that
> there is something worth emitting, i.e.
> 
> 	while (!retval) {
>         	len = packet_read();
>                 ...
>                 band = buf[0] & 0xff;
>                 switch (band) {
>                 case 3:
>                 	... /* emergency exit */
> 		case 2:
>                 	while ((brk = ...)) {
>                         	/* we have something to say */
> 				strbuf_reset(&outbuf);
>                                 strbuf_addstr(&outbuf, PREFIX);

That won't work. If at this point there is the beginning of a partial 
line queued in the buffer from the previous round waiting to see its 
line break, you just deleted the beginning of that line.

Furthermore, that partial line won't get a prefix if it doesn't have at 
least one line break in the packet data.

Rather the prefix should be added whenever the buffer is empty before 
every addition.

>                                 if (linelen)
>                                 	strbuf_addf(...);
> 				else
>                                 	strbuf_addch(*brk);
> 				fwrite(outbuf.buf, 1, outbuf.len, stderr);
> 				b = brk + 1;
> 			}
>                         if (*b) {
>                         	/* we still have something to say */
> 				strbuf_reset(&outbuf);
>                                 strbuf_addstr(&outbuf, PREFIX);
>                                	strbuf_addf(...);

This is also wrong.  If the middle part of a partial line is received, 
you just deleted its queued beginning.


Nicolas