Re: [PATCH 1/2] test-lib-functions: make packetize() more efficient

[Taylor Blau <me@ttaylorr.com>]

On Fri, Mar 27, 2020 at 04:03:00AM -0400, Jeff King wrote:
> The packetize() function takes its input on stdin, and requires 4
> separate sub-processes to format a simple string. We can do much better
> by getting the length via the shell's "${#packet}" construct. The one
> caveat is that the shell can't put a NUL into a variable, so we'll have
> to continue to provide the stdin form for a few calls.
>
> There are a few other cleanups here in the touched code:
>
>  - the stdin form of packetize() had an extra stray "%s" when printing
>    the packet
>
>  - the converted calls in t5562 can be made simpler by redirecting
>    output as a block, rather than repeated appending
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t5562-http-backend-content-length.sh | 19 ++++++++++++-------
>  t/test-lib-functions.sh                | 23 ++++++++++++++++-------
>  2 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
> index 4a110b307e..3f4ac71f83 100755
> --- a/t/t5562-http-backend-content-length.sh
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -53,15 +53,20 @@ test_expect_success 'setup' '
>  	test_commit c1 &&
>  	hash_head=$(git rev-parse HEAD) &&
>  	hash_prev=$(git rev-parse HEAD~1) &&
> -	printf "want %s" "$hash_head" | packetize >fetch_body &&
> -	printf 0000 >>fetch_body &&
> -	printf "have %s" "$hash_prev" | packetize >>fetch_body &&
> -	printf done | packetize >>fetch_body &&
> +	{
> +		packetize "want $hash_head" &&
> +		printf 0000 &&
> +		packetize "have $hash_prev" &&
> +		packetize "done"
> +	} >fetch_body &&

Nicely done, this is an obvious improvement in clarity over the
appending '>>' that was here before. The new version reads mouch more
cleanly.

>  	test_copy_bytes 10 <fetch_body >fetch_body.trunc &&
>  	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
> -	printf "%s %s refs/heads/newbranch\\0report-status\\n" "$ZERO_OID" "$hash_next" | packetize >push_body &&
> -	printf 0000 >>push_body &&
> -	echo "$hash_next" | git pack-objects --stdout >>push_body &&
> +	{
> +		printf "%s %s refs/heads/newbranch\\0report-status\\n" \
> +			"$ZERO_OID" "$hash_next" | packetize &&
> +		printf 0000 &&
> +		echo "$hash_next" | git pack-objects --stdout
> +	} >push_body &&
>  	test_copy_bytes 10 <push_body >push_body.trunc &&
>  	: >empty_body
>  '
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 352c213d52..216918a58c 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1362,14 +1362,23 @@ nongit () {
>  	)
>  } 7>&2 2>&4
>
> -# convert stdin to pktline representation; note that empty input becomes an
> -# empty packet, not a flush packet (for that you can just print 0000 yourself).
> +# 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() {
> -	cat >packetize.tmp &&
> -	len=$(wc -c <packetize.tmp) &&
> -	printf '%04x%s' "$(($len + 4))" &&
> -	cat packetize.tmp &&
> -	rm -f packetize.tmp
> +	if test $# -gt 0
> +	then
> +		packet="$*"

Mentioned off-list in a discussion already, but: though I find this
behavior of joining multiple arguments by a whitespace character a
little confusing (i.e., what would callers thing this function does if
they hadn't read the documentation?) I think that this is probably the
most sane thing that you could do here.

On the other hand, nowhere in this patch do we currently call packetize
with multiple arguments, so perhaps it would be made simpler if we
instead wrote "$1" anywhere there was "$packet".

> +		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
> +	else
> +		cat >packetize.tmp &&
> +		len=$(wc -c <packetize.tmp) &&
> +		printf '%04x' "$(($len + 4))" &&
> +		cat packetize.tmp &&
> +		rm -f packetize.tmp
> +	fi
>  }

Right: if the contents contains an unrepresentable byte, then it has to
be passed over stdin. This part is obviously correct.

>  # Parse the input as a series of pktlines, writing the result to stdout.
> --
> 2.26.0.581.g322f77c0ee

Thanks,
Taylor