{"thread":{"id":"60244","subject":"[PATCH] pkt-line: do not chomp EOL for sideband progress info","startedAt":"2023-09-19T07:20:07Z","lastAt":"2023-12-17T14:41:44Z","messageCount":19,"participants":["Jiang Xin","Junio C Hamano","Jonathan Tan","Oswald Buddenhagen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"482004","messageId":"20230919071956.14015-1-worldhello.net@gmail.com","threadId":"60244","inReplyTo":null,"subject":"[PATCH] pkt-line: do not chomp EOL for sideband progress info","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-19T07:19:56Z","receivedAt":"2023-09-19T07:20:07Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nIn the protocol negotiation stage, we need to turn on the flag\n\"PACKET_READ_CHOMP_NEWLINE\" to chomp EOL for each packet line from\nclient or server. But when receiving data and progress information\nusing sideband, we will turn off the flag \"PACKET_READ_CHOMP_NEWLINE\"\nto prevent mangling EOLs from data and progress information.\n\nWhen both the server and the client support \"sideband-all\" capability,\nwe have a dilemma that EOLs in negotiation packets should be trimmed,\nbut EOLs in progress infomation should be leaved as is.\n\nMove the logic of chomping EOLs from \"packet_read_with_status()\" to\n\"packet_reader_read()\" can resolve this dilemma.\n\nSigned-off-by: Jiang Xin \n---\n pkt-line.c | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex af83a19f4d..d6d08b6aa6 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -597,12 +597,18 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n enum packet_read_status packet_reader_read(struct packet_reader *reader)\n {\n \tstruct strbuf scratch = STRBUF_INIT;\n+\tint options = reader->options;\n \n \tif (reader->line_peeked) {\n \t\treader->line_peeked = 0;\n \t\treturn reader->status;\n \t}\n \n+\t/* Do not chomp newlines for sideband progress and error messages */\n+\tif (reader->use_sideband && options & PACKET_READ_CHOMP_NEWLINE) {\n+\t\toptions &= ~PACKET_READ_CHOMP_NEWLINE;\n+\t}\n+\n \t/*\n \t * Consume all progress packets until a primary payload packet is\n \t * received\n@@ -615,7 +621,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\t\t\t\t\t\t reader->buffer,\n \t\t\t\t\t\t\t reader->buffer_size,\n \t\t\t\t\t\t\t &reader->pktlen,\n-\t\t\t\t\t\t\t reader->options);\n+\t\t\t\t\t\t\t options);\n \t\tif (!reader->use_sideband)\n \t\t\tbreak;\n \t\tif (demultiplex_sideband(reader->me, reader->status,\n@@ -624,12 +630,19 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\t\tbreak;\n \t}\n \n-\tif (reader->status == PACKET_READ_NORMAL)\n+\tif (reader->status == PACKET_READ_NORMAL) {\n \t\t/* Skip the sideband designator if sideband is used */\n \t\treader->line = reader->use_sideband ?\n \t\t\treader->buffer + 1 : reader->buffer;\n-\telse\n+\n+\t\tif ((reader->options & PACKET_READ_CHOMP_NEWLINE) &&\n+\t\t reader->buffer[reader->pktlen - 1] == '\\n') {\n+\t\t\treader->buffer[reader->pktlen - 1] = 0;\n+\t\t\treader->pktlen--;\n+\t\t}\n+\t} else {\n \t\treader->line = NULL;\n+\t}\n \n \treturn reader->status;\n }\n-- \n2.40.1.49.g40e13c3520.dirty\n\n"},{"id":"482041","messageId":"xmqqled1eqkg.fsf@gitster.g","threadId":"60244","inReplyTo":"20230919071956.14015-1-worldhello.net@gmail.com","subject":"Re: [PATCH] pkt-line: do not chomp EOL for sideband progress info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-19T22:38:55Z","receivedAt":"2023-09-19T22:39:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n\nWho knows packet_reader interface well? Jonathan?\n\nThanks.\n\n\n> In the protocol negotiation stage, we need to turn on the flag\n> \"PACKET_READ_CHOMP_NEWLINE\" to chomp EOL for each packet line from\n> client or server. But when receiving data and progress information\n> using sideband, we will turn off the flag \"PACKET_READ_CHOMP_NEWLINE\"\n> to prevent mangling EOLs from data and progress information.\n>\n> When both the server and the client support \"sideband-all\" capability,\n> we have a dilemma that EOLs in negotiation packets should be trimmed,\n> but EOLs in progress infomation should be leaved as is.\n>\n> Move the logic of chomping EOLs from \"packet_read_with_status()\" to\n> \"packet_reader_read()\" can resolve this dilemma.\n>\n> Signed-off-by: Jiang Xin \n> ---\n> pkt-line.c | 19 ++++++++++++++++---\n> 1 file changed, 16 insertions(+), 3 deletions(-)\n>\n> diff --git a/pkt-line.c b/pkt-line.c\n> index af83a19f4d..d6d08b6aa6 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -597,12 +597,18 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n> enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> {\n> \tstruct strbuf scratch = STRBUF_INIT;\n> +\tint options = reader->options;\n> \n> \tif (reader->line_peeked) {\n> \t\treader->line_peeked = 0;\n> \t\treturn reader->status;\n> \t}\n> \n> +\t/* Do not chomp newlines for sideband progress and error messages */\n> +\tif (reader->use_sideband && options & PACKET_READ_CHOMP_NEWLINE) {\n> +\t\toptions &= ~PACKET_READ_CHOMP_NEWLINE;\n> +\t}\n> +\n> \t/*\n> \t * Consume all progress packets until a primary payload packet is\n> \t * received\n> @@ -615,7 +621,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> \t\t\t\t\t\t\t reader->buffer,\n> \t\t\t\t\t\t\t reader->buffer_size,\n> \t\t\t\t\t\t\t &reader->pktlen,\n> -\t\t\t\t\t\t\t reader->options);\n> +\t\t\t\t\t\t\t options);\n> \t\tif (!reader->use_sideband)\n> \t\t\tbreak;\n> \t\tif (demultiplex_sideband(reader->me, reader->status,\n> @@ -624,12 +630,19 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> \t\t\tbreak;\n> \t}\n> \n> -\tif (reader->status == PACKET_READ_NORMAL)\n> +\tif (reader->status == PACKET_READ_NORMAL) {\n> \t\t/* Skip the sideband designator if sideband is used */\n> \t\treader->line = reader->use_sideband ?\n> \t\t\treader->buffer + 1 : reader->buffer;\n> -\telse\n> +\n> +\t\tif ((reader->options & PACKET_READ_CHOMP_NEWLINE) &&\n> +\t\t reader->buffer[reader->pktlen - 1] == '\\n') {\n> +\t\t\treader->buffer[reader->pktlen - 1] = 0;\n> +\t\t\treader->pktlen--;\n> +\t\t}\n> +\t} else {\n> \t\treader->line = NULL;\n> +\t}\n> \n> \treturn reader->status;\n> }\n"},{"id":"482088","messageId":"20230920210832.2305886-1-jonathantanmy@google.com","threadId":"60244","inReplyTo":"20230919071956.14015-1-worldhello.net@gmail.com","subject":"Re: [PATCH] pkt-line: do not chomp EOL for sideband progress info","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-09-20T21:08:32Z","receivedAt":"2023-09-20T21:08:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jiang Xin writes:\n> From: Jiang Xin \n> \n> In the protocol negotiation stage, we need to turn on the flag\n> \"PACKET_READ_CHOMP_NEWLINE\" to chomp EOL for each packet line from\n> client or server. But when receiving data and progress information\n> using sideband, we will turn off the flag \"PACKET_READ_CHOMP_NEWLINE\"\n> to prevent mangling EOLs from data and progress information.\n> \n> When both the server and the client support \"sideband-all\" capability,\n> we have a dilemma that EOLs in negotiation packets should be trimmed,\n> but EOLs in progress infomation should be leaved as is.\n> \n> Move the logic of chomping EOLs from \"packet_read_with_status()\" to\n> \"packet_reader_read()\" can resolve this dilemma.\n> \n> Signed-off-by: Jiang Xin \n\nI think the summary is that when we use the struct packet_reader with\nsideband and newline chomping, we want the chomping to occur only on\nsideband 1, but the current code also chomps on sidebands 2 and 3 (3\nis for fatal errors so it doesn't matter as much, but for 2, it really\nmatters).\n\nThis makes sense to fix.\n\nAs for how this is fixed, one issue is that we now have 2 places in\nwhich newlines can be chomped (in packet_read_with_status() and with\nthis patch, packet_reader_read()). The issue is that we need to check\nthe sideband indicator before we chomp, and packet_read_with_status()\nonly knows how to chomp. So we either teach packet_read_with_status()\nhow to sideband, or tell packet_read_with_status() not to chomp and\nchomp it ourselves (like in this patch).\n\nOf the two, I would prefer it if packet_read_with_status() was taught\nhow to sideband - as it is, packet_read_with_status() is used 3 times\nin pkt-line.c and 1 time in remote-curl.c, and 2 of those times (in\npkt-line.c) are used with sideband. Doing this does not only solve the\nproblem here, but reduces code duplication.\n\nHaving said that, let me look at the code anyway.\n\n> @@ -597,12 +597,18 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n> enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> {\n> \tstruct strbuf scratch = STRBUF_INIT;\n> +\tint options = reader->options;\n> \n> \tif (reader->line_peeked) {\n> \t\treader->line_peeked = 0;\n> \t\treturn reader->status;\n> \t}\n> \n> +\t/* Do not chomp newlines for sideband progress and error messages */\n> +\tif (reader->use_sideband && options & PACKET_READ_CHOMP_NEWLINE) {\n> +\t\toptions &= ~PACKET_READ_CHOMP_NEWLINE;\n> +\t}\n> +\n\nThis needs a better explanation (than what's in the comment), I think.\nWhat this code is doing is disabling chomping because we have code that\nconditionally does it later.\n\n> \t/*\n> \t * Consume all progress packets until a primary payload packet is\n> \t * received\n> @@ -615,7 +621,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> \t\t\t\t\t\t\t reader->buffer,\n> \t\t\t\t\t\t\t reader->buffer_size,\n> \t\t\t\t\t\t\t &reader->pktlen,\n> -\t\t\t\t\t\t\t reader->options);\n> +\t\t\t\t\t\t\t options);\n\nOK, we're using our own custom options that may have\nPACKET_READ_CHOMP_NEWLINE unset.\n\n> @@ -624,12 +630,19 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> \t\t\tbreak;\n> \t}\n> \n> -\tif (reader->status == PACKET_READ_NORMAL)\n> +\tif (reader->status == PACKET_READ_NORMAL) {\n> \t\t/* Skip the sideband designator if sideband is used */\n> \t\treader->line = reader->use_sideband ?\n> \t\t\treader->buffer + 1 : reader->buffer;\n> -\telse\n> +\n> +\t\tif ((reader->options & PACKET_READ_CHOMP_NEWLINE) &&\n> +\t\t reader->buffer[reader->pktlen - 1] == '\\n') {\n> +\t\t\treader->buffer[reader->pktlen - 1] = 0;\n> +\t\t\treader->pktlen--;\n> +\t\t}\n\nWhen we reach here, we have skipped all sideband-2 pkt-lines, so\nunconditionally chomping it here is good. Might be better if there was\nalso a check that use_sideband is set, just for symmetry with the code\nnear the start of this function.\n\n\n"},{"id":"482236","messageId":"CANYiYbF+Xmk4rCNLMJe+i_CFafg8=QU5vbXWNUZbOVsDLTe5QQ@mail.gmail.com","threadId":"60244","inReplyTo":"20230920210832.2305886-1-jonathantanmy@google.com","subject":"Re: [PATCH] pkt-line: do not chomp EOL for sideband progress info","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-25T00:25:12Z","receivedAt":"2023-09-25T00:34:32Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Thu, Sep 21, 2023 at 5:08 AM Jonathan Tan wrote:\n>\n> Jiang Xin writes:\n> > From: Jiang Xin \n> >\n> > In the protocol negotiation stage, we need to turn on the flag\n> > \"PACKET_READ_CHOMP_NEWLINE\" to chomp EOL for each packet line from\n> > client or server. But when receiving data and progress information\n> > using sideband, we will turn off the flag \"PACKET_READ_CHOMP_NEWLINE\"\n> > to prevent mangling EOLs from data and progress information.\n> >\n> > When both the server and the client support \"sideband-all\" capability,\n> > we have a dilemma that EOLs in negotiation packets should be trimmed,\n> > but EOLs in progress infomation should be leaved as is.\n> >\n> > Move the logic of chomping EOLs from \"packet_read_with_status()\" to\n> > \"packet_reader_read()\" can resolve this dilemma.\n> >\n> > Signed-off-by: Jiang Xin \n>\n> I think the summary is that when we use the struct packet_reader with\n> sideband and newline chomping, we want the chomping to occur only on\n> sideband 1, but the current code also chomps on sidebands 2 and 3 (3\n> is for fatal errors so it doesn't matter as much, but for 2, it really\n> matters).\n>\n> This makes sense to fix.\n>\n> As for how this is fixed, one issue is that we now have 2 places in\n> which newlines can be chomped (in packet_read_with_status() and with\n> this patch, packet_reader_read()). The issue is that we need to check\n> the sideband indicator before we chomp, and packet_read_with_status()\n> only knows how to chomp. So we either teach packet_read_with_status()\n> how to sideband, or tell packet_read_with_status() not to chomp and\n> chomp it ourselves (like in this patch).\n>\n> Of the two, I would prefer it if packet_read_with_status() was taught\n> how to sideband - as it is, packet_read_with_status() is used 3 times\n> in pkt-line.c and 1 time in remote-curl.c, and 2 of those times (in\n> pkt-line.c) are used with sideband. Doing this does not only solve the\n> problem here, but reduces code duplication.\n\nYes, there are two places we can choose to fix. My first instinct is\nthat changes on packet_reader_read will have less impact. I will new\nimplementation in next reroll.\n\n> > @@ -624,12 +630,19 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> > break;\n> > }\n> >\n> > - if (reader->status == PACKET_READ_NORMAL)\n> > + if (reader->status == PACKET_READ_NORMAL) {\n> > /* Skip the sideband designator if sideband is used */\n> > reader->line = reader->use_sideband ?\n> > reader->buffer + 1 : reader->buffer;\n> > - else\n> > +\n> > + if ((reader->options & PACKET_READ_CHOMP_NEWLINE) &&\n> > + reader->buffer[reader->pktlen - 1] == '\\n') {\n> > + reader->buffer[reader->pktlen - 1] = 0;\n> > + reader->pktlen--;\n> > + }\n>\n> When we reach here, we have skipped all sideband-2 pkt-lines, so\n> unconditionally chomping it here is good. Might be better if there was\n> also a check that use_sideband is set, just for symmetry with the code\n> near the start of this function.\n>\n\nYou find my bug. Without checking the use_sideband flag, two\nconsecutive EOLwill be removed.\n\nBTW, the new reroll is not coming as fast as I planned, because when I\nadding new test cases, I find another issue in pkt-line. I will fix\nthese two issues in this series.\n\n--\nJiang Xin\n"},{"id":"482282","messageId":"20230925154144.15213-1-worldhello.net@gmail.com","threadId":"60244","inReplyTo":"CANYiYbF+Xmk4rCNLMJe+i_CFafg8=QU5vbXWNUZbOVsDLTe5QQ@mail.gmail.com","subject":"[PATCH v2 1/3] test-pkt-line: add option parser for unpack-sideband","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-25T15:41:42Z","receivedAt":"2023-09-25T15:42:53Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWe can use the test helper program \"test-tool pkt-line\" to test pkt-line\nrelated functions. E.g.:\n\n * Use \"test-tool pkt-line send-split-sideband\" to generate sideband\n messages.\n\n * We can pipe these generated sideband messages to command \"test-tool\n pkt-line unpack-sideband\" to test packet_reader_read() function.\n\nIn order to make a complete test of the packet_reader_read() function,\nadd option parser for command \"test-tool pkt-line unpack-sideband\".\n\nTo remove newlines in sideband messages, we can use:\n\n $ test-tool pkt-line unpack-sideband --chomp-newline\n\nTo preserve newlines in sideband messages, we can use:\n\n $ test-tool pkt-line unpack-sideband --no-chomp-newline\n\nTo parse sideband messages using \"demultiplex_sideband()\" inside the\nfunction \"packet_reader_read()\", we can use:\n\n $ test-tool pkt-line unpack-sideband --reader-use-sideband\n\nAdd several new test cases in t0070. Among these test cases, we pipe\noutput of the \"send-split-sideband\" subcommand to the \"unpack-sideband\"\nsubcommand. We found two issues:\n\n 1. The two splitted sideband messages \"Hello,\" and \" world!\\n\" should\n be concatenated together. But when we enabled the function\n \"demultiplex_sideband()\" to parse sideband messages, the first part\n of the splitted message (\"Hello,\") is lost.\n\n 2. The newline characters in sideband 2 (progress info) and sideband 3\n (error message) should be preserved, but they are also trimmed.\n\nWill fix the above two issues in subsequent commits.\n\nSigned-off-by: Jiang Xin \n---\n t/helper/test-pkt-line.c | 58 ++++++++++++++++++++++++++++++++++++----\n t/t0070-fundamental.sh | 58 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 111 insertions(+), 5 deletions(-)\n\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex f4d134a145..9aa35f7861 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -2,6 +2,7 @@\n #include \"test-tool.h\"\n #include \"pkt-line.h\"\n #include \"write-or-die.h\"\n+#include \"parse-options.h\"\n \n static void pack_line(const char *line)\n {\n@@ -64,12 +65,33 @@ static void unpack(void)\n \t}\n }\n \n-static void unpack_sideband(void)\n+static void unpack_sideband(int argc, const char **argv)\n {\n \tstruct packet_reader reader;\n-\tpacket_reader_init(&reader, 0, NULL, 0,\n-\t\t\t PACKET_READ_GENTLE_ON_EOF |\n-\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\tint options = PACKET_READ_GENTLE_ON_EOF;\n+\tint chomp_newline = 1;\n+\tint reader_use_sideband = 0;\n+\tconst char *const unpack_sideband_usage[] = {\n+\t\t\"test_tool unpack_sideband [options...]\", NULL\n+\t};\n+\tstruct option cmd_options[] = {\n+\t\tOPT_BOOL(0, \"reader-use-sideband\", &reader_use_sideband,\n+\t\t\t \"set use_sideband bit for packet reader (Default: off)\"),\n+\t\tOPT_BOOL(0, \"chomp-newline\", &chomp_newline,\n+\t\t\t \"chomp newline in packet (Default: on)\"),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, \"\", cmd_options, unpack_sideband_usage,\n+\t\t\t 0);\n+\tif (argc > 0)\n+\t\tusage_msg_opt(_(\"too many arguments\"), unpack_sideband_usage,\n+\t\t\t cmd_options);\n+\n+\tif (chomp_newline)\n+\t\toptions |= PACKET_READ_CHOMP_NEWLINE;\n+\tpacket_reader_init(&reader, 0, NULL, 0, options);\n+\treader.use_sideband = reader_use_sideband;\n \n \twhile (packet_reader_read(&reader) != PACKET_READ_EOF) {\n \t\tint band;\n@@ -79,6 +101,16 @@ static void unpack_sideband(void)\n \t\tcase PACKET_READ_EOF:\n \t\t\tbreak;\n \t\tcase PACKET_READ_NORMAL:\n+\t\t\t/*\n+\t\t\t * When the \"use_sideband\" field of the reader is turned\n+\t\t\t * on, sideband packets other than the payload have been\n+\t\t\t * parsed and consumed.\n+\t\t\t */\n+\t\t\tif (reader.use_sideband) {\n+\t\t\t\twrite_or_die(1, reader.line, reader.pktlen - 1);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n \t\t\tband = reader.line[0] & 0xff;\n \t\t\tif (band < 1 || band > 2)\n \t\t\t\tcontinue; /* skip non-sideband packets */\n@@ -97,15 +129,31 @@ static void unpack_sideband(void)\n \n static int send_split_sideband(void)\n {\n+\tconst char *foo = \"Foo.\\n\";\n+\tconst char *bar = \"Bar.\\n\";\n \tconst char *part1 = \"Hello,\";\n \tconst char *primary = \"\\001primary: regular output\\n\";\n \tconst char *part2 = \" world!\\n\";\n \n+\t/* Each sideband message has a trailing newline character. */\n+\tsend_sideband(1, 2, foo, strlen(foo), LARGE_PACKET_MAX);\n+\tsend_sideband(1, 2, bar, strlen(bar), LARGE_PACKET_MAX);\n+\n+\t/*\n+\t * One sideband message is divided into part1 and part2\n+\t * by the primary message.\n+\t */\n \tsend_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);\n \tpacket_write(1, primary, strlen(primary));\n \tsend_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);\n \tpacket_response_end(1);\n \n+\t/*\n+\t * The unpack_sideband() function above requires a flush\n+\t * packet to end parsing.\n+\t */\n+\tpacket_flush(1);\n+\n \treturn 0;\n }\n \n@@ -126,7 +174,7 @@ int cmd__pkt_line(int argc, const char **argv)\n \telse if (!strcmp(argv[1], \"unpack\"))\n \t\tunpack();\n \telse if (!strcmp(argv[1], \"unpack-sideband\"))\n-\t\tunpack_sideband();\n+\t\tunpack_sideband(argc - 1, argv + 1);\n \telse if (!strcmp(argv[1], \"send-split-sideband\"))\n \t\tsend_split_sideband();\n \telse if (!strcmp(argv[1], \"receive-sideband\"))\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 574de34198..1053913d2d 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -53,4 +53,62 @@ test_expect_success 'missing sideband designator is reported' '\n \ttest_i18ngrep \"missing sideband\" err\n '\n \n+test_expect_success 'unpack-sideband: --no-chomp-newline' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--no-chomp-newline out 2>err &&\n+\tcat >expect-out <<-EOF &&\n+\t\tprimary: regular output\n+\tEOF\n+\tcat >expect-err <<-EOF &&\n+\t\tFoo.\n+\t\tBar.\n+\t\tHello, world!\n+\tEOF\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n+test_expect_success 'unpack-sideband: --chomp-newline (default)' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--chomp-newline out 2>err &&\n+\tprintf \"primary: regular output\" >expect-out &&\n+\tprintf \"Foo.Bar.Hello, world!\" >expect-err &&\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n+test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--reader-use-sideband \\\n+\t\t--no-chomp-newline out 2>err &&\n+\tcat >expect-out <<-EOF &&\n+\t\tprimary: regular output\n+\tEOF\n+\tprintf \"remote: Foo. \\n\" >expect-err &&\n+\tprintf \"remote: Bar. \\n\" >>expect-err &&\n+\tprintf \"remote: Hello, world! \\n\" >>expect-err &&\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n+test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--reader-use-sideband \\\n+\t\t--chomp-newline out 2>err &&\n+\tprintf \"primary: regular output\" >expect-out &&\n+\tprintf \"remote: Foo. \\n\" >expect-err &&\n+\tprintf \"remote: Bar. \\n\" >>expect-err &&\n+\tprintf \"remote: Hello, world! \\n\" >>expect-err &&\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n test_done\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482283","messageId":"20230925154144.15213-2-worldhello.net@gmail.com","threadId":"60244","inReplyTo":"CANYiYbF+Xmk4rCNLMJe+i_CFafg8=QU5vbXWNUZbOVsDLTe5QQ@mail.gmail.com","subject":"[PATCH v2 2/3] pkt-line: memorize sideband fragment in reader","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-25T15:41:43Z","receivedAt":"2023-09-25T15:42:56Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen we turn on the \"use_sideband\" field of the packet_reader,\n\"packet_reader_read()\" will call the function \"demultiplex_sideband()\"\nto parse and consume sideband messages. Sideband fragment which does not\nend with \"\\r\" or \"\\n\" will be saved in the sixth parameter \"scratch\"\nand it can be reused and be concatenated when parsing another sideband\nmessage.\n\nIn \"packet_reader_read()\" function, the local variable \"scratch\" can\nonly be reused by subsequent sideband messages. But if there is a\npayload message between two sideband fragments, the first fragment\nwhich is saved in the local variable \"scratch\" will be lost.\n\nTo solve this problem, we can add a new field \"scratch\" in\npacket_reader to memorize the sideband fragment across different calls\nof \"packet_reader_read()\".\n\nSigned-off-by: Jiang Xin \n---\n pkt-line.c | 5 ++---\n pkt-line.h | 3 +++\n t/t0070-fundamental.sh | 2 +-\n 3 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex af83a19f4d..5943777a17 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -592,12 +592,11 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n \treader->options = options;\n \treader->me = \"git\";\n \treader->hash_algo = &hash_algos[GIT_HASH_SHA1];\n+\tstrbuf_init(&reader->scratch, 0);\n }\n \n enum packet_read_status packet_reader_read(struct packet_reader *reader)\n {\n-\tstruct strbuf scratch = STRBUF_INIT;\n-\n \tif (reader->line_peeked) {\n \t\treader->line_peeked = 0;\n \t\treturn reader->status;\n@@ -620,7 +619,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\t\tbreak;\n \t\tif (demultiplex_sideband(reader->me, reader->status,\n \t\t\t\t\t reader->buffer, reader->pktlen, 1,\n-\t\t\t\t\t &scratch, &sideband_type))\n+\t\t\t\t\t &reader->scratch, &sideband_type))\n \t\t\tbreak;\n \t}\n \ndiff --git a/pkt-line.h b/pkt-line.h\nindex 954eec8719..be1010d34e 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -194,6 +194,9 @@ struct packet_reader {\n \n \t/* hash algorithm in use */\n \tconst struct git_hash_algo *hash_algo;\n+\n+\t/* hold temporary sideband message */\n+\tstruct strbuf scratch;\n };\n \n /*\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 1053913d2d..a927c665d6 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -81,7 +81,7 @@ test_expect_success 'unpack-sideband: --chomp-newline (default)' '\n \ttest_cmp expect-err err\n '\n \n-test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '\n+test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newline' '\n \ttest_when_finished \"rm -f expect-out expect-err\" &&\n \ttest-tool pkt-line send-split-sideband >split-sideband &&\n \ttest-tool pkt-line unpack-sideband \\\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482284","messageId":"20230925154144.15213-3-worldhello.net@gmail.com","threadId":"60244","inReplyTo":"CANYiYbF+Xmk4rCNLMJe+i_CFafg8=QU5vbXWNUZbOVsDLTe5QQ@mail.gmail.com","subject":"[PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-25T15:41:44Z","receivedAt":"2023-09-25T15:42:58Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen calling \"packet_read_with_status()\" to parse pkt-line encoded\npackets, we can turn on the flag \"PACKET_READ_CHOMP_NEWLINE\" to chomp\nnewline character for each packet for better line matching. But when\nreceiving data and progress information using sideband, we should turn\noff the flag \"PACKET_READ_CHOMP_NEWLINE\" to prevent mangling newline\ncharacters from data and progress information.\n\nWhen both the server and the client support \"sideband-all\" capability,\nwe have a dilemma that newline characters in negotiation packets should\nbe removed, but the newline characters in the progress information\nshould be left intact.\n\nAdd new flag \"PACKET_READ_USE_SIDEBAND\" for \"packet_read_with_status()\"\nto prevent mangling newline characters in sideband messages.\n\nHelped-by: Jonathan Tan \nSigned-off-by: Jiang Xin \n---\n pkt-line.c | 32 ++++++++++++++++++++++++++++++--\n pkt-line.h | 1 +\n t/t0070-fundamental.sh | 2 +-\n 3 files changed, 32 insertions(+), 3 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 5943777a17..865ad19484 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t}\n \n \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n-\t len && buffer[len-1] == '\\n')\n-\t\tlen--;\n+\t len && buffer[len-1] == '\\n') {\n+\t\tif (options & PACKET_READ_USE_SIDEBAND) {\n+\t\t\tint band = *buffer & 0xff;\n+\t\t\tswitch (band) {\n+\t\t\tcase 1:\n+\t\t\t\t/* Chomp newline for payload */\n+\t\t\t\tlen--;\n+\t\t\t\tbreak;\n+\t\t\tcase 2:\n+\t\t\t\t/* fallthrough */\n+\t\t\tcase 3:\n+\t\t\t\t/*\n+\t\t\t\t * Do not chomp newline for progress and error\n+\t\t\t\t * message.\n+\t\t\t\t */\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\t/*\n+\t\t\t\t * Bad sideband, let's leave it to\n+\t\t\t\t * demultiplex_sideband() to catch this error.\n+\t\t\t\t */\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tlen--;\n+\t\t}\n+\t}\n \n \tbuffer[len] = 0;\n \tif (options & PACKET_READ_REDACT_URI_PATH &&\n@@ -602,6 +627,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\treturn reader->status;\n \t}\n \n+\tif (reader->use_sideband)\n+\t\treader->options |= PACKET_READ_USE_SIDEBAND;\n+\n \t/*\n \t * Consume all progress packets until a primary payload packet is\n \t * received\ndiff --git a/pkt-line.h b/pkt-line.h\nindex be1010d34e..a7ff2e2f18 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -85,6 +85,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n #define PACKET_READ_REDACT_URI_PATH (1u<<4)\n+#define PACKET_READ_USE_SIDEBAND (1u<<5)\n int packet_read(int fd, char *buffer, unsigned size, int options);\n \n /*\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex a927c665d6..138c2becc1 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli\n \ttest_cmp expect-err err\n '\n \n-test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n+test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n \ttest_when_finished \"rm -f expect-out expect-err\" &&\n \ttest-tool pkt-line send-split-sideband >split-sideband &&\n \ttest-tool pkt-line unpack-sideband \\\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482307","messageId":"xmqqa5t9rkft.fsf@gitster.g","threadId":"60244","inReplyTo":"20230925154144.15213-3-worldhello.net@gmail.com","subject":"Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-25T21:51:02Z","receivedAt":"2023-09-25T21:51:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> Add new flag \"PACKET_READ_USE_SIDEBAND\" for \"packet_read_with_status()\"\n> to prevent mangling newline characters in sideband messages.\n>\n> Helped-by: Jonathan Tan \n> Signed-off-by: Jiang Xin \n> ---\n> pkt-line.c | 32 ++++++++++++++++++++++++++++++--\n> pkt-line.h | 1 +\n> t/t0070-fundamental.sh | 2 +-\n> 3 files changed, 32 insertions(+), 3 deletions(-)\n>\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 5943777a17..865ad19484 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \t}\n> \n> \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n> -\t len && buffer[len-1] == '\\n')\n> -\t\tlen--;\n> +\t len && buffer[len-1] == '\\n') {\n> +\t\tif (options & PACKET_READ_USE_SIDEBAND) {\n> +\t\t\tint band = *buffer & 0xff;\n> +\t\t\tswitch (band) {\n> +\t\t\tcase 1:\n> +\t\t\t\t/* Chomp newline for payload */\n> +\t\t\t\tlen--;\n> +\t\t\t\tbreak;\n> +\t\t\tcase 2:\n> +\t\t\t\t/* fallthrough */\n> +\t\t\tcase 3:\n> +\t\t\t\t/*\n> +\t\t\t\t * Do not chomp newline for progress and error\n> +\t\t\t\t * message.\n> +\t\t\t\t */\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\t/*\n> +\t\t\t\t * Bad sideband, let's leave it to\n> +\t\t\t\t * demultiplex_sideband() to catch this error.\n> +\t\t\t\t */\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tlen--;\n> +\t\t}\n> +\t}\n\nThat's a mouthful and we could shorten it a lot, but is very easy to\nfollow the logic ;-)\n\n> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\n> index a927c665d6..138c2becc1 100755\n> --- a/t/t0070-fundamental.sh\n> +++ b/t/t0070-fundamental.sh\n> @@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli\n> \ttest_cmp expect-err err\n> '\n> \n> -test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n> +test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n> \ttest_when_finished \"rm -f expect-out expect-err\" &&\n> \ttest-tool pkt-line send-split-sideband >split-sideband &&\n> \ttest-tool pkt-line unpack-sideband \\\n\nWe cannot quite see what got fixed that is outside the postimage,\nbut it is nice that we have one fewer test_expect_failure in the\nend.\n\nWill queue. Thanks.\n"},{"id":"482332","messageId":"ZRKax7Me5uIHKHoC@ugly","threadId":"60244","inReplyTo":"xmqqa5t9rkft.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-26T08:48:07Z","receivedAt":"2023-09-26T08:48:14Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":">Jiang Xin writes:\n>\n>> +++ b/pkt-line.c\n>> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n>> \t}\n>> +\t\t\tcase 2:\n>> +\t\t\t\t/* fallthrough */\n>> +\t\t\tcase 3:\n>\nwhile not entirely unprecedented, it's unnecessary and even \ncounter-productive to annotate directly adjacent cases with fallthrough.\n\nregards\n"},{"id":"482640","messageId":"CANYiYbHp-3YuSPHnR8gjS40UJLrJV5FPzqd_BtjyR8TAALhfRQ@mail.gmail.com","threadId":"60244","inReplyTo":"ZRKax7Me5uIHKHoC@ugly","subject":"Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T13:02:25Z","receivedAt":"2023-10-04T13:02:42Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Tue, Sep 26, 2023 at 4:48 PM Oswald Buddenhagen\n wrote:\n>\n> >Jiang Xin writes:\n> >\n> >> +++ b/pkt-line.c\n> >> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> >> }\n> >> + case 2:\n> >> + /* fallthrough */\n> >> + case 3:\n> >\n> while not entirely unprecedented, it's unnecessary and even\n> counter-productive to annotate directly adjacent cases with fallthrough.\n\nI see in \"blame.c\" there are directly adjacent cases like below. I\nwill remove the fallthrough statement.\n\n case 'A':\n case 'T':\n /* Did not exist in parent, or type changed */\n break;\n\nThanks.\n"},{"id":"482643","messageId":"cover.1696425168.git.zhiyou.jx@alibaba-inc.com","threadId":"60244","inReplyTo":"ZRKax7Me5uIHKHoC@ugly","subject":"[PATCH v3 0/3] Sideband demultiplexer fixes","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T13:18:11Z","receivedAt":"2023-10-04T13:18:22Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nSideband demultiplexer fixes.\n\nRange-diff v2...v3\n\n1: fd6d61893d ! 1: 68ac3ea711 pkt-line: do not chomp newlines for sideband messages\n @@ Commit message\n to prevent mangling newline characters in sideband messages.\n \n Helped-by: Jonathan Tan \n + Helped-by: Oswald Buddenhagen \n Signed-off-by: Jiang Xin \n \n ## pkt-line.c ##\n @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b\n +\t\t\t\tlen--;\n +\t\t\t\tbreak;\n +\t\t\tcase 2:\n -+\t\t\t\t/* fallthrough */\n +\t\t\tcase 3:\n +\t\t\t\t/*\n +\t\t\t\t * Do not chomp newline for progress and error\n\n---\n\nJiang Xin (3):\n test-pkt-line: add option parser for unpack-sideband\n pkt-line: memorize sideband fragment in reader\n pkt-line: do not chomp newlines for sideband messages\n\n pkt-line.c | 36 +++++++++++++++++++++----\n pkt-line.h | 4 +++\n t/helper/test-pkt-line.c | 58 ++++++++++++++++++++++++++++++++++++----\n t/t0070-fundamental.sh | 58 ++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 146 insertions(+), 10 deletions(-)\n\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482644","messageId":"e991ba2099aaa12ed01382bfaca54a3a213919c3.1696425168.git.zhiyou.jx@alibaba-inc.com","threadId":"60244","inReplyTo":"cover.1696425168.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 2/3] pkt-line: memorize sideband fragment in reader","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T13:18:13Z","receivedAt":"2023-10-04T13:18:28Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen we turn on the \"use_sideband\" field of the packet_reader,\n\"packet_reader_read()\" will call the function \"demultiplex_sideband()\"\nto parse and consume sideband messages. Sideband fragment which does not\nend with \"\\r\" or \"\\n\" will be saved in the sixth parameter \"scratch\"\nand it can be reused and be concatenated when parsing another sideband\nmessage.\n\nIn \"packet_reader_read()\" function, the local variable \"scratch\" can\nonly be reused by subsequent sideband messages. But if there is a\npayload message between two sideband fragments, the first fragment\nwhich is saved in the local variable \"scratch\" will be lost.\n\nTo solve this problem, we can add a new field \"scratch\" in\npacket_reader to memorize the sideband fragment across different calls\nof \"packet_reader_read()\".\n\nSigned-off-by: Jiang Xin \n---\n pkt-line.c | 5 ++---\n pkt-line.h | 3 +++\n t/t0070-fundamental.sh | 2 +-\n 3 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex af83a19f4d..5943777a17 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -592,12 +592,11 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n \treader->options = options;\n \treader->me = \"git\";\n \treader->hash_algo = &hash_algos[GIT_HASH_SHA1];\n+\tstrbuf_init(&reader->scratch, 0);\n }\n \n enum packet_read_status packet_reader_read(struct packet_reader *reader)\n {\n-\tstruct strbuf scratch = STRBUF_INIT;\n-\n \tif (reader->line_peeked) {\n \t\treader->line_peeked = 0;\n \t\treturn reader->status;\n@@ -620,7 +619,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\t\tbreak;\n \t\tif (demultiplex_sideband(reader->me, reader->status,\n \t\t\t\t\t reader->buffer, reader->pktlen, 1,\n-\t\t\t\t\t &scratch, &sideband_type))\n+\t\t\t\t\t &reader->scratch, &sideband_type))\n \t\t\tbreak;\n \t}\n \ndiff --git a/pkt-line.h b/pkt-line.h\nindex 954eec8719..be1010d34e 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -194,6 +194,9 @@ struct packet_reader {\n \n \t/* hash algorithm in use */\n \tconst struct git_hash_algo *hash_algo;\n+\n+\t/* hold temporary sideband message */\n+\tstruct strbuf scratch;\n };\n \n /*\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 1053913d2d..a927c665d6 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -81,7 +81,7 @@ test_expect_success 'unpack-sideband: --chomp-newline (default)' '\n \ttest_cmp expect-err err\n '\n \n-test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '\n+test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newline' '\n \ttest_when_finished \"rm -f expect-out expect-err\" &&\n \ttest-tool pkt-line send-split-sideband >split-sideband &&\n \ttest-tool pkt-line unpack-sideband \\\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482645","messageId":"17d88ecab6228fbf56f934bc09c921268d202874.1696425168.git.zhiyou.jx@alibaba-inc.com","threadId":"60244","inReplyTo":"cover.1696425168.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 1/3] test-pkt-line: add option parser for unpack-sideband","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T13:18:12Z","receivedAt":"2023-10-04T13:18:30Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWe can use the test helper program \"test-tool pkt-line\" to test pkt-line\nrelated functions. E.g.:\n\n * Use \"test-tool pkt-line send-split-sideband\" to generate sideband\n messages.\n\n * We can pipe these generated sideband messages to command \"test-tool\n pkt-line unpack-sideband\" to test packet_reader_read() function.\n\nIn order to make a complete test of the packet_reader_read() function,\nadd option parser for command \"test-tool pkt-line unpack-sideband\".\n\nTo remove newlines in sideband messages, we can use:\n\n $ test-tool pkt-line unpack-sideband --chomp-newline\n\nTo preserve newlines in sideband messages, we can use:\n\n $ test-tool pkt-line unpack-sideband --no-chomp-newline\n\nTo parse sideband messages using \"demultiplex_sideband()\" inside the\nfunction \"packet_reader_read()\", we can use:\n\n $ test-tool pkt-line unpack-sideband --reader-use-sideband\n\nAdd several new test cases in t0070. Among these test cases, we pipe\noutput of the \"send-split-sideband\" subcommand to the \"unpack-sideband\"\nsubcommand. We found two issues:\n\n 1. The two splitted sideband messages \"Hello,\" and \" world!\\n\" should\n be concatenated together. But when we enabled the function\n \"demultiplex_sideband()\" to parse sideband messages, the first part\n of the splitted message (\"Hello,\") is lost.\n\n 2. The newline characters in sideband 2 (progress info) and sideband 3\n (error message) should be preserved, but they are also trimmed.\n\nWill fix the above two issues in subsequent commits.\n\nSigned-off-by: Jiang Xin \n---\n t/helper/test-pkt-line.c | 58 ++++++++++++++++++++++++++++++++++++----\n t/t0070-fundamental.sh | 58 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 111 insertions(+), 5 deletions(-)\n\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex f4d134a145..9aa35f7861 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -2,6 +2,7 @@\n #include \"test-tool.h\"\n #include \"pkt-line.h\"\n #include \"write-or-die.h\"\n+#include \"parse-options.h\"\n \n static void pack_line(const char *line)\n {\n@@ -64,12 +65,33 @@ static void unpack(void)\n \t}\n }\n \n-static void unpack_sideband(void)\n+static void unpack_sideband(int argc, const char **argv)\n {\n \tstruct packet_reader reader;\n-\tpacket_reader_init(&reader, 0, NULL, 0,\n-\t\t\t PACKET_READ_GENTLE_ON_EOF |\n-\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\tint options = PACKET_READ_GENTLE_ON_EOF;\n+\tint chomp_newline = 1;\n+\tint reader_use_sideband = 0;\n+\tconst char *const unpack_sideband_usage[] = {\n+\t\t\"test_tool unpack_sideband [options...]\", NULL\n+\t};\n+\tstruct option cmd_options[] = {\n+\t\tOPT_BOOL(0, \"reader-use-sideband\", &reader_use_sideband,\n+\t\t\t \"set use_sideband bit for packet reader (Default: off)\"),\n+\t\tOPT_BOOL(0, \"chomp-newline\", &chomp_newline,\n+\t\t\t \"chomp newline in packet (Default: on)\"),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, \"\", cmd_options, unpack_sideband_usage,\n+\t\t\t 0);\n+\tif (argc > 0)\n+\t\tusage_msg_opt(_(\"too many arguments\"), unpack_sideband_usage,\n+\t\t\t cmd_options);\n+\n+\tif (chomp_newline)\n+\t\toptions |= PACKET_READ_CHOMP_NEWLINE;\n+\tpacket_reader_init(&reader, 0, NULL, 0, options);\n+\treader.use_sideband = reader_use_sideband;\n \n \twhile (packet_reader_read(&reader) != PACKET_READ_EOF) {\n \t\tint band;\n@@ -79,6 +101,16 @@ static void unpack_sideband(void)\n \t\tcase PACKET_READ_EOF:\n \t\t\tbreak;\n \t\tcase PACKET_READ_NORMAL:\n+\t\t\t/*\n+\t\t\t * When the \"use_sideband\" field of the reader is turned\n+\t\t\t * on, sideband packets other than the payload have been\n+\t\t\t * parsed and consumed.\n+\t\t\t */\n+\t\t\tif (reader.use_sideband) {\n+\t\t\t\twrite_or_die(1, reader.line, reader.pktlen - 1);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n \t\t\tband = reader.line[0] & 0xff;\n \t\t\tif (band < 1 || band > 2)\n \t\t\t\tcontinue; /* skip non-sideband packets */\n@@ -97,15 +129,31 @@ static void unpack_sideband(void)\n \n static int send_split_sideband(void)\n {\n+\tconst char *foo = \"Foo.\\n\";\n+\tconst char *bar = \"Bar.\\n\";\n \tconst char *part1 = \"Hello,\";\n \tconst char *primary = \"\\001primary: regular output\\n\";\n \tconst char *part2 = \" world!\\n\";\n \n+\t/* Each sideband message has a trailing newline character. */\n+\tsend_sideband(1, 2, foo, strlen(foo), LARGE_PACKET_MAX);\n+\tsend_sideband(1, 2, bar, strlen(bar), LARGE_PACKET_MAX);\n+\n+\t/*\n+\t * One sideband message is divided into part1 and part2\n+\t * by the primary message.\n+\t */\n \tsend_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);\n \tpacket_write(1, primary, strlen(primary));\n \tsend_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);\n \tpacket_response_end(1);\n \n+\t/*\n+\t * The unpack_sideband() function above requires a flush\n+\t * packet to end parsing.\n+\t */\n+\tpacket_flush(1);\n+\n \treturn 0;\n }\n \n@@ -126,7 +174,7 @@ int cmd__pkt_line(int argc, const char **argv)\n \telse if (!strcmp(argv[1], \"unpack\"))\n \t\tunpack();\n \telse if (!strcmp(argv[1], \"unpack-sideband\"))\n-\t\tunpack_sideband();\n+\t\tunpack_sideband(argc - 1, argv + 1);\n \telse if (!strcmp(argv[1], \"send-split-sideband\"))\n \t\tsend_split_sideband();\n \telse if (!strcmp(argv[1], \"receive-sideband\"))\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 574de34198..1053913d2d 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -53,4 +53,62 @@ test_expect_success 'missing sideband designator is reported' '\n \ttest_i18ngrep \"missing sideband\" err\n '\n \n+test_expect_success 'unpack-sideband: --no-chomp-newline' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--no-chomp-newline out 2>err &&\n+\tcat >expect-out <<-EOF &&\n+\t\tprimary: regular output\n+\tEOF\n+\tcat >expect-err <<-EOF &&\n+\t\tFoo.\n+\t\tBar.\n+\t\tHello, world!\n+\tEOF\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n+test_expect_success 'unpack-sideband: --chomp-newline (default)' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--chomp-newline out 2>err &&\n+\tprintf \"primary: regular output\" >expect-out &&\n+\tprintf \"Foo.Bar.Hello, world!\" >expect-err &&\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n+test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--reader-use-sideband \\\n+\t\t--no-chomp-newline out 2>err &&\n+\tcat >expect-out <<-EOF &&\n+\t\tprimary: regular output\n+\tEOF\n+\tprintf \"remote: Foo. \\n\" >expect-err &&\n+\tprintf \"remote: Bar. \\n\" >>expect-err &&\n+\tprintf \"remote: Hello, world! \\n\" >>expect-err &&\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n+test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--reader-use-sideband \\\n+\t\t--chomp-newline out 2>err &&\n+\tprintf \"primary: regular output\" >expect-out &&\n+\tprintf \"remote: Foo. \\n\" >expect-err &&\n+\tprintf \"remote: Bar. \\n\" >>expect-err &&\n+\tprintf \"remote: Hello, world! \\n\" >>expect-err &&\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n test_done\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482646","messageId":"68ac3ea711ce2705a328e6b59c95c0ca884a7e6c.1696425168.git.zhiyou.jx@alibaba-inc.com","threadId":"60244","inReplyTo":"cover.1696425168.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 3/3] pkt-line: do not chomp newlines for sideband messages","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T13:18:14Z","receivedAt":"2023-10-04T13:18:31Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen calling \"packet_read_with_status()\" to parse pkt-line encoded\npackets, we can turn on the flag \"PACKET_READ_CHOMP_NEWLINE\" to chomp\nnewline character for each packet for better line matching. But when\nreceiving data and progress information using sideband, we should turn\noff the flag \"PACKET_READ_CHOMP_NEWLINE\" to prevent mangling newline\ncharacters from data and progress information.\n\nWhen both the server and the client support \"sideband-all\" capability,\nwe have a dilemma that newline characters in negotiation packets should\nbe removed, but the newline characters in the progress information\nshould be left intact.\n\nAdd new flag \"PACKET_READ_USE_SIDEBAND\" for \"packet_read_with_status()\"\nto prevent mangling newline characters in sideband messages.\n\nHelped-by: Jonathan Tan \nHelped-by: Oswald Buddenhagen \nSigned-off-by: Jiang Xin \n---\n pkt-line.c | 31 +++++++++++++++++++++++++++++--\n pkt-line.h | 1 +\n t/t0070-fundamental.sh | 2 +-\n 3 files changed, 31 insertions(+), 3 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 5943777a17..e9061e61a4 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -462,8 +462,32 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t}\n \n \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n-\t len && buffer[len-1] == '\\n')\n-\t\tlen--;\n+\t len && buffer[len-1] == '\\n') {\n+\t\tif (options & PACKET_READ_USE_SIDEBAND) {\n+\t\t\tint band = *buffer & 0xff;\n+\t\t\tswitch (band) {\n+\t\t\tcase 1:\n+\t\t\t\t/* Chomp newline for payload */\n+\t\t\t\tlen--;\n+\t\t\t\tbreak;\n+\t\t\tcase 2:\n+\t\t\tcase 3:\n+\t\t\t\t/*\n+\t\t\t\t * Do not chomp newline for progress and error\n+\t\t\t\t * message.\n+\t\t\t\t */\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\t/*\n+\t\t\t\t * Bad sideband, let's leave it to\n+\t\t\t\t * demultiplex_sideband() to catch this error.\n+\t\t\t\t */\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tlen--;\n+\t\t}\n+\t}\n \n \tbuffer[len] = 0;\n \tif (options & PACKET_READ_REDACT_URI_PATH &&\n@@ -602,6 +626,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\treturn reader->status;\n \t}\n \n+\tif (reader->use_sideband)\n+\t\treader->options |= PACKET_READ_USE_SIDEBAND;\n+\n \t/*\n \t * Consume all progress packets until a primary payload packet is\n \t * received\ndiff --git a/pkt-line.h b/pkt-line.h\nindex be1010d34e..a7ff2e2f18 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -85,6 +85,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n #define PACKET_READ_REDACT_URI_PATH (1u<<4)\n+#define PACKET_READ_USE_SIDEBAND (1u<<5)\n int packet_read(int fd, char *buffer, unsigned size, int options);\n \n /*\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex a927c665d6..138c2becc1 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli\n \ttest_cmp expect-err err\n '\n \n-test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n+test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n \ttest_when_finished \"rm -f expect-out expect-err\" &&\n \ttest-tool pkt-line send-split-sideband >split-sideband &&\n \ttest-tool pkt-line unpack-sideband \\\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482669","messageId":"xmqqa5syp302.fsf@gitster.g","threadId":"60244","inReplyTo":"CANYiYbHp-3YuSPHnR8gjS40UJLrJV5FPzqd_BtjyR8TAALhfRQ@mail.gmail.com","subject":"Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-04T20:05:33Z","receivedAt":"2023-10-04T20:05:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> On Tue, Sep 26, 2023 at 4:48 PM Oswald Buddenhagen\n> wrote:\n>>\n>> >Jiang Xin writes:\n>> >\n>> >> +++ b/pkt-line.c\n>> >> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n>> >> }\n>> >> + case 2:\n>> >> + /* fallthrough */\n>> >> + case 3:\n>> >\n>> while not entirely unprecedented, it's unnecessary and even\n>> counter-productive to annotate directly adjacent cases with fallthrough.\n>\n> I see in \"blame.c\" there are directly adjacent cases like below. I\n> will remove the fallthrough statement.\n>\n> case 'A':\n> case 'T':\n> /* Did not exist in parent, or type changed */\n> break;\n\nYeah, it is far clearer to understand if it is written without the\n\"fallthru\" comment between the cases and instead a comment that\nexplains both cases after them (exactly like the example you found\nin \"blame.c\"). When we want \"fallthru\" comment is if we had some\nprocessing specific to the earlier case ('A' or '2') that is not\ndone in the later case ('T' or '3'), in which case we may want to\nexplicitly say we did not forget to \"break\" by adding the \"fallthru\"\ncomment. But it does not apply here.\n\nThanks.\n\n"},{"id":"485754","messageId":"cover.1702823801.git.zhiyou.jx@alibaba-inc.com","threadId":"60244","inReplyTo":"cover.1696425168.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v4 0/3] Sideband-all demultiplexer fixes","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-17T14:41:35Z","receivedAt":"2023-12-17T14:41:41Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\n# Changes since v3:\n\nChange commit message and comments to make them clear for review.\n\n\n# range-diff v3...v4\n\n1: e387088da2 ! 1: ff4e5aff2a test-pkt-line: add option parser for unpack-sideband\n @@ Commit message\n * Use \"test-tool pkt-line send-split-sideband\" to generate sideband\n messages.\n \n - * We can pipe these generated sideband messages to command \"test-tool\n - pkt-line unpack-sideband\" to test packet_reader_read() function.\n + * Pipe these generated sideband messages to command \"test-tool pkt-line\n + unpack-sideband\" to test packet_reader_read() function.\n \n In order to make a complete test of the packet_reader_read() function,\n add option parser for command \"test-tool pkt-line unpack-sideband\".\n \n - To remove newlines in sideband messages, we can use:\n + * To remove newlines in sideband messages, we can use:\n \n - $ test-tool pkt-line unpack-sideband --chomp-newline\n + $ test-tool pkt-line unpack-sideband --chomp-newline\n \n - To preserve newlines in sideband messages, we can use:\n + * To preserve newlines in sideband messages, we can use:\n \n - $ test-tool pkt-line unpack-sideband --no-chomp-newline\n + $ test-tool pkt-line unpack-sideband --no-chomp-newline\n \n - To parse sideband messages using \"demultiplex_sideband()\" inside the\n - function \"packet_reader_read()\", we can use:\n + * To parse sideband messages using \"demultiplex_sideband()\" inside the\n + function \"packet_reader_read()\", we can use:\n \n - $ test-tool pkt-line unpack-sideband --reader-use-sideband\n + $ test-tool pkt-line unpack-sideband --reader-use-sideband\n \n - Add several new test cases in t0070. Among these test cases, we pipe\n + We also add new example sideband packets in send_split_sideband() and\n + add several new test cases in t0070. Among these test cases, we pipe\n output of the \"send-split-sideband\" subcommand to the \"unpack-sideband\"\n subcommand. We found two issues:\n \n 1. The two splitted sideband messages \"Hello,\" and \" world!\\n\" should\n - be concatenated together. But when we enabled the function\n - \"demultiplex_sideband()\" to parse sideband messages, the first part\n - of the splitted message (\"Hello,\") is lost.\n + be concatenated together. But when we turn on use_sideband field of\n + reader to parse sideband messages, the first part of the splitted\n + message (\"Hello,\") is lost.\n \n 2. The newline characters in sideband 2 (progress info) and sideband 3\n - (error message) should be preserved, but they are also trimmed.\n + (error message) should be preserved, but they are both trimmed.\n \n Will fix the above two issues in subsequent commits.\n \n Signed-off-by: Jiang Xin \n \n ## t/helper/test-pkt-line.c ##\n @@\n @@ t/helper/test-pkt-line.c: static void unpack_sideband(void)\n +\t\t\t/*\n +\t\t\t * When the \"use_sideband\" field of the reader is turned\n +\t\t\t * on, sideband packets other than the payload have been\n -+\t\t\t * parsed and consumed.\n ++\t\t\t * parsed and consumed in packet_reader_read(), and only\n ++\t\t\t * the payload arrives here.\n +\t\t\t */\n +\t\t\tif (reader.use_sideband) {\n +\t\t\t\twrite_or_die(1, reader.line, reader.pktlen - 1);\n @@ t/helper/test-pkt-line.c: static void unpack_sideband(void)\n \tpacket_response_end(1);\n \n +\t/*\n -+\t * The unpack_sideband() function above requires a flush\n -+\t * packet to end parsing.\n ++\t * We use unpack_sideband() to consume packets. A flush packet\n ++\t * is required to end parsing.\n +\t */\n +\tpacket_flush(1);\n +\n @@ t/t0070-fundamental.sh: test_expect_success 'missing sideband designator is repo\n +\ttest_cmp expect-err err\n +'\n +\n -+test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '\n ++test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '\n +\ttest_when_finished \"rm -f expect-out expect-err\" &&\n +\ttest-tool pkt-line send-split-sideband >split-sideband &&\n +\ttest-tool pkt-line unpack-sideband \\\n @@ t/t0070-fundamental.sh: test_expect_success 'missing sideband designator is repo\n +\ttest_cmp expect-err err\n +'\n +\n -+test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n ++test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '\n +\ttest_when_finished \"rm -f expect-out expect-err\" &&\n +\ttest-tool pkt-line send-split-sideband >split-sideband &&\n +\ttest-tool pkt-line unpack-sideband \\\n2: 633bfbac39 ! 2: 5942b74cab pkt-line: memorize sideband fragment in reader\n @@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband: --chomp-newline (d\n \ttest_cmp expect-err err\n '\n \n --test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '\n -+test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newline' '\n +-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '\n ++test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '\n \ttest_when_finished \"rm -f expect-out expect-err\" &&\n \ttest-tool pkt-line send-split-sideband >split-sideband &&\n \ttest-tool pkt-line unpack-sideband \\\n3: 2a2da65fac ! 3: dd2e34da16 pkt-line: do not chomp newlines for sideband messages\n @@ pkt-line.h: void packet_fflush(FILE *f);\n /*\n \n ## t/t0070-fundamental.sh ##\n -@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli\n +@@ t/t0070-fundamental.sh: test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no\n \ttest_cmp expect-err err\n '\n \n --test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n -+test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '\n +-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '\n ++test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '\n \ttest_when_finished \"rm -f expect-out expect-err\" &&\n \ttest-tool pkt-line send-split-sideband >split-sideband &&\n \ttest-tool pkt-line unpack-sideband \\\n\n\nJiang Xin (3):\n test-pkt-line: add option parser for unpack-sideband\n pkt-line: memorize sideband fragment in reader\n pkt-line: do not chomp newlines for sideband messages\n\n pkt-line.c | 36 ++++++++++++++++++++----\n pkt-line.h | 4 +++\n t/helper/test-pkt-line.c | 59 ++++++++++++++++++++++++++++++++++++----\n t/t0070-fundamental.sh | 58 +++++++++++++++++++++++++++++++++++++++\n 4 files changed, 147 insertions(+), 10 deletions(-)\n\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485755","messageId":"ff4e5aff2acc5e7e1d8546798639c556e74d7076.1702823801.git.zhiyou.jx@alibaba-inc.com","threadId":"60244","inReplyTo":"cover.1702823801.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v4 1/3] test-pkt-line: add option parser for unpack-sideband","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-17T14:41:36Z","receivedAt":"2023-12-17T14:41:42Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWe can use the test helper program \"test-tool pkt-line\" to test pkt-line\nrelated functions. E.g.:\n\n * Use \"test-tool pkt-line send-split-sideband\" to generate sideband\n messages.\n\n * Pipe these generated sideband messages to command \"test-tool pkt-line\n unpack-sideband\" to test packet_reader_read() function.\n\nIn order to make a complete test of the packet_reader_read() function,\nadd option parser for command \"test-tool pkt-line unpack-sideband\".\n\n * To remove newlines in sideband messages, we can use:\n\n $ test-tool pkt-line unpack-sideband --chomp-newline\n\n * To preserve newlines in sideband messages, we can use:\n\n $ test-tool pkt-line unpack-sideband --no-chomp-newline\n\n * To parse sideband messages using \"demultiplex_sideband()\" inside the\n function \"packet_reader_read()\", we can use:\n\n $ test-tool pkt-line unpack-sideband --reader-use-sideband\n\nWe also add new example sideband packets in send_split_sideband() and\nadd several new test cases in t0070. Among these test cases, we pipe\noutput of the \"send-split-sideband\" subcommand to the \"unpack-sideband\"\nsubcommand. We found two issues:\n\n 1. The two splitted sideband messages \"Hello,\" and \" world!\\n\" should\n be concatenated together. But when we turn on use_sideband field of\n reader to parse sideband messages, the first part of the splitted\n message (\"Hello,\") is lost.\n\n 2. The newline characters in sideband 2 (progress info) and sideband 3\n (error message) should be preserved, but they are both trimmed.\n\nWill fix the above two issues in subsequent commits.\n\nSigned-off-by: Jiang Xin \n---\n t/helper/test-pkt-line.c | 59 ++++++++++++++++++++++++++++++++++++----\n t/t0070-fundamental.sh | 58 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 112 insertions(+), 5 deletions(-)\n\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex f4d134a145..6b306cf5ca 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -2,6 +2,7 @@\n #include \"test-tool.h\"\n #include \"pkt-line.h\"\n #include \"write-or-die.h\"\n+#include \"parse-options.h\"\n \n static void pack_line(const char *line)\n {\n@@ -64,12 +65,33 @@ static void unpack(void)\n \t}\n }\n \n-static void unpack_sideband(void)\n+static void unpack_sideband(int argc, const char **argv)\n {\n \tstruct packet_reader reader;\n-\tpacket_reader_init(&reader, 0, NULL, 0,\n-\t\t\t PACKET_READ_GENTLE_ON_EOF |\n-\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\tint options = PACKET_READ_GENTLE_ON_EOF;\n+\tint chomp_newline = 1;\n+\tint reader_use_sideband = 0;\n+\tconst char *const unpack_sideband_usage[] = {\n+\t\t\"test_tool unpack_sideband [options...]\", NULL\n+\t};\n+\tstruct option cmd_options[] = {\n+\t\tOPT_BOOL(0, \"reader-use-sideband\", &reader_use_sideband,\n+\t\t\t \"set use_sideband bit for packet reader (Default: off)\"),\n+\t\tOPT_BOOL(0, \"chomp-newline\", &chomp_newline,\n+\t\t\t \"chomp newline in packet (Default: on)\"),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, \"\", cmd_options, unpack_sideband_usage,\n+\t\t\t 0);\n+\tif (argc > 0)\n+\t\tusage_msg_opt(_(\"too many arguments\"), unpack_sideband_usage,\n+\t\t\t cmd_options);\n+\n+\tif (chomp_newline)\n+\t\toptions |= PACKET_READ_CHOMP_NEWLINE;\n+\tpacket_reader_init(&reader, 0, NULL, 0, options);\n+\treader.use_sideband = reader_use_sideband;\n \n \twhile (packet_reader_read(&reader) != PACKET_READ_EOF) {\n \t\tint band;\n@@ -79,6 +101,17 @@ static void unpack_sideband(void)\n \t\tcase PACKET_READ_EOF:\n \t\t\tbreak;\n \t\tcase PACKET_READ_NORMAL:\n+\t\t\t/*\n+\t\t\t * When the \"use_sideband\" field of the reader is turned\n+\t\t\t * on, sideband packets other than the payload have been\n+\t\t\t * parsed and consumed in packet_reader_read(), and only\n+\t\t\t * the payload arrives here.\n+\t\t\t */\n+\t\t\tif (reader.use_sideband) {\n+\t\t\t\twrite_or_die(1, reader.line, reader.pktlen - 1);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n \t\t\tband = reader.line[0] & 0xff;\n \t\t\tif (band < 1 || band > 2)\n \t\t\t\tcontinue; /* skip non-sideband packets */\n@@ -97,15 +130,31 @@ static void unpack_sideband(void)\n \n static int send_split_sideband(void)\n {\n+\tconst char *foo = \"Foo.\\n\";\n+\tconst char *bar = \"Bar.\\n\";\n \tconst char *part1 = \"Hello,\";\n \tconst char *primary = \"\\001primary: regular output\\n\";\n \tconst char *part2 = \" world!\\n\";\n \n+\t/* Each sideband message has a trailing newline character. */\n+\tsend_sideband(1, 2, foo, strlen(foo), LARGE_PACKET_MAX);\n+\tsend_sideband(1, 2, bar, strlen(bar), LARGE_PACKET_MAX);\n+\n+\t/*\n+\t * One sideband message is divided into part1 and part2\n+\t * by the primary message.\n+\t */\n \tsend_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);\n \tpacket_write(1, primary, strlen(primary));\n \tsend_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);\n \tpacket_response_end(1);\n \n+\t/*\n+\t * We use unpack_sideband() to consume packets. A flush packet\n+\t * is required to end parsing.\n+\t */\n+\tpacket_flush(1);\n+\n \treturn 0;\n }\n \n@@ -126,7 +175,7 @@ int cmd__pkt_line(int argc, const char **argv)\n \telse if (!strcmp(argv[1], \"unpack\"))\n \t\tunpack();\n \telse if (!strcmp(argv[1], \"unpack-sideband\"))\n-\t\tunpack_sideband();\n+\t\tunpack_sideband(argc - 1, argv + 1);\n \telse if (!strcmp(argv[1], \"send-split-sideband\"))\n \t\tsend_split_sideband();\n \telse if (!strcmp(argv[1], \"receive-sideband\"))\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 574de34198..297a7f772e 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -53,4 +53,62 @@ test_expect_success 'missing sideband designator is reported' '\n \ttest_i18ngrep \"missing sideband\" err\n '\n \n+test_expect_success 'unpack-sideband: --no-chomp-newline' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--no-chomp-newline out 2>err &&\n+\tcat >expect-out <<-EOF &&\n+\t\tprimary: regular output\n+\tEOF\n+\tcat >expect-err <<-EOF &&\n+\t\tFoo.\n+\t\tBar.\n+\t\tHello, world!\n+\tEOF\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n+test_expect_success 'unpack-sideband: --chomp-newline (default)' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--chomp-newline out 2>err &&\n+\tprintf \"primary: regular output\" >expect-out &&\n+\tprintf \"Foo.Bar.Hello, world!\" >expect-err &&\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n+test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--reader-use-sideband \\\n+\t\t--no-chomp-newline out 2>err &&\n+\tcat >expect-out <<-EOF &&\n+\t\tprimary: regular output\n+\tEOF\n+\tprintf \"remote: Foo. \\n\" >expect-err &&\n+\tprintf \"remote: Bar. \\n\" >>expect-err &&\n+\tprintf \"remote: Hello, world! \\n\" >>expect-err &&\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n+test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '\n+\ttest_when_finished \"rm -f expect-out expect-err\" &&\n+\ttest-tool pkt-line send-split-sideband >split-sideband &&\n+\ttest-tool pkt-line unpack-sideband \\\n+\t\t--reader-use-sideband \\\n+\t\t--chomp-newline out 2>err &&\n+\tprintf \"primary: regular output\" >expect-out &&\n+\tprintf \"remote: Foo. \\n\" >expect-err &&\n+\tprintf \"remote: Bar. \\n\" >>expect-err &&\n+\tprintf \"remote: Hello, world! \\n\" >>expect-err &&\n+\ttest_cmp expect-out out &&\n+\ttest_cmp expect-err err\n+'\n+\n test_done\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485756","messageId":"5942b74cab4ffe8509169a0a4b8442019fd05e01.1702823801.git.zhiyou.jx@alibaba-inc.com","threadId":"60244","inReplyTo":"cover.1702823801.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v4 2/3] pkt-line: memorize sideband fragment in reader","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-17T14:41:37Z","receivedAt":"2023-12-17T14:41:43Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen we turn on the \"use_sideband\" field of the packet_reader,\n\"packet_reader_read()\" will call the function \"demultiplex_sideband()\"\nto parse and consume sideband messages. Sideband fragment which does not\nend with \"\\r\" or \"\\n\" will be saved in the sixth parameter \"scratch\"\nand it can be reused and be concatenated when parsing another sideband\nmessage.\n\nIn \"packet_reader_read()\" function, the local variable \"scratch\" can\nonly be reused by subsequent sideband messages. But if there is a\npayload message between two sideband fragments, the first fragment\nwhich is saved in the local variable \"scratch\" will be lost.\n\nTo solve this problem, we can add a new field \"scratch\" in\npacket_reader to memorize the sideband fragment across different calls\nof \"packet_reader_read()\".\n\nSigned-off-by: Jiang Xin \n---\n pkt-line.c | 5 ++---\n pkt-line.h | 3 +++\n t/t0070-fundamental.sh | 2 +-\n 3 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex af83a19f4d..5943777a17 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -592,12 +592,11 @@ void packet_reader_init(struct packet_reader *reader, int fd,\n \treader->options = options;\n \treader->me = \"git\";\n \treader->hash_algo = &hash_algos[GIT_HASH_SHA1];\n+\tstrbuf_init(&reader->scratch, 0);\n }\n \n enum packet_read_status packet_reader_read(struct packet_reader *reader)\n {\n-\tstruct strbuf scratch = STRBUF_INIT;\n-\n \tif (reader->line_peeked) {\n \t\treader->line_peeked = 0;\n \t\treturn reader->status;\n@@ -620,7 +619,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\t\tbreak;\n \t\tif (demultiplex_sideband(reader->me, reader->status,\n \t\t\t\t\t reader->buffer, reader->pktlen, 1,\n-\t\t\t\t\t &scratch, &sideband_type))\n+\t\t\t\t\t &reader->scratch, &sideband_type))\n \t\t\tbreak;\n \t}\n \ndiff --git a/pkt-line.h b/pkt-line.h\nindex 954eec8719..be1010d34e 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -194,6 +194,9 @@ struct packet_reader {\n \n \t/* hash algorithm in use */\n \tconst struct git_hash_algo *hash_algo;\n+\n+\t/* hold temporary sideband message */\n+\tstruct strbuf scratch;\n };\n \n /*\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 297a7f772e..275edbf6e7 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -81,7 +81,7 @@ test_expect_success 'unpack-sideband: --chomp-newline (default)' '\n \ttest_cmp expect-err err\n '\n \n-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '\n+test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no chomp payload' '\n \ttest_when_finished \"rm -f expect-out expect-err\" &&\n \ttest-tool pkt-line send-split-sideband >split-sideband &&\n \ttest-tool pkt-line unpack-sideband \\\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485757","messageId":"dd2e34da16ccd213e77a317c1d5afd2e44ebf339.1702823801.git.zhiyou.jx@alibaba-inc.com","threadId":"60244","inReplyTo":"cover.1702823801.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v4 3/3] pkt-line: do not chomp newlines for sideband messages","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-17T14:41:38Z","receivedAt":"2023-12-17T14:41:44Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen calling \"packet_read_with_status()\" to parse pkt-line encoded\npackets, we can turn on the flag \"PACKET_READ_CHOMP_NEWLINE\" to chomp\nnewline character for each packet for better line matching. But when\nreceiving data and progress information using sideband, we should turn\noff the flag \"PACKET_READ_CHOMP_NEWLINE\" to prevent mangling newline\ncharacters from data and progress information.\n\nWhen both the server and the client support \"sideband-all\" capability,\nwe have a dilemma that newline characters in negotiation packets should\nbe removed, but the newline characters in the progress information\nshould be left intact.\n\nAdd new flag \"PACKET_READ_USE_SIDEBAND\" for \"packet_read_with_status()\"\nto prevent mangling newline characters in sideband messages.\n\nHelped-by: Jonathan Tan \nHelped-by: Oswald Buddenhagen \nSigned-off-by: Jiang Xin \n---\n pkt-line.c | 31 +++++++++++++++++++++++++++++--\n pkt-line.h | 1 +\n t/t0070-fundamental.sh | 2 +-\n 3 files changed, 31 insertions(+), 3 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 5943777a17..e9061e61a4 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -462,8 +462,32 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t}\n \n \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n-\t len && buffer[len-1] == '\\n')\n-\t\tlen--;\n+\t len && buffer[len-1] == '\\n') {\n+\t\tif (options & PACKET_READ_USE_SIDEBAND) {\n+\t\t\tint band = *buffer & 0xff;\n+\t\t\tswitch (band) {\n+\t\t\tcase 1:\n+\t\t\t\t/* Chomp newline for payload */\n+\t\t\t\tlen--;\n+\t\t\t\tbreak;\n+\t\t\tcase 2:\n+\t\t\tcase 3:\n+\t\t\t\t/*\n+\t\t\t\t * Do not chomp newline for progress and error\n+\t\t\t\t * message.\n+\t\t\t\t */\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\t/*\n+\t\t\t\t * Bad sideband, let's leave it to\n+\t\t\t\t * demultiplex_sideband() to catch this error.\n+\t\t\t\t */\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tlen--;\n+\t\t}\n+\t}\n \n \tbuffer[len] = 0;\n \tif (options & PACKET_READ_REDACT_URI_PATH &&\n@@ -602,6 +626,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \t\treturn reader->status;\n \t}\n \n+\tif (reader->use_sideband)\n+\t\treader->options |= PACKET_READ_USE_SIDEBAND;\n+\n \t/*\n \t * Consume all progress packets until a primary payload packet is\n \t * received\ndiff --git a/pkt-line.h b/pkt-line.h\nindex be1010d34e..a7ff2e2f18 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -85,6 +85,7 @@ void packet_fflush(FILE *f);\n #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)\n #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)\n #define PACKET_READ_REDACT_URI_PATH (1u<<4)\n+#define PACKET_READ_USE_SIDEBAND (1u<<5)\n int packet_read(int fd, char *buffer, unsigned size, int options);\n \n /*\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 275edbf6e7..0d2b7d8d93 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, no\n \ttest_cmp expect-err err\n '\n \n-test_expect_failure 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '\n+test_expect_success 'unpack-sideband: packet_reader_read() consumes sideband, chomp payload' '\n \ttest_when_finished \"rm -f expect-out expect-err\" &&\n \ttest-tool pkt-line send-split-sideband >split-sideband &&\n \ttest-tool pkt-line unpack-sideband \\\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"}]}