Nicolas Pitre <nico@xxxxxxxxxxx> 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;
...
}
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html