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

[Junio C Hamano <gitster@pobox.com>]

Æ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