On Thu, Sep 21, 2023 at 5:08 AM Jonathan Tan [off-list ref] wrote:

Jiang Xin [off-list ref] writes:

quoted

From: Jiang Xin <redacted>

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 <redacted>

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.

Yes, there are two places we can choose to fix. My first instinct is
that changes on packet_reader_read will have less impact. I will new
implementation in next reroll.

quoted

@@ -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.

You find my bug. Without checking the use_sideband flag, two
consecutive EOLwill be removed.

BTW, the new reroll is not coming as fast as I planned, because when I
adding new test cases, I find another issue in pkt-line. I will fix
these two issues in this series.

--
Jiang Xin