threads / patch / 60244

patchpkt-line: do not chomp EOL for sideband progress info

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

## tl;dr

19 messages between Sep 19, 2023 and Dec 17, 2023. Diffs are folded; open one to read it.

replies: 18people: 4as markdown or json

Jiang Xin· Sep 19, 2023, 07:19 UTC · lore
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(-)
Show changes to pkt-line.c +16 −3
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
Junio C Hamano· Sep 19, 2023, 22:38 UTC · re: Jiang Xin · lore

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>
Who knows packet_reader interface well?  Jonathan?
Thanks.
Show 72 quoted lines
> 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;
>  }
Jonathan Tan· Sep 20, 2023, 21:08 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 16 quoted lines
> 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.
Show 16 quoted lines
> @@ -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.

Show 9 quoted lines
>  	/*
>  	 * 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.

Show 16 quoted lines
> @@ -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.

Jiang Xin· Sep 25, 2023, 00:25 UTC · re: Jonathan Tan · lore

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

On Thu, Sep 21, 2023 at 5:08 AM Jonathan Tan <jonathantanmy@google.com> wrote:
Show 40 quoted lines
>
> 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.

Show 22 quoted lines
> > @@ -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

Jiang Xin· Sep 25, 2023, 15:41 UTC · re: Jiang Xin · lore

[PATCH v2 1/3] test-pkt-line: add option parser for unpack-sideband

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

We can use the test helper program "test-tool pkt-line" to test pkt-line related functions. E.g.:

 * Use "test-tool pkt-line send-split-sideband" to generate sideband
   messages.
 * We can pipe these generated sideband messages to command "test-tool
   pkt-line unpack-sideband" to test packet_reader_read() function.

In order to make a complete test of the packet_reader_read() function, add option parser for command "test-tool pkt-line unpack-sideband".

To remove newlines in sideband messages, we can use:
    $ test-tool pkt-line unpack-sideband --chomp-newline
To preserve newlines in sideband messages, we can use:
    $ test-tool pkt-line unpack-sideband --no-chomp-newline

To parse sideband messages using "demultiplex_sideband()" inside the function "packet_reader_read()", we can use:

    $ test-tool pkt-line unpack-sideband --reader-use-sideband

Add several new test cases in t0070. Among these test cases, we pipe output of the "send-split-sideband" subcommand to the "unpack-sideband" subcommand. We found two issues:

 1. The two splitted sideband messages "Hello," and " world!\n" should
    be concatenated together. But when we enabled the function
    "demultiplex_sideband()" to parse sideband messages, the first part
    of the splitted message ("Hello,") is lost.
 2. The newline characters in sideband 2 (progress info) and sideband 3
    (error message) should be preserved, but they are also trimmed.
Will fix the above two issues in subsequent commits.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/helper/test-pkt-line.c | 58 ++++++++++++++++++++++++++++++++++++----
 t/t0070-fundamental.sh   | 58 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 5 deletions(-)
Show changes to 2 files +111 −5

t/helper/test-pkt-line.c, t/t0070-fundamental.sh

diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index f4d134a145..9aa35f7861 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -2,6 +2,7 @@
 #include "test-tool.h"
 #include "pkt-line.h"
 #include "write-or-die.h"
+#include "parse-options.h"
 
 static void pack_line(const char *line)
 {
@@ -64,12 +65,33 @@ static void unpack(void)
 	}
 }
 
-static void unpack_sideband(void)
+static void unpack_sideband(int argc, const char **argv)
 {
 	struct packet_reader reader;
-	packet_reader_init(&reader, 0, NULL, 0,
-			   PACKET_READ_GENTLE_ON_EOF |
-			   PACKET_READ_CHOMP_NEWLINE);
+	int options = PACKET_READ_GENTLE_ON_EOF;
+	int chomp_newline = 1;
+	int reader_use_sideband = 0;
+	const char *const unpack_sideband_usage[] = {
+		"test_tool unpack_sideband [options...]", NULL
+	};
+	struct option cmd_options[] = {
+		OPT_BOOL(0, "reader-use-sideband", &reader_use_sideband,
+			 "set use_sideband bit for packet reader (Default: off)"),
+		OPT_BOOL(0, "chomp-newline", &chomp_newline,
+			 "chomp newline in packet (Default: on)"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, "", cmd_options, unpack_sideband_usage,
+			     0);
+	if (argc > 0)
+		usage_msg_opt(_("too many arguments"), unpack_sideband_usage,
+			      cmd_options);
+
+	if (chomp_newline)
+		options |= PACKET_READ_CHOMP_NEWLINE;
+	packet_reader_init(&reader, 0, NULL, 0, options);
+	reader.use_sideband = reader_use_sideband;
 
 	while (packet_reader_read(&reader) != PACKET_READ_EOF) {
 		int band;
@@ -79,6 +101,16 @@ static void unpack_sideband(void)
 		case PACKET_READ_EOF:
 			break;
 		case PACKET_READ_NORMAL:
+			/*
+			 * When the "use_sideband" field of the reader is turned
+			 * on, sideband packets other than the payload have been
+			 * parsed and consumed.
+			 */
+			if (reader.use_sideband) {
+				write_or_die(1, reader.line, reader.pktlen - 1);
+				break;
+			}
+
 			band = reader.line[0] & 0xff;
 			if (band < 1 || band > 2)
 				continue; /* skip non-sideband packets */
@@ -97,15 +129,31 @@ static void unpack_sideband(void)
 
 static int send_split_sideband(void)
 {
+	const char *foo = "Foo.\n";
+	const char *bar = "Bar.\n";
 	const char *part1 = "Hello,";
 	const char *primary = "\001primary: regular output\n";
 	const char *part2 = " world!\n";
 
+	/* Each sideband message has a trailing newline character. */
+	send_sideband(1, 2, foo, strlen(foo), LARGE_PACKET_MAX);
+	send_sideband(1, 2, bar, strlen(bar), LARGE_PACKET_MAX);
+
+	/*
+	 * One sideband message is divided into part1 and part2
+	 * by the primary message.
+	 */
 	send_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);
 	packet_write(1, primary, strlen(primary));
 	send_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);
 	packet_response_end(1);
 
+	/*
+	 * The unpack_sideband() function above requires a flush
+	 * packet to end parsing.
+	 */
+	packet_flush(1);
+
 	return 0;
 }
 
@@ -126,7 +174,7 @@ int cmd__pkt_line(int argc, const char **argv)
 	else if (!strcmp(argv[1], "unpack"))
 		unpack();
 	else if (!strcmp(argv[1], "unpack-sideband"))
