Jiang Xin <worldhello.net@xxxxxxxxx> writes:
> From: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
>
> In the protocol negotiation stage, we need to turn on the flag
> "PACKET_READ_CHOMP_NEWLINE" to chomp EOL for each packet line from
> client or server. But when receiving data and progress information
> using sideband, we will turn off the flag "PACKET_READ_CHOMP_NEWLINE"
> to prevent mangling EOLs from data and progress information.
>
> When both the server and the client support "sideband-all" capability,
> we have a dilemma that EOLs in negotiation packets should be trimmed,
> but EOLs in progress infomation should be leaved as is.
>
> Move the logic of chomping EOLs from "packet_read_with_status()" to
> "packet_reader_read()" can resolve this dilemma.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
I think the summary is that when we use the struct packet_reader with
sideband and newline chomping, we want the chomping to occur only on
sideband 1, but the current code also chomps on sidebands 2 and 3 (3
is for fatal errors so it doesn't matter as much, but for 2, it really
matters).
This makes sense to fix.
As for how this is fixed, one issue is that we now have 2 places in
which newlines can be chomped (in packet_read_with_status() and with
this patch, packet_reader_read()). The issue is that we need to check
the sideband indicator before we chomp, and packet_read_with_status()
only knows how to chomp. So we either teach packet_read_with_status()
how to sideband, or tell packet_read_with_status() not to chomp and
chomp it ourselves (like in this patch).
Of the two, I would prefer it if packet_read_with_status() was taught
how to sideband - as it is, packet_read_with_status() is used 3 times
in pkt-line.c and 1 time in remote-curl.c, and 2 of those times (in
pkt-line.c) are used with sideband. Doing this does not only solve the
problem here, but reduces code duplication.
Having said that, let me look at the code anyway.
> @@ -597,12 +597,18 @@ void packet_reader_init(struct packet_reader *reader, int fd,
> enum packet_read_status packet_reader_read(struct packet_reader *reader)
> {
> struct strbuf scratch = STRBUF_INIT;
> + int options = reader->options;
>
> if (reader->line_peeked) {
> reader->line_peeked = 0;
> return reader->status;
> }
>
> + /* Do not chomp newlines for sideband progress and error messages */
> + if (reader->use_sideband && options & PACKET_READ_CHOMP_NEWLINE) {
> + options &= ~PACKET_READ_CHOMP_NEWLINE;
> + }
> +
This needs a better explanation (than what's in the comment), I think.
What this code is doing is disabling chomping because we have code that
conditionally does it later.
> /*
> * Consume all progress packets until a primary payload packet is
> * received
> @@ -615,7 +621,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
> reader->buffer,
> reader->buffer_size,
> &reader->pktlen,
> - reader->options);
> + options);
OK, we're using our own custom options that may have
PACKET_READ_CHOMP_NEWLINE unset.
> @@ -624,12 +630,19 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
> break;
> }
>
> - if (reader->status == PACKET_READ_NORMAL)
> + if (reader->status == PACKET_READ_NORMAL) {
> /* Skip the sideband designator if sideband is used */
> reader->line = reader->use_sideband ?
> reader->buffer + 1 : reader->buffer;
> - else
> +
> + if ((reader->options & PACKET_READ_CHOMP_NEWLINE) &&
> + reader->buffer[reader->pktlen - 1] == '\n') {
> + reader->buffer[reader->pktlen - 1] = 0;
> + reader->pktlen--;
> + }
When we reach here, we have skipped all sideband-2 pkt-lines, so
unconditionally chomping it here is good. Might be better if there was
also a check that use_sideband is set, just for symmetry with the code
near the start of this function.
