[PATCH 0/5] tests: migrate to "test-tool pkt-line"

[PATCH 0/5] tests: migrate to "test-tool pkt-line"

[Ævar Arnfjörð Bjarmason]

Change code that uses [de]packetize() to use "test-tool
pkt-line". That we had these two concurrently in the tests is mostly
just a historical anomaly/inconsistency that we can fix.

Even the CC'd author of [de]packetize() has recently been using the
test-tool, in af22a63c399 (sideband: diagnose more sideband anomalies,
2020-10-28).

"Mostly" because it turns out we were missing one feature in the
test-tool, to packetize raw input without any \n or \0 munging. This
series adds that capabability, migrates those users, and finally
removes the now-obsolete [de]packetize() functions.

Ævar Arnfjörð Bjarmason (5):
  serve tests: add missing "extra delim" test
  serve tests: use test_cmp in "protocol violations" test
  tests: replace [de]packetize() shell+perl test-tool pkt-line
  tests: replace remaining packetize() with "test-tool pkt-line"
  test-lib-functions.sh: remove unused [de]packetize() functions

 t/helper/test-pkt-line.c               | 13 ++++++
 t/t5410-receive-pack-alternates.sh     | 42 +++++++++++++-----
 t/t5411/once-0010-report-status-v1.sh  | 12 ++---
 t/t5500-fetch-pack.sh                  | 15 ++++---
 t/t5530-upload-pack-error.sh           | 24 +++++-----
 t/t5562-http-backend-content-length.sh | 16 ++++---
 t/t5570-git-daemon.sh                  | 22 ++++++----
 t/t5704-protocol-violations.sh         | 61 ++++++++++++++++++--------
 t/test-lib-functions.sh                | 42 ------------------
 9 files changed, 136 insertions(+), 111 deletions(-)

-- 
2.32.0.636.g43e71d69cff

[PATCH 1/5] serve tests: add missing "extra delim" test

[Ævar Arnfjörð Bjarmason]

When the object-info protocol v2 call was added in
a2ba162cda2 (object-info: support for retrieving object info,
2021-04-20) we didn't add a corresponding test here.

We had tests for ls-refs and fetch already since
4845b772458 (upload-pack: handle unexpected delim packets,
2020-03-27), but the same behavior in object-info (which is clearly
copied from the template of "ls-refs") did not have the corresponding
tests.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5704-protocol-violations.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 5c941949b98..95f68e1d4b7 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -32,4 +32,21 @@ test_expect_success 'extra delim packet in v2 fetch args' '
 	test_i18ngrep "expected flush after fetch arguments" err
 '
 
+test_expect_success 'extra delim packet in v2 object-info args' '
+	# protocol expects 0000 flush after the 0001
+	test-tool pkt-line pack >input <<-EOF &&
+	command=object-info
+	object-format=$(test_oid algo)
+	0001
+	0001
+	EOF
+
+	cat >err.expect <<-\EOF &&
+	fatal: object-info: expected flush after arguments
+	EOF
+	test_must_fail env GIT_PROTOCOL=version=2 \
+		git upload-pack . <input 2>err.actual &&
+	test_cmp err.expect err.actual
+'
+
 test_done
-- 
2.32.0.636.g43e71d69cff

[PATCH 2/5] serve tests: use test_cmp in "protocol violations" test

[Ævar Arnfjörð Bjarmason]

Amend the protocol violations tests to check the full output, not just
grep it. This changes code added in 4845b772458 (upload-pack: handle
unexpected delim packets, 2020-03-27). The newly added test in the
preceding commit already did the full test_cmp.

I have a related series to tweak the output from upload-pack et al, we
really want to make sure we have this exact output, and not fewer or
more lines etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5704-protocol-violations.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 95f68e1d4b7..038fffd3d03 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -14,9 +14,12 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '
 		# protocol expects 0000 flush here
 		printf 0001
 	} >input &&
+	cat >err.expect <<-\EOF &&
+	fatal: expected flush after ls-refs arguments
+	EOF
 	test_must_fail env GIT_PROTOCOL=version=2 \
-		git upload-pack . <input 2>err &&
-	test_i18ngrep "expected flush after ls-refs arguments" err
+		git upload-pack . <input 2>err.actual &&
+	test_cmp err.expect err.actual
 '
 
 test_expect_success 'extra delim packet in v2 fetch args' '
@@ -27,9 +30,12 @@ test_expect_success 'extra delim packet in v2 fetch args' '
 		# protocol expects 0000 flush here
 		printf 0001
 	} >input &&
+	cat >err.expect <<-\EOF &&
+	fatal: expected flush after fetch arguments
+	EOF
 	test_must_fail env GIT_PROTOCOL=version=2 \
-		git upload-pack . <input 2>err &&
-	test_i18ngrep "expected flush after fetch arguments" err
+		git upload-pack . <input 2>err.actual &&
+	test_cmp err.expect err.actual
 '
 
 test_expect_success 'extra delim packet in v2 object-info args' '
-- 
2.32.0.636.g43e71d69cff

[PATCH 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line

[Ævar Arnfjörð Bjarmason]

The shell+perl "[de]packetize()" helper functions were added in
4414a150025 (t/lib-git-daemon: add network-protocol helpers,
2018-01-24), and around the same time we added the "pkt-line" helper
in 74e70029615 (test-pkt-line: introduce a packet-line test helper,
2018-03-14).

For some reason it seems we've mostly used the shell+perl version
instead of the helper since then. There were discussions around
88124ab2636 (test-lib-functions: make packetize() more efficient,
2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()
stdin code, 2020-03-29) to improve them and make them more efficient.

Let's instead just use the test helper, I think this results in both
more legible code, and for anyone who cares about efficiency it'll be
faster.

We can't convert all the users of packetize(), it has a feature the
test-tool is missing. This'll be addressed in the subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5410-receive-pack-alternates.sh     | 42 ++++++++++++++++++--------
 t/t5411/once-0010-report-status-v1.sh  |  8 ++---
 t/t5500-fetch-pack.sh                  | 15 +++++----
 t/t5530-upload-pack-error.sh           | 24 ++++++++-------
 t/t5562-http-backend-content-length.sh | 13 ++++----
 t/t5570-git-daemon.sh                  | 12 +++++---
 t/t5704-protocol-violations.sh         | 30 +++++++++---------
 7 files changed, 86 insertions(+), 58 deletions(-)

diff --git a/t/t5410-receive-pack-alternates.sh b/t/t5410-receive-pack-alternates.sh
index 0b28e4e452f..d0053d95a44 100755
--- a/t/t5410-receive-pack-alternates.sh
+++ b/t/t5410-receive-pack-alternates.sh
@@ -16,10 +16,6 @@ test_expect_success 'setup' '
 	test_commit private
 '
 
-extract_haves () {
-	depacketize | perl -lne '/^(\S+) \.have/ and print $1'
-}
-
 test_expect_success 'with core.alternateRefsCommand' '
 	write_script fork/alternate-refs <<-\EOF &&
 		git --git-dir="$1" for-each-ref \
@@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '
 			refs/heads/public/
 	EOF
 	test_config -C fork core.alternateRefsCommand ./alternate-refs &&
-	git rev-parse public/branch >expect &&
-	printf "0000" | git receive-pack fork >actual &&
-	extract_haves <actual >actual.haves &&
-	test_cmp expect actual.haves
+
+	test-tool pkt-line pack >in <<-\EOF &&
+	0000
+	EOF
+
+	cat >expect <<-EOF &&
+	$(git rev-parse main) refs/heads/main
+	$(git rev-parse base) refs/tags/base
+	$(git rev-parse public) .have
+	0000
+	EOF
+
+	git receive-pack fork >out <in &&
+	test-tool pkt-line unpack <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'with core.alternateRefsPrefixes' '
 	test_config -C fork core.alternateRefsPrefixes "refs/heads/private" &&
-	git rev-parse private/branch >expect &&
-	printf "0000" | git receive-pack fork >actual &&
-	extract_haves <actual >actual.haves &&
-	test_cmp expect actual.haves
+
+	test-tool pkt-line pack >in <<-\EOF &&
+	0000
+	EOF
+
+	cat >expect <<-EOF &&
+	$(git rev-parse main) refs/heads/main
+	$(git rev-parse base) refs/tags/base
+	$(git rev-parse private) .have
+	0000
+	EOF
+
+	git receive-pack fork >out <in &&
+	test-tool pkt-line unpack <out >actual &&
+	test_cmp expect actual
 '
 
 test_done
diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
index 1233a46eac5..cf33d993192 100644
--- a/t/t5411/once-0010-report-status-v1.sh
+++ b/t/t5411/once-0010-report-status-v1.sh
@@ -34,13 +34,13 @@ test_expect_success "proc-receive: report status v1" '
 				$A $B | packetize
 		fi &&
 		printf "%s %s refs/for/main/topic1\n" \
-			$ZERO_OID $A | packetize &&
+			$ZERO_OID $A | test-tool pkt-line pack &&
 		printf "%s %s refs/heads/foo\n" \
-			$ZERO_OID $A | packetize &&
+			$ZERO_OID $A | test-tool pkt-line pack &&
 		printf "%s %s refs/for/next/topic\n" \
-			$ZERO_OID $A | packetize &&
+			$ZERO_OID $A | test-tool pkt-line pack &&
 		printf "%s %s refs/for/main/topic2\n" \
-			$ZERO_OID $A | packetize &&
+			$ZERO_OID $A | test-tool pkt-line pack &&
 		printf 0000 &&
 		printf "" | git -C "$upstream" pack-objects --stdout
 	} | git receive-pack "$upstream" --stateless-rpc \
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 8a5d3492c71..ff0b7dd89f9 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -872,14 +872,17 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '
 	git commit-graph write --reachable &&
 	git config core.commitGraph true &&
 
-	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
-	0012command=fetch
-	$(echo "object-format=$(test_oid algo)" | packetize)
-	00010013deepen-since 1
-	$(echo "want $(git rev-parse other)" | packetize)
-	$(echo "have $(git rev-parse main)" | packetize)
+	test-tool pkt-line pack >in <<-EOF &&
+	command=fetch
+	object-format=$(test_oid algo)
+	0001
+	deepen-since 1
+	want $(git rev-parse other)
+	have $(git rev-parse main)
 	0000
 	EOF
+
+	GIT_PROTOCOL=version=2 git upload-pack . <in >/dev/null
 	)
 '
 
diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh
index 7c1460eaa99..8ccaae10475 100755
--- a/t/t5530-upload-pack-error.sh
+++ b/t/t5530-upload-pack-error.sh
@@ -90,18 +90,20 @@ test_expect_success 'upload-pack fails due to error in pack-objects enumeration'
 
 test_expect_success 'upload-pack tolerates EOF just after stateless client wants' '
 	test_commit initial &&
-	head=$(git rev-parse HEAD) &&
-
-	{
-		packetize "want $head" &&
-		packetize "shallow $head" &&
-		packetize "deepen 1" &&
-		printf "0000"
-	} >request &&
 
-	printf "0000" >expect &&
-
-	git upload-pack --stateless-rpc . <request >actual &&
+	head=$(git rev-parse HEAD) &&
+	test-tool pkt-line pack >request <<-EOF &&
+	want $head
+	shallow $head
+	deepen 1
+	0000
+	EOF
+
+	cat >expect <<-\EOF &&
+	0000
+	EOF
+	git upload-pack --stateless-rpc . <request >out &&
+	test-tool pkt-line unpack <out >actual &&
 	test_cmp expect actual
 '
 
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index e5d3d15ba8d..e6c8338b648 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -53,12 +53,13 @@ test_expect_success 'setup' '
 	test_commit c1 &&
 	hash_head=$(git rev-parse HEAD) &&
 	hash_prev=$(git rev-parse HEAD~1) &&
-	{
-		packetize "want $hash_head" &&
-		printf 0000 &&
-		packetize "have $hash_prev" &&
-		packetize "done"
-	} >fetch_body &&
+	test-tool pkt-line pack >fetch_body <<-EOF &&
+	want $hash_head
+	0000
+	have $hash_prev
+	done
+	0000
+	EOF
 	test_copy_bytes 10 <fetch_body >fetch_body.trunc &&
 	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
 	{
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 82c31ab6cd8..2dde0348816 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
 		printf "0000"
 	} >input &&
 	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
-	depacketize <output >output.raw &&
+	test-tool pkt-line unpack <output >actual &&
+
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) HEAD
+	$(git rev-parse refs/heads/main) refs/heads/main
+	0000
+	EOF
 
-	# just pick out the value of main, which avoids any protocol
-	# particulars
-	perl -lne "print \$1 if m{^(\\S+) refs/heads/main}" <output.raw >actual &&
-	git -C "$repo" rev-parse main >expect &&
 	test_cmp expect actual
 '
 
diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 038fffd3d03..44e2c0d3ded 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -7,13 +7,14 @@ making sure that we do not segfault or otherwise behave badly.'
 . ./test-lib.sh
 
 test_expect_success 'extra delim packet in v2 ls-refs args' '
-	{
-		packetize command=ls-refs &&
-		packetize "object-format=$(test_oid algo)" &&
-		printf 0001 &&
-		# protocol expects 0000 flush here
-		printf 0001
-	} >input &&
+	# protocol expects 0000 flush after the 0001
+	test-tool pkt-line pack >input <<-EOF &&
+	command=ls-refs
+	object-format=$(test_oid algo)
+	0001
+	0001
+	EOF
+
 	cat >err.expect <<-\EOF &&
 	fatal: expected flush after ls-refs arguments
 	EOF
@@ -23,13 +24,14 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '
 '
 
 test_expect_success 'extra delim packet in v2 fetch args' '
-	{
-		packetize command=fetch &&
-		packetize "object-format=$(test_oid algo)" &&
-		printf 0001 &&
-		# protocol expects 0000 flush here
-		printf 0001
-	} >input &&
+	# protocol expects 0000 flush after the 0001
+	test-tool pkt-line pack >input <<-EOF &&
+	command=fetch
+	object-format=$(test_oid algo)
+	0001
+	0001
+	EOF
+
 	cat >err.expect <<-\EOF &&
 	fatal: expected flush after fetch arguments
 	EOF
-- 
2.32.0.636.g43e71d69cff

[PATCH 4/5] tests: replace remaining packetize() with "test-tool pkt-line"

[Ævar Arnfjörð Bjarmason]

Move the only remaining users of "packetize()" over to "test-tool
pkt-line", for this we need a new "pack-raw-stdin" subcommand in the
test-tool. The "pack" command takes input on stdin, but splits it by
"\n", furthermore we'll format the output using C-strings, so the
embedded "\0" being tested for here would cause the string to be
truncated.

So we need another mode that just calls packet_write() on the raw
input we got on stdin.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-pkt-line.c               | 13 +++++++++++++
 t/t5411/once-0010-report-status-v1.sh  |  4 ++--
 t/t5562-http-backend-content-length.sh |  3 ++-
 t/t5570-git-daemon.sh                  | 10 ++++++----
 4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 5e638f0b970..a2437fbe57d 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -26,6 +26,17 @@ static void pack(int argc, const char **argv)
 	}
 }
 
