[PATCH 00/15] generalize chainlint self-tests

[PATCH 00/15] generalize chainlint self-tests

[Eric Sunshine]

A while back, Peff successfully nerd-sniped[1] me into tackling a
long-brewing idea I had about (possibly) improving `chainlint`
performance by linting all tests in all scripts with a single command
invocation instead of running `sed` 25300+ times (once for each test).
A description of some of the important features of the new linter can be
found at [2].

Although the new chainlint implementation has been complete for several
months, I'm still working out how to organize its patch series. In the
meantime, _this_ patch series makes it possible for the new linter to
re-use the existing chainlint self-tests. It does so by cleansing the
self-test ".test" and ".expect" files of implementation details and
limitations specific to chainlint.sed.

I'm sending this series separate from the (upcoming) patch series which
actually introduces the new chainlint because it would be too burdensome
on reviewers as a single series which, combined, would likely contain
30-35 patches; this preparatory series is already long enough at 15
patches. Although this series arose from the same work which begat
'es/test-chain-lint'[2], it is independent of that series.

This series merges cleanly with 'next' but has a conflict in t/Makefile
from 'ab/make-dependency' in 'seen'. The resolution involves replacing
`'$(CHAINLINTTMP_SQ)'` with `$(call shellquote,$(CHAINLINTTMP))`. The
resolved code should look like this:

--- >8 ---
clean-chainlint:
	$(RM) -r $(call shellquote,$(CHAINLINTTMP))

check-chainlint:
	@mkdir -p $(call shellquote,$(CHAINLINTTMP)) && \
	sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >$(call shellquote,$(CHAINLINTTMP))/tests && \
	sed -e '/^[ 	]*$$/d' $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >$(call shellquote,$(CHAINLINTTMP))/expect && \
	$(CHAINLINT) $(call shellquote,$(CHAINLINTTMP))/tests | grep -v '^[	]*$$' >$(call shellquote,$(CHAINLINTTMP))/actual && \
	diff -u $(call shellquote,$(CHAINLINTTMP))/expect $(call shellquote,$(CHAINLINTTMP))/actual
--- >8 ---

[1]: https://lore.kernel.org/git/YJzGcZpZ+E9R0gYd@coredump.intra.peff.net/
[2]: https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/