-		unpack_sideband();
+		unpack_sideband(argc - 1, argv + 1);
 	else if (!strcmp(argv[1], "send-split-sideband"))
 		send_split_sideband();
 	else if (!strcmp(argv[1], "receive-sideband"))
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 574de34198..1053913d2d 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -53,4 +53,62 @@ test_expect_success 'missing sideband designator is reported' '
 	test_i18ngrep "missing sideband" err
 '
 
+test_expect_success 'unpack-sideband: --no-chomp-newline' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--no-chomp-newline <split-sideband >out 2>err &&
+	cat >expect-out <<-EOF &&
+		primary: regular output
+	EOF
+	cat >expect-err <<-EOF &&
+		Foo.
+		Bar.
+		Hello, world!
+	EOF
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
+test_expect_success 'unpack-sideband: --chomp-newline (default)' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--chomp-newline <split-sideband >out 2>err &&
+	printf "primary: regular output" >expect-out &&
+	printf "Foo.Bar.Hello, world!" >expect-err &&
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--reader-use-sideband \
+		--no-chomp-newline <split-sideband >out 2>err &&
+	cat >expect-out <<-EOF &&
+		primary: regular output
+	EOF
+	printf "remote: Foo.        \n"           >expect-err &&
+	printf "remote: Bar.        \n"          >>expect-err &&
+	printf "remote: Hello, world!        \n" >>expect-err &&
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--reader-use-sideband \
+		--chomp-newline <split-sideband >out 2>err &&
+	printf "primary: regular output" >expect-out &&
+	printf "remote: Foo.        \n"           >expect-err &&
+	printf "remote: Bar.        \n"          >>expect-err &&
+	printf "remote: Hello, world!        \n" >>expect-err &&
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
 test_done
-- 
2.40.1.50.gf560bcc116.dirty
Jiang Xin· Sep 25, 2023, 15:41 UTC · re: Jiang Xin · lore

[PATCH v2 2/3] pkt-line: memorize sideband fragment in reader

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When we turn on the "use_sideband" field of the packet_reader, "packet_reader_read()" will call the function "demultiplex_sideband()" to parse and consume sideband messages. Sideband fragment which does not end with "\r" or "\n" will be saved in the sixth parameter "scratch" and it can be reused and be concatenated when parsing another sideband message.

In "packet_reader_read()" function, the local variable "scratch" can only be reused by subsequent sideband messages. But if there is a payload message between two sideband fragments, the first fragment which is saved in the local variable "scratch" will be lost.

To solve this problem, we can add a new field "scratch" in packet_reader to memorize the sideband fragment across different calls of "packet_reader_read()".

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 pkt-line.c             | 5 ++---
 pkt-line.h             | 3 +++
 t/t0070-fundamental.sh | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)
Show changes to 3 files +6 −4

pkt-line.c, pkt-line.h, t/t0070-fundamental.sh

diff --git a/pkt-line.c b/pkt-line.c
index af83a19f4d..5943777a17 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -592,12 +592,11 @@ void packet_reader_init(struct packet_reader *reader, int fd,
 	reader->options = options;
 	reader->me = "git";
 	reader->hash_algo = &hash_algos[GIT_HASH_SHA1];
+	strbuf_init(&reader->scratch, 0);
 }
 
 enum packet_read_status packet_reader_read(struct packet_reader *reader)
 {
-	struct strbuf scratch = STRBUF_INIT;
-
 	if (reader->line_peeked) {
 		reader->line_peeked = 0;
 		return reader->status;
@@ -620,7 +619,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 			break;
 		if (demultiplex_sideband(reader->me, reader->status,
 					 reader->buffer, reader->pktlen, 1,
-					 &scratch, &sideband_type))
+					 &reader->scratch, &sideband_type))
 			break;
 	}
 
diff --git a/pkt-line.h b/pkt-line.h
index 954eec8719..be1010d34e 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -194,6 +194,9 @@ struct packet_reader {
 
 	/* hash algorithm in use */
 	const struct git_hash_algo *hash_algo;
+
+	/* hold temporary sideband message */
+	struct strbuf scratch;
 };
 
 /*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 1053913d2d..a927c665d6 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -81,7 +81,7 @@ test_expect_success 'unpack-sideband: --chomp-newline (default)' '
 	test_cmp expect-err err
 '
 
-test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
+test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
 	test_when_finished "rm -f expect-out expect-err" &&
 	test-tool pkt-line send-split-sideband >split-sideband &&
 	test-tool pkt-line unpack-sideband \
-- 
2.40.1.50.gf560bcc116.dirty
Jiang Xin· Sep 25, 2023, 15:41 UTC · re: Jiang Xin · lore

[PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When calling "packet_read_with_status()" to parse pkt-line encoded packets, we can turn on the flag "PACKET_READ_CHOMP_NEWLINE" to chomp newline character for each packet for better line matching. But when receiving data and progress information using sideband, we should turn off the flag "PACKET_READ_CHOMP_NEWLINE" to prevent mangling newline characters from data and progress information.

When both the server and the client support "sideband-all" capability, we have a dilemma that newline characters in negotiation packets should be removed, but the newline characters in the progress information should be left intact.

Add new flag "PACKET_READ_USE_SIDEBAND" for "packet_read_with_status()" to prevent mangling newline characters in sideband messages.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 pkt-line.c             | 32 ++++++++++++++++++++++++++++++--
 pkt-line.h             |  1 +
 t/t0070-fundamental.sh |  2 +-
 3 files changed, 32 insertions(+), 3 deletions(-)
Show changes to 3 files +32 −3

pkt-line.c, pkt-line.h, t/t0070-fundamental.sh

diff --git a/pkt-line.c b/pkt-line.c
index 5943777a17..865ad19484 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 	}
 
 	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
-	    len && buffer[len-1] == '\n')
-		len--;
+	    len && buffer[len-1] == '\n') {
+		if (options & PACKET_READ_USE_SIDEBAND) {
+			int band = *buffer & 0xff;
+			switch (band) {
+			case 1:
+				/* Chomp newline for payload */
+				len--;
+				break;
+			case 2:
+				/* fallthrough */
+			case 3:
+				/*
+				 * Do not chomp newline for progress and error
+				 * message.
+				 */
+				break;
+			default:
+				/*
+				 * Bad sideband, let's leave it to
+				 * demultiplex_sideband() to catch this error.
+				 */
+				break;
+			}
+		} else {
+			len--;
+		}
+	}
 
 	buffer[len] = 0;
 	if (options & PACKET_READ_REDACT_URI_PATH &&
@@ -602,6 +627,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 		return reader->status;
 	}
 