+static void pack_raw_stdin(void)
+{
+
+	struct strbuf sb = STRBUF_INIT;
+	strbuf_read(&sb, 0, 0);
+	if (strbuf_read(&sb, 0, 0) < 0)
+		die_errno("failed to read from stdin");
+	packet_write(1, sb.buf, sb.len);
+	strbuf_release(&sb);
+}
+
 static void unpack(void)
 {
 	struct packet_reader reader;
@@ -110,6 +121,8 @@ int cmd__pkt_line(int argc, const char **argv)
 
 	if (!strcmp(argv[1], "pack"))
 		pack(argc - 2, argv + 2);
+	else if (!strcmp(argv[1], "pack-raw-stdin"))
+		pack_raw_stdin();
 	else if (!strcmp(argv[1], "unpack"))
 		unpack();
 	else if (!strcmp(argv[1], "unpack-sideband"))
diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
index cf33d993192..75d4233e49f 100644
--- a/t/t5411/once-0010-report-status-v1.sh
+++ b/t/t5411/once-0010-report-status-v1.sh
@@ -28,10 +28,10 @@ test_expect_success "proc-receive: report status v1" '
 		if test -z "$GIT_DEFAULT_HASH" || test "$GIT_DEFAULT_HASH" = "sha1"
 		then
 			printf "%s %s refs/heads/main\0report-status\n" \
-				$A $B | packetize
+				$A $B | test-tool pkt-line pack-raw-stdin
 		else
 			printf "%s %s refs/heads/main\0report-status object-format=$GIT_DEFAULT_HASH\n" \
-				$A $B | packetize
+				$A $B | test-tool pkt-line pack-raw-stdin
 		fi &&
 		printf "%s %s refs/for/main/topic1\n" \
 			$ZERO_OID $A | test-tool pkt-line pack &&
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index e6c8338b648..23a8a8d5c70 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -64,7 +64,8 @@ test_expect_success 'setup' '
 	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
 	{
 		printf "%s %s refs/heads/newbranch\\0report-status object-format=%s\\n" \
-			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize &&
+			"$ZERO_OID" "$hash_next" "$(test_oid algo)" |
+			test-tool pkt-line pack-raw-stdin &&
 		printf 0000 &&
 		echo "$hash_next" | git pack-objects --stdout
 	} >push_body &&
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 2dde0348816..b52afb0cdea 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -193,10 +193,12 @@ test_expect_success 'hostname cannot break out of directory' '
 '
 
 test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
-	{
-		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
-		printf "0000"
-	} >input &&
+	printf "git-upload-pack /interp.git\n\0host=localhost" >has-null &&
+	test-tool pkt-line pack-raw-stdin >input <has-null &&
+	test-tool pkt-line pack >>input <<-\EOF &&
+	0000
+	EOF
+
 	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
 	test-tool pkt-line unpack <output >actual &&
 
-- 
2.32.0.636.g43e71d69cff

[PATCH 5/5] test-lib-functions.sh: remove unused [de]packetize() functions

[Ævar Arnfjörð Bjarmason]

Remove the now-unused "packetize()" and "depacketize()" functions
added in 4414a150025 (t/lib-git-daemon: add network-protocol helpers,
2018-01-24). As discussed in the preceding commits we've now moved all
their users over to "test-tool pkt-line".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 42 -----------------------------------------
 1 file changed, 42 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index f0448daa74b..13971aa4e34 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1453,48 +1453,6 @@ nongit () {
 	)
 } 7>&2 2>&4
 
-# convert function arguments or stdin (if not arguments given) to pktline
-# representation. If multiple arguments are given, they are separated by
-# whitespace and put in a single packet. Note that data containing NULs must be
-# given on stdin, and that empty input becomes an empty packet, not a flush
-# packet (for that you can just print 0000 yourself).
-packetize () {
-	if test $# -gt 0
-	then
-		packet="$*"
-		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
-	else
-		perl -e '
-			my $packet = do { local $/; <STDIN> };
-			printf "%04x%s", 4 + length($packet), $packet;
-		'
-	fi
-}
-
-# Parse the input as a series of pktlines, writing the result to stdout.
-# Sideband markers are removed automatically, and the output is routed to
-# stderr if appropriate.
-#
-# NUL bytes are converted to "\\0" for ease of parsing with text tools.
-depacketize () {
-	perl -e '
-		while (read(STDIN, $len, 4) == 4) {
-			if ($len eq "0000") {
-				print "FLUSH\n";
-			} else {
-				read(STDIN, $buf, hex($len) - 4);
-				$buf =~ s/\0/\\0/g;
-				if ($buf =~ s/^[\x2\x3]//) {
-					print STDERR $buf;
-				} else {
-					$buf =~ s/^\x1//;
-					print $buf;
-				}
-			}
-		}
-	'
-}
-
 # Converts base-16 data into base-8. The output is given as a sequence of
 # escaped octals, suitable for consumption by 'printf'.
 hex2oct () {
-- 
2.32.0.636.g43e71d69cff

[PATCH v2 0/5] tests: migrate to "test-tool pkt-line"

[Ævar Arnfjörð Bjarmason]

This series is marked for "will merge to next" already, but not there
yet. A trivial v2 whitespace fix in case Junio's in time to pick it
up.

See v1 at https://lore.kernel.org/git/cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (5):
  serve tests: add missing "extra delim" test
  serve tests: use test_cmp in "protocol violations" test
  tests: replace [de]packetize() shell+perl test-tool pkt-line
  tests: replace remaining packetize() with "test-tool pkt-line"
  test-lib-functions.sh: remove unused [de]packetize() functions

 t/helper/test-pkt-line.c               | 12 +++++
 t/t5410-receive-pack-alternates.sh     | 42 +++++++++++++-----
 t/t5411/once-0010-report-status-v1.sh  | 12 ++---
 t/t5500-fetch-pack.sh                  | 15 ++++---
 t/t5530-upload-pack-error.sh           | 24 +++++-----
 t/t5562-http-backend-content-length.sh | 16 ++++---
 t/t5570-git-daemon.sh                  | 22 ++++++----
 t/t5704-protocol-violations.sh         | 61 ++++++++++++++++++--------
 t/test-lib-functions.sh                | 42 ------------------
 9 files changed, 135 insertions(+), 111 deletions(-)

Range-diff against v1:
1:  fcb53980597 = 1:  67aa8141153 serve tests: add missing "extra delim" test
2:  c3544fb53cd = 2:  64dfd14865c serve tests: use test_cmp in "protocol violations" test
3:  c1015fa6ab0 = 3:  c33f344ab20 tests: replace [de]packetize() shell+perl test-tool pkt-line
4:  ab23513b48b ! 4:  a44e1790f2a tests: replace remaining packetize() with "test-tool pkt-line"
    @@ t/helper/test-pkt-line.c: static void pack(int argc, const char **argv)
      
     +static void pack_raw_stdin(void)
     +{
    -+
     +	struct strbuf sb = STRBUF_INIT;
     +	strbuf_read(&sb, 0, 0);
     +	if (strbuf_read(&sb, 0, 0) < 0)
5:  2d22b83971a = 5:  cc91d15ef70 test-lib-functions.sh: remove unused [de]packetize() functions
-- 
2.32.0-dev

[PATCH v2 1/5] serve tests: add missing "extra delim" test

[Ævar Arnfjörð Bjarmason]

When the object-info protocol v2 call was added in
a2ba162cda2 (object-info: support for retrieving object info,
2021-04-20) we didn't add a corresponding test here.

We had tests for ls-refs and fetch already since
4845b772458 (upload-pack: handle unexpected delim packets,
2020-03-27), but the same behavior in object-info (which is clearly
copied from the template of "ls-refs") did not have the corresponding
tests.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5704-protocol-violations.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 5c941949b98..95f68e1d4b7 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -32,4 +32,21 @@ test_expect_success 'extra delim packet in v2 fetch args' '
 	test_i18ngrep "expected flush after fetch arguments" err
 '
 
+test_expect_success 'extra delim packet in v2 object-info args' '
+	# protocol expects 0000 flush after the 0001
+	test-tool pkt-line pack >input <<-EOF &&
+	command=object-info
+	object-format=$(test_oid algo)
+	0001
+	0001
+	EOF
+
+	cat >err.expect <<-\EOF &&
+	fatal: object-info: expected flush after arguments
+	EOF
+	test_must_fail env GIT_PROTOCOL=version=2 \
+		git upload-pack . <input 2>err.actual &&
+	test_cmp err.expect err.actual
+'
+
 test_done
-- 
2.32.0-dev

[PATCH v2 2/5] serve tests: use test_cmp in "protocol violations" test

[Ævar Arnfjörð Bjarmason]

Amend the protocol violations tests to check the full output, not just
grep it. This changes code added in 4845b772458 (upload-pack: handle
unexpected delim packets, 2020-03-27). The newly added test in the
preceding commit already did the full test_cmp.

I have a related series to tweak the output from upload-pack et al, we
really want to make sure we have this exact output, and not fewer or
more lines etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5704-protocol-violations.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 95f68e1d4b7..038fffd3d03 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -14,9 +14,12 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '
 		# protocol expects 0000 flush here
 		printf 0001
 	} >input &&
+	cat >err.expect <<-\EOF &&
+	fatal: expected flush after ls-refs arguments
+	EOF
 	test_must_fail env GIT_PROTOCOL=version=2 \
-		git upload-pack . <input 2>err &&
-	test_i18ngrep "expected flush after ls-refs arguments" err
+		git upload-pack . <input 2>err.actual &&
+	test_cmp err.expect err.actual
 '
 
 test_expect_success 'extra delim packet in v2 fetch args' '
@@ -27,9 +30,12 @@ test_expect_success 'extra delim packet in v2 fetch args' '
 		# protocol expects 0000 flush here
 		printf 0001
 	} >input &&
+	cat >err.expect <<-\EOF &&
+	fatal: expected flush after fetch arguments
+	EOF
 	test_must_fail env GIT_PROTOCOL=version=2 \
-		git upload-pack . <input 2>err &&
-	test_i18ngrep "expected flush after fetch arguments" err
+		git upload-pack . <input 2>err.actual &&
+	test_cmp err.expect err.actual
 '
 
 test_expect_success 'extra delim packet in v2 object-info args' '
-- 
2.32.0-dev

[PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line

[Ævar Arnfjörð Bjarmason]

The shell+perl "[de]packetize()" helper functions were added in
4414a150025 (t/lib-git-daemon: add network-protocol helpers,
2018-01-24), and around the same time we added the "pkt-line" helper
in 74e70029615 (test-pkt-line: introduce a packet-line test helper,
2018-03-14).

For some reason it seems we've mostly used the shell+perl version
instead of the helper since then. There were discussions around
88124ab2636 (test-lib-functions: make packetize() more efficient,
2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()
stdin code, 2020-03-29) to improve them and make them more efficient.

Let's instead just use the test helper, I think this results in both
more legible code, and for anyone who cares about efficiency it'll be
faster.

We can't convert all the users of packetize(), it has a feature the
test-tool is missing. This'll be addressed in the subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5410-receive-pack-alternates.sh     | 42 ++++++++++++++++++--------
 t/t5411/once-0010-report-status-v1.sh  |  8 ++---
 t/t5500-fetch-pack.sh                  | 15 +++++----
 t/t5530-upload-pack-error.sh           | 24 ++++++++-------
 t/t5562-http-backend-content-length.sh | 13 ++++----
 t/t5570-git-daemon.sh                  | 12 +++++---
 t/t5704-protocol-violations.sh         | 30 +++++++++---------
 7 files changed, 86 insertions(+), 58 deletions(-)

diff --git a/t/t5410-receive-pack-alternates.sh b/t/t5410-receive-pack-alternates.sh
index 0b28e4e452f..d0053d95a44 100755
--- a/t/t5410-receive-pack-alternates.sh
+++ b/t/t5410-receive-pack-alternates.sh
@@ -16,10 +16,6 @@ test_expect_success 'setup' '
 	test_commit private
 '
 
-extract_haves () {
-	depacketize | perl -lne '/^(\S+) \.have/ and print $1'
-}
-
 test_expect_success 'with core.alternateRefsCommand' '
 	write_script fork/alternate-refs <<-\EOF &&
 		git --git-dir="$1" for-each-ref \
@@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '
 			refs/heads/public/
 	EOF
 	test_config -C fork core.alternateRefsCommand ./alternate-refs &&
-	git rev-parse public/branch >expect &&
-	printf "0000" | git receive-pack fork >actual &&
-	extract_haves <actual >actual.haves &&
-	test_cmp expect actual.haves
+
+	test-tool pkt-line pack >in <<-\EOF &&
+	0000
+	EOF
+
+	cat >expect <<-EOF &&
+	$(git rev-parse main) refs/heads/main
+	$(git rev-parse base) refs/tags/base
+	$(git rev-parse public) .have
+	0000
+	EOF
+
+	git receive-pack fork >out <in &&
+	test-tool pkt-line unpack <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'with core.alternateRefsPrefixes' '
 	test_config -C fork core.alternateRefsPrefixes "refs/heads/private" &&
-	git rev-parse private/branch >expect &&
-	printf "0000" | git receive-pack fork >actual &&
-	extract_haves <actual >actual.haves &&
-	test_cmp expect actual.haves
+
+	test-tool pkt-line pack >in <<-\EOF &&
+	0000
+	EOF
+
+	cat >expect <<-EOF &&
+	$(git rev-parse main) refs/heads/main
+	$(git rev-parse base) refs/tags/base
+	$(git rev-parse private) .have
+	0000
+	EOF
+
+	git receive-pack fork >out <in &&
+	test-tool pkt-line unpack <out >actual &&
+	test_cmp expect actual
 '
 
 test_done
diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
index 1233a46eac5..cf33d993192 100644
--- a/t/t5411/once-0010-report-status-v1.sh
+++ b/t/t5411/once-0010-report-status-v1.sh
@@ -34,13 +34,13 @@ test_expect_success "proc-receive: report status v1" '
 				$A $B | packetize
 		fi &&
 		printf "%s %s refs/for/main/topic1\n" \
-			$ZERO_OID $A | packetize &&
+			$ZERO_OID $A | test-tool pkt-line pack &&
 		printf "%s %s refs/heads/foo\n" \
-			$ZERO_OID $A | packetize &&
+			$ZERO_OID $A | test-tool pkt-line pack &&
 		printf "%s %s refs/for/next/topic\n" \
-			$ZERO_OID $A | packetize &&
+			$ZERO_OID $A | test-tool pkt-line pack &&
 		printf "%s %s refs/for/main/topic2\n" \
-			$ZERO_OID $A | packetize &&
+			$ZERO_OID $A | test-tool pkt-line pack &&
 		printf 0000 &&
 		printf "" | git -C "$upstream" pack-objects --stdout
 	} | git receive-pack "$upstream" --stateless-rpc \
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 8a5d3492c71..ff0b7dd89f9 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -872,14 +872,17 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '
 	git commit-graph write --reachable &&
 	git config core.commitGraph true &&
 
-	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
-	0012command=fetch
-	$(echo "object-format=$(test_oid algo)" | packetize)
-	00010013deepen-since 1
-	$(echo "want $(git rev-parse other)" | packetize)
-	$(echo "have $(git rev-parse main)" | packetize)
+	test-tool pkt-line pack >in <<-EOF &&
+	command=fetch
+	object-format=$(test_oid algo)
+	0001
+	deepen-since 1
+	want $(git rev-parse other)
+	have $(git rev-parse main)
 	0000
 	EOF
+
+	GIT_PROTOCOL=version=2 git upload-pack . <in >/dev/null
 	)
 '
 
diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh
index 7c1460eaa99..8ccaae10475 100755
--- a/t/t5530-upload-pack-error.sh
+++ b/t/t5530-upload-pack-error.sh
@@ -90,18 +90,20 @@ test_expect_success 'upload-pack fails due to error in pack-objects enumeration'
 
 test_expect_success 'upload-pack tolerates EOF just after stateless client wants' '
 	test_commit initial &&
-	head=$(git rev-parse HEAD) &&
-
-	{
-		packetize "want $head" &&
-		packetize "shallow $head" &&
-		packetize "deepen 1" &&
-		printf "0000"
-	} >request &&
 
-	printf "0000" >expect &&
-
-	git upload-pack --stateless-rpc . <request >actual &&
+	head=$(git rev-parse HEAD) &&
+	test-tool pkt-line pack >request <<-EOF &&
+	want $head
+	shallow $head
+	deepen 1
+	0000
+	EOF
+
+	cat >expect <<-\EOF &&
+	0000
+	EOF
+	git upload-pack --stateless-rpc . <request >out &&
+	test-tool pkt-line unpack <out >actual &&
 	test_cmp expect actual
 '
 
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index e5d3d15ba8d..e6c8338b648 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -53,12 +53,13 @@ test_expect_success 'setup' '
 	test_commit c1 &&
 	hash_head=$(git rev-parse HEAD) &&
 	hash_prev=$(git rev-parse HEAD~1) &&
-	{
-		packetize "want $hash_head" &&
-		printf 0000 &&
-		packetize "have $hash_prev" &&
-		packetize "done"
-	} >fetch_body &&
+	test-tool pkt-line pack >fetch_body <<-EOF &&
+	want $hash_head
+	0000
+	have $hash_prev
+	done
+	0000
+	EOF
 	test_copy_bytes 10 <fetch_body >fetch_body.trunc &&
 	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
 	{
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 82c31ab6cd8..2dde0348816 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
 		printf "0000"
 	} >input &&
 	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
-	depacketize <output >output.raw &&
+	test-tool pkt-line unpack <output >actual &&
+
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) HEAD
+	$(git rev-parse refs/heads/main) refs/heads/main
+	0000
+	EOF
 
-	# just pick out the value of main, which avoids any protocol
-	# particulars
-	perl -lne "print \$1 if m{^(\\S+) refs/heads/main}" <output.raw >actual &&
-	git -C "$repo" rev-parse main >expect &&
 	test_cmp expect actual
 '
 
diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 038fffd3d03..44e2c0d3ded 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -7,13 +7,14 @@ making sure that we do not segfault or otherwise behave badly.'
 . ./test-lib.sh
 
 test_expect_success 'extra delim packet in v2 ls-refs args' '
