[RFC/PATCH 0/2] here-doc test bodies

[RFC/PATCH 0/2] here-doc test bodies

[Jeff King]

I've been wanting to do this for years, but after getting bitten by a
misplaced quote the other day, I finally did. This series allows you to
do:

  test_expect_success <<\EOT
          something 'with single quotes'
  EOT

Thoughts?

The first patch is the implementation. The second one shows it off.

  [1/2]: test-lib: allow test snippets as here-docs
  [2/2]: t1404: convert to here-doc test bodies

 t/README                     |   8 +
 t/t1404-update-ref-errors.sh | 274 +++++++++++++++++------------------
 t/test-lib-functions.sh      |  30 +++-
 3 files changed, 171 insertions(+), 141 deletions(-)

-Peff

[PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

Most test snippets are wrapped in single quotes, like:

  test_expect_success 'some description' '
          do_something
  '

This sometimes makes the snippets awkward to write, because you can't
easily use single quotes. We sometimes work around this with $SQ, or by
loosening regexes to use "." instead of a literal quote, or by using
double quotes when we'd prefer to use single-quotes (and just adding
extra backslash-escapes to avoid interpolation).

This commit adds another option: feeding the snippet on the function's
stdin. This doesn't conflict with anything the snippet would want to do,
because we always redirect its stdin from /dev/null anyway (which we'll
continue to do).

