Re: [PATCH v4] Refactor recv_sideband()

[Junio C Hamano <gitster@pobox.com>]

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.

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);
                                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(...);
			}
                        break;
		...
                }
	}