Re: [PATCH] pkt-line: do not chomp EOL for sideband progress info

>
> Jiang Xin <worldhello.net@gmail.com> writes:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > 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@alibaba-inc.com>
>
> 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.

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