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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

I do not think I ran git-svn stuff myself for the past 10 years,
though, after a few projects that matter to me migrated away from
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".

Yeah, IIRC the list was written in response to a request for _some_
guidance, so it largely came from in-house rules of my previous
life, back when I had to deal with various flavours of UNIXen.

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

That checker came much much later than the guidelines so it is not
surprising at all for it to be "buggy", in the sense that it does
not check everything the guidelines ask.  Yes, we may need bugfixes
and there may be other bugs, too.

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

The largest issue (which is not that large, though) I felt with the
approach when I saw the patch for the first time was that we need
the big warning comment before the helper, i.e.

> +# Note that data containing NULs must be given on stdin,...

But after thinking about it a bit more, I think it is probably OK.
I do not think you can assign a string with NUL in it to a variable
and you can use "$(command substitution)" as an argument to the
helper to pass such a string, either (not portably anyway).  If such
a string cannot be made into "$*", ${#packet} won't have to worry
about counting bytes in a string with NUL in it to begin with, so
the above note won't even be necessary (it would have to say "you
cannot pass data containing NULs as arguments---you are welcome to
try, but you won't be able to do so, not portably anyway"), anyway
;-).