| Message ID | patch-1.1-546974eed59-20210716T153909Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8e5e53450c985580e05c32885750dcc50d3ddd99 |
| Headers | show |
| Series | [v4] test-lib-functions: use test-tool for [de]packetize() | expand |
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
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
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
Æ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
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