+	if (reader->use_sideband)
+		reader->options |= PACKET_READ_USE_SIDEBAND;
+
 	/*
 	 * Consume all progress packets until a primary payload packet is
 	 * received
diff --git a/pkt-line.h b/pkt-line.h
index be1010d34e..a7ff2e2f18 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -85,6 +85,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
 #define PACKET_READ_REDACT_URI_PATH      (1u<<4)
+#define PACKET_READ_USE_SIDEBAND         (1u<<5)
 int packet_read(int fd, char *buffer, unsigned size, int options);
 
 /*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index a927c665d6..138c2becc1 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli
 	test_cmp expect-err err
 '
 
-test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
+test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '
 	test_when_finished "rm -f expect-out expect-err" &&
 	test-tool pkt-line send-split-sideband >split-sideband &&
 	test-tool pkt-line unpack-sideband \
-- 
2.40.1.50.gf560bcc116.dirty
Junio C Hamano· Sep 25, 2023, 21:51 UTC · re: Jiang Xin · lore

Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages

Jiang Xin <worldhello.net@gmail.com> writes:
Show 48 quoted lines
> Add new flag "PACKET_READ_USE_SIDEBAND" for "packet_read_with_status()"
> to prevent mangling newline characters in sideband messages.
>
> Helped-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  pkt-line.c             | 32 ++++++++++++++++++++++++++++++--
>  pkt-line.h             |  1 +
>  t/t0070-fundamental.sh |  2 +-
>  3 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 5943777a17..865ad19484 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
>  	}
>  
>  	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> -	    len && buffer[len-1] == '\n')
> -		len--;
> +	    len && buffer[len-1] == '\n') {
> +		if (options & PACKET_READ_USE_SIDEBAND) {
> +			int band = *buffer & 0xff;
> +			switch (band) {
> +			case 1:
> +				/* Chomp newline for payload */
> +				len--;
> +				break;
> +			case 2:
> +				/* fallthrough */
> +			case 3:
> +				/*
> +				 * Do not chomp newline for progress and error
> +				 * message.
> +				 */
> +				break;
> +			default:
> +				/*
> +				 * Bad sideband, let's leave it to
> +				 * demultiplex_sideband() to catch this error.
> +				 */
> +				break;
> +			}
> +		} else {
> +			len--;
> +		}
> +	}

That's a mouthful and we could shorten it a lot, but is very easy to follow the logic ;-)

Show 13 quoted lines
> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
> index a927c665d6..138c2becc1 100755
> --- a/t/t0070-fundamental.sh
> +++ b/t/t0070-fundamental.sh
> @@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli
>  	test_cmp expect-err err
>  '
>  
> -test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
> +test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '
>  	test_when_finished "rm -f expect-out expect-err" &&
>  	test-tool pkt-line send-split-sideband >split-sideband &&
>  	test-tool pkt-line unpack-sideband \

We cannot quite see what got fixed that is outside the postimage, but it is nice that we have one fewer test_expect_failure in the end.

Will queue.  Thanks.
Oswald Buddenhagen· Sep 26, 2023, 08:48 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages

Show 9 quoted lines
>Jiang Xin <worldhello.net@gmail.com> writes:
>
>> +++ b/pkt-line.c
>> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
>>  	}
>> +			case 2:
>> +				/* fallthrough */
>> +			case 3:
>

while not entirely unprecedented, it's unnecessary and even counter-productive to annotate directly adjacent cases with fallthrough.

regards
Jiang Xin· Oct 4, 2023, 13:02 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages

On Tue, Sep 26, 2023 at 4:48 PM Oswald Buddenhagen <oswald.buddenhagen@gmx.de> wrote:

Show 12 quoted lines
>
> >Jiang Xin <worldhello.net@gmail.com> writes:
> >
> >> +++ b/pkt-line.c
> >> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> >>      }
> >> +                    case 2:
> >> +                            /* fallthrough */
> >> +                    case 3:
> >
> while not entirely unprecedented, it's unnecessary and even
> counter-productive to annotate directly adjacent cases with fallthrough.

I see in "blame.c" there are directly adjacent cases like below. I will remove the fallthrough statement.

        case 'A':
        case 'T':
                /* Did not exist in parent, or type changed */
                break;
Thanks.
Junio C Hamano· Oct 4, 2023, 20:05 UTC · re: Jiang Xin · lore

Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages

Jiang Xin <worldhello.net@gmail.com> writes:
Show 22 quoted lines
> On Tue, Sep 26, 2023 at 4:48 PM Oswald Buddenhagen
> <oswald.buddenhagen@gmx.de> wrote:
>>
>> >Jiang Xin <worldhello.net@gmail.com> writes:
>> >
>> >> +++ b/pkt-line.c
>> >> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
>> >>      }
>> >> +                    case 2:
>> >> +                            /* fallthrough */
>> >> +                    case 3:
>> >
>> while not entirely unprecedented, it's unnecessary and even
>> counter-productive to annotate directly adjacent cases with fallthrough.
>
> I see in "blame.c" there are directly adjacent cases like below. I
> will remove the fallthrough statement.
>
>         case 'A':
>         case 'T':
>                 /* Did not exist in parent, or type changed */
>                 break;

Yeah, it is far clearer to understand if it is written without the "fallthru" comment between the cases and instead a comment that explains both cases after them (exactly like the example you found in "blame.c"). When we want "fallthru" comment is if we had some processing specific to the earlier case ('A' or '2') that is not done in the later case ('T' or '3'), in which case we may want to explicitly say we did not forget to "break" by adding the "fallthru" comment. But it does not apply here.

Thanks.
Jiang Xin· Oct 4, 2023, 13:18 UTC · re: Oswald Buddenhagen · lore

