{"thread":{"id":"56071","subject":"[PATCH 0/5] tests: migrate to \"test-tool pkt-line\"","startedAt":"2021-07-07T10:21:59Z","lastAt":"2021-07-19T19:15:06Z","messageCount":33,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"429408","messageId":"cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com","threadId":"56071","inReplyTo":null,"subject":"[PATCH 0/5] tests: migrate to \"test-tool pkt-line\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:21:44Z","receivedAt":"2021-07-07T10:21:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code that uses [de]packetize() to use \"test-tool\npkt-line\". That we had these two concurrently in the tests is mostly\njust a historical anomaly/inconsistency that we can fix.\n\nEven the CC'd author of [de]packetize() has recently been using the\ntest-tool, in af22a63c399 (sideband: diagnose more sideband anomalies,\n2020-10-28).\n\n\"Mostly\" because it turns out we were missing one feature in the\ntest-tool, to packetize raw input without any \\n or \\0 munging. This\nseries adds that capabability, migrates those users, and finally\nremoves the now-obsolete [de]packetize() functions.\n\nÆvar Arnfjörð Bjarmason (5):\n serve tests: add missing \"extra delim\" test\n serve tests: use test_cmp in \"protocol violations\" test\n tests: replace [de]packetize() shell+perl test-tool pkt-line\n tests: replace remaining packetize() with \"test-tool pkt-line\"\n test-lib-functions.sh: remove unused [de]packetize() functions\n\n t/helper/test-pkt-line.c | 13 ++++++\n t/t5410-receive-pack-alternates.sh | 42 +++++++++++++-----\n t/t5411/once-0010-report-status-v1.sh | 12 ++---\n t/t5500-fetch-pack.sh | 15 ++++---\n t/t5530-upload-pack-error.sh | 24 +++++-----\n t/t5562-http-backend-content-length.sh | 16 ++++---\n t/t5570-git-daemon.sh | 22 ++++++----\n t/t5704-protocol-violations.sh | 61 ++++++++++++++++++--------\n t/test-lib-functions.sh | 42 ------------------\n 9 files changed, 136 insertions(+), 111 deletions(-)\n\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429409","messageId":"patch-1.5-fcb53980597-20210707T101549Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com","subject":"[PATCH 1/5] serve tests: add missing \"extra delim\" test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:21:45Z","receivedAt":"2021-07-07T10:21:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the object-info protocol v2 call was added in\na2ba162cda2 (object-info: support for retrieving object info,\n2021-04-20) we didn't add a corresponding test here.\n\nWe had tests for ls-refs and fetch already since\n4845b772458 (upload-pack: handle unexpected delim packets,\n2020-03-27), but the same behavior in object-info (which is clearly\ncopied from the template of \"ls-refs\") did not have the corresponding\ntests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5704-protocol-violations.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 5c941949b98..95f68e1d4b7 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -32,4 +32,21 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n \ttest_i18ngrep \"expected flush after fetch arguments\" err\n '\n \n+test_expect_success 'extra delim packet in v2 object-info args' '\n+\t# protocol expects 0000 flush after the 0001\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=object-info\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\t0001\n+\tEOF\n+\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: object-info: expected flush after arguments\n+\tEOF\n+\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n+\t\tgit upload-pack . err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n test_done\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429410","messageId":"patch-2.5-c3544fb53cd-20210707T101549Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com","subject":"[PATCH 2/5] serve tests: use test_cmp in \"protocol violations\" test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:21:46Z","receivedAt":"2021-07-07T10:22:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the protocol violations tests to check the full output, not just\ngrep it. This changes code added in 4845b772458 (upload-pack: handle\nunexpected delim packets, 2020-03-27). The newly added test in the\npreceding commit already did the full test_cmp.\n\nI have a related series to tweak the output from upload-pack et al, we\nreally want to make sure we have this exact output, and not fewer or\nmore lines etc.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5704-protocol-violations.sh | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 95f68e1d4b7..038fffd3d03 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -14,9 +14,12 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '\n \t\t# protocol expects 0000 flush here\n \t\tprintf 0001\n \t} >input &&\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: expected flush after ls-refs arguments\n+\tEOF\n \ttest_must_fail env GIT_PROTOCOL=version=2 \\\n-\t\tgit upload-pack . err &&\n-\ttest_i18ngrep \"expected flush after ls-refs arguments\" err\n+\t\tgit upload-pack . err.actual &&\n+\ttest_cmp err.expect err.actual\n '\n \n test_expect_success 'extra delim packet in v2 fetch args' '\n@@ -27,9 +30,12 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n \t\t# protocol expects 0000 flush here\n \t\tprintf 0001\n \t} >input &&\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: expected flush after fetch arguments\n+\tEOF\n \ttest_must_fail env GIT_PROTOCOL=version=2 \\\n-\t\tgit upload-pack . err &&\n-\ttest_i18ngrep \"expected flush after fetch arguments\" err\n+\t\tgit upload-pack . err.actual &&\n+\ttest_cmp err.expect err.actual\n '\n \n test_expect_success 'extra delim packet in v2 object-info args' '\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429411","messageId":"patch-3.5-c1015fa6ab0-20210707T101549Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com","subject":"[PATCH 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:21:47Z","receivedAt":"2021-07-07T10:22:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The shell+perl \"[de]packetize()\" helper functions were added in\n4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n2018-01-24), and around the same time we added the \"pkt-line\" helper\nin 74e70029615 (test-pkt-line: introduce a packet-line test helper,\n2018-03-14).\n\nFor some reason it seems we've mostly used the shell+perl version\ninstead of the helper since then. There were discussions around\n88124ab2636 (test-lib-functions: make packetize() more efficient,\n2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()\nstdin code, 2020-03-29) to improve them and make them more efficient.\n\nLet's instead just use the test helper, I think this results in both\nmore legible code, and for anyone who cares about efficiency it'll be\nfaster.\n\nWe can't convert all the users of packetize(), it has a feature the\ntest-tool is missing. This'll be addressed in the subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5410-receive-pack-alternates.sh | 42 ++++++++++++++++++--------\n t/t5411/once-0010-report-status-v1.sh | 8 ++---\n t/t5500-fetch-pack.sh | 15 +++++----\n t/t5530-upload-pack-error.sh | 24 ++++++++-------\n t/t5562-http-backend-content-length.sh | 13 ++++----\n t/t5570-git-daemon.sh | 12 +++++---\n t/t5704-protocol-violations.sh | 30 +++++++++---------\n 7 files changed, 86 insertions(+), 58 deletions(-)\n\ndiff --git a/t/t5410-receive-pack-alternates.sh b/t/t5410-receive-pack-alternates.sh\nindex 0b28e4e452f..d0053d95a44 100755\n--- a/t/t5410-receive-pack-alternates.sh\n+++ b/t/t5410-receive-pack-alternates.sh\n@@ -16,10 +16,6 @@ test_expect_success 'setup' '\n \ttest_commit private\n '\n \n-extract_haves () {\n-\tdepacketize | perl -lne '/^(\\S+) \\.have/ and print $1'\n-}\n-\n test_expect_success 'with core.alternateRefsCommand' '\n \twrite_script fork/alternate-refs <<-\\EOF &&\n \t\tgit --git-dir=\"$1\" for-each-ref \\\n@@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '\n \t\t\trefs/heads/public/\n \tEOF\n \ttest_config -C fork core.alternateRefsCommand ./alternate-refs &&\n-\tgit rev-parse public/branch >expect &&\n-\tprintf \"0000\" | git receive-pack fork >actual &&\n-\textract_haves actual.haves &&\n-\ttest_cmp expect actual.haves\n+\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main) refs/heads/main\n+\t$(git rev-parse base) refs/tags/base\n+\t$(git rev-parse public) .have\n+\t0000\n+\tEOF\n+\n+\tgit receive-pack fork >out actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'with core.alternateRefsPrefixes' '\n \ttest_config -C fork core.alternateRefsPrefixes \"refs/heads/private\" &&\n-\tgit rev-parse private/branch >expect &&\n-\tprintf \"0000\" | git receive-pack fork >actual &&\n-\textract_haves actual.haves &&\n-\ttest_cmp expect actual.haves\n+\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main) refs/heads/main\n+\t$(git rev-parse base) refs/tags/base\n+\t$(git rev-parse private) .have\n+\t0000\n+\tEOF\n+\n+\tgit receive-pack fork >out actual &&\n+\ttest_cmp expect actual\n '\n \n test_done\ndiff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\nindex 1233a46eac5..cf33d993192 100644\n--- a/t/t5411/once-0010-report-status-v1.sh\n+++ b/t/t5411/once-0010-report-status-v1.sh\n@@ -34,13 +34,13 @@ test_expect_success \"proc-receive: report status v1\" '\n \t\t\t\t$A $B | packetize\n \t\tfi &&\n \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n \t\tprintf \"%s %s refs/heads/foo\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n \t\tprintf \"%s %s refs/for/next/topic\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n \t\tprintf \"%s %s refs/for/main/topic2\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n \t\tprintf 0000 &&\n \t\tprintf \"\" | git -C \"$upstream\" pack-objects --stdout\n \t} | git receive-pack \"$upstream\" --stateless-rpc \\\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 8a5d3492c71..ff0b7dd89f9 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -872,14 +872,17 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '\n \tgit commit-graph write --reachable &&\n \tgit config core.commitGraph true &&\n \n-\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n-\t0012command=fetch\n-\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n-\t00010013deepen-since 1\n-\t$(echo \"want $(git rev-parse other)\" | packetize)\n-\t$(echo \"have $(git rev-parse main)\" | packetize)\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\tdeepen-since 1\n+\twant $(git rev-parse other)\n+\thave $(git rev-parse main)\n \t0000\n \tEOF\n+\n+\tGIT_PROTOCOL=version=2 git upload-pack . /dev/null\n \t)\n '\n \ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 7c1460eaa99..8ccaae10475 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -90,18 +90,20 @@ test_expect_success 'upload-pack fails due to error in pack-objects enumeration'\n \n test_expect_success 'upload-pack tolerates EOF just after stateless client wants' '\n \ttest_commit initial &&\n-\thead=$(git rev-parse HEAD) &&\n-\n-\t{\n-\t\tpacketize \"want $head\" &&\n-\t\tpacketize \"shallow $head\" &&\n-\t\tpacketize \"deepen 1\" &&\n-\t\tprintf \"0000\"\n-\t} >request &&\n \n-\tprintf \"0000\" >expect &&\n-\n-\tgit upload-pack --stateless-rpc . actual &&\n+\thead=$(git rev-parse HEAD) &&\n+\ttest-tool pkt-line pack >request <<-EOF &&\n+\twant $head\n+\tshallow $head\n+\tdeepen 1\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\t0000\n+\tEOF\n+\tgit upload-pack --stateless-rpc . out &&\n+\ttest-tool pkt-line unpack actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex e5d3d15ba8d..e6c8338b648 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -53,12 +53,13 @@ test_expect_success 'setup' '\n \ttest_commit c1 &&\n \thash_head=$(git rev-parse HEAD) &&\n \thash_prev=$(git rev-parse HEAD~1) &&\n-\t{\n-\t\tpacketize \"want $hash_head\" &&\n-\t\tprintf 0000 &&\n-\t\tpacketize \"have $hash_prev\" &&\n-\t\tpacketize \"done\"\n-\t} >fetch_body &&\n+\ttest-tool pkt-line pack >fetch_body <<-EOF &&\n+\twant $hash_head\n+\t0000\n+\thave $hash_prev\n+\tdone\n+\t0000\n+\tEOF\n \ttest_copy_bytes 10 fetch_body.trunc &&\n \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n \t{\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 82c31ab6cd8..2dde0348816 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n \t\tprintf \"0000\"\n \t} >input &&\n \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n-\tdepacketize output.raw &&\n+\ttest-tool pkt-line unpack actual &&\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD) HEAD\n+\t$(git rev-parse refs/heads/main) refs/heads/main\n+\t0000\n+\tEOF\n \n-\t# just pick out the value of main, which avoids any protocol\n-\t# particulars\n-\tperl -lne \"print \\$1 if m{^(\\\\S+) refs/heads/main}\" actual &&\n-\tgit -C \"$repo\" rev-parse main >expect &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 038fffd3d03..44e2c0d3ded 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -7,13 +7,14 @@ making sure that we do not segfault or otherwise behave badly.'\n . ./test-lib.sh\n \n test_expect_success 'extra delim packet in v2 ls-refs args' '\n-\t{\n-\t\tpacketize command=ls-refs &&\n-\t\tpacketize \"object-format=$(test_oid algo)\" &&\n-\t\tprintf 0001 &&\n-\t\t# protocol expects 0000 flush here\n-\t\tprintf 0001\n-\t} >input &&\n+\t# protocol expects 0000 flush after the 0001\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=ls-refs\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\t0001\n+\tEOF\n+\n \tcat >err.expect <<-\\EOF &&\n \tfatal: expected flush after ls-refs arguments\n \tEOF\n@@ -23,13 +24,14 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '\n '\n \n test_expect_success 'extra delim packet in v2 fetch args' '\n-\t{\n-\t\tpacketize command=fetch &&\n-\t\tpacketize \"object-format=$(test_oid algo)\" &&\n-\t\tprintf 0001 &&\n-\t\t# protocol expects 0000 flush here\n-\t\tprintf 0001\n-\t} >input &&\n+\t# protocol expects 0000 flush after the 0001\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\t0001\n+\tEOF\n+\n \tcat >err.expect <<-\\EOF &&\n \tfatal: expected flush after fetch arguments\n \tEOF\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429412","messageId":"patch-4.5-ab23513b48b-20210707T101549Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com","subject":"[PATCH 4/5] tests: replace remaining packetize() with \"test-tool pkt-line\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:21:48Z","receivedAt":"2021-07-07T10:22:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the only remaining users of \"packetize()\" over to \"test-tool\npkt-line\", for this we need a new \"pack-raw-stdin\" subcommand in the\ntest-tool. The \"pack\" command takes input on stdin, but splits it by\n\"\\n\", furthermore we'll format the output using C-strings, so the\nembedded \"\\0\" being tested for here would cause the string to be\ntruncated.\n\nSo we need another mode that just calls packet_write() on the raw\ninput we got on stdin.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/helper/test-pkt-line.c | 13 +++++++++++++\n t/t5411/once-0010-report-status-v1.sh | 4 ++--\n t/t5562-http-backend-content-length.sh | 3 ++-\n t/t5570-git-daemon.sh | 10 ++++++----\n 4 files changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex 5e638f0b970..a2437fbe57d 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -26,6 +26,17 @@ static void pack(int argc, const char **argv)\n \t}\n }\n \n+static void pack_raw_stdin(void)\n+{\n+\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstrbuf_read(&sb, 0, 0);\n+\tif (strbuf_read(&sb, 0, 0) < 0)\n+\t\tdie_errno(\"failed to read from stdin\");\n+\tpacket_write(1, sb.buf, sb.len);\n+\tstrbuf_release(&sb);\n+}\n+\n static void unpack(void)\n {\n \tstruct packet_reader reader;\n@@ -110,6 +121,8 @@ int cmd__pkt_line(int argc, const char **argv)\n \n \tif (!strcmp(argv[1], \"pack\"))\n \t\tpack(argc - 2, argv + 2);\n+\telse if (!strcmp(argv[1], \"pack-raw-stdin\"))\n+\t\tpack_raw_stdin();\n \telse if (!strcmp(argv[1], \"unpack\"))\n \t\tunpack();\n \telse if (!strcmp(argv[1], \"unpack-sideband\"))\ndiff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\nindex cf33d993192..75d4233e49f 100644\n--- a/t/t5411/once-0010-report-status-v1.sh\n+++ b/t/t5411/once-0010-report-status-v1.sh\n@@ -28,10 +28,10 @@ test_expect_success \"proc-receive: report status v1\" '\n \t\tif test -z \"$GIT_DEFAULT_HASH\" || test \"$GIT_DEFAULT_HASH\" = \"sha1\"\n \t\tthen\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status\\n\" \\\n-\t\t\t\t$A $B | packetize\n+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n \t\telse\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status object-format=$GIT_DEFAULT_HASH\\n\" \\\n-\t\t\t\t$A $B | packetize\n+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n \t\tfi &&\n \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n \t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex e6c8338b648..23a8a8d5c70 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -64,7 +64,8 @@ test_expect_success 'setup' '\n \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n \t{\n \t\tprintf \"%s %s refs/heads/newbranch\\\\0report-status object-format=%s\\\\n\" \\\n-\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize &&\n+\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" |\n+\t\t\ttest-tool pkt-line pack-raw-stdin &&\n \t\tprintf 0000 &&\n \t\techo \"$hash_next\" | git pack-objects --stdout\n \t} >push_body &&\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 2dde0348816..b52afb0cdea 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -193,10 +193,12 @@ test_expect_success 'hostname cannot break out of directory' '\n '\n \n test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n-\t{\n-\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n-\t\tprintf \"0000\"\n-\t} >input &&\n+\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" >has-null &&\n+\ttest-tool pkt-line pack-raw-stdin >input >input <<-\\EOF &&\n+\t0000\n+\tEOF\n+\n \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n \ttest-tool pkt-line unpack actual &&\n \n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429413","messageId":"patch-5.5-2d22b83971a-20210707T101549Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com","subject":"[PATCH 5/5] test-lib-functions.sh: remove unused [de]packetize() functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-07T10:21:49Z","receivedAt":"2021-07-07T10:22:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the now-unused \"packetize()\" and \"depacketize()\" functions\nadded in 4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n2018-01-24). As discussed in the preceding commits we've now moved all\ntheir users over to \"test-tool pkt-line\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib-functions.sh | 42 -----------------------------------------\n 1 file changed, 42 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex f0448daa74b..13971aa4e34 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1453,48 +1453,6 @@ nongit () {\n \t)\n } 7>&2 2>&4\n \n-# convert function arguments or stdin (if not arguments given) to pktline\n-# representation. If multiple arguments are given, they are separated by\n-# whitespace and put in a single packet. Note that data containing NULs must be\n-# given on stdin, and that empty input becomes an empty packet, not a flush\n-# packet (for that you can just print 0000 yourself).\n-packetize () {\n-\tif test $# -gt 0\n-\tthen\n-\t\tpacket=\"$*\"\n-\t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n-\telse\n-\t\tperl -e '\n-\t\t\tmy $packet = do { local $/; };\n-\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n-\t\t'\n-\tfi\n-}\n-\n-# Parse the input as a series of pktlines, writing the result to stdout.\n-# Sideband markers are removed automatically, and the output is routed to\n-# stderr if appropriate.\n-#\n-# NUL bytes are converted to \"\\\\0\" for ease of parsing with text tools.\n-depacketize () {\n-\tperl -e '\n-\t\twhile (read(STDIN, $len, 4) == 4) {\n-\t\t\tif ($len eq \"0000\") {\n-\t\t\t\tprint \"FLUSH\\n\";\n-\t\t\t} else {\n-\t\t\t\tread(STDIN, $buf, hex($len) - 4);\n-\t\t\t\t$buf =~ s/\\0/\\\\0/g;\n-\t\t\t\tif ($buf =~ s/^[\\x2\\x3]//) {\n-\t\t\t\t\tprint STDERR $buf;\n-\t\t\t\t} else {\n-\t\t\t\t\t$buf =~ s/^\\x1//;\n-\t\t\t\t\tprint $buf;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n-\t'\n-}\n-\n # Converts base-16 data into base-8. The output is given as a sequence of\n # escaped octals, suitable for consumption by 'printf'.\n hex2oct () {\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429768","messageId":"cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com","subject":"[PATCH v2 0/5] tests: migrate to \"test-tool pkt-line\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-12T16:44:15Z","receivedAt":"2021-07-12T16:44:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series is marked for \"will merge to next\" already, but not there\nyet. A trivial v2 whitespace fix in case Junio's in time to pick it\nup.\n\nSee v1 at https://lore.kernel.org/git/cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (5):\n serve tests: add missing \"extra delim\" test\n serve tests: use test_cmp in \"protocol violations\" test\n tests: replace [de]packetize() shell+perl test-tool pkt-line\n tests: replace remaining packetize() with \"test-tool pkt-line\"\n test-lib-functions.sh: remove unused [de]packetize() functions\n\n t/helper/test-pkt-line.c | 12 +++++\n t/t5410-receive-pack-alternates.sh | 42 +++++++++++++-----\n t/t5411/once-0010-report-status-v1.sh | 12 ++---\n t/t5500-fetch-pack.sh | 15 ++++---\n t/t5530-upload-pack-error.sh | 24 +++++-----\n t/t5562-http-backend-content-length.sh | 16 ++++---\n t/t5570-git-daemon.sh | 22 ++++++----\n t/t5704-protocol-violations.sh | 61 ++++++++++++++++++--------\n t/test-lib-functions.sh | 42 ------------------\n 9 files changed, 135 insertions(+), 111 deletions(-)\n\nRange-diff against v1:\n1: fcb53980597 = 1: 67aa8141153 serve tests: add missing \"extra delim\" test\n2: c3544fb53cd = 2: 64dfd14865c serve tests: use test_cmp in \"protocol violations\" test\n3: c1015fa6ab0 = 3: c33f344ab20 tests: replace [de]packetize() shell+perl test-tool pkt-line\n4: ab23513b48b ! 4: a44e1790f2a tests: replace remaining packetize() with \"test-tool pkt-line\"\n @@ t/helper/test-pkt-line.c: static void pack(int argc, const char **argv)\n \n +static void pack_raw_stdin(void)\n +{\n -+\n +\tstruct strbuf sb = STRBUF_INIT;\n +\tstrbuf_read(&sb, 0, 0);\n +\tif (strbuf_read(&sb, 0, 0) < 0)\n5: 2d22b83971a = 5: cc91d15ef70 test-lib-functions.sh: remove unused [de]packetize() functions\n-- \n2.32.0-dev\n\n"},{"id":"429769","messageId":"patch-1.5-67aa8141153-20210712T164208Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com","subject":"[PATCH v2 1/5] serve tests: add missing \"extra delim\" test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-12T16:44:16Z","receivedAt":"2021-07-12T16:44:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the object-info protocol v2 call was added in\na2ba162cda2 (object-info: support for retrieving object info,\n2021-04-20) we didn't add a corresponding test here.\n\nWe had tests for ls-refs and fetch already since\n4845b772458 (upload-pack: handle unexpected delim packets,\n2020-03-27), but the same behavior in object-info (which is clearly\ncopied from the template of \"ls-refs\") did not have the corresponding\ntests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5704-protocol-violations.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 5c941949b98..95f68e1d4b7 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -32,4 +32,21 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n \ttest_i18ngrep \"expected flush after fetch arguments\" err\n '\n \n+test_expect_success 'extra delim packet in v2 object-info args' '\n+\t# protocol expects 0000 flush after the 0001\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=object-info\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\t0001\n+\tEOF\n+\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: object-info: expected flush after arguments\n+\tEOF\n+\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n+\t\tgit upload-pack . err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n test_done\n-- \n2.32.0-dev\n\n"},{"id":"429770","messageId":"patch-2.5-64dfd14865c-20210712T164208Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com","subject":"[PATCH v2 2/5] serve tests: use test_cmp in \"protocol violations\" test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-12T16:44:17Z","receivedAt":"2021-07-12T16:44:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the protocol violations tests to check the full output, not just\ngrep it. This changes code added in 4845b772458 (upload-pack: handle\nunexpected delim packets, 2020-03-27). The newly added test in the\npreceding commit already did the full test_cmp.\n\nI have a related series to tweak the output from upload-pack et al, we\nreally want to make sure we have this exact output, and not fewer or\nmore lines etc.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5704-protocol-violations.sh | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 95f68e1d4b7..038fffd3d03 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -14,9 +14,12 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '\n \t\t# protocol expects 0000 flush here\n \t\tprintf 0001\n \t} >input &&\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: expected flush after ls-refs arguments\n+\tEOF\n \ttest_must_fail env GIT_PROTOCOL=version=2 \\\n-\t\tgit upload-pack . err &&\n-\ttest_i18ngrep \"expected flush after ls-refs arguments\" err\n+\t\tgit upload-pack . err.actual &&\n+\ttest_cmp err.expect err.actual\n '\n \n test_expect_success 'extra delim packet in v2 fetch args' '\n@@ -27,9 +30,12 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n \t\t# protocol expects 0000 flush here\n \t\tprintf 0001\n \t} >input &&\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: expected flush after fetch arguments\n+\tEOF\n \ttest_must_fail env GIT_PROTOCOL=version=2 \\\n-\t\tgit upload-pack . err &&\n-\ttest_i18ngrep \"expected flush after fetch arguments\" err\n+\t\tgit upload-pack . err.actual &&\n+\ttest_cmp err.expect err.actual\n '\n \n test_expect_success 'extra delim packet in v2 object-info args' '\n-- \n2.32.0-dev\n\n"},{"id":"429771","messageId":"patch-3.5-c33f344ab20-20210712T164208Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com","subject":"[PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-12T16:44:18Z","receivedAt":"2021-07-12T16:44:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The shell+perl \"[de]packetize()\" helper functions were added in\n4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n2018-01-24), and around the same time we added the \"pkt-line\" helper\nin 74e70029615 (test-pkt-line: introduce a packet-line test helper,\n2018-03-14).\n\nFor some reason it seems we've mostly used the shell+perl version\ninstead of the helper since then. There were discussions around\n88124ab2636 (test-lib-functions: make packetize() more efficient,\n2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()\nstdin code, 2020-03-29) to improve them and make them more efficient.\n\nLet's instead just use the test helper, I think this results in both\nmore legible code, and for anyone who cares about efficiency it'll be\nfaster.\n\nWe can't convert all the users of packetize(), it has a feature the\ntest-tool is missing. This'll be addressed in the subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5410-receive-pack-alternates.sh | 42 ++++++++++++++++++--------\n t/t5411/once-0010-report-status-v1.sh | 8 ++---\n t/t5500-fetch-pack.sh | 15 +++++----\n t/t5530-upload-pack-error.sh | 24 ++++++++-------\n t/t5562-http-backend-content-length.sh | 13 ++++----\n t/t5570-git-daemon.sh | 12 +++++---\n t/t5704-protocol-violations.sh | 30 +++++++++---------\n 7 files changed, 86 insertions(+), 58 deletions(-)\n\ndiff --git a/t/t5410-receive-pack-alternates.sh b/t/t5410-receive-pack-alternates.sh\nindex 0b28e4e452f..d0053d95a44 100755\n--- a/t/t5410-receive-pack-alternates.sh\n+++ b/t/t5410-receive-pack-alternates.sh\n@@ -16,10 +16,6 @@ test_expect_success 'setup' '\n \ttest_commit private\n '\n \n-extract_haves () {\n-\tdepacketize | perl -lne '/^(\\S+) \\.have/ and print $1'\n-}\n-\n test_expect_success 'with core.alternateRefsCommand' '\n \twrite_script fork/alternate-refs <<-\\EOF &&\n \t\tgit --git-dir=\"$1\" for-each-ref \\\n@@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '\n \t\t\trefs/heads/public/\n \tEOF\n \ttest_config -C fork core.alternateRefsCommand ./alternate-refs &&\n-\tgit rev-parse public/branch >expect &&\n-\tprintf \"0000\" | git receive-pack fork >actual &&\n-\textract_haves actual.haves &&\n-\ttest_cmp expect actual.haves\n+\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main) refs/heads/main\n+\t$(git rev-parse base) refs/tags/base\n+\t$(git rev-parse public) .have\n+\t0000\n+\tEOF\n+\n+\tgit receive-pack fork >out actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'with core.alternateRefsPrefixes' '\n \ttest_config -C fork core.alternateRefsPrefixes \"refs/heads/private\" &&\n-\tgit rev-parse private/branch >expect &&\n-\tprintf \"0000\" | git receive-pack fork >actual &&\n-\textract_haves actual.haves &&\n-\ttest_cmp expect actual.haves\n+\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main) refs/heads/main\n+\t$(git rev-parse base) refs/tags/base\n+\t$(git rev-parse private) .have\n+\t0000\n+\tEOF\n+\n+\tgit receive-pack fork >out actual &&\n+\ttest_cmp expect actual\n '\n \n test_done\ndiff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\nindex 1233a46eac5..cf33d993192 100644\n--- a/t/t5411/once-0010-report-status-v1.sh\n+++ b/t/t5411/once-0010-report-status-v1.sh\n@@ -34,13 +34,13 @@ test_expect_success \"proc-receive: report status v1\" '\n \t\t\t\t$A $B | packetize\n \t\tfi &&\n \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n \t\tprintf \"%s %s refs/heads/foo\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n \t\tprintf \"%s %s refs/for/next/topic\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n \t\tprintf \"%s %s refs/for/main/topic2\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n \t\tprintf 0000 &&\n \t\tprintf \"\" | git -C \"$upstream\" pack-objects --stdout\n \t} | git receive-pack \"$upstream\" --stateless-rpc \\\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 8a5d3492c71..ff0b7dd89f9 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -872,14 +872,17 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '\n \tgit commit-graph write --reachable &&\n \tgit config core.commitGraph true &&\n \n-\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n-\t0012command=fetch\n-\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n-\t00010013deepen-since 1\n-\t$(echo \"want $(git rev-parse other)\" | packetize)\n-\t$(echo \"have $(git rev-parse main)\" | packetize)\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\tdeepen-since 1\n+\twant $(git rev-parse other)\n+\thave $(git rev-parse main)\n \t0000\n \tEOF\n+\n+\tGIT_PROTOCOL=version=2 git upload-pack . /dev/null\n \t)\n '\n \ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 7c1460eaa99..8ccaae10475 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -90,18 +90,20 @@ test_expect_success 'upload-pack fails due to error in pack-objects enumeration'\n \n test_expect_success 'upload-pack tolerates EOF just after stateless client wants' '\n \ttest_commit initial &&\n-\thead=$(git rev-parse HEAD) &&\n-\n-\t{\n-\t\tpacketize \"want $head\" &&\n-\t\tpacketize \"shallow $head\" &&\n-\t\tpacketize \"deepen 1\" &&\n-\t\tprintf \"0000\"\n-\t} >request &&\n \n-\tprintf \"0000\" >expect &&\n-\n-\tgit upload-pack --stateless-rpc . actual &&\n+\thead=$(git rev-parse HEAD) &&\n+\ttest-tool pkt-line pack >request <<-EOF &&\n+\twant $head\n+\tshallow $head\n+\tdeepen 1\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\t0000\n+\tEOF\n+\tgit upload-pack --stateless-rpc . out &&\n+\ttest-tool pkt-line unpack actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex e5d3d15ba8d..e6c8338b648 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -53,12 +53,13 @@ test_expect_success 'setup' '\n \ttest_commit c1 &&\n \thash_head=$(git rev-parse HEAD) &&\n \thash_prev=$(git rev-parse HEAD~1) &&\n-\t{\n-\t\tpacketize \"want $hash_head\" &&\n-\t\tprintf 0000 &&\n-\t\tpacketize \"have $hash_prev\" &&\n-\t\tpacketize \"done\"\n-\t} >fetch_body &&\n+\ttest-tool pkt-line pack >fetch_body <<-EOF &&\n+\twant $hash_head\n+\t0000\n+\thave $hash_prev\n+\tdone\n+\t0000\n+\tEOF\n \ttest_copy_bytes 10 fetch_body.trunc &&\n \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n \t{\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 82c31ab6cd8..2dde0348816 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n \t\tprintf \"0000\"\n \t} >input &&\n \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n-\tdepacketize output.raw &&\n+\ttest-tool pkt-line unpack actual &&\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD) HEAD\n+\t$(git rev-parse refs/heads/main) refs/heads/main\n+\t0000\n+\tEOF\n \n-\t# just pick out the value of main, which avoids any protocol\n-\t# particulars\n-\tperl -lne \"print \\$1 if m{^(\\\\S+) refs/heads/main}\" actual &&\n-\tgit -C \"$repo\" rev-parse main >expect &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 038fffd3d03..44e2c0d3ded 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -7,13 +7,14 @@ making sure that we do not segfault or otherwise behave badly.'\n . ./test-lib.sh\n \n test_expect_success 'extra delim packet in v2 ls-refs args' '\n-\t{\n-\t\tpacketize command=ls-refs &&\n-\t\tpacketize \"object-format=$(test_oid algo)\" &&\n-\t\tprintf 0001 &&\n-\t\t# protocol expects 0000 flush here\n-\t\tprintf 0001\n-\t} >input &&\n+\t# protocol expects 0000 flush after the 0001\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=ls-refs\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\t0001\n+\tEOF\n+\n \tcat >err.expect <<-\\EOF &&\n \tfatal: expected flush after ls-refs arguments\n \tEOF\n@@ -23,13 +24,14 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '\n '\n \n test_expect_success 'extra delim packet in v2 fetch args' '\n-\t{\n-\t\tpacketize command=fetch &&\n-\t\tpacketize \"object-format=$(test_oid algo)\" &&\n-\t\tprintf 0001 &&\n-\t\t# protocol expects 0000 flush here\n-\t\tprintf 0001\n-\t} >input &&\n+\t# protocol expects 0000 flush after the 0001\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\t0001\n+\tEOF\n+\n \tcat >err.expect <<-\\EOF &&\n \tfatal: expected flush after fetch arguments\n \tEOF\n-- \n2.32.0-dev\n\n"},{"id":"429772","messageId":"patch-4.5-a44e1790f2a-20210712T164208Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com","subject":"[PATCH v2 4/5] tests: replace remaining packetize() with \"test-tool pkt-line\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-12T16:44:19Z","receivedAt":"2021-07-12T16:44:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the only remaining users of \"packetize()\" over to \"test-tool\npkt-line\", for this we need a new \"pack-raw-stdin\" subcommand in the\ntest-tool. The \"pack\" command takes input on stdin, but splits it by\n\"\\n\", furthermore we'll format the output using C-strings, so the\nembedded \"\\0\" being tested for here would cause the string to be\ntruncated.\n\nSo we need another mode that just calls packet_write() on the raw\ninput we got on stdin.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/helper/test-pkt-line.c | 12 ++++++++++++\n t/t5411/once-0010-report-status-v1.sh | 4 ++--\n t/t5562-http-backend-content-length.sh | 3 ++-\n t/t5570-git-daemon.sh | 10 ++++++----\n 4 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex 5e638f0b970..8563a0da4c5 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -26,6 +26,16 @@ static void pack(int argc, const char **argv)\n \t}\n }\n \n+static void pack_raw_stdin(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstrbuf_read(&sb, 0, 0);\n+\tif (strbuf_read(&sb, 0, 0) < 0)\n+\t\tdie_errno(\"failed to read from stdin\");\n+\tpacket_write(1, sb.buf, sb.len);\n+\tstrbuf_release(&sb);\n+}\n+\n static void unpack(void)\n {\n \tstruct packet_reader reader;\n@@ -110,6 +120,8 @@ int cmd__pkt_line(int argc, const char **argv)\n \n \tif (!strcmp(argv[1], \"pack\"))\n \t\tpack(argc - 2, argv + 2);\n+\telse if (!strcmp(argv[1], \"pack-raw-stdin\"))\n+\t\tpack_raw_stdin();\n \telse if (!strcmp(argv[1], \"unpack\"))\n \t\tunpack();\n \telse if (!strcmp(argv[1], \"unpack-sideband\"))\ndiff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\nindex cf33d993192..75d4233e49f 100644\n--- a/t/t5411/once-0010-report-status-v1.sh\n+++ b/t/t5411/once-0010-report-status-v1.sh\n@@ -28,10 +28,10 @@ test_expect_success \"proc-receive: report status v1\" '\n \t\tif test -z \"$GIT_DEFAULT_HASH\" || test \"$GIT_DEFAULT_HASH\" = \"sha1\"\n \t\tthen\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status\\n\" \\\n-\t\t\t\t$A $B | packetize\n+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n \t\telse\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status object-format=$GIT_DEFAULT_HASH\\n\" \\\n-\t\t\t\t$A $B | packetize\n+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n \t\tfi &&\n \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n \t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex e6c8338b648..23a8a8d5c70 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -64,7 +64,8 @@ test_expect_success 'setup' '\n \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n \t{\n \t\tprintf \"%s %s refs/heads/newbranch\\\\0report-status object-format=%s\\\\n\" \\\n-\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize &&\n+\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" |\n+\t\t\ttest-tool pkt-line pack-raw-stdin &&\n \t\tprintf 0000 &&\n \t\techo \"$hash_next\" | git pack-objects --stdout\n \t} >push_body &&\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 2dde0348816..b52afb0cdea 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -193,10 +193,12 @@ test_expect_success 'hostname cannot break out of directory' '\n '\n \n test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n-\t{\n-\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n-\t\tprintf \"0000\"\n-\t} >input &&\n+\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" >has-null &&\n+\ttest-tool pkt-line pack-raw-stdin >input >input <<-\\EOF &&\n+\t0000\n+\tEOF\n+\n \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n \ttest-tool pkt-line unpack actual &&\n \n-- \n2.32.0-dev\n\n"},{"id":"429773","messageId":"patch-5.5-cc91d15ef70-20210712T164208Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com","subject":"[PATCH v2 5/5] test-lib-functions.sh: remove unused [de]packetize() functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-12T16:44:20Z","receivedAt":"2021-07-12T16:44:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the now-unused \"packetize()\" and \"depacketize()\" functions\nadded in 4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n2018-01-24). As discussed in the preceding commits we've now moved all\ntheir users over to \"test-tool pkt-line\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib-functions.sh | 42 -----------------------------------------\n 1 file changed, 42 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex b2810478a21..77e4434dcda 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1453,48 +1453,6 @@ nongit () {\n \t)\n } 7>&2 2>&4\n \n-# convert function arguments or stdin (if not arguments given) to pktline\n-# representation. If multiple arguments are given, they are separated by\n-# whitespace and put in a single packet. Note that data containing NULs must be\n-# given on stdin, and that empty input becomes an empty packet, not a flush\n-# packet (for that you can just print 0000 yourself).\n-packetize () {\n-\tif test $# -gt 0\n-\tthen\n-\t\tpacket=\"$*\"\n-\t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n-\telse\n-\t\tperl -e '\n-\t\t\tmy $packet = do { local $/; };\n-\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n-\t\t'\n-\tfi\n-}\n-\n-# Parse the input as a series of pktlines, writing the result to stdout.\n-# Sideband markers are removed automatically, and the output is routed to\n-# stderr if appropriate.\n-#\n-# NUL bytes are converted to \"\\\\0\" for ease of parsing with text tools.\n-depacketize () {\n-\tperl -e '\n-\t\twhile (read(STDIN, $len, 4) == 4) {\n-\t\t\tif ($len eq \"0000\") {\n-\t\t\t\tprint \"FLUSH\\n\";\n-\t\t\t} else {\n-\t\t\t\tread(STDIN, $buf, hex($len) - 4);\n-\t\t\t\t$buf =~ s/\\0/\\\\0/g;\n-\t\t\t\tif ($buf =~ s/^[\\x2\\x3]//) {\n-\t\t\t\t\tprint STDERR $buf;\n-\t\t\t\t} else {\n-\t\t\t\t\t$buf =~ s/^\\x1//;\n-\t\t\t\t\tprint $buf;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n-\t'\n-}\n-\n # Converts base-16 data into base-8. The output is given as a sequence of\n # escaped octals, suitable for consumption by 'printf'.\n hex2oct () {\n-- \n2.32.0-dev\n\n"},{"id":"429832","messageId":"xmqqpmvn2rkm.fsf@gitster.g","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/5] tests: migrate to \"test-tool pkt-line\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-12T20:41:13Z","receivedAt":"2021-07-12T20:41:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This series is marked for \"will merge to next\" already, but not there\n> yet. A trivial v2 whitespace fix in case Junio's in time to pick it\n> up.\n>\n> See v1 at https://lore.kernel.org/git/cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com/\n>\n> Ævar Arnfjörð Bjarmason (5):\n> serve tests: add missing \"extra delim\" test\n> serve tests: use test_cmp in \"protocol violations\" test\n> tests: replace [de]packetize() shell+perl test-tool pkt-line\n> tests: replace remaining packetize() with \"test-tool pkt-line\"\n> test-lib-functions.sh: remove unused [de]packetize() functions\n>\n> t/helper/test-pkt-line.c | 12 +++++\n> t/t5410-receive-pack-alternates.sh | 42 +++++++++++++-----\n> t/t5411/once-0010-report-status-v1.sh | 12 ++---\n> t/t5500-fetch-pack.sh | 15 ++++---\n> t/t5530-upload-pack-error.sh | 24 +++++-----\n> t/t5562-http-backend-content-length.sh | 16 ++++---\n> t/t5570-git-daemon.sh | 22 ++++++----\n> t/t5704-protocol-violations.sh | 61 ++++++++++++++++++--------\n> t/test-lib-functions.sh | 42 ------------------\n> 9 files changed, 135 insertions(+), 111 deletions(-)\n>\n> Range-diff against v1:\n> 1: fcb53980597 = 1: 67aa8141153 serve tests: add missing \"extra delim\" test\n> 2: c3544fb53cd = 2: 64dfd14865c serve tests: use test_cmp in \"protocol violations\" test\n> 3: c1015fa6ab0 = 3: c33f344ab20 tests: replace [de]packetize() shell+perl test-tool pkt-line\n> 4: ab23513b48b ! 4: a44e1790f2a tests: replace remaining packetize() with \"test-tool pkt-line\"\n> @@ t/helper/test-pkt-line.c: static void pack(int argc, const char **argv)\n> \n> +static void pack_raw_stdin(void)\n> +{\n> -+\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstrbuf_read(&sb, 0, 0);\n> +\tif (strbuf_read(&sb, 0, 0) < 0)\n> 5: 2d22b83971a = 5: cc91d15ef70 test-lib-functions.sh: remove unused [de]packetize() functions\n\nWell, if you care that much, I'd prefer to also see an blank line\nbetween the end of decl and the first statement.\n\n;-)\n"},{"id":"429931","messageId":"YO38ladlVVGAmQnl@coredump.intra.peff.net","threadId":"56071","inReplyTo":"patch-3.5-c33f344ab20-20210712T164208Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-13T20:50:29Z","receivedAt":"2021-07-13T20:50:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 12, 2021 at 06:44:18PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> The shell+perl \"[de]packetize()\" helper functions were added in\n> 4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n> 2018-01-24), and around the same time we added the \"pkt-line\" helper\n> in 74e70029615 (test-pkt-line: introduce a packet-line test helper,\n> 2018-03-14).\n> \n> For some reason it seems we've mostly used the shell+perl version\n> instead of the helper since then. There were discussions around\n> 88124ab2636 (test-lib-functions: make packetize() more efficient,\n> 2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()\n> stdin code, 2020-03-29) to improve them and make them more efficient.\n\nThis seems like a good goal, and the conversions look mostly faithful\n(with one exception below). Having the helper tool recognize \"0000\" as a\nflush makes some of the scripts much nicer to read (even if it is\ntechnically ambiguous in the input).\n\nI did seem like there were some other stylistic things happening, too,\nthough. For instance:\n\n> -extract_haves () {\n> -\tdepacketize | perl -lne '/^(\\S+) \\.have/ and print $1'\n> -}\n> -\n> test_expect_success 'with core.alternateRefsCommand' '\n> \twrite_script fork/alternate-refs <<-\\EOF &&\n> \t\tgit --git-dir=\"$1\" for-each-ref \\\n> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '\n> \t\t\trefs/heads/public/\n> \tEOF\n> \ttest_config -C fork core.alternateRefsCommand ./alternate-refs &&\n> -\tgit rev-parse public/branch >expect &&\n> -\tprintf \"0000\" | git receive-pack fork >actual &&\n> -\textract_haves actual.haves &&\n> -\ttest_cmp expect actual.haves\n> +\n> +\ttest-tool pkt-line pack >in <<-\\EOF &&\n> +\t0000\n> +\tEOF\n> +\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse main) refs/heads/main\n> +\t$(git rev-parse base) refs/tags/base\n> +\t$(git rev-parse public) .have\n> +\t0000\n> +\tEOF\n\nThis is testing the whole output, rather than just the \"have\" lines (as\nthe original did). It was intentionally written the other way for two\nreasons:\n\n - it keeps the focus on what we are actually testing: the .have\n behavior\n\n - it makes the test less brittle to other changes in the script\n\nI can buy the argument that sometimes testing the whole output, even if\nit is more brittle, helps us find other unexpected outcomes (e.g., the\nstderr test_cmp vs grep thing earlier in the series). But here it just\nseems strictly worse to me.\n\nIt would be easy to just replace depacketize with \"pktline unpack\", but\nkeep the perl one-liner. I don't think it's a _huge_ deal in this case\neither way, but I'm not enthused about the trend.\n\n> +\ttest-tool pkt-line pack >in <<-\\EOF &&\n> +\t0000\n> +\tEOF\n\nThis used to just be \"printf 0000\". The new version is longer and less\nefficient, but I'm OK with it on the grounds of consistency (all inputs\nflow through \"pkt-line pack\", and all outputs through \"pkt-line unpack\").\n\n> diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\n> index 1233a46eac5..cf33d993192 100644\n> --- a/t/t5411/once-0010-report-status-v1.sh\n> +++ b/t/t5411/once-0010-report-status-v1.sh\n> @@ -34,13 +34,13 @@ test_expect_success \"proc-receive: report status v1\" '\n> \t\t\t\t$A $B | packetize\n> \t\tfi &&\n> \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n> -\t\t\t$ZERO_OID $A | packetize &&\n> +\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n> \t\tprintf \"%s %s refs/heads/foo\\n\" \\\n> -\t\t\t$ZERO_OID $A | packetize &&\n> +\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n> \t\tprintf \"%s %s refs/for/next/topic\\n\" \\\n> -\t\t\t$ZERO_OID $A | packetize &&\n> +\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n> \t\tprintf \"%s %s refs/for/main/topic2\\n\" \\\n> -\t\t\t$ZERO_OID $A | packetize &&\n> +\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n\nNow that you're using the multi-line-capable helper, these could all be\na single (and much more readable):\n\n test-tool pkt-line pack <<-EOF\n $ZERO_OID $A refs/for/main/topic1\n $ZERO_OID $A refs/heads/foo\n $ZERO_OID $A refs/for/next/topic\n $ZERO_OID $A refs/for/main/topic2\n EOF\n\ncouldn't they?\n\n> -\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n> -\t0012command=fetch\n> -\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n> -\t00010013deepen-since 1\n> -\t$(echo \"want $(git rev-parse other)\" | packetize)\n> -\t$(echo \"have $(git rev-parse main)\" | packetize)\n> +\ttest-tool pkt-line pack >in <<-EOF &&\n> +\tcommand=fetch\n> +\tobject-format=$(test_oid algo)\n> +\t0001\n> +\tdeepen-since 1\n> +\twant $(git rev-parse other)\n> +\thave $(git rev-parse main)\n> \t0000\n> \tEOF\n\nThis is much more readable. Nice.\n\n> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n> index e5d3d15ba8d..e6c8338b648 100755\n> --- a/t/t5562-http-backend-content-length.sh\n> +++ b/t/t5562-http-backend-content-length.sh\n> @@ -53,12 +53,13 @@ test_expect_success 'setup' '\n> \ttest_commit c1 &&\n> \thash_head=$(git rev-parse HEAD) &&\n> \thash_prev=$(git rev-parse HEAD~1) &&\n> -\t{\n> -\t\tpacketize \"want $hash_head\" &&\n> -\t\tprintf 0000 &&\n> -\t\tpacketize \"have $hash_prev\" &&\n> -\t\tpacketize \"done\"\n> -\t} >fetch_body &&\n> +\ttest-tool pkt-line pack >fetch_body <<-EOF &&\n> +\twant $hash_head\n> +\t0000\n> +\thave $hash_prev\n> +\tdone\n> +\t0000\n> +\tEOF\n\nThere's a flush packet at the end of your input in the post-image that\ndoesn't seem to be in the original.\n\n> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n> index 82c31ab6cd8..2dde0348816 100755\n> --- a/t/t5570-git-daemon.sh\n> +++ b/t/t5570-git-daemon.sh\n> @@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n> \t\tprintf \"0000\"\n> \t} >input &&\n\nThis one doesn't use \"pkt-line pack\". Which is good, because we care\nabout being exact about things like newlines here.\n\n> \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n> -\tdepacketize output.raw &&\n> +\ttest-tool pkt-line unpack actual &&\n> +\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse HEAD) HEAD\n> +\t$(git rev-parse refs/heads/main) refs/heads/main\n> +\t0000\n> +\tEOF\n> \n> -\t# just pick out the value of main, which avoids any protocol\n> -\t# particulars\n> -\tperl -lne \"print \\$1 if m{^(\\\\S+) refs/heads/main}\" actual &&\n> -\tgit -C \"$repo\" rev-parse main >expect &&\n> \ttest_cmp expect actual\n> '\n\nThis is another case where you're checking the output for more than the\noriginal did, ignoring the comment. :) When using depacketize, the bits\nafter the \"\\0\" were encoded and kept, so it was necessary. The pkt-line\nhelper just throws those bits away, so it's OK (I'm a little surprised\nthat discarding those bits wasn't a roadblock for converting some tests,\nbut I guess we didn't have any low-level protocol tests that cared).\n\n-Peff\n"},{"id":"429932","messageId":"YO3+dBHL5ZBe/NbU@coredump.intra.peff.net","threadId":"56071","inReplyTo":"patch-4.5-a44e1790f2a-20210712T164208Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/5] tests: replace remaining packetize() with \"test-tool pkt-line\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-13T20:58:28Z","receivedAt":"2021-07-13T20:58:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 12, 2021 at 06:44:19PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Move the only remaining users of \"packetize()\" over to \"test-tool\n> pkt-line\", for this we need a new \"pack-raw-stdin\" subcommand in the\n> test-tool. The \"pack\" command takes input on stdin, but splits it by\n> \"\\n\", furthermore we'll format the output using C-strings, so the\n> embedded \"\\0\" being tested for here would cause the string to be\n> truncated.\n> \n> So we need another mode that just calls packet_write() on the raw\n> input we got on stdin.\n\nMakes sense. Lacking this \"raw\" version was the sticking point in the\npast for using the helper in more places.\n\n> +static void pack_raw_stdin(void)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstrbuf_read(&sb, 0, 0);\n> +\tif (strbuf_read(&sb, 0, 0) < 0)\n> +\t\tdie_errno(\"failed to read from stdin\");\n\nWhat's up with the two reads here?\n\nIt looks like just a duplication. And it happens to work because each\none tries to read to eof, making the second one generally a noop.\n\n> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n> index 2dde0348816..b52afb0cdea 100755\n> --- a/t/t5570-git-daemon.sh\n> +++ b/t/t5570-git-daemon.sh\n> @@ -193,10 +193,12 @@ test_expect_success 'hostname cannot break out of directory' '\n> '\n> \n> test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n> -\t{\n> -\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n> -\t\tprintf \"0000\"\n> -\t} >input &&\n> +\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" >has-null &&\n> +\ttest-tool pkt-line pack-raw-stdin >input +\ttest-tool pkt-line pack >>input <<-\\EOF &&\n> +\t0000\n> +\tEOF\n\nThis is a minor style nit, but I really prefer redirecting output from\na block (as in the original) rather than iterative appending (in the\nresult). IMHO it makes it more obvious what is going into the file and\nwhat is not.\n\nI.e.:\n\n {\n printf \"git-upload-pack /interp.git\\n\\0host=localhost\" |\n\t test-tool pkt-line pack-raw-stdin &&\n\t printf \"0000\" | test-tool pkt-line pack\n } >input\n\n(again here the packing of \"0000\" is pointless, but I'm OK with it for\nconsistency).\n\n-Peff\n"},{"id":"429933","messageId":"YO3+3F5fRONVtqTI@coredump.intra.peff.net","threadId":"56071","inReplyTo":"xmqqpmvn2rkm.fsf@gitster.g","subject":"Re: [PATCH v2 0/5] tests: migrate to \"test-tool pkt-line\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-13T21:00:12Z","receivedAt":"2021-07-13T21:00:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 12, 2021 at 01:41:13PM -0700, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > This series is marked for \"will merge to next\" already, but not there\n> > yet. A trivial v2 whitespace fix in case Junio's in time to pick it\n> > up.\n> >\n> > See v1 at https://lore.kernel.org/git/cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com/\n> >\n> > Ævar Arnfjörð Bjarmason (5):\n> > serve tests: add missing \"extra delim\" test\n> > serve tests: use test_cmp in \"protocol violations\" test\n> > tests: replace [de]packetize() shell+perl test-tool pkt-line\n> > tests: replace remaining packetize() with \"test-tool pkt-line\"\n> > test-lib-functions.sh: remove unused [de]packetize() functions\n> >\n> > t/helper/test-pkt-line.c | 12 +++++\n> > t/t5410-receive-pack-alternates.sh | 42 +++++++++++++-----\n> > t/t5411/once-0010-report-status-v1.sh | 12 ++---\n> > t/t5500-fetch-pack.sh | 15 ++++---\n> > t/t5530-upload-pack-error.sh | 24 +++++-----\n> > t/t5562-http-backend-content-length.sh | 16 ++++---\n> > t/t5570-git-daemon.sh | 22 ++++++----\n> > t/t5704-protocol-violations.sh | 61 ++++++++++++++++++--------\n> > t/test-lib-functions.sh | 42 ------------------\n> > 9 files changed, 135 insertions(+), 111 deletions(-)\n> >\n> > Range-diff against v1:\n> > 1: fcb53980597 = 1: 67aa8141153 serve tests: add missing \"extra delim\" test\n> > 2: c3544fb53cd = 2: 64dfd14865c serve tests: use test_cmp in \"protocol violations\" test\n> > 3: c1015fa6ab0 = 3: c33f344ab20 tests: replace [de]packetize() shell+perl test-tool pkt-line\n> > 4: ab23513b48b ! 4: a44e1790f2a tests: replace remaining packetize() with \"test-tool pkt-line\"\n> > @@ t/helper/test-pkt-line.c: static void pack(int argc, const char **argv)\n> > \n> > +static void pack_raw_stdin(void)\n> > +{\n> > -+\n> > +\tstruct strbuf sb = STRBUF_INIT;\n> > +\tstrbuf_read(&sb, 0, 0);\n> > +\tif (strbuf_read(&sb, 0, 0) < 0)\n> > 5: 2d22b83971a = 5: cc91d15ef70 test-lib-functions.sh: remove unused [de]packetize() functions\n> \n> Well, if you care that much, I'd prefer to also see an blank line\n> between the end of decl and the first statement.\n\nI was offline last week and hadn't had a chance to look until today. I\ndid find a few things. Mostly style nits that could potentially be\nignored, but two small \"correctness\" things that somehow manage to not\ncause any test failures. :)\n\n-Peff\n"},{"id":"429953","messageId":"875yxd3h7c.fsf@evledraar.gmail.com","threadId":"56071","inReplyTo":"YO38ladlVVGAmQnl@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-13T23:41:27Z","receivedAt":"2021-07-13T23:52:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 13 2021, Jeff King wrote:\n\n> On Mon, Jul 12, 2021 at 06:44:18PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> The shell+perl \"[de]packetize()\" helper functions were added in\n>> 4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n>> 2018-01-24), and around the same time we added the \"pkt-line\" helper\n>> in 74e70029615 (test-pkt-line: introduce a packet-line test helper,\n>> 2018-03-14).\n>> \n>> For some reason it seems we've mostly used the shell+perl version\n>> instead of the helper since then. There were discussions around\n>> 88124ab2636 (test-lib-functions: make packetize() more efficient,\n>> 2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()\n>> stdin code, 2020-03-29) to improve them and make them more efficient.\n>\n> This seems like a good goal, and the conversions look mostly faithful\n> (with one exception below). Having the helper tool recognize \"0000\" as a\n> flush makes some of the scripts much nicer to read (even if it is\n> technically ambiguous in the input).\n>\n> I did seem like there were some other stylistic things happening, too,\n> though. For instance:\n>\n>> -extract_haves () {\n>> -\tdepacketize | perl -lne '/^(\\S+) \\.have/ and print $1'\n>> -}\n>> -\n>> test_expect_success 'with core.alternateRefsCommand' '\n>> \twrite_script fork/alternate-refs <<-\\EOF &&\n>> \t\tgit --git-dir=\"$1\" for-each-ref \\\n>> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '\n>> \t\t\trefs/heads/public/\n>> \tEOF\n>> \ttest_config -C fork core.alternateRefsCommand ./alternate-refs &&\n>> -\tgit rev-parse public/branch >expect &&\n>> -\tprintf \"0000\" | git receive-pack fork >actual &&\n>> -\textract_haves actual.haves &&\n>> -\ttest_cmp expect actual.haves\n>> +\n>> +\ttest-tool pkt-line pack >in <<-\\EOF &&\n>> +\t0000\n>> +\tEOF\n>> +\n>> +\tcat >expect <<-EOF &&\n>> +\t$(git rev-parse main) refs/heads/main\n>> +\t$(git rev-parse base) refs/tags/base\n>> +\t$(git rev-parse public) .have\n>> +\t0000\n>> +\tEOF\n>\n> This is testing the whole output, rather than just the \"have\" lines (as\n> the original did). It was intentionally written the other way for two\n> reasons:\n>\n> - it keeps the focus on what we are actually testing: the .have\n> behavior\n>\n> - it makes the test less brittle to other changes in the script\n>\n> I can buy the argument that sometimes testing the whole output, even if\n> it is more brittle, helps us find other unexpected outcomes (e.g., the\n> stderr test_cmp vs grep thing earlier in the series). But here it just\n> seems strictly worse to me.\n>\n> It would be easy to just replace depacketize with \"pktline unpack\", but\n> keep the perl one-liner. I don't think it's a _huge_ deal in this case\n> either way, but I'm not enthused about the trend.\n\nFWIW this series was spun off from an effort of fixing a bug related to\nprotocol-y tests doing just such a \"we can grep the output, we know we\nonly need xyz\", and the only test for it not failing because we picked\nthe wrong xyz.\n\nIn this case yeah we could keep the grep. I do think in general that\nunless output is overly verbose we should test_cmp it, and in this case\nit's really not.\n\nOn the focus it seems it's the opposite for the two uf us. It takes my\nfocus away from the test itself when reading it. I.e. I start wondering\nwhy the grep, is the output variable or whatever, especially in a case\nlike this where we're hardly saving ourselves overall lines or reducing\ncomplexity.\n\n>> +\ttest-tool pkt-line pack >in <<-\\EOF &&\n>> +\t0000\n>> +\tEOF\n>\n> This used to just be \"printf 0000\". The new version is longer and less\n> efficient, but I'm OK with it on the grounds of consistency (all inputs\n> flow through \"pkt-line pack\", and all outputs through \"pkt-line unpack\").\n\nThey aren't equivalent because the pkt-line helper (and pkt-line.c in\ngeneral) will be forgiving about the presence or lack of trailing\nnewlines, but some of these tests were not.\n\nI think we should use the helper in/out for all of those, because we're\nexplicitly interested if the protocol round-trips the way we expect, and\nnot per-se if the exact bytes match.\n\n>> diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\n>> index 1233a46eac5..cf33d993192 100644\n>> --- a/t/t5411/once-0010-report-status-v1.sh\n>> +++ b/t/t5411/once-0010-report-status-v1.sh\n>> @@ -34,13 +34,13 @@ test_expect_success \"proc-receive: report status v1\" '\n>> \t\t\t\t$A $B | packetize\n>> \t\tfi &&\n>> \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n>> -\t\t\t$ZERO_OID $A | packetize &&\n>> +\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n>> \t\tprintf \"%s %s refs/heads/foo\\n\" \\\n>> -\t\t\t$ZERO_OID $A | packetize &&\n>> +\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n>> \t\tprintf \"%s %s refs/for/next/topic\\n\" \\\n>> -\t\t\t$ZERO_OID $A | packetize &&\n>> +\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n>> \t\tprintf \"%s %s refs/for/main/topic2\\n\" \\\n>> -\t\t\t$ZERO_OID $A | packetize &&\n>> +\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n>\n> Now that you're using the multi-line-capable helper, these could all be\n> a single (and much more readable):\n>\n> test-tool pkt-line pack <<-EOF\n> $ZERO_OID $A refs/for/main/topic1\n> $ZERO_OID $A refs/heads/foo\n> $ZERO_OID $A refs/for/next/topic\n> $ZERO_OID $A refs/for/main/topic2\n> EOF\n>\n> couldn't they?\n\nYeah, but you never know what you'll get \"let's do the small change\"\nv.s. \"let's avoid refactoring while we're at it\" feedback :)\n\nI figured it was easier to review with just the s/packetize/test-tool\npkt-line pack/g, but sure, I can change it.\n\n>> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n>> index e5d3d15ba8d..e6c8338b648 100755\n>> --- a/t/t5562-http-backend-content-length.sh\n>> +++ b/t/t5562-http-backend-content-length.sh\n>> @@ -53,12 +53,13 @@ test_expect_success 'setup' '\n>> \ttest_commit c1 &&\n>> \thash_head=$(git rev-parse HEAD) &&\n>> \thash_prev=$(git rev-parse HEAD~1) &&\n>> -\t{\n>> -\t\tpacketize \"want $hash_head\" &&\n>> -\t\tprintf 0000 &&\n>> -\t\tpacketize \"have $hash_prev\" &&\n>> -\t\tpacketize \"done\"\n>> -\t} >fetch_body &&\n>> +\ttest-tool pkt-line pack >fetch_body <<-EOF &&\n>> +\twant $hash_head\n>> +\t0000\n>> +\thave $hash_prev\n>> +\tdone\n>> +\t0000\n>> +\tEOF\n>\n> There's a flush packet at the end of your input in the post-image that\n> doesn't seem to be in the original.\n\nYeah, but isn't round-tripping through the helper the right thing here?\n\n>> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n>> index 82c31ab6cd8..2dde0348816 100755\n>> --- a/t/t5570-git-daemon.sh\n>> +++ b/t/t5570-git-daemon.sh\n>> @@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n>> \t\tprintf \"0000\"\n>> \t} >input &&\n>\n> This one doesn't use \"pkt-line pack\". Which is good, because we care\n> about being exact about things like newlines here.\n\nYes, as opposed to here, where we don't want the helper.\n\n>> \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n>> -\tdepacketize output.raw &&\n>> +\ttest-tool pkt-line unpack actual &&\n>> +\n>> +\tcat >expect <<-EOF &&\n>> +\t$(git rev-parse HEAD) HEAD\n>> +\t$(git rev-parse refs/heads/main) refs/heads/main\n>> +\t0000\n>> +\tEOF\n>> \n>> -\t# just pick out the value of main, which avoids any protocol\n>> -\t# particulars\n>> -\tperl -lne \"print \\$1 if m{^(\\\\S+) refs/heads/main}\" actual &&\n>> -\tgit -C \"$repo\" rev-parse main >expect &&\n>> \ttest_cmp expect actual\n>> '\n>\n> This is another case where you're checking the output for more than the\n> original did, ignoring the comment. :) When using depacketize, the bits\n> after the \"\\0\" were encoded and kept, so it was necessary. The pkt-line\n> helper just throws those bits away, so it's OK (I'm a little surprised\n> that discarding those bits wasn't a roadblock for converting some tests,\n> but I guess we didn't have any low-level protocol tests that cared).\n\n... I see you got to this bit in 4/5.\n"},{"id":"429954","messageId":"8735sh3h09.fsf@evledraar.gmail.com","threadId":"56071","inReplyTo":"YO3+dBHL5ZBe/NbU@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/5] tests: replace remaining packetize() with \"test-tool pkt-line\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-13T23:52:09Z","receivedAt":"2021-07-13T23:56:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 13 2021, Jeff King wrote:\n\n> On Mon, Jul 12, 2021 at 06:44:19PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Move the only remaining users of \"packetize()\" over to \"test-tool\n>> pkt-line\", for this we need a new \"pack-raw-stdin\" subcommand in the\n>> test-tool. The \"pack\" command takes input on stdin, but splits it by\n>> \"\\n\", furthermore we'll format the output using C-strings, so the\n>> embedded \"\\0\" being tested for here would cause the string to be\n>> truncated.\n>> \n>> So we need another mode that just calls packet_write() on the raw\n>> input we got on stdin.\n>\n> Makes sense. Lacking this \"raw\" version was the sticking point in the\n> past for using the helper in more places.\n>\n>> +static void pack_raw_stdin(void)\n>> +{\n>> +\tstruct strbuf sb = STRBUF_INIT;\n>> +\tstrbuf_read(&sb, 0, 0);\n>> +\tif (strbuf_read(&sb, 0, 0) < 0)\n>> +\t\tdie_errno(\"failed to read from stdin\");\n>\n> What's up with the two reads here?\n>\n> It looks like just a duplication. And it happens to work because each\n> one tries to read to eof, making the second one generally a noop.\n\nYes oops, just a copy/paste error I didn't spot.\n\n>> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n>> index 2dde0348816..b52afb0cdea 100755\n>> --- a/t/t5570-git-daemon.sh\n>> +++ b/t/t5570-git-daemon.sh\n>> @@ -193,10 +193,12 @@ test_expect_success 'hostname cannot break out of directory' '\n>> '\n>> \n>> test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n>> -\t{\n>> -\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n>> -\t\tprintf \"0000\"\n>> -\t} >input &&\n>> +\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" >has-null &&\n>> +\ttest-tool pkt-line pack-raw-stdin >input > +\ttest-tool pkt-line pack >>input <<-\\EOF &&\n>> +\t0000\n>> +\tEOF\n>\n> This is a minor style nit, but I really prefer redirecting output from\n> a block (as in the original) rather than iterative appending (in the\n> result). IMHO it makes it more obvious what is going into the file and\n> what is not.\n>\n> I.e.:\n>\n> {\n> printf \"git-upload-pack /interp.git\\n\\0host=localhost\" |\n> \t test-tool pkt-line pack-raw-stdin &&\n> \t printf \"0000\" | test-tool pkt-line pack\n> } >input\n>\n> (again here the packing of \"0000\" is pointless, but I'm OK with it for\n> consistency).\n\nSure, I can use {} blocks, but re the reply on 3/5 about \"0000\"\nv.s. \"0000\\n\" you'd like to move back to print not-a-newline here\nv.s. having the helper eat lines ending with \\n like everywhere else?\n\nIt's not incorrect in this case, it just seems less obvious to\nme. I.e. the printf in the former case is because we explicitly care\nabout the exact raw input, if there's a trailing \\n or not, in the\nlatter case we don't, so I think it's clearier to use the usual <<-\\EOF\npattern rather than printf.\n"},{"id":"429971","messageId":"patch-1.5-67aa814115-20210714T005115Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-0000000000-20210714T005115Z-avarab@gmail.com","subject":"[PATCH v3 1/5] serve tests: add missing \"extra delim\" test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T00:54:01Z","receivedAt":"2021-07-14T00:54:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When the object-info protocol v2 call was added in\na2ba162cda2 (object-info: support for retrieving object info,\n2021-04-20) we didn't add a corresponding test here.\n\nWe had tests for ls-refs and fetch already since\n4845b772458 (upload-pack: handle unexpected delim packets,\n2020-03-27), but the same behavior in object-info (which is clearly\ncopied from the template of \"ls-refs\") did not have the corresponding\ntests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5704-protocol-violations.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 5c941949b9..95f68e1d4b 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -32,4 +32,21 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n \ttest_i18ngrep \"expected flush after fetch arguments\" err\n '\n \n+test_expect_success 'extra delim packet in v2 object-info args' '\n+\t# protocol expects 0000 flush after the 0001\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=object-info\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\t0001\n+\tEOF\n+\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: object-info: expected flush after arguments\n+\tEOF\n+\ttest_must_fail env GIT_PROTOCOL=version=2 \\\n+\t\tgit upload-pack . err.actual &&\n+\ttest_cmp err.expect err.actual\n+'\n+\n test_done\n-- \n2.32.0.788.ge724008458\n\n"},{"id":"429972","messageId":"cover-0.5-0000000000-20210714T005115Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com","subject":"[PATCH v3 0/5] tests: migrate to \"test-tool pkt-line\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T00:54:00Z","receivedAt":"2021-07-14T00:54:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code that uses [de]packetize() to use \"test-tool\npkt-line\". That we had these two concurrently in the tests is mostly\njust a historical anomaly/inconsistency that we can fix.\n\nHopefully addresses Jeff King's feedback on v2: https://lore.kernel.org/git/cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com/#t\n\nI:\n\n * Changed the t5411/once-0010-report-status-v1.sh pattern to move\n away from the verbose \"printf\"..\n\n * Got rid of the stray strbuf_read()\n\n * Used a {} block in t/t5562-http-backend-content-length.sh\n\nI kept the test_cmp in favor of the old logic of grepping out specific\nparts, now with an updated commit message addressing that in 3/5, and\nthat we consistently include \"0000\" in all cases, even when we can\nomit it.\n\nÆvar Arnfjörð Bjarmason (5):\n serve tests: add missing \"extra delim\" test\n serve tests: use test_cmp in \"protocol violations\" test\n tests: replace [de]packetize() shell+perl test-tool pkt-line\n tests: replace remaining packetize() with \"test-tool pkt-line\"\n test-lib-functions.sh: remove unused [de]packetize() functions\n\n t/helper/test-pkt-line.c | 12 +++++\n t/t5410-receive-pack-alternates.sh | 42 +++++++++++++-----\n t/t5411/once-0010-report-status-v1.sh | 20 ++++-----\n t/t5500-fetch-pack.sh | 15 ++++---\n t/t5530-upload-pack-error.sh | 24 +++++-----\n t/t5562-http-backend-content-length.sh | 16 ++++---\n t/t5570-git-daemon.sh | 20 ++++++---\n t/t5704-protocol-violations.sh | 61 ++++++++++++++++++--------\n t/test-lib-functions.sh | 42 ------------------\n 9 files changed, 138 insertions(+), 114 deletions(-)\n\nRange-diff against v2:\n1: 67aa814115 = 1: 67aa814115 serve tests: add missing \"extra delim\" test\n2: 64dfd14865 = 2: 64dfd14865 serve tests: use test_cmp in \"protocol violations\" test\n3: c33f344ab2 ! 3: 92bfd8a87b tests: replace [de]packetize() shell+perl test-tool pkt-line\n @@ Commit message\n more legible code, and for anyone who cares about efficiency it'll be\n faster.\n \n + The conversion away from extract_haves() in\n + t5410-receive-pack-alternates.sh and the \"just pick out\" logic in\n + t5570-git-daemon.sh isn't strictly required for this migration, but in\n + this case it's easy enough to test_cmp the whole output, so let's just\n + do that.\n +\n + Similarly, there are cases here of changing \"printf 0000\" to a version\n + that'll emit a trailing newline after \"0000\", or where we can do away\n + with the \"0000\" trailer. Let's just always include, and include it in\n + the same way.\n +\n We can't convert all the users of packetize(), it has a feature the\n test-tool is missing. This'll be addressed in the subsequent commit.\n \n @@ t/t5410-receive-pack-alternates.sh: test_expect_success 'with core.alternateRefs\n \n ## t/t5411/once-0010-report-status-v1.sh ##\n @@ t/t5411/once-0010-report-status-v1.sh: test_expect_success \"proc-receive: report status v1\" '\n + \t\t\tprintf \"%s %s refs/heads/main\\0report-status object-format=$GIT_DEFAULT_HASH\\n\" \\\n \t\t\t\t$A $B | packetize\n \t\tfi &&\n - \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n +-\t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n -\t\t\t$ZERO_OID $A | packetize &&\n -+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n - \t\tprintf \"%s %s refs/heads/foo\\n\" \\\n +-\t\tprintf \"%s %s refs/heads/foo\\n\" \\\n -\t\t\t$ZERO_OID $A | packetize &&\n -+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n - \t\tprintf \"%s %s refs/for/next/topic\\n\" \\\n +-\t\tprintf \"%s %s refs/for/next/topic\\n\" \\\n -\t\t\t$ZERO_OID $A | packetize &&\n -+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n - \t\tprintf \"%s %s refs/for/main/topic2\\n\" \\\n +-\t\tprintf \"%s %s refs/for/main/topic2\\n\" \\\n -\t\t\t$ZERO_OID $A | packetize &&\n -+\t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n - \t\tprintf 0000 &&\n +-\t\tprintf 0000 &&\n ++\t\ttest-tool pkt-line pack <<-EOF &&\n ++\t\t$ZERO_OID $A refs/for/main/topic1\n ++\t\t$ZERO_OID $A refs/heads/foo\n ++\t\t$ZERO_OID $A refs/for/next/topic\n ++\t\t$ZERO_OID $A refs/for/main/topic2\n ++\t\t0000\n ++\t\tEOF\n \t\tprintf \"\" | git -C \"$upstream\" pack-objects --stdout\n \t} | git receive-pack \"$upstream\" --stateless-rpc \\\n + \t>out 2>&1 &&\n \n ## t/t5500-fetch-pack.sh ##\n @@ t/t5500-fetch-pack.sh: test_expect_success 'shallow since with commit graph and already-seen commit' '\n4: a44e1790f2 ! 4: 9842c85d1f tests: replace remaining packetize() with \"test-tool pkt-line\"\n @@ t/helper/test-pkt-line.c: static void pack(int argc, const char **argv)\n +static void pack_raw_stdin(void)\n +{\n +\tstruct strbuf sb = STRBUF_INIT;\n -+\tstrbuf_read(&sb, 0, 0);\n ++\n +\tif (strbuf_read(&sb, 0, 0) < 0)\n +\t\tdie_errno(\"failed to read from stdin\");\n +\tpacket_write(1, sb.buf, sb.len);\n @@ t/t5411/once-0010-report-status-v1.sh: test_expect_success \"proc-receive: report\n -\t\t\t\t$A $B | packetize\n +\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n \t\tfi &&\n - \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n - \t\t\t$ZERO_OID $A | test-tool pkt-line pack &&\n + \t\ttest-tool pkt-line pack <<-EOF &&\n + \t\t$ZERO_OID $A refs/for/main/topic1\n \n ## t/t5562-http-backend-content-length.sh ##\n @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '\n @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '\n \n ## t/t5570-git-daemon.sh ##\n @@ t/t5570-git-daemon.sh: test_expect_success 'hostname cannot break out of directory' '\n - '\n \n test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n --\t{\n + \t{\n -\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n -\t\tprintf \"0000\"\n --\t} >input &&\n -+\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" >has-null &&\n -+\ttest-tool pkt-line pack-raw-stdin >input >input <<-\\EOF &&\n -+\t0000\n -+\tEOF\n ++\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" |\n ++\t\ttest-tool pkt-line pack-raw-stdin &&\n ++\t\ttest-tool pkt-line pack <<-\\EOF\n ++\t\t0000\n ++\t\tEOF\n + \t} >input &&\n +\n \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n \ttest-tool pkt-line unpack actual &&\n5: cc91d15ef7 = 5: 7ca83c5a55 test-lib-functions.sh: remove unused [de]packetize() functions\n-- \n2.32.0.788.ge724008458\n\n"},{"id":"429973","messageId":"patch-3.5-92bfd8a87b-20210714T005115Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-0000000000-20210714T005115Z-avarab@gmail.com","subject":"[PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T00:54:03Z","receivedAt":"2021-07-14T00:54:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The shell+perl \"[de]packetize()\" helper functions were added in\n4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n2018-01-24), and around the same time we added the \"pkt-line\" helper\nin 74e70029615 (test-pkt-line: introduce a packet-line test helper,\n2018-03-14).\n\nFor some reason it seems we've mostly used the shell+perl version\ninstead of the helper since then. There were discussions around\n88124ab2636 (test-lib-functions: make packetize() more efficient,\n2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()\nstdin code, 2020-03-29) to improve them and make them more efficient.\n\nLet's instead just use the test helper, I think this results in both\nmore legible code, and for anyone who cares about efficiency it'll be\nfaster.\n\nThe conversion away from extract_haves() in\nt5410-receive-pack-alternates.sh and the \"just pick out\" logic in\nt5570-git-daemon.sh isn't strictly required for this migration, but in\nthis case it's easy enough to test_cmp the whole output, so let's just\ndo that.\n\nSimilarly, there are cases here of changing \"printf 0000\" to a version\nthat'll emit a trailing newline after \"0000\", or where we can do away\nwith the \"0000\" trailer. Let's just always include, and include it in\nthe same way.\n\nWe can't convert all the users of packetize(), it has a feature the\ntest-tool is missing. This'll be addressed in the subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5410-receive-pack-alternates.sh | 42 ++++++++++++++++++--------\n t/t5411/once-0010-report-status-v1.sh | 16 +++++-----\n t/t5500-fetch-pack.sh | 15 +++++----\n t/t5530-upload-pack-error.sh | 24 ++++++++-------\n t/t5562-http-backend-content-length.sh | 13 ++++----\n t/t5570-git-daemon.sh | 12 +++++---\n t/t5704-protocol-violations.sh | 30 +++++++++---------\n 7 files changed, 89 insertions(+), 63 deletions(-)\n\ndiff --git a/t/t5410-receive-pack-alternates.sh b/t/t5410-receive-pack-alternates.sh\nindex 0b28e4e452..d0053d95a4 100755\n--- a/t/t5410-receive-pack-alternates.sh\n+++ b/t/t5410-receive-pack-alternates.sh\n@@ -16,10 +16,6 @@ test_expect_success 'setup' '\n \ttest_commit private\n '\n \n-extract_haves () {\n-\tdepacketize | perl -lne '/^(\\S+) \\.have/ and print $1'\n-}\n-\n test_expect_success 'with core.alternateRefsCommand' '\n \twrite_script fork/alternate-refs <<-\\EOF &&\n \t\tgit --git-dir=\"$1\" for-each-ref \\\n@@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '\n \t\t\trefs/heads/public/\n \tEOF\n \ttest_config -C fork core.alternateRefsCommand ./alternate-refs &&\n-\tgit rev-parse public/branch >expect &&\n-\tprintf \"0000\" | git receive-pack fork >actual &&\n-\textract_haves actual.haves &&\n-\ttest_cmp expect actual.haves\n+\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main) refs/heads/main\n+\t$(git rev-parse base) refs/tags/base\n+\t$(git rev-parse public) .have\n+\t0000\n+\tEOF\n+\n+\tgit receive-pack fork >out actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'with core.alternateRefsPrefixes' '\n \ttest_config -C fork core.alternateRefsPrefixes \"refs/heads/private\" &&\n-\tgit rev-parse private/branch >expect &&\n-\tprintf \"0000\" | git receive-pack fork >actual &&\n-\textract_haves actual.haves &&\n-\ttest_cmp expect actual.haves\n+\n+\ttest-tool pkt-line pack >in <<-\\EOF &&\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main) refs/heads/main\n+\t$(git rev-parse base) refs/tags/base\n+\t$(git rev-parse private) .have\n+\t0000\n+\tEOF\n+\n+\tgit receive-pack fork >out actual &&\n+\ttest_cmp expect actual\n '\n \n test_done\ndiff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\nindex 1233a46eac..ddf3da5a26 100644\n--- a/t/t5411/once-0010-report-status-v1.sh\n+++ b/t/t5411/once-0010-report-status-v1.sh\n@@ -33,15 +33,13 @@ test_expect_success \"proc-receive: report status v1\" '\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status object-format=$GIT_DEFAULT_HASH\\n\" \\\n \t\t\t\t$A $B | packetize\n \t\tfi &&\n-\t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n-\t\tprintf \"%s %s refs/heads/foo\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n-\t\tprintf \"%s %s refs/for/next/topic\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n-\t\tprintf \"%s %s refs/for/main/topic2\\n\" \\\n-\t\t\t$ZERO_OID $A | packetize &&\n-\t\tprintf 0000 &&\n+\t\ttest-tool pkt-line pack <<-EOF &&\n+\t\t$ZERO_OID $A refs/for/main/topic1\n+\t\t$ZERO_OID $A refs/heads/foo\n+\t\t$ZERO_OID $A refs/for/next/topic\n+\t\t$ZERO_OID $A refs/for/main/topic2\n+\t\t0000\n+\t\tEOF\n \t\tprintf \"\" | git -C \"$upstream\" pack-objects --stdout\n \t} | git receive-pack \"$upstream\" --stateless-rpc \\\n \t>out 2>&1 &&\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 8a5d3492c7..ff0b7dd89f 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -872,14 +872,17 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '\n \tgit commit-graph write --reachable &&\n \tgit config core.commitGraph true &&\n \n-\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n-\t0012command=fetch\n-\t$(echo \"object-format=$(test_oid algo)\" | packetize)\n-\t00010013deepen-since 1\n-\t$(echo \"want $(git rev-parse other)\" | packetize)\n-\t$(echo \"have $(git rev-parse main)\" | packetize)\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\tdeepen-since 1\n+\twant $(git rev-parse other)\n+\thave $(git rev-parse main)\n \t0000\n \tEOF\n+\n+\tGIT_PROTOCOL=version=2 git upload-pack . /dev/null\n \t)\n '\n \ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 7c1460eaa9..8ccaae1047 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -90,18 +90,20 @@ test_expect_success 'upload-pack fails due to error in pack-objects enumeration'\n \n test_expect_success 'upload-pack tolerates EOF just after stateless client wants' '\n \ttest_commit initial &&\n-\thead=$(git rev-parse HEAD) &&\n-\n-\t{\n-\t\tpacketize \"want $head\" &&\n-\t\tpacketize \"shallow $head\" &&\n-\t\tpacketize \"deepen 1\" &&\n-\t\tprintf \"0000\"\n-\t} >request &&\n \n-\tprintf \"0000\" >expect &&\n-\n-\tgit upload-pack --stateless-rpc . actual &&\n+\thead=$(git rev-parse HEAD) &&\n+\ttest-tool pkt-line pack >request <<-EOF &&\n+\twant $head\n+\tshallow $head\n+\tdeepen 1\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\t0000\n+\tEOF\n+\tgit upload-pack --stateless-rpc . out &&\n+\ttest-tool pkt-line unpack actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex e5d3d15ba8..e6c8338b64 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -53,12 +53,13 @@ test_expect_success 'setup' '\n \ttest_commit c1 &&\n \thash_head=$(git rev-parse HEAD) &&\n \thash_prev=$(git rev-parse HEAD~1) &&\n-\t{\n-\t\tpacketize \"want $hash_head\" &&\n-\t\tprintf 0000 &&\n-\t\tpacketize \"have $hash_prev\" &&\n-\t\tpacketize \"done\"\n-\t} >fetch_body &&\n+\ttest-tool pkt-line pack >fetch_body <<-EOF &&\n+\twant $hash_head\n+\t0000\n+\thave $hash_prev\n+\tdone\n+\t0000\n+\tEOF\n \ttest_copy_bytes 10 fetch_body.trunc &&\n \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n \t{\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 82c31ab6cd..2dde034881 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n \t\tprintf \"0000\"\n \t} >input &&\n \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n-\tdepacketize output.raw &&\n+\ttest-tool pkt-line unpack actual &&\n+\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD) HEAD\n+\t$(git rev-parse refs/heads/main) refs/heads/main\n+\t0000\n+\tEOF\n \n-\t# just pick out the value of main, which avoids any protocol\n-\t# particulars\n-\tperl -lne \"print \\$1 if m{^(\\\\S+) refs/heads/main}\" actual &&\n-\tgit -C \"$repo\" rev-parse main >expect &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 038fffd3d0..44e2c0d3de 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -7,13 +7,14 @@ making sure that we do not segfault or otherwise behave badly.'\n . ./test-lib.sh\n \n test_expect_success 'extra delim packet in v2 ls-refs args' '\n-\t{\n-\t\tpacketize command=ls-refs &&\n-\t\tpacketize \"object-format=$(test_oid algo)\" &&\n-\t\tprintf 0001 &&\n-\t\t# protocol expects 0000 flush here\n-\t\tprintf 0001\n-\t} >input &&\n+\t# protocol expects 0000 flush after the 0001\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=ls-refs\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\t0001\n+\tEOF\n+\n \tcat >err.expect <<-\\EOF &&\n \tfatal: expected flush after ls-refs arguments\n \tEOF\n@@ -23,13 +24,14 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '\n '\n \n test_expect_success 'extra delim packet in v2 fetch args' '\n-\t{\n-\t\tpacketize command=fetch &&\n-\t\tpacketize \"object-format=$(test_oid algo)\" &&\n-\t\tprintf 0001 &&\n-\t\t# protocol expects 0000 flush here\n-\t\tprintf 0001\n-\t} >input &&\n+\t# protocol expects 0000 flush after the 0001\n+\ttest-tool pkt-line pack >input <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\t0001\n+\tEOF\n+\n \tcat >err.expect <<-\\EOF &&\n \tfatal: expected flush after fetch arguments\n \tEOF\n-- \n2.32.0.788.ge724008458\n\n"},{"id":"429974","messageId":"patch-2.5-64dfd14865-20210714T005115Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-0000000000-20210714T005115Z-avarab@gmail.com","subject":"[PATCH v3 2/5] serve tests: use test_cmp in \"protocol violations\" test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T00:54:02Z","receivedAt":"2021-07-14T00:54:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the protocol violations tests to check the full output, not just\ngrep it. This changes code added in 4845b772458 (upload-pack: handle\nunexpected delim packets, 2020-03-27). The newly added test in the\npreceding commit already did the full test_cmp.\n\nI have a related series to tweak the output from upload-pack et al, we\nreally want to make sure we have this exact output, and not fewer or\nmore lines etc.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5704-protocol-violations.sh | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 95f68e1d4b..038fffd3d0 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -14,9 +14,12 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '\n \t\t# protocol expects 0000 flush here\n \t\tprintf 0001\n \t} >input &&\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: expected flush after ls-refs arguments\n+\tEOF\n \ttest_must_fail env GIT_PROTOCOL=version=2 \\\n-\t\tgit upload-pack . err &&\n-\ttest_i18ngrep \"expected flush after ls-refs arguments\" err\n+\t\tgit upload-pack . err.actual &&\n+\ttest_cmp err.expect err.actual\n '\n \n test_expect_success 'extra delim packet in v2 fetch args' '\n@@ -27,9 +30,12 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n \t\t# protocol expects 0000 flush here\n \t\tprintf 0001\n \t} >input &&\n+\tcat >err.expect <<-\\EOF &&\n+\tfatal: expected flush after fetch arguments\n+\tEOF\n \ttest_must_fail env GIT_PROTOCOL=version=2 \\\n-\t\tgit upload-pack . err &&\n-\ttest_i18ngrep \"expected flush after fetch arguments\" err\n+\t\tgit upload-pack . err.actual &&\n+\ttest_cmp err.expect err.actual\n '\n \n test_expect_success 'extra delim packet in v2 object-info args' '\n-- \n2.32.0.788.ge724008458\n\n"},{"id":"429975","messageId":"patch-4.5-9842c85d1f-20210714T005115Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-0000000000-20210714T005115Z-avarab@gmail.com","subject":"[PATCH v3 4/5] tests: replace remaining packetize() with \"test-tool pkt-line\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T00:54:04Z","receivedAt":"2021-07-14T00:54:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the only remaining users of \"packetize()\" over to \"test-tool\npkt-line\", for this we need a new \"pack-raw-stdin\" subcommand in the\ntest-tool. The \"pack\" command takes input on stdin, but splits it by\n\"\\n\", furthermore we'll format the output using C-strings, so the\nembedded \"\\0\" being tested for here would cause the string to be\ntruncated.\n\nSo we need another mode that just calls packet_write() on the raw\ninput we got on stdin.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/helper/test-pkt-line.c | 12 ++++++++++++\n t/t5411/once-0010-report-status-v1.sh | 4 ++--\n t/t5562-http-backend-content-length.sh | 3 ++-\n t/t5570-git-daemon.sh | 8 ++++++--\n 4 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex 5e638f0b97..c5e052e537 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -26,6 +26,16 @@ static void pack(int argc, const char **argv)\n \t}\n }\n \n+static void pack_raw_stdin(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (strbuf_read(&sb, 0, 0) < 0)\n+\t\tdie_errno(\"failed to read from stdin\");\n+\tpacket_write(1, sb.buf, sb.len);\n+\tstrbuf_release(&sb);\n+}\n+\n static void unpack(void)\n {\n \tstruct packet_reader reader;\n@@ -110,6 +120,8 @@ int cmd__pkt_line(int argc, const char **argv)\n \n \tif (!strcmp(argv[1], \"pack\"))\n \t\tpack(argc - 2, argv + 2);\n+\telse if (!strcmp(argv[1], \"pack-raw-stdin\"))\n+\t\tpack_raw_stdin();\n \telse if (!strcmp(argv[1], \"unpack\"))\n \t\tunpack();\n \telse if (!strcmp(argv[1], \"unpack-sideband\"))\ndiff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\nindex ddf3da5a26..666b3d8726 100644\n--- a/t/t5411/once-0010-report-status-v1.sh\n+++ b/t/t5411/once-0010-report-status-v1.sh\n@@ -28,10 +28,10 @@ test_expect_success \"proc-receive: report status v1\" '\n \t\tif test -z \"$GIT_DEFAULT_HASH\" || test \"$GIT_DEFAULT_HASH\" = \"sha1\"\n \t\tthen\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status\\n\" \\\n-\t\t\t\t$A $B | packetize\n+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n \t\telse\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status object-format=$GIT_DEFAULT_HASH\\n\" \\\n-\t\t\t\t$A $B | packetize\n+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n \t\tfi &&\n \t\ttest-tool pkt-line pack <<-EOF &&\n \t\t$ZERO_OID $A refs/for/main/topic1\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex e6c8338b64..23a8a8d5c7 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -64,7 +64,8 @@ test_expect_success 'setup' '\n \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n \t{\n \t\tprintf \"%s %s refs/heads/newbranch\\\\0report-status object-format=%s\\\\n\" \\\n-\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize &&\n+\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" |\n+\t\t\ttest-tool pkt-line pack-raw-stdin &&\n \t\tprintf 0000 &&\n \t\techo \"$hash_next\" | git pack-objects --stdout\n \t} >push_body &&\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 2dde034881..e2cb4f376d 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -194,9 +194,13 @@ test_expect_success 'hostname cannot break out of directory' '\n \n test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n \t{\n-\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n-\t\tprintf \"0000\"\n+\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" |\n+\t\ttest-tool pkt-line pack-raw-stdin &&\n+\t\ttest-tool pkt-line pack <<-\\EOF\n+\t\t0000\n+\t\tEOF\n \t} >input &&\n+\n \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n \ttest-tool pkt-line unpack actual &&\n \n-- \n2.32.0.788.ge724008458\n\n"},{"id":"429976","messageId":"patch-5.5-7ca83c5a55-20210714T005115Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-0000000000-20210714T005115Z-avarab@gmail.com","subject":"[PATCH v3 5/5] test-lib-functions.sh: remove unused [de]packetize() functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T00:54:05Z","receivedAt":"2021-07-14T00:54:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the now-unused \"packetize()\" and \"depacketize()\" functions\nadded in 4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n2018-01-24). As discussed in the preceding commits we've now moved all\ntheir users over to \"test-tool pkt-line\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib-functions.sh | 42 -----------------------------------------\n 1 file changed, 42 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex b2810478a2..77e4434dcd 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1453,48 +1453,6 @@ nongit () {\n \t)\n } 7>&2 2>&4\n \n-# convert function arguments or stdin (if not arguments given) to pktline\n-# representation. If multiple arguments are given, they are separated by\n-# whitespace and put in a single packet. Note that data containing NULs must be\n-# given on stdin, and that empty input becomes an empty packet, not a flush\n-# packet (for that you can just print 0000 yourself).\n-packetize () {\n-\tif test $# -gt 0\n-\tthen\n-\t\tpacket=\"$*\"\n-\t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n-\telse\n-\t\tperl -e '\n-\t\t\tmy $packet = do { local $/; };\n-\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n-\t\t'\n-\tfi\n-}\n-\n-# Parse the input as a series of pktlines, writing the result to stdout.\n-# Sideband markers are removed automatically, and the output is routed to\n-# stderr if appropriate.\n-#\n-# NUL bytes are converted to \"\\\\0\" for ease of parsing with text tools.\n-depacketize () {\n-\tperl -e '\n-\t\twhile (read(STDIN, $len, 4) == 4) {\n-\t\t\tif ($len eq \"0000\") {\n-\t\t\t\tprint \"FLUSH\\n\";\n-\t\t\t} else {\n-\t\t\t\tread(STDIN, $buf, hex($len) - 4);\n-\t\t\t\t$buf =~ s/\\0/\\\\0/g;\n-\t\t\t\tif ($buf =~ s/^[\\x2\\x3]//) {\n-\t\t\t\t\tprint STDERR $buf;\n-\t\t\t\t} else {\n-\t\t\t\t\t$buf =~ s/^\\x1//;\n-\t\t\t\t\tprint $buf;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n-\t'\n-}\n-\n # Converts base-16 data into base-8. The output is given as a sequence of\n # escaped octals, suitable for consumption by 'printf'.\n hex2oct () {\n-- \n2.32.0.788.ge724008458\n\n"},{"id":"429981","messageId":"YO455OO5q50m7WiZ@coredump.intra.peff.net","threadId":"56071","inReplyTo":"875yxd3h7c.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-14T01:12:04Z","receivedAt":"2021-07-14T01:12:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 14, 2021 at 01:41:27AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > This is testing the whole output, rather than just the \"have\" lines (as\n> > the original did). It was intentionally written the other way for two\n> > reasons:\n> >\n> > - it keeps the focus on what we are actually testing: the .have\n> > behavior\n> >\n> > - it makes the test less brittle to other changes in the script\n> >\n> > I can buy the argument that sometimes testing the whole output, even if\n> > it is more brittle, helps us find other unexpected outcomes (e.g., the\n> > stderr test_cmp vs grep thing earlier in the series). But here it just\n> > seems strictly worse to me.\n> >\n> > It would be easy to just replace depacketize with \"pktline unpack\", but\n> > keep the perl one-liner. I don't think it's a _huge_ deal in this case\n> > either way, but I'm not enthused about the trend.\n> \n> FWIW this series was spun off from an effort of fixing a bug related to\n> protocol-y tests doing just such a \"we can grep the output, we know we\n> only need xyz\", and the only test for it not failing because we picked\n> the wrong xyz.\n> \n> In this case yeah we could keep the grep. I do think in general that\n> unless output is overly verbose we should test_cmp it, and in this case\n> it's really not.\n> \n> On the focus it seems it's the opposite for the two uf us. It takes my\n> focus away from the test itself when reading it. I.e. I start wondering\n> why the grep, is the output variable or whatever, especially in a case\n> like this where we're hardly saving ourselves overall lines or reducing\n> complexity.\n\nOK. I don't really agree, but I don't feel strongly enough to argue\nabout it. (I guess I'm a little more invested here because you're not\nwriting new tests, which I would definitely not have said anything\nabout, but rather changing tests that I wrote. ;) ).\n\n> >> +\ttest-tool pkt-line pack >in <<-\\EOF &&\n> >> +\t0000\n> >> +\tEOF\n> >\n> > This used to just be \"printf 0000\". The new version is longer and less\n> > efficient, but I'm OK with it on the grounds of consistency (all inputs\n> > flow through \"pkt-line pack\", and all outputs through \"pkt-line unpack\").\n> \n> They aren't equivalent because the pkt-line helper (and pkt-line.c in\n> general) will be forgiving about the presence or lack of trailing\n> newlines, but some of these tests were not.\n> \n> I think we should use the helper in/out for all of those, because we're\n> explicitly interested if the protocol round-trips the way we expect, and\n> not per-se if the exact bytes match.\n\nThat's not at all true for flush packets, though. They _must_ be exactly\n\"0000\", no newlines or anything else. The pkt-line tool is smart enough\nto drop the newline in this case (i.e., it recognizes that this is not a\npacket that says \"0000\", but a special flush token).\n\nSo it really is byte-equivalent to \"printf 0000\", and it must be.\n\nThere is a separate issue, which I didn't raise, which is that:\n\n printf foo | packetize\n\nmay be different than:\n\n test-tool pkt-line pack <<\\EOF\n foo\n EOF\n\nFor the most part, I agree it is not that important. Some of the tests\nreally do care (because they are testing syntactic stuff), and those\nshould be using your new raw-stdin feature. I did briefly wonder if we\nlosing a little bit of accidental diversity of the tests to put them all\nthrough a generator that will always add the newline. But it _shouldn't_\nmatter in general, and if it does, we should be making specific tests\nwhere it does.\n\n> > Now that you're using the multi-line-capable helper, these could all be\n> > a single (and much more readable):\n> >\n> > test-tool pkt-line pack <<-EOF\n> > $ZERO_OID $A refs/for/main/topic1\n> > $ZERO_OID $A refs/heads/foo\n> > $ZERO_OID $A refs/for/next/topic\n> > $ZERO_OID $A refs/for/main/topic2\n> > EOF\n> >\n> > couldn't they?\n> \n> Yeah, but you never know what you'll get \"let's do the small change\"\n> v.s. \"let's avoid refactoring while we're at it\" feedback :)\n> \n> I figured it was easier to review with just the s/packetize/test-tool\n> pkt-line pack/g, but sure, I can change it.\n\nWell, yes, but that seems like a _much_ smaller change than all of the\nrewriting you were doing elsewhere, which is actually changing what\nwe're checking (albeit in a way that is functionally equivalent). I can\nlive with it either way, but it just seemed weird to go so far in some\nhunks and then not here.\n\n> >> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n> >> index e5d3d15ba8d..e6c8338b648 100755\n> >> --- a/t/t5562-http-backend-content-length.sh\n> >> +++ b/t/t5562-http-backend-content-length.sh\n> >> @@ -53,12 +53,13 @@ test_expect_success 'setup' '\n> >> \ttest_commit c1 &&\n> >> \thash_head=$(git rev-parse HEAD) &&\n> >> \thash_prev=$(git rev-parse HEAD~1) &&\n> >> -\t{\n> >> -\t\tpacketize \"want $hash_head\" &&\n> >> -\t\tprintf 0000 &&\n> >> -\t\tpacketize \"have $hash_prev\" &&\n> >> -\t\tpacketize \"done\"\n> >> -\t} >fetch_body &&\n> >> +\ttest-tool pkt-line pack >fetch_body <<-EOF &&\n> >> +\twant $hash_head\n> >> +\t0000\n> >> +\thave $hash_prev\n> >> +\tdone\n> >> +\t0000\n> >> +\tEOF\n> >\n> > There's a flush packet at the end of your input in the post-image that\n> > doesn't seem to be in the original.\n> \n> Yeah, but isn't round-tripping through the helper the right thing here?\n\nI'm not sure I understand. In the original, the final bytes in\nfetch_body would be \"0008done\". In yours, it will be \"0009done\\n0000\".\nThe extra newline is OK, but that added flush packet is not an\nequivalent protocol input to feed to git-http-backend. I expect that Git\ndoesn't complain because after the \"done\" there shouldn't be anything\nsaid, so the bytes are simply dropped.\n\nGoing through pkt-line is good and reasonable, if that's what you mean\nby round-tripping. Your input just has an extra \"0000\" on the final line\nof the here-doc.\n\n> >> -\tdepacketize output.raw &&\n> >> +\ttest-tool pkt-line unpack actual &&\n> >> +\n> >> +\tcat >expect <<-EOF &&\n> >> +\t$(git rev-parse HEAD) HEAD\n> >> +\t$(git rev-parse refs/heads/main) refs/heads/main\n> >> +\t0000\n> >> +\tEOF\n> >> \n> >> -\t# just pick out the value of main, which avoids any protocol\n> >> -\t# particulars\n> >> -\tperl -lne \"print \\$1 if m{^(\\\\S+) refs/heads/main}\" actual &&\n> >> -\tgit -C \"$repo\" rev-parse main >expect &&\n> >> \ttest_cmp expect actual\n> >> '\n> >\n> > This is another case where you're checking the output for more than the\n> > original did, ignoring the comment. :) When using depacketize, the bits\n> > after the \"\\0\" were encoded and kept, so it was necessary. The pkt-line\n> > helper just throws those bits away, so it's OK (I'm a little surprised\n> > that discarding those bits wasn't a roadblock for converting some tests,\n> > but I guess we didn't have any low-level protocol tests that cared).\n> \n> ... I see you got to this bit in 4/5.\n\nI'm not sure I was completely clear. Yes, if there is a NUL in the\ninput, we must use 4/5's raw-stdin mode. But here I am talking about the\n_unpack_ operation.\n\nFor a v0 protocol response that buries capabilities after a NUL,\ndepacketize will show it (the \"\\0\" escape is literally part of the\noutput):\n\n $ git init foo\n $ cd foo\n $ git commit --allow-empty -m foo\n $ git upload-pack . | depacketize\n 2b78ba01d65c1b0a44c0ac0dd8275e7802b74f41 HEAD\\0multi_ack thin-pack side-band side-band-64k ofs-delta shallow deepen-since deepen-not deepen-relative no-progress include-tag multi_ack_detailed symref=HEAD:refs/heads/main object-format=sha1 agent=git/2.32.0.663.g7cfef204d6\n 2b78ba01d65c1b0a44c0ac0dd8275e7802b74f41 refs/heads/main\n FLUSH\n\nwhereas the pkt-line helper will not:\n\n $ git upload-pack . | ../t/helper/test-tool pkt-line unpack\n 2b78ba01d65c1b0a44c0ac0dd8275e7802b74f41 HEAD\n 2b78ba01d65c1b0a44c0ac0dd8275e7802b74f41 refs/heads/main\n 0000\n\nThat's more convenient in many cases, but I was just saying I was\nsurprised we did not have any tests that ever tested any of the bits\npast the NUL (we do, of course, but they are using real Git commands to\nmake sure the correct stuff happened, and not peeking at the protocol\nthemselves).\n\nI think it's OK to proceed. If we later do want a test that looks past\nthe NUL, we can add that feature to the pkt-line helper. Though looking\nat the helper implementation, it does not even seem intentional; it\nsimply treats the packet data as a C string, feeding it to printf. So if\nyou do want to be thorough in checking expected packet data in outputs,\nit is definitely throwing away some of that data.\n\n-Peff\n"},{"id":"429987","messageId":"YO460pigGesWU/5H@coredump.intra.peff.net","threadId":"56071","inReplyTo":"8735sh3h09.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 4/5] tests: replace remaining packetize() with \"test-tool pkt-line\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-14T01:16:02Z","receivedAt":"2021-07-14T01:16:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 14, 2021 at 01:52:09AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > I.e.:\n> >\n> > {\n> > printf \"git-upload-pack /interp.git\\n\\0host=localhost\" |\n> > \t test-tool pkt-line pack-raw-stdin &&\n> > \t printf \"0000\" | test-tool pkt-line pack\n> > } >input\n> >\n> > (again here the packing of \"0000\" is pointless, but I'm OK with it for\n> > consistency).\n> \n> Sure, I can use {} blocks, but re the reply on 3/5 about \"0000\"\n> v.s. \"0000\\n\" you'd like to move back to print not-a-newline here\n> v.s. having the helper eat lines ending with \\n like everywhere else?\n> \n> It's not incorrect in this case, it just seems less obvious to\n> me. I.e. the printf in the former case is because we explicitly care\n> about the exact raw input, if there's a trailing \\n or not, in the\n> latter case we don't, so I think it's clearier to use the usual <<-\\EOF\n> pattern rather than printf.\n\nI don't think it's incorrect in any of the cases. I was just noting that\n\"printf 0000\" is fewer characters and fewer processes than either piping\nto pkt-line or using a here-doc.\n\nI don't feel strongly on using it if you want to keep things consistent\nbetween the tests. Nor on using a here-doc versus piping (I don't see\nany problem with switching between them based on which is shorter or\nmore readable in any given instance; if you're piping, it could also be\n\"echo\").\n\n-Peff\n"},{"id":"429995","messageId":"YO5iOqWx46UxdhKX@nand.local","threadId":"56071","inReplyTo":"patch-3.5-92bfd8a87b-20210714T005115Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-14T04:04:10Z","receivedAt":"2021-07-14T04:04:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 14, 2021 at 02:54:03AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> -extract_haves () {\n> -\tdepacketize | perl -lne '/^(\\S+) \\.have/ and print $1'\n> -}\n> -\n> test_expect_success 'with core.alternateRefsCommand' '\n> \twrite_script fork/alternate-refs <<-\\EOF &&\n> \t\tgit --git-dir=\"$1\" for-each-ref \\\n> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '\n> \t\t\trefs/heads/public/\n> \tEOF\n> \ttest_config -C fork core.alternateRefsCommand ./alternate-refs &&\n> -\tgit rev-parse public/branch >expect &&\n> -\tprintf \"0000\" | git receive-pack fork >actual &&\n> -\textract_haves actual.haves &&\n> -\ttest_cmp expect actual.haves\n> +\n> +\ttest-tool pkt-line pack >in <<-\\EOF &&\n> +\t0000\n> +\tEOF\n> +\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse main) refs/heads/main\n> +\t$(git rev-parse base) refs/tags/base\n> +\t$(git rev-parse public) .have\n> +\t0000\n> +\tEOF\n> +\n> +\tgit receive-pack fork >out +\ttest-tool pkt-line unpack actual &&\n\nI don't think extracting the haves themselves (and stripping \".have\"\nfrom the output) adds much verbosity at all. Wouldn't it be:\n\n test-tool pkt-line unpack actual &&\n perl -lne '/^(\\S+) \\.have/ and print $1' actual.haves\n\n(in fact, it might be quite nice to leave extract_haves as-is changing\ndepacketize for 'test-tool pkt-line unpack').\n\nFor what it's worth, I also don't care enough to argue about it, but I\nam curious why the change here was more invasive compared to other\nlocations (where depacketize was just replaced with the earlier\nincantation).\n\nIf memory serves, I am the original author of this test (via 89284c1d6c\n(transport.c: introduce core.alternateRefsCommand, 2018-10-08)), but the\nPerl bits are Peff's from the discussion in:\n\n https://lore.kernel.org/git/20180922195258.GA20983@sigill.intra.peff.net/\n\nThanks,\nTaylor\n"},{"id":"430067","messageId":"xmqqpmvkyie6.fsf@gitster.g","threadId":"56071","inReplyTo":"YO5iOqWx46UxdhKX@nand.local","subject":"Re: [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-14T16:22:57Z","receivedAt":"2021-07-14T16:23:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Wed, Jul 14, 2021 at 02:54:03AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> -extract_haves () {\n>> -\tdepacketize | perl -lne '/^(\\S+) \\.have/ and print $1'\n>> -}\n>> -\n>> test_expect_success 'with core.alternateRefsCommand' '\n>> \twrite_script fork/alternate-refs <<-\\EOF &&\n>> \t\tgit --git-dir=\"$1\" for-each-ref \\\n>> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '\n>> \t\t\trefs/heads/public/\n>> \tEOF\n>> \ttest_config -C fork core.alternateRefsCommand ./alternate-refs &&\n>> -\tgit rev-parse public/branch >expect &&\n>> -\tprintf \"0000\" | git receive-pack fork >actual &&\n>> -\textract_haves actual.haves &&\n>> -\ttest_cmp expect actual.haves\n>> +\n>> +\ttest-tool pkt-line pack >in <<-\\EOF &&\n>> +\t0000\n>> +\tEOF\n>> +\n>> +\tcat >expect <<-EOF &&\n>> +\t$(git rev-parse main) refs/heads/main\n>> +\t$(git rev-parse base) refs/tags/base\n>> +\t$(git rev-parse public) .have\n>> +\t0000\n>> +\tEOF\n>> +\n>> +\tgit receive-pack fork >out > +\ttest-tool pkt-line unpack actual &&\n>\n> I don't think extracting the haves themselves (and stripping \".have\"\n> from the output) adds much verbosity at all. Wouldn't it be:\n>\n> test-tool pkt-line unpack actual &&\n> perl -lne '/^(\\S+) \\.have/ and print $1' actual.haves\n>\n> (in fact, it might be quite nice to leave extract_haves as-is changing\n> depacketize for 'test-tool pkt-line unpack').\n\nI tend to agree with you ane Peff, after reading the resulting tests\nmyself.\n\nSpecifically I have problem with this line of reasoning:\n\n The conversion away from extract_haves() in ... isn't strictly\n required for this migration, but in this case it's easy enough\n to test_cmp the whole output, so let's just do that.\n\nas if using test_cmp to compare the whole output is always a better\nway to test, which is far from cut-and-dried clear and obvious. The\ndefault ought to be to keep the original behaviour, unless you can\nclearly convince others that either (1) the new way is better, or\n(2) keeping the old way is too hard and the cost for doing so\noutweighs the benefit.\n\nWhile there certainly is some value in end-to-end tests, they\ninevitably become brittle. We prefer focused tests that verify\nthings we _care_ about, while keeping the expected vector unaffected\nby future changes in details that do not matter in what is being\ntested. If we are interested in \".have\"s, we shouldn't be affected\nby irrelevant details like what other branches and tags appear in\nthe output and in what order, for example.\n\nOf course, if there is a solid reason why we would care not just\n\".have\" but other details, it is perfectly justifiable thing to do\nto update the tests, but we'd want to see (1) such an unrelated\nchange done in a separate step and (2) with its own justification.\n\nSo...\n"},{"id":"430345","messageId":"patch-1.1-546974eed59-20210716T153909Z-avarab@gmail.com","threadId":"56071","inReplyTo":"cover-0.5-0000000000-20210714T005115Z-avarab@gmail.com","subject":"[PATCH v4] test-lib-functions: use test-tool for [de]packetize()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-16T15:41:33Z","receivedAt":"2021-07-16T15:41:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The shell+perl \"[de]packetize()\" helper functions were added in\n4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n2018-01-24), and around the same time we added the \"pkt-line\" helper\nin 74e70029615 (test-pkt-line: introduce a packet-line test helper,\n2018-03-14).\n\nFor some reason it seems we've mostly used the shell+perl version\ninstead of the helper since then. There were discussions around\n88124ab2636 (test-lib-functions: make packetize() more efficient,\n2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()\nstdin code, 2020-03-29) to improve them and make them more efficient.\n\nThere was one good reason to do so, we needed an equivalent of\n\"test-tool pkt-line pack\", but that command wasn't capable of handling\ninput with \"\\n\" (a feature) or \"\\0\" (just because it happens to be\nprintf-based under the hood).\n\nLet's add a \"pkt-line-raw\" helper for that, and expose is at a\npacketize_raw() to go with the existing packetize() on the shell\nlevel, this gives us the smallest amount of change to the tests\nthemselves.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nI ejected changing/adding test code for this v4. This keeps the\ncompatibility wrappers and just narrowly changes the things that need\na packetize_raw() to use that new helper.\n\nI left the simpler packetize() case as a \"printf\", it could also use\nthe test tool, but in case someone cares about process overhead on say\nWindows this entire change should be strictly better than the status\nquo.\n\nRange-diff against v3:\n1: 67aa8141153 < -: ----------- serve tests: add missing \"extra delim\" test\n2: 64dfd14865c < -: ----------- serve tests: use test_cmp in \"protocol violations\" test\n3: 92bfd8a87b8 < -: ----------- tests: replace [de]packetize() shell+perl test-tool pkt-line\n4: 9842c85d1f3 ! 1: 546974eed59 tests: replace remaining packetize() with \"test-tool pkt-line\"\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - tests: replace remaining packetize() with \"test-tool pkt-line\"\n + test-lib-functions: use test-tool for [de]packetize()\n \n - Move the only remaining users of \"packetize()\" over to \"test-tool\n - pkt-line\", for this we need a new \"pack-raw-stdin\" subcommand in the\n - test-tool. The \"pack\" command takes input on stdin, but splits it by\n - \"\\n\", furthermore we'll format the output using C-strings, so the\n - embedded \"\\0\" being tested for here would cause the string to be\n - truncated.\n + The shell+perl \"[de]packetize()\" helper functions were added in\n + 4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n + 2018-01-24), and around the same time we added the \"pkt-line\" helper\n + in 74e70029615 (test-pkt-line: introduce a packet-line test helper,\n + 2018-03-14).\n \n - So we need another mode that just calls packet_write() on the raw\n - input we got on stdin.\n + For some reason it seems we've mostly used the shell+perl version\n + instead of the helper since then. There were discussions around\n + 88124ab2636 (test-lib-functions: make packetize() more efficient,\n + 2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()\n + stdin code, 2020-03-29) to improve them and make them more efficient.\n +\n + There was one good reason to do so, we needed an equivalent of\n + \"test-tool pkt-line pack\", but that command wasn't capable of handling\n + input with \"\\n\" (a feature) or \"\\0\" (just because it happens to be\n + printf-based under the hood).\n +\n + Let's add a \"pkt-line-raw\" helper for that, and expose is at a\n + packetize_raw() to go with the existing packetize() on the shell\n + level, this gives us the smallest amount of change to the tests\n + themselves.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ t/t5411/once-0010-report-status-v1.sh: test_expect_success \"proc-receive: report\n \t\tthen\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status\\n\" \\\n -\t\t\t\t$A $B | packetize\n -+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n ++\t\t\t\t$A $B | packetize_raw\n \t\telse\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status object-format=$GIT_DEFAULT_HASH\\n\" \\\n -\t\t\t\t$A $B | packetize\n -+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n ++\t\t\t\t$A $B | packetize_raw\n \t\tfi &&\n - \t\ttest-tool pkt-line pack <<-EOF &&\n - \t\t$ZERO_OID $A refs/for/main/topic1\n + \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n + \t\t\t$ZERO_OID $A | packetize &&\n \n ## t/t5562-http-backend-content-length.sh ##\n @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '\n @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '\n \t{\n \t\tprintf \"%s %s refs/heads/newbranch\\\\0report-status object-format=%s\\\\n\" \\\n -\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize &&\n -+\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" |\n -+\t\t\ttest-tool pkt-line pack-raw-stdin &&\n ++\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize_raw\n \t\tprintf 0000 &&\n \t\techo \"$hash_next\" | git pack-objects --stdout\n \t} >push_body &&\n @@ t/t5570-git-daemon.sh: test_expect_success 'hostname cannot break out of directo\n test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n \t{\n -\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n --\t\tprintf \"0000\"\n -+\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" |\n -+\t\ttest-tool pkt-line pack-raw-stdin &&\n -+\t\ttest-tool pkt-line pack <<-\\EOF\n -+\t\t0000\n -+\t\tEOF\n ++\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize_raw\n + \t\tprintf \"0000\"\n \t} >input &&\n -+\n \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n - \ttest-tool pkt-line unpack actual &&\n +\n + ## t/test-lib-functions.sh ##\n +@@ t/test-lib-functions.sh: nongit () {\n + \t)\n + } 7>&2 2>&4\n + \n +-# convert function arguments or stdin (if not arguments given) to pktline\n +-# representation. If multiple arguments are given, they are separated by\n +-# whitespace and put in a single packet. Note that data containing NULs must be\n +-# given on stdin, and that empty input becomes an empty packet, not a flush\n +-# packet (for that you can just print 0000 yourself).\n ++# These functions are historical wrappers around \"test-tool pkt-line\"\n ++# for older tests. Use \"test-tool pkt-line\" itself in new tests.\n + packetize () {\n + \tif test $# -gt 0\n + \tthen\n + \t\tpacket=\"$*\"\n + \t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n + \telse\n +-\t\tperl -e '\n +-\t\t\tmy $packet = do { local $/; };\n +-\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n +-\t\t'\n ++\t\ttest-tool pkt-line pack\n + \tfi\n + }\n + \n +-# Parse the input as a series of pktlines, writing the result to stdout.\n +-# Sideband markers are removed automatically, and the output is routed to\n +-# stderr if appropriate.\n +-#\n +-# NUL bytes are converted to \"\\\\0\" for ease of parsing with text tools.\n ++packetize_raw () {\n ++\ttest-tool pkt-line pack-raw-stdin\n ++}\n ++\n + depacketize () {\n +-\tperl -e '\n +-\t\twhile (read(STDIN, $len, 4) == 4) {\n +-\t\t\tif ($len eq \"0000\") {\n +-\t\t\t\tprint \"FLUSH\\n\";\n +-\t\t\t} else {\n +-\t\t\t\tread(STDIN, $buf, hex($len) - 4);\n +-\t\t\t\t$buf =~ s/\\0/\\\\0/g;\n +-\t\t\t\tif ($buf =~ s/^[\\x2\\x3]//) {\n +-\t\t\t\t\tprint STDERR $buf;\n +-\t\t\t\t} else {\n +-\t\t\t\t\t$buf =~ s/^\\x1//;\n +-\t\t\t\t\tprint $buf;\n +-\t\t\t\t}\n +-\t\t\t}\n +-\t\t}\n +-\t'\n ++\ttest-tool pkt-line unpack\n + }\n \n + # Converts base-16 data into base-8. The output is given as a sequence of\n5: 7ca83c5a551 < -: ----------- test-lib-functions.sh: remove unused [de]packetize() functions\n\n t/helper/test-pkt-line.c | 12 ++++++++\n t/t5411/once-0010-report-status-v1.sh | 4 +--\n t/t5562-http-backend-content-length.sh | 2 +-\n t/t5570-git-daemon.sh | 2 +-\n t/test-lib-functions.sh | 38 ++++++--------------------\n 5 files changed, 24 insertions(+), 34 deletions(-)\n\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex 5e638f0b970..c5e052e5378 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -26,6 +26,16 @@ static void pack(int argc, const char **argv)\n \t}\n }\n \n+static void pack_raw_stdin(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (strbuf_read(&sb, 0, 0) < 0)\n+\t\tdie_errno(\"failed to read from stdin\");\n+\tpacket_write(1, sb.buf, sb.len);\n+\tstrbuf_release(&sb);\n+}\n+\n static void unpack(void)\n {\n \tstruct packet_reader reader;\n@@ -110,6 +120,8 @@ int cmd__pkt_line(int argc, const char **argv)\n \n \tif (!strcmp(argv[1], \"pack\"))\n \t\tpack(argc - 2, argv + 2);\n+\telse if (!strcmp(argv[1], \"pack-raw-stdin\"))\n+\t\tpack_raw_stdin();\n \telse if (!strcmp(argv[1], \"unpack\"))\n \t\tunpack();\n \telse if (!strcmp(argv[1], \"unpack-sideband\"))\ndiff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\nindex 1233a46eac5..297b10925d5 100644\n--- a/t/t5411/once-0010-report-status-v1.sh\n+++ b/t/t5411/once-0010-report-status-v1.sh\n@@ -28,10 +28,10 @@ test_expect_success \"proc-receive: report status v1\" '\n \t\tif test -z \"$GIT_DEFAULT_HASH\" || test \"$GIT_DEFAULT_HASH\" = \"sha1\"\n \t\tthen\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status\\n\" \\\n-\t\t\t\t$A $B | packetize\n+\t\t\t\t$A $B | packetize_raw\n \t\telse\n \t\t\tprintf \"%s %s refs/heads/main\\0report-status object-format=$GIT_DEFAULT_HASH\\n\" \\\n-\t\t\t\t$A $B | packetize\n+\t\t\t\t$A $B | packetize_raw\n \t\tfi &&\n \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n \t\t\t$ZERO_OID $A | packetize &&\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex e5d3d15ba8d..05a58069b0c 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -63,7 +63,7 @@ test_expect_success 'setup' '\n \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n \t{\n \t\tprintf \"%s %s refs/heads/newbranch\\\\0report-status object-format=%s\\\\n\" \\\n-\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize &&\n+\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize_raw\n \t\tprintf 0000 &&\n \t\techo \"$hash_next\" | git pack-objects --stdout\n \t} >push_body &&\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 82c31ab6cd8..b87ca06a585 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -194,7 +194,7 @@ test_expect_success 'hostname cannot break out of directory' '\n \n test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n \t{\n-\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n+\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize_raw\n \t\tprintf \"0000\"\n \t} >input &&\n \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex b2810478a21..e5b80e0f88e 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1453,46 +1453,24 @@ nongit () {\n \t)\n } 7>&2 2>&4\n \n-# convert function arguments or stdin (if not arguments given) to pktline\n-# representation. If multiple arguments are given, they are separated by\n-# whitespace and put in a single packet. Note that data containing NULs must be\n-# given on stdin, and that empty input becomes an empty packet, not a flush\n-# packet (for that you can just print 0000 yourself).\n+# These functions are historical wrappers around \"test-tool pkt-line\"\n+# for older tests. Use \"test-tool pkt-line\" itself in new tests.\n packetize () {\n \tif test $# -gt 0\n \tthen\n \t\tpacket=\"$*\"\n \t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n \telse\n-\t\tperl -e '\n-\t\t\tmy $packet = do { local $/; };\n-\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n-\t\t'\n+\t\ttest-tool pkt-line pack\n \tfi\n }\n \n-# Parse the input as a series of pktlines, writing the result to stdout.\n-# Sideband markers are removed automatically, and the output is routed to\n-# stderr if appropriate.\n-#\n-# NUL bytes are converted to \"\\\\0\" for ease of parsing with text tools.\n+packetize_raw () {\n+\ttest-tool pkt-line pack-raw-stdin\n+}\n+\n depacketize () {\n-\tperl -e '\n-\t\twhile (read(STDIN, $len, 4) == 4) {\n-\t\t\tif ($len eq \"0000\") {\n-\t\t\t\tprint \"FLUSH\\n\";\n-\t\t\t} else {\n-\t\t\t\tread(STDIN, $buf, hex($len) - 4);\n-\t\t\t\t$buf =~ s/\\0/\\\\0/g;\n-\t\t\t\tif ($buf =~ s/^[\\x2\\x3]//) {\n-\t\t\t\t\tprint STDERR $buf;\n-\t\t\t\t} else {\n-\t\t\t\t\t$buf =~ s/^\\x1//;\n-\t\t\t\t\tprint $buf;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n-\t'\n+\ttest-tool pkt-line unpack\n }\n \n # Converts base-16 data into base-8. The output is given as a sequence of\n-- \n2.32.0.874.gfa1990a4f10\n\n"},{"id":"430354","messageId":"YPG5few/PeStqe/2@nand.local","threadId":"56071","inReplyTo":"patch-1.1-546974eed59-20210716T153909Z-avarab@gmail.com","subject":"Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-16T16:53:17Z","receivedAt":"2021-07-16T16:53:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 16, 2021 at 05:41:33PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> I ejected changing/adding test code for this v4. This keeps the\n> compatibility wrappers and just narrowly changes the things that need\n> a packetize_raw() to use that new helper.\n>\n> I left the simpler packetize() case as a \"printf\", it could also use\n> the test tool, but in case someone cares about process overhead on say\n> Windows this entire change should be strictly better than the status\n> quo.\n\nThanks, this version of the series looks much better to me.\n\n> +static void pack_raw_stdin(void)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tif (strbuf_read(&sb, 0, 0) < 0)\n> +\t\tdie_errno(\"failed to read from stdin\");\n> +\tpacket_write(1, sb.buf, sb.len);\n> +\tstrbuf_release(&sb);\n> +}\n> +\n\nLooks good to me.\n\n> -# convert function arguments or stdin (if not arguments given) to pktline\n> -# representation. If multiple arguments are given, they are separated by\n> -# whitespace and put in a single packet. Note that data containing NULs must be\n> -# given on stdin, and that empty input becomes an empty packet, not a flush\n> -# packet (for that you can just print 0000 yourself).\n> +# These functions are historical wrappers around \"test-tool pkt-line\"\n> +# for older tests. Use \"test-tool pkt-line\" itself in new tests.\n\nNice. This keeps the diff relatively minimal. I don't really have a\nstrong opinion on saying \"packetize\" or \"test-tool pkt-line pack\" in the\nfuture. Arguably the latter is more direct, but it's also more of a\nmouthful.\n\nI'm fine to take the approach you have here, and adjust the guidance in\nthe future depending on if people really do start preferring one over\nthe other in new tests.\n\n> packetize () {\n> \tif test $# -gt 0\n> \tthen\n> \t\tpacket=\"$*\"\n> \t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n> \telse\n> -\t\tperl -e '\n> -\t\t\tmy $packet = do { local $/; };\n> -\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n> -\t\t'\n> +\t\ttest-tool pkt-line pack\n> \tfi\n> }\n\nFor what it's worth, I would be happy to remove the printf shortcut\nentirely. Some quick grepping indicates only 22 uses of the word\n\"packetize\" in our whole test suite (one of them being the function\ndeclaration itself). And of the 21 callers, only 10 pass at least one\nargument:\n\n git grep -Ew 'packetize [^()&]+' -- t\n\nSo I would be fine with adding 10 more new processes to the test suite\nin the name of simplifying this declaration. I don't feel strongly,\nthough, since the conditional here does not really add that much\ncomplexity.\n\nThanks,\nTaylor\n"},{"id":"430376","messageId":"YPHYHx9W4HSRMmq+@coredump.intra.peff.net","threadId":"56071","inReplyTo":"patch-1.1-546974eed59-20210716T153909Z-avarab@gmail.com","subject":"Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-16T19:03:59Z","receivedAt":"2021-07-16T19:04:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 16, 2021 at 05:41:33PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I ejected changing/adding test code for this v4. This keeps the\n> compatibility wrappers and just narrowly changes the things that need\n> a packetize_raw() to use that new helper.\n> \n> I left the simpler packetize() case as a \"printf\", it could also use\n> the test tool, but in case someone cares about process overhead on say\n> Windows this entire change should be strictly better than the status\n> quo.\n\nThis looks fine to me. I actually did like some of the readability\nimprovements possible in the other patches, but they can still be easily\nbuilt on top if we care to.\n\nUsing \"test-tool pkt-line unpack\" doesn't behave exactly like\ndepacketize:\n\n - as noted earlier, it drops stuff after a NUL\n\n - it says \"0000\" instead of \"FLUSH\"\n\nBut we don't seem to depend on either of those in any tests, so it's a\ngood time to switch. :)\n\n-Peff\n"},{"id":"430378","messageId":"YPHZNJgAkd3m6bhm@coredump.intra.peff.net","threadId":"56071","inReplyTo":"YPG5few/PeStqe/2@nand.local","subject":"Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-16T19:08:36Z","receivedAt":"2021-07-16T19:08:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 16, 2021 at 12:53:17PM -0400, Taylor Blau wrote:\n\n> > packetize () {\n> > \tif test $# -gt 0\n> > \tthen\n> > \t\tpacket=\"$*\"\n> > \t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n> > \telse\n> > -\t\tperl -e '\n> > -\t\t\tmy $packet = do { local $/; };\n> > -\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n> > -\t\t'\n> > +\t\ttest-tool pkt-line pack\n> > \tfi\n> > }\n> \n> For what it's worth, I would be happy to remove the printf shortcut\n> entirely. Some quick grepping indicates only 22 uses of the word\n> \"packetize\" in our whole test suite (one of them being the function\n> declaration itself). And of the 21 callers, only 10 pass at least one\n> argument:\n> \n> git grep -Ew 'packetize [^()&]+' -- t\n> \n> So I would be fine with adding 10 more new processes to the test suite\n> in the name of simplifying this declaration. I don't feel strongly,\n> though, since the conditional here does not really add that much\n> complexity.\n\nI'd be fine with that, too. Part of the goal of the cmdline option was\nmaking callers more readable. E.g.,\n\n -printf \"want %s\" \"$hash_head\" | packetize\n +packetize \"want $hash_head\"\n\nBut I think the here-doc input to \"test-tool pkt-line\" is generally\nequally nice, especially when you have multiple lines.\n\n(the commit introducing the cmdline version talks about efficiency, but\nthere the old version was using 4 processes!)\n\n-Peff\n"},{"id":"430539","messageId":"xmqqh7gqkuc7.fsf@gitster.g","threadId":"56071","inReplyTo":"patch-1.1-546974eed59-20210716T153909Z-avarab@gmail.com","subject":"Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-19T18:54:32Z","receivedAt":"2021-07-19T19:15:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The shell+perl \"[de]packetize()\" helper functions were added in\n> 4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n> 2018-01-24), and around the same time we added the \"pkt-line\" helper\n> in 74e70029615 (test-pkt-line: introduce a packet-line test helper,\n> 2018-03-14).\n>\n> For some reason it seems we've mostly used the shell+perl version\n> instead of the helper since then. There were discussions around\n> 88124ab2636 (test-lib-functions: make packetize() more efficient,\n> 2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()\n> stdin code, 2020-03-29) to improve them and make them more efficient.\n>\n> There was one good reason to do so, we needed an equivalent of\n> \"test-tool pkt-line pack\", but that command wasn't capable of handling\n> input with \"\\n\" (a feature) or \"\\0\" (just because it happens to be\n> printf-based under the hood).\n>\n> Let's add a \"pkt-line-raw\" helper for that, and expose is at a\n> packetize_raw() to go with the existing packetize() on the shell\n> level, this gives us the smallest amount of change to the tests\n> themselves.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n> I ejected changing/adding test code for this v4. This keeps the\n> compatibility wrappers and just narrowly changes the things that need\n> a packetize_raw() to use that new helper.\n>\n> I left the simpler packetize() case as a \"printf\", it could also use\n> the test tool, but in case someone cares about process overhead on say\n> Windows this entire change should be strictly better than the status\n> quo.\n\nThanks, all. Will replace. Let me mark this for 'next'.\n\n\n\n> Range-diff against v3:\n> 1: 67aa8141153 < -: ----------- serve tests: add missing \"extra delim\" test\n> 2: 64dfd14865c < -: ----------- serve tests: use test_cmp in \"protocol violations\" test\n> 3: 92bfd8a87b8 < -: ----------- tests: replace [de]packetize() shell+perl test-tool pkt-line\n> 4: 9842c85d1f3 ! 1: 546974eed59 tests: replace remaining packetize() with \"test-tool pkt-line\"\n> @@ Metadata\n> Author: Ævar Arnfjörð Bjarmason \n> \n> ## Commit message ##\n> - tests: replace remaining packetize() with \"test-tool pkt-line\"\n> + test-lib-functions: use test-tool for [de]packetize()\n> \n> - Move the only remaining users of \"packetize()\" over to \"test-tool\n> - pkt-line\", for this we need a new \"pack-raw-stdin\" subcommand in the\n> - test-tool. The \"pack\" command takes input on stdin, but splits it by\n> - \"\\n\", furthermore we'll format the output using C-strings, so the\n> - embedded \"\\0\" being tested for here would cause the string to be\n> - truncated.\n> + The shell+perl \"[de]packetize()\" helper functions were added in\n> + 4414a150025 (t/lib-git-daemon: add network-protocol helpers,\n> + 2018-01-24), and around the same time we added the \"pkt-line\" helper\n> + in 74e70029615 (test-pkt-line: introduce a packet-line test helper,\n> + 2018-03-14).\n> \n> - So we need another mode that just calls packet_write() on the raw\n> - input we got on stdin.\n> + For some reason it seems we've mostly used the shell+perl version\n> + instead of the helper since then. There were discussions around\n> + 88124ab2636 (test-lib-functions: make packetize() more efficient,\n> + 2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()\n> + stdin code, 2020-03-29) to improve them and make them more efficient.\n> +\n> + There was one good reason to do so, we needed an equivalent of\n> + \"test-tool pkt-line pack\", but that command wasn't capable of handling\n> + input with \"\\n\" (a feature) or \"\\0\" (just because it happens to be\n> + printf-based under the hood).\n> +\n> + Let's add a \"pkt-line-raw\" helper for that, and expose is at a\n> + packetize_raw() to go with the existing packetize() on the shell\n> + level, this gives us the smallest amount of change to the tests\n> + themselves.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> \n> @@ t/t5411/once-0010-report-status-v1.sh: test_expect_success \"proc-receive: report\n> \t\tthen\n> \t\t\tprintf \"%s %s refs/heads/main\\0report-status\\n\" \\\n> -\t\t\t\t$A $B | packetize\n> -+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n> ++\t\t\t\t$A $B | packetize_raw\n> \t\telse\n> \t\t\tprintf \"%s %s refs/heads/main\\0report-status object-format=$GIT_DEFAULT_HASH\\n\" \\\n> -\t\t\t\t$A $B | packetize\n> -+\t\t\t\t$A $B | test-tool pkt-line pack-raw-stdin\n> ++\t\t\t\t$A $B | packetize_raw\n> \t\tfi &&\n> - \t\ttest-tool pkt-line pack <<-EOF &&\n> - \t\t$ZERO_OID $A refs/for/main/topic1\n> + \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n> + \t\t\t$ZERO_OID $A | packetize &&\n> \n> ## t/t5562-http-backend-content-length.sh ##\n> @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '\n> @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '\n> \t{\n> \t\tprintf \"%s %s refs/heads/newbranch\\\\0report-status object-format=%s\\\\n\" \\\n> -\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize &&\n> -+\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" |\n> -+\t\t\ttest-tool pkt-line pack-raw-stdin &&\n> ++\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize_raw\n> \t\tprintf 0000 &&\n> \t\techo \"$hash_next\" | git pack-objects --stdout\n> \t} >push_body &&\n> @@ t/t5570-git-daemon.sh: test_expect_success 'hostname cannot break out of directo\n> test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n> \t{\n> -\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n> --\t\tprintf \"0000\"\n> -+\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" |\n> -+\t\ttest-tool pkt-line pack-raw-stdin &&\n> -+\t\ttest-tool pkt-line pack <<-\\EOF\n> -+\t\t0000\n> -+\t\tEOF\n> ++\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize_raw\n> + \t\tprintf \"0000\"\n> \t} >input &&\n> -+\n> \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n> - \ttest-tool pkt-line unpack actual &&\n> +\n> + ## t/test-lib-functions.sh ##\n> +@@ t/test-lib-functions.sh: nongit () {\n> + \t)\n> + } 7>&2 2>&4\n> + \n> +-# convert function arguments or stdin (if not arguments given) to pktline\n> +-# representation. If multiple arguments are given, they are separated by\n> +-# whitespace and put in a single packet. Note that data containing NULs must be\n> +-# given on stdin, and that empty input becomes an empty packet, not a flush\n> +-# packet (for that you can just print 0000 yourself).\n> ++# These functions are historical wrappers around \"test-tool pkt-line\"\n> ++# for older tests. Use \"test-tool pkt-line\" itself in new tests.\n> + packetize () {\n> + \tif test $# -gt 0\n> + \tthen\n> + \t\tpacket=\"$*\"\n> + \t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n> + \telse\n> +-\t\tperl -e '\n> +-\t\t\tmy $packet = do { local $/; };\n> +-\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n> +-\t\t'\n> ++\t\ttest-tool pkt-line pack\n> + \tfi\n> + }\n> + \n> +-# Parse the input as a series of pktlines, writing the result to stdout.\n> +-# Sideband markers are removed automatically, and the output is routed to\n> +-# stderr if appropriate.\n> +-#\n> +-# NUL bytes are converted to \"\\\\0\" for ease of parsing with text tools.\n> ++packetize_raw () {\n> ++\ttest-tool pkt-line pack-raw-stdin\n> ++}\n> ++\n> + depacketize () {\n> +-\tperl -e '\n> +-\t\twhile (read(STDIN, $len, 4) == 4) {\n> +-\t\t\tif ($len eq \"0000\") {\n> +-\t\t\t\tprint \"FLUSH\\n\";\n> +-\t\t\t} else {\n> +-\t\t\t\tread(STDIN, $buf, hex($len) - 4);\n> +-\t\t\t\t$buf =~ s/\\0/\\\\0/g;\n> +-\t\t\t\tif ($buf =~ s/^[\\x2\\x3]//) {\n> +-\t\t\t\t\tprint STDERR $buf;\n> +-\t\t\t\t} else {\n> +-\t\t\t\t\t$buf =~ s/^\\x1//;\n> +-\t\t\t\t\tprint $buf;\n> +-\t\t\t\t}\n> +-\t\t\t}\n> +-\t\t}\n> +-\t'\n> ++\ttest-tool pkt-line unpack\n> + }\n> \n> + # Converts base-16 data into base-8. The output is given as a sequence of\n> 5: 7ca83c5a551 < -: ----------- test-lib-functions.sh: remove unused [de]packetize() functions\n>\n> t/helper/test-pkt-line.c | 12 ++++++++\n> t/t5411/once-0010-report-status-v1.sh | 4 +--\n> t/t5562-http-backend-content-length.sh | 2 +-\n> t/t5570-git-daemon.sh | 2 +-\n> t/test-lib-functions.sh | 38 ++++++--------------------\n> 5 files changed, 24 insertions(+), 34 deletions(-)\n>\n> diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\n> index 5e638f0b970..c5e052e5378 100644\n> --- a/t/helper/test-pkt-line.c\n> +++ b/t/helper/test-pkt-line.c\n> @@ -26,6 +26,16 @@ static void pack(int argc, const char **argv)\n> \t}\n> }\n> \n> +static void pack_raw_stdin(void)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tif (strbuf_read(&sb, 0, 0) < 0)\n> +\t\tdie_errno(\"failed to read from stdin\");\n> +\tpacket_write(1, sb.buf, sb.len);\n> +\tstrbuf_release(&sb);\n> +}\n> +\n> static void unpack(void)\n> {\n> \tstruct packet_reader reader;\n> @@ -110,6 +120,8 @@ int cmd__pkt_line(int argc, const char **argv)\n> \n> \tif (!strcmp(argv[1], \"pack\"))\n> \t\tpack(argc - 2, argv + 2);\n> +\telse if (!strcmp(argv[1], \"pack-raw-stdin\"))\n> +\t\tpack_raw_stdin();\n> \telse if (!strcmp(argv[1], \"unpack\"))\n> \t\tunpack();\n> \telse if (!strcmp(argv[1], \"unpack-sideband\"))\n> diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh\n> index 1233a46eac5..297b10925d5 100644\n> --- a/t/t5411/once-0010-report-status-v1.sh\n> +++ b/t/t5411/once-0010-report-status-v1.sh\n> @@ -28,10 +28,10 @@ test_expect_success \"proc-receive: report status v1\" '\n> \t\tif test -z \"$GIT_DEFAULT_HASH\" || test \"$GIT_DEFAULT_HASH\" = \"sha1\"\n> \t\tthen\n> \t\t\tprintf \"%s %s refs/heads/main\\0report-status\\n\" \\\n> -\t\t\t\t$A $B | packetize\n> +\t\t\t\t$A $B | packetize_raw\n> \t\telse\n> \t\t\tprintf \"%s %s refs/heads/main\\0report-status object-format=$GIT_DEFAULT_HASH\\n\" \\\n> -\t\t\t\t$A $B | packetize\n> +\t\t\t\t$A $B | packetize_raw\n> \t\tfi &&\n> \t\tprintf \"%s %s refs/for/main/topic1\\n\" \\\n> \t\t\t$ZERO_OID $A | packetize &&\n> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n> index e5d3d15ba8d..05a58069b0c 100755\n> --- a/t/t5562-http-backend-content-length.sh\n> +++ b/t/t5562-http-backend-content-length.sh\n> @@ -63,7 +63,7 @@ test_expect_success 'setup' '\n> \thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n> \t{\n> \t\tprintf \"%s %s refs/heads/newbranch\\\\0report-status object-format=%s\\\\n\" \\\n> -\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize &&\n> +\t\t\t\"$ZERO_OID\" \"$hash_next\" \"$(test_oid algo)\" | packetize_raw\n> \t\tprintf 0000 &&\n> \t\techo \"$hash_next\" | git pack-objects --stdout\n> \t} >push_body &&\n> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n> index 82c31ab6cd8..b87ca06a585 100755\n> --- a/t/t5570-git-daemon.sh\n> +++ b/t/t5570-git-daemon.sh\n> @@ -194,7 +194,7 @@ test_expect_success 'hostname cannot break out of directory' '\n> \n> test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n> \t{\n> -\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n> +\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize_raw\n> \t\tprintf \"0000\"\n> \t} >input &&\n> \tfake_nc \"$GIT_DAEMON_HOST_PORT\" output &&\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index b2810478a21..e5b80e0f88e 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1453,46 +1453,24 @@ nongit () {\n> \t)\n> } 7>&2 2>&4\n> \n> -# convert function arguments or stdin (if not arguments given) to pktline\n> -# representation. If multiple arguments are given, they are separated by\n> -# whitespace and put in a single packet. Note that data containing NULs must be\n> -# given on stdin, and that empty input becomes an empty packet, not a flush\n> -# packet (for that you can just print 0000 yourself).\n> +# These functions are historical wrappers around \"test-tool pkt-line\"\n> +# for older tests. Use \"test-tool pkt-line\" itself in new tests.\n> packetize () {\n> \tif test $# -gt 0\n> \tthen\n> \t\tpacket=\"$*\"\n> \t\tprintf '%04x%s' \"$((4 + ${#packet}))\" \"$packet\"\n> \telse\n> -\t\tperl -e '\n> -\t\t\tmy $packet = do { local $/; };\n> -\t\t\tprintf \"%04x%s\", 4 + length($packet), $packet;\n> -\t\t'\n> +\t\ttest-tool pkt-line pack\n> \tfi\n> }\n> \n> -# Parse the input as a series of pktlines, writing the result to stdout.\n> -# Sideband markers are removed automatically, and the output is routed to\n> -# stderr if appropriate.\n> -#\n> -# NUL bytes are converted to \"\\\\0\" for ease of parsing with text tools.\n> +packetize_raw () {\n> +\ttest-tool pkt-line pack-raw-stdin\n> +}\n> +\n> depacketize () {\n> -\tperl -e '\n> -\t\twhile (read(STDIN, $len, 4) == 4) {\n> -\t\t\tif ($len eq \"0000\") {\n> -\t\t\t\tprint \"FLUSH\\n\";\n> -\t\t\t} else {\n> -\t\t\t\tread(STDIN, $buf, hex($len) - 4);\n> -\t\t\t\t$buf =~ s/\\0/\\\\0/g;\n> -\t\t\t\tif ($buf =~ s/^[\\x2\\x3]//) {\n> -\t\t\t\t\tprint STDERR $buf;\n> -\t\t\t\t} else {\n> -\t\t\t\t\t$buf =~ s/^\\x1//;\n> -\t\t\t\t\tprint $buf;\n> -\t\t\t\t}\n> -\t\t\t}\n> -\t\t}\n> -\t'\n> +\ttest-tool pkt-line unpack\n> }\n> \n> # Converts base-16 data into base-8. The output is given as a sequence of\n"}]}