-	{
-		packetize command=ls-refs &&
-		packetize "object-format=$(test_oid algo)" &&
-		printf 0001 &&
-		# protocol expects 0000 flush here
-		printf 0001
-	} >input &&
+	# protocol expects 0000 flush after the 0001
+	test-tool pkt-line pack >input <<-EOF &&
+	command=ls-refs
+	object-format=$(test_oid algo)
+	0001
+	0001
+	EOF
+
 	cat >err.expect <<-\EOF &&
 	fatal: expected flush after ls-refs arguments
 	EOF
@@ -23,13 +24,14 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '
 '
 
 test_expect_success 'extra delim packet in v2 fetch args' '
-	{
-		packetize command=fetch &&
-		packetize "object-format=$(test_oid algo)" &&
-		printf 0001 &&
-		# protocol expects 0000 flush here
-		printf 0001
-	} >input &&
+	# protocol expects 0000 flush after the 0001
+	test-tool pkt-line pack >input <<-EOF &&
+	command=fetch
+	object-format=$(test_oid algo)
+	0001
+	0001
+	EOF
+
 	cat >err.expect <<-\EOF &&
 	fatal: expected flush after fetch arguments
 	EOF
-- 
2.32.0-dev

Re: [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line

[Jeff King]

On Mon, Jul 12, 2021 at 06:44:18PM +0200, Ævar Arnfjörð Bjarmason wrote:

> The shell+perl "[de]packetize()" helper functions were added in
> 4414a150025 (t/lib-git-daemon: add network-protocol helpers,
> 2018-01-24), and around the same time we added the "pkt-line" helper
> in 74e70029615 (test-pkt-line: introduce a packet-line test helper,
> 2018-03-14).
> 
> For some reason it seems we've mostly used the shell+perl version
> instead of the helper since then. There were discussions around
> 88124ab2636 (test-lib-functions: make packetize() more efficient,
> 2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()
> stdin code, 2020-03-29) to improve them and make them more efficient.

This seems like a good goal, and the conversions look mostly faithful
(with one exception below). Having the helper tool recognize "0000" as a
flush makes some of the scripts much nicer to read (even if it is
technically ambiguous in the input).

I did seem like there were some other stylistic things happening, too,
though. For instance:

> -extract_haves () {
> -	depacketize | perl -lne '/^(\S+) \.have/ and print $1'
> -}
> -
>  test_expect_success 'with core.alternateRefsCommand' '
>  	write_script fork/alternate-refs <<-\EOF &&
>  		git --git-dir="$1" for-each-ref \
> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '
>  			refs/heads/public/
>  	EOF
>  	test_config -C fork core.alternateRefsCommand ./alternate-refs &&
> -	git rev-parse public/branch >expect &&
> -	printf "0000" | git receive-pack fork >actual &&
> -	extract_haves <actual >actual.haves &&
> -	test_cmp expect actual.haves
> +
> +	test-tool pkt-line pack >in <<-\EOF &&
> +	0000
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	$(git rev-parse main) refs/heads/main
> +	$(git rev-parse base) refs/tags/base
> +	$(git rev-parse public) .have
> +	0000
> +	EOF

This is testing the whole output, rather than just the "have" lines (as
the original did). It was intentionally written the other way for two
reasons:

  - it keeps the focus on what we are actually testing: the .have
    behavior

  - it makes the test less brittle to other changes in the script

I can buy the argument that sometimes testing the whole output, even if
it is more brittle, helps us find other unexpected outcomes (e.g., the
stderr test_cmp vs grep thing earlier in the series). But here it just
seems strictly worse to me.

It would be easy to just replace depacketize with "pktline unpack", but
keep the perl one-liner. I don't think it's a _huge_ deal in this case
either way, but I'm not enthused about the trend.

> +	test-tool pkt-line pack >in <<-\EOF &&
> +	0000
> +	EOF

This used to just be "printf 0000". The new version is longer and less
efficient, but I'm OK with it on the grounds of consistency (all inputs
flow through "pkt-line pack", and all outputs through "pkt-line unpack").

> diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
> index 1233a46eac5..cf33d993192 100644
> --- a/t/t5411/once-0010-report-status-v1.sh
> +++ b/t/t5411/once-0010-report-status-v1.sh
> @@ -34,13 +34,13 @@ test_expect_success "proc-receive: report status v1" '
>  				$A $B | packetize
>  		fi &&
>  		printf "%s %s refs/for/main/topic1\n" \
> -			$ZERO_OID $A | packetize &&
> +			$ZERO_OID $A | test-tool pkt-line pack &&
>  		printf "%s %s refs/heads/foo\n" \
> -			$ZERO_OID $A | packetize &&
> +			$ZERO_OID $A | test-tool pkt-line pack &&
>  		printf "%s %s refs/for/next/topic\n" \
> -			$ZERO_OID $A | packetize &&
> +			$ZERO_OID $A | test-tool pkt-line pack &&
>  		printf "%s %s refs/for/main/topic2\n" \
> -			$ZERO_OID $A | packetize &&
> +			$ZERO_OID $A | test-tool pkt-line pack &&

Now that you're using the multi-line-capable helper, these could all be
a single (and much more readable):

  test-tool pkt-line pack <<-EOF
  $ZERO_OID $A refs/for/main/topic1
  $ZERO_OID $A refs/heads/foo
  $ZERO_OID $A refs/for/next/topic
  $ZERO_OID $A refs/for/main/topic2
  EOF

couldn't they?

> -	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
> -	0012command=fetch
> -	$(echo "object-format=$(test_oid algo)" | packetize)
> -	00010013deepen-since 1
> -	$(echo "want $(git rev-parse other)" | packetize)
> -	$(echo "have $(git rev-parse main)" | packetize)
> +	test-tool pkt-line pack >in <<-EOF &&
> +	command=fetch
> +	object-format=$(test_oid algo)
> +	0001
> +	deepen-since 1
> +	want $(git rev-parse other)
> +	have $(git rev-parse main)
>  	0000
>  	EOF

This is much more readable. Nice.

> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
> index e5d3d15ba8d..e6c8338b648 100755
> --- a/t/t5562-http-backend-content-length.sh
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -53,12 +53,13 @@ test_expect_success 'setup' '
>  	test_commit c1 &&
>  	hash_head=$(git rev-parse HEAD) &&
>  	hash_prev=$(git rev-parse HEAD~1) &&
> -	{
> -		packetize "want $hash_head" &&
> -		printf 0000 &&
> -		packetize "have $hash_prev" &&
> -		packetize "done"
> -	} >fetch_body &&
> +	test-tool pkt-line pack >fetch_body <<-EOF &&
> +	want $hash_head
> +	0000
> +	have $hash_prev
> +	done
> +	0000
> +	EOF

There's a flush packet at the end of your input in the post-image that
doesn't seem to be in the original.

> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index 82c31ab6cd8..2dde0348816 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
>  		printf "0000"
>  	} >input &&

This one doesn't use "pkt-line pack". Which is good, because we care
about being exact about things like newlines here.

>  	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
> -	depacketize <output >output.raw &&
> +	test-tool pkt-line unpack <output >actual &&
> +
> +	cat >expect <<-EOF &&
> +	$(git rev-parse HEAD) HEAD
> +	$(git rev-parse refs/heads/main) refs/heads/main
> +	0000
> +	EOF
>  
> -	# just pick out the value of main, which avoids any protocol
> -	# particulars
> -	perl -lne "print \$1 if m{^(\\S+) refs/heads/main}" <output.raw >actual &&
> -	git -C "$repo" rev-parse main >expect &&
>  	test_cmp expect actual
>  '

This is another case where you're checking the output for more than the
original did, ignoring the comment. :) When using depacketize, the bits
after the "\0" were encoded and kept, so it was necessary. The pkt-line
helper just throws those bits away, so it's OK (I'm a little surprised
that discarding those bits wasn't a roadblock for converting some tests,
but I guess we didn't have any low-level protocol tests that cared).

-Peff

Re: [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 13 2021, Jeff King wrote:

> On Mon, Jul 12, 2021 at 06:44:18PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> The shell+perl "[de]packetize()" helper functions were added in
>> 4414a150025 (t/lib-git-daemon: add network-protocol helpers,
>> 2018-01-24), and around the same time we added the "pkt-line" helper
>> in 74e70029615 (test-pkt-line: introduce a packet-line test helper,
>> 2018-03-14).
>> 
>> For some reason it seems we've mostly used the shell+perl version
>> instead of the helper since then. There were discussions around
>> 88124ab2636 (test-lib-functions: make packetize() more efficient,
>> 2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()
>> stdin code, 2020-03-29) to improve them and make them more efficient.
>
> This seems like a good goal, and the conversions look mostly faithful
> (with one exception below). Having the helper tool recognize "0000" as a
> flush makes some of the scripts much nicer to read (even if it is
> technically ambiguous in the input).
>
> I did seem like there were some other stylistic things happening, too,
> though. For instance:
>
>> -extract_haves () {
>> -	depacketize | perl -lne '/^(\S+) \.have/ and print $1'
>> -}
>> -
>>  test_expect_success 'with core.alternateRefsCommand' '
>>  	write_script fork/alternate-refs <<-\EOF &&
>>  		git --git-dir="$1" for-each-ref \
>> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '
>>  			refs/heads/public/
>>  	EOF
>>  	test_config -C fork core.alternateRefsCommand ./alternate-refs &&
>> -	git rev-parse public/branch >expect &&
>> -	printf "0000" | git receive-pack fork >actual &&
>> -	extract_haves <actual >actual.haves &&
>> -	test_cmp expect actual.haves
>> +
>> +	test-tool pkt-line pack >in <<-\EOF &&
>> +	0000
>> +	EOF
>> +
>> +	cat >expect <<-EOF &&
>> +	$(git rev-parse main) refs/heads/main
>> +	$(git rev-parse base) refs/tags/base
>> +	$(git rev-parse public) .have
>> +	0000
>> +	EOF
>
> This is testing the whole output, rather than just the "have" lines (as
> the original did). It was intentionally written the other way for two
> reasons:
>
>   - it keeps the focus on what we are actually testing: the .have
>     behavior
>
>   - it makes the test less brittle to other changes in the script
>
> I can buy the argument that sometimes testing the whole output, even if
> it is more brittle, helps us find other unexpected outcomes (e.g., the
> stderr test_cmp vs grep thing earlier in the series). But here it just
> seems strictly worse to me.
>
> It would be easy to just replace depacketize with "pktline unpack", but
> keep the perl one-liner. I don't think it's a _huge_ deal in this case
> either way, but I'm not enthused about the trend.

FWIW this series was spun off from an effort of fixing a bug related to
protocol-y tests doing just such a "we can grep the output, we know we
only need xyz", and the only test for it not failing because we picked
the wrong xyz.

In this case yeah we could keep the grep. I do think in general that
unless output is overly verbose we should test_cmp it, and in this case
it's really not.

On the focus it seems it's the opposite for the two uf us. It takes my
focus away from the test itself when reading it. I.e. I start wondering
why the grep, is the output variable or whatever, especially in a case
like this where we're hardly saving ourselves overall lines or reducing
complexity.

>> +	test-tool pkt-line pack >in <<-\EOF &&
>> +	0000
>> +	EOF
>
> This used to just be "printf 0000". The new version is longer and less
> efficient, but I'm OK with it on the grounds of consistency (all inputs
> flow through "pkt-line pack", and all outputs through "pkt-line unpack").

They aren't equivalent because the pkt-line helper (and pkt-line.c in
general) will be forgiving about the presence or lack of trailing
newlines, but some of these tests were not.

I think we should use the helper in/out for all of those, because we're
explicitly interested if the protocol round-trips the way we expect, and
not per-se if the exact bytes match.

>> diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
>> index 1233a46eac5..cf33d993192 100644
>> --- a/t/t5411/once-0010-report-status-v1.sh
>> +++ b/t/t5411/once-0010-report-status-v1.sh
>> @@ -34,13 +34,13 @@ test_expect_success "proc-receive: report status v1" '
>>  				$A $B | packetize
>>  		fi &&
>>  		printf "%s %s refs/for/main/topic1\n" \
>> -			$ZERO_OID $A | packetize &&
>> +			$ZERO_OID $A | test-tool pkt-line pack &&
>>  		printf "%s %s refs/heads/foo\n" \
>> -			$ZERO_OID $A | packetize &&
>> +			$ZERO_OID $A | test-tool pkt-line pack &&
>>  		printf "%s %s refs/for/next/topic\n" \
>> -			$ZERO_OID $A | packetize &&
>> +			$ZERO_OID $A | test-tool pkt-line pack &&
>>  		printf "%s %s refs/for/main/topic2\n" \
>> -			$ZERO_OID $A | packetize &&
>> +			$ZERO_OID $A | test-tool pkt-line pack &&
>
> Now that you're using the multi-line-capable helper, these could all be
> a single (and much more readable):
>
>   test-tool pkt-line pack <<-EOF
>   $ZERO_OID $A refs/for/main/topic1
>   $ZERO_OID $A refs/heads/foo
>   $ZERO_OID $A refs/for/next/topic
>   $ZERO_OID $A refs/for/main/topic2
>   EOF
>
> couldn't they?

Yeah, but you never know what you'll get "let's do the small change"
v.s. "let's avoid refactoring while we're at it" feedback :)

I figured it was easier to review with just the s/packetize/test-tool
pkt-line pack/g, but sure, I can change it.

>> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
>> index e5d3d15ba8d..e6c8338b648 100755
>> --- a/t/t5562-http-backend-content-length.sh
>> +++ b/t/t5562-http-backend-content-length.sh
>> @@ -53,12 +53,13 @@ test_expect_success 'setup' '
>>  	test_commit c1 &&
>>  	hash_head=$(git rev-parse HEAD) &&
>>  	hash_prev=$(git rev-parse HEAD~1) &&
>> -	{
>> -		packetize "want $hash_head" &&
>> -		printf 0000 &&
>> -		packetize "have $hash_prev" &&
>> -		packetize "done"
>> -	} >fetch_body &&
>> +	test-tool pkt-line pack >fetch_body <<-EOF &&
>> +	want $hash_head
>> +	0000
>> +	have $hash_prev
>> +	done
>> +	0000
>> +	EOF
>
> There's a flush packet at the end of your input in the post-image that
> doesn't seem to be in the original.

Yeah, but isn't round-tripping through the helper the right thing here?

>> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
>> index 82c31ab6cd8..2dde0348816 100755
>> --- a/t/t5570-git-daemon.sh
>> +++ b/t/t5570-git-daemon.sh
>> @@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
>>  		printf "0000"
>>  	} >input &&
>
> This one doesn't use "pkt-line pack". Which is good, because we care
> about being exact about things like newlines here.

Yes, as opposed to here, where we don't want the helper.

>>  	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
>> -	depacketize <output >output.raw &&
>> +	test-tool pkt-line unpack <output >actual &&
>> +
>> +	cat >expect <<-EOF &&
>> +	$(git rev-parse HEAD) HEAD
>> +	$(git rev-parse refs/heads/main) refs/heads/main
>> +	0000
>> +	EOF
>>  
>> -	# just pick out the value of main, which avoids any protocol
>> -	# particulars
>> -	perl -lne "print \$1 if m{^(\\S+) refs/heads/main}" <output.raw >actual &&
>> -	git -C "$repo" rev-parse main >expect &&
>>  	test_cmp expect actual
>>  '
>
> This is another case where you're checking the output for more than the
> original did, ignoring the comment. :) When using depacketize, the bits
> after the "\0" were encoded and kept, so it was necessary. The pkt-line
> helper just throws those bits away, so it's OK (I'm a little surprised
> that discarding those bits wasn't a roadblock for converting some tests,
> but I guess we didn't have any low-level protocol tests that cared).

... I see you got to this bit in 4/5.

Re: [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line

[Jeff King]

On Wed, Jul 14, 2021 at 01:41:27AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > This is testing the whole output, rather than just the "have" lines (as
> > the original did). It was intentionally written the other way for two
> > reasons:
> >
> >   - it keeps the focus on what we are actually testing: the .have
> >     behavior
> >
> >   - it makes the test less brittle to other changes in the script
> >
> > I can buy the argument that sometimes testing the whole output, even if
> > it is more brittle, helps us find other unexpected outcomes (e.g., the
> > stderr test_cmp vs grep thing earlier in the series). But here it just
> > seems strictly worse to me.
> >
> > It would be easy to just replace depacketize with "pktline unpack", but
> > keep the perl one-liner. I don't think it's a _huge_ deal in this case
> > either way, but I'm not enthused about the trend.
> 
> FWIW this series was spun off from an effort of fixing a bug related to
> protocol-y tests doing just such a "we can grep the output, we know we
> only need xyz", and the only test for it not failing because we picked
> the wrong xyz.
> 
> In this case yeah we could keep the grep. I do think in general that
> unless output is overly verbose we should test_cmp it, and in this case
> it's really not.
> 
> On the focus it seems it's the opposite for the two uf us. It takes my
> focus away from the test itself when reading it. I.e. I start wondering
> why the grep, is the output variable or whatever, especially in a case
> like this where we're hardly saving ourselves overall lines or reducing
> complexity.

OK. I don't really agree, but I don't feel strongly enough to argue
about it. (I guess I'm a little more invested here because you're not
writing new tests, which I would definitely not have said anything
about, but rather changing tests that I wrote. ;) ).