[PATCH v3 0/3] Sideband demultiplexer fixes

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Sideband demultiplexer fixes.
Range-diff v2...v3
1:  fd6d61893d ! 1:  68ac3ea711 pkt-line: do not chomp newlines for sideband messages
    @@ Commit message
         to prevent mangling newline characters in sideband messages.
     
         Helped-by: Jonathan Tan <jonathantanmy@google.com>
    +    Helped-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## pkt-line.c ##
    @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
     +				len--;
     +				break;
     +			case 2:
    -+				/* fallthrough */
     +			case 3:
     +				/*
     +				 * Do not chomp newline for progress and error
---
Jiang Xin (3):
  test-pkt-line: add option parser for unpack-sideband
  pkt-line: memorize sideband fragment in reader
  pkt-line: do not chomp newlines for sideband messages
 pkt-line.c               | 36 +++++++++++++++++++++----
 pkt-line.h               |  4 +++
 t/helper/test-pkt-line.c | 58 ++++++++++++++++++++++++++++++++++++----
 t/t0070-fundamental.sh   | 58 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 146 insertions(+), 10 deletions(-)
-- 
2.40.1.50.gf560bcc116.dirty
Jiang Xin· Oct 4, 2023, 13:18 UTC · re: Jiang Xin · lore

[PATCH v3 2/3] pkt-line: memorize sideband fragment in reader

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When we turn on the "use_sideband" field of the packet_reader, "packet_reader_read()" will call the function "demultiplex_sideband()" to parse and consume sideband messages. Sideband fragment which does not end with "\r" or "\n" will be saved in the sixth parameter "scratch" and it can be reused and be concatenated when parsing another sideband message.

In "packet_reader_read()" function, the local variable "scratch" can only be reused by subsequent sideband messages. But if there is a payload message between two sideband fragments, the first fragment which is saved in the local variable "scratch" will be lost.

To solve this problem, we can add a new field "scratch" in packet_reader to memorize the sideband fragment across different calls of "packet_reader_read()".

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 pkt-line.c             | 5 ++---
 pkt-line.h             | 3 +++
 t/t0070-fundamental.sh | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)
Show changes to 3 files +6 −4

pkt-line.c, pkt-line.h, t/t0070-fundamental.sh

diff --git a/pkt-line.c b/pkt-line.c
index af83a19f4d..5943777a17 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -592,12 +592,11 @@ void packet_reader_init(struct packet_reader *reader, int fd,
 	reader->options = options;
 	reader->me = "git";
 	reader->hash_algo = &hash_algos[GIT_HASH_SHA1];
+	strbuf_init(&reader->scratch, 0);
 }
 
 enum packet_read_status packet_reader_read(struct packet_reader *reader)
 {
-	struct strbuf scratch = STRBUF_INIT;
-
 	if (reader->line_peeked) {
 		reader->line_peeked = 0;
 		return reader->status;
@@ -620,7 +619,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 			break;
 		if (demultiplex_sideband(reader->me, reader->status,
 					 reader->buffer, reader->pktlen, 1,
-					 &scratch, &sideband_type))
+					 &reader->scratch, &sideband_type))
 			break;
 	}
 
diff --git a/pkt-line.h b/pkt-line.h
index 954eec8719..be1010d34e 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -194,6 +194,9 @@ struct packet_reader {
 
 	/* hash algorithm in use */
 	const struct git_hash_algo *hash_algo;
+
+	/* hold temporary sideband message */
+	struct strbuf scratch;
 };
 
 /*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 1053913d2d..a927c665d6 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -81,7 +81,7 @@ test_expect_success 'unpack-sideband: --chomp-newline (default)' '
 	test_cmp expect-err err
 '
 
-test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
+test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
 	test_when_finished "rm -f expect-out expect-err" &&
 	test-tool pkt-line send-split-sideband >split-sideband &&
 	test-tool pkt-line unpack-sideband \
-- 
2.40.1.50.gf560bcc116.dirty
Jiang Xin· Oct 4, 2023, 13:18 UTC · re: Jiang Xin · lore

[PATCH v3 1/3] test-pkt-line: add option parser for unpack-sideband

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

We can use the test helper program "test-tool pkt-line" to test pkt-line related functions. E.g.:

 * Use "test-tool pkt-line send-split-sideband" to generate sideband
   messages.
 * We can pipe these generated sideband messages to command "test-tool
   pkt-line unpack-sideband" to test packet_reader_read() function.

In order to make a complete test of the packet_reader_read() function, add option parser for command "test-tool pkt-line unpack-sideband".

To remove newlines in sideband messages, we can use:
    $ test-tool pkt-line unpack-sideband --chomp-newline
To preserve newlines in sideband messages, we can use:
    $ test-tool pkt-line unpack-sideband --no-chomp-newline

To parse sideband messages using "demultiplex_sideband()" inside the function "packet_reader_read()", we can use:

    $ test-tool pkt-line unpack-sideband --reader-use-sideband

Add several new test cases in t0070. Among these test cases, we pipe output of the "send-split-sideband" subcommand to the "unpack-sideband" subcommand. We found two issues:

 1. The two splitted sideband messages "Hello," and " world!\n" should
    be concatenated together. But when we enabled the function
    "demultiplex_sideband()" to parse sideband messages, the first part
    of the splitted message ("Hello,") is lost.
 2. The newline characters in sideband 2 (progress info) and sideband 3
    (error message) should be preserved, but they are also trimmed.
Will fix the above two issues in subsequent commits.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/helper/test-pkt-line.c | 58 ++++++++++++++++++++++++++++++++++++----
 t/t0070-fundamental.sh   | 58 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 5 deletions(-)
Show changes to 2 files +111 −5

t/helper/test-pkt-line.c, t/t0070-fundamental.sh

diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index f4d134a145..9aa35f7861 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -2,6 +2,7 @@
 #include "test-tool.h"
 #include "pkt-line.h"
 #include "write-or-die.h"
+#include "parse-options.h"
 
 static void pack_line(const char *line)
 {
@@ -64,12 +65,33 @@ static void unpack(void)
 	}
 }
 
-static void unpack_sideband(void)
+static void unpack_sideband(int argc, const char **argv)
 {
 	struct packet_reader reader;
-	packet_reader_init(&reader, 0, NULL, 0,
-			   PACKET_READ_GENTLE_ON_EOF |
-			   PACKET_READ_CHOMP_NEWLINE);
+	int options = PACKET_READ_GENTLE_ON_EOF;
+	int chomp_newline = 1;
+	int reader_use_sideband = 0;
+	const char *const unpack_sideband_usage[] = {
+		"test_tool unpack_sideband [options...]", NULL
+	};
+	struct option cmd_options[] = {
+		OPT_BOOL(0, "reader-use-sideband", &reader_use_sideband,
+			 "set use_sideband bit for packet reader (Default: off)"),
+		OPT_BOOL(0, "chomp-newline", &chomp_newline,
+			 "chomp newline in packet (Default: on)"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, "", cmd_options, unpack_sideband_usage,
+			     0);
+	if (argc > 0)
+		usage_msg_opt(_("too many arguments"), unpack_sideband_usage,
+			      cmd_options);
+
+	if (chomp_newline)
+		options |= PACKET_READ_CHOMP_NEWLINE;
+	packet_reader_init(&reader, 0, NULL, 0, options);
+	reader.use_sideband = reader_use_sideband;
 
 	while (packet_reader_read(&reader) != PACKET_READ_EOF) {
 		int band;
@@ -79,6 +101,16 @@ static void unpack_sideband(void)
 		case PACKET_READ_EOF:
 			break;
 		case PACKET_READ_NORMAL:
+			/*
+			 * When the "use_sideband" field of the reader is turned
+			 * on, sideband packets other than the payload have been
+			 * parsed and consumed.
+			 */
+			if (reader.use_sideband) {
+				write_or_die(1, reader.line, reader.pktlen - 1);
+				break;
+			}
+
 			band = reader.line[0] & 0xff;
 			if (band < 1 || band > 2)
 				continue; /* skip non-sideband packets */
