* [PATCH 0/5] tests: migrate to "test-tool pkt-line" @ 2021-07-07 10:21 Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason ` (5 more replies) 0 siblings, 6 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/5] serve tests: add missing "extra delim" test 2021-07-07 10:21 [PATCH 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 ` Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 2/5] serve tests: use test_cmp in "protocol violations" test Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 5 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/5] serve tests: use test_cmp in "protocol violations" test 2021-07-07 10:21 [PATCH 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 ` Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 5 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line 2021-07-07 10:21 [PATCH 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 2/5] serve tests: use test_cmp in "protocol violations" test Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 ` Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 5 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 4/5] tests: replace remaining packetize() with "test-tool pkt-line" 2021-07-07 10:21 [PATCH 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-07-07 10:21 ` [PATCH 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 ` Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 5/5] test-lib-functions.sh: remove unused [de]packetize() functions Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason 5 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 5/5] test-lib-functions.sh: remove unused [de]packetize() functions 2021-07-07 10:21 [PATCH 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2021-07-07 10:21 ` [PATCH 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 ` Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason 5 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-07 10:21 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" 2021-07-07 10:21 [PATCH 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2021-07-07 10:21 ` [PATCH 5/5] test-lib-functions.sh: remove unused [de]packetize() functions Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 ` Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason ` (6 more replies) 5 siblings, 7 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 1/5] serve tests: add missing "extra delim" test 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 ` Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 2/5] serve tests: use test_cmp in "protocol violations" test Ævar Arnfjörð Bjarmason ` (5 subsequent siblings) 6 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 2/5] serve tests: use test_cmp in "protocol violations" test 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 ` Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 6 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 2/5] serve tests: use test_cmp in "protocol violations" test Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 ` Ævar Arnfjörð Bjarmason 2021-07-13 20:50 ` Jeff King 2021-07-12 16:44 ` [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 6 siblings, 1 reply; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line 2021-07-12 16:44 ` [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason @ 2021-07-13 20:50 ` Jeff King 2021-07-13 23:41 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 33+ messages in thread From: Jeff King @ 2021-07-13 20:50 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line 2021-07-13 20:50 ` Jeff King @ 2021-07-13 23:41 ` Ævar Arnfjörð Bjarmason 2021-07-14 1:12 ` Jeff King 0 siblings, 1 reply; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-13 23:41 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line 2021-07-13 23:41 ` Ævar Arnfjörð Bjarmason @ 2021-07-14 1:12 ` Jeff King 0 siblings, 0 replies; 33+ messages in thread From: Jeff King @ 2021-07-14 1:12 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line" 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-07-12 16:44 ` [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 ` Ævar Arnfjörð Bjarmason 2021-07-13 20:58 ` Jeff King 2021-07-12 16:44 ` [PATCH v2 5/5] test-lib-functions.sh: remove unused [de]packetize() functions Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 6 siblings, 1 reply; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line" 2021-07-12 16:44 ` [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason @ 2021-07-13 20:58 ` Jeff King 2021-07-13 23:52 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 33+ messages in thread From: Jeff King @ 2021-07-13 20:58 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line" 2021-07-13 20:58 ` Jeff King @ 2021-07-13 23:52 ` Ævar Arnfjörð Bjarmason 2021-07-14 1:16 ` Jeff King 0 siblings, 1 reply; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-13 23:52 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line" 2021-07-13 23:52 ` Ævar Arnfjörð Bjarmason @ 2021-07-14 1:16 ` Jeff King 0 siblings, 0 replies; 33+ messages in thread From: Jeff King @ 2021-07-14 1:16 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 5/5] test-lib-functions.sh: remove unused [de]packetize() functions 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2021-07-12 16:44 ` [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 ` Ævar Arnfjörð Bjarmason 2021-07-12 20:41 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Junio C Hamano 2021-07-14 0:54 ` [PATCH v3 " Ævar Arnfjörð Bjarmason 6 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-12 16:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2021-07-12 16:44 ` [PATCH v2 5/5] test-lib-functions.sh: remove unused [de]packetize() functions Ævar Arnfjörð Bjarmason @ 2021-07-12 20:41 ` Junio C Hamano 2021-07-13 21:00 ` Jeff King 2021-07-14 0:54 ` [PATCH v3 " Ævar Arnfjörð Bjarmason 6 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2021-07-12 20:41 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King Æ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. ;-) ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" 2021-07-12 20:41 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Junio C Hamano @ 2021-07-13 21:00 ` Jeff King 0 siblings, 0 replies; 33+ messages in thread From: Jeff King @ 2021-07-13 21:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 0/5] tests: migrate to "test-tool pkt-line" 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (5 preceding siblings ...) 2021-07-12 20:41 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Junio C Hamano @ 2021-07-14 0:54 ` Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason ` (5 more replies) 6 siblings, 6 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 1/5] serve tests: add missing "extra delim" test 2021-07-14 0:54 ` [PATCH v3 " Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 ` Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 2/5] serve tests: use test_cmp in "protocol violations" test Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 5 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 2/5] serve tests: use test_cmp in "protocol violations" test 2021-07-14 0:54 ` [PATCH v3 " Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 ` Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 5 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line 2021-07-14 0:54 ` [PATCH v3 " Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 2/5] serve tests: use test_cmp in "protocol violations" test Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 ` Ævar Arnfjörð Bjarmason 2021-07-14 4:04 ` Taylor Blau 2021-07-14 0:54 ` [PATCH v3 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 5 siblings, 1 reply; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line 2021-07-14 0:54 ` [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason @ 2021-07-14 4:04 ` Taylor Blau 2021-07-14 16:22 ` Junio C Hamano 0 siblings, 1 reply; 33+ messages in thread From: Taylor Blau @ 2021-07-14 4:04 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, Jeff King 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line 2021-07-14 4:04 ` Taylor Blau @ 2021-07-14 16:22 ` Junio C Hamano 0 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2021-07-14 16:22 UTC (permalink / raw) To: Taylor Blau; +Cc: Ævar Arnfjörð Bjarmason, git, Jeff King 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... ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 4/5] tests: replace remaining packetize() with "test-tool pkt-line" 2021-07-14 0:54 ` [PATCH v3 " Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-07-14 0:54 ` [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 ` Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 5/5] test-lib-functions.sh: remove unused [de]packetize() functions Ævar Arnfjörð Bjarmason 2021-07-16 15:41 ` [PATCH v4] test-lib-functions: use test-tool for [de]packetize() Ævar Arnfjörð Bjarmason 5 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v3 5/5] test-lib-functions.sh: remove unused [de]packetize() functions 2021-07-14 0:54 ` [PATCH v3 " Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2021-07-14 0:54 ` [PATCH v3 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 ` Ævar Arnfjörð Bjarmason 2021-07-16 15:41 ` [PATCH v4] test-lib-functions: use test-tool for [de]packetize() Ævar Arnfjörð Bjarmason 5 siblings, 0 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-14 0:54 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4] test-lib-functions: use test-tool for [de]packetize() 2021-07-14 0:54 ` [PATCH v3 " Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2021-07-14 0:54 ` [PATCH v3 5/5] test-lib-functions.sh: remove unused [de]packetize() functions Ævar Arnfjörð Bjarmason @ 2021-07-16 15:41 ` Ævar Arnfjörð Bjarmason 2021-07-16 16:53 ` Taylor Blau ` (2 more replies) 5 siblings, 3 replies; 33+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-16 15:41 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Æ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 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize() 2021-07-16 15:41 ` [PATCH v4] test-lib-functions: use test-tool for [de]packetize() Ævar Arnfjörð Bjarmason @ 2021-07-16 16:53 ` Taylor Blau 2021-07-16 19:08 ` Jeff King 2021-07-16 19:03 ` Jeff King 2021-07-19 18:54 ` Junio C Hamano 2 siblings, 1 reply; 33+ messages in thread From: Taylor Blau @ 2021-07-16 16:53 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, 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. 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize() 2021-07-16 16:53 ` Taylor Blau @ 2021-07-16 19:08 ` Jeff King 0 siblings, 0 replies; 33+ messages in thread From: Jeff King @ 2021-07-16 19:08 UTC (permalink / raw) To: Taylor Blau; +Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize() 2021-07-16 15:41 ` [PATCH v4] test-lib-functions: use test-tool for [de]packetize() Ævar Arnfjörð Bjarmason 2021-07-16 16:53 ` Taylor Blau @ 2021-07-16 19:03 ` Jeff King 2021-07-19 18:54 ` Junio C Hamano 2 siblings, 0 replies; 33+ messages in thread From: Jeff King @ 2021-07-16 19:03 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, 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. 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize() 2021-07-16 15:41 ` [PATCH v4] test-lib-functions: use test-tool for [de]packetize() Ævar Arnfjörð Bjarmason 2021-07-16 16:53 ` Taylor Blau 2021-07-16 19:03 ` Jeff King @ 2021-07-19 18:54 ` Junio C Hamano 2 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2021-07-19 18:54 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Taylor Blau Æ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 ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2021-07-19 19:15 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-07 10:21 [PATCH 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 2/5] serve tests: use test_cmp in "protocol violations" test Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason 2021-07-07 10:21 ` [PATCH 5/5] test-lib-functions.sh: remove unused [de]packetize() functions Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 2/5] serve tests: use test_cmp in "protocol violations" test Ævar Arnfjörð Bjarmason 2021-07-12 16:44 ` [PATCH v2 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason 2021-07-13 20:50 ` Jeff King 2021-07-13 23:41 ` Ævar Arnfjörð Bjarmason 2021-07-14 1:12 ` Jeff King 2021-07-12 16:44 ` [PATCH v2 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason 2021-07-13 20:58 ` Jeff King 2021-07-13 23:52 ` Ævar Arnfjörð Bjarmason 2021-07-14 1:16 ` Jeff King 2021-07-12 16:44 ` [PATCH v2 5/5] test-lib-functions.sh: remove unused [de]packetize() functions Ævar Arnfjörð Bjarmason 2021-07-12 20:41 ` [PATCH v2 0/5] tests: migrate to "test-tool pkt-line" Junio C Hamano 2021-07-13 21:00 ` Jeff King 2021-07-14 0:54 ` [PATCH v3 " Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 1/5] serve tests: add missing "extra delim" test Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 2/5] serve tests: use test_cmp in "protocol violations" test Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line Ævar Arnfjörð Bjarmason 2021-07-14 4:04 ` Taylor Blau 2021-07-14 16:22 ` Junio C Hamano 2021-07-14 0:54 ` [PATCH v3 4/5] tests: replace remaining packetize() with "test-tool pkt-line" Ævar Arnfjörð Bjarmason 2021-07-14 0:54 ` [PATCH v3 5/5] test-lib-functions.sh: remove unused [de]packetize() functions Ævar Arnfjörð Bjarmason 2021-07-16 15:41 ` [PATCH v4] test-lib-functions: use test-tool for [de]packetize() Ævar Arnfjörð Bjarmason 2021-07-16 16:53 ` Taylor Blau 2021-07-16 19:08 ` Jeff King 2021-07-16 19:03 ` Jeff King 2021-07-19 18:54 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.