> >> +	test-tool pkt-line pack >in <<-\EOF &&
> >> +	0000
> >> +	EOF
> >
> > This used to just be "printf 0000". The new version is longer and less
> > efficient, but I'm OK with it on the grounds of consistency (all inputs
> > flow through "pkt-line pack", and all outputs through "pkt-line unpack").
> 
> They aren't equivalent because the pkt-line helper (and pkt-line.c in
> general) will be forgiving about the presence or lack of trailing
> newlines, but some of these tests were not.
> 
> I think we should use the helper in/out for all of those, because we're
> explicitly interested if the protocol round-trips the way we expect, and
> not per-se if the exact bytes match.

That's not at all true for flush packets, though. They _must_ be exactly
"0000", no newlines or anything else. The pkt-line tool is smart enough
to drop the newline in this case (i.e., it recognizes that this is not a
packet that says "0000", but a special flush token).

So it really is byte-equivalent to "printf 0000", and it must be.

There is a separate issue, which I didn't raise, which is that:

  printf foo | packetize

may be different than:

  test-tool pkt-line pack <<\EOF
  foo
  EOF

For the most part, I agree it is not that important. Some of the tests
really do care (because they are testing syntactic stuff), and those
should be using your new raw-stdin feature. I did briefly wonder if we
losing a little bit of accidental diversity of the tests to put them all
through a generator that will always add the newline. But it _shouldn't_
matter in general, and if it does, we should be making specific tests
where it does.

> > Now that you're using the multi-line-capable helper, these could all be
> > a single (and much more readable):
> >
> >   test-tool pkt-line pack <<-EOF
> >   $ZERO_OID $A refs/for/main/topic1
> >   $ZERO_OID $A refs/heads/foo
> >   $ZERO_OID $A refs/for/next/topic
> >   $ZERO_OID $A refs/for/main/topic2
> >   EOF
> >
> > couldn't they?
> 
> Yeah, but you never know what you'll get "let's do the small change"
> v.s. "let's avoid refactoring while we're at it" feedback :)
> 
> I figured it was easier to review with just the s/packetize/test-tool
> pkt-line pack/g, but sure, I can change it.

Well, yes, but that seems like a _much_ smaller change than all of the
rewriting you were doing elsewhere, which is actually changing what
we're checking (albeit in a way that is functionally equivalent). I can
live with it either way, but it just seemed weird to go so far in some
hunks and then not here.

> >> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
> >> index e5d3d15ba8d..e6c8338b648 100755
> >> --- a/t/t5562-http-backend-content-length.sh
> >> +++ b/t/t5562-http-backend-content-length.sh
> >> @@ -53,12 +53,13 @@ test_expect_success 'setup' '
> >>  	test_commit c1 &&
> >>  	hash_head=$(git rev-parse HEAD) &&
> >>  	hash_prev=$(git rev-parse HEAD~1) &&
> >> -	{
> >> -		packetize "want $hash_head" &&
> >> -		printf 0000 &&
> >> -		packetize "have $hash_prev" &&
> >> -		packetize "done"
> >> -	} >fetch_body &&
> >> +	test-tool pkt-line pack >fetch_body <<-EOF &&
> >> +	want $hash_head
> >> +	0000
> >> +	have $hash_prev
> >> +	done
> >> +	0000
> >> +	EOF
> >
> > There's a flush packet at the end of your input in the post-image that
> > doesn't seem to be in the original.
> 
> Yeah, but isn't round-tripping through the helper the right thing here?

I'm not sure I understand. In the original, the final bytes in
fetch_body would be "0008done". In yours, it will be "0009done\n0000".
The extra newline is OK, but that added flush packet is not an
equivalent protocol input to feed to git-http-backend. I expect that Git
doesn't complain because after the "done" there shouldn't be anything
said, so the bytes are simply dropped.

Going through pkt-line is good and reasonable, if that's what you mean
by round-tripping. Your input just has an extra "0000" on the final line
of the here-doc.

> >> -	depacketize <output >output.raw &&
> >> +	test-tool pkt-line unpack <output >actual &&
> >> +
> >> +	cat >expect <<-EOF &&
> >> +	$(git rev-parse HEAD) HEAD
> >> +	$(git rev-parse refs/heads/main) refs/heads/main
> >> +	0000
> >> +	EOF
> >>  
> >> -	# just pick out the value of main, which avoids any protocol
> >> -	# particulars
> >> -	perl -lne "print \$1 if m{^(\\S+) refs/heads/main}" <output.raw >actual &&
> >> -	git -C "$repo" rev-parse main >expect &&
> >>  	test_cmp expect actual
> >>  '
> >
> > This is another case where you're checking the output for more than the
> > original did, ignoring the comment. :) When using depacketize, the bits
> > after the "\0" were encoded and kept, so it was necessary. The pkt-line
> > helper just throws those bits away, so it's OK (I'm a little surprised
> > that discarding those bits wasn't a roadblock for converting some tests,
> > but I guess we didn't have any low-level protocol tests that cared).
> 
> ... I see you got to this bit in 4/5.

I'm not sure I was completely clear. Yes, if there is a NUL in the
input, we must use 4/5's raw-stdin mode. But here I am talking about the
_unpack_ operation.

For a v0 protocol response that buries capabilities after a NUL,
depacketize will show it (the "\0" escape is literally part of the
output):

  $ git init foo
  $ cd foo
  $ git commit --allow-empty -m foo
  $ git upload-pack . | depacketize
  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
  2b78ba01d65c1b0a44c0ac0dd8275e7802b74f41 refs/heads/main
  FLUSH

whereas the pkt-line helper will not:

  $ git upload-pack . | ../t/helper/test-tool pkt-line unpack
  2b78ba01d65c1b0a44c0ac0dd8275e7802b74f41 HEAD
  2b78ba01d65c1b0a44c0ac0dd8275e7802b74f41 refs/heads/main
  0000

That's more convenient in many cases, but I was just saying I was
surprised we did not have any tests that ever tested any of the bits
past the NUL (we do, of course, but they are using real Git commands to
make sure the correct stuff happened, and not peeking at the protocol
themselves).

I think it's OK to proceed. If we later do want a test that looks past
the NUL, we can add that feature to the pkt-line helper. Though looking
at the helper implementation, it does not even seem intentional; it
simply treats the packet data as a C string, feeding it to printf. So if
you do want to be thorough in checking expected packet data in outputs,
it is definitely throwing away some of that data.

-Peff

[PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line"

[Ævar Arnfjörð Bjarmason]

Move the only remaining users of "packetize()" over to "test-tool
pkt-line", for this we need a new "pack-raw-stdin" subcommand in the
test-tool. The "pack" command takes input on stdin, but splits it by
"\n", furthermore we'll format the output using C-strings, so the
embedded "\0" being tested for here would cause the string to be
truncated.

So we need another mode that just calls packet_write() on the raw
input we got on stdin.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-pkt-line.c               | 12 ++++++++++++
 t/t5411/once-0010-report-status-v1.sh  |  4 ++--
 t/t5562-http-backend-content-length.sh |  3 ++-
 t/t5570-git-daemon.sh                  | 10 ++++++----
 4 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 5e638f0b970..8563a0da4c5 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -26,6 +26,16 @@ static void pack(int argc, const char **argv)
 	}
 }
 