@@ -97,15 +129,31 @@ static void unpack_sideband(void)
 
 static int send_split_sideband(void)
 {
+	const char *foo = "Foo.\n";
+	const char *bar = "Bar.\n";
 	const char *part1 = "Hello,";
 	const char *primary = "\001primary: regular output\n";
 	const char *part2 = " world!\n";
 
+	/* Each sideband message has a trailing newline character. */
+	send_sideband(1, 2, foo, strlen(foo), LARGE_PACKET_MAX);
+	send_sideband(1, 2, bar, strlen(bar), LARGE_PACKET_MAX);
+
+	/*
+	 * One sideband message is divided into part1 and part2
+	 * by the primary message.
+	 */
 	send_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);
 	packet_write(1, primary, strlen(primary));
 	send_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);
 	packet_response_end(1);
 
+	/*
+	 * The unpack_sideband() function above requires a flush
+	 * packet to end parsing.
+	 */
+	packet_flush(1);
+
 	return 0;
 }
 
@@ -126,7 +174,7 @@ int cmd__pkt_line(int argc, const char **argv)
 	else if (!strcmp(argv[1], "unpack"))
 		unpack();
 	else if (!strcmp(argv[1], "unpack-sideband"))
-		unpack_sideband();
+		unpack_sideband(argc - 1, argv + 1);
 	else if (!strcmp(argv[1], "send-split-sideband"))
 		send_split_sideband();
 	else if (!strcmp(argv[1], "receive-sideband"))
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 574de34198..1053913d2d 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -53,4 +53,62 @@ test_expect_success 'missing sideband designator is reported' '
 	test_i18ngrep "missing sideband" err
 '
 
+test_expect_success 'unpack-sideband: --no-chomp-newline' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--no-chomp-newline <split-sideband >out 2>err &&
+	cat >expect-out <<-EOF &&
+		primary: regular output
+	EOF
+	cat >expect-err <<-EOF &&
+		Foo.
+		Bar.
+		Hello, world!
+	EOF
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
+test_expect_success 'unpack-sideband: --chomp-newline (default)' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--chomp-newline <split-sideband >out 2>err &&
+	printf "primary: regular output" >expect-out &&
+	printf "Foo.Bar.Hello, world!" >expect-err &&
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--reader-use-sideband \
+		--no-chomp-newline <split-sideband >out 2>err &&
+	cat >expect-out <<-EOF &&
+		primary: regular output
+	EOF
+	printf "remote: Foo.        \n"           >expect-err &&
+	printf "remote: Bar.        \n"          >>expect-err &&
+	printf "remote: Hello, world!        \n" >>expect-err &&
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--reader-use-sideband \
+		--chomp-newline <split-sideband >out 2>err &&
+	printf "primary: regular output" >expect-out &&
+	printf "remote: Foo.        \n"           >expect-err &&
+	printf "remote: Bar.        \n"          >>expect-err &&
+	printf "remote: Hello, world!        \n" >>expect-err &&
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
 test_done
-- 
2.40.1.50.gf560bcc116.dirty
Jiang Xin· Oct 4, 2023, 13:18 UTC · re: Jiang Xin · lore

[PATCH v3 3/3] pkt-line: do not chomp newlines for sideband messages

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When calling "packet_read_with_status()" to parse pkt-line encoded packets, we can turn on the flag "PACKET_READ_CHOMP_NEWLINE" to chomp newline character for each packet for better line matching. But when receiving data and progress information using sideband, we should turn off the flag "PACKET_READ_CHOMP_NEWLINE" to prevent mangling newline characters from data and progress information.

When both the server and the client support "sideband-all" capability, we have a dilemma that newline characters in negotiation packets should be removed, but the newline characters in the progress information should be left intact.

Add new flag "PACKET_READ_USE_SIDEBAND" for "packet_read_with_status()" to prevent mangling newline characters in sideband messages.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 pkt-line.c             | 31 +++++++++++++++++++++++++++++--
 pkt-line.h             |  1 +
 t/t0070-fundamental.sh |  2 +-
 3 files changed, 31 insertions(+), 3 deletions(-)
Show changes to 3 files +31 −3

pkt-line.c, pkt-line.h, t/t0070-fundamental.sh

diff --git a/pkt-line.c b/pkt-line.c
index 5943777a17..e9061e61a4 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -462,8 +462,32 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 	}
 
 	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
-	    len && buffer[len-1] == '\n')
-		len--;
+	    len && buffer[len-1] == '\n') {
+		if (options & PACKET_READ_USE_SIDEBAND) {
+			int band = *buffer & 0xff;
+			switch (band) {
+			case 1:
+				/* Chomp newline for payload */
+				len--;
+				break;
+			case 2:
+			case 3:
+				/*
+				 * Do not chomp newline for progress and error
+				 * message.
+				 */
+				break;
+			default:
+				/*
+				 * Bad sideband, let's leave it to
+				 * demultiplex_sideband() to catch this error.
+				 */
+				break;
+			}
+		} else {
+			len--;
+		}
+	}
 
 	buffer[len] = 0;
 	if (options & PACKET_READ_REDACT_URI_PATH &&
@@ -602,6 +626,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 		return reader->status;
 	}
 
