Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxx>
>
> ## 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@xxxxxxxxx>
>
> @@ 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