A few notes on the implementation:

  - it would be nice to push this down into test_run_, but we can't, as
    test_expect_success and test_expect_failure want to see the actual
    script content to report it for verbose-mode. A helper function
    limits the amount of duplication in those callers here.

  - The helper function is a little awkward to call, as you feed it the
    name of the variable you want to set. The more natural thing in
    shell would be command substitution like:

      body=$(body_or_stdin "$2")

    but that loses trailing whitespace. There are tricks around this,
    like:

      body=$(body_or_stdin "$2"; printf '.)
      body=${body%.}

    but we'd prefer to keep such tricks in the helper, not in each
    caller.

  - I implemented the helper using a sequence of "read" calls. Together
    with "-r" and unsetting the IFS, this preserves incoming whitespace.
    An alternative is to use "cat" (which then requires the gross "."
    trick above). But this saves us a process, which is probably a good
    thing. The "read" builtin does use more read() syscalls than
    necessary (one per byte), but that is almost certainly a win over a
    separate process.

    Both are probably slower than passing a single-quoted string, but
    the difference is lost in the noise for a script that I converted as
    an experiment.

  - I handle test_expect_success and test_expect_failure here. If we
    like this style, we could easily extend it to other spots (e.g.,
    lazy_prereq bodies) on top of this patch.

  - even though we are using "local", we have to be careful about our
    variable names. Within test_expect_success, any variable we declare
    with local will be seen by the test snippets themselves (so it won't
    persist between tests like normal variables would).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/README                |  8 ++++++++
 t/test-lib-functions.sh | 30 ++++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/t/README b/t/README
index fd9375b146..a234c87792 100644
--- a/t/README
+++ b/t/README
@@ -755,6 +755,14 @@ library for your script to use.
 	    'git-write-tree should be able to write an empty tree.' \
 	    'tree=$(git-write-tree)'
 
+   If <script> is `-` (a single dash), then the script to run is read
+   from stdin. This lets you more easily use single quotes within the
+   script by using a here-doc. For example:
+
+        test_expect_success 'output contains expected string' <<-\EOT
+	        grep "this string has 'quotes' in it" output
+	EOT
+
    If you supply three parameters the first will be taken to be a
    prerequisite; see the test_set_prereq and test_have_prereq
    documentation below:
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6348e8d733..3c8081b256 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -602,6 +602,24 @@ test_verify_prereq () {
 	BUG "'$test_prereq' does not look like a prereq"
 }
 
+# assign the variable named by "$1" with the contents of "$2";
+# if "$2" is "-", then read stdin into "$1" instead
+test_body_or_stdin () {
+	if test "$2" != "-"
+	then
+		eval "$1=\$2"
+		return
+	fi
+
+	# start with a newline, to match hanging newline from open-quote style
+	eval "$1=\$LF"
+	local test_line
+	while IFS= read -r test_line
+	do
+		eval "$1=\${$1}\${test_line}\${LF}"
+	done
+}
+
 test_expect_failure () {
 	test_start_
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
@@ -611,8 +629,10 @@ test_expect_failure () {
 	export test_prereq
 	if ! test_skip "$@"
 	then
-		say >&3 "checking known breakage of $TEST_NUMBER.$test_count '$1': $2"
-		if test_run_ "$2" expecting_failure
+		local test_body
+		test_body_or_stdin test_body "$2"
+		say >&3 "checking known breakage of $TEST_NUMBER.$test_count '$1': $test_body"
+		if test_run_ "$test_body" expecting_failure
 		then
 			test_known_broken_ok_ "$1"
 		else
@@ -631,8 +651,10 @@ test_expect_success () {
 	export test_prereq
 	if ! test_skip "$@"
 	then
-		say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $2"
-		if test_run_ "$2"
+		local test_body
+		test_body_or_stdin test_body "$2"
+		say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $test_body"
+		if test_run_ "$test_body"
 		then
 			test_ok_ "$1"
 		else
-- 
2.31.1.629.gb61be4ec8d

[PATCH 2/2] t1404: convert to here-doc test bodies

[Jeff King]

The t1404 script checks a lot of output from Git which contains single
quotes. Because the test snippets are themselves wrapped in the same
single-quotes, we have to resort to using $SQ to match them. This is
error-prone and makes the tests harder to read.

Instead, let's use the new here-doc feature added in the previous
commit, which lets us write anything in the test body we want (except
the here-doc end marker on a line by itself, of course).

Note that we do use "\" in our marker to avoid interpolation (which is
the whole point). But we don't use "<<-", as we want to preserve
whitespace in the snippet (and running with "-v" before and after shows
that we produce the exact same output, except with the ugly $SQ
references fixed).

I just converted every test here, even though only some of them use
$SQ. But it would be equally correct to mix-and-match styles if we don't
mind the inconsistency.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t1404-update-ref-errors.sh | 274 +++++++++++++++++------------------
 1 file changed, 137 insertions(+), 137 deletions(-)

diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 8b51c4efc1..16ada22148 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -91,7 +91,7 @@ df_test() {
 		delname="$delref"
 	fi &&
 	cat >expected-err <<-EOF &&
-	fatal: cannot lock ref $SQ$addname$SQ: $SQ$delref$SQ exists; cannot create $SQ$addref$SQ
+	fatal: cannot lock ref '$addname': '$delref' exists; cannot create '$addref'
 	EOF
 	$pack &&
 	if $add_del
@@ -107,443 +107,443 @@ df_test() {
 	test_cmp expected-refs actual-refs
 }
 
-test_expect_success 'setup' '
+test_expect_success 'setup' - <<\EOT
 
 	git commit --allow-empty -m Initial &&
 	C=$(git rev-parse HEAD) &&
 	git commit --allow-empty -m Second &&
 	D=$(git rev-parse HEAD) &&
 	git commit --allow-empty -m Third &&
 	E=$(git rev-parse HEAD)
-'
+EOT
 
-test_expect_success 'existing loose ref is a simple prefix of new' '
+test_expect_success 'existing loose ref is a simple prefix of new' - <<\EOT
 
 	prefix=refs/1l &&
 	test_update_rejected "a c e" false "b c/x d" \
-		"$SQ$prefix/c$SQ exists; cannot create $SQ$prefix/c/x$SQ"
+		"'$prefix/c' exists; cannot create '$prefix/c/x'"
 
-'
+EOT
 
-test_expect_success 'existing packed ref is a simple prefix of new' '
+test_expect_success 'existing packed ref is a simple prefix of new' - <<\EOT
 
 	prefix=refs/1p &&
 	test_update_rejected "a c e" true "b c/x d" \
-		"$SQ$prefix/c$SQ exists; cannot create $SQ$prefix/c/x$SQ"
+		"'$prefix/c' exists; cannot create '$prefix/c/x'"
 
-'
+EOT
 
-test_expect_success 'existing loose ref is a deeper prefix of new' '
+test_expect_success 'existing loose ref is a deeper prefix of new' - <<\EOT
 
 	prefix=refs/2l &&
 	test_update_rejected "a c e" false "b c/x/y d" \
-		"$SQ$prefix/c$SQ exists; cannot create $SQ$prefix/c/x/y$SQ"
+		"'$prefix/c' exists; cannot create '$prefix/c/x/y'"
 
-'
+EOT
 
-test_expect_success 'existing packed ref is a deeper prefix of new' '
+test_expect_success 'existing packed ref is a deeper prefix of new' - <<\EOT
 
 	prefix=refs/2p &&
 	test_update_rejected "a c e" true "b c/x/y d" \
-		"$SQ$prefix/c$SQ exists; cannot create $SQ$prefix/c/x/y$SQ"
+		"'$prefix/c' exists; cannot create '$prefix/c/x/y'"
 
-'
+EOT
 
-test_expect_success 'new ref is a simple prefix of existing loose' '
+test_expect_success 'new ref is a simple prefix of existing loose' - <<\EOT
 
 	prefix=refs/3l &&
 	test_update_rejected "a c/x e" false "b c d" \
-		"$SQ$prefix/c/x$SQ exists; cannot create $SQ$prefix/c$SQ"
+		"'$prefix/c/x' exists; cannot create '$prefix/c'"
 
-'
+EOT
 
-test_expect_success 'new ref is a simple prefix of existing packed' '
+test_expect_success 'new ref is a simple prefix of existing packed' - <<\EOT
 
 	prefix=refs/3p &&
 	test_update_rejected "a c/x e" true "b c d" \
-		"$SQ$prefix/c/x$SQ exists; cannot create $SQ$prefix/c$SQ"
+		"'$prefix/c/x' exists; cannot create '$prefix/c'"
 
-'
+EOT
 
-test_expect_success 'new ref is a deeper prefix of existing loose' '
+test_expect_success 'new ref is a deeper prefix of existing loose' - <<\EOT
 
 	prefix=refs/4l &&
 	test_update_rejected "a c/x/y e" false "b c d" \
-		"$SQ$prefix/c/x/y$SQ exists; cannot create $SQ$prefix/c$SQ"
+		"'$prefix/c/x/y' exists; cannot create '$prefix/c'"
 
-'
+EOT
 
-test_expect_success 'new ref is a deeper prefix of existing packed' '
+test_expect_success 'new ref is a deeper prefix of existing packed' - <<\EOT
 
 	prefix=refs/4p &&
 	test_update_rejected "a c/x/y e" true "b c d" \
-		"$SQ$prefix/c/x/y$SQ exists; cannot create $SQ$prefix/c$SQ"
+		"'$prefix/c/x/y' exists; cannot create '$prefix/c'"
 
-'
+EOT
 
-test_expect_success 'one new ref is a simple prefix of another' '
+test_expect_success 'one new ref is a simple prefix of another' - <<\EOT
 
 	prefix=refs/5 &&
 	test_update_rejected "a e" false "b c c/x d" \
-		"cannot process $SQ$prefix/c$SQ and $SQ$prefix/c/x$SQ at the same time"
+		"cannot process '$prefix/c' and '$prefix/c/x' at the same time"
 
-'
+EOT
 
-test_expect_success 'empty directory should not fool rev-parse' '
+test_expect_success 'empty directory should not fool rev-parse' - <<\EOT
 	prefix=refs/e-rev-parse &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
 	mkdir -p .git/$prefix/foo/bar/baz &&
 	echo "$C" >expected &&
 	git rev-parse $prefix/foo >actual &&
 	test_cmp expected actual
-'
+EOT
 
-test_expect_success 'empty directory should not fool for-each-ref' '
+test_expect_success 'empty directory should not fool for-each-ref' - <<\EOT
 	prefix=refs/e-for-each-ref &&
 	git update-ref $prefix/foo $C &&
 	git for-each-ref $prefix >expected &&
 	git pack-refs --all &&
 	mkdir -p .git/$prefix/foo/bar/baz &&
 	git for-each-ref $prefix >actual &&
 	test_cmp expected actual
-'
+EOT
 
-test_expect_success 'empty directory should not fool create' '
+test_expect_success 'empty directory should not fool create' - <<\EOT
 	prefix=refs/e-create &&
 	mkdir -p .git/$prefix/foo/bar/baz &&
 	printf "create %s $C\n" $prefix/foo |
 	git update-ref --stdin
-'
+EOT
 
-test_expect_success 'empty directory should not fool verify' '
+test_expect_success 'empty directory should not fool verify' - <<\EOT
 	prefix=refs/e-verify &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
 	mkdir -p .git/$prefix/foo/bar/baz &&
 	printf "verify %s $C\n" $prefix/foo |
 	git update-ref --stdin
-'
+EOT
 
-test_expect_success 'empty directory should not fool 1-arg update' '
+test_expect_success 'empty directory should not fool 1-arg update' - <<\EOT
 	prefix=refs/e-update-1 &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
 	mkdir -p .git/$prefix/foo/bar/baz &&
 	printf "update %s $D\n" $prefix/foo |
 	git update-ref --stdin
-'
+EOT
 
-test_expect_success 'empty directory should not fool 2-arg update' '
+test_expect_success 'empty directory should not fool 2-arg update' - <<\EOT
 	prefix=refs/e-update-2 &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
 	mkdir -p .git/$prefix/foo/bar/baz &&
 	printf "update %s $D $C\n" $prefix/foo |
 	git update-ref --stdin
-'
+EOT
 
-test_expect_success 'empty directory should not fool 0-arg delete' '
+test_expect_success 'empty directory should not fool 0-arg delete' - <<\EOT
 	prefix=refs/e-delete-0 &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
 	mkdir -p .git/$prefix/foo/bar/baz &&
 	printf "delete %s\n" $prefix/foo |
 	git update-ref --stdin
-'
+EOT
 
-test_expect_success 'empty directory should not fool 1-arg delete' '
+test_expect_success 'empty directory should not fool 1-arg delete' - <<\EOT
 	prefix=refs/e-delete-1 &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
 	mkdir -p .git/$prefix/foo/bar/baz &&
 	printf "delete %s $C\n" $prefix/foo |
 	git update-ref --stdin
-'
+EOT
 
-test_expect_success 'D/F conflict prevents add long + delete short' '
+test_expect_success 'D/F conflict prevents add long + delete short' - <<\EOT
 	df_test refs/df-al-ds --add-del foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents add short + delete long' '
+test_expect_success 'D/F conflict prevents add short + delete long' - <<\EOT
 	df_test refs/df-as-dl --add-del foo foo/bar
-'
+EOT
 
-test_expect_success 'D/F conflict prevents delete long + add short' '
+test_expect_success 'D/F conflict prevents delete long + add short' - <<\EOT
 	df_test refs/df-dl-as --del-add foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents delete short + add long' '
+test_expect_success 'D/F conflict prevents delete short + add long' - <<\EOT
 	df_test refs/df-ds-al --del-add foo foo/bar
-'
+EOT
 
-test_expect_success 'D/F conflict prevents add long + delete short packed' '
+test_expect_success 'D/F conflict prevents add long + delete short packed' - <<\EOT
 	df_test refs/df-al-dsp --pack --add-del foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents add short + delete long packed' '
+test_expect_success 'D/F conflict prevents add short + delete long packed' - <<\EOT
 	df_test refs/df-as-dlp --pack --add-del foo foo/bar
-'
+EOT
 
-test_expect_success 'D/F conflict prevents delete long packed + add short' '
+test_expect_success 'D/F conflict prevents delete long packed + add short' - <<\EOT
 	df_test refs/df-dlp-as --pack --del-add foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents delete short packed + add long' '
+test_expect_success 'D/F conflict prevents delete short packed + add long' - <<\EOT
 	df_test refs/df-dsp-al --pack --del-add foo foo/bar
-'
+EOT
 
 # Try some combinations involving symbolic refs...
 
-test_expect_success 'D/F conflict prevents indirect add long + delete short' '
+test_expect_success 'D/F conflict prevents indirect add long + delete short' - <<\EOT
 	df_test refs/df-ial-ds --sym-add --add-del foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents indirect add long + indirect delete short' '
+test_expect_success 'D/F conflict prevents indirect add long + indirect delete short' - <<\EOT
 	df_test refs/df-ial-ids --sym-add --sym-del --add-del foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents indirect add short + indirect delete long' '
+test_expect_success 'D/F conflict prevents indirect add short + indirect delete long' - <<\EOT
 	df_test refs/df-ias-idl --sym-add --sym-del --add-del foo foo/bar
-'
+EOT
 
-test_expect_success 'D/F conflict prevents indirect delete long + indirect add short' '
+test_expect_success 'D/F conflict prevents indirect delete long + indirect add short' - <<\EOT
 	df_test refs/df-idl-ias --sym-add --sym-del --del-add foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents indirect add long + delete short packed' '
+test_expect_success 'D/F conflict prevents indirect add long + delete short packed' - <<\EOT
 	df_test refs/df-ial-dsp --sym-add --pack --add-del foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents indirect add long + indirect delete short packed' '
+test_expect_success 'D/F conflict prevents indirect add long + indirect delete short packed' - <<\EOT
 	df_test refs/df-ial-idsp --sym-add --sym-del --pack --add-del foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents add long + indirect delete short packed' '
+test_expect_success 'D/F conflict prevents add long + indirect delete short packed' - <<\EOT
 	df_test refs/df-al-idsp --sym-del --pack --add-del foo/bar foo
-'
+EOT
 
-test_expect_success 'D/F conflict prevents indirect delete long packed + indirect add short' '
+test_expect_success 'D/F conflict prevents indirect delete long packed + indirect add short' - <<\EOT
 	df_test refs/df-idlp-ias --sym-add --sym-del --pack --del-add foo/bar foo
-'
+EOT
 
 # Test various errors when reading the old values of references...
 
-test_expect_success 'missing old value blocks update' '
+test_expect_success 'missing old value blocks update' - <<\EOT
 	prefix=refs/missing-update &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ
+	fatal: cannot lock ref '$prefix/foo': unable to resolve reference '$prefix/foo'
 	EOF
 	printf "%s\n" "update $prefix/foo $E $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'incorrect old value blocks update' '
+test_expect_success 'incorrect old value blocks update' - <<\EOT
 	prefix=refs/incorrect-update &&
 	git update-ref $prefix/foo $C &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: is at $C but expected $D
+	fatal: cannot lock ref '$prefix/foo': is at $C but expected $D
 	EOF
 	printf "%s\n" "update $prefix/foo $E $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'existing old value blocks create' '
+test_expect_success 'existing old value blocks create' - <<\EOT
 	prefix=refs/existing-create &&
 	git update-ref $prefix/foo $C &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: reference already exists
+	fatal: cannot lock ref '$prefix/foo': reference already exists
 	EOF
 	printf "%s\n" "create $prefix/foo $E" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'incorrect old value blocks delete' '
+test_expect_success 'incorrect old value blocks delete' - <<\EOT
 	prefix=refs/incorrect-delete &&
 	git update-ref $prefix/foo $C &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: is at $C but expected $D
+	fatal: cannot lock ref '$prefix/foo': is at $C but expected $D
 	EOF
 	printf "%s\n" "delete $prefix/foo $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'missing old value blocks indirect update' '
+test_expect_success 'missing old value blocks indirect update' - <<\EOT
 	prefix=refs/missing-indirect-update &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ
+	fatal: cannot lock ref '$prefix/symref': unable to resolve reference '$prefix/foo'
 	EOF
 	printf "%s\n" "update $prefix/symref $E $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'incorrect old value blocks indirect update' '
+test_expect_success 'incorrect old value blocks indirect update' - <<\EOT
 	prefix=refs/incorrect-indirect-update &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	git update-ref $prefix/foo $C &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: is at $C but expected $D
+	fatal: cannot lock ref '$prefix/symref': is at $C but expected $D
 	EOF
 	printf "%s\n" "update $prefix/symref $E $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'existing old value blocks indirect create' '
+test_expect_success 'existing old value blocks indirect create' - <<\EOT
 	prefix=refs/existing-indirect-create &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	git update-ref $prefix/foo $C &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: reference already exists
+	fatal: cannot lock ref '$prefix/symref': reference already exists
 	EOF
 	printf "%s\n" "create $prefix/symref $E" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'incorrect old value blocks indirect delete' '
+test_expect_success 'incorrect old value blocks indirect delete' - <<\EOT
 	prefix=refs/incorrect-indirect-delete &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	git update-ref $prefix/foo $C &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: is at $C but expected $D
+	fatal: cannot lock ref '$prefix/symref': is at $C but expected $D
 	EOF
 	printf "%s\n" "delete $prefix/symref $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'missing old value blocks indirect no-deref update' '
+test_expect_success 'missing old value blocks indirect no-deref update' - <<\EOT
 	prefix=refs/missing-noderef-update &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: reference is missing but expected $D
+	fatal: cannot lock ref '$prefix/symref': reference is missing but expected $D
 	EOF
 	printf "%s\n" "option no-deref" "update $prefix/symref $E $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'incorrect old value blocks indirect no-deref update' '
+test_expect_success 'incorrect old value blocks indirect no-deref update' - <<\EOT
 	prefix=refs/incorrect-noderef-update &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	git update-ref $prefix/foo $C &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: is at $C but expected $D
+	fatal: cannot lock ref '$prefix/symref': is at $C but expected $D
 	EOF
 	printf "%s\n" "option no-deref" "update $prefix/symref $E $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'existing old value blocks indirect no-deref create' '
+test_expect_success 'existing old value blocks indirect no-deref create' - <<\EOT
 	prefix=refs/existing-noderef-create &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	git update-ref $prefix/foo $C &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: reference already exists
+	fatal: cannot lock ref '$prefix/symref': reference already exists
 	EOF
 	printf "%s\n" "option no-deref" "create $prefix/symref $E" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'incorrect old value blocks indirect no-deref delete' '
+test_expect_success 'incorrect old value blocks indirect no-deref delete' - <<\EOT
 	prefix=refs/incorrect-noderef-delete &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	git update-ref $prefix/foo $C &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: is at $C but expected $D
+	fatal: cannot lock ref '$prefix/symref': is at $C but expected $D
 	EOF
 	printf "%s\n" "option no-deref" "delete $prefix/symref $D" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'non-empty directory blocks create' '
+test_expect_success 'non-empty directory blocks create' - <<\EOT
 	prefix=refs/ne-create &&
 	mkdir -p .git/$prefix/foo/bar &&
 	: >.git/$prefix/foo/bar/baz.lock &&
 	test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: there is a non-empty directory $SQ.git/$prefix/foo$SQ blocking reference $SQ$prefix/foo$SQ
+	fatal: cannot lock ref '$prefix/foo': there is a non-empty directory '.git/$prefix/foo' blocking reference '$prefix/foo'
 	EOF
 	printf "%s\n" "update $prefix/foo $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ
+	fatal: cannot lock ref '$prefix/foo': unable to resolve reference '$prefix/foo'
 	EOF
 	printf "%s\n" "update $prefix/foo $D $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'broken reference blocks create' '
+test_expect_success 'broken reference blocks create' - <<\EOT
 	prefix=refs/broken-create &&
 	mkdir -p .git/$prefix &&
 	echo "gobbledigook" >.git/$prefix/foo &&
 	test_when_finished "rm -f .git/$prefix/foo" &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
+	fatal: cannot lock ref '$prefix/foo': unable to resolve reference '$prefix/foo': reference broken
 	EOF
 	printf "%s\n" "update $prefix/foo $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/foo$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
+	fatal: cannot lock ref '$prefix/foo': unable to resolve reference '$prefix/foo': reference broken
 	EOF
 	printf "%s\n" "update $prefix/foo $D $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'non-empty directory blocks indirect create' '
+test_expect_success 'non-empty directory blocks indirect create' - <<\EOT
 	prefix=refs/ne-indirect-create &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	mkdir -p .git/$prefix/foo/bar &&
 	: >.git/$prefix/foo/bar/baz.lock &&
 	test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: there is a non-empty directory $SQ.git/$prefix/foo$SQ blocking reference $SQ$prefix/foo$SQ
+	fatal: cannot lock ref '$prefix/symref': there is a non-empty directory '.git/$prefix/foo' blocking reference '$prefix/foo'
 	EOF
 	printf "%s\n" "update $prefix/symref $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ
+	fatal: cannot lock ref '$prefix/symref': unable to resolve reference '$prefix/foo'
 	EOF
 	printf "%s\n" "update $prefix/symref $D $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'broken reference blocks indirect create' '
+test_expect_success 'broken reference blocks indirect create' - <<\EOT
 	prefix=refs/broken-indirect-create &&
 	git symbolic-ref $prefix/symref $prefix/foo &&
 	echo "gobbledigook" >.git/$prefix/foo &&
 	test_when_finished "rm -f .git/$prefix/foo" &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
+	fatal: cannot lock ref '$prefix/symref': unable to resolve reference '$prefix/foo': reference broken
 	EOF
 	printf "%s\n" "update $prefix/symref $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err &&
 	cat >expected <<-EOF &&
-	fatal: cannot lock ref $SQ$prefix/symref$SQ: unable to resolve reference $SQ$prefix/foo$SQ: reference broken
+	fatal: cannot lock ref '$prefix/symref': unable to resolve reference '$prefix/foo': reference broken
 	EOF
 	printf "%s\n" "update $prefix/symref $D $C" |
 	test_must_fail git update-ref --stdin 2>output.err &&
 	test_cmp expected output.err
-'
+EOT
 
-test_expect_success 'no bogus intermediate values during delete' '
+test_expect_success 'no bogus intermediate values during delete' - <<\EOT
 	prefix=refs/slow-transaction &&
 	# Set up a reference with differing loose and packed versions:
 	git update-ref $prefix/foo $C &&
@@ -598,9 +598,9 @@ test_expect_success 'no bogus intermediate values during delete' '
 	wait $pid2 &&
 	test_must_be_empty out &&
 	test_must_fail git rev-parse --verify --quiet $prefix/foo
-'
+EOT
 
-test_expect_success 'delete fails cleanly if packed-refs file is locked' '
+test_expect_success 'delete fails cleanly if packed-refs file is locked' - <<\EOT
 	prefix=refs/locked-packed-refs &&
 	# Set up a reference with differing loose and packed versions:
 	git update-ref $prefix/foo $C &&
@@ -612,11 +612,11 @@ test_expect_success 'delete fails cleanly if packed-refs file is locked' '
 	test_when_finished "rm -f .git/packed-refs.lock" &&
 	test_must_fail git update-ref -d $prefix/foo >out 2>err &&
 	git for-each-ref $prefix >actual &&
-	test_i18ngrep "Unable to create $SQ.*packed-refs.lock$SQ: " err &&
+	test_i18ngrep "Unable to create '.*packed-refs.lock': " err &&
 	test_cmp unchanged actual
-'
+EOT
 
-test_expect_success 'delete fails cleanly if packed-refs.new write fails' '
+test_expect_success 'delete fails cleanly if packed-refs.new write fails' - <<\EOT
 	# Setup and expectations are similar to the test above.
 	prefix=refs/failed-packed-refs &&
 	git update-ref $prefix/foo $C &&
@@ -630,6 +630,6 @@ test_expect_success 'delete fails cleanly if packed-refs.new write fails' '
 	test_must_fail git update-ref -d $prefix/foo &&
 	git for-each-ref $prefix >actual &&
 	test_cmp unchanged actual
-'
+EOT
 
 test_done
-- 
2.31.1.629.gb61be4ec8d

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Fri, Apr 09, 2021 at 06:28:19PM -0400, Jeff King wrote:

> +   If <script> is `-` (a single dash), then the script to run is read
> +   from stdin. This lets you more easily use single quotes within the
> +   script by using a here-doc. For example:
> +
> +        test_expect_success 'output contains expected string' <<-\EOT
> +	        grep "this string has 'quotes' in it" output
> +	EOT

Whoops, this should have an extra "-", of course (I got this wrong in
the cover letter, too). I.e.:

  test_expect_success 'whatever' - <<\EOT
     ...
  EOT

It would be nice to drop it, but then:

  test_expect_success PREREQ 'whatever' <<\EOT

becomes ambiguous (and I don't think there is a portable and reliable
way to decide that our input is coming from a here-doc versus the
original stdin).

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +   If <script> is `-` (a single dash), then the script to run is read
> +   from stdin. This lets you more easily use single quotes within the
> +   script by using a here-doc. For example:
> +
> +        test_expect_success 'output contains expected string' <<-\EOT

Missing '-'?

> +	        grep "this string has 'quotes' in it" output
> +	EOT
> +
> ...
> +	# start with a newline, to match hanging newline from open-quote style
> +	eval "$1=\$LF"
> +	local test_line
> +	while IFS= read -r test_line
> +	do
> +		eval "$1=\${$1}\${test_line}\${LF}"
> +	done

I wonder if we can do this without relying on "read -r" (which I
distrust, perhaps out of superstition)?  Perhaps by slurping the
whole thing with "$(cat)"?

Thanks.

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +	# start with a newline, to match hanging newline from open-quote style
>> +	eval "$1=\$LF"
>> +	local test_line
>> +	while IFS= read -r test_line
>> +	do
>> +		eval "$1=\${$1}\${test_line}\${LF}"
>> +	done
>
> I wonder if we can do this without relying on "read -r" (which I
> distrust, perhaps out of superstition)?  Perhaps by slurping the
> whole thing with "$(cat)"?

Meaning, something along this line...

----- >8 --------- >8 --------- >8 --------- >8 ----
#!/bin/sh
LF='
'
ttt () {
	eval "$1"='$LF$(cat)'
}
ttt script <<\EOT
	a
	b
EOT
echo "<<<$script>>>"
----- 8< --------- 8< --------- 8< --------- 8< ----

Re: [RFC/PATCH 0/2] here-doc test bodies

[Derrick Stolee]

On 4/9/2021 6:26 PM, Jeff King wrote:
> I've been wanting to do this for years, but after getting bitten by a
> misplaced quote the other day, I finally did. This series allows you to
> do:
> 
>   test_expect_success <<\EOT
>           something 'with single quotes'
>   EOT
> 
> Thoughts?

Odd. I _just_ hit this for the first time today. I didn't know
about the $SQ trick, so I just modified my 'sed' call to drop
the single quotes from the string I was trying to match [1].

[1] https://lore.kernel.org/git/36aa6837-722c-9ef0-84cc-77e982db9f6e@gmail.com/

I think this is a good pattern for this kind of thing. I
imagine that if the test itself needed heredocs, then they
would be interpreted correctly when evaluating stdin (as
long as a different identifier is used).

I also like the "EOT" pattern as an example.

Thanks,
-Stolee

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Fri, Apr 09, 2021 at 05:57:10PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> +	# start with a newline, to match hanging newline from open-quote style
> >> +	eval "$1=\$LF"
> >> +	local test_line
> >> +	while IFS= read -r test_line
> >> +	do
> >> +		eval "$1=\${$1}\${test_line}\${LF}"
> >> +	done
> >
> > I wonder if we can do this without relying on "read -r" (which I
> > distrust, perhaps out of superstition)?  Perhaps by slurping the
> > whole thing with "$(cat)"?
> 
> Meaning, something along this line...
> 
> ----- >8 --------- >8 --------- >8 --------- >8 ----
> #!/bin/sh
> LF='
> '
> ttt () {
> 	eval "$1"='$LF$(cat)'
> }
> ttt script <<\EOT
> 	a
> 	b
> EOT
> echo "<<<$script>>>"
> ----- 8< --------- 8< --------- 8< --------- 8< ----

I wrote it using cat initially. If you want to preserve trailing
whitespace, you have to do something like:

  val=$(printf '\n'
        cat
	printf '.')
  val=${val%.}

I was worried about adding the overhead of the extra subshell and
process for the command substitution, but perhaps that is overblown.
TBH, worrying about whitespace may be overblown, too. Some test snippets
have extra blank lines at the end, but possibly we should just not care.

I imagine "read -r" does not work reliably for binary junk, but keep in
mind that we are talking about text shell script snippets (that are
already in shell strings anyway).

-Peff

Re: [RFC/PATCH 0/2] here-doc test bodies

[Jeff King]

On Fri, Apr 09, 2021 at 09:03:24PM -0400, Derrick Stolee wrote:

> On 4/9/2021 6:26 PM, Jeff King wrote:
> > I've been wanting to do this for years, but after getting bitten by a
> > misplaced quote the other day, I finally did. This series allows you to
> > do:
> > 
> >   test_expect_success <<\EOT
> >           something 'with single quotes'
> >   EOT
> > 
> > Thoughts?
> 
> Odd. I _just_ hit this for the first time today. I didn't know
> about the $SQ trick, so I just modified my 'sed' call to drop
> the single quotes from the string I was trying to match [1].
> 
> [1] https://lore.kernel.org/git/36aa6837-722c-9ef0-84cc-77e982db9f6e@gmail.com/

Yep, that case would definitely benefit. We have a variety of little
workarounds now, but it would be nice to not even need them.

> I think this is a good pattern for this kind of thing. I
> imagine that if the test itself needed heredocs, then they
> would be interpreted correctly when evaluating stdin (as
> long as a different identifier is used).

Yeah, some of the tests I converted in t1404 have their own here-docs.
It works for the same reason that the single-quoted snippets work: the
outer doc slurps the whole thing to the "EOT" marker. So we do not even
consider the inner here-docs until we are eval-ing the snippet.

> I also like the "EOT" pattern as an example.

I think it would make sense to have a convention here for readability.
Also, I added this to the sharness vim highlighter from [1]:

diff --git a/sharness.vim b/sharness.vim
index 6ffc64f..7dbcd18 100644
--- a/sharness.vim
+++ b/sharness.vim
@@ -21,6 +21,9 @@ syn region shsTestBody contained transparent excludenl matchgroup=shQuote start=
 syn region shsTestBody contained oneline transparent excludenl keepend matchgroup=shQuote start=+ '+hs=s+1 end=+'$+ contains=@shSubShList
 syn region shsTestBody contained oneline transparent excludenl keepend matchgroup=shQuote start=+ "+hs=s+1 end=+"$+ contains=@shSubShList
 
+" here-doc body
+syn region shsTestBody contained transparent excludenl matchgroup=shQuote start=+ <<\\EOT+ end=+EOT$+ contains=@shSubShList
+
 syn match shsPrereq contained "\<[A-Z_,]\+\>" nextgroup=shsTestTitle
 syn match shsPrereqLazy contained "\<[A-Z_,]\+\>" nextgroup=shsTestBody
 

which makes the innards look nice. :) I think it could be written to
allow any marker, but there would probably need to be some magic with
matching the opening and closing markers.

-Peff

[1] https://lore.kernel.org/git/CAMP44s1D-Zp3KDS+Hi74a=Lkc7Nc_0qiEzQEF0Pmj=bD8i+=JQ@mail.gmail.com/

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Ævar Arnfjörð Bjarmason]

On Sat, Apr 10 2021, Jeff King wrote:

> Most test snippets are wrapped in single quotes, like:
>
>   test_expect_success 'some description' '
>           do_something
>   '
>
> This sometimes makes the snippets awkward to write, because you can't
> easily use single quotes. We sometimes work around this with $SQ, or by
> loosening regexes to use "." instead of a literal quote, or by using
> double quotes when we'd prefer to use single-quotes (and just adding
> extra backslash-escapes to avoid interpolation).
>
> This commit adds another option: feeding the snippet on the function's
> stdin. This doesn't conflict with anything the snippet would want to do,
> because we always redirect its stdin from /dev/null anyway (which we'll
> continue to do).

I like this, and not having to write $SQ, '"'"' etc.

> A few notes on the implementation:
>
>   - it would be nice to push this down into test_run_, but we can't, as
>     test_expect_success and test_expect_failure want to see the actual
>     script content to report it for verbose-mode. A helper function
>     limits the amount of duplication in those callers here.

I've got an unsubmitted series (a bigger part of the -V rewrite) which
conflicted with this one, because I'd moved that message into those
helper functions.

But in that case I end up having to have this in
test_expect_{success,failure} anyway, because the change I had was to
move it into test_{ok,failure}_, i.e. to color the emitted body under
verbose differently depending on test ok/failure (which means deferring
the "this is our test body" until after the run).

It got slightly awkward because before I could pass "$@" to those (they
pass "$1" now), but with your change there's a "-" left on the argument
list, so we need to pass "$1" and "$test_body".

Anyway, it's no problem, just musings on having re-arranged this code
you're pointing out needs/could be re-arranged.

Maybe it would be easier to pass test_run arguments saying whether we
expect failure or not, and then move the whole if/else after it into its
own body. It already takes the "expecting_failure" parameter, so this on
top of master:

	diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
	index 6348e8d733..9e20bd607d 100644
	--- a/t/test-lib-functions.sh
	+++ b/t/test-lib-functions.sh
	@@ -611,8 +611,7 @@ test_expect_failure () {
	 	export test_prereq
	 	if ! test_skip "$@"
	 	then
	-		say >&3 "checking known breakage of $TEST_NUMBER.$test_count '$1': $2"
	-		if test_run_ "$2" expecting_failure
	+		if test_run_ "$1" "$2" expecting_failure
	 		then
	 			test_known_broken_ok_ "$1"
	 		else
	@@ -631,8 +630,7 @@ test_expect_success () {
	 	export test_prereq
	 	if ! test_skip "$@"
	 	then
	-		say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $2"
	-		if test_run_ "$2"
	+		if test_run_ "$1" "$2"
	 		then
	 			test_ok_ "$1"
	 		else
	diff --git a/t/test-lib.sh b/t/test-lib.sh
	index d3f6af6a65..5a1192e80c 100644
	--- a/t/test-lib.sh
	+++ b/t/test-lib.sh
	@@ -935,9 +935,20 @@ test_eval_ () {
	 }
	 
	 test_run_ () {
	+	local description
	+	description="$1"
	+	shift
	+
	 	test_cleanup=:
	 	expecting_failure=$2
	 
	+	if test -n "$expecting_failure"
	+	then
	+		say >&3 "checking known breakage of $TEST_NUMBER.$test_count '$description': $1"
	+	else
	+		say >&3 "expecting success of $TEST_NUMBER.$test_count '$description': $1"
	+	fi
	+
	 	if test "${GIT_TEST_CHAIN_LINT:-1}" != 0; then
	 		# turn off tracing for this test-eval, as it simply creates
	 		# confusing noise in the "-x" output

... or maybe not, but in any case, if the verbose mode was what was
stopping you from moving this down to "test_run_" just that seems like
an easy change.

I like your current implementation better, i.e. to have the stdin
consumption happen ASAP and have the others be low-level utility
functions, but I don't care much, but if you wanted it the other way
maybe the above diff helps.
	
>   - The helper function is a little awkward to call, as you feed it the
>     name of the variable you want to set. The more natural thing in
>     shell would be command substitution like:
>
>       body=$(body_or_stdin "$2")
>
>     but that loses trailing whitespace. There are tricks around this,
>     like:
>
>       body=$(body_or_stdin "$2"; printf '.)
>       body=${body%.}
>
>     but we'd prefer to keep such tricks in the helper, not in each
>     caller.

I see why you did this, and for a narrow change it's a good thing.

FWIW having spent some more time on making the TAP format more pruttah
in a parallel WIP series I think this is ultimately a losing
game. You're inserting the extra LF because you don't want to have the
"checking..." and the first line of the test body on the same line;

But we have all of:

    test_expect_success 'foo' 'true'
    test_expect_success 'foo' '
        true
    '

And now:

    test_expect_success 'foo' - <<\EOT
        true
    '

So if (definitely not needed for your change) wanted to always make this
pretty/indented we'd need to push that logic down to the formatter,
which would insert a leading LF and/or indentation as appropriate.

I just declared that if you use the first form you don't get
indentation :)

>   - I implemented the helper using a sequence of "read" calls. Together
>     with "-r" and unsetting the IFS, this preserves incoming whitespace.
>     An alternative is to use "cat" (which then requires the gross "."
>     trick above). But this saves us a process, which is probably a good
>     thing. The "read" builtin does use more read() syscalls than
>     necessary (one per byte), but that is almost certainly a win over a
>     separate process.
>
>     Both are probably slower than passing a single-quoted string, but
>     the difference is lost in the noise for a script that I converted as
>     an experiment.
>
>   - I handle test_expect_success and test_expect_failure here. If we
>     like this style, we could easily extend it to other spots (e.g.,
>     lazy_prereq bodies) on top of this patch.
>
>   - even though we are using "local", we have to be careful about our
>     variable names. Within test_expect_success, any variable we declare
>     with local will be seen by the test snippets themselves (so it won't
>     persist between tests like normal variables would).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

[PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

Most test snippets are wrapped in single quotes, like:

  test_expect_success 'some description' '
          do_something
  '

This sometimes makes the snippets awkward to write, because you can't
easily use single quotes within them. We sometimes work around this with
$SQ, or by loosening regexes to use "." instead of a literal quote, or
by using double quotes when we'd prefer to use single-quotes (and just
adding extra backslash-escapes to avoid interpolation).

This commit adds another option: feeding the snippet via the function's
stdin. This doesn't conflict with anything the snippet would want to do,
because we always redirect its stdin from /dev/null anyway (which we'll
continue to do).

A few notes on the implementation:

  - it would be nice to push this down into test_run_, but we can't, as
    test_expect_success and test_expect_failure want to see the actual
    script content to report it for verbose-mode. A helper function
    limits the amount of duplication in those callers here.

  - The helper function is a little awkward to call, as you feed it the
    name of the variable you want to set. The more natural thing in
    shell would be command substitution like:

      body=$(body_or_stdin "$2")

    but that loses trailing whitespace. There are tricks around this,
    like:

      body=$(body_or_stdin "$2"; printf .)
      body=${body%.}

    but we'd prefer to keep such tricks in the helper, not in each
    caller.

  - I implemented the helper using a sequence of "read" calls. Together
    with "-r" and unsetting the IFS, this preserves incoming whitespace.
    An alternative is to use "cat" (which then requires the gross "."
    trick above). But this saves us a process, which is probably a good
    thing. The "read" builtin does use more read() syscalls than
    necessary (one per byte), but that is almost certainly a win over a
    separate process.

    Both are probably slower than passing a single-quoted string, but
    the difference is lost in the noise for a script that I converted as
    an experiment.

  - I handle test_expect_success and test_expect_failure here. If we
    like this style, we could easily extend it to other spots (e.g.,
    lazy_prereq bodies) on top of this patch.

  - even though we are using "local", we have to be careful about our
    variable names. Within test_expect_success, any variable we declare
    with local will be seen as local by the test snippets themselves (so
    it wouldn't persist between tests like normal variables would).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/README                |  8 ++++++++
 t/test-lib-functions.sh | 32 +++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/t/README b/t/README
index d9e0e07506..dec644f997 100644
--- a/t/README
+++ b/t/README
@@ -906,6 +906,14 @@ see test-lib-functions.sh for the full list and their options.
 	    'git-write-tree should be able to write an empty tree.' \
 	    'tree=$(git-write-tree)'
 
+   If <script> is `-` (a single dash), then the script to run is read
+   from stdin. This lets you more easily use single quotes within the
+   script by using a here-doc. For example:
+
+	test_expect_success 'output contains expected string' - <<\EOT
+		grep "this string has 'quotes' in it" output
+	EOT
+
    If you supply three parameters the first will be taken to be a
    prerequisite; see the test_set_prereq and test_have_prereq
    documentation below:
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 427b375b39..803ed2df39 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -872,6 +872,24 @@ test_verify_prereq () {
 	BUG "'$test_prereq' does not look like a prereq"
 }
 
+# assign the variable named by "$1" with the contents of "$2";
+# if "$2" is "-", then read stdin into "$1" instead
+test_body_or_stdin () {
+	if test "$2" != "-"
+	then
+		eval "$1=\$2"
+		return
+	fi
+
+	# start with a newline, to match hanging newline from open-quote style
+	eval "$1=\$LF"
+	local test_line
+	while IFS= read -r test_line
+	do
+		eval "$1=\${$1}\${test_line}\${LF}"
+	done
+}
+
 test_expect_failure () {
 	test_start_ "$@"
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
@@ -881,9 +899,11 @@ test_expect_failure () {
 	export test_prereq
 	if ! test_skip "$@"
 	then
+		local test_body
+		test_body_or_stdin test_body "$2"
 		test -n "$test_skip_test_preamble" ||
-		say >&3 "checking known breakage of $TEST_NUMBER.$test_count '$1': $2"
-		if test_run_ "$2" expecting_failure
+		say >&3 "checking known breakage of $TEST_NUMBER.$test_count '$1': $test_body"
+		if test_run_ "$test_body" expecting_failure
 		then
 			test_known_broken_ok_ "$1"
 		else
@@ -902,13 +922,15 @@ test_expect_success () {
 	export test_prereq
 	if ! test_skip "$@"
 	then
+		local test_body
+		test_body_or_stdin test_body "$2"
 		test -n "$test_skip_test_preamble" ||
-		say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $2"
-		if test_run_ "$2"
+		say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $test_body"
+		if test_run_ "$test_body"
 		then
 			test_ok_ "$1"
 		else
-			test_failure_ "$@"
+			test_failure_ "$1" "$test_body"
 		fi
 	fi
 	test_finish_
-- 
2.45.2.1165.ga18b536d12

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Eric Sunshine]

On Mon, Jul 1, 2024 at 6:08 PM Jeff King <peff@peff.net> wrote:
> [...]
> This commit adds another option: feeding the snippet via the function's
> stdin. This doesn't conflict with anything the snippet would want to do,
> because we always redirect its stdin from /dev/null anyway (which we'll
> continue to do).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/t/README b/t/README
> @@ -906,6 +906,14 @@ see test-lib-functions.sh for the full list and their options.
> +   If <script> is `-` (a single dash), then the script to run is read
> +   from stdin. This lets you more easily use single quotes within the
> +   script by using a here-doc. For example:
> +
> +       test_expect_success 'output contains expected string' - <<\EOT
> +               grep "this string has 'quotes' in it" output
> +       EOT

We lose `chainlint` functionality for test bodies specified in this manner.

Restoring such functionality will require some (possibly)
not-so-subtle changes. There are at least a couple issues which need
to be addressed:

(1) chainlint.pl:ScriptParser::parse_cmd() only currently recognizes
`test_expect_* [prereq] 'title' 'body'` but will now also need to
recognize `test_expect_success [prereq] 'title' - <body-as-here-doc>`.

(2) Until now, chainlint.pl has never had to concern itself with the
body of a here-doc; it just throws them away. With this new calling
convention, here-doc bodies become relevant and must be returned by
the lexer. This may involve some not-so-minor surgery.

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> We lose `chainlint` functionality for test bodies specified in this manner.

Ouch.

> Restoring such functionality will require some (possibly)
> not-so-subtle changes. There are at least a couple issues which need
> to be addressed:
>
> (1) chainlint.pl:ScriptParser::parse_cmd() only currently recognizes
> `test_expect_* [prereq] 'title' 'body'` but will now also need to
> recognize `test_expect_success [prereq] 'title' - <body-as-here-doc>`.
>
> (2) Until now, chainlint.pl has never had to concern itself with the
> body of a here-doc; it just throws them away. With this new calling
> convention, here-doc bodies become relevant and must be returned by
> the lexer. This may involve some not-so-minor surgery.

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Mon, Jul 01, 2024 at 06:45:19PM -0400, Eric Sunshine wrote:

> > @@ -906,6 +906,14 @@ see test-lib-functions.sh for the full list and their options.
> > +   If <script> is `-` (a single dash), then the script to run is read
> > +   from stdin. This lets you more easily use single quotes within the
> > +   script by using a here-doc. For example:
> > +
> > +       test_expect_success 'output contains expected string' - <<\EOT
> > +               grep "this string has 'quotes' in it" output
> > +       EOT
> 
> We lose `chainlint` functionality for test bodies specified in this manner.
> 
> Restoring such functionality will require some (possibly)
> not-so-subtle changes. There are at least a couple issues which need
> to be addressed:
> 
> (1) chainlint.pl:ScriptParser::parse_cmd() only currently recognizes
> `test_expect_* [prereq] 'title' 'body'` but will now also need to
> recognize `test_expect_success [prereq] 'title' - <body-as-here-doc>`.
> 
> (2) Until now, chainlint.pl has never had to concern itself with the
> body of a here-doc; it just throws them away. With this new calling
> convention, here-doc bodies become relevant and must be returned by
> the lexer. This may involve some not-so-minor surgery.

Hmm. The patch below seems to work on a simple test.

The lexer stuffs the heredoc into a special variable. Which at first
glance feels like a hack versus returning it from the token stream, but
the contents really _aren't_ part of that stream. They're a separate
magic thing that is found on the stdin of whatever command the tokens
represent.

And then ScriptParser::parse_cmd() just has to recognize that any "<<"
token isn't interesting, and that "-" means "read the here-doc".

Obviously we'd want to add to the chainlint tests here. It looks like
the current test infrastructure is focused on evaluating snippets, with
the test_expect_success part already handled.

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 1bbd985b78..7eb904afaa 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -168,12 +168,15 @@ sub swallow_heredocs {
 	my $self = shift @_;
 	my $b = $self->{buff};
 	my $tags = $self->{heretags};
+	$self->{parser}->{heredoc} = '';
 	while (my $tag = shift @$tags) {
 		my $start = pos($$b);
 		my $indent = $$tag[0] =~ s/^\t// ? '\\s*' : '';
 		$$b =~ /(?:\G|\n)$indent\Q$$tag[0]\E(?:\n|\z)/gc;
 		if (pos($$b) > $start) {
 			my $body = substr($$b, $start, pos($$b) - $start);
+			$self->{parser}->{heredoc} .=
+				substr($body, 0, length($body) - length($&));
 			$self->{lineno} += () = $body =~ /\n/sg;
 			next;
 		}
@@ -618,6 +621,9 @@ sub check_test {
 	my $self = shift @_;
 	my ($title, $body) = map(unwrap, @_);
 	$self->{ntests}++;
+	if ($body eq '-') {
+		$body = $self->{heredoc};
+	}
 	my $parser = TestParser->new(\$body);
 	my @tokens = $parser->parse();
 	my $problems = $parser->{problems};
@@ -648,7 +654,7 @@ sub parse_cmd {
 	my @tokens = $self->SUPER::parse_cmd();
 	return @tokens unless @tokens && $tokens[0]->[0] =~ /^test_expect_(?:success|failure)$/;
 	my $n = $#tokens;
-	$n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\|)$/;
+	$n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\||<<[A-Za-z]+)$/;
 	$self->check_test($tokens[1], $tokens[2]) if $n == 2; # title body
 	$self->check_test($tokens[2], $tokens[3]) if $n > 2;  # prereq title body
 	return @tokens;

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Mon, Jul 01, 2024 at 08:51:45PM -0400, Jeff King wrote:

> Hmm. The patch below seems to work on a simple test.
> 
> The lexer stuffs the heredoc into a special variable. Which at first
> glance feels like a hack versus returning it from the token stream, but
> the contents really _aren't_ part of that stream. They're a separate
> magic thing that is found on the stdin of whatever command the tokens
> represent.
> 
> And then ScriptParser::parse_cmd() just has to recognize that any "<<"
> token isn't interesting, and that "-" means "read the here-doc".

BTW, there's one non-obvious thing here about why this works. You'd
think that:

  test_expect_success 'foo' <<\EOT
	cat <<-\EOF
	this is a here-doc
	EOF
	echo ok
  EOT

wouldn't work, because the lexer only has a single here-doc store, and
the inner one is going to overwrite the outer. But we don't lex the
inner contents of the test snippet until we've processed the
test_expect_success line, at which point we've copied it out.

So I dunno. It feels a bit hacky, but I think it's how you have to do it
anyway.

> @@ -648,7 +654,7 @@ sub parse_cmd {
>  	my @tokens = $self->SUPER::parse_cmd();
>  	return @tokens unless @tokens && $tokens[0]->[0] =~ /^test_expect_(?:success|failure)$/;
>  	my $n = $#tokens;
> -	$n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\|)$/;
> +	$n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\||<<[A-Za-z]+)$/;

One curiosity I noted is that the backslash of my "<<\EOT" seems to be
eaten by the lexer (I guess because it doesn't know the special meaning
of backslash here, and just does the usual "take the next char
literally"). I think that is OK for our purposes here, though we might
in the long run want to raise a linting error if you accidentally used
an interpolating here-doc (it's not strictly wrong to do so, but I think
we generally frown on it as a style thing).

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Mon, Jul 01, 2024 at 08:51:44PM -0400, Jeff King wrote:

> Obviously we'd want to add to the chainlint tests here. It looks like
> the current test infrastructure is focused on evaluating snippets, with
> the test_expect_success part already handled.

So doing this (with the patch I showed earlier):

diff --git a/t/Makefile b/t/Makefile
index b2eb9f770b..7c97aa3673 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -106,18 +106,28 @@ clean: clean-except-prove-cache
 clean-chainlint:
 	$(RM) -r '$(CHAINLINTTMP_SQ)'
 
+CHAINLINTTESTS_SRC = $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS))
 check-chainlint:
 	@mkdir -p '$(CHAINLINTTMP_SQ)' && \
 	for i in $(CHAINLINTTESTS); do \
 		echo "test_expect_success '$$i' '" && \
 		sed -e '/^# LINT: /d' chainlint/$$i.test && \
 		echo "'"; \
 	done >'$(CHAINLINTTMP_SQ)'/tests && \
+	for i in $$(grep -L "'" $(CHAINLINTTESTS_SRC)); do \
+		echo "test_expect_success '$$i' - <<\\\\EOT" && \
+		sed -e '/^# LINT: /d' $$i && \
+		echo "EOT"; \
+	done >>'$(CHAINLINTTMP_SQ)'/tests && \
 	{ \
 		echo "# chainlint: $(CHAINLINTTMP_SQ)/tests" && \
 		for i in $(CHAINLINTTESTS); do \
 			echo "# chainlint: $$i" && \
 			cat chainlint/$$i.expect; \
+		done && \
+		for i in $$(grep -L "'" $(CHAINLINTTESTS_SRC)); do \
+			echo "# chainlint: $$i" && \
+			cat $${i%.test}.expect; \
 		done \
 	} >'$(CHAINLINTTMP_SQ)'/expect && \
 	$(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \

does pass. It's just running all of the tests inside an "EOT" block. But
we have to omit ones that have single quotes in them, because they are
making the implicit assumption that they're inside a single-quoted block
(so they do things like '"$foo"', or '\'', etc, which behave differently
in a here-doc).

It was a nice check that the output is the same in both cases, but it's
a bit limiting as a test suite, as there's no room to introduce test
cases that vary the test_expect_success lines. I'm thinking the path
forward may be:

  1. Move the test_expect_success wrapping lines into each
     chainlint/*.test file. It's a little bit of extra boilerplate, but
     it makes them a bit easier to reason about on their own.

  2. Add a few new tests that use here-docs with a few variations
     ("<<EOT", "<<\EOT", probably a here-doc inside the test here-doc).

Does that sound OK to you?

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Eric Sunshine]

On Mon, Jul 1, 2024 at 8:51 PM Jeff King <peff@peff.net> wrote:
> On Mon, Jul 01, 2024 at 06:45:19PM -0400, Eric Sunshine wrote:
> > We lose `chainlint` functionality for test bodies specified in this manner.
>
> Hmm. The patch below seems to work on a simple test.
>
> The lexer stuffs the heredoc into a special variable. Which at first
> glance feels like a hack versus returning it from the token stream, but
> the contents really _aren't_ part of that stream. They're a separate
> magic thing that is found on the stdin of whatever command the tokens
> represent.

I created a white-room fix for this issue, as well, before taking a
look at your patch. The two implementations bear a strong similarity
which suggests that we agree upon the basic approach.

My implementation, however, takes a more formal and paranoid stance.
Rather than squirreling away only the most-recently-seen heredoc body,
it stores each heredoc body along with the tag which introduced it.
This makes it robust against cases when multiple heredocs are
initiated on the same line (even within different parse contexts):

    cat <<EOFA && x=$(cat <<EOFB &&
    A body
    EOFA
    B body
    EOFB

Of course, that's not likely to come up in the context of
test_expect_* calls, but I prefer the added robustness over the more
lax approach.

> And then ScriptParser::parse_cmd() just has to recognize that any "<<"
> token isn't interesting, and that "-" means "read the here-doc".

In my implementation, the `<<` token is "interesting" because the
heredoc tag is attached to it, and the tag is needed to pluck the
heredoc body from the set of saved bodies (since my implementation
doesn't assume most-recently-seen body is the correct one).

> Obviously we'd want to add to the chainlint tests here. It looks like
> the current test infrastructure is focused on evaluating snippets, with
> the test_expect_success part already handled.

Yes, the "snippet" approach is a throwback to the old chainlint.sed
implementation when there wasn't any actual parsing going on. As you
note, this unfortunately does not allow for testing parsing-related
aspects of the implementation, which is a limitation I most definitely
felt when chainlint.pl was implemented. It probably would be a good
idea to update the infrastructure to allow for more broad testing but
that doesn't need to be part of the changes being discussed here.

> diff --git a/t/chainlint.pl b/t/chainlint.pl
> @@ -168,12 +168,15 @@ sub swallow_heredocs {
>                 if (pos($$b) > $start) {
>                         my $body = substr($$b, $start, pos($$b) - $start);
> +                       $self->{parser}->{heredoc} .=
> +                               substr($body, 0, length($body) - length($&));
>                         $self->{lineno} += () = $body =~ /\n/sg;

In my implementation, I use regex to strip off the ending tag before
storing the heredoc body. When I later looked at your implementation,
I noticed that you used substr() -- which seems preferable -- but
discovered that it strips too much in some cases. For instance, in
t0600, I saw that:

    cat >expected <<-\EOF &&
    HEAD
    PSEUDO_WT_HEAD
    refs/bisect/wt-random
    refs/heads/main
    refs/heads/wt-main
    EOF

was getting stripped down to:

    HEAD
    PSEUDO_WT_HEAD
    refs/bisect/wt-random
    refs/heads/main
    refs/heads/wt-ma{{missing-nl}}

It wasn't immediately obvious why this was happening, though I didn't
spend a lot of time trying to debug it.

Although I think my implementation is complete, I haven't submitted it
yet because I discovered that the changes you made to t1404 are
triggering false-positives:

    # chainlint: t1404-update-ref-errors.sh
    # chainlint: existing loose ref is a simple prefix of new
    120 prefix=refs/1l &&
    121 test_update_rejected a c e false b c/x d \
    122   '$prefix/c' exists; ?!AMP?! cannot create '$prefix/c/x'

Unfortunately, I ran out of time, thus haven't tracked down this
problem yet. I also haven't tested your implementation yet to
determine if this is due to a change I made or due to a deeper
existing issue with chainlint.pl.

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Eric Sunshine]

On Mon, Jul 1, 2024 at 9:13 PM Jeff King <peff@peff.net> wrote:
> On Mon, Jul 01, 2024 at 08:51:45PM -0400, Jeff King wrote:
> > And then ScriptParser::parse_cmd() just has to recognize that any "<<"
> > token isn't interesting, and that "-" means "read the here-doc".
>
> BTW, there's one non-obvious thing here about why this works. You'd
> think that:
>
>   test_expect_success 'foo' <<\EOT
>         cat <<-\EOF
>         this is a here-doc
>         EOF
>         echo ok
>   EOT
>
> wouldn't work, because the lexer only has a single here-doc store, and
> the inner one is going to overwrite the outer. But we don't lex the
> inner contents of the test snippet until we've processed the
> test_expect_success line, at which point we've copied it out.
>
> So I dunno. It feels a bit hacky, but I think it's how you have to do it
> anyway.

It wasn't non-obvious to me, but I suppose it's because I know the
author, or I am the author, or something.

> > -     $n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\|)$/;
> > +     $n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\||<<[A-Za-z]+)$/;
>
> One curiosity I noted is that the backslash of my "<<\EOT" seems to be
> eaten by the lexer (I guess because it doesn't know the special meaning
> of backslash here, and just does the usual "take the next char
> literally").

That's not the reason. It actively strips the backslash because it
knows that it doesn't care about it after this point and, more
importantly, because it needs to extract the raw heredoc tag name
(without the slash or other surrounding quotes) so that it can match
upon that name (say, "EOF") to find the end of the heredoc body.

It's mostly an accident of implementation (and probably a throwback to
chainlint.sed) that it strips the backslash early in
Lexer::scan_heredoc_tag() even though it doesn't actually have to be
stripped until Lexer::swallow_heredocs() needs to match the tag name
to find the end of the heredoc body. Thus, in retrospect, the
implementation could have retained the backslash (`\EOF`) or quotes
(`'EOF'` or `"EOF"`) and left it for swallow_heredocs() to strip them
only when needed.

There's another weird throwback to chainlint.sed in
Lexer::scan_heredoc_tag() where it transforms `<<-` into `<<\t`, which
is potentially more than a little confusing, especially since it is (I
believe) totally unnecessary in the context of chainlint.pl.

> I think that is OK for our purposes here, though we might
> in the long run want to raise a linting error if you accidentally used
> an interpolating here-doc (it's not strictly wrong to do so, but I think
> we generally frown on it as a style thing).

Such a linting warning would probably have to be context-sensitive so
it only triggers for test_expect_* calls.

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Eric Sunshine]

On Tue, Jul 2, 2024 at 5:19 PM Jeff King <peff@peff.net> wrote:
> On Mon, Jul 01, 2024 at 08:51:44PM -0400, Jeff King wrote:
> > Obviously we'd want to add to the chainlint tests here. It looks like
> > the current test infrastructure is focused on evaluating snippets, with
> > the test_expect_success part already handled.
>
> So doing this (with the patch I showed earlier):
>
> diff --git a/t/Makefile b/t/Makefile
> @@ -106,18 +106,28 @@ clean: clean-except-prove-cache
> +       for i in $$(grep -L "'" $(CHAINLINTTESTS_SRC)); do \
> +               echo "test_expect_success '$$i' - <<\\\\EOT" && \
> +               sed -e '/^# LINT: /d' $$i && \
> +               echo "EOT"; \
> +       done >>'$(CHAINLINTTMP_SQ)'/tests && \

Unfortunately, `grep -L` is not POSIX.

> does pass. It's just running all of the tests inside an "EOT" block. But
> we have to omit ones that have single quotes in them, because they are
> making the implicit assumption that they're inside a single-quoted block
> (so they do things like '"$foo"', or '\'', etc, which behave differently
> in a here-doc).
>
> It was a nice check that the output is the same in both cases, but it's
> a bit limiting as a test suite, as there's no room to introduce test
> cases that vary the test_expect_success lines.

Agreed. It feels rather hacky and awfully special-case, as it's only
(additionally) checking that the `test_expect_* title - <<EOT` form
works, but doesn't help at all with testing other parsing-related
behaviors of chainlint.pl (which is something I definitely wanted to
be able to do when implementing the Perl version).

> I'm thinking the path forward may be:
>
>   1. Move the test_expect_success wrapping lines into each
>      chainlint/*.test file. It's a little bit of extra boilerplate, but
>      it makes them a bit easier to reason about on their own.

Yes. This is exactly what I had in mind for moving forward. It's just
a one-time noise-patch cost but gives us much more flexibility in
terms of testing.

It also makes spot-testing the chainlint self-test files much simpler.
We would be able to do this:

    ./chainlint.pl chainlint/block.test

rather than much more painful:

    { echo "test_expect_success foo '" && cat chainlint/block.test &&
echo "'"; } >dummy && ./chainlint.pl dummy; rm dummy

or something similar.

>   2. Add a few new tests that use here-docs with a few variations
>      ("<<EOT", "<<\EOT", probably a here-doc inside the test here-doc).
>
> Does that sound OK to you?

Absolutely. I'm very much in favor of these changes.

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Eric Sunshine]

On Tue, Jul 2, 2024 at 5:25 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Jul 1, 2024 at 8:51 PM Jeff King <peff@peff.net> wrote:
> >                         my $body = substr($$b, $start, pos($$b) - $start);
> > +                       $self->{parser}->{heredoc} .=
> > +                               substr($body, 0, length($body) - length($&));
> >                         $self->{lineno} += () = $body =~ /\n/sg;
>
> In my implementation, I use regex to strip off the ending tag before
> storing the heredoc body. When I later looked at your implementation,
> I noticed that you used substr() -- which seems preferable -- but
> discovered that it strips too much in some cases. [...]

Nevermind this part. I just looked again at the misbehaving code
(which I had commented out but not deleted) and noticed that I botched
the implementation in two distinct ways. With those botches removed,
the substr() approach works just fine.

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Eric Sunshine]

On Tue, Jul 2, 2024 at 5:25 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> Although I think my implementation is complete, I haven't submitted it
> yet because I discovered that the changes you made to t1404 are
> triggering false-positives:
>
>     # chainlint: t1404-update-ref-errors.sh
>     # chainlint: existing loose ref is a simple prefix of new
>     120 prefix=refs/1l &&
>     121 test_update_rejected a c e false b c/x d \
>     122   '$prefix/c' exists; ?!AMP?! cannot create '$prefix/c/x'
>
> Unfortunately, I ran out of time, thus haven't tracked down this
> problem yet.

This is also now fixed. It wasn't any deep problem, just a minor oversight.

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Tue, Jul 02, 2024 at 05:59:46PM -0400, Eric Sunshine wrote:

> > diff --git a/t/Makefile b/t/Makefile
> > @@ -106,18 +106,28 @@ clean: clean-except-prove-cache
> > +       for i in $$(grep -L "'" $(CHAINLINTTESTS_SRC)); do \
> > +               echo "test_expect_success '$$i' - <<\\\\EOT" && \
> > +               sed -e '/^# LINT: /d' $$i && \
> > +               echo "EOT"; \
> > +       done >>'$(CHAINLINTTMP_SQ)'/tests && \
> 
> Unfortunately, `grep -L` is not POSIX.

Yeah, this was just for illustration. Even if it were portable, I don't
think it's a good direction. :)

> >   1. Move the test_expect_success wrapping lines into each
> >      chainlint/*.test file. It's a little bit of extra boilerplate, but
> >      it makes them a bit easier to reason about on their own.
> 
> Yes. This is exactly what I had in mind for moving forward. It's just
> a one-time noise-patch cost but gives us much more flexibility in
> terms of testing.
> 
> It also makes spot-testing the chainlint self-test files much simpler.
> We would be able to do this:
> 
>     ./chainlint.pl chainlint/block.test
> 
> rather than much more painful:
> 
>     { echo "test_expect_success foo '" && cat chainlint/block.test &&
> echo "'"; } >dummy && ./chainlint.pl dummy; rm dummy

Oh, nice. Having just written new chainlint tests, this made checking
them _way_ easier.

> >   2. Add a few new tests that use here-docs with a few variations
> >      ("<<EOT", "<<\EOT", probably a here-doc inside the test here-doc).
> >
> > Does that sound OK to you?
> 
> Absolutely. I'm very much in favor of these changes.

Great! I have patches which I'll send out in a moment.

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Tue, Jul 02, 2024 at 05:25:48PM -0400, Eric Sunshine wrote:

> I created a white-room fix for this issue, as well, before taking a
> look at your patch. The two implementations bear a strong similarity
> which suggests that we agree upon the basic approach.
> 
> My implementation, however, takes a more formal and paranoid stance.
> Rather than squirreling away only the most-recently-seen heredoc body,
> it stores each heredoc body along with the tag which introduced it.
> This makes it robust against cases when multiple heredocs are
> initiated on the same line (even within different parse contexts):
> 
>     cat <<EOFA && x=$(cat <<EOFB &&
>     A body
>     EOFA
>     B body
>     EOFB
> 
> Of course, that's not likely to come up in the context of
> test_expect_* calls, but I prefer the added robustness over the more
> lax approach.

Yes, that's so much better than what I wrote. I didn't engage my brain
very much when I read the in-code comments about multiple tags on the
same line, and I thought you meant:

  cat <<FOO <<BAR
  this is foo
  FOO
  this is bar
  BAR

which is...weird. It does "work" in the sense that "FOO" is a here-doc
that should be skipped past. But it is not doing anything useful; cat
sees only "this is bar" on stdin. So even for this case, the appending
behavior that my patch does would not make sense.

And of course for the actual useful thing, which you wrote above,
appending is just nonsense. Recording and accessing by tag is the right
thing.

> > And then ScriptParser::parse_cmd() just has to recognize that any "<<"
> > token isn't interesting, and that "-" means "read the here-doc".
> 
> In my implementation, the `<<` token is "interesting" because the
> heredoc tag is attached to it, and the tag is needed to pluck the
> heredoc body from the set of saved bodies (since my implementation
> doesn't assume most-recently-seen body is the correct one).

Ah, OK. So it would probably not be that big of a deal to record a
single bit for "this heredoc is interpolated". But until we have
anything useful to do with that information, let's not worry about it
for now.

> > diff --git a/t/chainlint.pl b/t/chainlint.pl
> > @@ -168,12 +168,15 @@ sub swallow_heredocs {
> >                 if (pos($$b) > $start) {
> >                         my $body = substr($$b, $start, pos($$b) - $start);
> > +                       $self->{parser}->{heredoc} .=
> > +                               substr($body, 0, length($body) - length($&));
> >                         $self->{lineno} += () = $body =~ /\n/sg;
> 
> In my implementation, I use regex to strip off the ending tag before
> storing the heredoc body. When I later looked at your implementation,
> I noticed that you used substr() -- which seems preferable -- but
> discovered that it strips too much in some cases. For instance, in
> t0600, I saw that:

Yeah, I was afraid of trying another regex, just because there are
optional bits (like indentation) that we'd have to account for. Since $&
contains the match already, that's all taken care of by the existing
regex. From your follow-up, it sounds like the substr() approach does
work (*phew*).

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Sat, Jul 06, 2024 at 01:31:05AM -0400, Jeff King wrote:

> > > And then ScriptParser::parse_cmd() just has to recognize that any "<<"
> > > token isn't interesting, and that "-" means "read the here-doc".
> > 
> > In my implementation, the `<<` token is "interesting" because the
> > heredoc tag is attached to it, and the tag is needed to pluck the
> > heredoc body from the set of saved bodies (since my implementation
> > doesn't assume most-recently-seen body is the correct one).
> 
> Ah, OK. So it would probably not be that big of a deal to record a
> single bit for "this heredoc is interpolated". But until we have
> anything useful to do with that information, let's not worry about it
> for now.

Oh, oops. I attached this response to the wrong message (I read them all
through before starting to respond). My response here was about the fact
that "<<\EOT" does not record the "\" anywhere from the lexer.

But yes, for your implementation, we do need to recognize "<<\EOT", etc,
to pull out "EOT".

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Tue, Jul 02, 2024 at 05:37:39PM -0400, Eric Sunshine wrote:

> > BTW, there's one non-obvious thing here about why this works. You'd
> > think that:
> >
> >   test_expect_success 'foo' <<\EOT
> >         cat <<-\EOF
> >         this is a here-doc
> >         EOF
> >         echo ok
> >   EOT
> >
> > wouldn't work, because the lexer only has a single here-doc store, and
> > the inner one is going to overwrite the outer. But we don't lex the
> > inner contents of the test snippet until we've processed the
> > test_expect_success line, at which point we've copied it out.
> >
> > So I dunno. It feels a bit hacky, but I think it's how you have to do it
> > anyway.
> 
> It wasn't non-obvious to me, but I suppose it's because I know the
> author, or I am the author, or something.

:) I had a brief moment of panic where I thought "wait, what I sent out
is going to break in this case!" and then was surprised when it worked.

> > > -     $n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\|)$/;
> > > +     $n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\||<<[A-Za-z]+)$/;
> >
> > One curiosity I noted is that the backslash of my "<<\EOT" seems to be
> > eaten by the lexer (I guess because it doesn't know the special meaning
> > of backslash here, and just does the usual "take the next char
> > literally").
> 
> That's not the reason. It actively strips the backslash because it
> knows that it doesn't care about it after this point and, more
> importantly, because it needs to extract the raw heredoc tag name
> (without the slash or other surrounding quotes) so that it can match
> upon that name (say, "EOF") to find the end of the heredoc body.
> 
> It's mostly an accident of implementation (and probably a throwback to
> chainlint.sed) that it strips the backslash early in
> Lexer::scan_heredoc_tag() even though it doesn't actually have to be
> stripped until Lexer::swallow_heredocs() needs to match the tag name
> to find the end of the heredoc body. Thus, in retrospect, the
> implementation could have retained the backslash (`\EOF`) or quotes
> (`'EOF'` or `"EOF"`) and left it for swallow_heredocs() to strip them
> only when needed.

OK. I think it does make things easier to normalize this a bit, so that
ScriptParser::parse_cmd() doesn't have to worry about all of the various
spellings. If we recorded a single bit for "this was quoted" alongside
the heredoc contents, that would be plenty. But as I (erroneously) said
elsewhere, we can worry about that later if we find something useful to
do with it.

> There's another weird throwback to chainlint.sed in
> Lexer::scan_heredoc_tag() where it transforms `<<-` into `<<\t`, which
> is potentially more than a little confusing, especially since it is (I
> believe) totally unnecessary in the context of chainlint.pl.

Ah, I hadn't noticed that. Looks like we use it in swallow_heredocs() to
read the tag data itself. But importantly the token stream still has
the correct original in it, which we need to correctly match in
ScriptParser::parse_cmd().

> > I think that is OK for our purposes here, though we might
> > in the long run want to raise a linting error if you accidentally used
> > an interpolating here-doc (it's not strictly wrong to do so, but I think
> > we generally frown on it as a style thing).
> 
> Such a linting warning would probably have to be context-sensitive so
> it only triggers for test_expect_* calls.

Yes, definitely.

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Eric Sunshine]

On Sat, Jul 6, 2024 at 1:31 AM Jeff King <peff@peff.net> wrote:
> On Tue, Jul 02, 2024 at 05:25:48PM -0400, Eric Sunshine wrote:
> > My implementation, however, takes a more formal and paranoid stance.
> > Rather than squirreling away only the most-recently-seen heredoc body,
> > it stores each heredoc body along with the tag which introduced it.
> > This makes it robust against cases when multiple heredocs are
> > initiated on the same line (even within different parse contexts):
> >
> >     cat <<EOFA && x=$(cat <<EOFB &&
> >     A body
> >     EOFA
> >     B body
> >     EOFB
> >
> > Of course, that's not likely to come up in the context of
> > test_expect_* calls, but I prefer the added robustness over the more
> > lax approach.
>
> Yes, that's so much better than what I wrote. I didn't engage my brain
> very much when I read the in-code comments about multiple tags on the
> same line, and I thought you meant:
>
>   cat <<FOO <<BAR
>   this is foo
>   FOO
>   this is bar
>   BAR
>
> which is...weird. It does "work" in the sense that "FOO" is a here-doc
> that should be skipped past. But it is not doing anything useful; cat
> sees only "this is bar" on stdin. So even for this case, the appending
> behavior that my patch does would not make sense.
>
> And of course for the actual useful thing, which you wrote above,
> appending is just nonsense. Recording and accessing by tag is the right
> thing.

In retrospect, I think my claim is bogus in the context of
ScriptParser::parse_cmd(). Specifically, ScriptParser::parse_cmd()
calls its parent ShellParser::parse_cmd() to latch one command.
ShellParser::parse_cmd() stops parsing as soon as it encounters a
command terminator (i.e. `;`, `&&`, `||`, `|`, '&', '\n') and returns
the command. Moreover, by definition, given the language
specification, the lexer only consumes the heredocs upon encountering
`\n`. Thus, if someone writes:

    test_expect_success title - <<\EOT && whatever &&
    ...test body...
    EOT

then ScriptParser::parse_cmd() will receive the command
`test_expect_success title -` from ShellParser::parse_cmd() but the
heredoc will not yet have been consumed by the lexer since it hasn't
yet encountered the newline[1].

So, the above example simply can't work correctly given the way
ScriptParser::parse_cmd() calls ScriptParser::check_test() as soon as
it encounters a `test_expect_success/failure` invocation since it
doesn't know if the heredocs have been latched at that point. To make
it properly robust, rather than immediately calling check_test(), it
would have to continue consuming commands, and saving the ones which
match `test_expect_success/failure` invocation, until it finally hits
a `\n`, and only then call check_test() with each command it saved.
But that's probably overkill at this point considering that we never
write code like the above, so the submitted patch[2] is probably good
enough for now.

FOOTNOTES

[1] One might rightly ask that if ShellParser::parse_cmd() returns
immediately upon seeing a command terminator (i.e. `;`, `&&`, etc.),
then how is it that even a simple:

    test_expect_success title - <<\EOT &&
    ...test body...
    EOT

can work correctly since the `\n` comes after the `&&`. The answer is
that, as a special case, the very last thing ShellParser::parse_cmd()
does is peek ahead to see if a `\n` follows the command terminator
(assuming the terminator is not itself a `\n`). When the next token is
indeed a `\n`, that peek operation causes the lexer to consume the
heredocs.

[2]: https://lore.kernel.org/git/20240702235034.88219-1-ericsunshine@charter.net/

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Eric Sunshine]

On Sat, Jul 6, 2024 at 2:11 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> So, the above example simply can't work correctly given the way
> ScriptParser::parse_cmd() calls ScriptParser::check_test() as soon as
> it encounters a `test_expect_success/failure` invocation since it
> doesn't know if the heredocs have been latched at that point. To make
> it properly robust, rather than immediately calling check_test(), it
> would have to continue consuming commands, and saving the ones which
> match `test_expect_success/failure` invocation, until it finally hits
> a `\n`, and only then call check_test() with each command it saved.
> But that's probably overkill at this point considering that we never
> write code like the above, so the submitted patch[2] is probably good
> enough for now.

Of course, the more I think about it, the more I dislike relying upon
what is effectively an accident of implementation; i.e. that in the
typical case, the heredoc will already have been latched by the time
ScriptParser::parse_cmd() has identified a `test_expect_success`
command, due to the fact that ShellParser::parse_cmd() has that
special case which peeks for `\n` immediately following some other
command terminator. As such, fixing ScriptParser::parse_cmd() to only
call check_test() once it is sure that a '\n' has been encountered is
becoming more appealing, though it is of course a more invasive and
fundamental change than the posted patch.

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Sat, Jul 06, 2024 at 02:11:13AM -0400, Eric Sunshine wrote:

> > >     cat <<EOFA && x=$(cat <<EOFB &&
> > >     A body
> > >     EOFA
> > >     B body
> > >     EOFB
> [...]
> In retrospect, I think my claim is bogus in the context of
> ScriptParser::parse_cmd(). Specifically, ScriptParser::parse_cmd()
> calls its parent ShellParser::parse_cmd() to latch one command.
> ShellParser::parse_cmd() stops parsing as soon as it encounters a
> command terminator (i.e. `;`, `&&`, `||`, `|`, '&', '\n') and returns
> the command. Moreover, by definition, given the language
> specification, the lexer only consumes the heredocs upon encountering
> `\n`. Thus, if someone writes:
> 
>     test_expect_success title - <<\EOT && whatever &&
>     ...test body...
>     EOT
> 
> then ScriptParser::parse_cmd() will receive the command
> `test_expect_success title -` from ShellParser::parse_cmd() but the
> heredoc will not yet have been consumed by the lexer since it hasn't
> yet encountered the newline[1].
> 
> So, the above example simply can't work correctly given the way
> ScriptParser::parse_cmd() calls ScriptParser::check_test() as soon as
> it encounters a `test_expect_success/failure` invocation since it
> doesn't know if the heredocs have been latched at that point.

Ah, yeah, I think you're right. I had parsed your example in my mind as:

  cat <<EOFA $(cat <<EOFB)

without an intervening "&&" (taking the second here-doc as an argument
to the original command). Which _does_ work with your patch.

> To make it properly robust, rather than immediately calling
> check_test(), it would have to continue consuming commands, and saving
> the ones which match `test_expect_success/failure` invocation, until
> it finally hits a `\n`, and only then call check_test() with each
> command it saved.  But that's probably overkill at this point
> considering that we never write code like the above, so the submitted
> patch[2] is probably good enough for now.

Yep, I'd agree with all of that.

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Jeff King]

On Sat, Jul 06, 2024 at 02:47:57AM -0400, Eric Sunshine wrote:

> On Sat, Jul 6, 2024 at 2:11 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > So, the above example simply can't work correctly given the way
> > ScriptParser::parse_cmd() calls ScriptParser::check_test() as soon as
> > it encounters a `test_expect_success/failure` invocation since it
> > doesn't know if the heredocs have been latched at that point. To make
> > it properly robust, rather than immediately calling check_test(), it
> > would have to continue consuming commands, and saving the ones which
> > match `test_expect_success/failure` invocation, until it finally hits
> > a `\n`, and only then call check_test() with each command it saved.
> > But that's probably overkill at this point considering that we never
> > write code like the above, so the submitted patch[2] is probably good
> > enough for now.
> 
> Of course, the more I think about it, the more I dislike relying upon
> what is effectively an accident of implementation; i.e. that in the
> typical case, the heredoc will already have been latched by the time
> ScriptParser::parse_cmd() has identified a `test_expect_success`
> command, due to the fact that ShellParser::parse_cmd() has that
> special case which peeks for `\n` immediately following some other
> command terminator. As such, fixing ScriptParser::parse_cmd() to only
> call check_test() once it is sure that a '\n' has been encountered is
> becoming more appealing, though it is of course a more invasive and
> fundamental change than the posted patch.

Rats, I just agreed with your earlier email. ;) I am OK with the
slightly hacky version we've posted (modulo the fixes I discussed
elsewhere). But if you want to take a little time to explore the more
robust fix, I am happy to review it.

-Peff

Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

[Eric Sunshine]

On Sat, Jul 6, 2024 at 2:55 AM Jeff King <peff@peff.net> wrote:
> On Sat, Jul 06, 2024 at 02:47:57AM -0400, Eric Sunshine wrote:
> > Of course, the more I think about it, the more I dislike relying upon
> > what is effectively an accident of implementation; i.e. that in the
> > typical case, the heredoc will already have been latched by the time
> > ScriptParser::parse_cmd() has identified a `test_expect_success`
> > command, due to the fact that ShellParser::parse_cmd() has that
> > special case which peeks for `\n` immediately following some other
> > command terminator. As such, fixing ScriptParser::parse_cmd() to only
> > call check_test() once it is sure that a '\n' has been encountered is
> > becoming more appealing, though it is of course a more invasive and
> > fundamental change than the posted patch.
>
> Rats, I just agreed with your earlier email. ;) I am OK with the
> slightly hacky version we've posted (modulo the fixes I discussed
> elsewhere). But if you want to take a little time to explore the more
> robust fix, I am happy to review it.

The primary reason I said "the more I dislike relying upon ... an
accident of implementation" is that this limitation is not documented
anywhere other than in this email thread. That said, I don't mind the
posted version of the patch being picked up. The "correct" approach
can always be implemented atop it at a later time.