+	if (reader->use_sideband)
+		reader->options |= PACKET_READ_USE_SIDEBAND;
+
 	/*
 	 * Consume all progress packets until a primary payload packet is
 	 * received
diff --git a/pkt-line.h b/pkt-line.h
index be1010d34e..a7ff2e2f18 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -85,6 +85,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
 #define PACKET_READ_REDACT_URI_PATH      (1u<<4)
+#define PACKET_READ_USE_SIDEBAND         (1u<<5)
 int packet_read(int fd, char *buffer, unsigned size, int options);
 
 /*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index a927c665d6..138c2becc1 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli
 	test_cmp expect-err err
 '
 
-test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
+test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '
 	test_when_finished "rm -f expect-out expect-err" &&
 	test-tool pkt-line send-split-sideband >split-sideband &&
 	test-tool pkt-line unpack-sideband \
-- 
2.40.1.50.gf560bcc116.dirty
Jiang Xin· Dec 17, 2023, 14:41 UTC · re: Jiang Xin · lore

[PATCH v4 0/3] Sideband-all demultiplexer fixes

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
# Changes since v3:
Change commit message and comments to make them clear for review.
# range-diff v3...v4
1:  e387088da2 ! 1:  ff4e5aff2a test-pkt-line: add option parser for unpack-sideband
    @@ Commit message
          * Use "test-tool pkt-line send-split-sideband" to generate sideband
            messages.
     
    -     * We can pipe these generated sideband messages to command "test-tool
    -       pkt-line unpack-sideband" to test packet_reader_read() function.
    +     * Pipe these generated sideband messages to command "test-tool pkt-line
    +       unpack-sideband" to test packet_reader_read() function.
     
         In order to make a complete test of the packet_reader_read() function,
         add option parser for command "test-tool pkt-line unpack-sideband".
     
    -    To remove newlines in sideband messages, we can use:
    +     * To remove newlines in sideband messages, we can use:
     
    -        $ test-tool pkt-line unpack-sideband --chomp-newline
    +            $ test-tool pkt-line unpack-sideband --chomp-newline
     
    -    To preserve newlines in sideband messages, we can use:
    +     * To preserve newlines in sideband messages, we can use:
     
    -        $ test-tool pkt-line unpack-sideband --no-chomp-newline
    +            $ test-tool pkt-line unpack-sideband --no-chomp-newline
     
    -    To parse sideband messages using "demultiplex_sideband()" inside the
    -    function "packet_reader_read()", we can use:
    +     * To parse sideband messages using "demultiplex_sideband()" inside the
    +       function "packet_reader_read()", we can use:
     
    -        $ test-tool pkt-line unpack-sideband --reader-use-sideband
    +            $ test-tool pkt-line unpack-sideband --reader-use-sideband
     
    -    Add several new test cases in t0070. Among these test cases, we pipe
    +    We also add new example sideband packets in send_split_sideband() and
    +    add several new test cases in t0070. Among these test cases, we pipe
         output of the "send-split-sideband" subcommand to the "unpack-sideband"
         subcommand. We found two issues:
     
          1. The two splitted sideband messages "Hello," and " world!\n" should
    -        be concatenated together. But when we enabled the function
    -        "demultiplex_sideband()" to parse sideband messages, the first part
    -        of the splitted message ("Hello,") is lost.
    +        be concatenated together. But when we turn on use_sideband field of
    +        reader to parse sideband messages, the first part of the splitted
    +        message ("Hello,") is lost.
     
          2. The newline characters in sideband 2 (progress info) and sideband 3
    -        (error message) should be preserved, but they are also trimmed.
    +        (error message) should be preserved, but they are both trimmed.
     
         Will fix the above two issues in subsequent commits.
     
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## t/helper/test-pkt-line.c ##
     @@
    @@ t/helper/test-pkt-line.c: static void unpack_sideband(void)
     +			/*
     +			 * When the "use_sideband" field of the reader is turned
     +			 * on, sideband packets other than the payload have been
    -+			 * parsed and consumed.
    ++			 * parsed and consumed in packet_reader_read(), and only
    ++			 * the payload arrives here.
     +			 */
     +			if (reader.use_sideband) {
     +				write_or_die(1, reader.line, reader.pktlen - 1);
    @@ t/helper/test-pkt-line.c: static void unpack_sideband(void)
      	packet_response_end(1);
      
     +	/*
    -+	 * The unpack_sideband() function above requires a flush
    -+	 * packet to end parsing.
    ++	 * We use unpack_sideband() to consume packets. A flush packet
    ++	 * is required to end parsing.
     +	 */
     +	packet_flush(1);
     +
    @@ t/t0070-fundamental.sh: test_expect_success 'missing sideband designator is repo
     +	test_cmp expect-err err
     +'
     +
    -+test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
    ++test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
     +	test_when_finished "rm -f expect-out expect-err" &&
     +	test-tool pkt-line send-split-sideband >split-sideband &&
     +	test-tool pkt-line unpack-sideband \
    @@ t/t0070-fundamental.sh: test_expect_success 'missing sideband designator is repo
     +	test_cmp expect-err err
     +'
     +
    -+test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
    ++test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
     +	test_when_finished "rm -f expect-out expect-err" &&
     +	test-tool pkt-line send-split-sideband >split-sideband &&
     +	test-tool pkt-line unpack-sideband \
2:  633bfbac39 ! 2:  5942b74cab pkt-line: memorize sideband fragment in reader
    @@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband: --chomp-newline (d
      	test_cmp expect-err err
      '
      
    --test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
    -+test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
    +-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
    ++test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
      	test_when_finished "rm -f expect-out expect-err" &&
      	test-tool pkt-line send-split-sideband >split-sideband &&
      	test-tool pkt-line unpack-sideband \
3:  2a2da65fac ! 3:  dd2e34da16 pkt-line: do not chomp newlines for sideband messages
    @@ pkt-line.h: void packet_fflush(FILE *f);
      /*
     
      ## t/t0070-fundamental.sh ##
    -@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli
    +@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no
      	test_cmp expect-err err
      '
      
    --test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
    -+test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '
    +-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
    ++test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
      	test_when_finished "rm -f expect-out expect-err" &&
      	test-tool pkt-line send-split-sideband >split-sideband &&
      	test-tool pkt-line unpack-sideband \
Jiang Xin (3):
  test-pkt-line: add option parser for unpack-sideband
  pkt-line: memorize sideband fragment in reader
  pkt-line: do not chomp newlines for sideband messages
 pkt-line.c               | 36 ++++++++++++++++++++----
 pkt-line.h               |  4 +++
 t/helper/test-pkt-line.c | 59 ++++++++++++++++++++++++++++++++++++----
 t/t0070-fundamental.sh   | 58 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 147 insertions(+), 10 deletions(-)
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
Jiang Xin· Dec 17, 2023, 14:41 UTC · re: Jiang Xin · lore

[PATCH v4 1/3] test-pkt-line: add option parser for unpack-sideband

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

We can use the test helper program "test-tool pkt-line" to test pkt-line related functions. E.g.:

 * Use "test-tool pkt-line send-split-sideband" to generate sideband
   messages.
 * Pipe these generated sideband messages to command "test-tool pkt-line
   unpack-sideband" to test packet_reader_read() function.

In order to make a complete test of the packet_reader_read() function, add option parser for command "test-tool pkt-line unpack-sideband".

 * To remove newlines in sideband messages, we can use:
        $ test-tool pkt-line unpack-sideband --chomp-newline
 * To preserve newlines in sideband messages, we can use:
        $ test-tool pkt-line unpack-sideband --no-chomp-newline
 * To parse sideband messages using "demultiplex_sideband()" inside the
   function "packet_reader_read()", we can use:
        $ test-tool pkt-line unpack-sideband --reader-use-sideband

We also add new example sideband packets in send_split_sideband() and add several new test cases in t0070. Among these test cases, we pipe output of the "send-split-sideband" subcommand to the "unpack-sideband" subcommand. We found two issues:

 1. The two splitted sideband messages "Hello," and " world!\n" should
    be concatenated together. But when we turn on use_sideband field of
    reader to parse sideband messages, the first part of the splitted
    message ("Hello,") is lost.
 2. The newline characters in sideband 2 (progress info) and sideband 3
    (error message) should be preserved, but they are both trimmed.
Will fix the above two issues in subsequent commits.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/helper/test-pkt-line.c | 59 ++++++++++++++++++++++++++++++++++++----
 t/t0070-fundamental.sh   | 58 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+), 5 deletions(-)
Show changes to 2 files +112 −5

t/helper/test-pkt-line.c, t/t0070-fundamental.sh

diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index f4d134a145..6b306cf5ca 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -2,6 +2,7 @@
 #include "test-tool.h"
 #include "pkt-line.h"
 #include "write-or-die.h"
+#include "parse-options.h"
 
 static void pack_line(const char *line)
 {
@@ -64,12 +65,33 @@ static void unpack(void)
 	}
 }
 
-static void unpack_sideband(void)
+static void unpack_sideband(int argc, const char **argv)
 {
 	struct packet_reader reader;
-	packet_reader_init(&reader, 0, NULL, 0,
-			   PACKET_READ_GENTLE_ON_EOF |
-			   PACKET_READ_CHOMP_NEWLINE);
+	int options = PACKET_READ_GENTLE_ON_EOF;
+	int chomp_newline = 1;
+	int reader_use_sideband = 0;
+	const char *const unpack_sideband_usage[] = {
+		"test_tool unpack_sideband [options...]", NULL
+	};
+	struct option cmd_options[] = {
+		OPT_BOOL(0, "reader-use-sideband", &reader_use_sideband,
+			 "set use_sideband bit for packet reader (Default: off)"),
+		OPT_BOOL(0, "chomp-newline", &chomp_newline,
+			 "chomp newline in packet (Default: on)"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, "", cmd_options, unpack_sideband_usage,
+			     0);
+	if (argc > 0)
+		usage_msg_opt(_("too many arguments"), unpack_sideband_usage,
+			      cmd_options);
+
+	if (chomp_newline)
+		options |= PACKET_READ_CHOMP_NEWLINE;
+	packet_reader_init(&reader, 0, NULL, 0, options);
+	reader.use_sideband = reader_use_sideband;
 
 	while (packet_reader_read(&reader) != PACKET_READ_EOF) {
 		int band;
@@ -79,6 +101,17 @@ static void unpack_sideband(void)
 		case PACKET_READ_EOF:
 			break;
 		case PACKET_READ_NORMAL:
+			/*
+			 * When the "use_sideband" field of the reader is turned
+			 * on, sideband packets other than the payload have been
+			 * parsed and consumed in packet_reader_read(), and only
+			 * the payload arrives here.
+			 */
+			if (reader.use_sideband) {
+				write_or_die(1, reader.line, reader.pktlen - 1);
+				break;
+			}
+
 			band = reader.line[0] & 0xff;
 			if (band < 1 || band > 2)
 				continue; /* skip non-sideband packets */
@@ -97,15 +130,31 @@ static void unpack_sideband(void)
 
 static int send_split_sideband(void)
 {
+	const char *foo = "Foo.\n";
+	const char *bar = "Bar.\n";
 	const char *part1 = "Hello,";
 	const char *primary = "\001primary: regular output\n";
 	const char *part2 = " world!\n";
 
+	/* Each sideband message has a trailing newline character. */
+	send_sideband(1, 2, foo, strlen(foo), LARGE_PACKET_MAX);
+	send_sideband(1, 2, bar, strlen(bar), LARGE_PACKET_MAX);
+
+	/*
+	 * One sideband message is divided into part1 and part2
+	 * by the primary message.
+	 */
 	send_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);
 	packet_write(1, primary, strlen(primary));
 	send_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);
 	packet_response_end(1);
 
+	/*
+	 * We use unpack_sideband() to consume packets. A flush packet
+	 * is required to end parsing.
+	 */
+	packet_flush(1);
+
 	return 0;
 }
 
