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

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

[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

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

[Junio C Hamano]

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

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

[Jonathan Tan]

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.

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

[Jiang Xin]

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

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

[Jiang Xin]

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(-)

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

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

[Jiang Xin]

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(-)

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

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

[Jiang Xin]

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(-)

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

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

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

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

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

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

[Oswald Buddenhagen]

>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

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

[Jiang Xin]

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;

Thanks.

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

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

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

[PATCH v3 0/3] Sideband demultiplexer fixes

[Jiang Xin]

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

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

[Jiang Xin]

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(-)

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

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

[Jiang Xin]

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(-)

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

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

[Jiang Xin]

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(-)

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

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

[Jiang Xin]

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

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

[Jiang Xin]

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(-)

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

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

[Jiang Xin]

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(-)

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

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

[Jiang Xin]

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(-)

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