Git Mailing List Archive mirror
 help / color / mirror / code / Atom feed
* [PATCH] pkt-line: do not chomp EOL for sideband progress info
@ 2023-09-19  7:19 Jiang Xin
  2023-09-19 22:38 ` Junio C Hamano
  2023-09-20 21:08 ` Jonathan Tan
  0 siblings, 2 replies; 3+ messages in thread
From: Jiang Xin @ 2023-09-19  7:19 UTC (permalink / raw)
  To: Git List, Junio C Hamano, Jonathan Tan; +Cc: Jiang Xin

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>
---
 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;
 }
-- 
2.40.1.49.g40e13c3520.dirty


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] pkt-line: do not chomp EOL for sideband progress info
  2023-09-19  7:19 [PATCH] pkt-line: do not chomp EOL for sideband progress info Jiang Xin
@ 2023-09-19 22:38 ` Junio C Hamano
  2023-09-20 21:08 ` Jonathan Tan
  1 sibling, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2023-09-19 22:38 UTC (permalink / raw)
  To: Jiang Xin; +Cc: Git List, Jonathan Tan, Jiang Xin

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;
>  }

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] pkt-line: do not chomp EOL for sideband progress info
  2023-09-19  7:19 [PATCH] pkt-line: do not chomp EOL for sideband progress info Jiang Xin
  2023-09-19 22:38 ` Junio C Hamano
@ 2023-09-20 21:08 ` Jonathan Tan
  1 sibling, 0 replies; 3+ messages in thread
From: Jonathan Tan @ 2023-09-20 21:08 UTC (permalink / raw)
  To: Jiang Xin; +Cc: Jonathan Tan, Git List, Junio C Hamano, Jiang Xin

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.



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-09-20 21:08 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-19  7:19 [PATCH] pkt-line: do not chomp EOL for sideband progress info Jiang Xin
2023-09-19 22:38 ` Junio C Hamano
2023-09-20 21:08 ` Jonathan Tan

Code repositories for project(s) associated with this public inbox

	https://80x24.org/pub/scm/git/git.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).