@@ -126,7 +175,7 @@ int cmd__pkt_line(int argc, const char **argv)
 	else if (!strcmp(argv[1], "unpack"))
 		unpack();
 	else if (!strcmp(argv[1], "unpack-sideband"))
-		unpack_sideband();
+		unpack_sideband(argc - 1, argv + 1);
 	else if (!strcmp(argv[1], "send-split-sideband"))
 		send_split_sideband();
 	else if (!strcmp(argv[1], "receive-sideband"))
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 574de34198..297a7f772e 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -53,4 +53,62 @@ test_expect_success 'missing sideband designator is reported' '
 	test_i18ngrep "missing sideband" err
 '
 
+test_expect_success 'unpack-sideband: --no-chomp-newline' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--no-chomp-newline <split-sideband >out 2>err &&
+	cat >expect-out <<-EOF &&
+		primary: regular output
+	EOF
+	cat >expect-err <<-EOF &&
+		Foo.
+		Bar.
+		Hello, world!
+	EOF
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
+test_expect_success 'unpack-sideband: --chomp-newline (default)' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--chomp-newline <split-sideband >out 2>err &&
+	printf "primary: regular output" >expect-out &&
+	printf "Foo.Bar.Hello, world!" >expect-err &&
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--reader-use-sideband \
+		--no-chomp-newline <split-sideband >out 2>err &&
+	cat >expect-out <<-EOF &&
+		primary: regular output
+	EOF
+	printf "remote: Foo.        \n"           >expect-err &&
+	printf "remote: Bar.        \n"          >>expect-err &&
+	printf "remote: Hello, world!        \n" >>expect-err &&
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
+	test_when_finished "rm -f expect-out expect-err" &&
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line unpack-sideband \
+		--reader-use-sideband \
+		--chomp-newline <split-sideband >out 2>err &&
+	printf "primary: regular output" >expect-out &&
+	printf "remote: Foo.        \n"           >expect-err &&
+	printf "remote: Bar.        \n"          >>expect-err &&
+	printf "remote: Hello, world!        \n" >>expect-err &&
+	test_cmp expect-out out &&
+	test_cmp expect-err err
+'
+
 test_done
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
Jiang Xin· Dec 17, 2023, 14:41 UTC · re: Jiang Xin · lore