+static void pack_raw_stdin(void)
+{
+	struct strbuf sb = STRBUF_INIT;
+	strbuf_read(&sb, 0, 0);
+	if (strbuf_read(&sb, 0, 0) < 0)
+		die_errno("failed to read from stdin");
+	packet_write(1, sb.buf, sb.len);
+	strbuf_release(&sb);
+}
+
 static void unpack(void)
 {
 	struct packet_reader reader;
@@ -110,6 +120,8 @@ int cmd__pkt_line(int argc, const char **argv)
 
 	if (!strcmp(argv[1], "pack"))
 		pack(argc - 2, argv + 2);
+	else if (!strcmp(argv[1], "pack-raw-stdin"))
+		pack_raw_stdin();
 	else if (!strcmp(argv[1], "unpack"))
 		unpack();
 	else if (!strcmp(argv[1], "unpack-sideband"))
diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
index cf33d993192..75d4233e49f 100644
--- a/t/t5411/once-0010-report-status-v1.sh
+++ b/t/t5411/once-0010-report-status-v1.sh
@@ -28,10 +28,10 @@ test_expect_success "proc-receive: report status v1" '
 		if test -z "$GIT_DEFAULT_HASH" || test "$GIT_DEFAULT_HASH" = "sha1"
 		then
 			printf "%s %s refs/heads/main\0report-status\n" \
-				$A $B | packetize
+				$A $B | test-tool pkt-line pack-raw-stdin
 		else
 			printf "%s %s refs/heads/main\0report-status object-format=$GIT_DEFAULT_HASH\n" \
-				$A $B | packetize
+				$A $B | test-tool pkt-line pack-raw-stdin
 		fi &&
 		printf "%s %s refs/for/main/topic1\n" \
 			$ZERO_OID $A | test-tool pkt-line pack &&
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index e6c8338b648..23a8a8d5c70 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -64,7 +64,8 @@ test_expect_success 'setup' '
 	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
 	{
 		printf "%s %s refs/heads/newbranch\\0report-status object-format=%s\\n" \
-			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize &&
+			"$ZERO_OID" "$hash_next" "$(test_oid algo)" |
+			test-tool pkt-line pack-raw-stdin &&
 		printf 0000 &&
 		echo "$hash_next" | git pack-objects --stdout
 	} >push_body &&
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 2dde0348816..b52afb0cdea 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -193,10 +193,12 @@ test_expect_success 'hostname cannot break out of directory' '
 '
 
 test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
-	{
-		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
-		printf "0000"
-	} >input &&
+	printf "git-upload-pack /interp.git\n\0host=localhost" >has-null &&
+	test-tool pkt-line pack-raw-stdin >input <has-null &&
+	test-tool pkt-line pack >>input <<-\EOF &&
+	0000
+	EOF
+
 	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
 	test-tool pkt-line unpack <output >actual &&
 
-- 
2.32.0-dev

Re: [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line"

[Jeff King]

On Mon, Jul 12, 2021 at 06:44:19PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Move the only remaining users of "packetize()" over to "test-tool
> pkt-line", for this we need a new "pack-raw-stdin" subcommand in the
> test-tool. The "pack" command takes input on stdin, but splits it by
> "\n", furthermore we'll format the output using C-strings, so the
> embedded "\0" being tested for here would cause the string to be
> truncated.
> 
> So we need another mode that just calls packet_write() on the raw
> input we got on stdin.

Makes sense. Lacking this "raw" version was the sticking point in the
past for using the helper in more places.

> +static void pack_raw_stdin(void)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	strbuf_read(&sb, 0, 0);
> +	if (strbuf_read(&sb, 0, 0) < 0)
> +		die_errno("failed to read from stdin");

What's up with the two reads here?

It looks like just a duplication. And it happens to work because each
one tries to read to eof, making the second one generally a noop.

> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index 2dde0348816..b52afb0cdea 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -193,10 +193,12 @@ test_expect_success 'hostname cannot break out of directory' '
>  '
>  
>  test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
> -	{
> -		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
> -		printf "0000"
> -	} >input &&
> +	printf "git-upload-pack /interp.git\n\0host=localhost" >has-null &&
> +	test-tool pkt-line pack-raw-stdin >input <has-null &&
> +	test-tool pkt-line pack >>input <<-\EOF &&
> +	0000
> +	EOF

This is a minor style nit, but I really prefer redirecting output from
a block (as in the original) rather than iterative appending (in the
result). IMHO it makes it more obvious what is going into the file and
what is not.

I.e.:

  {
          printf "git-upload-pack /interp.git\n\0host=localhost" |
	  test-tool pkt-line pack-raw-stdin &&
	  printf "0000" | test-tool pkt-line pack
  } >input

(again here the packing of "0000" is pointless, but I'm OK with it for
consistency).

-Peff

Re: [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line"

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 13 2021, Jeff King wrote:

> On Mon, Jul 12, 2021 at 06:44:19PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Move the only remaining users of "packetize()" over to "test-tool
>> pkt-line", for this we need a new "pack-raw-stdin" subcommand in the
>> test-tool. The "pack" command takes input on stdin, but splits it by
>> "\n", furthermore we'll format the output using C-strings, so the
>> embedded "\0" being tested for here would cause the string to be
>> truncated.
>> 
>> So we need another mode that just calls packet_write() on the raw
>> input we got on stdin.
>
> Makes sense. Lacking this "raw" version was the sticking point in the
> past for using the helper in more places.
>
>> +static void pack_raw_stdin(void)
>> +{
>> +	struct strbuf sb = STRBUF_INIT;
>> +	strbuf_read(&sb, 0, 0);
>> +	if (strbuf_read(&sb, 0, 0) < 0)
>> +		die_errno("failed to read from stdin");
>
> What's up with the two reads here?
>
> It looks like just a duplication. And it happens to work because each
> one tries to read to eof, making the second one generally a noop.

Yes oops, just a copy/paste error I didn't spot.

>> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
>> index 2dde0348816..b52afb0cdea 100755
>> --- a/t/t5570-git-daemon.sh
>> +++ b/t/t5570-git-daemon.sh
>> @@ -193,10 +193,12 @@ test_expect_success 'hostname cannot break out of directory' '
>>  '
>>  
>>  test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
>> -	{
>> -		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
>> -		printf "0000"
>> -	} >input &&
>> +	printf "git-upload-pack /interp.git\n\0host=localhost" >has-null &&
>> +	test-tool pkt-line pack-raw-stdin >input <has-null &&
>> +	test-tool pkt-line pack >>input <<-\EOF &&
>> +	0000
>> +	EOF
>
> This is a minor style nit, but I really prefer redirecting output from
> a block (as in the original) rather than iterative appending (in the
> result). IMHO it makes it more obvious what is going into the file and
> what is not.
>
> I.e.:
>
>   {
>           printf "git-upload-pack /interp.git\n\0host=localhost" |
> 	  test-tool pkt-line pack-raw-stdin &&
> 	  printf "0000" | test-tool pkt-line pack
>   } >input
>
> (again here the packing of "0000" is pointless, but I'm OK with it for
> consistency).

Sure, I can use {} blocks, but re the reply on 3/5 about "0000"
v.s. "0000\n" you'd like to move back to print not-a-newline here
v.s. having the helper eat lines ending with \n like everywhere else?

It's not incorrect in this case, it just seems less obvious to
me. I.e. the printf in the former case is because we explicitly care
about the exact raw input, if there's a trailing \n or not, in the
latter case we don't, so I think it's clearier to use the usual <<-\EOF
pattern rather than printf.

Re: [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line"

[Jeff King]

On Wed, Jul 14, 2021 at 01:52:09AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > I.e.:
> >
> >   {
> >           printf "git-upload-pack /interp.git\n\0host=localhost" |
> > 	  test-tool pkt-line pack-raw-stdin &&
> > 	  printf "0000" | test-tool pkt-line pack
> >   } >input
> >
> > (again here the packing of "0000" is pointless, but I'm OK with it for
> > consistency).
> 
> Sure, I can use {} blocks, but re the reply on 3/5 about "0000"
> v.s. "0000\n" you'd like to move back to print not-a-newline here
> v.s. having the helper eat lines ending with \n like everywhere else?
> 
> It's not incorrect in this case, it just seems less obvious to
> me. I.e. the printf in the former case is because we explicitly care
> about the exact raw input, if there's a trailing \n or not, in the
> latter case we don't, so I think it's clearier to use the usual <<-\EOF
> pattern rather than printf.

I don't think it's incorrect in any of the cases. I was just noting that
"printf 0000" is fewer characters and fewer processes than either piping
to pkt-line or using a here-doc.

I don't feel strongly on using it if you want to keep things consistent
between the tests. Nor on using a here-doc versus piping (I don't see
any problem with switching between them based on which is shorter or
more readable in any given instance; if you're piping, it could also be
"echo").

-Peff

[PATCH v2 5/5] test-lib-functions.sh: remove unused [de]packetize() functions

[Ævar Arnfjörð Bjarmason]

Remove the now-unused "packetize()" and "depacketize()" functions
added in 4414a150025 (t/lib-git-daemon: add network-protocol helpers,
2018-01-24). As discussed in the preceding commits we've now moved all
their users over to "test-tool pkt-line".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 42 -----------------------------------------
 1 file changed, 42 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b2810478a21..77e4434dcda 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1453,48 +1453,6 @@ nongit () {
 	)
 } 7>&2 2>&4
 
-# convert function arguments or stdin (if not arguments given) to pktline
-# representation. If multiple arguments are given, they are separated by
-# whitespace and put in a single packet. Note that data containing NULs must be
-# given on stdin, and that empty input becomes an empty packet, not a flush
-# packet (for that you can just print 0000 yourself).
-packetize () {
-	if test $# -gt 0
-	then
-		packet="$*"
-		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
-	else
-		perl -e '
-			my $packet = do { local $/; <STDIN> };
-			printf "%04x%s", 4 + length($packet), $packet;
-		'
-	fi
-}
-
-# Parse the input as a series of pktlines, writing the result to stdout.
-# Sideband markers are removed automatically, and the output is routed to
-# stderr if appropriate.
-#
-# NUL bytes are converted to "\\0" for ease of parsing with text tools.
-depacketize () {
-	perl -e '
-		while (read(STDIN, $len, 4) == 4) {
-			if ($len eq "0000") {
-				print "FLUSH\n";
-			} else {
-				read(STDIN, $buf, hex($len) - 4);
-				$buf =~ s/\0/\\0/g;
-				if ($buf =~ s/^[\x2\x3]//) {
-					print STDERR $buf;
-				} else {
-					$buf =~ s/^\x1//;
-					print $buf;
-				}
-			}
-		}
-	'
-}
-
 # Converts base-16 data into base-8. The output is given as a sequence of
 # escaped octals, suitable for consumption by 'printf'.
 hex2oct () {
-- 
2.32.0-dev

Re: [PATCH v2 0/5] tests: migrate to "test-tool pkt-line"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This series is marked for "will merge to next" already, but not there
> yet. A trivial v2 whitespace fix in case Junio's in time to pick it
> up.
>
> See v1 at https://lore.kernel.org/git/cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com/
>
> Ævar Arnfjörð Bjarmason (5):
>   serve tests: add missing "extra delim" test
>   serve tests: use test_cmp in "protocol violations" test
>   tests: replace [de]packetize() shell+perl test-tool pkt-line
>   tests: replace remaining packetize() with "test-tool pkt-line"
>   test-lib-functions.sh: remove unused [de]packetize() functions
>
>  t/helper/test-pkt-line.c               | 12 +++++
>  t/t5410-receive-pack-alternates.sh     | 42 +++++++++++++-----
>  t/t5411/once-0010-report-status-v1.sh  | 12 ++---
>  t/t5500-fetch-pack.sh                  | 15 ++++---
>  t/t5530-upload-pack-error.sh           | 24 +++++-----
>  t/t5562-http-backend-content-length.sh | 16 ++++---
>  t/t5570-git-daemon.sh                  | 22 ++++++----
>  t/t5704-protocol-violations.sh         | 61 ++++++++++++++++++--------
>  t/test-lib-functions.sh                | 42 ------------------
>  9 files changed, 135 insertions(+), 111 deletions(-)
>
> Range-diff against v1:
> 1:  fcb53980597 = 1:  67aa8141153 serve tests: add missing "extra delim" test
> 2:  c3544fb53cd = 2:  64dfd14865c serve tests: use test_cmp in "protocol violations" test
> 3:  c1015fa6ab0 = 3:  c33f344ab20 tests: replace [de]packetize() shell+perl test-tool pkt-line
> 4:  ab23513b48b ! 4:  a44e1790f2a tests: replace remaining packetize() with "test-tool pkt-line"
>     @@ t/helper/test-pkt-line.c: static void pack(int argc, const char **argv)
>       
>      +static void pack_raw_stdin(void)
>      +{
>     -+
>      +	struct strbuf sb = STRBUF_INIT;
>      +	strbuf_read(&sb, 0, 0);
>      +	if (strbuf_read(&sb, 0, 0) < 0)
> 5:  2d22b83971a = 5:  cc91d15ef70 test-lib-functions.sh: remove unused [de]packetize() functions

Well, if you care that much, I'd prefer to also see an blank line
between the end of decl and the first statement.

;-)

Re: [PATCH v2 0/5] tests: migrate to "test-tool pkt-line"

[Jeff King]

On Mon, Jul 12, 2021 at 01:41:13PM -0700, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > This series is marked for "will merge to next" already, but not there
> > yet. A trivial v2 whitespace fix in case Junio's in time to pick it
> > up.
> >
> > See v1 at https://lore.kernel.org/git/cover-0.5-00000000000-20210707T101549Z-avarab@gmail.com/
> >
> > Ævar Arnfjörð Bjarmason (5):
> >   serve tests: add missing "extra delim" test
> >   serve tests: use test_cmp in "protocol violations" test
> >   tests: replace [de]packetize() shell+perl test-tool pkt-line
> >   tests: replace remaining packetize() with "test-tool pkt-line"
> >   test-lib-functions.sh: remove unused [de]packetize() functions
> >
> >  t/helper/test-pkt-line.c               | 12 +++++
> >  t/t5410-receive-pack-alternates.sh     | 42 +++++++++++++-----
> >  t/t5411/once-0010-report-status-v1.sh  | 12 ++---
> >  t/t5500-fetch-pack.sh                  | 15 ++++---
> >  t/t5530-upload-pack-error.sh           | 24 +++++-----
> >  t/t5562-http-backend-content-length.sh | 16 ++++---
> >  t/t5570-git-daemon.sh                  | 22 ++++++----
> >  t/t5704-protocol-violations.sh         | 61 ++++++++++++++++++--------
> >  t/test-lib-functions.sh                | 42 ------------------
> >  9 files changed, 135 insertions(+), 111 deletions(-)
> >
> > Range-diff against v1:
> > 1:  fcb53980597 = 1:  67aa8141153 serve tests: add missing "extra delim" test
> > 2:  c3544fb53cd = 2:  64dfd14865c serve tests: use test_cmp in "protocol violations" test
> > 3:  c1015fa6ab0 = 3:  c33f344ab20 tests: replace [de]packetize() shell+perl test-tool pkt-line
> > 4:  ab23513b48b ! 4:  a44e1790f2a tests: replace remaining packetize() with "test-tool pkt-line"
> >     @@ t/helper/test-pkt-line.c: static void pack(int argc, const char **argv)
> >       
> >      +static void pack_raw_stdin(void)
> >      +{
> >     -+
> >      +	struct strbuf sb = STRBUF_INIT;
> >      +	strbuf_read(&sb, 0, 0);
> >      +	if (strbuf_read(&sb, 0, 0) < 0)
> > 5:  2d22b83971a = 5:  cc91d15ef70 test-lib-functions.sh: remove unused [de]packetize() functions
> 
> Well, if you care that much, I'd prefer to also see an blank line
> between the end of decl and the first statement.

I was offline last week and hadn't had a chance to look until today. I
did find a few things. Mostly style nits that could potentially be
ignored, but two small "correctness" things that somehow manage to not
cause any test failures. :)

-Peff

[PATCH v3 0/5] tests: migrate to "test-tool pkt-line"

[Ævar Arnfjörð Bjarmason]

Change code that uses [de]packetize() to use "test-tool
pkt-line". That we had these two concurrently in the tests is mostly
just a historical anomaly/inconsistency that we can fix.

Hopefully addresses Jeff King's feedback on v2: https://lore.kernel.org/git/cover-0.5-00000000000-20210712T164208Z-avarab@gmail.com/#t

I:

 * Changed the t5411/once-0010-report-status-v1.sh pattern to move
   away from the verbose "printf"..

 * Got rid of the stray strbuf_read()

 * Used a {} block in t/t5562-http-backend-content-length.sh

I kept the test_cmp in favor of the old logic of grepping out specific
parts, now with an updated commit message addressing that in 3/5, and
that we consistently include "0000" in all cases, even when we can
omit it.

Ævar Arnfjörð Bjarmason (5):
  serve tests: add missing "extra delim" test
  serve tests: use test_cmp in "protocol violations" test
  tests: replace [de]packetize() shell+perl test-tool pkt-line
  tests: replace remaining packetize() with "test-tool pkt-line"
  test-lib-functions.sh: remove unused [de]packetize() functions

 t/helper/test-pkt-line.c               | 12 +++++
 t/t5410-receive-pack-alternates.sh     | 42 +++++++++++++-----
 t/t5411/once-0010-report-status-v1.sh  | 20 ++++-----
 t/t5500-fetch-pack.sh                  | 15 ++++---
 t/t5530-upload-pack-error.sh           | 24 +++++-----
 t/t5562-http-backend-content-length.sh | 16 ++++---
 t/t5570-git-daemon.sh                  | 20 ++++++---
 t/t5704-protocol-violations.sh         | 61 ++++++++++++++++++--------
 t/test-lib-functions.sh                | 42 ------------------
 9 files changed, 138 insertions(+), 114 deletions(-)

Range-diff against v2:
1:  67aa814115 = 1:  67aa814115 serve tests: add missing "extra delim" test
2:  64dfd14865 = 2:  64dfd14865 serve tests: use test_cmp in "protocol violations" test
3:  c33f344ab2 ! 3:  92bfd8a87b tests: replace [de]packetize() shell+perl test-tool pkt-line
    @@ Commit message
         more legible code, and for anyone who cares about efficiency it'll be
         faster.
     
    +    The conversion away from extract_haves() in
    +    t5410-receive-pack-alternates.sh and the "just pick out" logic in
    +    t5570-git-daemon.sh isn't strictly required for this migration, but in
    +    this case it's easy enough to test_cmp the whole output, so let's just
    +    do that.
    +
    +    Similarly, there are cases here of changing "printf 0000" to a version
    +    that'll emit a trailing newline after "0000", or where we can do away
    +    with the "0000" trailer. Let's just always include, and include it in
    +    the same way.
    +
         We can't convert all the users of packetize(), it has a feature the
         test-tool is missing. This'll be addressed in the subsequent commit.
     
    @@ t/t5410-receive-pack-alternates.sh: test_expect_success 'with core.alternateRefs
     
      ## t/t5411/once-0010-report-status-v1.sh ##
     @@ t/t5411/once-0010-report-status-v1.sh: test_expect_success "proc-receive: report status v1" '
    + 			printf "%s %s refs/heads/main\0report-status object-format=$GIT_DEFAULT_HASH\n" \
      				$A $B | packetize
      		fi &&
    - 		printf "%s %s refs/for/main/topic1\n" \
    +-		printf "%s %s refs/for/main/topic1\n" \
     -			$ZERO_OID $A | packetize &&
    -+			$ZERO_OID $A | test-tool pkt-line pack &&
    - 		printf "%s %s refs/heads/foo\n" \
    +-		printf "%s %s refs/heads/foo\n" \
     -			$ZERO_OID $A | packetize &&
    -+			$ZERO_OID $A | test-tool pkt-line pack &&
    - 		printf "%s %s refs/for/next/topic\n" \
    +-		printf "%s %s refs/for/next/topic\n" \
     -			$ZERO_OID $A | packetize &&
    -+			$ZERO_OID $A | test-tool pkt-line pack &&
    - 		printf "%s %s refs/for/main/topic2\n" \
    +-		printf "%s %s refs/for/main/topic2\n" \
     -			$ZERO_OID $A | packetize &&
    -+			$ZERO_OID $A | test-tool pkt-line pack &&
    - 		printf 0000 &&
    +-		printf 0000 &&
    ++		test-tool pkt-line pack <<-EOF &&
    ++		$ZERO_OID $A refs/for/main/topic1
    ++		$ZERO_OID $A refs/heads/foo
    ++		$ZERO_OID $A refs/for/next/topic
    ++		$ZERO_OID $A refs/for/main/topic2
    ++		0000
    ++		EOF
      		printf "" | git -C "$upstream" pack-objects --stdout
      	} | git receive-pack "$upstream" --stateless-rpc \
    + 	>out 2>&1 &&
     
      ## t/t5500-fetch-pack.sh ##
     @@ t/t5500-fetch-pack.sh: test_expect_success 'shallow since with commit graph and already-seen commit' '
4:  a44e1790f2 ! 4:  9842c85d1f tests: replace remaining packetize() with "test-tool pkt-line"
    @@ t/helper/test-pkt-line.c: static void pack(int argc, const char **argv)
     +static void pack_raw_stdin(void)
     +{
     +	struct strbuf sb = STRBUF_INIT;
    -+	strbuf_read(&sb, 0, 0);
    ++
     +	if (strbuf_read(&sb, 0, 0) < 0)
     +		die_errno("failed to read from stdin");
     +	packet_write(1, sb.buf, sb.len);
    @@ t/t5411/once-0010-report-status-v1.sh: test_expect_success "proc-receive: report
     -				$A $B | packetize
     +				$A $B | test-tool pkt-line pack-raw-stdin
      		fi &&
    - 		printf "%s %s refs/for/main/topic1\n" \
    - 			$ZERO_OID $A | test-tool pkt-line pack &&
    + 		test-tool pkt-line pack <<-EOF &&
    + 		$ZERO_OID $A refs/for/main/topic1
     
      ## t/t5562-http-backend-content-length.sh ##
     @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '
    @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '
     
      ## t/t5570-git-daemon.sh ##
     @@ t/t5570-git-daemon.sh: test_expect_success 'hostname cannot break out of directory' '
    - '
      
      test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
    --	{
    + 	{
     -		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
     -		printf "0000"
    --	} >input &&
    -+	printf "git-upload-pack /interp.git\n\0host=localhost" >has-null &&
    -+	test-tool pkt-line pack-raw-stdin >input <has-null &&
    -+	test-tool pkt-line pack >>input <<-\EOF &&
    -+	0000
    -+	EOF
    ++		printf "git-upload-pack /interp.git\n\0host=localhost" |
    ++		test-tool pkt-line pack-raw-stdin &&
    ++		test-tool pkt-line pack <<-\EOF
    ++		0000
    ++		EOF
    + 	} >input &&
     +
      	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
      	test-tool pkt-line unpack <output >actual &&
5:  cc91d15ef7 = 5:  7ca83c5a55 test-lib-functions.sh: remove unused [de]packetize() functions
-- 
2.32.0.788.ge724008458

[PATCH v3 1/5] serve tests: add missing "extra delim" test

[Ævar Arnfjörð Bjarmason]

When the object-info protocol v2 call was added in
a2ba162cda2 (object-info: support for retrieving object info,
2021-04-20) we didn't add a corresponding test here.

We had tests for ls-refs and fetch already since
4845b772458 (upload-pack: handle unexpected delim packets,
2020-03-27), but the same behavior in object-info (which is clearly
copied from the template of "ls-refs") did not have the corresponding
tests.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5704-protocol-violations.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 5c941949b9..95f68e1d4b 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -32,4 +32,21 @@ test_expect_success 'extra delim packet in v2 fetch args' '
 	test_i18ngrep "expected flush after fetch arguments" err
 '
 
+test_expect_success 'extra delim packet in v2 object-info args' '
+	# protocol expects 0000 flush after the 0001
+	test-tool pkt-line pack >input <<-EOF &&
+	command=object-info
+	object-format=$(test_oid algo)
+	0001
+	0001
+	EOF
+
+	cat >err.expect <<-\EOF &&
+	fatal: object-info: expected flush after arguments
+	EOF
+	test_must_fail env GIT_PROTOCOL=version=2 \
+		git upload-pack . <input 2>err.actual &&
+	test_cmp err.expect err.actual
+'
+
 test_done
-- 
2.32.0.788.ge724008458

[PATCH v3 2/5] serve tests: use test_cmp in "protocol violations" test

[Ævar Arnfjörð Bjarmason]

Amend the protocol violations tests to check the full output, not just
grep it. This changes code added in 4845b772458 (upload-pack: handle
unexpected delim packets, 2020-03-27). The newly added test in the
preceding commit already did the full test_cmp.

I have a related series to tweak the output from upload-pack et al, we
really want to make sure we have this exact output, and not fewer or
more lines etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5704-protocol-violations.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 95f68e1d4b..038fffd3d0 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -14,9 +14,12 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '
 		# protocol expects 0000 flush here
 		printf 0001
 	} >input &&
+	cat >err.expect <<-\EOF &&
+	fatal: expected flush after ls-refs arguments
+	EOF
 	test_must_fail env GIT_PROTOCOL=version=2 \
-		git upload-pack . <input 2>err &&
-	test_i18ngrep "expected flush after ls-refs arguments" err
+		git upload-pack . <input 2>err.actual &&
+	test_cmp err.expect err.actual
 '
 
 test_expect_success 'extra delim packet in v2 fetch args' '
@@ -27,9 +30,12 @@ test_expect_success 'extra delim packet in v2 fetch args' '
 		# protocol expects 0000 flush here
 		printf 0001
 	} >input &&
+	cat >err.expect <<-\EOF &&
+	fatal: expected flush after fetch arguments
+	EOF
 	test_must_fail env GIT_PROTOCOL=version=2 \
-		git upload-pack . <input 2>err &&
-	test_i18ngrep "expected flush after fetch arguments" err
+		git upload-pack . <input 2>err.actual &&
+	test_cmp err.expect err.actual
 '
 
 test_expect_success 'extra delim packet in v2 object-info args' '
-- 
2.32.0.788.ge724008458

[PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line

[Ævar Arnfjörð Bjarmason]

The shell+perl "[de]packetize()" helper functions were added in
4414a150025 (t/lib-git-daemon: add network-protocol helpers,
2018-01-24), and around the same time we added the "pkt-line" helper
in 74e70029615 (test-pkt-line: introduce a packet-line test helper,
2018-03-14).

For some reason it seems we've mostly used the shell+perl version
instead of the helper since then. There were discussions around
88124ab2636 (test-lib-functions: make packetize() more efficient,
2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()
stdin code, 2020-03-29) to improve them and make them more efficient.

Let's instead just use the test helper, I think this results in both
more legible code, and for anyone who cares about efficiency it'll be
faster.

The conversion away from extract_haves() in
t5410-receive-pack-alternates.sh and the "just pick out" logic in
t5570-git-daemon.sh isn't strictly required for this migration, but in
this case it's easy enough to test_cmp the whole output, so let's just
do that.

Similarly, there are cases here of changing "printf 0000" to a version
that'll emit a trailing newline after "0000", or where we can do away
with the "0000" trailer. Let's just always include, and include it in
the same way.

We can't convert all the users of packetize(), it has a feature the
test-tool is missing. This'll be addressed in the subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5410-receive-pack-alternates.sh     | 42 ++++++++++++++++++--------
 t/t5411/once-0010-report-status-v1.sh  | 16 +++++-----
 t/t5500-fetch-pack.sh                  | 15 +++++----
 t/t5530-upload-pack-error.sh           | 24 ++++++++-------
 t/t5562-http-backend-content-length.sh | 13 ++++----
 t/t5570-git-daemon.sh                  | 12 +++++---
 t/t5704-protocol-violations.sh         | 30 +++++++++---------
 7 files changed, 89 insertions(+), 63 deletions(-)

diff --git a/t/t5410-receive-pack-alternates.sh b/t/t5410-receive-pack-alternates.sh
index 0b28e4e452..d0053d95a4 100755
--- a/t/t5410-receive-pack-alternates.sh
+++ b/t/t5410-receive-pack-alternates.sh
@@ -16,10 +16,6 @@ test_expect_success 'setup' '
 	test_commit private
 '
 
-extract_haves () {
-	depacketize | perl -lne '/^(\S+) \.have/ and print $1'
-}
-
 test_expect_success 'with core.alternateRefsCommand' '
 	write_script fork/alternate-refs <<-\EOF &&
 		git --git-dir="$1" for-each-ref \
@@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '
 			refs/heads/public/
 	EOF
 	test_config -C fork core.alternateRefsCommand ./alternate-refs &&
-	git rev-parse public/branch >expect &&
-	printf "0000" | git receive-pack fork >actual &&
-	extract_haves <actual >actual.haves &&
-	test_cmp expect actual.haves
+
+	test-tool pkt-line pack >in <<-\EOF &&
+	0000
+	EOF
+
+	cat >expect <<-EOF &&
+	$(git rev-parse main) refs/heads/main
+	$(git rev-parse base) refs/tags/base
+	$(git rev-parse public) .have
+	0000
+	EOF
+
+	git receive-pack fork >out <in &&
+	test-tool pkt-line unpack <out >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'with core.alternateRefsPrefixes' '
 	test_config -C fork core.alternateRefsPrefixes "refs/heads/private" &&
-	git rev-parse private/branch >expect &&
-	printf "0000" | git receive-pack fork >actual &&
-	extract_haves <actual >actual.haves &&
-	test_cmp expect actual.haves
+
+	test-tool pkt-line pack >in <<-\EOF &&
+	0000
+	EOF
+
+	cat >expect <<-EOF &&
+	$(git rev-parse main) refs/heads/main
+	$(git rev-parse base) refs/tags/base
+	$(git rev-parse private) .have
+	0000
+	EOF
+
+	git receive-pack fork >out <in &&
+	test-tool pkt-line unpack <out >actual &&
+	test_cmp expect actual
 '
 
 test_done
diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
index 1233a46eac..ddf3da5a26 100644
--- a/t/t5411/once-0010-report-status-v1.sh
+++ b/t/t5411/once-0010-report-status-v1.sh
@@ -33,15 +33,13 @@ test_expect_success "proc-receive: report status v1" '
 			printf "%s %s refs/heads/main\0report-status object-format=$GIT_DEFAULT_HASH\n" \
 				$A $B | packetize
 		fi &&
-		printf "%s %s refs/for/main/topic1\n" \
-			$ZERO_OID $A | packetize &&
-		printf "%s %s refs/heads/foo\n" \
-			$ZERO_OID $A | packetize &&
-		printf "%s %s refs/for/next/topic\n" \
-			$ZERO_OID $A | packetize &&
-		printf "%s %s refs/for/main/topic2\n" \
-			$ZERO_OID $A | packetize &&
-		printf 0000 &&
+		test-tool pkt-line pack <<-EOF &&
+		$ZERO_OID $A refs/for/main/topic1
+		$ZERO_OID $A refs/heads/foo
+		$ZERO_OID $A refs/for/next/topic
+		$ZERO_OID $A refs/for/main/topic2
+		0000
+		EOF
 		printf "" | git -C "$upstream" pack-objects --stdout
 	} | git receive-pack "$upstream" --stateless-rpc \
 	>out 2>&1 &&
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 8a5d3492c7..ff0b7dd89f 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -872,14 +872,17 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '
 	git commit-graph write --reachable &&
 	git config core.commitGraph true &&
 
-	GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
-	0012command=fetch
-	$(echo "object-format=$(test_oid algo)" | packetize)
-	00010013deepen-since 1
-	$(echo "want $(git rev-parse other)" | packetize)
-	$(echo "have $(git rev-parse main)" | packetize)
+	test-tool pkt-line pack >in <<-EOF &&
+	command=fetch
+	object-format=$(test_oid algo)
+	0001
+	deepen-since 1
+	want $(git rev-parse other)
+	have $(git rev-parse main)
 	0000
 	EOF
+
+	GIT_PROTOCOL=version=2 git upload-pack . <in >/dev/null
 	)
 '
 
diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh
index 7c1460eaa9..8ccaae1047 100755
--- a/t/t5530-upload-pack-error.sh
+++ b/t/t5530-upload-pack-error.sh
@@ -90,18 +90,20 @@ test_expect_success 'upload-pack fails due to error in pack-objects enumeration'
 
 test_expect_success 'upload-pack tolerates EOF just after stateless client wants' '
 	test_commit initial &&
-	head=$(git rev-parse HEAD) &&
-
-	{
-		packetize "want $head" &&
-		packetize "shallow $head" &&
-		packetize "deepen 1" &&
-		printf "0000"
-	} >request &&
 
-	printf "0000" >expect &&
-
-	git upload-pack --stateless-rpc . <request >actual &&
+	head=$(git rev-parse HEAD) &&
+	test-tool pkt-line pack >request <<-EOF &&
+	want $head
+	shallow $head
+	deepen 1
+	0000
+	EOF
+
+	cat >expect <<-\EOF &&
+	0000
+	EOF
+	git upload-pack --stateless-rpc . <request >out &&
+	test-tool pkt-line unpack <out >actual &&
 	test_cmp expect actual
 '
 
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index e5d3d15ba8..e6c8338b64 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -53,12 +53,13 @@ test_expect_success 'setup' '
 	test_commit c1 &&
 	hash_head=$(git rev-parse HEAD) &&
 	hash_prev=$(git rev-parse HEAD~1) &&
-	{
-		packetize "want $hash_head" &&
-		printf 0000 &&
-		packetize "have $hash_prev" &&
-		packetize "done"
-	} >fetch_body &&
+	test-tool pkt-line pack >fetch_body <<-EOF &&
+	want $hash_head
+	0000
+	have $hash_prev
+	done
+	0000
+	EOF
 	test_copy_bytes 10 <fetch_body >fetch_body.trunc &&
 	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
 	{
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 82c31ab6cd..2dde034881 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -198,12 +198,14 @@ test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
 		printf "0000"
 	} >input &&
 	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
-	depacketize <output >output.raw &&
+	test-tool pkt-line unpack <output >actual &&
+
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) HEAD
+	$(git rev-parse refs/heads/main) refs/heads/main
+	0000
+	EOF
 
-	# just pick out the value of main, which avoids any protocol
-	# particulars
-	perl -lne "print \$1 if m{^(\\S+) refs/heads/main}" <output.raw >actual &&
-	git -C "$repo" rev-parse main >expect &&
 	test_cmp expect actual
 '
 
diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 038fffd3d0..44e2c0d3de 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -7,13 +7,14 @@ making sure that we do not segfault or otherwise behave badly.'
 . ./test-lib.sh
 
 test_expect_success 'extra delim packet in v2 ls-refs args' '
-	{
-		packetize command=ls-refs &&
-		packetize "object-format=$(test_oid algo)" &&
-		printf 0001 &&
-		# protocol expects 0000 flush here
-		printf 0001
-	} >input &&
+	# protocol expects 0000 flush after the 0001
+	test-tool pkt-line pack >input <<-EOF &&
+	command=ls-refs
+	object-format=$(test_oid algo)
+	0001
+	0001
+	EOF
+
 	cat >err.expect <<-\EOF &&
 	fatal: expected flush after ls-refs arguments
 	EOF
@@ -23,13 +24,14 @@ test_expect_success 'extra delim packet in v2 ls-refs args' '
 '
 
 test_expect_success 'extra delim packet in v2 fetch args' '
-	{
-		packetize command=fetch &&
-		packetize "object-format=$(test_oid algo)" &&
-		printf 0001 &&
-		# protocol expects 0000 flush here
-		printf 0001
-	} >input &&
+	# protocol expects 0000 flush after the 0001
+	test-tool pkt-line pack >input <<-EOF &&
+	command=fetch
+	object-format=$(test_oid algo)
+	0001
+	0001
+	EOF
+
 	cat >err.expect <<-\EOF &&
 	fatal: expected flush after fetch arguments
 	EOF
-- 
2.32.0.788.ge724008458

Re: [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line

[Taylor Blau]

On Wed, Jul 14, 2021 at 02:54:03AM +0200, Ævar Arnfjörð Bjarmason wrote:
> -extract_haves () {
> -	depacketize | perl -lne '/^(\S+) \.have/ and print $1'
> -}
> -
>  test_expect_success 'with core.alternateRefsCommand' '
>  	write_script fork/alternate-refs <<-\EOF &&
>  		git --git-dir="$1" for-each-ref \
> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '
>  			refs/heads/public/
>  	EOF
>  	test_config -C fork core.alternateRefsCommand ./alternate-refs &&
> -	git rev-parse public/branch >expect &&
> -	printf "0000" | git receive-pack fork >actual &&
> -	extract_haves <actual >actual.haves &&
> -	test_cmp expect actual.haves
> +
> +	test-tool pkt-line pack >in <<-\EOF &&
> +	0000
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	$(git rev-parse main) refs/heads/main
> +	$(git rev-parse base) refs/tags/base
> +	$(git rev-parse public) .have
> +	0000
> +	EOF
> +
> +	git receive-pack fork >out <in &&
> +	test-tool pkt-line unpack <out >actual &&

I don't think extracting the haves themselves (and stripping ".have"
from the output) adds much verbosity at all. Wouldn't it be:

  test-tool pkt-line unpack <out >actual &&
  perl -lne '/^(\S+) \.have/ and print $1' <actual >actual.haves

(in fact, it might be quite nice to leave extract_haves as-is changing
depacketize for 'test-tool pkt-line unpack').

For what it's worth, I also don't care enough to argue about it, but I
am curious why the change here was more invasive compared to other
locations (where depacketize was just replaced with the earlier
incantation).

If memory serves, I am the original author of this test (via 89284c1d6c
(transport.c: introduce core.alternateRefsCommand, 2018-10-08)), but the
Perl bits are Peff's from the discussion in:

    https://lore.kernel.org/git/20180922195258.GA20983@sigill.intra.peff.net/

Thanks,
Taylor

Re: [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Wed, Jul 14, 2021 at 02:54:03AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> -extract_haves () {
>> -	depacketize | perl -lne '/^(\S+) \.have/ and print $1'
>> -}
>> -
>>  test_expect_success 'with core.alternateRefsCommand' '
>>  	write_script fork/alternate-refs <<-\EOF &&
>>  		git --git-dir="$1" for-each-ref \
>> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '
>>  			refs/heads/public/
>>  	EOF
>>  	test_config -C fork core.alternateRefsCommand ./alternate-refs &&
>> -	git rev-parse public/branch >expect &&
>> -	printf "0000" | git receive-pack fork >actual &&
>> -	extract_haves <actual >actual.haves &&
>> -	test_cmp expect actual.haves
>> +
>> +	test-tool pkt-line pack >in <<-\EOF &&
>> +	0000
>> +	EOF
>> +
>> +	cat >expect <<-EOF &&
>> +	$(git rev-parse main) refs/heads/main
>> +	$(git rev-parse base) refs/tags/base
>> +	$(git rev-parse public) .have
>> +	0000
>> +	EOF
>> +
>> +	git receive-pack fork >out <in &&
>> +	test-tool pkt-line unpack <out >actual &&
>
> I don't think extracting the haves themselves (and stripping ".have"
> from the output) adds much verbosity at all. Wouldn't it be:
>
>   test-tool pkt-line unpack <out >actual &&
>   perl -lne '/^(\S+) \.have/ and print $1' <actual >actual.haves
>
> (in fact, it might be quite nice to leave extract_haves as-is changing
> depacketize for 'test-tool pkt-line unpack').

I tend to agree with you ane Peff, after reading the resulting tests
myself.

Specifically I have problem with this line of reasoning:

    The conversion away from extract_haves() in ... isn't strictly
    required for this migration, but in this case it's easy enough
    to test_cmp the whole output, so let's just do that.

as if using test_cmp to compare the whole output is always a better
way to test, which is far from cut-and-dried clear and obvious.  The
default ought to be to keep the original behaviour, unless you can
clearly convince others that either (1) the new way is better, or
(2) keeping the old way is too hard and the cost for doing so
outweighs the benefit.

While there certainly is some value in end-to-end tests, they
inevitably become brittle.  We prefer focused tests that verify
things we _care_ about, while keeping the expected vector unaffected
by future changes in details that do not matter in what is being
tested.  If we are interested in ".have"s, we shouldn't be affected
by irrelevant details like what other branches and tags appear in
the output and in what order, for example.

Of course, if there is a solid reason why we would care not just
".have" but other details, it is perfectly justifiable thing to do
to update the tests, but we'd want to see (1) such an unrelated
change done in a separate step and (2) with its own justification.

So...

[PATCH v3 4/5] tests: replace remaining packetize() with "test-tool pkt-line"

[Ævar Arnfjörð Bjarmason]

Move the only remaining users of "packetize()" over to "test-tool
pkt-line", for this we need a new "pack-raw-stdin" subcommand in the
test-tool. The "pack" command takes input on stdin, but splits it by
"\n", furthermore we'll format the output using C-strings, so the
embedded "\0" being tested for here would cause the string to be
truncated.

So we need another mode that just calls packet_write() on the raw
input we got on stdin.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-pkt-line.c               | 12 ++++++++++++
 t/t5411/once-0010-report-status-v1.sh  |  4 ++--
 t/t5562-http-backend-content-length.sh |  3 ++-
 t/t5570-git-daemon.sh                  |  8 ++++++--
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 5e638f0b97..c5e052e537 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -26,6 +26,16 @@ static void pack(int argc, const char **argv)
 	}
 }
 