Eric Sunshine (15):
  t/chainlint/*.test: don't use invalid shell syntax
  t/chainlint/*.test: fix invalid test cases due to mixing quote types
  t/chainlint/*.test: generalize self-test commentary
  t/chainlint/one-liner: avoid overly intimate chainlint.sed knowledge
  t/Makefile: optimize chainlint self-test
  chainlint.sed: improve ?!AMP?! placement accuracy
  chainlint.sed: improve ?!SEMI?! placement accuracy
  chainlint.sed: tolerate harmless ";" at end of last line in block
  chainlint.sed: drop unnecessary distinction between ?!AMP?! and
    ?!SEMI?!
  chainlint.sed: drop subshell-closing ">" annotation
  chainlint.sed: make here-doc "<<-" operator recognition more
    POSIX-like
  chainlint.sed: don't mistake `<< word` in string as here-doc operator
  chainlint.sed: stop throwing away here-doc tags
  chainlint.sed: swallow comments consistently
  chainlint.sed: stop splitting "(..." into separate lines "(" and "..."

 t/Makefile                                    |  10 +-
 t/chainlint.sed                               | 124 +++++++++++-------
 t/chainlint/arithmetic-expansion.expect       |   6 +-
 t/chainlint/bash-array.expect                 |   4 +-
 t/chainlint/blank-line.expect                 |   2 +-
 t/chainlint/blank-line.test                   |   2 +-
 t/chainlint/block-comment.expect              |   6 +
 t/chainlint/block-comment.test                |   8 ++
 t/chainlint/block.expect                      |   4 +-
 t/chainlint/block.test                        |   3 +-
 t/chainlint/broken-chain.expect               |   4 +-
 t/chainlint/broken-chain.test                 |   2 +-
 t/chainlint/case-comment.expect               |   8 ++
 t/chainlint/case-comment.test                 |  11 ++
 t/chainlint/case.expect                       |  10 +-
 t/chainlint/case.test                         |   6 +-
 .../close-nested-and-parent-together.expect   |   5 +-
 t/chainlint/close-subshell.expect             |  16 +--
 t/chainlint/command-substitution.expect       |   6 +-
 t/chainlint/comment.expect                    |   2 +-
 t/chainlint/complex-if-in-cuddled-loop.expect |   5 +-
 t/chainlint/complex-if-in-cuddled-loop.test   |   2 +-
 t/chainlint/cuddled-if-then-else.expect       |   5 +-
 t/chainlint/cuddled-if-then-else.test         |   2 +-
 t/chainlint/cuddled-loop.expect               |   5 +-
 t/chainlint/cuddled-loop.test                 |   2 +-
 t/chainlint/cuddled.expect                    |  22 ++--
 t/chainlint/cuddled.test                      |   3 +-
 t/chainlint/exit-loop.expect                  |   6 +-
 t/chainlint/exit-subshell.expect              |   2 +-
 t/chainlint/for-loop.expect                   |   8 +-
 t/chainlint/for-loop.test                     |   8 +-
 t/chainlint/here-doc-close-subshell.expect    |   2 +-
 .../here-doc-multi-line-command-subst.expect  |   6 +-
 t/chainlint/here-doc-multi-line-string.expect |   4 +-
 t/chainlint/here-doc.expect                   |  10 +-
 t/chainlint/here-doc.test                     |   7 -
 t/chainlint/if-in-loop.expect                 |   8 +-
 t/chainlint/if-in-loop.test                   |   6 +-
 t/chainlint/if-then-else.expect               |  15 ++-
 t/chainlint/if-then-else.test                 |  17 +--
 t/chainlint/incomplete-line.expect            |   2 +-
 t/chainlint/inline-comment.expect             |   9 +-
 t/chainlint/loop-in-if.expect                 |   8 +-
 t/chainlint/loop-in-if.test                   |   6 +-
 ...ti-line-nested-command-substitution.expect |  10 +-
 t/chainlint/multi-line-string.expect          |  12 +-
 t/chainlint/multi-line-string.test            |  16 +--
 t/chainlint/negated-one-liner.expect          |   4 +-
 t/chainlint/nested-cuddled-subshell.expect    |  14 +-
 t/chainlint/nested-here-doc.expect            |   8 +-
 t/chainlint/nested-subshell-comment.expect    |   6 +-
 t/chainlint/nested-subshell-comment.test      |   2 +-
 t/chainlint/nested-subshell.expect            |   6 +-
 t/chainlint/nested-subshell.test              |   1 -
 t/chainlint/not-heredoc.expect                |  14 ++
 t/chainlint/not-heredoc.test                  |  16 +++
 t/chainlint/one-liner.expect                  |   6 +-
 t/chainlint/one-liner.test                    |   4 +-
 t/chainlint/p4-filespec.expect                |   2 +-
 t/chainlint/pipe.expect                       |   4 +-
 t/chainlint/pipe.test                         |   2 +-
 t/chainlint/semicolon.expect                  |  27 ++--
 t/chainlint/semicolon.test                    |   4 +-
 t/chainlint/subshell-here-doc.expect          |  15 +--
 t/chainlint/subshell-here-doc.test            |   8 +-
 t/chainlint/subshell-one-liner.expect         |  12 +-
 t/chainlint/t7900-subtree.expect              |  10 +-
 t/chainlint/t7900-subtree.test                |   4 +-
 t/chainlint/while-loop.expect                 |   8 +-
 t/chainlint/while-loop.test                   |   8 +-
 71 files changed, 339 insertions(+), 293 deletions(-)
 create mode 100644 t/chainlint/block-comment.expect
 create mode 100644 t/chainlint/block-comment.test
 create mode 100644 t/chainlint/case-comment.expect
 create mode 100644 t/chainlint/case-comment.test
 create mode 100644 t/chainlint/not-heredoc.expect
 create mode 100644 t/chainlint/not-heredoc.test

-- 
2.34.1.397.gfae76fe5da

[PATCH 01/15] t/chainlint/*.test: don't use invalid shell syntax

[Eric Sunshine]

The chainlint self-test code snippets are supposed to represent the body
of a test_expect_success() or test_expect_failure(), yet the contents of
these tests would have caused the shell to report syntax errors had they
been real test bodies. Although chainlint.sed, with its simplistic
heuristics, is blind to these syntactic problems, a future more robust
chainlint implementation might not have such a limitation, so make these
snippets syntactically valid.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint/if-then-else.expect    | 5 +++--
 t/chainlint/if-then-else.test      | 3 ++-
 t/chainlint/subshell-here-doc.test | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect
index 5953c7bfbc..a80f5e6c75 100644
--- a/t/chainlint/if-then-else.expect
+++ b/t/chainlint/if-then-else.expect
@@ -4,6 +4,7 @@
 ?!AMP?!		echo very
 		echo empty
 	elif test -z ""
+	then
 		echo foo
 	else
 		echo foo &&
@@ -14,6 +15,6 @@
 (
 	if test -n ""; then
 		echo very &&
-?!AMP?!		echo empty
-	if
+		echo empty
+	fi
 >)
diff --git a/t/chainlint/if-then-else.test b/t/chainlint/if-then-else.test
index 9bd8e9a4c6..d2b03ca6b4 100644
--- a/t/chainlint/if-then-else.test
+++ b/t/chainlint/if-then-else.test
@@ -7,6 +7,7 @@
 # LINT: last statement before 'elif' does not need "&&"
 		echo empty
 	elif test -z ""
+	then
 # LINT: last statement before 'else' does not need "&&"
 		echo foo
 	else
@@ -24,5 +25,5 @@
 	if test -n ""; then
 		echo very &&
 		echo empty
-	if
+	fi
 )
diff --git a/t/chainlint/subshell-here-doc.test b/t/chainlint/subshell-here-doc.test
index f6b3ba4214..0cce907ba8 100644
--- a/t/chainlint/subshell-here-doc.test
+++ b/t/chainlint/subshell-here-doc.test
@@ -11,7 +11,7 @@
 # LINT: missing "&&" on 'cat'
 	cat <<EOF >bip
 	fish fly high
-	EOF
+EOF
 
 # LINT: swallow here-doc (EOF is last line of subshell)
 	echo <<-\EOF >bop
-- 
2.34.1.397.gfae76fe5da

[PATCH 02/15] t/chainlint/*.test: fix invalid test cases due to mixing quote types

[Eric Sunshine]

The chainlint self-test code snippets are supposed to represent the body
of a test_expect_success() or test_expect_failure(), yet the contents of
a few tests would have caused the shell to report syntax errors had they
been real test bodies due to the mix of single- and double-quotes.
Although chainlint.sed, with its simplistic heuristics, is blind to this
problem, a future more robust chainlint implementation might not have
such a limitation. Therefore, stop mixing quote types haphazardly in
those tests and unify quoting throughout. While at it, drop chunks of
tests which merely repeat what is already tested elsewhere but with
alternative quotes.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint/broken-chain.test               |  2 +-
 t/chainlint/case.test                       |  6 +++---
 t/chainlint/complex-if-in-cuddled-loop.test |  2 +-
 t/chainlint/cuddled-if-then-else.test       |  2 +-
 t/chainlint/cuddled-loop.test               |  2 +-
 t/chainlint/for-loop.test                   |  8 ++++----
 t/chainlint/here-doc.expect                 |  2 --
 t/chainlint/here-doc.test                   |  7 -------
 t/chainlint/if-in-loop.test                 |  6 +++---
 t/chainlint/if-then-else.test               | 14 +++++++-------
 t/chainlint/loop-in-if.test                 |  6 +++---
 t/chainlint/multi-line-string.expect        |  8 +-------
 t/chainlint/multi-line-string.test          | 16 ++--------------
 t/chainlint/nested-subshell-comment.test    |  2 +-
 t/chainlint/pipe.test                       |  2 +-
 t/chainlint/subshell-here-doc.expect        |  1 -
 t/chainlint/subshell-here-doc.test          |  6 +-----
 t/chainlint/t7900-subtree.expect            |  4 ++--
 t/chainlint/t7900-subtree.test              |  4 ++--
 t/chainlint/while-loop.test                 |  8 ++++----
 20 files changed, 38 insertions(+), 70 deletions(-)

diff --git a/t/chainlint/broken-chain.test b/t/chainlint/broken-chain.test
index 3cc67b65d0..2a44aa73b7 100644
--- a/t/chainlint/broken-chain.test
+++ b/t/chainlint/broken-chain.test
@@ -1,6 +1,6 @@
 (
 	foo &&
-# LINT: missing "&&" from 'bar'
+# LINT: missing "&&" from "bar"
 	bar
 	baz &&
 # LINT: final statement before closing ")" legitimately lacks "&&"
diff --git a/t/chainlint/case.test b/t/chainlint/case.test
index 5ef6ff7db5..4cb086bf87 100644
--- a/t/chainlint/case.test
+++ b/t/chainlint/case.test
@@ -1,5 +1,5 @@
 (
-# LINT: "...)" arms in 'case' not misinterpreted as subshell-closing ")"
+# LINT: "...)" arms in "case" not misinterpreted as subshell-closing ")"
 	case "$x" in
 	x) foo ;;
 	*) bar ;;
@@ -7,7 +7,7 @@
 	foobar
 ) &&
 (
-# LINT: missing "&&" on 'esac'
+# LINT: missing "&&" on "esac"
 	case "$x" in
 	x) foo ;;
 	*) bar ;;
@@ -15,7 +15,7 @@
 	foobar
 ) &&
 (
-# LINT: "...)" arm in one-liner 'case' not misinterpreted as closing ")"
+# LINT: "...)" arm in one-liner "case" not misinterpreted as closing ")"
 	case "$x" in 1) true;; esac &&
 # LINT: same but missing "&&"
 	case "$y" in 2) false;; esac
diff --git a/t/chainlint/complex-if-in-cuddled-loop.test b/t/chainlint/complex-if-in-cuddled-loop.test
index 571bbd85cd..5efeda58b2 100644
--- a/t/chainlint/complex-if-in-cuddled-loop.test
+++ b/t/chainlint/complex-if-in-cuddled-loop.test
@@ -1,4 +1,4 @@
-# LINT: 'for' loop cuddled with "(" and ")" and nested 'if' with complex
+# LINT: "for" loop cuddled with "(" and ")" and nested "if" with complex
 # LINT: multi-line condition; indented with spaces, not tabs
 (for i in a b c; do
    if test "$(echo $(waffle bat))" = "eleventeen" &&
diff --git a/t/chainlint/cuddled-if-then-else.test b/t/chainlint/cuddled-if-then-else.test
index eed774a9d6..7c53f4efe3 100644
--- a/t/chainlint/cuddled-if-then-else.test
+++ b/t/chainlint/cuddled-if-then-else.test
@@ -1,4 +1,4 @@
-# LINT: 'if' cuddled with "(" and ")"; indented with spaces, not tabs
+# LINT: "if" cuddled with "(" and ")"; indented with spaces, not tabs
 (if test -z ""; then
     echo empty
  else
diff --git a/t/chainlint/cuddled-loop.test b/t/chainlint/cuddled-loop.test
index a841d781f0..3c2a62f751 100644
--- a/t/chainlint/cuddled-loop.test
+++ b/t/chainlint/cuddled-loop.test
@@ -1,4 +1,4 @@
-# LINT: 'while' loop cuddled with "(" and ")", with embedded (allowed)
+# LINT: "while" loop cuddled with "(" and ")", with embedded (allowed)
 # LINT: "|| exit {n}" to exit loop early, and using redirection "<" to feed
 # LINT: loop; indented with spaces, not tabs
 ( while read x
diff --git a/t/chainlint/for-loop.test b/t/chainlint/for-loop.test
index 7db76262bc..6cb3428158 100644
--- a/t/chainlint/for-loop.test
+++ b/t/chainlint/for-loop.test
@@ -1,17 +1,17 @@
 (
-# LINT: 'for', 'do', 'done' do not need "&&"
+# LINT: "for", "do", "done" do not need "&&"
 	for i in a b c
 	do
-# LINT: missing "&&" on 'echo'
+# LINT: missing "&&" on "echo"
 		echo $i
 # LINT: last statement of while does not need "&&"
 		cat <<-\EOF
 		bar
 		EOF
-# LINT: missing "&&" on 'done'
+# LINT: missing "&&" on "done"
 	done
 
-# LINT: 'do' on same line as 'for'
+# LINT: "do" on same line as "for"
 	for i in a b c; do
 		echo $i &&
 		cat $i
diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect
index 534b065e38..8449eb2e02 100644
--- a/t/chainlint/here-doc.expect
+++ b/t/chainlint/here-doc.expect
@@ -2,8 +2,6 @@ boodle wobba        gorgo snoot        wafta snurb &&
 
 cat >foo &&
 
-cat >bar &&
-
 cat >boo &&
 
 horticulture
diff --git a/t/chainlint/here-doc.test b/t/chainlint/here-doc.test
index ad4ce8afd9..3f5f92cad3 100644
--- a/t/chainlint/here-doc.test
+++ b/t/chainlint/here-doc.test
@@ -14,13 +14,6 @@ boz
 woz
 Arbitrary_Tag_42
 
-# LINT: swallow 'quoted' here-doc
-cat <<'FUMP' >bar &&
-snoz
-boz
-woz
-FUMP
-
 # LINT: swallow "quoted" here-doc
 cat <<"zump" >boo &&
 snoz
diff --git a/t/chainlint/if-in-loop.test b/t/chainlint/if-in-loop.test
index daf22da164..f0cf19cfad 100644
--- a/t/chainlint/if-in-loop.test
+++ b/t/chainlint/if-in-loop.test
@@ -3,13 +3,13 @@
 	do
 		if false
 		then
-# LINT: missing "&&" on 'echo'
+# LINT: missing "&&" on "echo"
 			echo "err"
 			exit 1
-# LINT: missing "&&" on 'fi'
+# LINT: missing "&&" on "fi"
 		fi
 		foo
-# LINT: missing "&&" on 'done'
+# LINT: missing "&&" on "done"
 	done
 	bar
 )
diff --git a/t/chainlint/if-then-else.test b/t/chainlint/if-then-else.test
index d2b03ca6b4..2055336c2b 100644
--- a/t/chainlint/if-then-else.test
+++ b/t/chainlint/if-then-else.test
@@ -1,27 +1,27 @@
 (
-# LINT: 'if', 'then', 'elif', 'else', 'fi' do not need "&&"
+# LINT: "if", "then", "elif", "else", "fi" do not need "&&"
 	if test -n ""
 	then
-# LINT: missing "&&" on 'echo'
+# LINT: missing "&&" on "echo"
 		echo very
-# LINT: last statement before 'elif' does not need "&&"
+# LINT: last statement before "elif" does not need "&&"
 		echo empty
 	elif test -z ""
 	then
-# LINT: last statement before 'else' does not need "&&"
+# LINT: last statement before "else" does not need "&&"
 		echo foo
 	else
 		echo foo &&
-# LINT: last statement before 'fi' does not need "&&"
+# LINT: last statement before "fi" does not need "&&"
 		cat <<-\EOF
 		bar
 		EOF
-# LINT: missing "&&" on 'fi'
+# LINT: missing "&&" on "fi"
 	fi
 	echo poodle
 ) &&
 (
-# LINT: 'then' on same line as 'if'
+# LINT: "then" on same line as "if"
 	if test -n ""; then
 		echo very &&
 		echo empty
diff --git a/t/chainlint/loop-in-if.test b/t/chainlint/loop-in-if.test
index 93e8ba8e4d..dfcc3f98fb 100644
--- a/t/chainlint/loop-in-if.test
+++ b/t/chainlint/loop-in-if.test
@@ -3,13 +3,13 @@
 	then
 		while true
 		do
-# LINT: missing "&&" on 'echo'
+# LINT: missing "&&" on "echo"
 			echo "pop"
 			echo "glup"
-# LINT: missing "&&" on 'done'
+# LINT: missing "&&" on "done"
 		done
 		foo
-# LINT: missing "&&" on 'fi'
+# LINT: missing "&&" on "fi"
 	fi
 	bar
 )
diff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect
index 170cb59993..2829516495 100644
--- a/t/chainlint/multi-line-string.expect
+++ b/t/chainlint/multi-line-string.expect
@@ -1,15 +1,9 @@
 (
 	x="line 1		line 2		line 3" &&
-?!AMP?!	y='line 1		line2'
+?!AMP?!	y="line 1		line2"
 	foobar
 >) &&
-(
-	echo "there's nothing to see here" &&
-	exit
->) &&
 (
 	echo "xyz" "abc		def		ghi" &&
-	echo 'xyz' 'abc		def		ghi' &&
-	echo 'xyz' "abc		def		ghi" &&
 	barfoo
 >)
diff --git a/t/chainlint/multi-line-string.test b/t/chainlint/multi-line-string.test
index 287ab89705..4a0af2107d 100644
--- a/t/chainlint/multi-line-string.test
+++ b/t/chainlint/multi-line-string.test
@@ -3,25 +3,13 @@
 		line 2
 		line 3" &&
 # LINT: missing "&&" on assignment
-	y='line 1
-		line2'
+	y="line 1
+		line2"
 	foobar
 ) &&
-(
-# LINT: apostrophe (in a contraction) within string not misinterpreted as
-# LINT: starting multi-line single-quoted string
-	echo "there's nothing to see here" &&
-	exit
-) &&
 (
 	echo "xyz" "abc
 		def
 		ghi" &&
-	echo 'xyz' 'abc
-		def
-		ghi' &&
-	echo 'xyz' "abc
-		def
-		ghi" &&
 	barfoo
 )
diff --git a/t/chainlint/nested-subshell-comment.test b/t/chainlint/nested-subshell-comment.test
index 0ff136ab3c..0215cdb192 100644
--- a/t/chainlint/nested-subshell-comment.test
+++ b/t/chainlint/nested-subshell-comment.test
@@ -7,7 +7,7 @@
 		# minor numbers of cows (or do they?)
 		baz &&
 		snaff
-# LINT: missing "&&" on ')'
+# LINT: missing "&&" on ")"
 	)
 	fuzzy
 )
diff --git a/t/chainlint/pipe.test b/t/chainlint/pipe.test
index e6af4de916..dd82534c66 100644
--- a/t/chainlint/pipe.test
+++ b/t/chainlint/pipe.test
@@ -4,7 +4,7 @@
 	bar |
 	baz &&
 
-# LINT: final line of pipe sequence ('cow') lacking "&&"
+# LINT: final line of pipe sequence ("cow") lacking "&&"
 	fish |
 	cow
 
diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index 74723e7340..7e057aee42 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -5,7 +5,6 @@
 >) &&
 (
 	cat >bup &&
-	cat >bup2 &&
 	cat >bup3 &&
 	meep
 >)
diff --git a/t/chainlint/subshell-here-doc.test b/t/chainlint/subshell-here-doc.test
index 0cce907ba8..d40eb65583 100644
--- a/t/chainlint/subshell-here-doc.test
+++ b/t/chainlint/subshell-here-doc.test
@@ -8,7 +8,7 @@
 	nevermore...
 	EOF
 
-# LINT: missing "&&" on 'cat'
+# LINT: missing "&&" on "cat"
 	cat <<EOF >bip
 	fish fly high
 EOF
@@ -27,10 +27,6 @@ EOF
 	glink
 	FIZZ
 	ARBITRARY
-	cat <<-'ARBITRARY2' >bup2 &&
-	glink
-	FIZZ
-	ARBITRARY2
 	cat <<-"ARBITRARY3" >bup3 &&
 	glink
 	FIZZ
diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect
index c9913429e6..f769244ef6 100644
--- a/t/chainlint/t7900-subtree.expect
+++ b/t/chainlint/t7900-subtree.expect
@@ -1,9 +1,9 @@
 (
 	chks="sub1sub2sub3sub4" &&
-	chks_sub=$(cat | sed 's,^,sub dir/,'
+	chks_sub=$(cat | sed "s,^,sub dir/,"
 >>) &&
 	chkms="main-sub1main-sub2main-sub3main-sub4" &&
-	chkms_sub=$(cat | sed 's,^,sub dir/,'
+	chkms_sub=$(cat | sed "s,^,sub dir/,"
 >>) &&
 	subfiles=$(git ls-files) &&
 	check_equal "$subfiles" "$chkms$chks"
diff --git a/t/chainlint/t7900-subtree.test b/t/chainlint/t7900-subtree.test
index 277d8358df..02f3129232 100644
--- a/t/chainlint/t7900-subtree.test
+++ b/t/chainlint/t7900-subtree.test
@@ -3,7 +3,7 @@
 sub2
 sub3
 sub4" &&
-	chks_sub=$(cat <<TXT | sed 's,^,sub dir/,'
+	chks_sub=$(cat <<TXT | sed "s,^,sub dir/,"
 $chks
 TXT
 ) &&
@@ -11,7 +11,7 @@ TXT
 main-sub2
 main-sub3
 main-sub4" &&
-	chkms_sub=$(cat <<TXT | sed 's,^,sub dir/,'
+	chkms_sub=$(cat <<TXT | sed "s,^,sub dir/,"
 $chkms
 TXT
 ) &&
diff --git a/t/chainlint/while-loop.test b/t/chainlint/while-loop.test
index f1df085bf0..d09fb016e4 100644
--- a/t/chainlint/while-loop.test
+++ b/t/chainlint/while-loop.test
@@ -1,17 +1,17 @@
 (
-# LINT: 'while, 'do', 'done' do not need "&&"
+# LINT: "while", "do", "done" do not need "&&"
 	while true
 	do
-# LINT: missing "&&" on 'echo'
+# LINT: missing "&&" on "echo"
 		echo foo
 # LINT: last statement of while does not need "&&"
 		cat <<-\EOF
 		bar
 		EOF
-# LINT: missing "&&" on 'done'
+# LINT: missing "&&" on "done"
 	done
 
-# LINT: 'do' on same line as 'while'
+# LINT: "do" on same line as "while"
 	while true; do
 		echo foo &&
 		cat bar
-- 
2.34.1.397.gfae76fe5da

[PATCH 03/15] t/chainlint/*.test: generalize self-test commentary

[Eric Sunshine]

The purpose of chainlint.sed is to detect &&-chain breakage only within
subshells (one level deep); it doesn't bother checking for top-level
&&-chain breakage since the &&-chain checker built into t/test-lib.sh
should detect broken &&-chains outside of subshells by making them
magically exit with code 117. However, this division of labor may not
always be the case if a more capable chainlint implementation is ever
developed. Beyond that, due to being sed-based and due to its use of
heuristics, chainlint.sed has several limitations (such as being unable
to detect &&-chain breakage in subshells more than one level deep since
it only manually emulates recursion into a subshell).

Some of the comments in the chainlint self-tests unnecessarily reflect
the limitations of chainlint.sed even though those limitations are not
what is being tested. Therefore, simplify and generalize the comments to
explain only what is being tested, thus ensuring that they won't become
outdated if a more capable chainlint is ever developed.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint/blank-line.test      | 2 +-
 t/chainlint/block.test           | 3 +--
 t/chainlint/cuddled.test         | 3 +--
 t/chainlint/nested-subshell.test | 1 -
 t/chainlint/one-liner.test       | 2 +-
 t/chainlint/semicolon.test       | 4 ++--
 6 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/t/chainlint/blank-line.test b/t/chainlint/blank-line.test
index f6dd14302b..0fdf15b3e1 100644
--- a/t/chainlint/blank-line.test
+++ b/t/chainlint/blank-line.test
@@ -3,7 +3,7 @@
 	nothing &&
 
 	something
-# LINT: swallow blank lines since final _statement_ before subshell end is
+# LINT: ignore blank lines since final _statement_ before subshell end is
 # LINT: significant to "&&"-check, not final _line_ (which might be blank)
 
 
diff --git a/t/chainlint/block.test b/t/chainlint/block.test
index d859151af1..0a82fd579f 100644
--- a/t/chainlint/block.test
+++ b/t/chainlint/block.test
@@ -1,6 +1,5 @@
 (
-# LINT: missing "&&" in block not currently detected (for consistency with
-# LINT: --chain-lint at top level and to provide escape hatch if needed)
+# LINT: missing "&&" after first "echo"
 	foo &&
 	{
 		echo a
diff --git a/t/chainlint/cuddled.test b/t/chainlint/cuddled.test
index 0499fa4180..257b5b5eed 100644
--- a/t/chainlint/cuddled.test
+++ b/t/chainlint/cuddled.test
@@ -1,5 +1,4 @@
-# LINT: first subshell statement cuddled with opening "("; for implementation
-# LINT: simplicity, "(..." is split into two lines, "(" and "..."
+# LINT: first subshell statement cuddled with opening "("
 (cd foo &&
 	bar
 ) &&
diff --git a/t/chainlint/nested-subshell.test b/t/chainlint/nested-subshell.test
index 998b05a47d..440ee9992d 100644
--- a/t/chainlint/nested-subshell.test
+++ b/t/chainlint/nested-subshell.test
@@ -7,7 +7,6 @@
 
 	cd foo &&
 	(
-# LINT: nested multi-line subshell not presently checked for missing "&&"
 		echo a
 		echo b
 	) >file
diff --git a/t/chainlint/one-liner.test b/t/chainlint/one-liner.test
index ec9acb9825..69796d7505 100644
--- a/t/chainlint/one-liner.test
+++ b/t/chainlint/one-liner.test
@@ -3,7 +3,7 @@
 (foo && bar) |
 (foo && bar) >baz &&
 
-# LINT: top-level one-liner subshell missing internal "&&"
+# LINT: top-level one-liner subshell missing internal "&&" and broken &&-chain
 (foo; bar) &&
 (foo; bar) |
 (foo; bar) >baz
diff --git a/t/chainlint/semicolon.test b/t/chainlint/semicolon.test
index d82c8ebbc0..67e1192c50 100644
--- a/t/chainlint/semicolon.test
+++ b/t/chainlint/semicolon.test
@@ -15,11 +15,11 @@
 	cat foo; echo bar
 ) &&
 (
-# LINT: unnecessary terminating semicolon
+# LINT: semicolon unnecessary but legitimate
 	foo;
 ) &&
 (cd foo &&
 	for i in a b c; do
-# LINT: unnecessary terminating semicolon
+# LINT: semicolon unnecessary but legitimate
 		echo;
 	done)
-- 
2.34.1.397.gfae76fe5da

[PATCH 04/15] t/chainlint/one-liner: avoid overly intimate chainlint.sed knowledge

[Eric Sunshine]

The purpose of chainlint.sed is to detect &&-chain breakage only within
subshells (one level deep); it doesn't bother checking for top-level
&&-chain breakage since the &&-chain checker built into t/test-lib.sh
should detect broken &&-chains outside of subshells by making them
magically exit with code 117.

Unfortunately, one of the chainlint.sed self-tests has overly intimate
knowledge of this particular division of responsibilities and only cares
about what chainlint.sed itself will produce, while ignoring the fact
that a more all-encompassing linter would complain about a broken
&&-chain outside the subshell. This makes it difficult to re-use the
test with a more capable chainlint implementation should one ever be
developed. Therefore, adjust the test and its "expected" output to
avoid being specific to the tunnel-vision of this one implementation.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint/one-liner.expect | 2 +-
 t/chainlint/one-liner.test   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect
index 237f227349..c64058f7af 100644
--- a/t/chainlint/one-liner.expect
+++ b/t/chainlint/one-liner.expect
@@ -4,6 +4,6 @@
 
 ?!SEMI?!(foo; bar) &&
 ?!SEMI?!(foo; bar) |
-?!SEMI?!(foo; bar) >baz
+?!SEMI?!(foo; bar) >baz &&
 
 (foo "bar; baz")
diff --git a/t/chainlint/one-liner.test b/t/chainlint/one-liner.test
index 69796d7505..be9858fa29 100644
--- a/t/chainlint/one-liner.test
+++ b/t/chainlint/one-liner.test
@@ -6,7 +6,7 @@
 # LINT: top-level one-liner subshell missing internal "&&" and broken &&-chain
 (foo; bar) &&
 (foo; bar) |
-(foo; bar) >baz
+(foo; bar) >baz &&
 
 # LINT: ";" in string not misinterpreted as broken &&-chain
 (foo "bar; baz")
-- 
2.34.1.397.gfae76fe5da

[PATCH 05/15] t/Makefile: optimize chainlint self-test

[Eric Sunshine]

Rather than running `chainlint` and `diff` once per self-test -- which
may become expensive as more tests are added -- instead run `chainlint`
a single time over all tests bodies collectively and compare the result
to the collective "expected" output.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/Makefile | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 882d26eee3..f4ae40be46 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -71,12 +71,10 @@ clean-chainlint:
 
 check-chainlint:
 	@mkdir -p '$(CHAINLINTTMP_SQ)' && \
-	err=0 && \
-	for i in $(CHAINLINTTESTS); do \
-		$(CHAINLINT) <chainlint/$$i.test | \
-		sed -e '/^# LINT: /d' >'$(CHAINLINTTMP_SQ)'/$$i.actual && \
-		diff -u chainlint/$$i.expect '$(CHAINLINTTMP_SQ)'/$$i.actual || err=1; \
-	done && exit $$err
+	sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
+	cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
+	$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \
+	diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
 
 test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
 	test-lint-filenames
-- 
2.34.1.397.gfae76fe5da

[RFC PATCH] t/Makefile: use dependency graph for "check-chainlint"

[Ævar Arnfjörð Bjarmason]

This gets rid of this fixed cost when running "make T=<name>":

$ git hyperfine -L rev HEAD~1,HEAD~0 -s 'make -C t check-chainlint' 'make -C t check-chainlint'
Benchmark 1: make -C t check-chainlint' in 'HEAD~1
  Time (mean ± σ):     111.3 ms ±   0.5 ms    [User: 116.9 ms, System: 26.2 ms]
  Range (min … max):   110.5 ms … 112.5 ms    26 runs

Benchmark 2: make -C t check-chainlint' in 'HEAD~0
  Time (mean ± σ):      12.5 ms ±   0.2 ms    [User: 11.5 ms, System: 1.0 ms]
  Range (min … max):    12.1 ms …  13.2 ms    223 runs

Summary
  'make -C t check-chainlint' in 'HEAD~0' ran
    8.92 ± 0.16 times faster than 'make -C t check-chainlint' in 'HEAD~1'
---
On Mon, Dec 13 2021, Eric Sunshine wrote:

> Rather than running `chainlint` and `diff` once per self-test -- which
> may become expensive as more tests are added -- instead run `chainlint`
> a single time over all tests bodies collectively and compare the result
> to the collective "expected" output.

I think that "optimizing" things like this is an anti-pattern. I.e. we
have N chainlint test files, and N potential outputs from that (ok or
not, and with/without error). If one of the chainlint tests changes
we'd like to re-run it, if not we can re-use an earlier run.

This is something make's dependency logic is perfectly suited for, and
will be faster than any optimization of turning a for-loop into a
"sed" command we run every time, since we'll only need to "stat" a few
things to see that there's nothing to do.

I've had the below as part of my local build for a while. It
conflicts/would be improved by my in-flight ab/make-dependency, which
you also ran into conflicts with.

This also has the advantage that you'll see what specific test failed
from the Makefile output. Aside from the ".build" rule needing to be
fixed up to use the "mkdir" trick in ab/make-dependency it should also
use $(QUIET_GEN) or whatever.

I'm not quite happy with the below, the wildcard/patsubst is a bit
messy, and it doesn't properly work around "make test" and how it runs
"clean" after its run, but you'll find that "make chainlint" will
properly cache things with this.

 t/.gitignore |  1 +
 t/Makefile   | 29 +++++++++++++++--------------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/t/.gitignore b/t/.gitignore
index 91cf5772fe5..f142d6d42fd 100644
--- a/t/.gitignore
+++ b/t/.gitignore
@@ -1,3 +1,4 @@
+/.build/
 /trash directory*
 /test-results
 /.prove
diff --git a/t/Makefile b/t/Makefile
index 882d26eee30..9bac0e683c9 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -18,10 +18,8 @@ TEST_LINT ?= test-lint
 
 ifdef TEST_OUTPUT_DIRECTORY
 TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
-CHAINLINTTMP = $(TEST_OUTPUT_DIRECTORY)/chainlinttmp
 else
 TEST_RESULTS_DIRECTORY = test-results
-CHAINLINTTMP = chainlinttmp
 endif
 
 # Shell quote;
@@ -29,13 +27,12 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
-CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP))
 
 T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
 TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
 THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
 TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
-CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
+CHAINLINTTESTS = $(wildcard chainlint/*.test)
 CHAINLINT = sed -f chainlint.sed
 
 all: $(DEFAULT_TEST_TARGET)
@@ -67,16 +64,20 @@ clean: clean-except-prove-cache
 	$(RM) .prove
 
 clean-chainlint:
-	$(RM) -r '$(CHAINLINTTMP_SQ)'
-
-check-chainlint:
-	@mkdir -p '$(CHAINLINTTMP_SQ)' && \
-	err=0 && \
-	for i in $(CHAINLINTTESTS); do \
-		$(CHAINLINT) <chainlint/$$i.test | \
-		sed -e '/^# LINT: /d' >'$(CHAINLINTTMP_SQ)'/$$i.actual && \
-		diff -u chainlint/$$i.expect '$(CHAINLINTTMP_SQ)'/$$i.actual || err=1; \
-	done && exit $$err
+	$(RM) -r .build/chainlint
+
+BUILT_CHAINLINTTESTS = $(patsubst %,.build/%.actual,$(CHAINLINTTESTS))
+
+.build/chainlint:
+	mkdir -p $@
+
+$(BUILT_CHAINLINTTESTS): | .build/chainlint
+$(BUILT_CHAINLINTTESTS): .build/%.actual: %
+	$(CHAINLINT) <$< | \
+	sed -e '/^# LINT: /d' >$@ && \
+	diff -u $(basename $<).expect $@
+
+check-chainlint: $(BUILT_CHAINLINTTESTS)
 
 test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
 	test-lint-filenames
-- 
2.34.1.1025.g9a0c3a30920

Re: [RFC PATCH] t/Makefile: use dependency graph for "check-chainlint"

[Eric Sunshine]

On Mon, Dec 13, 2021 at 5:09 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Dec 13 2021, Eric Sunshine wrote:
> > Rather than running `chainlint` and `diff` once per self-test -- which
> > may become expensive as more tests are added -- instead run `chainlint`
> > a single time over all tests bodies collectively and compare the result
> > to the collective "expected" output.
>
> I think that "optimizing" things like this is an anti-pattern. I.e. we
> have N chainlint test files, and N potential outputs from that (ok or
> not, and with/without error). If one of the chainlint tests changes
> we'd like to re-run it, if not we can re-use an earlier run.

As mentioned in a reply elsewhere, the commit message sells this as an
optimization, but that's not the real reason for the change, which is
that the rewritten `check-chainlint` target for the upcoming new
chainlint really wants to have a composite file of the "test" input
and a composite of the "expect" output. I didn't know how to sell that
change in this preparatory patch series, so I weakly framed it as an
optimization. The reason for making this a preparatory step is that it
makes for a less noisy patch later on when the new chainlint is
actually plugged into the `check-chainlint` target. At least, it was
less noisy originally... in the final implementation, I think it ends
up being noisy anyhow. So, maybe it makes sense to drop this patch
altogether(?).

> This is something make's dependency logic is perfectly suited for, and
> will be faster than any optimization of turning a for-loop into a
> "sed" command we run every time, since we'll only need to "stat" a few
> things to see that there's nothing to do.
>
> +BUILT_CHAINLINTTESTS = $(patsubst %,.build/%.actual,$(CHAINLINTTESTS))
> +
> +.build/chainlint:
> +       mkdir -p $@
> +
> +$(BUILT_CHAINLINTTESTS): | .build/chainlint
> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %
> +       $(CHAINLINT) <$< | \
> +       sed -e '/^# LINT: /d' >$@ && \
> +       diff -u $(basename $<).expect $@
> +
> +check-chainlint: $(BUILT_CHAINLINTTESTS)

This sort of optimization makes sense (I think) even with the new
chainlint preferring to see composite "test" and "expect" files rather
than the individual files. The individual files would be prerequisites
of the composite files, thus the composites would only be regenerated
if the individual files change. And the composite files would be
prerequisites of the `check-chainlint` target, so it would only run if
the composite files change (or if chainlint itself changes).

In fact, with the new chainlint checking all tests in all scripts at
once, this technique should apply nicely to it, as well, since the
names of test scripts (t????-*.sh) are fed to it as command-line
arguments. Thus, the t????-*.sh files could be prerequisites of the
chainlint rule which would use $? to check only test scripts which
have changed since the previous run.

Having said all that, I don't think think the changes in this series
or the upcoming new chainlint series make the situation any worse
(Makefile-wise) than its current state, and the sort of optimizations
discussed here can easily be made after those series land. (And, as my
Git time is rather limited these days, I'd really like to focus on
getting the core components landed.)

Re: [RFC PATCH] t/Makefile: use dependency graph for "check-chainlint"

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 14 2021, Eric Sunshine wrote:

> On Mon, Dec 13, 2021 at 5:09 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Mon, Dec 13 2021, Eric Sunshine wrote:
>> > Rather than running `chainlint` and `diff` once per self-test -- which
>> > may become expensive as more tests are added -- instead run `chainlint`
>> > a single time over all tests bodies collectively and compare the result
>> > to the collective "expected" output.
>>
>> I think that "optimizing" things like this is an anti-pattern. I.e. we
>> have N chainlint test files, and N potential outputs from that (ok or
>> not, and with/without error). If one of the chainlint tests changes
>> we'd like to re-run it, if not we can re-use an earlier run.
>
> As mentioned in a reply elsewhere, the commit message sells this as an
> optimization, but that's not the real reason for the change, which is
> that the rewritten `check-chainlint` target for the upcoming new
> chainlint really wants to have a composite file of the "test" input
> and a composite of the "expect" output. I didn't know how to sell that
> change in this preparatory patch series, so I weakly framed it as an
> optimization. The reason for making this a preparatory step is that it
> makes for a less noisy patch later on when the new chainlint is
> actually plugged into the `check-chainlint` target. At least, it was
> less noisy originally... in the final implementation, I think it ends
> up being noisy anyhow. So, maybe it makes sense to drop this patch
> altogether(?).

I think you should do whatever you think makes sense here, I was just
pointing out that alternate Makefile approach in case it was helpful.

>> This is something make's dependency logic is perfectly suited for, and
>> will be faster than any optimization of turning a for-loop into a
>> "sed" command we run every time, since we'll only need to "stat" a few
>> things to see that there's nothing to do.
>>
>> +BUILT_CHAINLINTTESTS = $(patsubst %,.build/%.actual,$(CHAINLINTTESTS))
>> +
>> +.build/chainlint:
>> +       mkdir -p $@
>> +
>> +$(BUILT_CHAINLINTTESTS): | .build/chainlint
>> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %
>> +       $(CHAINLINT) <$< | \
>> +       sed -e '/^# LINT: /d' >$@ && \
>> +       diff -u $(basename $<).expect $@
>> +
>> +check-chainlint: $(BUILT_CHAINLINTTESTS)
>
> This sort of optimization makes sense (I think) even with the new
> chainlint preferring to see composite "test" and "expect" files rather
> than the individual files. The individual files would be prerequisites
> of the composite files, thus the composites would only be regenerated
> if the individual files change. And the composite files would be
> prerequisites of the `check-chainlint` target, so it would only run if
> the composite files change (or if chainlint itself changes).
>
> In fact, with the new chainlint checking all tests in all scripts at
> once, this technique should apply nicely to it, as well, since the
> names of test scripts (t????-*.sh) are fed to it as command-line
> arguments. Thus, the t????-*.sh files could be prerequisites of the
> chainlint rule which would use $? to check only test scripts which
> have changed since the previous run.

*nod*

> Having said all that, I don't think think the changes in this series
> or the upcoming new chainlint series make the situation any worse
> (Makefile-wise) than its current state, and the sort of optimizations
> discussed here can easily be made after those series land. (And, as my
> Git time is rather limited these days, I'd really like to focus on
> getting the core components landed.)

Sounds good to me. We have plenty of these "doesn't have deps" targets
(e.g. the check shell syntax one you noted), we can just fix/change them
some other time.

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Fabian Stelzer]

On 13.12.2021 01:30, Eric Sunshine wrote:
>Rather than running `chainlint` and `diff` once per self-test -- which
>may become expensive as more tests are added -- instead run `chainlint`
>a single time over all tests bodies collectively and compare the result
>to the collective "expected" output.
>
>Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>---
> t/Makefile | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
>diff --git a/t/Makefile b/t/Makefile
>index 882d26eee3..f4ae40be46 100644
>--- a/t/Makefile
>+++ b/t/Makefile
>@@ -71,12 +71,10 @@ clean-chainlint:
>
> check-chainlint:
> 	@mkdir -p '$(CHAINLINTTMP_SQ)' && \
>-	err=0 && \
>-	for i in $(CHAINLINTTESTS); do \
>-		$(CHAINLINT) <chainlint/$$i.test | \
>-		sed -e '/^# LINT: /d' >'$(CHAINLINTTMP_SQ)'/$$i.actual && \
>-		diff -u chainlint/$$i.expect '$(CHAINLINTTMP_SQ)'/$$i.actual || err=1; \
>-	done && exit $$err
>+	sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
>+	cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
>+	$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \
>+	diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
>

If I read this right you are relying on the order of the .test & .expect 
files to match. I did something similar in a test prereq which resulted in a 
bug when setting the test_output_dir to something residing in /dev/shm, 
since the order of files in /dev/shm is reversed (at least on some 
platforms). Even though this should work as is I could see this leading to a 
similar bug in the future.

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Eric Sunshine]

On Mon, Dec 13, 2021 at 5:22 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> On 13.12.2021 01:30, Eric Sunshine wrote:
> > check-chainlint:
> >+      sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
> >+      cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
> >+      $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \
> >+      diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
>
> If I read this right you are relying on the order of the .test & .expect
> files to match. I did something similar in a test prereq which resulted in a
> bug when setting the test_output_dir to something residing in /dev/shm,
> since the order of files in /dev/shm is reversed (at least on some
> platforms). Even though this should work as is I could see this leading to a
> similar bug in the future.

It's not seen in the patch context, but earlier in the file we have:

    CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))

which provides stability via `sort`, thus ensures that the order of
the ".test" and ".expect" match.

I think that addresses your concern (unless I misunderstand your observation).

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Fabian Stelzer]

On 13.12.2021 09:27, Eric Sunshine wrote:
>On Mon, Dec 13, 2021 at 5:22 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> On 13.12.2021 01:30, Eric Sunshine wrote:
>> > check-chainlint:
>> >+      sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
>> >+      cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
>> >+      $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \
>> >+      diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
>>
>> If I read this right you are relying on the order of the .test & .expect
>> files to match. I did something similar in a test prereq which resulted in a
>> bug when setting the test_output_dir to something residing in /dev/shm,
>> since the order of files in /dev/shm is reversed (at least on some
>> platforms). Even though this should work as is I could see this leading to a
>> similar bug in the future.
>
>It's not seen in the patch context, but earlier in the file we have:
>
>    CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))
>
>which provides stability via `sort`, thus ensures that the order of
>the ".test" and ".expect" match.
>
>I think that addresses your concern (unless I misunderstand your observation).

Yes, thats what i meant. I didn't realize $CHAINLINTTESTS is already the 
sorted glob. Thanks for clarifying.

Personally i find the initial for loop variant to be the most readable.  
Ævars makefile targets could be very nice too, but especially:

+$(BUILT_CHAINLINTTESTS): | .build/chainlint
+$(BUILT_CHAINLINTTESTS): .build/%.actual: %
+       $(CHAINLINT) <$< | \
+	 sed -e '/^# LINT: /d' >$@ && \
+       diff -u $(basename $<).expect $@

i find very hard to grasp :/
I have no idea what is going on here: `<$< |` ?

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Eric Sunshine]

On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> Personally i find the initial for loop variant to be the most readable.
> Ævars makefile targets could be very nice too, but especially:
>
> +$(BUILT_CHAINLINTTESTS): | .build/chainlint
> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %
> +       $(CHAINLINT) <$< | \
> +        sed -e '/^# LINT: /d' >$@ && \
> +       diff -u $(basename $<).expect $@
>
> i find very hard to grasp :/
> I have no idea what is going on here: `<$< |` ?

Ya, that line-noise is an unfortunate combination of shell and
Makefile gobbledygook. The `$<` is effectively the source file (the
file being passed into chainlint.sed), and the rest of it is just
normal shell. `<` is redirection (using the source file `$<` as
stdin), and `|` is the pipe operator (sending the output of
chainlint.sed to another `sed`), and capturing it all via shell `>`
redirection in `$@` which is the Makefile variable for the target
file.

Anyhow, although the commit message tries to sell this change as some
sort of optimization, it's really in preparation for the new chainlint
which wants to check all tests in all files with a single invocation
rather than being invoked over and over and over. The self-test files
also require more preprocessing to work with the new chainlint, so the
implementation of `check-chainlint` gets rather more complex once the
end state is reached. I'll think about it a bit, but at the moment,
I'm still leaning toward this intermediate step as being beneficial to
reaching the end state. However, my opinion could change since the way
this is done here was probably influenced by an earlier iteration of
the new chainlint, but now that the implementation of the new
chainlint is concrete, it may not be especially important to do it
this way.

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 13 2021, Eric Sunshine wrote:

> On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> Personally i find the initial for loop variant to be the most readable.
>> Ævars makefile targets could be very nice too, but especially:
>>
>> +$(BUILT_CHAINLINTTESTS): | .build/chainlint
>> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %
>> +       $(CHAINLINT) <$< | \
>> +        sed -e '/^# LINT: /d' >$@ && \
>> +       diff -u $(basename $<).expect $@
>>
>> i find very hard to grasp :/
>> I have no idea what is going on here: `<$< |` ?
>
> Ya, that line-noise is an unfortunate combination of shell and
> Makefile gobbledygook. The `$<` is effectively the source file (the
> file being passed into chainlint.sed), and the rest of it is just
> normal shell. `<` is redirection (using the source file `$<` as
> stdin), and `|` is the pipe operator (sending the output of
> chainlint.sed to another `sed`), and capturing it all via shell `>`
> redirection in `$@` which is the Makefile variable for the target
> file.

To add to that;
https://www.gnu.org/software/make/manual/html_node/Rules.html#Rules and
other relevant parts of the GNU make manual are very helpful here.

> Anyhow, although the commit message tries to sell this change as some
> sort of optimization, it's really in preparation for the new chainlint
> which wants to check all tests in all files with a single invocation
> rather than being invoked over and over and over. The self-test files
> also require more preprocessing to work with the new chainlint, so the
> implementation of `check-chainlint` gets rather more complex once the
> end state is reached. I'll think about it a bit, but at the moment,
> I'm still leaning toward this intermediate step as being beneficial to
> reaching the end state. However, my opinion could change since the way
> this is done here was probably influenced by an earlier iteration of
> the new chainlint, but now that the implementation of the new
> chainlint is concrete, it may not be especially important to do it
> this way.

I don't really care about the details of whether it's invoked once or N
times, although I think the N times with proper dependencies tends to
give you better error messages, but maybe you'll be changing it
significantly enough that the current map between chainlint files and
approximately what sort of thing they check won't be there anymore.

In any case, I'd think that a rule that used $< now (i.e. 1=1 file->out
prereq) would be better for the current state, and could just be changed
to use one of $^ or $+ later.

I.e. you can declare a "check.done" that depends on {1..10}.todo, and
get a list of all of those {1..10}.todo files if one changes, or just
the ones whose mtime is newer than a "check.done".

The reason I looked at this to begin with is that it takes it ~100-150ms
to run now, which adds up if you're e.g. using "make test T=<glob>" in
"git rebase -i --exec".

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Eric Sunshine]

On Mon, Dec 13, 2021 at 11:17 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Dec 13 2021, Eric Sunshine wrote:
> > On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> >> I have no idea what is going on here: `<$< |` ?
> >
> > Ya, that line-noise is an unfortunate combination of shell and
> > Makefile gobbledygook. The `$<` is effectively the source file (the
> > file being passed into chainlint.sed), and the rest of it is just
> > normal shell. `<` is redirection (using the source file `$<` as
> > stdin), and `|` is the pipe operator (sending the output of
> > chainlint.sed to another `sed`), and capturing it all via shell `>`
> > redirection in `$@` which is the Makefile variable for the target
> > file.
>
> To add to that;
> https://www.gnu.org/software/make/manual/html_node/Rules.html#Rules and
> other relevant parts of the GNU make manual are very helpful here.

And the Makefile variables $< and $@, in particular, are documented here:
https://www.gnu.org/software/make/manual/html_node/Automatic-Variables.html

> I don't really care about the details of whether it's invoked once or N
> times, although I think the N times with proper dependencies tends to
> give you better error messages, but maybe you'll be changing it
> significantly enough that the current map between chainlint files and
> approximately what sort of thing they check won't be there anymore.
>
> In any case, I'd think that a rule that used $< now (i.e. 1=1 file->out
> prereq) would be better for the current state, and could just be changed
> to use one of $^ or $+ later.
>
> I.e. you can declare a "check.done" that depends on {1..10}.todo, and
> get a list of all of those {1..10}.todo files if one changes, or just
> the ones whose mtime is newer than a "check.done".
>
> The reason I looked at this to begin with is that it takes it ~100-150ms
> to run now, which adds up if you're e.g. using "make test T=<glob>" in
> "git rebase -i --exec".

Regarding this last point, one idea I strongly considered (and have
not rejected) is to stop making `check-chainlin` a dependency of
`test` and `prove`. Unlike most of the test suite, in which a change
to any part of the Git source code could potentially cause any test to
fail -- thus, it is important to run the full test suite for any
source code change -- the `check-chainlint` target is completely
isolated from everything else; it only checks whether `chainlint`
itself functions correctly. The only time it really makes sense to run
`check-chainlint` is when chainlint itself is changed in order to
verify that it still functions as expected. Considering how
infrequently (i.e. never) chainlint is modified, it seems somewhat
silly for every `make test` or `make prove` invoked by anybody
anywhere to repeatedly and forever validate chainlint[*]. Instead, it
could be the responsibility of the person modifying chainlint to run
the `check-chainlint` self-tests.

[*]: There is at least one exception. Various implementations of `sed`
could behave differently, thus impacting the behavior of
chainlint.sed. This is not just a theoretical concern. I did all the
development of this series on macOS, where everything worked as
intended. Shortly before sending the series to the list, I subjected
it to other platforms via CI and found that it failed on Linux due to
minor behavioral differences in `sed` on Linux (though, very oddly, it
worked just fine on Windows). I might not have caught this problem if
`check-chainlint` had not been run automatically by `make test`.

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Eric Sunshine]

On Mon, Dec 13, 2021 at 12:05 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Dec 13, 2021 at 11:17 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > The reason I looked at this to begin with is that it takes it ~100-150ms
> > to run now, which adds up if you're e.g. using "make test T=<glob>" in
> > "git rebase -i --exec".
>
> Regarding this last point, one idea I strongly considered (and have
> not rejected) is to stop making `check-chainlin` a dependency of
> `test` and `prove`. [...]

Another less sledge-hammer approach would be to make t/Makefile
respect GIT_TEST_CHAIN_LINT so that it doesn't run `check-chainlint`
for `test` and `prove` when that variable is `0`. That would allow
your `git rebase -i --exec` case to avoid the wasted extra overhead of
`check-chainlint` (and chainlint in general).

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 13 2021, Eric Sunshine wrote:

> On Mon, Dec 13, 2021 at 12:05 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Dec 13, 2021 at 11:17 AM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>> > The reason I looked at this to begin with is that it takes it ~100-150ms
>> > to run now, which adds up if you're e.g. using "make test T=<glob>" in
>> > "git rebase -i --exec".
>>
>> Regarding this last point, one idea I strongly considered (and have
>> not rejected) is to stop making `check-chainlin` a dependency of
>> `test` and `prove`. [...]
>
> Another less sledge-hammer approach would be to make t/Makefile
> respect GIT_TEST_CHAIN_LINT so that it doesn't run `check-chainlint`
> for `test` and `prove` when that variable is `0`. That would allow
> your `git rebase -i --exec` case to avoid the wasted extra overhead of
> `check-chainlint` (and chainlint in general).

Yes, but I just don't see why it's needed.

We need to build e.g. t/helpers/ to run the tests, and doing that is
probably always going to take eons of compilation times compared to
these assertions.

But it's a one-off eon because we declare the dependency DAG and don't
recompile them unless the sources or their dependencies change. Likewise
for these chainlint tests we can (and maybe should) make them optional,
but as long as we're not needlessly running them when no changes have
happened...

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Eric Sunshine]

On Mon, Dec 13, 2021 at 2:36 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Dec 13 2021, Eric Sunshine wrote:
> > Another less sledge-hammer approach would be to make t/Makefile
> > respect GIT_TEST_CHAIN_LINT so that it doesn't run `check-chainlint`
> > for `test` and `prove` when that variable is `0`. That would allow
> > your `git rebase -i --exec` case to avoid the wasted extra overhead of
> > `check-chainlint` (and chainlint in general).
>
> Yes, but I just don't see why it's needed.
>
> We need to build e.g. t/helpers/ to run the tests, and doing that is
> probably always going to take eons of compilation times compared to
> these assertions.
>
> But it's a one-off eon because we declare the dependency DAG and don't
> recompile them unless the sources or their dependencies change. Likewise
> for these chainlint tests we can (and maybe should) make them optional,
> but as long as we're not needlessly running them when no changes have
> happened...

That's a good point, and the same observation applies to
t/check-non-portable-shell.pl which is run unconditionally, as well,
whenever `test` and `prove` are run.

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Fabian Stelzer]

On 13.12.2021 11:02, Eric Sunshine wrote:
>On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> Personally i find the initial for loop variant to be the most readable.
>> Ævars makefile targets could be very nice too, but especially:
>>
>> +$(BUILT_CHAINLINTTESTS): | .build/chainlint
>> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %
>> +       $(CHAINLINT) <$< | \
>> +        sed -e '/^# LINT: /d' >$@ && \
>> +       diff -u $(basename $<).expect $@
>>
>> i find very hard to grasp :/
>> I have no idea what is going on here: `<$< |` ?
>
>Ya, that line-noise is an unfortunate combination of shell and
>Makefile gobbledygook. The `$<` is effectively the source file (the
>file being passed into chainlint.sed), and the rest of it is just
>normal shell. `<` is redirection (using the source file `$<` as
>stdin), and `|` is the pipe operator (sending the output of
>chainlint.sed to another `sed`), and capturing it all via shell `>`
>redirection in `$@` which is the Makefile variable for the target
>file.
>

Thanks, that explains it nicely. I'm not familiar enough with makefile 
syntax.

>Anyhow, although the commit message tries to sell this change as some
>sort of optimization, it's really in preparation for the new chainlint
>which wants to check all tests in all files with a single invocation
>rather than being invoked over and over and over. The self-test files
>also require more preprocessing to work with the new chainlint, so the
>implementation of `check-chainlint` gets rather more complex once the
>end state is reached. I'll think about it a bit, but at the moment,
>I'm still leaning toward this intermediate step as being beneficial to
>reaching the end state. However, my opinion could change since the way
>this is done here was probably influenced by an earlier iteration of
>the new chainlint, but now that the implementation of the new
>chainlint is concrete, it may not be especially important to do it
>this way.

I don't mind much either way and i tend to favor the efficient variant as 
well. On the other hand i can also see some bug mixing up the contents of 
these files producing a huge diff with no good indication for the developer 
of what has happened.

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 13 2021, Fabian Stelzer wrote:

> On 13.12.2021 09:27, Eric Sunshine wrote:
>>On Mon, Dec 13, 2021 at 5:22 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>>> On 13.12.2021 01:30, Eric Sunshine wrote:
>>> > check-chainlint:
>>> >+      sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
>>> >+      cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
>>> >+      $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \
>>> >+      diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
>>>
>>> If I read this right you are relying on the order of the .test & .expect
>>> files to match. I did something similar in a test prereq which resulted in a
>>> bug when setting the test_output_dir to something residing in /dev/shm,
>>> since the order of files in /dev/shm is reversed (at least on some
>>> platforms). Even though this should work as is I could see this leading to a
>>> similar bug in the future.
>>
>>It's not seen in the patch context, but earlier in the file we have:
>>
>>    CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))
>>
>>which provides stability via `sort`, thus ensures that the order of
>>the ".test" and ".expect" match.
>>
>>I think that addresses your concern (unless I misunderstand your observation).
>
> Yes, thats what i meant. I didn't realize $CHAINLINTTESTS is already
> the sorted glob. Thanks for clarifying.
>
> Personally i find the initial for loop variant to be the most
> readable.  Ævars makefile targets could be very nice too, but
> especially:
>
> +$(BUILT_CHAINLINTTESTS): | .build/chainlint
> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %
> +       $(CHAINLINT) <$< | \
> +	 sed -e '/^# LINT: /d' >$@ && \
> +       diff -u $(basename $<).expect $@
>
> i find very hard to grasp :/
> I have no idea what is going on here: `<$< |` ?

It's a minor point, and not relevant to this series proceeding.

But just FWIW I think both of you are wrong about the potenital for a
".test" and ".expect" bug here.

I.e. yes the CHAINLINTTESTS variable is sorted:

    CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))

But in Eric's patch we just have this relevant to this concern of
(paraphrased) "would it not be sorted break it?":

	+	sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
	+	cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \

So it doesn't matter if it's sorted our not.

I.e. we've got {A,B,C}.{test,expect} files in a directory, and we're
constructing a "A.test" and "A.expect" via "$(patsubst)".

So if it's "A B C", "C B A", "A C B" etc. won't matter. We'll always get
".test" files corresponding to ".expect".

If it's not sorted we'll get failure output in an unexpected order, but
it won't matter to whether we detect a failure or not.

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Eric Sunshine]

On Thu, Dec 16, 2021 at 8:22 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > On 13.12.2021 09:27, Eric Sunshine wrote:
> >>It's not seen in the patch context, but earlier in the file we have:
> >>
> >>    CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))
> >>
> >>which provides stability via `sort`, thus ensures that the order of
> >>the ".test" and ".expect" match.
> >>
> >>I think that addresses your concern (unless I misunderstand your observation).
>
> But just FWIW I think both of you are wrong about the potenital for a
> ".test" and ".expect" bug here.
>
> I.e. yes the CHAINLINTTESTS variable is sorted:
>
> But in Eric's patch we just have this relevant to this concern of
> (paraphrased) "would it not be sorted break it?":
>
>         +       sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
>         +       cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
>
> So it doesn't matter if it's sorted our not.
>
> I.e. we've got {A,B,C}.{test,expect} files in a directory, and we're
> constructing a "A.test" and "A.expect" via "$(patsubst)".
>
> So if it's "A B C", "C B A", "A C B" etc. won't matter. We'll always get
> ".test" files corresponding to ".expect".

Yes, sorry, I meant to say something along these lines in my reply, in
addition to mentioning `sort`, but forgot. Taking a look at this
again, though, makes me wonder if the CHAINLINTTESTS assignment should
be done with `:=` rather than `=` (unless GNU make is smart enough to
only invoke the `wildcard` operation only once, in which case it
wouldn't particularly matter).

Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 16 2021, Eric Sunshine wrote:

> On Thu, Dec 16, 2021 at 8:22 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> > On 13.12.2021 09:27, Eric Sunshine wrote:
>> >>It's not seen in the patch context, but earlier in the file we have:
>> >>
>> >>    CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))
>> >>
>> >>which provides stability via `sort`, thus ensures that the order of
>> >>the ".test" and ".expect" match.
>> >>
>> >>I think that addresses your concern (unless I misunderstand your observation).
>>
>> But just FWIW I think both of you are wrong about the potenital for a
>> ".test" and ".expect" bug here.
>>
>> I.e. yes the CHAINLINTTESTS variable is sorted:
>>
>> But in Eric's patch we just have this relevant to this concern of
>> (paraphrased) "would it not be sorted break it?":
>>
>>         +       sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
>>         +       cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
>>
>> So it doesn't matter if it's sorted our not.
>>
>> I.e. we've got {A,B,C}.{test,expect} files in a directory, and we're
>> constructing a "A.test" and "A.expect" via "$(patsubst)".
>>
>> So if it's "A B C", "C B A", "A C B" etc. won't matter. We'll always get
>> ".test" files corresponding to ".expect".
>
> Yes, sorry, I meant to say something along these lines in my reply, in
> addition to mentioning `sort`, but forgot. Taking a look at this
> again, though, makes me wonder if the CHAINLINTTESTS assignment should
> be done with `:=` rather than `=` (unless GNU make is smart enough to
> only invoke the `wildcard` operation only once, in which case it
> wouldn't particularly matter).

It appears to be smart enough to do that, i.e. it'll invoke a $(shell)
assignment N times for -jN unless you use simply-expanded variables, but
not for a simple wildcard.

But I really don't think that'll matter, doing that I/O is trivial
compared to other things involved there.

If for some weird reson (e.g. hundreds of thousands of dependency files)
you find yourself trying to optimize make runs on this basis, this is
the wrong way to go about it.

Instead you'd have a rule depending on the contain directory, whose
mtime will be updated should anything there change. You shouldn't need
that trickery for anything the size of the git codebase, but that
approach will beat trying to optimize the O(n) lstat() calls you'll
otherwise be doing on the contents of the directory.

[PATCH 06/15] chainlint.sed: improve ?!AMP?! placement accuracy

[Eric Sunshine]

When chainlint.sed detects a broken &&-chain, it places an ?!AMP?!
annotation at the beginning of the line. However, this is an unusual
location for programmers accustomed to error messages (from compilers,
for instance) indicating the exact point of the problem. Therefore,
relocate the ?!AMP?! annotation to the end of the line in order to
better direct the programmer's attention to the source of the problem.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.sed                                      | 8 ++++----
 t/chainlint/arithmetic-expansion.expect              | 2 +-
 t/chainlint/block.expect                             | 2 +-
 t/chainlint/broken-chain.expect                      | 2 +-
 t/chainlint/case.expect                              | 4 ++--
 t/chainlint/command-substitution.expect              | 2 +-
 t/chainlint/cuddled.expect                           | 4 ++--
 t/chainlint/for-loop.expect                          | 4 ++--
 t/chainlint/here-doc-multi-line-command-subst.expect | 2 +-
 t/chainlint/here-doc-multi-line-string.expect        | 2 +-
 t/chainlint/if-in-loop.expect                        | 6 +++---
 t/chainlint/if-then-else.expect                      | 4 ++--
 t/chainlint/inline-comment.expect                    | 2 +-
 t/chainlint/loop-in-if.expect                        | 6 +++---
 t/chainlint/multi-line-string.expect                 | 2 +-
 t/chainlint/nested-cuddled-subshell.expect           | 6 +++---
 t/chainlint/nested-here-doc.expect                   | 2 +-
 t/chainlint/nested-subshell-comment.expect           | 2 +-
 t/chainlint/pipe.expect                              | 2 +-
 t/chainlint/semicolon.expect                         | 2 +-
 t/chainlint/subshell-here-doc.expect                 | 2 +-
 t/chainlint/subshell-one-liner.expect                | 4 ++--
 t/chainlint/while-loop.expect                        | 4 ++--
 23 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index 8a25c5b855..883a2b307c 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -236,7 +236,7 @@ s/.*\n//
 # line ends with pipe "...|" -- valid; not missing "&&"
 /|[ 	]*$/bcont
 # missing end-of-line "&&" -- mark suspect
-/&&[ 	]*$/!s/^/?!AMP?!/
+/&&[ 	]*$/!s/$/ ?!AMP?!/
 :cont
 # retrieve and print previous line
 x
@@ -303,7 +303,7 @@ bcase
 # that line legitimately lacks "&&"
 :else
 x
-s/?!AMP?!//
+s/ ?!AMP?!$//
 x
 bcont
 
@@ -311,7 +311,7 @@ bcont
 # "suspect" from final contained line since that line legitimately lacks "&&"
 :done
 x
-s/?!AMP?!//
+s/ ?!AMP?!$//
 x
 # is 'done' or 'fi' cuddled with ")" to close subshell?
 /done.*)/bclose
@@ -354,7 +354,7 @@ bblock
 # since that line legitimately lacks "&&" and exit subshell loop
 :clssolo
 x
-s/?!AMP?!//
+s/ ?!AMP?!$//
 p
 x
 s/^/>/
diff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect
index 09457d3196..56cd5b69f5 100644
--- a/t/chainlint/arithmetic-expansion.expect
+++ b/t/chainlint/arithmetic-expansion.expect
@@ -4,6 +4,6 @@
 	baz
 >) &&
 (
-?!AMP?!	bar=$((42 + 1))
+	bar=$((42 + 1)) ?!AMP?!
 	baz
 >)
diff --git a/t/chainlint/block.expect b/t/chainlint/block.expect
index fed7e89ae8..6333237cb2 100644
--- a/t/chainlint/block.expect
+++ b/t/chainlint/block.expect
@@ -7,6 +7,6 @@
 	bar &&
 	{
 		echo c
-?!AMP?!	}
+	} ?!AMP?!
 	baz
 >)
diff --git a/t/chainlint/broken-chain.expect b/t/chainlint/broken-chain.expect
index 55b0f42a53..0960a8c7c0 100644
--- a/t/chainlint/broken-chain.expect
+++ b/t/chainlint/broken-chain.expect
@@ -1,6 +1,6 @@
 (
 	foo &&
-?!AMP?!	bar
+	bar ?!AMP?!
 	baz &&
 	wop
 >)
diff --git a/t/chainlint/case.expect b/t/chainlint/case.expect
index 41f121fbbf..a4b92d4613 100644
--- a/t/chainlint/case.expect
+++ b/t/chainlint/case.expect
@@ -9,11 +9,11 @@
 	case "$x" in
 	x) foo ;;
 	*) bar ;;
-?!AMP?!	esac
+	esac ?!AMP?!
 	foobar
 >) &&
 (
 	case "$x" in 1) true;; esac &&
-?!AMP?!	case "$y" in 2) false;; esac
+	case "$y" in 2) false;; esac ?!AMP?!
 	foobar
 >)
diff --git a/t/chainlint/command-substitution.expect b/t/chainlint/command-substitution.expect
index ad4118e537..f276067b7b 100644
--- a/t/chainlint/command-substitution.expect
+++ b/t/chainlint/command-substitution.expect
@@ -4,6 +4,6 @@
 	baz
 >) &&
 (
-?!AMP?!	bar=$(gobble blocks)
+	bar=$(gobble blocks) ?!AMP?!
 	baz
 >)
diff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect
index b506d46221..b6c4ed90a9 100644
--- a/t/chainlint/cuddled.expect
+++ b/t/chainlint/cuddled.expect
@@ -4,7 +4,7 @@ cd foo &&
 >) &&
 
 (
-?!AMP?!cd foo
+cd foo ?!AMP?!
 	bar
 >) &&
 
@@ -17,5 +17,5 @@ cd foo &&
 >	bar) &&
 
 (
-?!AMP?!cd foo
+cd foo ?!AMP?!
 >	bar)
diff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect
index c33cf56ee7..dc209e21bd 100644
--- a/t/chainlint/for-loop.expect
+++ b/t/chainlint/for-loop.expect
@@ -1,9 +1,9 @@
 (
 	for i in a b c
 	do
-?!AMP?!		echo $i
+		echo $i ?!AMP?!
 		cat
-?!AMP?!	done
+	done ?!AMP?!
 	for i in a b c; do
 		echo $i &&
 		cat $i
diff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect
index e5fb752d2f..3a35bb014c 100644
--- a/t/chainlint/here-doc-multi-line-command-subst.expect
+++ b/t/chainlint/here-doc-multi-line-command-subst.expect
@@ -1,5 +1,5 @@
 (
 	x=$(bobble &&
-?!AMP?!>>		wiffle)
+>>		wiffle) ?!AMP?!
 	echo $x
 >)
diff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect
index 32038a070c..a3b9a5472d 100644
--- a/t/chainlint/here-doc-multi-line-string.expect
+++ b/t/chainlint/here-doc-multi-line-string.expect
@@ -1,4 +1,4 @@
 (
-?!AMP?!	cat && echo "multi-line	string"
+	cat && echo "multi-line	string" ?!AMP?!
 	bap
 >)
diff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect
index 03d3ceb22d..7d91837269 100644
--- a/t/chainlint/if-in-loop.expect
+++ b/t/chainlint/if-in-loop.expect
@@ -3,10 +3,10 @@
 	do
 		if false
 		then
-?!AMP?!			echo "err"
+			echo "err" ?!AMP?!
 			exit 1
-?!AMP?!		fi
+		fi ?!AMP?!
 		foo
-?!AMP?!	done
+	done ?!AMP?!
 	bar
 >)
diff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect
index a80f5e6c75..3055d5606c 100644
--- a/t/chainlint/if-then-else.expect
+++ b/t/chainlint/if-then-else.expect
@@ -1,7 +1,7 @@
 (
 	if test -n ""
 	then
-?!AMP?!		echo very
+		echo very ?!AMP?!
 		echo empty
 	elif test -z ""
 	then
@@ -9,7 +9,7 @@
 	else
 		echo foo &&
 		cat
-?!AMP?!	fi
+	fi ?!AMP?!
 	echo poodle
 >) &&
 (
diff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect
index fc9f250ac4..3d655a32b0 100644
--- a/t/chainlint/inline-comment.expect
+++ b/t/chainlint/inline-comment.expect
@@ -1,6 +1,6 @@
 (
 	foobar &&
-?!AMP?!	barfoo
+	barfoo ?!AMP?!
 	flibble "not a # comment"
 >) &&
 
diff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect
index 088e622c31..cebd3ae95e 100644
--- a/t/chainlint/loop-in-if.expect
+++ b/t/chainlint/loop-in-if.expect
@@ -3,10 +3,10 @@
 	then
 		while true
 		do
-?!AMP?!			echo "pop"
+			echo "pop" ?!AMP?!
 			echo "glup"
-?!AMP?!		done
+		done ?!AMP?!
 		foo
-?!AMP?!	fi
+	fi ?!AMP?!
 	bar
 >)
diff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect
index 2829516495..f1be2baf0a 100644
--- a/t/chainlint/multi-line-string.expect
+++ b/t/chainlint/multi-line-string.expect
@@ -1,6 +1,6 @@
 (
 	x="line 1		line 2		line 3" &&
-?!AMP?!	y="line 1		line2"
+	y="line 1		line2" ?!AMP?!
 	foobar
 >) &&
 (
diff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect
index c2a59ffc33..aa522658ed 100644
--- a/t/chainlint/nested-cuddled-subshell.expect
+++ b/t/chainlint/nested-cuddled-subshell.expect
@@ -4,16 +4,16 @@
 >>	) &&
 	(cd foo &&
 		bar
-?!AMP?!>>	)
+>>	) ?!AMP?!
 	(
 		cd foo &&
 >>		bar) &&
 	(
 		cd foo &&
-?!AMP?!>>		bar)
+>>		bar) ?!AMP?!
 	(cd foo &&
 >>		bar) &&
 	(cd foo &&
-?!AMP?!>>		bar)
+>>		bar) ?!AMP?!
 	foobar
 >)
diff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect
index 0c9ef1cfc6..f9604d3fac 100644
--- a/t/chainlint/nested-here-doc.expect
+++ b/t/chainlint/nested-here-doc.expect
@@ -2,6 +2,6 @@ cat >foop &&
 
 (
 	cat &&
-?!AMP?!	cat
+	cat ?!AMP?!
 	foobar
 >)
diff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect
index 15b68d4373..925e49bae9 100644
--- a/t/chainlint/nested-subshell-comment.expect
+++ b/t/chainlint/nested-subshell-comment.expect
@@ -6,6 +6,6 @@
 		# minor numbers of cows (or do they?)
 		baz &&
 		snaff
-?!AMP?!>>	)
+>>	) ?!AMP?!
 	fuzzy
 >)
diff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect
index 211b901dbc..ede6bcc607 100644
--- a/t/chainlint/pipe.expect
+++ b/t/chainlint/pipe.expect
@@ -3,6 +3,6 @@
 	bar |
 	baz &&
 	fish |
-?!AMP?!	cow
+	cow ?!AMP?!
 	sunder
 >)
diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect
index 1d79384606..ffc87bdffb 100644
--- a/t/chainlint/semicolon.expect
+++ b/t/chainlint/semicolon.expect
@@ -1,5 +1,5 @@
 (
-?!AMP?!?!SEMI?!	cat foo ; echo bar
+?!SEMI?!	cat foo ; echo bar ?!AMP?!
 ?!SEMI?!	cat foo ; echo bar
 >) &&
 (
diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index 7e057aee42..9d3f25b3f5 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -1,6 +1,6 @@
 (
 	echo wobba 	       gorgo snoot 	       wafta snurb &&
-?!AMP?!	cat >bip
+	cat >bip ?!AMP?!
 	echo >bop
 >) &&
 (
diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect
index 51162821d7..ec77aa5b95 100644
--- a/t/chainlint/subshell-one-liner.expect
+++ b/t/chainlint/subshell-one-liner.expect
@@ -8,7 +8,7 @@
 	(foo || exit 1) &&
 	(foo || exit 1) |
 	(foo || exit 1) >baz &&
-?!AMP?!	(foo && bar)
-?!AMP?!?!SEMI?!	(foo && bar; baz)
+	(foo && bar) ?!AMP?!
+?!SEMI?!	(foo && bar; baz) ?!AMP?!
 	foobar
 >)
diff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect
index 13cff2c0a5..f8b9fcf62b 100644
--- a/t/chainlint/while-loop.expect
+++ b/t/chainlint/while-loop.expect
@@ -1,9 +1,9 @@
 (
 	while true
 	do
-?!AMP?!		echo foo
+		echo foo ?!AMP?!
 		cat
-?!AMP?!	done
+	done ?!AMP?!
 	while true; do
 		echo foo &&
 		cat bar
-- 
2.34.1.397.gfae76fe5da

[PATCH 07/15] chainlint.sed: improve ?!SEMI?! placement accuracy

[Eric Sunshine]

When chainlint.sed detects commands separated by a semicolon rather than
by `&&`, it places a ?!SEMI?! annotation at the beginning of the line.
However, this is an unusual location for programmers accustomed to error
messages (from compilers, for instance) indicating the exact point of
the problem. Therefore, relocate the ?!SEMI?! annotation to the location
of the semicolon in order to better direct the programmer's attention to
the source of the problem.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.sed                       |  4 ++--
 t/chainlint/negated-one-liner.expect  |  4 ++--
 t/chainlint/one-liner.expect          |  6 +++---
 t/chainlint/semicolon.expect          | 14 +++++++-------
 t/chainlint/subshell-one-liner.expect |  8 ++++----
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index 883a2b307c..60c2099c18 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -126,7 +126,7 @@ b
 # "&&" (but not ";" in a string)
 :oneline
 /;/{
-	/"[^"]*;[^"]*"/!s/^/?!SEMI?!/
+	/"[^"]*;[^"]*"/!s/;/; ?!SEMI?!/
 }
 b
 
@@ -230,7 +230,7 @@ s/.*\n//
 # string and not ";;" in one-liner "case...esac")
 /;/{
 	/;;/!{
-		/"[^"]*;[^"]*"/!s/^/?!SEMI?!/
+		/"[^"]*;[^"]*"/!s/;/; ?!SEMI?!/
 	}
 }
 # line ends with pipe "...|" -- valid; not missing "&&"
diff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect
index cf18429d03..60baf84b7a 100644
--- a/t/chainlint/negated-one-liner.expect
+++ b/t/chainlint/negated-one-liner.expect
@@ -1,5 +1,5 @@
 ! (foo && bar) &&
 ! (foo && bar) >baz &&
 
-?!SEMI?!! (foo; bar) &&
-?!SEMI?!! (foo; bar) >baz
+! (foo; ?!SEMI?! bar) &&
+! (foo; ?!SEMI?! bar) >baz
diff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect
index c64058f7af..3b46554728 100644
--- a/t/chainlint/one-liner.expect
+++ b/t/chainlint/one-liner.expect
@@ -2,8 +2,8 @@
 (foo && bar) |
 (foo && bar) >baz &&
 
-?!SEMI?!(foo; bar) &&
-?!SEMI?!(foo; bar) |
-?!SEMI?!(foo; bar) >baz &&
+(foo; ?!SEMI?! bar) &&
+(foo; ?!SEMI?! bar) |
+(foo; ?!SEMI?! bar) >baz &&
 
 (foo "bar; baz")
diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect
index ffc87bdffb..d2d804f5b0 100644
--- a/t/chainlint/semicolon.expect
+++ b/t/chainlint/semicolon.expect
@@ -1,20 +1,20 @@
 (
-?!SEMI?!	cat foo ; echo bar ?!AMP?!
-?!SEMI?!	cat foo ; echo bar
+	cat foo ; ?!SEMI?! echo bar ?!AMP?!
+	cat foo ; ?!SEMI?! echo bar
 >) &&
 (
-?!SEMI?!	cat foo ; echo bar &&
-?!SEMI?!	cat foo ; echo bar
+	cat foo ; ?!SEMI?! echo bar &&
+	cat foo ; ?!SEMI?! echo bar
 >) &&
 (
 	echo "foo; bar" &&
-?!SEMI?!	cat foo; echo bar
+	cat foo; ?!SEMI?! echo bar
 >) &&
 (
-?!SEMI?!	foo;
+	foo; ?!SEMI?!
 >) &&
 (
 cd foo &&
 	for i in a b c; do
-?!SEMI?!		echo;
+		echo; ?!SEMI?!
 >	done)
diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect
index ec77aa5b95..432217801b 100644
--- a/t/chainlint/subshell-one-liner.expect
+++ b/t/chainlint/subshell-one-liner.expect
@@ -2,13 +2,13 @@
 	(foo && bar) &&
 	(foo && bar) |
 	(foo && bar) >baz &&
-?!SEMI?!	(foo; bar) &&
-?!SEMI?!	(foo; bar) |
-?!SEMI?!	(foo; bar) >baz &&
+	(foo; ?!SEMI?! bar) &&
+	(foo; ?!SEMI?! bar) |
+	(foo; ?!SEMI?! bar) >baz &&
 	(foo || exit 1) &&
 	(foo || exit 1) |
 	(foo || exit 1) >baz &&
 	(foo && bar) ?!AMP?!
-?!SEMI?!	(foo && bar; baz) ?!AMP?!
+	(foo && bar; ?!SEMI?! baz) ?!AMP?!
 	foobar
 >)
-- 
2.34.1.397.gfae76fe5da

[PATCH 08/15] chainlint.sed: tolerate harmless ";" at end of last line in block

[Eric Sunshine]

chainlint.sed flags ";" when used as a command terminator since it
breaks the &&-chain, thus can allow failures to go undetected. However,
when a command terminated by ";" is the last command in the body of a
compound statement, such as `command-2` in:

    if test $# -gt 1
    then
        command-1 &&
        command-2;
    fi

then the ";" is harmless and the exit code from `command-2` is passed
through untouched and becomes the exit code of the compound statement,
as if the ";" was not present. Therefore, tolerate a trailing ";" in
this position rather than complaining about broken &&-chain.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.sed              | 11 ++++++-----
 t/chainlint/semicolon.expect |  4 ++--
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index 60c2099c18..91077b6e26 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -47,8 +47,9 @@
 # "?!AMP?!" violation is removed from the "bar" line (retrieved from the "hold"
 # area) since the final statement of a subshell must not end with "&&". The
 # final line of a subshell may still break the &&-chain by using ";" internally
-# to chain commands together rather than "&&", so "?!SEMI?!" is never removed
-# from a line (even though "?!AMP?!" might be).
+# to chain commands together rather than "&&", so "?!SEMI?!" is not removed
+# from such a line; however, if the line ends with "?!SEMI?!", then the ";" is
+# harmless and the annotation is removed.
 #
 # Care is taken to recognize the last _statement_ of a multi-line subshell, not
 # necessarily the last textual _line_ within the subshell, since &&-chaining
@@ -303,7 +304,7 @@ bcase
 # that line legitimately lacks "&&"
 :else
 x
-s/ ?!AMP?!$//
+s/\( ?!SEMI?!\)* ?!AMP?!$//
 x
 bcont
 
@@ -311,7 +312,7 @@ bcont
 # "suspect" from final contained line since that line legitimately lacks "&&"
 :done
 x
-s/ ?!AMP?!$//
+s/\( ?!SEMI?!\)* ?!AMP?!$//
 x
 # is 'done' or 'fi' cuddled with ")" to close subshell?
 /done.*)/bclose
@@ -354,7 +355,7 @@ bblock
 # since that line legitimately lacks "&&" and exit subshell loop
 :clssolo
 x
-s/ ?!AMP?!$//
+s/\( ?!SEMI?!\)* ?!AMP?!$//
 p
 x
 s/^/>/
diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect
index d2d804f5b0..0e6389f532 100644
--- a/t/chainlint/semicolon.expect
+++ b/t/chainlint/semicolon.expect
@@ -11,10 +11,10 @@
 	cat foo; ?!SEMI?! echo bar
 >) &&
 (
-	foo; ?!SEMI?!
+	foo;
 >) &&
 (
 cd foo &&
 	for i in a b c; do
-		echo; ?!SEMI?!
+		echo;
 >	done)
-- 
2.34.1.397.gfae76fe5da

[PATCH 09/15] chainlint.sed: drop unnecessary distinction between ?!AMP?! and ?!SEMI?!

[Eric Sunshine]

From inception, when chainlint.sed encountered a line using semicolon to
separate commands rather than `&&`, it would insert a ?!SEMI?!
annotation at the beginning of the line rather ?!AMP?! even though the
&&-chain is also broken by the semicolon. Given a line such as:

    ?!SEMI?! cmd1; cmd2 &&

the ?!SEMI?! annotation makes it easier to see what the problem is than
if the output had been:

    ?!AMP?! cmd1; cmd2 &&

which might confuse the test author into thinking that the linter is
broken (since the line clearly ends with `&&`).

However, now that the ?!AMP?! an ?!SEMI?! annotations are inserted at
the point of breakage rather than at the beginning of the line, and
taking into account that both represent a broken &&-chain, there is
little reason to distinguish between the two. Using ?!AMP?! alone is
sufficient to point the test author at the problem. For instance, in:

    cmd1; ?!AMP?! cmd2 &&
    cmd3

it is clear that the &&-chain is broken between `cmd1` and `cmd2`.
Likewise, in:

    cmd1 && cmd2 ?!AMP?!
    cmd3

it is clear that the &&-chain is broken between `cmd2` and `cmd3`.
Finally, in:

    cmd1; ?!AMP?! cmd2 ?!AMP?!
    cmd3

it is clear that the &&-chain is broken between each command.

Hence, there is no longer a good reason to make a distinction between a
broken &&-chain due to a semicolon and a broken chain due to a missing
`&&` at end-of-line. Therefore, drop the ?!SEMI?! annotation and use
?!AMP?! exclusively.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.sed                       | 21 ++++++++++-----------
 t/chainlint/negated-one-liner.expect  |  4 ++--
 t/chainlint/one-liner.expect          |  6 +++---
 t/chainlint/semicolon.expect          | 10 +++++-----
 t/chainlint/subshell-one-liner.expect |  8 ++++----
 5 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index 91077b6e26..f5fcca09ca 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -24,9 +24,9 @@
 # in order to avoid misinterpreting the ")" in constructs such as "x=$(...)"
 # and "case $x in *)" as ending the subshell.
 #
-# Lines missing a final "&&" are flagged with "?!AMP?!", and lines which chain
-# commands with ";" internally rather than "&&" are flagged "?!SEMI?!". A line
-# may be flagged for both violations.
+# Lines missing a final "&&" are flagged with "?!AMP?!", as are lines which
+# chain commands with ";" internally rather than "&&". A line may be flagged
+# for both violations.
 #
 # Detection of a missing &&-link in a multi-line subshell is complicated by the
 # fact that the last statement before the closing ")" must not end with "&&".
@@ -47,9 +47,8 @@
 # "?!AMP?!" violation is removed from the "bar" line (retrieved from the "hold"
 # area) since the final statement of a subshell must not end with "&&". The
 # final line of a subshell may still break the &&-chain by using ";" internally
-# to chain commands together rather than "&&", so "?!SEMI?!" is not removed
-# from such a line; however, if the line ends with "?!SEMI?!", then the ";" is
-# harmless and the annotation is removed.
+# to chain commands together rather than "&&", but an internal "?!AMP?!" is
+# never removed from a line even though a line-ending "?!AMP?!" might be.
 #
 # Care is taken to recognize the last _statement_ of a multi-line subshell, not
 # necessarily the last textual _line_ within the subshell, since &&-chaining
@@ -127,7 +126,7 @@ b
 # "&&" (but not ";" in a string)
 :oneline
 /;/{
-	/"[^"]*;[^"]*"/!s/;/; ?!SEMI?!/
+	/"[^"]*;[^"]*"/!s/;/; ?!AMP?!/
 }
 b
 
@@ -231,7 +230,7 @@ s/.*\n//
 # string and not ";;" in one-liner "case...esac")
 /;/{
 	/;;/!{
-		/"[^"]*;[^"]*"/!s/;/; ?!SEMI?!/
+		/"[^"]*;[^"]*"/!s/;/; ?!AMP?!/
 	}
 }
 # line ends with pipe "...|" -- valid; not missing "&&"
@@ -304,7 +303,7 @@ bcase
 # that line legitimately lacks "&&"
 :else
 x
-s/\( ?!SEMI?!\)* ?!AMP?!$//
+s/\( ?!AMP?!\)* ?!AMP?!$//
 x
 bcont
 
@@ -312,7 +311,7 @@ bcont
 # "suspect" from final contained line since that line legitimately lacks "&&"
 :done
 x
-s/\( ?!SEMI?!\)* ?!AMP?!$//
+s/\( ?!AMP?!\)* ?!AMP?!$//
 x
 # is 'done' or 'fi' cuddled with ")" to close subshell?
 /done.*)/bclose
@@ -355,7 +354,7 @@ bblock
 # since that line legitimately lacks "&&" and exit subshell loop
 :clssolo
 x
-s/\( ?!SEMI?!\)* ?!AMP?!$//
+s/\( ?!AMP?!\)* ?!AMP?!$//
 p
 x
 s/^/>/
diff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect
index 60baf84b7a..ad4c2d949e 100644
--- a/t/chainlint/negated-one-liner.expect
+++ b/t/chainlint/negated-one-liner.expect
@@ -1,5 +1,5 @@
 ! (foo && bar) &&
 ! (foo && bar) >baz &&
 
-! (foo; ?!SEMI?! bar) &&
-! (foo; ?!SEMI?! bar) >baz
+! (foo; ?!AMP?! bar) &&
+! (foo; ?!AMP?! bar) >baz
diff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect
index 3b46554728..57a7a444c1 100644
--- a/t/chainlint/one-liner.expect
+++ b/t/chainlint/one-liner.expect
@@ -2,8 +2,8 @@
 (foo && bar) |
 (foo && bar) >baz &&
 
-(foo; ?!SEMI?! bar) &&
-(foo; ?!SEMI?! bar) |
-(foo; ?!SEMI?! bar) >baz &&
+(foo; ?!AMP?! bar) &&
+(foo; ?!AMP?! bar) |
+(foo; ?!AMP?! bar) >baz &&
 
 (foo "bar; baz")
diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect
index 0e6389f532..54a08ce582 100644
--- a/t/chainlint/semicolon.expect
+++ b/t/chainlint/semicolon.expect
@@ -1,14 +1,14 @@
 (
-	cat foo ; ?!SEMI?! echo bar ?!AMP?!
-	cat foo ; ?!SEMI?! echo bar
+	cat foo ; ?!AMP?! echo bar ?!AMP?!
+	cat foo ; ?!AMP?! echo bar
 >) &&
 (
-	cat foo ; ?!SEMI?! echo bar &&
-	cat foo ; ?!SEMI?! echo bar
+	cat foo ; ?!AMP?! echo bar &&
+	cat foo ; ?!AMP?! echo bar
 >) &&
 (
 	echo "foo; bar" &&
-	cat foo; ?!SEMI?! echo bar
+	cat foo; ?!AMP?! echo bar
 >) &&
 (
 	foo;
diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect
index 432217801b..4b44632b09 100644
--- a/t/chainlint/subshell-one-liner.expect
+++ b/t/chainlint/subshell-one-liner.expect
@@ -2,13 +2,13 @@
 	(foo && bar) &&
 	(foo && bar) |
 	(foo && bar) >baz &&
-	(foo; ?!SEMI?! bar) &&
-	(foo; ?!SEMI?! bar) |
-	(foo; ?!SEMI?! bar) >baz &&
+	(foo; ?!AMP?! bar) &&
+	(foo; ?!AMP?! bar) |
+	(foo; ?!AMP?! bar) >baz &&
 	(foo || exit 1) &&
 	(foo || exit 1) |
 	(foo || exit 1) >baz &&
 	(foo && bar) ?!AMP?!
-	(foo && bar; ?!SEMI?! baz) ?!AMP?!
+	(foo && bar; ?!AMP?! baz) ?!AMP?!
 	foobar
 >)
-- 
2.34.1.397.gfae76fe5da

[PATCH 10/15] chainlint.sed: drop subshell-closing ">" annotation

[Eric Sunshine]

chainlint.sed inserts a ">" annotation at the beginning of a line to
signal that its heuristics have identified an end-of-subshell. This was
useful as a debugging aid during development of the script, but it has
no value to test writers and might even confuse them into thinking that
the linter is misbehaving by inserting line-noise into the shell code it
is validating. Moreover, its presence also potentially makes it
difficult to reuse the chainlint self-test "expect" output should a more
capable linter ever be developed. Therefore, drop the ">" annotation.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.sed                                  |  9 ---------
 t/chainlint/arithmetic-expansion.expect          |  4 ++--
 t/chainlint/bash-array.expect                    |  4 ++--
 t/chainlint/blank-line.expect                    |  2 +-
 t/chainlint/block.expect                         |  2 +-
 t/chainlint/broken-chain.expect                  |  2 +-
 t/chainlint/case.expect                          |  6 +++---
 .../close-nested-and-parent-together.expect      |  2 +-
 t/chainlint/close-subshell.expect                | 16 ++++++++--------
 t/chainlint/command-substitution.expect          |  4 ++--
 t/chainlint/comment.expect                       |  2 +-
 t/chainlint/complex-if-in-cuddled-loop.expect    |  2 +-
 t/chainlint/cuddled-if-then-else.expect          |  2 +-
 t/chainlint/cuddled-loop.expect                  |  2 +-
 t/chainlint/cuddled.expect                       | 10 +++++-----
 t/chainlint/exit-loop.expect                     |  6 +++---
 t/chainlint/exit-subshell.expect                 |  2 +-
 t/chainlint/for-loop.expect                      |  2 +-
 t/chainlint/here-doc-close-subshell.expect       |  2 +-
 .../here-doc-multi-line-command-subst.expect     |  4 ++--
 t/chainlint/here-doc-multi-line-string.expect    |  2 +-
 t/chainlint/if-in-loop.expect                    |  2 +-
 t/chainlint/if-then-else.expect                  |  4 ++--
 t/chainlint/incomplete-line.expect               |  2 +-
 t/chainlint/inline-comment.expect                |  4 ++--
 t/chainlint/loop-in-if.expect                    |  2 +-
 ...multi-line-nested-command-substitution.expect | 10 +++++-----
 t/chainlint/multi-line-string.expect             |  4 ++--
 t/chainlint/nested-cuddled-subshell.expect       | 14 +++++++-------
 t/chainlint/nested-here-doc.expect               |  2 +-
 t/chainlint/nested-subshell-comment.expect       |  4 ++--
 t/chainlint/nested-subshell.expect               |  6 +++---
 t/chainlint/p4-filespec.expect                   |  2 +-
 t/chainlint/pipe.expect                          |  2 +-
 t/chainlint/semicolon.expect                     | 10 +++++-----
 t/chainlint/subshell-here-doc.expect             |  4 ++--
 t/chainlint/subshell-one-liner.expect            |  2 +-
 t/chainlint/t7900-subtree.expect                 |  6 +++---
 t/chainlint/while-loop.expect                    |  2 +-
 39 files changed, 80 insertions(+), 89 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index f5fcca09ca..2689e13636 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -76,12 +76,6 @@
 # newline, thus the closing here-doc tag has been found. The closing tag line
 # and the "<...>" prefix on the target line are then discarded, leaving just
 # the target line "cat >out".
-#
-# To facilitate regression testing (and manual debugging), a ">" annotation is
-# applied to the line containing ")" which closes a subshell, ">>" to a line
-# closing a nested subshell, and ">>>" to a line closing both at once. This
-# makes it easy to detect whether the heuristics correctly identify
-# end-of-subshell.
 #------------------------------------------------------------------------------
 
 # incomplete line -- slurp up next line
@@ -337,7 +331,6 @@ n
 x
 bnstslrp
 :nstcl
-s/^/>>/
 # is it "))" which closes nested and parent subshells?
 /)[ 	]*)/bslurp
 bchkchn
@@ -357,7 +350,6 @@ x
 s/\( ?!AMP?!\)* ?!AMP?!$//
 p
 x
-s/^/>/
 b
 
 # found closing "...)" -- exit subshell loop
@@ -365,5 +357,4 @@ b
 x
 p
 x
-s/^/>/
 b
diff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect
index 56cd5b69f5..46ee1046af 100644
--- a/t/chainlint/arithmetic-expansion.expect
+++ b/t/chainlint/arithmetic-expansion.expect
@@ -2,8 +2,8 @@
 	foo &&
 	bar=$((42 + 1)) &&
 	baz
->) &&
+) &&
 (
 	bar=$((42 + 1)) ?!AMP?!
 	baz
->)
+)
diff --git a/t/chainlint/bash-array.expect b/t/chainlint/bash-array.expect
index c4a830d1c1..4c34eaee45 100644
--- a/t/chainlint/bash-array.expect
+++ b/t/chainlint/bash-array.expect
@@ -2,9 +2,9 @@
 	foo &&
 	bar=(gumbo stumbo wumbo) &&
 	baz
->) &&
+) &&
 (
 	foo &&
 	bar=${#bar[@]} &&
 	baz
->)
+)
diff --git a/t/chainlint/blank-line.expect b/t/chainlint/blank-line.expect
index 3be939ed38..f76fde1ffb 100644
--- a/t/chainlint/blank-line.expect
+++ b/t/chainlint/blank-line.expect
@@ -1,4 +1,4 @@
 (
 	nothing &&
 	something
->)
+)
diff --git a/t/chainlint/block.expect b/t/chainlint/block.expect
index 6333237cb2..da60257ebc 100644
--- a/t/chainlint/block.expect
+++ b/t/chainlint/block.expect
@@ -9,4 +9,4 @@
 		echo c
 	} ?!AMP?!
 	baz
->)
+)
diff --git a/t/chainlint/broken-chain.expect b/t/chainlint/broken-chain.expect
index 0960a8c7c0..cfb58fb6b9 100644
--- a/t/chainlint/broken-chain.expect
+++ b/t/chainlint/broken-chain.expect
@@ -3,4 +3,4 @@
 	bar ?!AMP?!
 	baz &&
 	wop
->)
+)
diff --git a/t/chainlint/case.expect b/t/chainlint/case.expect
index a4b92d4613..31f280d8ce 100644
--- a/t/chainlint/case.expect
+++ b/t/chainlint/case.expect
@@ -4,16 +4,16 @@
 	*) bar ;;
 	esac &&
 	foobar
->) &&
+) &&
 (
 	case "$x" in
 	x) foo ;;
 	*) bar ;;
 	esac ?!AMP?!
 	foobar
->) &&
+) &&
 (
 	case "$x" in 1) true;; esac &&
 	case "$y" in 2) false;; esac ?!AMP?!
 	foobar
->)
+)
diff --git a/t/chainlint/close-nested-and-parent-together.expect b/t/chainlint/close-nested-and-parent-together.expect
index 2a910f9d66..5ef509eb49 100644
--- a/t/chainlint/close-nested-and-parent-together.expect
+++ b/t/chainlint/close-nested-and-parent-together.expect
@@ -1,4 +1,4 @@
 (
 cd foo &&
 	(bar &&
->>>		baz))
+		baz))
diff --git a/t/chainlint/close-subshell.expect b/t/chainlint/close-subshell.expect
index 184688718a..0f87db9ae6 100644
--- a/t/chainlint/close-subshell.expect
+++ b/t/chainlint/close-subshell.expect
@@ -1,25 +1,25 @@
 (
 	foo
->) &&
+) &&
 (
 	bar
->) >out &&
+) >out &&
 (
 	baz
->) 2>err &&
+) 2>err &&
 (
 	boo
->) <input &&
+) <input &&
 (
 	bip
->) | wuzzle &&
+) | wuzzle &&
 (
 	bop
->) | fazz 	fozz &&
+) | fazz 	fozz &&
 (
 	bup
->) |
+) |
 fuzzle &&
 (
 	yop
->)
+)
diff --git a/t/chainlint/command-substitution.expect b/t/chainlint/command-substitution.expect
index f276067b7b..c72e4df9e7 100644
--- a/t/chainlint/command-substitution.expect
+++ b/t/chainlint/command-substitution.expect
@@ -2,8 +2,8 @@
 	foo &&
 	bar=$(gobble) &&
 	baz
->) &&
+) &&
 (
 	bar=$(gobble blocks) ?!AMP?!
 	baz
->)
+)
diff --git a/t/chainlint/comment.expect b/t/chainlint/comment.expect
index 3be939ed38..f76fde1ffb 100644
--- a/t/chainlint/comment.expect
+++ b/t/chainlint/comment.expect
@@ -1,4 +1,4 @@
 (
 	nothing &&
 	something
->)
+)
diff --git a/t/chainlint/complex-if-in-cuddled-loop.expect b/t/chainlint/complex-if-in-cuddled-loop.expect
index 9674b88cf2..b8aa626ed0 100644
--- a/t/chainlint/complex-if-in-cuddled-loop.expect
+++ b/t/chainlint/complex-if-in-cuddled-loop.expect
@@ -6,5 +6,5 @@ for i in a b c; do
    else
      echo >file
    fi
-> done) &&
+ done) &&
 test ! -f file
diff --git a/t/chainlint/cuddled-if-then-else.expect b/t/chainlint/cuddled-if-then-else.expect
index ab2a026fbc..4e089b087a 100644
--- a/t/chainlint/cuddled-if-then-else.expect
+++ b/t/chainlint/cuddled-if-then-else.expect
@@ -3,5 +3,5 @@ if test -z ""; then
     echo empty
  else
     echo bizzy
-> fi) &&
+ fi) &&
 echo foobar
diff --git a/t/chainlint/cuddled-loop.expect b/t/chainlint/cuddled-loop.expect
index 8c0260d7f1..7932303763 100644
--- a/t/chainlint/cuddled-loop.expect
+++ b/t/chainlint/cuddled-loop.expect
@@ -1,5 +1,5 @@
 (
  while read x
   do foobar bop || exit 1
->  done <file ) &&
+  done <file ) &&
 outside subshell
diff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect
index b6c4ed90a9..773476adc8 100644
--- a/t/chainlint/cuddled.expect
+++ b/t/chainlint/cuddled.expect
@@ -1,21 +1,21 @@
 (
 cd foo &&
 	bar
->) &&
+) &&
 
 (
 cd foo ?!AMP?!
 	bar
->) &&
+) &&
 
 (
 	cd foo &&
->	bar) &&
+	bar) &&
 
 (
 cd foo &&
->	bar) &&
+	bar) &&
 
 (
 cd foo ?!AMP?!
->	bar)
+	bar)
diff --git a/t/chainlint/exit-loop.expect b/t/chainlint/exit-loop.expect
index 84d8bdebc0..f76aa60466 100644
--- a/t/chainlint/exit-loop.expect
+++ b/t/chainlint/exit-loop.expect
@@ -5,7 +5,7 @@
 		bar &&
 		baz
 	done
->) &&
+) &&
 (
 	while true
 	do
@@ -13,7 +13,7 @@
 		bar &&
 		baz
 	done
->) &&
+) &&
 (
 	i=0 &&
 	while test $i -lt 10
@@ -21,4 +21,4 @@
 		echo $i || exit
 		i=$(($i + 1))
 	done
->)
+)
diff --git a/t/chainlint/exit-subshell.expect b/t/chainlint/exit-subshell.expect
index bf78454f74..da80339f78 100644
--- a/t/chainlint/exit-subshell.expect
+++ b/t/chainlint/exit-subshell.expect
@@ -2,4 +2,4 @@
 	foo || exit 1
 	bar &&
 	baz
->)
+)
diff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect
index dc209e21bd..b74df064c5 100644
--- a/t/chainlint/for-loop.expect
+++ b/t/chainlint/for-loop.expect
@@ -8,4 +8,4 @@
 		echo $i &&
 		cat $i
 	done
->)
+)
diff --git a/t/chainlint/here-doc-close-subshell.expect b/t/chainlint/here-doc-close-subshell.expect
index f011e335e5..e748526570 100644
--- a/t/chainlint/here-doc-close-subshell.expect
+++ b/t/chainlint/here-doc-close-subshell.expect
@@ -1,2 +1,2 @@
 (
->	cat)
+	cat)
diff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect
index 3a35bb014c..f1248f8ade 100644
--- a/t/chainlint/here-doc-multi-line-command-subst.expect
+++ b/t/chainlint/here-doc-multi-line-command-subst.expect
@@ -1,5 +1,5 @@
 (
 	x=$(bobble &&
->>		wiffle) ?!AMP?!
+		wiffle) ?!AMP?!
 	echo $x
->)
+)
diff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect
index a3b9a5472d..7e883b252e 100644
--- a/t/chainlint/here-doc-multi-line-string.expect
+++ b/t/chainlint/here-doc-multi-line-string.expect
@@ -1,4 +1,4 @@
 (
 	cat && echo "multi-line	string" ?!AMP?!
 	bap
->)
+)
diff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect
index 7d91837269..03b82a3e58 100644
--- a/t/chainlint/if-in-loop.expect
+++ b/t/chainlint/if-in-loop.expect
@@ -9,4 +9,4 @@
 		foo
 	done ?!AMP?!
 	bar
->)
+)
diff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect
index 3055d5606c..debcf7b756 100644
--- a/t/chainlint/if-then-else.expect
+++ b/t/chainlint/if-then-else.expect
@@ -11,10 +11,10 @@
 		cat
 	fi ?!AMP?!
 	echo poodle
->) &&
+) &&
 (
 	if test -n ""; then
 		echo very &&
 		echo empty
 	fi
->)
+)
diff --git a/t/chainlint/incomplete-line.expect b/t/chainlint/incomplete-line.expect
index 2f3ebabdc2..ffac8f9018 100644
--- a/t/chainlint/incomplete-line.expect
+++ b/t/chainlint/incomplete-line.expect
@@ -1,4 +1,4 @@
 line 1 line 2 line 3 line 4 &&
 (
 	line 5 	line 6 	line 7 	line 8
->)
+)
diff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect
index 3d655a32b0..f6b42757d2 100644
--- a/t/chainlint/inline-comment.expect
+++ b/t/chainlint/inline-comment.expect
@@ -2,8 +2,8 @@
 	foobar &&
 	barfoo ?!AMP?!
 	flibble "not a # comment"
->) &&
+) &&
 
 (
 cd foo &&
->	flibble "not a # comment")
+	flibble "not a # comment")
diff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect
index cebd3ae95e..e1be42376c 100644
--- a/t/chainlint/loop-in-if.expect
+++ b/t/chainlint/loop-in-if.expect
@@ -9,4 +9,4 @@
 		foo
 	fi ?!AMP?!
 	bar
->)
+)
diff --git a/t/chainlint/multi-line-nested-command-substitution.expect b/t/chainlint/multi-line-nested-command-substitution.expect
index 59b6c8b850..300058341b 100644
--- a/t/chainlint/multi-line-nested-command-substitution.expect
+++ b/t/chainlint/multi-line-nested-command-substitution.expect
@@ -3,16 +3,16 @@
 	x=$(
 		echo bar |
 		cat
->>	) &&
+	) &&
 	echo ok
->) |
+) |
 sort &&
 (
 	bar &&
 	x=$(echo bar |
 		cat
->>	) &&
+	) &&
 	y=$(echo baz |
->>		fip) &&
+		fip) &&
 	echo fail
->)
+)
diff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect
index f1be2baf0a..ab0dadf748 100644
--- a/t/chainlint/multi-line-string.expect
+++ b/t/chainlint/multi-line-string.expect
@@ -2,8 +2,8 @@
 	x="line 1		line 2		line 3" &&
 	y="line 1		line2" ?!AMP?!
 	foobar
->) &&
+) &&
 (
 	echo "xyz" "abc		def		ghi" &&
 	barfoo
->)
+)
diff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect
index aa522658ed..2a86885ee6 100644
--- a/t/chainlint/nested-cuddled-subshell.expect
+++ b/t/chainlint/nested-cuddled-subshell.expect
@@ -1,19 +1,19 @@
 (
 	(cd foo &&
 		bar
->>	) &&
+	) &&
 	(cd foo &&
 		bar
->>	) ?!AMP?!
+	) ?!AMP?!
 	(
 		cd foo &&
->>		bar) &&
+		bar) &&
 	(
 		cd foo &&
->>		bar) ?!AMP?!
+		bar) ?!AMP?!
 	(cd foo &&
->>		bar) &&
+		bar) &&
 	(cd foo &&
->>		bar) ?!AMP?!
+		bar) ?!AMP?!
 	foobar
->)
+)
diff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect
index f9604d3fac..2a51205d32 100644
--- a/t/chainlint/nested-here-doc.expect
+++ b/t/chainlint/nested-here-doc.expect
@@ -4,4 +4,4 @@ cat >foop &&
 	cat &&
 	cat ?!AMP?!
 	foobar
->)
+)
diff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect
index 925e49bae9..9138cf386d 100644
--- a/t/chainlint/nested-subshell-comment.expect
+++ b/t/chainlint/nested-subshell-comment.expect
@@ -6,6 +6,6 @@
 		# minor numbers of cows (or do they?)
 		baz &&
 		snaff
->>	) ?!AMP?!
+	) ?!AMP?!
 	fuzzy
->)
+)
diff --git a/t/chainlint/nested-subshell.expect b/t/chainlint/nested-subshell.expect
index c8165ad19e..41a48adaa2 100644
--- a/t/chainlint/nested-subshell.expect
+++ b/t/chainlint/nested-subshell.expect
@@ -3,10 +3,10 @@
 	(
 		echo a &&
 		echo b
->>	) >file &&
+	) >file &&
 	cd foo &&
 	(
 		echo a
 		echo b
->>	) >file
->)
+	) >file
+)
diff --git a/t/chainlint/p4-filespec.expect b/t/chainlint/p4-filespec.expect
index 98b3d881fd..1290fd1ff2 100644
--- a/t/chainlint/p4-filespec.expect
+++ b/t/chainlint/p4-filespec.expect
@@ -1,4 +1,4 @@
 (
 	p4 print -1 //depot/fiddle#42 >file &&
 	foobar
->)
+)
diff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect
index ede6bcc607..2cfc028297 100644
--- a/t/chainlint/pipe.expect
+++ b/t/chainlint/pipe.expect
@@ -5,4 +5,4 @@
 	fish |
 	cow ?!AMP?!
 	sunder
->)
+)
diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect
index 54a08ce582..05141a96cf 100644
--- a/t/chainlint/semicolon.expect
+++ b/t/chainlint/semicolon.expect
@@ -1,20 +1,20 @@
 (
 	cat foo ; ?!AMP?! echo bar ?!AMP?!
 	cat foo ; ?!AMP?! echo bar
->) &&
+) &&
 (
 	cat foo ; ?!AMP?! echo bar &&
 	cat foo ; ?!AMP?! echo bar
->) &&
+) &&
 (
 	echo "foo; bar" &&
 	cat foo; ?!AMP?! echo bar
->) &&
+) &&
 (
 	foo;
->) &&
+) &&
 (
 cd foo &&
 	for i in a b c; do
 		echo;
->	done)
+	done)
diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index 9d3f25b3f5..b7250ca753 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -2,9 +2,9 @@
 	echo wobba 	       gorgo snoot 	       wafta snurb &&
 	cat >bip ?!AMP?!
 	echo >bop
->) &&
+) &&
 (
 	cat >bup &&
 	cat >bup3 &&
 	meep
->)
+)
diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect
index 4b44632b09..b7015361bf 100644
--- a/t/chainlint/subshell-one-liner.expect
+++ b/t/chainlint/subshell-one-liner.expect
@@ -11,4 +11,4 @@
 	(foo && bar) ?!AMP?!
 	(foo && bar; ?!AMP?! baz) ?!AMP?!
 	foobar
->)
+)
diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect
index f769244ef6..215aca01c2 100644
--- a/t/chainlint/t7900-subtree.expect
+++ b/t/chainlint/t7900-subtree.expect
@@ -1,10 +1,10 @@
 (
 	chks="sub1sub2sub3sub4" &&
 	chks_sub=$(cat | sed "s,^,sub dir/,"
->>) &&
+) &&
 	chkms="main-sub1main-sub2main-sub3main-sub4" &&
 	chkms_sub=$(cat | sed "s,^,sub dir/,"
->>) &&
+) &&
 	subfiles=$(git ls-files) &&
 	check_equal "$subfiles" "$chkms$chks"
->)
+)
diff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect
index f8b9fcf62b..e2813b378e 100644
--- a/t/chainlint/while-loop.expect
+++ b/t/chainlint/while-loop.expect
@@ -8,4 +8,4 @@
 		echo foo &&
 		cat bar
 	done
->)
+)
-- 
2.34.1.397.gfae76fe5da

[PATCH 11/15] chainlint.sed: make here-doc "<<-" operator recognition more POSIX-like

[Eric Sunshine]

According to POSIX, "<<" and "<<-" are distinct shell operators. For the
latter to be recognized, no whitespace is allowed before the "-", though
whitespace is allowed after the operator. However, the chainlint
patterns which identify here-docs are both too loose and too tight,
incorrectly allowing whitespace between "<<" and "-" but disallowing it
between "-" and the here-doc tag. Fix the patterns to better match
POSIX.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.sed | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index 2689e13636..b382746526 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -88,8 +88,8 @@
 
 # here-doc -- swallow it to avoid false hits within its body (but keep the
 # command to which it was attached)
-/<<[ 	]*[-\\'"]*[A-Za-z0-9_]/ {
-	s/^\(.*\)<<[ 	]*[-\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<</
+/<<-*[ 	]*[\\'"]*[A-Za-z0-9_]/ {
+	s/^\(.*\)<<-*[ 	]*[\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<</
 	s/[ 	]*<<//
 	:hered
 	N
@@ -152,7 +152,7 @@ s/.*\n//
 }
 :folded
 # here-doc -- swallow it
-/<<[ 	]*[-\\'"]*[A-Za-z0-9_]/bheredoc
+/<<-*[ 	]*[\\'"]*[A-Za-z0-9_]/bheredoc
 # comment or empty line -- discard since final non-comment, non-empty line
 # before closing ")", "done", "elsif", "else", or "fi" will need to be
 # re-visited to drop "suspect" marking since final line of those constructs
@@ -274,7 +274,7 @@ bfolded
 # found here-doc -- swallow it to avoid false hits within its body (but keep
 # the command to which it was attached)
 :heredoc
-s/^\(.*\)<<[ 	]*[-\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<</
+s/^\(.*\)<<-*[ 	]*[\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<</
 s/[ 	]*<<//
 :hdocsub
 N
-- 
2.34.1.397.gfae76fe5da

[PATCH 12/15] chainlint.sed: don't mistake `<< word` in string as here-doc operator

[Eric Sunshine]

Tighten here-doc recognition to prevent it from being fooled by text
which looks like a here-doc operator but happens merely to be the
content of a string, such as this real-world case from t7201:

    echo "<<<<<<< ours" &&
    echo ourside &&
    echo "=======" &&
    echo theirside &&
    echo ">>>>>>> theirs"

This problem went unnoticed because chainlint.sed is not a real parser,
but rather applies heuristics to pretend to understand shell code. In
this case, it saw what it thought was a here-doc operator (`<< ours`),
and fell off the end of the test looking for the closing tag "ours"
which it never found, thus swallowed the remainder of the test without
checking it for &&-chain breakage.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.sed                |  8 ++++++--
 t/chainlint/not-heredoc.expect | 14 ++++++++++++++
 t/chainlint/not-heredoc.test   | 16 ++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)
 create mode 100644 t/chainlint/not-heredoc.expect
 create mode 100644 t/chainlint/not-heredoc.test

diff --git a/t/chainlint.sed b/t/chainlint.sed
index b382746526..2f786f890d 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -89,6 +89,7 @@
 # here-doc -- swallow it to avoid false hits within its body (but keep the
 # command to which it was attached)
 /<<-*[ 	]*[\\'"]*[A-Za-z0-9_]/ {
+	/"[^"]*<<[^"]*"/bnotdoc
 	s/^\(.*\)<<-*[ 	]*[\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<</
 	s/[ 	]*<<//
 	:hered
@@ -100,6 +101,7 @@
 	s/^<[^>]*>//
 	s/\n.*$//
 }
+:notdoc
 
 # one-liner "(...) &&"
 /^[ 	]*!*[ 	]*(..*)[ 	]*&&[ 	]*$/boneline
@@ -151,8 +153,10 @@ s/.*\n//
 	/"[^'"]*'[^'"]*"/!bsqstr
 }
 :folded
-# here-doc -- swallow it
-/<<-*[ 	]*[\\'"]*[A-Za-z0-9_]/bheredoc
+# here-doc -- swallow it (but not "<<" in a string)
+/<<-*[ 	]*[\\'"]*[A-Za-z0-9_]/{
+	/"[^"]*<<[^"]*"/!bheredoc
+}
 # comment or empty line -- discard since final non-comment, non-empty line
 # before closing ")", "done", "elsif", "else", or "fi" will need to be
 # re-visited to drop "suspect" marking since final line of those constructs
diff --git a/t/chainlint/not-heredoc.expect b/t/chainlint/not-heredoc.expect
new file mode 100644
index 0000000000..2e9bb135fe
--- /dev/null
+++ b/t/chainlint/not-heredoc.expect
@@ -0,0 +1,14 @@
+echo "<<<<<<< ours" &&
+echo ourside &&
+echo "=======" &&
+echo theirside &&
+echo ">>>>>>> theirs" &&
+
+(
+	echo "<<<<<<< ours" &&
+	echo ourside &&
+	echo "=======" &&
+	echo theirside &&
+	echo ">>>>>>> theirs" ?!AMP?!
+	poodle
+) >merged
diff --git a/t/chainlint/not-heredoc.test b/t/chainlint/not-heredoc.test
new file mode 100644
index 0000000000..9aa57346cd
--- /dev/null
+++ b/t/chainlint/not-heredoc.test
@@ -0,0 +1,16 @@
+# LINT: "<< ours" inside string is not here-doc
+echo "<<<<<<< ours" &&
+echo ourside &&
+echo "=======" &&
+echo theirside &&
+echo ">>>>>>> theirs" &&
+
+(
+# LINT: "<< ours" inside string is not here-doc
+	echo "<<<<<<< ours" &&
+	echo ourside &&
+	echo "=======" &&
+	echo theirside &&
+	echo ">>>>>>> theirs"
+	poodle
+) >merged
-- 
2.34.1.397.gfae76fe5da

[PATCH 13/15] chainlint.sed: stop throwing away here-doc tags

[Eric Sunshine]

The purpose of chainlint is to highlight problems it finds in test code
by inserting annotations at the location of each problem. Arbitrarily
eliding bits of the code it is checking is not helpful, yet this is
exactly what chainlint.sed does by cavalierly and unnecessarily dropping
the here-doc operator and tag; i.e. `cat <<TAG` becomes simply `cat` in
the output. This behavior can make it more difficult for the test writer
to align the annotated output of chainlint.sed with the original test
code. Address this by retaining here-doc tags.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.sed                               | 24 ++++++++++++-------
 t/chainlint/for-loop.expect                   |  2 +-
 t/chainlint/here-doc-close-subshell.expect    |  2 +-
 .../here-doc-multi-line-command-subst.expect  |  2 +-
 t/chainlint/here-doc-multi-line-string.expect |  2 +-
 t/chainlint/here-doc.expect                   |  8 +++----
 t/chainlint/if-then-else.expect               |  2 +-
 t/chainlint/nested-here-doc.expect            |  6 ++---
 t/chainlint/subshell-here-doc.expect          | 10 ++++----
 t/chainlint/t7900-subtree.expect              |  4 ++--
 t/chainlint/while-loop.expect                 |  2 +-
 11 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index 2f786f890d..0e575c0c62 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -62,20 +62,20 @@
 # receives similar treatment.
 #
 # Swallowing here-docs with arbitrary tags requires a bit of finesse. When a
-# line such as "cat <<EOF >out" is seen, the here-doc tag is moved to the front
-# of the line enclosed in angle brackets as a sentinel, giving "<EOF>cat >out".
+# line such as "cat <<EOF" is seen, the here-doc tag is copied to the front of
+# the line enclosed in angle brackets as a sentinel, giving "<EOF>cat <<EOF".
 # As each subsequent line is read, it is appended to the target line and a
 # (whitespace-loose) back-reference match /^<(.*)>\n\1$/ is attempted to see if
 # the content inside "<...>" matches the entirety of the newly-read line. For
 # instance, if the next line read is "some data", when concatenated with the
-# target line, it becomes "<EOF>cat >out\nsome data", and a match is attempted
+# target line, it becomes "<EOF>cat <<EOF\nsome data", and a match is attempted
 # to see if "EOF" matches "some data". Since it doesn't, the next line is
 # attempted. When a line consisting of only "EOF" (and possible whitespace) is
-# encountered, it is appended to the target line giving "<EOF>cat >out\nEOF",
+# encountered, it is appended to the target line giving "<EOF>cat <<EOF\nEOF",
 # in which case the "EOF" inside "<...>" does match the text following the
 # newline, thus the closing here-doc tag has been found. The closing tag line
 # and the "<...>" prefix on the target line are then discarded, leaving just
-# the target line "cat >out".
+# the target line "cat <<EOF".
 #------------------------------------------------------------------------------
 
 # incomplete line -- slurp up next line
@@ -90,8 +90,7 @@
 # command to which it was attached)
 /<<-*[ 	]*[\\'"]*[A-Za-z0-9_]/ {
 	/"[^"]*<<[^"]*"/bnotdoc
-	s/^\(.*\)<<-*[ 	]*[\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<</
-	s/[ 	]*<<//
+	s/^\(.*<<-*[ 	]*\)[\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1\2/
 	:hered
 	N
 	/^<\([^>]*\)>.*\n[ 	]*\1[ 	]*$/!{
@@ -238,6 +237,7 @@ s/.*\n//
 :cont
 # retrieve and print previous line
 x
+s/?!HERE?!/<</g
 n
 bslurp
 
@@ -278,8 +278,7 @@ bfolded
 # found here-doc -- swallow it to avoid false hits within its body (but keep
 # the command to which it was attached)
 :heredoc
-s/^\(.*\)<<-*[ 	]*[\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<</
-s/[ 	]*<<//
+s/^\(.*\)<<\(-*[ 	]*\)[\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\3>\1?!HERE?!\2\3/
 :hdocsub
 N
 /^<\([^>]*\)>.*\n[ 	]*\1[ 	]*$/!{
@@ -293,6 +292,7 @@ bfolded
 # found "case ... in" -- pass through untouched
 :case
 x
+s/?!HERE?!/<</g
 n
 /^[ 	]*esac/bslurp
 bcase
@@ -320,6 +320,7 @@ bchkchn
 :nest
 x
 :nstslrp
+s/?!HERE?!/<</g
 n
 # closing ")" on own line -- stop nested slurp
 /^[ 	]*)/bnstcl
@@ -342,6 +343,7 @@ bchkchn
 # found multi-line "{...\n...}" block -- pass through untouched
 :block
 x
+s/?!HERE?!/<</g
 n
 # closing "}" -- stop block slurp
 /}/bchkchn
@@ -352,13 +354,17 @@ bblock
 :clssolo
 x
 s/\( ?!AMP?!\)* ?!AMP?!$//
+s/?!HERE?!/<</g
 p
 x
+s/?!HERE?!/<</g
 b
 
 # found closing "...)" -- exit subshell loop
 :close
 x
+s/?!HERE?!/<</g
 p
 x
+s/?!HERE?!/<</g
 b
diff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect
index b74df064c5..6671b8cd84 100644
--- a/t/chainlint/for-loop.expect
+++ b/t/chainlint/for-loop.expect
@@ -2,7 +2,7 @@
 	for i in a b c
 	do
 		echo $i ?!AMP?!
-		cat
+		cat <<-EOF
 	done ?!AMP?!
 	for i in a b c; do
 		echo $i &&
diff --git a/t/chainlint/here-doc-close-subshell.expect b/t/chainlint/here-doc-close-subshell.expect
index e748526570..2af9ced71c 100644
--- a/t/chainlint/here-doc-close-subshell.expect
+++ b/t/chainlint/here-doc-close-subshell.expect
@@ -1,2 +1,2 @@
 (
-	cat)
+	cat <<-INPUT)
diff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect
index f1248f8ade..f8b3aa73c4 100644
--- a/t/chainlint/here-doc-multi-line-command-subst.expect
+++ b/t/chainlint/here-doc-multi-line-command-subst.expect
@@ -1,5 +1,5 @@
 (
-	x=$(bobble &&
+	x=$(bobble <<-END &&
 		wiffle) ?!AMP?!
 	echo $x
 )
diff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect
index 7e883b252e..2578191ca8 100644
--- a/t/chainlint/here-doc-multi-line-string.expect
+++ b/t/chainlint/here-doc-multi-line-string.expect
@@ -1,4 +1,4 @@
 (
-	cat && echo "multi-line	string" ?!AMP?!
+	cat <<-TXT && echo "multi-line	string" ?!AMP?!
 	bap
 )
diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect
index 8449eb2e02..110059ba58 100644
--- a/t/chainlint/here-doc.expect
+++ b/t/chainlint/here-doc.expect
@@ -1,7 +1,7 @@
-boodle wobba        gorgo snoot        wafta snurb &&
+boodle wobba        gorgo snoot        wafta snurb <<EOF &&
 
-cat >foo &&
+cat <<-Arbitrary_Tag_42 >foo &&
 
-cat >boo &&
+cat <<zump >boo &&
 
-horticulture
+horticulture <<EOF
diff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect
index debcf7b756..44d86c3597 100644
--- a/t/chainlint/if-then-else.expect
+++ b/t/chainlint/if-then-else.expect
@@ -8,7 +8,7 @@
 		echo foo
 	else
 		echo foo &&
-		cat
+		cat <<-EOF
 	fi ?!AMP?!
 	echo poodle
 ) &&
diff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect
index 2a51205d32..e3bef63f75 100644
--- a/t/chainlint/nested-here-doc.expect
+++ b/t/chainlint/nested-here-doc.expect
@@ -1,7 +1,7 @@
-cat >foop &&
+cat <<ARBITRARY >foop &&
 
 (
-	cat &&
-	cat ?!AMP?!
+	cat <<-INPUT_END &&
+	cat <<-EOT ?!AMP?!
 	foobar
 )
diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect
index b7250ca753..029d129299 100644
--- a/t/chainlint/subshell-here-doc.expect
+++ b/t/chainlint/subshell-here-doc.expect
@@ -1,10 +1,10 @@
 (
-	echo wobba 	       gorgo snoot 	       wafta snurb &&
-	cat >bip ?!AMP?!
-	echo >bop
+	echo wobba 	       gorgo snoot 	       wafta snurb <<-EOF &&
+	cat <<EOF >bip ?!AMP?!
+	echo <<-EOF >bop
 ) &&
 (
-	cat >bup &&
-	cat >bup3 &&
+	cat <<-ARBITRARY >bup &&
+	cat <<-ARBITRARY3 >bup3 &&
 	meep
 )
diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect
index 215aca01c2..1cccc7bf7e 100644
--- a/t/chainlint/t7900-subtree.expect
+++ b/t/chainlint/t7900-subtree.expect
@@ -1,9 +1,9 @@
 (
 	chks="sub1sub2sub3sub4" &&
-	chks_sub=$(cat | sed "s,^,sub dir/,"
+	chks_sub=$(cat <<TXT | sed "s,^,sub dir/,"
 ) &&
 	chkms="main-sub1main-sub2main-sub3main-sub4" &&
-	chkms_sub=$(cat | sed "s,^,sub dir/,"
+	chkms_sub=$(cat <<TXT | sed "s,^,sub dir/,"
 ) &&
 	subfiles=$(git ls-files) &&
 	check_equal "$subfiles" "$chkms$chks"
diff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect
index e2813b378e..0d3a9b3d12 100644
--- a/t/chainlint/while-loop.expect
+++ b/t/chainlint/while-loop.expect
@@ -2,7 +2,7 @@
 	while true
 	do
 		echo foo ?!AMP?!
-		cat
+		cat <<-EOF
 	done ?!AMP?!
 	while true; do
 		echo foo &&
-- 
2.34.1.397.gfae76fe5da

[PATCH 14/15] chainlint.sed: swallow comments consistently

[Eric Sunshine]

When checking for broken a &&-chain, chainlint.sed knows that the final
statement in a subshell should not end with `&&`, so it takes care to
make a distinction between the final line which is an actual statement
and any lines which may be mere comments preceding the closing ')'. As
such, it swallows comment lines so that they do not interfere with the
&&-chain check.

However, since `sed` does not provide any sort of real recursion,
chainlint.sed only checks &&-chains in subshells one level deep; it
doesn't do any checking in deeper subshells or in `{...}` blocks within
subshells. Furthermore, on account of potential implementation
complexity, it doesn't check &&-chains within `case` arms.

Due to an oversight, it also doesn't swallow comments inside deep
subshells, `{...}` blocks, or `case` statements, which makes its output
inconsistent (swallowing comments in some cases but not others).
Unfortunately, this inconsistency seeps into the chainlint self-test
"expect" files, which potentially makes it difficult to reuse the
self-tests should a more capable chainlint ever be developed. Therefore,
teach chainlint.sed to consistently swallow comments in all cases.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/chainlint.sed                            | 21 +++++++++++++++++++--
 t/chainlint/block-comment.expect           |  6 ++++++
 t/chainlint/block-comment.test             |  8 ++++++++
 t/chainlint/case-comment.expect            |  8 ++++++++
 t/chainlint/case-comment.test              | 11 +++++++++++
 t/chainlint/nested-subshell-comment.expect |  2 --
 6 files changed, 52 insertions(+), 4 deletions(-)
 create mode 100644 t/chainlint/block-comment.expect
 create mode 100644 t/chainlint/block-comment.test
 create mode 100644 t/chainlint/case-comment.expect
 create mode 100644 t/chainlint/case-comment.test

diff --git a/t/chainlint.sed b/t/chainlint.sed
index 0e575c0c62..b1505ef2cd 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -294,6 +294,12 @@ bfolded
 x
 s/?!HERE?!/<</g
 n
+:cascom
+/^[ 	]*#/{
+	N
+	s/.*\n//
+	bcascom
+}
 /^[ 	]*esac/bslurp
 bcase
 
@@ -322,10 +328,15 @@ x
 :nstslrp
 s/?!HERE?!/<</g
 n
+:nstcom
+# comment -- not closing ")" if in comment
+/^[ 	]*#/{
+	N
+	s/.*\n//
+	bnstcom
+}
 # closing ")" on own line -- stop nested slurp
 /^[ 	]*)/bnstcl
-# comment -- not closing ")" if in comment
-/^[ 	]*#/bnstcnt
 # "$((...))" -- arithmetic expansion; not closing ")"
 /\$(([^)][^)]*))[^)]*$/bnstcnt
 # "$(...)" -- command substitution; not closing ")"
@@ -345,6 +356,12 @@ bchkchn
 x
 s/?!HERE?!/<</g
 n
+:blkcom
+/^[ 	]*#/{
+	N
+	s/.*\n//
+	bblkcom
+}
 # closing "}" -- stop block slurp
 /}/bchkchn
 bblock
diff --git a/t/chainlint/block-comment.expect b/t/chainlint/block-comment.expect
new file mode 100644
index 0000000000..d10b2eeaf2
--- /dev/null
+++ b/t/chainlint/block-comment.expect
@@ -0,0 +1,6 @@
+(
+	{
+		echo a &&
+		echo b
+	}
+)
diff --git a/t/chainlint/block-comment.test b/t/chainlint/block-comment.test
new file mode 100644
index 0000000000..df2beea888
--- /dev/null
+++ b/t/chainlint/block-comment.test
@@ -0,0 +1,8 @@
+(
+	{
+		# show a
+		echo a &&
+		# show b
+		echo b
+	}
+)
diff --git a/t/chainlint/case-comment.expect b/t/chainlint/case-comment.expect
new file mode 100644
index 0000000000..1e4b054bda
--- /dev/null
+++ b/t/chainlint/case-comment.expect
@@ -0,0 +1,8 @@
+(
+	case "$x" in
+	x) foo ;;
+	*)
+		bar
+		;;
+	esac
+)
diff --git a/t/chainlint/case-comment.test b/t/chainlint/case-comment.test
new file mode 100644
index 0000000000..641c157b98
--- /dev/null
+++ b/t/chainlint/case-comment.test
@@ -0,0 +1,11 @@
+(
+	case "$x" in
+	# found foo
+	x) foo ;;
+	# found other
+	*)
+		# treat it as bar
+		bar
+		;;
+	esac
+)
diff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect
index 9138cf386d..be4b27a305 100644
--- a/t/chainlint/nested-subshell-comment.expect
+++ b/t/chainlint/nested-subshell-comment.expect
@@ -2,8 +2,6 @@
 	foo &&
 	(
 		bar &&
-		# bottles wobble while fiddles gobble
-		# minor numbers of cows (or do they?)
 		baz &&
 		snaff
 	) ?!AMP?!
-- 
2.34.1.397.gfae76fe5da

[PATCH 15/15] chainlint.sed: stop splitting "(..." into separate lines "(" and "..."

[Eric Sunshine]

Because `sed` is line-oriented, for ease of implementation, when
chainlint.sed encounters an opening subshell in which the first command
is cuddled with the "(", it splits the line into two lines: one
containing only "(", and the other containing whatever follows "(".
This allows chainlint.sed to get by with a single set of regular
expressions for matching shell statements rather than having to
duplicate each expression (one set for matching non-cuddled statements,
and one set for matching cuddled statements).

However, although syntactically and semantically immaterial, this
transformation has no value to test authors and might even confuse them
into thinking that the linter is misbehaving by inserting (whitespace)
line-noise into the shell code it is validating. Moreover, it also
allows an implementation detail of chainlint.sed to seep into the
chainlint self-test "expect" files, which potentially makes it difficult
to reuse the self-tests should a more capable chainlint ever be
developed.

To address these concerns, stop splitting cuddled "(..." into two lines.

Note that, as an implementation artifact, due to sed's line-oriented
nature, this change inserts a blank line at output time just before the
"(..." line is emitted. It would be possible to suppress this blank line
but doing so would add a fair bit of complexity to chainlint.sed.
Therefore, rather than suppressing the extra blank line, the Makefile's
`check-chainlint` target which runs the chainlint self-tests is instead
modified to ignore blank lines when comparing chainlint output against
the self-test "expect" output. This is a reasonable compromise for two
reasons. First, the purpose of the chainlint self-tests is to verify
that the ?!AMP?! annotations are being correctly added; precise
whitespace is immaterial. Second, by necessity, chainlint.sed itself
already throws away all blank lines within subshells since, when
checking for a broken &&-chain, it needs to check the final _statement_
in a subshell, not the final _line_ (which might be blank), thus it has
never made any attempt to precisely reproduce blank lines in its output.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/Makefile                                    |  4 +-
 t/chainlint.sed                               | 38 ++++++++++++-------
 .../close-nested-and-parent-together.expect   |  3 +-
 t/chainlint/complex-if-in-cuddled-loop.expect |  3 +-
 t/chainlint/cuddled-if-then-else.expect       |  3 +-
 t/chainlint/cuddled-loop.expect               |  3 +-
 t/chainlint/cuddled.expect                    | 12 ++----
 t/chainlint/inline-comment.expect             |  3 +-
 t/chainlint/semicolon.expect                  |  3 +-
 9 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index f4ae40be46..46cd5fc527 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -72,8 +72,8 @@ clean-chainlint:
 check-chainlint:
 	@mkdir -p '$(CHAINLINTTMP_SQ)' && \
 	sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \
-	cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
-	$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \
+	sed -e '/^[ 	]*$$/d' $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \
+	$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests | grep -v '^[	]*$$' >'$(CHAINLINTTMP_SQ)'/actual && \
 	diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
 
 test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
diff --git a/t/chainlint.sed b/t/chainlint.sed
index b1505ef2cd..dc4ce37cb5 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -131,11 +131,15 @@ b
 	h
 	bnextln
 }
-# "(..." line -- split off and stash "(", then process "..." as its own line
+# "(..." line -- "(" opening subshell cuddled with command; temporarily replace
+# "(" with sentinel "^" and process the line as if "(" had been seen solo on
+# the preceding line; this temporary replacement prevents several rules from
+# accidentally thinking "(" introduces a nested subshell; "^" is changed back
+# to "(" at output time
 x
-s/.*/(/
+s/.*//
 x
-s/(//
+s/(/^/
 bslurp
 
 :nextln
@@ -168,12 +172,12 @@ s/.*\n//
 	/"[^"]*#[^"]*"/!s/[ 	]#.*$//
 }
 # one-liner "case ... esac"
-/^[ 	]*case[ 	]*..*esac/bchkchn
+/^[ 	^]*case[ 	]*..*esac/bchkchn
 # multi-line "case ... esac"
-/^[ 	]*case[ 	]..*[ 	]in/bcase
+/^[ 	^]*case[ 	]..*[ 	]in/bcase
 # multi-line "for ... done" or "while ... done"
-/^[ 	]*for[ 	]..*[ 	]in/bcont
-/^[ 	]*while[ 	]/bcont
+/^[ 	^]*for[ 	]..*[ 	]in/bcont
+/^[ 	^]*while[ 	]/bcont
 /^[ 	]*do[ 	]/bcont
 /^[ 	]*do[ 	]*$/bcont
 /;[ 	]*do/bcont
@@ -184,7 +188,7 @@ s/.*\n//
 /||[ 	]*exit[ 	]/bcont
 /||[ 	]*exit[ 	]*$/bcont
 # multi-line "if...elsif...else...fi"
-/^[ 	]*if[ 	]/bcont
+/^[ 	^]*if[ 	]/bcont
 /^[ 	]*then[ 	]/bcont
 /^[ 	]*then[ 	]*$/bcont
 /;[ 	]*then/bcont
@@ -197,15 +201,15 @@ s/.*\n//
 /^[ 	]*fi[ 	]*[<>|]/bdone
 /^[ 	]*fi[ 	]*)/bdone
 # nested one-liner "(...) &&"