[PATCH v4 2/3] pkt-line: memorize sideband fragment in reader

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When we turn on the "use_sideband" field of the packet_reader, "packet_reader_read()" will call the function "demultiplex_sideband()" to parse and consume sideband messages. Sideband fragment which does not end with "\r" or "\n" will be saved in the sixth parameter "scratch" and it can be reused and be concatenated when parsing another sideband message.

In "packet_reader_read()" function, the local variable "scratch" can only be reused by subsequent sideband messages. But if there is a payload message between two sideband fragments, the first fragment which is saved in the local variable "scratch" will be lost.

To solve this problem, we can add a new field "scratch" in packet_reader to memorize the sideband fragment across different calls of "packet_reader_read()".

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 pkt-line.c             | 5 ++---
 pkt-line.h             | 3 +++
 t/t0070-fundamental.sh | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)
Show changes to 3 files +6 −4

pkt-line.c, pkt-line.h, t/t0070-fundamental.sh

diff --git a/pkt-line.c b/pkt-line.c
index af83a19f4d..5943777a17 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -592,12 +592,11 @@ void packet_reader_init(struct packet_reader *reader, int fd,
 	reader->options = options;
 	reader->me = "git";
 	reader->hash_algo = &hash_algos[GIT_HASH_SHA1];
+	strbuf_init(&reader->scratch, 0);
 }
 
 enum packet_read_status packet_reader_read(struct packet_reader *reader)
 {
-	struct strbuf scratch = STRBUF_INIT;
-
 	if (reader->line_peeked) {
 		reader->line_peeked = 0;
 		return reader->status;
@@ -620,7 +619,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 			break;
 		if (demultiplex_sideband(reader->me, reader->status,
 					 reader->buffer, reader->pktlen, 1,
-					 &scratch, &sideband_type))
+					 &reader->scratch, &sideband_type))
 			break;
 	}
 
diff --git a/pkt-line.h b/pkt-line.h
index 954eec8719..be1010d34e 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -194,6 +194,9 @@ struct packet_reader {
 
 	/* hash algorithm in use */
 	const struct git_hash_algo *hash_algo;
+
+	/* hold temporary sideband message */
+	struct strbuf scratch;
 };
 
 /*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 297a7f772e..275edbf6e7 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -81,7 +81,7 @@ test_expect_success 'unpack-sideband: --chomp-newline (default)' '
 	test_cmp expect-err err
 '
 
-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
+test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '
 	test_when_finished "rm -f expect-out expect-err" &&
 	test-tool pkt-line send-split-sideband >split-sideband &&
 	test-tool pkt-line unpack-sideband \
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
Jiang Xin· Dec 17, 2023, 14:41 UTC · re: Jiang Xin · lore

[PATCH v4 3/3] pkt-line: do not chomp newlines for sideband messages

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When calling "packet_read_with_status()" to parse pkt-line encoded packets, we can turn on the flag "PACKET_READ_CHOMP_NEWLINE" to chomp newline character for each packet for better line matching. But when receiving data and progress information using sideband, we should turn off the flag "PACKET_READ_CHOMP_NEWLINE" to prevent mangling newline characters from data and progress information.

When both the server and the client support "sideband-all" capability, we have a dilemma that newline characters in negotiation packets should be removed, but the newline characters in the progress information should be left intact.

Add new flag "PACKET_READ_USE_SIDEBAND" for "packet_read_with_status()" to prevent mangling newline characters in sideband messages.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 pkt-line.c             | 31 +++++++++++++++++++++++++++++--
 pkt-line.h             |  1 +
 t/t0070-fundamental.sh |  2 +-
 3 files changed, 31 insertions(+), 3 deletions(-)
Show changes to 3 files +31 −3

pkt-line.c, pkt-line.h, t/t0070-fundamental.sh

diff --git a/pkt-line.c b/pkt-line.c
index 5943777a17..e9061e61a4 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -462,8 +462,32 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 	}
 
 	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
-	    len && buffer[len-1] == '\n')
-		len--;
+	    len && buffer[len-1] == '\n') {
+		if (options & PACKET_READ_USE_SIDEBAND) {
+			int band = *buffer & 0xff;
+			switch (band) {
+			case 1:
+				/* Chomp newline for payload */
+				len--;
+				break;
+			case 2:
+			case 3:
+				/*
+				 * Do not chomp newline for progress and error
+				 * message.
+				 */
+				break;
+			default:
+				/*
+				 * Bad sideband, let's leave it to
+				 * demultiplex_sideband() to catch this error.
+				 */
+				break;
+			}
+		} else {
+			len--;
+		}
+	}
 
 	buffer[len] = 0;
 	if (options & PACKET_READ_REDACT_URI_PATH &&
@@ -602,6 +626,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 		return reader->status;
 	}
 
+	if (reader->use_sideband)
+		reader->options |= PACKET_READ_USE_SIDEBAND;
+
 	/*
 	 * Consume all progress packets until a primary payload packet is
 	 * received
diff --git a/pkt-line.h b/pkt-line.h
index be1010d34e..a7ff2e2f18 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -85,6 +85,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
 #define PACKET_READ_REDACT_URI_PATH      (1u<<4)
+#define PACKET_READ_USE_SIDEBAND         (1u<<5)
 int packet_read(int fd, char *buffer, unsigned size, int options);
 
 /*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 275edbf6e7..0d2b7d8d93 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no
 	test_cmp expect-err err
 '
 
-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
+test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '
 	test_when_finished "rm -f expect-out expect-err" &&
 	test-tool pkt-line send-split-sideband >split-sideband &&
 	test-tool pkt-line unpack-sideband \
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

← back to recent threads