+static void pack_raw_stdin(void)
+{
+	struct strbuf sb = STRBUF_INIT;
+
+	if (strbuf_read(&sb, 0, 0) < 0)
+		die_errno("failed to read from stdin");
+	packet_write(1, sb.buf, sb.len);
+	strbuf_release(&sb);
+}
+
 static void unpack(void)
 {
 	struct packet_reader reader;
@@ -110,6 +120,8 @@ int cmd__pkt_line(int argc, const char **argv)
 
 	if (!strcmp(argv[1], "pack"))
 		pack(argc - 2, argv + 2);
+	else if (!strcmp(argv[1], "pack-raw-stdin"))
+		pack_raw_stdin();
 	else if (!strcmp(argv[1], "unpack"))
 		unpack();
 	else if (!strcmp(argv[1], "unpack-sideband"))
diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
index ddf3da5a26..666b3d8726 100644
--- a/t/t5411/once-0010-report-status-v1.sh
+++ b/t/t5411/once-0010-report-status-v1.sh
@@ -28,10 +28,10 @@ test_expect_success "proc-receive: report status v1" '
 		if test -z "$GIT_DEFAULT_HASH" || test "$GIT_DEFAULT_HASH" = "sha1"
 		then
 			printf "%s %s refs/heads/main\0report-status\n" \
-				$A $B | packetize
+				$A $B | test-tool pkt-line pack-raw-stdin
 		else
 			printf "%s %s refs/heads/main\0report-status object-format=$GIT_DEFAULT_HASH\n" \
-				$A $B | packetize
+				$A $B | test-tool pkt-line pack-raw-stdin
 		fi &&
 		test-tool pkt-line pack <<-EOF &&
 		$ZERO_OID $A refs/for/main/topic1
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index e6c8338b64..23a8a8d5c7 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -64,7 +64,8 @@ test_expect_success 'setup' '
 	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
 	{
 		printf "%s %s refs/heads/newbranch\\0report-status object-format=%s\\n" \
-			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize &&
+			"$ZERO_OID" "$hash_next" "$(test_oid algo)" |
+			test-tool pkt-line pack-raw-stdin &&
 		printf 0000 &&
 		echo "$hash_next" | git pack-objects --stdout
 	} >push_body &&
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 2dde034881..e2cb4f376d 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -194,9 +194,13 @@ test_expect_success 'hostname cannot break out of directory' '
 
 test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
 	{
-		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
-		printf "0000"
+		printf "git-upload-pack /interp.git\n\0host=localhost" |
+		test-tool pkt-line pack-raw-stdin &&
+		test-tool pkt-line pack <<-\EOF
+		0000
+		EOF
 	} >input &&
+
 	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
 	test-tool pkt-line unpack <output >actual &&
 
-- 
2.32.0.788.ge724008458

[PATCH v3 5/5] test-lib-functions.sh: remove unused [de]packetize() functions

[Ævar Arnfjörð Bjarmason]

Remove the now-unused "packetize()" and "depacketize()" functions
added in 4414a150025 (t/lib-git-daemon: add network-protocol helpers,
2018-01-24). As discussed in the preceding commits we've now moved all
their users over to "test-tool pkt-line".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 42 -----------------------------------------
 1 file changed, 42 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b2810478a2..77e4434dcd 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1453,48 +1453,6 @@ nongit () {
 	)
 } 7>&2 2>&4
 
