git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Jul 13, 2021, 20:50 UTC
Message-ID
<YO38ladlVVGAmQnl@coredump.intra.peff.net>
In-Reply-To
<patch-3.5-c33f344ab20-20210712T164208Z-avarab@gmail.com>
On Mon, Jul 12, 2021 at 06:44:18PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 11 quoted lines
> 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:

Show 26 quoted lines
> -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").

Show 19 quoted lines
> 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?
Show 15 quoted lines
> -	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.
Show 21 quoted lines
> 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.

Show 7 quoted lines
> 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.

Show 16 quoted lines
>  	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
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 11 of 33 in “tests: migrate to "test-tool pkt-line"”
  1. 0/5 tests: migrate to "test-tool pkt-line"Ævar Arnfjörð Bjarmason, Jul 7, 2021
  2. 1/5 serve tests: add missing "extra delim" testÆvar Arnfjörð Bjarmason, Jul 7, 2021
  3. 2/5 serve tests: use test_cmp in "protocol violations" testÆvar Arnfjörð Bjarmason, Jul 7, 2021
  4. 3/5 tests: replace [de]packetize() shell+perl test-tool pkt-lineÆvar Arnfjörð Bjarmason, Jul 7, 2021
  5. 4/5 tests: replace remaining packetize() with "test-tool pkt-line"Ævar Arnfjörð Bjarmason, Jul 7, 2021
  6. 5/5 test-lib-functions.sh: remove unused [de]packetize() functionsÆvar Arnfjörð Bjarmason, Jul 7, 2021
  7. 0/5 tests: migrate to "test-tool pkt-line"Ævar Arnfjörð Bjarmason, Jul 12, 2021
  8. 1/5 serve tests: add missing "extra delim" testÆvar Arnfjörð Bjarmason, Jul 12, 2021
  9. 2/5 serve tests: use test_cmp in "protocol violations" testÆvar Arnfjörð Bjarmason, Jul 12, 2021
  10. 3/5 tests: replace [de]packetize() shell+perl test-tool pkt-lineÆvar Arnfjörð Bjarmason, Jul 12, 2021
  11. Jeff KingJul 13, 2021
  12. Ævar Arnfjörð BjarmasonJul 13, 2021
  13. Jeff KingJul 14, 2021
  14. 4/5 tests: replace remaining packetize() with "test-tool pkt-line"Ævar Arnfjörð Bjarmason, Jul 12, 2021
  15. Jeff KingJul 13, 2021
  16. Ævar Arnfjörð BjarmasonJul 13, 2021
  17. Jeff KingJul 14, 2021
  18. 5/5 test-lib-functions.sh: remove unused [de]packetize() functionsÆvar Arnfjörð Bjarmason, Jul 12, 2021
  19. Junio C HamanoJul 12, 2021
  20. Jeff KingJul 13, 2021
  21. 0/5 tests: migrate to "test-tool pkt-line"Ævar Arnfjörð Bjarmason, Jul 14, 2021
  22. 1/5 serve tests: add missing "extra delim" testÆvar Arnfjörð Bjarmason, Jul 14, 2021
  23. 3/5 tests: replace [de]packetize() shell+perl test-tool pkt-lineÆvar Arnfjörð Bjarmason, Jul 14, 2021
  24. Taylor BlauJul 14, 2021
  25. Junio C HamanoJul 14, 2021
  26. 2/5 serve tests: use test_cmp in "protocol violations" testÆvar Arnfjörð Bjarmason, Jul 14, 2021
  27. 4/5 tests: replace remaining packetize() with "test-tool pkt-line"Ævar Arnfjörð Bjarmason, Jul 14, 2021
  28. 5/5 test-lib-functions.sh: remove unused [de]packetize() functionsÆvar Arnfjörð Bjarmason, Jul 14, 2021
  29. test-lib-functions: use test-tool for [de]packetize()Ævar Arnfjörð Bjarmason, Jul 16, 2021
  30. Taylor BlauJul 16, 2021
  31. Jeff KingJul 16, 2021
  32. Jeff KingJul 16, 2021
  33. Junio C HamanoJul 19, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.