| Message ID | 20230919071956.14015-1-worldhello.net@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pkt-line: do not chomp EOL for sideband progress info | expand |
Jiang Xin <worldhello.net@gmail.com> writes: > From: Jiang Xin <zhiyou.jx@alibaba-inc.com> Who knows packet_reader interface well? Jonathan? Thanks. > 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> > --- > pkt-line.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/pkt-line.c b/pkt-line.c > index af83a19f4d..d6d08b6aa6 100644 > --- a/pkt-line.c > +++ b/pkt-line.c > @@ -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; > + } > + > /* > * 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); > if (!reader->use_sideband) > break; > if (demultiplex_sideband(reader->me, reader->status, > @@ -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--; > + } > + } else { > reader->line = NULL; > + } > > return reader->status; > }
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. 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.
On Thu, Sep 21, 2023 at 5:08 AM Jonathan Tan <jonathantanmy@google.com> wrote: > > 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. 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
diff --git a/pkt-line.c b/pkt-line.c index af83a19f4d..d6d08b6aa6 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -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; + } + /* * 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); if (!reader->use_sideband) break; if (demultiplex_sideband(reader->me, reader->status, @@ -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--; + } + } else { reader->line = NULL; + } return reader->status; }