-# convert function arguments or stdin (if not arguments given) to pktline
-# representation. If multiple arguments are given, they are separated by
-# whitespace and put in a single packet. Note that data containing NULs must be
-# given on stdin, and that empty input becomes an empty packet, not a flush
-# packet (for that you can just print 0000 yourself).
-packetize () {
-	if test $# -gt 0
-	then
-		packet="$*"
-		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
-	else
-		perl -e '
-			my $packet = do { local $/; <STDIN> };
-			printf "%04x%s", 4 + length($packet), $packet;
-		'
-	fi
-}
-
-# Parse the input as a series of pktlines, writing the result to stdout.
-# Sideband markers are removed automatically, and the output is routed to
-# stderr if appropriate.
-#
-# NUL bytes are converted to "\\0" for ease of parsing with text tools.
-depacketize () {
-	perl -e '
-		while (read(STDIN, $len, 4) == 4) {
-			if ($len eq "0000") {
-				print "FLUSH\n";
-			} else {
-				read(STDIN, $buf, hex($len) - 4);
-				$buf =~ s/\0/\\0/g;
-				if ($buf =~ s/^[\x2\x3]//) {
-					print STDERR $buf;
-				} else {
-					$buf =~ s/^\x1//;
-					print $buf;
-				}
-			}
-		}
-	'
-}
-
 # Converts base-16 data into base-8. The output is given as a sequence of
 # escaped octals, suitable for consumption by 'printf'.
 hex2oct () {
-- 
2.32.0.788.ge724008458

[PATCH v4] test-lib-functions: use test-tool for [de]packetize()

[Ævar Arnfjörð Bjarmason]

The shell+perl "[de]packetize()" helper functions were added in
4414a150025 (t/lib-git-daemon: add network-protocol helpers,
2018-01-24), and around the same time we added the "pkt-line" helper
in 74e70029615 (test-pkt-line: introduce a packet-line test helper,
2018-03-14).

For some reason it seems we've mostly used the shell+perl version
instead of the helper since then. There were discussions around
88124ab2636 (test-lib-functions: make packetize() more efficient,
2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()
stdin code, 2020-03-29) to improve them and make them more efficient.

There was one good reason to do so, we needed an equivalent of
"test-tool pkt-line pack", but that command wasn't capable of handling
input with "\n" (a feature) or "\0" (just because it happens to be
printf-based under the hood).

Let's add a "pkt-line-raw" helper for that, and expose is at a
packetize_raw() to go with the existing packetize() on the shell
level, this gives us the smallest amount of change to the tests
themselves.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I ejected changing/adding test code for this v4. This keeps the
compatibility wrappers and just narrowly changes the things that need
a packetize_raw() to use that new helper.

I left the simpler packetize() case as a "printf", it could also use
the test tool, but in case someone cares about process overhead on say
Windows this entire change should be strictly better than the status
quo.

Range-diff against v3:
1:  67aa8141153 < -:  ----------- serve tests: add missing "extra delim" test
2:  64dfd14865c < -:  ----------- serve tests: use test_cmp in "protocol violations" test
3:  92bfd8a87b8 < -:  ----------- tests: replace [de]packetize() shell+perl test-tool pkt-line
4:  9842c85d1f3 ! 1:  546974eed59 tests: replace remaining packetize() with "test-tool pkt-line"
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    tests: replace remaining packetize() with "test-tool pkt-line"
    +    test-lib-functions: use test-tool for [de]packetize()
     
    -    Move the only remaining users of "packetize()" over to "test-tool
    -    pkt-line", for this we need a new "pack-raw-stdin" subcommand in the
    -    test-tool. The "pack" command takes input on stdin, but splits it by
    -    "\n", furthermore we'll format the output using C-strings, so the
    -    embedded "\0" being tested for here would cause the string to be
    -    truncated.
    +    The shell+perl "[de]packetize()" helper functions were added in
    +    4414a150025 (t/lib-git-daemon: add network-protocol helpers,
    +    2018-01-24), and around the same time we added the "pkt-line" helper
    +    in 74e70029615 (test-pkt-line: introduce a packet-line test helper,
    +    2018-03-14).
     
    -    So we need another mode that just calls packet_write() on the raw
    -    input we got on stdin.
    +    For some reason it seems we've mostly used the shell+perl version
    +    instead of the helper since then. There were discussions around
    +    88124ab2636 (test-lib-functions: make packetize() more efficient,
    +    2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()
    +    stdin code, 2020-03-29) to improve them and make them more efficient.
    +
    +    There was one good reason to do so, we needed an equivalent of
    +    "test-tool pkt-line pack", but that command wasn't capable of handling
    +    input with "\n" (a feature) or "\0" (just because it happens to be
    +    printf-based under the hood).
    +
    +    Let's add a "pkt-line-raw" helper for that, and expose is at a
    +    packetize_raw() to go with the existing packetize() on the shell
    +    level, this gives us the smallest amount of change to the tests
    +    themselves.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/t5411/once-0010-report-status-v1.sh: test_expect_success "proc-receive: report
      		then
      			printf "%s %s refs/heads/main\0report-status\n" \
     -				$A $B | packetize
    -+				$A $B | test-tool pkt-line pack-raw-stdin
    ++				$A $B | packetize_raw
      		else
      			printf "%s %s refs/heads/main\0report-status object-format=$GIT_DEFAULT_HASH\n" \
     -				$A $B | packetize
    -+				$A $B | test-tool pkt-line pack-raw-stdin
    ++				$A $B | packetize_raw
      		fi &&
    - 		test-tool pkt-line pack <<-EOF &&
    - 		$ZERO_OID $A refs/for/main/topic1
    + 		printf "%s %s refs/for/main/topic1\n" \
    + 			$ZERO_OID $A | packetize &&
     
      ## t/t5562-http-backend-content-length.sh ##
     @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '
    @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '
      	{
      		printf "%s %s refs/heads/newbranch\\0report-status object-format=%s\\n" \
     -			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize &&
    -+			"$ZERO_OID" "$hash_next" "$(test_oid algo)" |
    -+			test-tool pkt-line pack-raw-stdin &&
    ++			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize_raw
      		printf 0000 &&
      		echo "$hash_next" | git pack-objects --stdout
      	} >push_body &&
    @@ t/t5570-git-daemon.sh: test_expect_success 'hostname cannot break out of directo
      test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
      	{
     -		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
    --		printf "0000"
    -+		printf "git-upload-pack /interp.git\n\0host=localhost" |
    -+		test-tool pkt-line pack-raw-stdin &&
    -+		test-tool pkt-line pack <<-\EOF
    -+		0000
    -+		EOF
    ++		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize_raw
    + 		printf "0000"
      	} >input &&
    -+
      	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
    - 	test-tool pkt-line unpack <output >actual &&
    +
    + ## t/test-lib-functions.sh ##
    +@@ t/test-lib-functions.sh: nongit () {
    + 	)
    + } 7>&2 2>&4
    + 
    +-# convert function arguments or stdin (if not arguments given) to pktline
    +-# representation. If multiple arguments are given, they are separated by
    +-# whitespace and put in a single packet. Note that data containing NULs must be
    +-# given on stdin, and that empty input becomes an empty packet, not a flush
    +-# packet (for that you can just print 0000 yourself).
    ++# These functions are historical wrappers around "test-tool pkt-line"
    ++# for older tests. Use "test-tool pkt-line" itself in new tests.
    + packetize () {
    + 	if test $# -gt 0
    + 	then
    + 		packet="$*"
    + 		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
    + 	else
    +-		perl -e '
    +-			my $packet = do { local $/; <STDIN> };
    +-			printf "%04x%s", 4 + length($packet), $packet;
    +-		'
    ++		test-tool pkt-line pack
    + 	fi
    + }
    + 
    +-# Parse the input as a series of pktlines, writing the result to stdout.
    +-# Sideband markers are removed automatically, and the output is routed to
    +-# stderr if appropriate.
    +-#
    +-# NUL bytes are converted to "\\0" for ease of parsing with text tools.
    ++packetize_raw () {
    ++	test-tool pkt-line pack-raw-stdin
    ++}
    ++
    + depacketize () {
    +-	perl -e '
    +-		while (read(STDIN, $len, 4) == 4) {
    +-			if ($len eq "0000") {
    +-				print "FLUSH\n";
    +-			} else {
    +-				read(STDIN, $buf, hex($len) - 4);
    +-				$buf =~ s/\0/\\0/g;
    +-				if ($buf =~ s/^[\x2\x3]//) {
    +-					print STDERR $buf;
    +-				} else {
    +-					$buf =~ s/^\x1//;
    +-					print $buf;
    +-				}
    +-			}
    +-		}
    +-	'
    ++	test-tool pkt-line unpack
    + }
      
    + # Converts base-16 data into base-8. The output is given as a sequence of
5:  7ca83c5a551 < -:  ----------- test-lib-functions.sh: remove unused [de]packetize() functions

 t/helper/test-pkt-line.c               | 12 ++++++++
 t/t5411/once-0010-report-status-v1.sh  |  4 +--
 t/t5562-http-backend-content-length.sh |  2 +-
 t/t5570-git-daemon.sh                  |  2 +-
 t/test-lib-functions.sh                | 38 ++++++--------------------
 5 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 5e638f0b970..c5e052e5378 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -26,6 +26,16 @@ static void pack(int argc, const char **argv)
 	}
 }
 
+static void pack_raw_stdin(void)
+{
+	struct strbuf sb = STRBUF_INIT;
+
+	if (strbuf_read(&sb, 0, 0) < 0)
+		die_errno("failed to read from stdin");
+	packet_write(1, sb.buf, sb.len);
+	strbuf_release(&sb);
+}
+
 static void unpack(void)
 {
 	struct packet_reader reader;
@@ -110,6 +120,8 @@ int cmd__pkt_line(int argc, const char **argv)
 
 	if (!strcmp(argv[1], "pack"))
 		pack(argc - 2, argv + 2);
+	else if (!strcmp(argv[1], "pack-raw-stdin"))
+		pack_raw_stdin();
 	else if (!strcmp(argv[1], "unpack"))
 		unpack();
 	else if (!strcmp(argv[1], "unpack-sideband"))
diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
index 1233a46eac5..297b10925d5 100644
--- a/t/t5411/once-0010-report-status-v1.sh
+++ b/t/t5411/once-0010-report-status-v1.sh
@@ -28,10 +28,10 @@ test_expect_success "proc-receive: report status v1" '
 		if test -z "$GIT_DEFAULT_HASH" || test "$GIT_DEFAULT_HASH" = "sha1"
 		then
 			printf "%s %s refs/heads/main\0report-status\n" \
-				$A $B | packetize
+				$A $B | packetize_raw
 		else
 			printf "%s %s refs/heads/main\0report-status object-format=$GIT_DEFAULT_HASH\n" \
-				$A $B | packetize
+				$A $B | packetize_raw
 		fi &&
 		printf "%s %s refs/for/main/topic1\n" \
 			$ZERO_OID $A | packetize &&
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index e5d3d15ba8d..05a58069b0c 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -63,7 +63,7 @@ test_expect_success 'setup' '
 	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
 	{
 		printf "%s %s refs/heads/newbranch\\0report-status object-format=%s\\n" \
-			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize &&
+			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize_raw
 		printf 0000 &&
 		echo "$hash_next" | git pack-objects --stdout
 	} >push_body &&
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 82c31ab6cd8..b87ca06a585 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -194,7 +194,7 @@ test_expect_success 'hostname cannot break out of directory' '
 
 test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
 	{
-		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
+		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize_raw
 		printf "0000"
 	} >input &&
 	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b2810478a21..e5b80e0f88e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1453,46 +1453,24 @@ nongit () {
 	)
 } 7>&2 2>&4
 
-# convert function arguments or stdin (if not arguments given) to pktline
-# representation. If multiple arguments are given, they are separated by
-# whitespace and put in a single packet. Note that data containing NULs must be
-# given on stdin, and that empty input becomes an empty packet, not a flush
-# packet (for that you can just print 0000 yourself).
+# These functions are historical wrappers around "test-tool pkt-line"
+# for older tests. Use "test-tool pkt-line" itself in new tests.
 packetize () {
 	if test $# -gt 0
 	then
 		packet="$*"
 		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
 	else
-		perl -e '
-			my $packet = do { local $/; <STDIN> };
-			printf "%04x%s", 4 + length($packet), $packet;
-		'
+		test-tool pkt-line pack
 	fi
 }
 
-# Parse the input as a series of pktlines, writing the result to stdout.
-# Sideband markers are removed automatically, and the output is routed to
-# stderr if appropriate.
-#
-# NUL bytes are converted to "\\0" for ease of parsing with text tools.
+packetize_raw () {
+	test-tool pkt-line pack-raw-stdin
+}
+
 depacketize () {
-	perl -e '
-		while (read(STDIN, $len, 4) == 4) {
-			if ($len eq "0000") {
-				print "FLUSH\n";
-			} else {
-				read(STDIN, $buf, hex($len) - 4);
-				$buf =~ s/\0/\\0/g;
-				if ($buf =~ s/^[\x2\x3]//) {
-					print STDERR $buf;
-				} else {
-					$buf =~ s/^\x1//;
-					print $buf;
-				}
-			}
-		}
-	'
+	test-tool pkt-line unpack
 }
 
 # Converts base-16 data into base-8. The output is given as a sequence of
-- 
2.32.0.874.gfa1990a4f10

Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize()

[Taylor Blau]

On Fri, Jul 16, 2021 at 05:41:33PM +0200, Ævar Arnfjörð Bjarmason wrote:
> I ejected changing/adding test code for this v4. This keeps the
> compatibility wrappers and just narrowly changes the things that need
> a packetize_raw() to use that new helper.
>
> I left the simpler packetize() case as a "printf", it could also use
> the test tool, but in case someone cares about process overhead on say
> Windows this entire change should be strictly better than the status
> quo.

Thanks, this version of the series looks much better to me.

> +static void pack_raw_stdin(void)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (strbuf_read(&sb, 0, 0) < 0)
> +		die_errno("failed to read from stdin");
> +	packet_write(1, sb.buf, sb.len);
> +	strbuf_release(&sb);
> +}
> +

Looks good to me.