-/^[ 	]*(.*)[ 	]*&&[ 	]*$/bchkchn
+/^[ 	^]*(.*)[ 	]*&&[ 	]*$/bchkchn
 # nested one-liner "(...)"
-/^[ 	]*(.*)[ 	]*$/bchkchn
+/^[ 	^]*(.*)[ 	]*$/bchkchn
 # nested one-liner "(...) >x" (or "2>x" or "<x" or "|x")
-/^[ 	]*(.*)[ 	]*[0-9]*[<>|]/bchkchn
+/^[ 	^]*(.*)[ 	]*[0-9]*[<>|]/bchkchn
 # nested multi-line "(...\n...)"
-/^[ 	]*(/bnest
+/^[ 	^]*(/bnest
 # multi-line "{...\n...}"
-/^[ 	]*{/bblock
+/^[ 	^]*{/bblock
 # closing ")" on own line -- exit subshell
 /^[ 	]*)/bclssolo
 # "$((...))" -- arithmetic expansion; not closing ")"
@@ -237,6 +241,7 @@ s/.*\n//
 :cont
 # retrieve and print previous line
 x
+s/^\([ 	]*\)^/\1(/
 s/?!HERE?!/<</g
 n
 bslurp
@@ -292,6 +297,7 @@ bfolded
 # found "case ... in" -- pass through untouched
 :case
 x
+s/^\([ 	]*\)^/\1(/
 s/?!HERE?!/<</g
 n
 :cascom
@@ -326,6 +332,7 @@ bchkchn
 :nest
 x
 :nstslrp
+s/^\([ 	]*\)^/\1(/
 s/?!HERE?!/<</g
 n
 :nstcom
@@ -354,6 +361,7 @@ bchkchn
 # found multi-line "{...\n...}" block -- pass through untouched
 :block
 x
+s/^\([ 	]*\)^/\1(/
 s/?!HERE?!/<</g
 n
 :blkcom
@@ -371,17 +379,21 @@ bblock
 :clssolo
 x
 s/\( ?!AMP?!\)* ?!AMP?!$//
+s/^\([ 	]*\)^/\1(/
 s/?!HERE?!/<</g
 p
 x
+s/^\([ 	]*\)^/\1(/
 s/?!HERE?!/<</g
 b
 
 # found closing "...)" -- exit subshell loop
 :close
 x
+s/^\([ 	]*\)^/\1(/
 s/?!HERE?!/<</g
 p
 x
+s/^\([ 	]*\)^/\1(/
 s/?!HERE?!/<</g
 b
diff --git a/t/chainlint/close-nested-and-parent-together.expect b/t/chainlint/close-nested-and-parent-together.expect
index 5ef509eb49..72d482f76d 100644
--- a/t/chainlint/close-nested-and-parent-together.expect
+++ b/t/chainlint/close-nested-and-parent-together.expect
@@ -1,4 +1,3 @@
-(
-cd foo &&
+(cd foo &&
 	(bar &&
 		baz))
diff --git a/t/chainlint/complex-if-in-cuddled-loop.expect b/t/chainlint/complex-if-in-cuddled-loop.expect
index b8aa626ed0..2fca183409 100644
--- a/t/chainlint/complex-if-in-cuddled-loop.expect
+++ b/t/chainlint/complex-if-in-cuddled-loop.expect
@@ -1,5 +1,4 @@
-(
-for i in a b c; do
+(for i in a b c; do
    if test "$(echo $(waffle bat))" = "eleventeen" &&
      test "$x" = "$y"; then
      :
diff --git a/t/chainlint/cuddled-if-then-else.expect b/t/chainlint/cuddled-if-then-else.expect
index 4e089b087a..1d8ed58c49 100644
--- a/t/chainlint/cuddled-if-then-else.expect
+++ b/t/chainlint/cuddled-if-then-else.expect
@@ -1,5 +1,4 @@
-(
-if test -z ""; then
+(if test -z ""; then
     echo empty
  else
     echo bizzy
diff --git a/t/chainlint/cuddled-loop.expect b/t/chainlint/cuddled-loop.expect
index 7932303763..9cf260708e 100644
--- a/t/chainlint/cuddled-loop.expect
+++ b/t/chainlint/cuddled-loop.expect
@@ -1,5 +1,4 @@
-(
- while read x
+( while read x
   do foobar bop || exit 1
   done <file ) &&
 outside subshell
diff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect
index 773476adc8..c3e0be4047 100644
--- a/t/chainlint/cuddled.expect
+++ b/t/chainlint/cuddled.expect
@@ -1,10 +1,8 @@
-(
-cd foo &&
+(cd foo &&
 	bar
 ) &&
 
-(
-cd foo ?!AMP?!
+(cd foo ?!AMP?!
 	bar
 ) &&
 
@@ -12,10 +10,8 @@ cd foo ?!AMP?!
 	cd foo &&
 	bar) &&
 
-(
-cd foo &&
+(cd foo &&
 	bar) &&
 
-(
-cd foo ?!AMP?!
+(cd foo ?!AMP?!
 	bar)
diff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect
index f6b42757d2..dd0dace077 100644
--- a/t/chainlint/inline-comment.expect
+++ b/t/chainlint/inline-comment.expect
@@ -4,6 +4,5 @@
 	flibble "not a # comment"
 ) &&
 
-(
-cd foo &&
+(cd foo &&
 	flibble "not a # comment")
diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect
index 05141a96cf..ed0b3707ae 100644
--- a/t/chainlint/semicolon.expect
+++ b/t/chainlint/semicolon.expect
@@ -13,8 +13,7 @@
 (
 	foo;
 ) &&
-(
-cd foo &&
+(cd foo &&
 	for i in a b c; do
 		echo;
 	done)
-- 
2.34.1.397.gfae76fe5da

Re: [PATCH 00/15] generalize chainlint self-tests

[Elijah Newren]

On Sun, Dec 12, 2021 at 10:31 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> A while back, Peff successfully nerd-sniped[1] me into tackling a
> long-brewing idea I had about (possibly) improving `chainlint`
> performance by linting all tests in all scripts with a single command
> invocation instead of running `sed` 25300+ times (once for each test).
> A description of some of the important features of the new linter can be
> found at [2].
>
> Although the new chainlint implementation has been complete for several
> months, I'm still working out how to organize its patch series. In the
> meantime, _this_ patch series makes it possible for the new linter to
> re-use the existing chainlint self-tests. It does so by cleansing the
> self-test ".test" and ".expect" files of implementation details and
> limitations specific to chainlint.sed.

I read through the patches in this series, and didn't note any
problems.  However, my knowledge of sed is just the basics.  Even in a
few cases where regexes were all that were really involved, the
regexes were lengthy enough that my eyes just glazed over.  So, my
review is kind of superficial, but the preparatory patches certainly
seem good to me, and the commit messages are well explained, and the
non-sed changes are consistent with the described changes, and the
easier sed stuff looked good.  It's clear you put a lot of care into
carefully explaining and dividing up the patches in a nice and logical
manner.

Re: [PATCH 00/15] generalize chainlint self-tests

[Eric Sunshine]

On Tue, Dec 14, 2021 at 7:00 PM Elijah Newren <newren@gmail.com> wrote:
> On Sun, Dec 12, 2021 at 10:31 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > Although the new chainlint implementation has been complete for several
> > months, I'm still working out how to organize its patch series. In the
> > meantime, _this_ patch series makes it possible for the new linter to
> > re-use the existing chainlint self-tests. It does so by cleansing the
> > self-test ".test" and ".expect" files of implementation details and
> > limitations specific to chainlint.sed.
>
> I read through the patches in this series, and didn't note any
> problems.  However, my knowledge of sed is just the basics.  Even in a
> few cases where regexes were all that were really involved, the
> regexes were lengthy enough that my eyes just glazed over.  So, my
> review is kind of superficial, but the preparatory patches certainly
> seem good to me, and the commit messages are well explained, and the
> non-sed changes are consistent with the described changes, and the
> easier sed stuff looked good.  It's clear you put a lot of care into
> carefully explaining and dividing up the patches in a nice and logical
> manner.

Thanks, Elijah, for reading through the series. I wasn't necessarily
expecting anyone to read the patches carefully, especially the
`sed`-specific changes[*] since it's such an out-of-the-way part of
the project, but it's nice to know that the time I put into organizing
the series and writing the commit messages wasn't wasted.

[*] With the comments stripped out, the entire chainlint.sed script
does quite a good job of emulating gobs of line-noise burped up by an
old dial-up modem, so it's no surprise your eyes glazed over.