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

From: Jeff King <peff@peff.net>
Date: Jul 16, 2021, 19:08 UTC
Message-ID: <YPHZNJgAkd3m6bhm@coredump.intra.peff.net>
In-Reply-To: <YPG5few/PeStqe/2@nand.local>

On Fri, Jul 16, 2021 at 12:53:17PM -0400, Taylor Blau wrote:

Show 26 quoted lines

> >  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