Re: [PATCH] t3900: add some more quotes

From: Jeff King <peff@peff.net>
Date: Jan 11, 2018, 09:38 UTC
Message-ID: <20180111093819.GB9190@sigill.intra.peff.net>
In-Reply-To: <xmqqy3l5z9hx.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 10, 2018 at 01:31:22PM -0800, Junio C Hamano wrote:

Show 21 quoted lines

> > +# Read from stdin into the variable given in $1.
> > +test_read_to_eof () {
> > +	# Bash's "read" is sufficiently flexible that we can skip the extra
> > +	# process.
> > +	if test -n "$BASH_VERSION"
> > +	then
> > +		# 64k should be enough for anyone...
> > +		read -N 65536 -r "$1"
> > +	else
> > +		# command substitution eats trailing whitespace, so we add
> > +		# and then remove a non-whitespace character.
> > +		eval "$1=\$(cat; printf x)"
> > +		eval "$1=\${$1%x}"
> > +	fi
> > +}
> 
> Yuck.  Hacky but nice.
> 
> I think this will make it easier to read but I suspect here text
> would use temporary files and may slow things down a bit?  I do not
> know if it is even measurable, though.

Yeah, since I was able to contain the horrible-ness in this function, I think the overall readability/portability is probably OK. My main concern was that it would be slower (hence the bash hackery). I applied the patch below on top to see what kind of impact we could measure across the whole suite:

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index be8a47d304..6f2e6e7560 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -441,6 +441,15 @@ test_expect_success () {
 	then
 		test_read_to_eof test_block
 		set -- "$1" "$test_block"
+	else
+		# this is obviously a dumb thing to do, but it's just
+		# a performance-testing hack.
+		test_read_to_eof test_block <<EOF
+$2
+EOF
+		# our here-doc hackery added an extra newline
+		set -- "$1" "${test_block%
+}"
 	fi
 	test_verify_prereq
 	export test_prereq

After doing five trial timings each of "make test" on:

  - stock git with dash
  - this patch with dash (so using "cat" to read the here-doc)
  - stock git with bash
  - this patch with bash (so using an internal "read")

I couldn't come up with any definitive slowdown.

In the first two, there's a fair bit of run-to-run noise (easily 10%),
and my best run was actually _with_ the patch (by only 3%, but still the
opposite of what you'd expect).

Running the (stock) suite with bash is definitively slower than with
dash (by about 20%). Adding in this patch didn't seem to make it any
slower.

So I dunno. Maybe it's not so crazy an idea.

-Peff