> -# convert function arguments or stdin (if not arguments given) to pktline
> -# representation. If multiple arguments are given, they are separated by
> -# whitespace and put in a single packet. Note that data containing NULs must be
> -# given on stdin, and that empty input becomes an empty packet, not a flush
> -# packet (for that you can just print 0000 yourself).
> +# These functions are historical wrappers around "test-tool pkt-line"
> +# for older tests. Use "test-tool pkt-line" itself in new tests.

Nice. This keeps the diff relatively minimal. I don't really have a
strong opinion on saying "packetize" or "test-tool pkt-line pack" in the
future. Arguably the latter is more direct, but it's also more of a
mouthful.

I'm fine to take the approach you have here, and adjust the guidance in
the future depending on if people really do start preferring one over
the other in new tests.

>  packetize () {
>  	if test $# -gt 0
>  	then
>  		packet="$*"
>  		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
>  	else
> -		perl -e '
> -			my $packet = do { local $/; <STDIN> };
> -			printf "%04x%s", 4 + length($packet), $packet;
> -		'
> +		test-tool pkt-line pack
>  	fi
>  }

For what it's worth, I would be happy to remove the printf shortcut
entirely. Some quick grepping indicates only 22 uses of the word
"packetize" in our whole test suite (one of them being the function
declaration itself). And of the 21 callers, only 10 pass at least one
argument:

    git grep -Ew 'packetize [^()&]+' -- t

So I would be fine with adding 10 more new processes to the test suite
in the name of simplifying this declaration. I don't feel strongly,
though, since the conditional here does not really add that much
complexity.

Thanks,
Taylor

Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize()

[Jeff King]

On Fri, Jul 16, 2021 at 12:53:17PM -0400, Taylor Blau wrote:

> >  packetize () {
> >  	if test $# -gt 0
> >  	then
> >  		packet="$*"
> >  		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
> >  	else
> > -		perl -e '
> > -			my $packet = do { local $/; <STDIN> };
> > -			printf "%04x%s", 4 + length($packet), $packet;
> > -		'
> > +		test-tool pkt-line pack
> >  	fi
> >  }
> 
> For what it's worth, I would be happy to remove the printf shortcut
> entirely. Some quick grepping indicates only 22 uses of the word
> "packetize" in our whole test suite (one of them being the function
> declaration itself). And of the 21 callers, only 10 pass at least one
> argument:
> 
>     git grep -Ew 'packetize [^()&]+' -- t
> 
> So I would be fine with adding 10 more new processes to the test suite
> in the name of simplifying this declaration. I don't feel strongly,
> though, since the conditional here does not really add that much
> complexity.

I'd be fine with that, too. Part of the goal of the cmdline option was
making callers more readable. E.g.,

  -printf "want %s" "$hash_head" | packetize
  +packetize "want $hash_head"

But I think the here-doc input to "test-tool pkt-line" is generally
equally nice, especially when you have multiple lines.

(the commit introducing the cmdline version talks about efficiency, but
there the old version was using 4 processes!)

-Peff

Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize()

[Jeff King]

On Fri, Jul 16, 2021 at 05:41:33PM +0200, Ævar Arnfjörð Bjarmason wrote:

> I ejected changing/adding test code for this v4. This keeps the
> compatibility wrappers and just narrowly changes the things that need
> a packetize_raw() to use that new helper.
> 
> I left the simpler packetize() case as a "printf", it could also use
> the test tool, but in case someone cares about process overhead on say
> Windows this entire change should be strictly better than the status
> quo.

This looks fine to me. I actually did like some of the readability
improvements possible in the other patches, but they can still be easily
built on top if we care to.

Using "test-tool pkt-line unpack" doesn't behave exactly like
depacketize:

  - as noted earlier, it drops stuff after a NUL

  - it says "0000" instead of "FLUSH"

But we don't seem to depend on either of those in any tests, so it's a
good time to switch. :)

-Peff

Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The shell+perl "[de]packetize()" helper functions were added in
> 4414a150025 (t/lib-git-daemon: add network-protocol helpers,
> 2018-01-24), and around the same time we added the "pkt-line" helper
> in 74e70029615 (test-pkt-line: introduce a packet-line test helper,
> 2018-03-14).
>
> For some reason it seems we've mostly used the shell+perl version
> instead of the helper since then. There were discussions around
> 88124ab2636 (test-lib-functions: make packetize() more efficient,
> 2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()
> stdin code, 2020-03-29) to improve them and make them more efficient.
>
> There was one good reason to do so, we needed an equivalent of
> "test-tool pkt-line pack", but that command wasn't capable of handling
> input with "\n" (a feature) or "\0" (just because it happens to be
> printf-based under the hood).
>
> Let's add a "pkt-line-raw" helper for that, and expose is at a
> packetize_raw() to go with the existing packetize() on the shell
> level, this gives us the smallest amount of change to the tests
> themselves.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> I ejected changing/adding test code for this v4. This keeps the
> compatibility wrappers and just narrowly changes the things that need
> a packetize_raw() to use that new helper.
>
> I left the simpler packetize() case as a "printf", it could also use
> the test tool, but in case someone cares about process overhead on say
> Windows this entire change should be strictly better than the status
> quo.

Thanks, all.  Will replace.  Let me mark this for 'next'.



> Range-diff against v3:
> 1:  67aa8141153 < -:  ----------- serve tests: add missing "extra delim" test
> 2:  64dfd14865c < -:  ----------- serve tests: use test_cmp in "protocol violations" test
> 3:  92bfd8a87b8 < -:  ----------- tests: replace [de]packetize() shell+perl test-tool pkt-line
> 4:  9842c85d1f3 ! 1:  546974eed59 tests: replace remaining packetize() with "test-tool pkt-line"
>     @@ Metadata
>      Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      
>       ## Commit message ##
>     -    tests: replace remaining packetize() with "test-tool pkt-line"
>     +    test-lib-functions: use test-tool for [de]packetize()
>      
>     -    Move the only remaining users of "packetize()" over to "test-tool
>     -    pkt-line", for this we need a new "pack-raw-stdin" subcommand in the
>     -    test-tool. The "pack" command takes input on stdin, but splits it by
>     -    "\n", furthermore we'll format the output using C-strings, so the
>     -    embedded "\0" being tested for here would cause the string to be
>     -    truncated.
>     +    The shell+perl "[de]packetize()" helper functions were added in
>     +    4414a150025 (t/lib-git-daemon: add network-protocol helpers,
>     +    2018-01-24), and around the same time we added the "pkt-line" helper
>     +    in 74e70029615 (test-pkt-line: introduce a packet-line test helper,
>     +    2018-03-14).
>      
>     -    So we need another mode that just calls packet_write() on the raw
>     -    input we got on stdin.
>     +    For some reason it seems we've mostly used the shell+perl version
>     +    instead of the helper since then. There were discussions around
>     +    88124ab2636 (test-lib-functions: make packetize() more efficient,
>     +    2020-03-27) and cacae4329fa (test-lib-functions: simplify packetize()
>     +    stdin code, 2020-03-29) to improve them and make them more efficient.
>     +
>     +    There was one good reason to do so, we needed an equivalent of
>     +    "test-tool pkt-line pack", but that command wasn't capable of handling
>     +    input with "\n" (a feature) or "\0" (just because it happens to be
>     +    printf-based under the hood).
>     +
>     +    Let's add a "pkt-line-raw" helper for that, and expose is at a
>     +    packetize_raw() to go with the existing packetize() on the shell
>     +    level, this gives us the smallest amount of change to the tests
>     +    themselves.
>      
>          Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      
>     @@ t/t5411/once-0010-report-status-v1.sh: test_expect_success "proc-receive: report
>       		then
>       			printf "%s %s refs/heads/main\0report-status\n" \
>      -				$A $B | packetize
>     -+				$A $B | test-tool pkt-line pack-raw-stdin
>     ++				$A $B | packetize_raw
>       		else
>       			printf "%s %s refs/heads/main\0report-status object-format=$GIT_DEFAULT_HASH\n" \
>      -				$A $B | packetize
>     -+				$A $B | test-tool pkt-line pack-raw-stdin
>     ++				$A $B | packetize_raw
>       		fi &&
>     - 		test-tool pkt-line pack <<-EOF &&
>     - 		$ZERO_OID $A refs/for/main/topic1
>     + 		printf "%s %s refs/for/main/topic1\n" \
>     + 			$ZERO_OID $A | packetize &&
>      
>       ## t/t5562-http-backend-content-length.sh ##
>      @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '
>     @@ t/t5562-http-backend-content-length.sh: test_expect_success 'setup' '
>       	{
>       		printf "%s %s refs/heads/newbranch\\0report-status object-format=%s\\n" \
>      -			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize &&
>     -+			"$ZERO_OID" "$hash_next" "$(test_oid algo)" |
>     -+			test-tool pkt-line pack-raw-stdin &&
>     ++			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize_raw
>       		printf 0000 &&
>       		echo "$hash_next" | git pack-objects --stdout
>       	} >push_body &&
>     @@ t/t5570-git-daemon.sh: test_expect_success 'hostname cannot break out of directo
>       test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
>       	{
>      -		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
>     --		printf "0000"
>     -+		printf "git-upload-pack /interp.git\n\0host=localhost" |
>     -+		test-tool pkt-line pack-raw-stdin &&
>     -+		test-tool pkt-line pack <<-\EOF
>     -+		0000
>     -+		EOF
>     ++		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize_raw
>     + 		printf "0000"
>       	} >input &&
>     -+
>       	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
>     - 	test-tool pkt-line unpack <output >actual &&
>     +
>     + ## t/test-lib-functions.sh ##
>     +@@ t/test-lib-functions.sh: nongit () {
>     + 	)
>     + } 7>&2 2>&4
>     + 
>     +-# convert function arguments or stdin (if not arguments given) to pktline
>     +-# representation. If multiple arguments are given, they are separated by
>     +-# whitespace and put in a single packet. Note that data containing NULs must be
>     +-# given on stdin, and that empty input becomes an empty packet, not a flush
>     +-# packet (for that you can just print 0000 yourself).
>     ++# These functions are historical wrappers around "test-tool pkt-line"
>     ++# for older tests. Use "test-tool pkt-line" itself in new tests.
>     + packetize () {
>     + 	if test $# -gt 0
>     + 	then
>     + 		packet="$*"
>     + 		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
>     + 	else
>     +-		perl -e '
>     +-			my $packet = do { local $/; <STDIN> };
>     +-			printf "%04x%s", 4 + length($packet), $packet;
>     +-		'
>     ++		test-tool pkt-line pack
>     + 	fi
>     + }
>     + 
>     +-# Parse the input as a series of pktlines, writing the result to stdout.
>     +-# Sideband markers are removed automatically, and the output is routed to
>     +-# stderr if appropriate.
>     +-#
>     +-# NUL bytes are converted to "\\0" for ease of parsing with text tools.
>     ++packetize_raw () {
>     ++	test-tool pkt-line pack-raw-stdin
>     ++}
>     ++
>     + depacketize () {
>     +-	perl -e '
>     +-		while (read(STDIN, $len, 4) == 4) {
>     +-			if ($len eq "0000") {
>     +-				print "FLUSH\n";
>     +-			} else {
>     +-				read(STDIN, $buf, hex($len) - 4);
>     +-				$buf =~ s/\0/\\0/g;
>     +-				if ($buf =~ s/^[\x2\x3]//) {
>     +-					print STDERR $buf;
>     +-				} else {
>     +-					$buf =~ s/^\x1//;
>     +-					print $buf;
>     +-				}
>     +-			}
>     +-		}
>     +-	'
>     ++	test-tool pkt-line unpack
>     + }
>       
>     + # Converts base-16 data into base-8. The output is given as a sequence of
> 5:  7ca83c5a551 < -:  ----------- test-lib-functions.sh: remove unused [de]packetize() functions
>
>  t/helper/test-pkt-line.c               | 12 ++++++++
>  t/t5411/once-0010-report-status-v1.sh  |  4 +--
>  t/t5562-http-backend-content-length.sh |  2 +-
>  t/t5570-git-daemon.sh                  |  2 +-
>  t/test-lib-functions.sh                | 38 ++++++--------------------
>  5 files changed, 24 insertions(+), 34 deletions(-)
>
> diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
> index 5e638f0b970..c5e052e5378 100644
> --- a/t/helper/test-pkt-line.c
> +++ b/t/helper/test-pkt-line.c
> @@ -26,6 +26,16 @@ static void pack(int argc, const char **argv)
>  	}
>  }
>  
> +static void pack_raw_stdin(void)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (strbuf_read(&sb, 0, 0) < 0)
> +		die_errno("failed to read from stdin");
> +	packet_write(1, sb.buf, sb.len);
> +	strbuf_release(&sb);
> +}
> +
>  static void unpack(void)
>  {
>  	struct packet_reader reader;
> @@ -110,6 +120,8 @@ int cmd__pkt_line(int argc, const char **argv)
>  
>  	if (!strcmp(argv[1], "pack"))
>  		pack(argc - 2, argv + 2);
> +	else if (!strcmp(argv[1], "pack-raw-stdin"))
> +		pack_raw_stdin();
>  	else if (!strcmp(argv[1], "unpack"))
>  		unpack();
>  	else if (!strcmp(argv[1], "unpack-sideband"))
> diff --git a/t/t5411/once-0010-report-status-v1.sh b/t/t5411/once-0010-report-status-v1.sh
> index 1233a46eac5..297b10925d5 100644
> --- a/t/t5411/once-0010-report-status-v1.sh
> +++ b/t/t5411/once-0010-report-status-v1.sh
> @@ -28,10 +28,10 @@ test_expect_success "proc-receive: report status v1" '
>  		if test -z "$GIT_DEFAULT_HASH" || test "$GIT_DEFAULT_HASH" = "sha1"
>  		then
>  			printf "%s %s refs/heads/main\0report-status\n" \
> -				$A $B | packetize
> +				$A $B | packetize_raw
>  		else
>  			printf "%s %s refs/heads/main\0report-status object-format=$GIT_DEFAULT_HASH\n" \
> -				$A $B | packetize
> +				$A $B | packetize_raw
>  		fi &&
>  		printf "%s %s refs/for/main/topic1\n" \
>  			$ZERO_OID $A | packetize &&
> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
> index e5d3d15ba8d..05a58069b0c 100755
> --- a/t/t5562-http-backend-content-length.sh
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -63,7 +63,7 @@ test_expect_success 'setup' '
>  	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
>  	{
>  		printf "%s %s refs/heads/newbranch\\0report-status object-format=%s\\n" \
> -			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize &&
> +			"$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize_raw
>  		printf 0000 &&
>  		echo "$hash_next" | git pack-objects --stdout
>  	} >push_body &&
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index 82c31ab6cd8..b87ca06a585 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -194,7 +194,7 @@ test_expect_success 'hostname cannot break out of directory' '
>  
>  test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
>  	{
> -		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
> +		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize_raw
>  		printf "0000"
>  	} >input &&
>  	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index b2810478a21..e5b80e0f88e 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1453,46 +1453,24 @@ nongit () {
>  	)
>  } 7>&2 2>&4
>  
> -# convert function arguments or stdin (if not arguments given) to pktline
> -# representation. If multiple arguments are given, they are separated by
> -# whitespace and put in a single packet. Note that data containing NULs must be
> -# given on stdin, and that empty input becomes an empty packet, not a flush
> -# packet (for that you can just print 0000 yourself).
> +# These functions are historical wrappers around "test-tool pkt-line"
> +# for older tests. Use "test-tool pkt-line" itself in new tests.
>  packetize () {
>  	if test $# -gt 0
>  	then
>  		packet="$*"
>  		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
>  	else
> -		perl -e '
> -			my $packet = do { local $/; <STDIN> };
> -			printf "%04x%s", 4 + length($packet), $packet;
> -		'
> +		test-tool pkt-line pack
>  	fi
>  }
>  
> -# Parse the input as a series of pktlines, writing the result to stdout.
> -# Sideband markers are removed automatically, and the output is routed to
> -# stderr if appropriate.
> -#
> -# NUL bytes are converted to "\\0" for ease of parsing with text tools.
> +packetize_raw () {
> +	test-tool pkt-line pack-raw-stdin
> +}
> +
>  depacketize () {
> -	perl -e '
> -		while (read(STDIN, $len, 4) == 4) {
> -			if ($len eq "0000") {
> -				print "FLUSH\n";
> -			} else {
> -				read(STDIN, $buf, hex($len) - 4);
> -				$buf =~ s/\0/\\0/g;
> -				if ($buf =~ s/^[\x2\x3]//) {
> -					print STDERR $buf;
> -				} else {
> -					$buf =~ s/^\x1//;
> -					print $buf;
> -				}
> -			}
> -		}
> -	'
> +	test-tool pkt-line unpack
>  }
>  
>  # Converts base-16 data into base-8. The output is given as a sequence of