On Thu, Sep 21, 2023 at 5:08 AM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
>
> 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.
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.
> > @@ -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
