Re: [PATCH v4] test-lib-functions: use test-tool for [de]packetize()

[Taylor Blau <me@ttaylorr.com>]

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