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

From: Jeff King <peff@peff.net>
Date: Mar 28, 2020, 11:20 UTC
Message-ID: <20200328112010.GC639140@coredump.intra.peff.net>
In-Reply-To: <xmqq1rpdhaid.fsf@gitster.c.googlers.com>

On Fri, Mar 27, 2020 at 12:18:34PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Yuck.  Binary protocol and shell variables do not mix well.
> 
> Documentation/CodingGuidelines forbids ${#parameter} in the first
> place and we seem to use it only when we know we are using bash.
> 
> Perhaps we should start considering to lift it.  I dunno.

Hmph. I had a vague recollection there but checked beforehand:

 - we do use it in t9010, which is /bin/sh (and have since 2010). I
   thought it might not be run on obscure platforms because it's
   svn-related, but I think it doesn't actually require svn.

 - it's in POSIX, at least as far back as 2004 (I couldn't find an easy
   copy of the 2001 version). That doesn't prove there aren't
   problematic systems, of course, but it at least passes the bar of
   "not even in POSIX".

 - it's not in check-non-portable-shell.pl. :) That doesn't mean
   CodingGuidelines is wrong, but we should probably reconcile them.

Given that the first point means we've had a 10-year weather balloon, and that the original rule seems to have come from a big list of rules[1] rather than a specific known problem shell, I think we should declare it available.

[1] https://lore.kernel.org/git/7vtznzf5jb.fsf@gitster.siamese.dyndns.org/

Show 15 quoted lines

> >  packetize() {
> > +	if test $# -gt 0
> > +	then
> > +		packet="$*"
> > +		printf '%04x%s' "$((4 + ${#packet}))" "$packet"
> 
> This allows 
> 
> 		packetize "want $hash_head"
> 
> to be written like so:
> 
> 		packetize want "$hash_head"
> 
> which maybe is a handy thing to do.

Yeah. I admit I don't care overly much between that and doing something else sensible with multiple arguments (like putting each one in its own packet). I just really didn't want to silently ignore anything after "$1". This at least behaves like "echo".

Show 13 quoted lines

> > +	else
> > +		cat >packetize.tmp &&
> > +		len=$(wc -c <packetize.tmp) &&
> > +		printf '%04x' "$(($len + 4))" &&
> > +		cat packetize.tmp &&
> > +		rm -f packetize.tmp
> 
> 	perl -e '
> 		my $data = do { local $?; <STDIN> };
>                 printf "%04x%s", length($data), $data;
> 	'
> 
> That's one process but much heavier than cat/wc/printf/cat, I guess.

Yeah, I was tempted to do that, but ${#packet} is even one process shorter. :) It might be worth simplifying the stdin path above, but it's much less important if most of those call-sites go away.

-Peff