{"thread":{"id":"57072","subject":"[PATCH 00/15] generalize chainlint self-tests","startedAt":"2021-12-13T06:32:00Z","lastAt":"2021-12-16T20:10:25Z","messageCount":34,"participants":["Eric Sunshine","Fabian Stelzer","Ævar Arnfjörð Bjarmason","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"443946","messageId":"20211213063059.19424-1-sunshine@sunshineco.com","threadId":"57072","inReplyTo":null,"subject":"[PATCH 00/15] generalize chainlint self-tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:44Z","receivedAt":"2021-12-13T06:32:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"A while back, Peff successfully nerd-sniped[1] me into tackling a\nlong-brewing idea I had about (possibly) improving `chainlint`\nperformance by linting all tests in all scripts with a single command\ninvocation instead of running `sed` 25300+ times (once for each test).\nA description of some of the important features of the new linter can be\nfound at [2].\n\nAlthough the new chainlint implementation has been complete for several\nmonths, I'm still working out how to organize its patch series. In the\nmeantime, _this_ patch series makes it possible for the new linter to\nre-use the existing chainlint self-tests. It does so by cleansing the\nself-test \".test\" and \".expect\" files of implementation details and\nlimitations specific to chainlint.sed.\n\nI'm sending this series separate from the (upcoming) patch series which\nactually introduces the new chainlint because it would be too burdensome\non reviewers as a single series which, combined, would likely contain\n30-35 patches; this preparatory series is already long enough at 15\npatches. Although this series arose from the same work which begat\n'es/test-chain-lint'[2], it is independent of that series.\n\nThis series merges cleanly with 'next' but has a conflict in t/Makefile\nfrom 'ab/make-dependency' in 'seen'. The resolution involves replacing\n`'$(CHAINLINTTMP_SQ)'` with `$(call shellquote,$(CHAINLINTTMP))`. The\nresolved code should look like this:\n\n--- >8 ---\nclean-chainlint:\n\t$(RM) -r $(call shellquote,$(CHAINLINTTMP))\n\ncheck-chainlint:\n\t@mkdir -p $(call shellquote,$(CHAINLINTTMP)) && \\\n\tsed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >$(call shellquote,$(CHAINLINTTMP))/tests && \\\n\tsed -e '/^[ \t]*$$/d' $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >$(call shellquote,$(CHAINLINTTMP))/expect && \\\n\t$(CHAINLINT) $(call shellquote,$(CHAINLINTTMP))/tests | grep -v '^[\t]*$$' >$(call shellquote,$(CHAINLINTTMP))/actual && \\\n\tdiff -u $(call shellquote,$(CHAINLINTTMP))/expect $(call shellquote,$(CHAINLINTTMP))/actual\n--- >8 ---\n\n[1]: https://lore.kernel.org/git/YJzGcZpZ+E9R0gYd@coredump.intra.peff.net/\n[2]: https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/\n\nEric Sunshine (15):\n t/chainlint/*.test: don't use invalid shell syntax\n t/chainlint/*.test: fix invalid test cases due to mixing quote types\n t/chainlint/*.test: generalize self-test commentary\n t/chainlint/one-liner: avoid overly intimate chainlint.sed knowledge\n t/Makefile: optimize chainlint self-test\n chainlint.sed: improve ?!AMP?! placement accuracy\n chainlint.sed: improve ?!SEMI?! placement accuracy\n chainlint.sed: tolerate harmless \";\" at end of last line in block\n chainlint.sed: drop unnecessary distinction between ?!AMP?! and\n ?!SEMI?!\n chainlint.sed: drop subshell-closing \">\" annotation\n chainlint.sed: make here-doc \"<<-\" operator recognition more\n POSIX-like\n chainlint.sed: don't mistake `<< word` in string as here-doc operator\n chainlint.sed: stop throwing away here-doc tags\n chainlint.sed: swallow comments consistently\n chainlint.sed: stop splitting \"(...\" into separate lines \"(\" and \"...\"\n\n t/Makefile | 10 +-\n t/chainlint.sed | 124 +++++++++++-------\n t/chainlint/arithmetic-expansion.expect | 6 +-\n t/chainlint/bash-array.expect | 4 +-\n t/chainlint/blank-line.expect | 2 +-\n t/chainlint/blank-line.test | 2 +-\n t/chainlint/block-comment.expect | 6 +\n t/chainlint/block-comment.test | 8 ++\n t/chainlint/block.expect | 4 +-\n t/chainlint/block.test | 3 +-\n t/chainlint/broken-chain.expect | 4 +-\n t/chainlint/broken-chain.test | 2 +-\n t/chainlint/case-comment.expect | 8 ++\n t/chainlint/case-comment.test | 11 ++\n t/chainlint/case.expect | 10 +-\n t/chainlint/case.test | 6 +-\n .../close-nested-and-parent-together.expect | 5 +-\n t/chainlint/close-subshell.expect | 16 +--\n t/chainlint/command-substitution.expect | 6 +-\n t/chainlint/comment.expect | 2 +-\n t/chainlint/complex-if-in-cuddled-loop.expect | 5 +-\n t/chainlint/complex-if-in-cuddled-loop.test | 2 +-\n t/chainlint/cuddled-if-then-else.expect | 5 +-\n t/chainlint/cuddled-if-then-else.test | 2 +-\n t/chainlint/cuddled-loop.expect | 5 +-\n t/chainlint/cuddled-loop.test | 2 +-\n t/chainlint/cuddled.expect | 22 ++--\n t/chainlint/cuddled.test | 3 +-\n t/chainlint/exit-loop.expect | 6 +-\n t/chainlint/exit-subshell.expect | 2 +-\n t/chainlint/for-loop.expect | 8 +-\n t/chainlint/for-loop.test | 8 +-\n t/chainlint/here-doc-close-subshell.expect | 2 +-\n .../here-doc-multi-line-command-subst.expect | 6 +-\n t/chainlint/here-doc-multi-line-string.expect | 4 +-\n t/chainlint/here-doc.expect | 10 +-\n t/chainlint/here-doc.test | 7 -\n t/chainlint/if-in-loop.expect | 8 +-\n t/chainlint/if-in-loop.test | 6 +-\n t/chainlint/if-then-else.expect | 15 ++-\n t/chainlint/if-then-else.test | 17 +--\n t/chainlint/incomplete-line.expect | 2 +-\n t/chainlint/inline-comment.expect | 9 +-\n t/chainlint/loop-in-if.expect | 8 +-\n t/chainlint/loop-in-if.test | 6 +-\n ...ti-line-nested-command-substitution.expect | 10 +-\n t/chainlint/multi-line-string.expect | 12 +-\n t/chainlint/multi-line-string.test | 16 +--\n t/chainlint/negated-one-liner.expect | 4 +-\n t/chainlint/nested-cuddled-subshell.expect | 14 +-\n t/chainlint/nested-here-doc.expect | 8 +-\n t/chainlint/nested-subshell-comment.expect | 6 +-\n t/chainlint/nested-subshell-comment.test | 2 +-\n t/chainlint/nested-subshell.expect | 6 +-\n t/chainlint/nested-subshell.test | 1 -\n t/chainlint/not-heredoc.expect | 14 ++\n t/chainlint/not-heredoc.test | 16 +++\n t/chainlint/one-liner.expect | 6 +-\n t/chainlint/one-liner.test | 4 +-\n t/chainlint/p4-filespec.expect | 2 +-\n t/chainlint/pipe.expect | 4 +-\n t/chainlint/pipe.test | 2 +-\n t/chainlint/semicolon.expect | 27 ++--\n t/chainlint/semicolon.test | 4 +-\n t/chainlint/subshell-here-doc.expect | 15 +--\n t/chainlint/subshell-here-doc.test | 8 +-\n t/chainlint/subshell-one-liner.expect | 12 +-\n t/chainlint/t7900-subtree.expect | 10 +-\n t/chainlint/t7900-subtree.test | 4 +-\n t/chainlint/while-loop.expect | 8 +-\n t/chainlint/while-loop.test | 8 +-\n 71 files changed, 339 insertions(+), 293 deletions(-)\n create mode 100644 t/chainlint/block-comment.expect\n create mode 100644 t/chainlint/block-comment.test\n create mode 100644 t/chainlint/case-comment.expect\n create mode 100644 t/chainlint/case-comment.test\n create mode 100644 t/chainlint/not-heredoc.expect\n create mode 100644 t/chainlint/not-heredoc.test\n\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443947","messageId":"20211213063059.19424-2-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 01/15] t/chainlint/*.test: don't use invalid shell syntax","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:45Z","receivedAt":"2021-12-13T06:32:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The chainlint self-test code snippets are supposed to represent the body\nof a test_expect_success() or test_expect_failure(), yet the contents of\nthese tests would have caused the shell to report syntax errors had they\nbeen real test bodies. Although chainlint.sed, with its simplistic\nheuristics, is blind to these syntactic problems, a future more robust\nchainlint implementation might not have such a limitation, so make these\nsnippets syntactically valid.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint/if-then-else.expect | 5 +++--\n t/chainlint/if-then-else.test | 3 ++-\n t/chainlint/subshell-here-doc.test | 2 +-\n 3 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect\nindex 5953c7bfbc..a80f5e6c75 100644\n--- a/t/chainlint/if-then-else.expect\n+++ b/t/chainlint/if-then-else.expect\n@@ -4,6 +4,7 @@\n ?!AMP?!\t\techo very\n \t\techo empty\n \telif test -z \"\"\n+\tthen\n \t\techo foo\n \telse\n \t\techo foo &&\n@@ -14,6 +15,6 @@\n (\n \tif test -n \"\"; then\n \t\techo very &&\n-?!AMP?!\t\techo empty\n-\tif\n+\t\techo empty\n+\tfi\n >)\ndiff --git a/t/chainlint/if-then-else.test b/t/chainlint/if-then-else.test\nindex 9bd8e9a4c6..d2b03ca6b4 100644\n--- a/t/chainlint/if-then-else.test\n+++ b/t/chainlint/if-then-else.test\n@@ -7,6 +7,7 @@\n # LINT: last statement before 'elif' does not need \"&&\"\n \t\techo empty\n \telif test -z \"\"\n+\tthen\n # LINT: last statement before 'else' does not need \"&&\"\n \t\techo foo\n \telse\n@@ -24,5 +25,5 @@\n \tif test -n \"\"; then\n \t\techo very &&\n \t\techo empty\n-\tif\n+\tfi\n )\ndiff --git a/t/chainlint/subshell-here-doc.test b/t/chainlint/subshell-here-doc.test\nindex f6b3ba4214..0cce907ba8 100644\n--- a/t/chainlint/subshell-here-doc.test\n+++ b/t/chainlint/subshell-here-doc.test\n@@ -11,7 +11,7 @@\n # LINT: missing \"&&\" on 'cat'\n \tcat <bip\n \tfish fly high\n-\tEOF\n+EOF\n \n # LINT: swallow here-doc (EOF is last line of subshell)\n \techo <<-\\EOF >bop\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443948","messageId":"20211213063059.19424-4-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 03/15] t/chainlint/*.test: generalize self-test commentary","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:47Z","receivedAt":"2021-12-13T06:32:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The purpose of chainlint.sed is to detect &&-chain breakage only within\nsubshells (one level deep); it doesn't bother checking for top-level\n&&-chain breakage since the &&-chain checker built into t/test-lib.sh\nshould detect broken &&-chains outside of subshells by making them\nmagically exit with code 117. However, this division of labor may not\nalways be the case if a more capable chainlint implementation is ever\ndeveloped. Beyond that, due to being sed-based and due to its use of\nheuristics, chainlint.sed has several limitations (such as being unable\nto detect &&-chain breakage in subshells more than one level deep since\nit only manually emulates recursion into a subshell).\n\nSome of the comments in the chainlint self-tests unnecessarily reflect\nthe limitations of chainlint.sed even though those limitations are not\nwhat is being tested. Therefore, simplify and generalize the comments to\nexplain only what is being tested, thus ensuring that they won't become\noutdated if a more capable chainlint is ever developed.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint/blank-line.test | 2 +-\n t/chainlint/block.test | 3 +--\n t/chainlint/cuddled.test | 3 +--\n t/chainlint/nested-subshell.test | 1 -\n t/chainlint/one-liner.test | 2 +-\n t/chainlint/semicolon.test | 4 ++--\n 6 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/t/chainlint/blank-line.test b/t/chainlint/blank-line.test\nindex f6dd14302b..0fdf15b3e1 100644\n--- a/t/chainlint/blank-line.test\n+++ b/t/chainlint/blank-line.test\n@@ -3,7 +3,7 @@\n \tnothing &&\n \n \tsomething\n-# LINT: swallow blank lines since final _statement_ before subshell end is\n+# LINT: ignore blank lines since final _statement_ before subshell end is\n # LINT: significant to \"&&\"-check, not final _line_ (which might be blank)\n \n \ndiff --git a/t/chainlint/block.test b/t/chainlint/block.test\nindex d859151af1..0a82fd579f 100644\n--- a/t/chainlint/block.test\n+++ b/t/chainlint/block.test\n@@ -1,6 +1,5 @@\n (\n-# LINT: missing \"&&\" in block not currently detected (for consistency with\n-# LINT: --chain-lint at top level and to provide escape hatch if needed)\n+# LINT: missing \"&&\" after first \"echo\"\n \tfoo &&\n \t{\n \t\techo a\ndiff --git a/t/chainlint/cuddled.test b/t/chainlint/cuddled.test\nindex 0499fa4180..257b5b5eed 100644\n--- a/t/chainlint/cuddled.test\n+++ b/t/chainlint/cuddled.test\n@@ -1,5 +1,4 @@\n-# LINT: first subshell statement cuddled with opening \"(\"; for implementation\n-# LINT: simplicity, \"(...\" is split into two lines, \"(\" and \"...\"\n+# LINT: first subshell statement cuddled with opening \"(\"\n (cd foo &&\n \tbar\n ) &&\ndiff --git a/t/chainlint/nested-subshell.test b/t/chainlint/nested-subshell.test\nindex 998b05a47d..440ee9992d 100644\n--- a/t/chainlint/nested-subshell.test\n+++ b/t/chainlint/nested-subshell.test\n@@ -7,7 +7,6 @@\n \n \tcd foo &&\n \t(\n-# LINT: nested multi-line subshell not presently checked for missing \"&&\"\n \t\techo a\n \t\techo b\n \t) >file\ndiff --git a/t/chainlint/one-liner.test b/t/chainlint/one-liner.test\nindex ec9acb9825..69796d7505 100644\n--- a/t/chainlint/one-liner.test\n+++ b/t/chainlint/one-liner.test\n@@ -3,7 +3,7 @@\n (foo && bar) |\n (foo && bar) >baz &&\n \n-# LINT: top-level one-liner subshell missing internal \"&&\"\n+# LINT: top-level one-liner subshell missing internal \"&&\" and broken &&-chain\n (foo; bar) &&\n (foo; bar) |\n (foo; bar) >baz\ndiff --git a/t/chainlint/semicolon.test b/t/chainlint/semicolon.test\nindex d82c8ebbc0..67e1192c50 100644\n--- a/t/chainlint/semicolon.test\n+++ b/t/chainlint/semicolon.test\n@@ -15,11 +15,11 @@\n \tcat foo; echo bar\n ) &&\n (\n-# LINT: unnecessary terminating semicolon\n+# LINT: semicolon unnecessary but legitimate\n \tfoo;\n ) &&\n (cd foo &&\n \tfor i in a b c; do\n-# LINT: unnecessary terminating semicolon\n+# LINT: semicolon unnecessary but legitimate\n \t\techo;\n \tdone)\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443949","messageId":"20211213063059.19424-3-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 02/15] t/chainlint/*.test: fix invalid test cases due to mixing quote types","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:46Z","receivedAt":"2021-12-13T06:32:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The chainlint self-test code snippets are supposed to represent the body\nof a test_expect_success() or test_expect_failure(), yet the contents of\na few tests would have caused the shell to report syntax errors had they\nbeen real test bodies due to the mix of single- and double-quotes.\nAlthough chainlint.sed, with its simplistic heuristics, is blind to this\nproblem, a future more robust chainlint implementation might not have\nsuch a limitation. Therefore, stop mixing quote types haphazardly in\nthose tests and unify quoting throughout. While at it, drop chunks of\ntests which merely repeat what is already tested elsewhere but with\nalternative quotes.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint/broken-chain.test | 2 +-\n t/chainlint/case.test | 6 +++---\n t/chainlint/complex-if-in-cuddled-loop.test | 2 +-\n t/chainlint/cuddled-if-then-else.test | 2 +-\n t/chainlint/cuddled-loop.test | 2 +-\n t/chainlint/for-loop.test | 8 ++++----\n t/chainlint/here-doc.expect | 2 --\n t/chainlint/here-doc.test | 7 -------\n t/chainlint/if-in-loop.test | 6 +++---\n t/chainlint/if-then-else.test | 14 +++++++-------\n t/chainlint/loop-in-if.test | 6 +++---\n t/chainlint/multi-line-string.expect | 8 +-------\n t/chainlint/multi-line-string.test | 16 ++--------------\n t/chainlint/nested-subshell-comment.test | 2 +-\n t/chainlint/pipe.test | 2 +-\n t/chainlint/subshell-here-doc.expect | 1 -\n t/chainlint/subshell-here-doc.test | 6 +-----\n t/chainlint/t7900-subtree.expect | 4 ++--\n t/chainlint/t7900-subtree.test | 4 ++--\n t/chainlint/while-loop.test | 8 ++++----\n 20 files changed, 38 insertions(+), 70 deletions(-)\n\ndiff --git a/t/chainlint/broken-chain.test b/t/chainlint/broken-chain.test\nindex 3cc67b65d0..2a44aa73b7 100644\n--- a/t/chainlint/broken-chain.test\n+++ b/t/chainlint/broken-chain.test\n@@ -1,6 +1,6 @@\n (\n \tfoo &&\n-# LINT: missing \"&&\" from 'bar'\n+# LINT: missing \"&&\" from \"bar\"\n \tbar\n \tbaz &&\n # LINT: final statement before closing \")\" legitimately lacks \"&&\"\ndiff --git a/t/chainlint/case.test b/t/chainlint/case.test\nindex 5ef6ff7db5..4cb086bf87 100644\n--- a/t/chainlint/case.test\n+++ b/t/chainlint/case.test\n@@ -1,5 +1,5 @@\n (\n-# LINT: \"...)\" arms in 'case' not misinterpreted as subshell-closing \")\"\n+# LINT: \"...)\" arms in \"case\" not misinterpreted as subshell-closing \")\"\n \tcase \"$x\" in\n \tx) foo ;;\n \t*) bar ;;\n@@ -7,7 +7,7 @@\n \tfoobar\n ) &&\n (\n-# LINT: missing \"&&\" on 'esac'\n+# LINT: missing \"&&\" on \"esac\"\n \tcase \"$x\" in\n \tx) foo ;;\n \t*) bar ;;\n@@ -15,7 +15,7 @@\n \tfoobar\n ) &&\n (\n-# LINT: \"...)\" arm in one-liner 'case' not misinterpreted as closing \")\"\n+# LINT: \"...)\" arm in one-liner \"case\" not misinterpreted as closing \")\"\n \tcase \"$x\" in 1) true;; esac &&\n # LINT: same but missing \"&&\"\n \tcase \"$y\" in 2) false;; esac\ndiff --git a/t/chainlint/complex-if-in-cuddled-loop.test b/t/chainlint/complex-if-in-cuddled-loop.test\nindex 571bbd85cd..5efeda58b2 100644\n--- a/t/chainlint/complex-if-in-cuddled-loop.test\n+++ b/t/chainlint/complex-if-in-cuddled-loop.test\n@@ -1,4 +1,4 @@\n-# LINT: 'for' loop cuddled with \"(\" and \")\" and nested 'if' with complex\n+# LINT: \"for\" loop cuddled with \"(\" and \")\" and nested \"if\" with complex\n # LINT: multi-line condition; indented with spaces, not tabs\n (for i in a b c; do\n if test \"$(echo $(waffle bat))\" = \"eleventeen\" &&\ndiff --git a/t/chainlint/cuddled-if-then-else.test b/t/chainlint/cuddled-if-then-else.test\nindex eed774a9d6..7c53f4efe3 100644\n--- a/t/chainlint/cuddled-if-then-else.test\n+++ b/t/chainlint/cuddled-if-then-else.test\n@@ -1,4 +1,4 @@\n-# LINT: 'if' cuddled with \"(\" and \")\"; indented with spaces, not tabs\n+# LINT: \"if\" cuddled with \"(\" and \")\"; indented with spaces, not tabs\n (if test -z \"\"; then\n echo empty\n else\ndiff --git a/t/chainlint/cuddled-loop.test b/t/chainlint/cuddled-loop.test\nindex a841d781f0..3c2a62f751 100644\n--- a/t/chainlint/cuddled-loop.test\n+++ b/t/chainlint/cuddled-loop.test\n@@ -1,4 +1,4 @@\n-# LINT: 'while' loop cuddled with \"(\" and \")\", with embedded (allowed)\n+# LINT: \"while\" loop cuddled with \"(\" and \")\", with embedded (allowed)\n # LINT: \"|| exit {n}\" to exit loop early, and using redirection \"<\" to feed\n # LINT: loop; indented with spaces, not tabs\n ( while read x\ndiff --git a/t/chainlint/for-loop.test b/t/chainlint/for-loop.test\nindex 7db76262bc..6cb3428158 100644\n--- a/t/chainlint/for-loop.test\n+++ b/t/chainlint/for-loop.test\n@@ -1,17 +1,17 @@\n (\n-# LINT: 'for', 'do', 'done' do not need \"&&\"\n+# LINT: \"for\", \"do\", \"done\" do not need \"&&\"\n \tfor i in a b c\n \tdo\n-# LINT: missing \"&&\" on 'echo'\n+# LINT: missing \"&&\" on \"echo\"\n \t\techo $i\n # LINT: last statement of while does not need \"&&\"\n \t\tcat <<-\\EOF\n \t\tbar\n \t\tEOF\n-# LINT: missing \"&&\" on 'done'\n+# LINT: missing \"&&\" on \"done\"\n \tdone\n \n-# LINT: 'do' on same line as 'for'\n+# LINT: \"do\" on same line as \"for\"\n \tfor i in a b c; do\n \t\techo $i &&\n \t\tcat $i\ndiff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect\nindex 534b065e38..8449eb2e02 100644\n--- a/t/chainlint/here-doc.expect\n+++ b/t/chainlint/here-doc.expect\n@@ -2,8 +2,6 @@ boodle wobba gorgo snoot wafta snurb &&\n \n cat >foo &&\n \n-cat >bar &&\n-\n cat >boo &&\n \n horticulture\ndiff --git a/t/chainlint/here-doc.test b/t/chainlint/here-doc.test\nindex ad4ce8afd9..3f5f92cad3 100644\n--- a/t/chainlint/here-doc.test\n+++ b/t/chainlint/here-doc.test\n@@ -14,13 +14,6 @@ boz\n woz\n Arbitrary_Tag_42\n \n-# LINT: swallow 'quoted' here-doc\n-cat <<'FUMP' >bar &&\n-snoz\n-boz\n-woz\n-FUMP\n-\n # LINT: swallow \"quoted\" here-doc\n cat <<\"zump\" >boo &&\n snoz\ndiff --git a/t/chainlint/if-in-loop.test b/t/chainlint/if-in-loop.test\nindex daf22da164..f0cf19cfad 100644\n--- a/t/chainlint/if-in-loop.test\n+++ b/t/chainlint/if-in-loop.test\n@@ -3,13 +3,13 @@\n \tdo\n \t\tif false\n \t\tthen\n-# LINT: missing \"&&\" on 'echo'\n+# LINT: missing \"&&\" on \"echo\"\n \t\t\techo \"err\"\n \t\t\texit 1\n-# LINT: missing \"&&\" on 'fi'\n+# LINT: missing \"&&\" on \"fi\"\n \t\tfi\n \t\tfoo\n-# LINT: missing \"&&\" on 'done'\n+# LINT: missing \"&&\" on \"done\"\n \tdone\n \tbar\n )\ndiff --git a/t/chainlint/if-then-else.test b/t/chainlint/if-then-else.test\nindex d2b03ca6b4..2055336c2b 100644\n--- a/t/chainlint/if-then-else.test\n+++ b/t/chainlint/if-then-else.test\n@@ -1,27 +1,27 @@\n (\n-# LINT: 'if', 'then', 'elif', 'else', 'fi' do not need \"&&\"\n+# LINT: \"if\", \"then\", \"elif\", \"else\", \"fi\" do not need \"&&\"\n \tif test -n \"\"\n \tthen\n-# LINT: missing \"&&\" on 'echo'\n+# LINT: missing \"&&\" on \"echo\"\n \t\techo very\n-# LINT: last statement before 'elif' does not need \"&&\"\n+# LINT: last statement before \"elif\" does not need \"&&\"\n \t\techo empty\n \telif test -z \"\"\n \tthen\n-# LINT: last statement before 'else' does not need \"&&\"\n+# LINT: last statement before \"else\" does not need \"&&\"\n \t\techo foo\n \telse\n \t\techo foo &&\n-# LINT: last statement before 'fi' does not need \"&&\"\n+# LINT: last statement before \"fi\" does not need \"&&\"\n \t\tcat <<-\\EOF\n \t\tbar\n \t\tEOF\n-# LINT: missing \"&&\" on 'fi'\n+# LINT: missing \"&&\" on \"fi\"\n \tfi\n \techo poodle\n ) &&\n (\n-# LINT: 'then' on same line as 'if'\n+# LINT: \"then\" on same line as \"if\"\n \tif test -n \"\"; then\n \t\techo very &&\n \t\techo empty\ndiff --git a/t/chainlint/loop-in-if.test b/t/chainlint/loop-in-if.test\nindex 93e8ba8e4d..dfcc3f98fb 100644\n--- a/t/chainlint/loop-in-if.test\n+++ b/t/chainlint/loop-in-if.test\n@@ -3,13 +3,13 @@\n \tthen\n \t\twhile true\n \t\tdo\n-# LINT: missing \"&&\" on 'echo'\n+# LINT: missing \"&&\" on \"echo\"\n \t\t\techo \"pop\"\n \t\t\techo \"glup\"\n-# LINT: missing \"&&\" on 'done'\n+# LINT: missing \"&&\" on \"done\"\n \t\tdone\n \t\tfoo\n-# LINT: missing \"&&\" on 'fi'\n+# LINT: missing \"&&\" on \"fi\"\n \tfi\n \tbar\n )\ndiff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect\nindex 170cb59993..2829516495 100644\n--- a/t/chainlint/multi-line-string.expect\n+++ b/t/chainlint/multi-line-string.expect\n@@ -1,15 +1,9 @@\n (\n \tx=\"line 1\t\tline 2\t\tline 3\" &&\n-?!AMP?!\ty='line 1\t\tline2'\n+?!AMP?!\ty=\"line 1\t\tline2\"\n \tfoobar\n >) &&\n-(\n-\techo \"there's nothing to see here\" &&\n-\texit\n->) &&\n (\n \techo \"xyz\" \"abc\t\tdef\t\tghi\" &&\n-\techo 'xyz' 'abc\t\tdef\t\tghi' &&\n-\techo 'xyz' \"abc\t\tdef\t\tghi\" &&\n \tbarfoo\n >)\ndiff --git a/t/chainlint/multi-line-string.test b/t/chainlint/multi-line-string.test\nindex 287ab89705..4a0af2107d 100644\n--- a/t/chainlint/multi-line-string.test\n+++ b/t/chainlint/multi-line-string.test\n@@ -3,25 +3,13 @@\n \t\tline 2\n \t\tline 3\" &&\n # LINT: missing \"&&\" on assignment\n-\ty='line 1\n-\t\tline2'\n+\ty=\"line 1\n+\t\tline2\"\n \tfoobar\n ) &&\n-(\n-# LINT: apostrophe (in a contraction) within string not misinterpreted as\n-# LINT: starting multi-line single-quoted string\n-\techo \"there's nothing to see here\" &&\n-\texit\n-) &&\n (\n \techo \"xyz\" \"abc\n \t\tdef\n \t\tghi\" &&\n-\techo 'xyz' 'abc\n-\t\tdef\n-\t\tghi' &&\n-\techo 'xyz' \"abc\n-\t\tdef\n-\t\tghi\" &&\n \tbarfoo\n )\ndiff --git a/t/chainlint/nested-subshell-comment.test b/t/chainlint/nested-subshell-comment.test\nindex 0ff136ab3c..0215cdb192 100644\n--- a/t/chainlint/nested-subshell-comment.test\n+++ b/t/chainlint/nested-subshell-comment.test\n@@ -7,7 +7,7 @@\n \t\t# minor numbers of cows (or do they?)\n \t\tbaz &&\n \t\tsnaff\n-# LINT: missing \"&&\" on ')'\n+# LINT: missing \"&&\" on \")\"\n \t)\n \tfuzzy\n )\ndiff --git a/t/chainlint/pipe.test b/t/chainlint/pipe.test\nindex e6af4de916..dd82534c66 100644\n--- a/t/chainlint/pipe.test\n+++ b/t/chainlint/pipe.test\n@@ -4,7 +4,7 @@\n \tbar |\n \tbaz &&\n \n-# LINT: final line of pipe sequence ('cow') lacking \"&&\"\n+# LINT: final line of pipe sequence (\"cow\") lacking \"&&\"\n \tfish |\n \tcow\n \ndiff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect\nindex 74723e7340..7e057aee42 100644\n--- a/t/chainlint/subshell-here-doc.expect\n+++ b/t/chainlint/subshell-here-doc.expect\n@@ -5,7 +5,6 @@\n >) &&\n (\n \tcat >bup &&\n-\tcat >bup2 &&\n \tcat >bup3 &&\n \tmeep\n >)\ndiff --git a/t/chainlint/subshell-here-doc.test b/t/chainlint/subshell-here-doc.test\nindex 0cce907ba8..d40eb65583 100644\n--- a/t/chainlint/subshell-here-doc.test\n+++ b/t/chainlint/subshell-here-doc.test\n@@ -8,7 +8,7 @@\n \tnevermore...\n \tEOF\n \n-# LINT: missing \"&&\" on 'cat'\n+# LINT: missing \"&&\" on \"cat\"\n \tcat <bip\n \tfish fly high\n EOF\n@@ -27,10 +27,6 @@ EOF\n \tglink\n \tFIZZ\n \tARBITRARY\n-\tcat <<-'ARBITRARY2' >bup2 &&\n-\tglink\n-\tFIZZ\n-\tARBITRARY2\n \tcat <<-\"ARBITRARY3\" >bup3 &&\n \tglink\n \tFIZZ\ndiff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect\nindex c9913429e6..f769244ef6 100644\n--- a/t/chainlint/t7900-subtree.expect\n+++ b/t/chainlint/t7900-subtree.expect\n@@ -1,9 +1,9 @@\n (\n \tchks=\"sub1sub2sub3sub4\" &&\n-\tchks_sub=$(cat | sed 's,^,sub dir/,'\n+\tchks_sub=$(cat | sed \"s,^,sub dir/,\"\n >>) &&\n \tchkms=\"main-sub1main-sub2main-sub3main-sub4\" &&\n-\tchkms_sub=$(cat | sed 's,^,sub dir/,'\n+\tchkms_sub=$(cat | sed \"s,^,sub dir/,\"\n >>) &&\n \tsubfiles=$(git ls-files) &&\n \tcheck_equal \"$subfiles\" \"$chkms$chks\"\ndiff --git a/t/chainlint/t7900-subtree.test b/t/chainlint/t7900-subtree.test\nindex 277d8358df..02f3129232 100644\n--- a/t/chainlint/t7900-subtree.test\n+++ b/t/chainlint/t7900-subtree.test\n@@ -3,7 +3,7 @@\n sub2\n sub3\n sub4\" &&\n-\tchks_sub=$(cat <\n---\n t/chainlint/one-liner.expect | 2 +-\n t/chainlint/one-liner.test | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect\nindex 237f227349..c64058f7af 100644\n--- a/t/chainlint/one-liner.expect\n+++ b/t/chainlint/one-liner.expect\n@@ -4,6 +4,6 @@\n \n ?!SEMI?!(foo; bar) &&\n ?!SEMI?!(foo; bar) |\n-?!SEMI?!(foo; bar) >baz\n+?!SEMI?!(foo; bar) >baz &&\n \n (foo \"bar; baz\")\ndiff --git a/t/chainlint/one-liner.test b/t/chainlint/one-liner.test\nindex 69796d7505..be9858fa29 100644\n--- a/t/chainlint/one-liner.test\n+++ b/t/chainlint/one-liner.test\n@@ -6,7 +6,7 @@\n # LINT: top-level one-liner subshell missing internal \"&&\" and broken &&-chain\n (foo; bar) &&\n (foo; bar) |\n-(foo; bar) >baz\n+(foo; bar) >baz &&\n \n # LINT: \";\" in string not misinterpreted as broken &&-chain\n (foo \"bar; baz\")\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443951","messageId":"20211213063059.19424-6-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:49Z","receivedAt":"2021-12-13T06:32:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Rather than running `chainlint` and `diff` once per self-test -- which\nmay become expensive as more tests are added -- instead run `chainlint`\na single time over all tests bodies collectively and compare the result\nto the collective \"expected\" output.\n\nSigned-off-by: Eric Sunshine \n---\n t/Makefile | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 882d26eee3..f4ae40be46 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -71,12 +71,10 @@ clean-chainlint:\n \n check-chainlint:\n \t@mkdir -p '$(CHAINLINTTMP_SQ)' && \\\n-\terr=0 && \\\n-\tfor i in $(CHAINLINTTESTS); do \\\n-\t\t$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/$$i.actual && \\\n-\t\tdiff -u chainlint/$$i.expect '$(CHAINLINTTMP_SQ)'/$$i.actual || err=1; \\\n-\tdone && exit $$err\n+\tsed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n+\tcat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n+\t$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \\\n+\tdiff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n \n test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \\\n \ttest-lint-filenames\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443952","messageId":"20211213063059.19424-7-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 06/15] chainlint.sed: improve ?!AMP?! placement accuracy","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:50Z","receivedAt":"2021-12-13T06:32:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When chainlint.sed detects a broken &&-chain, it places an ?!AMP?!\nannotation at the beginning of the line. However, this is an unusual\nlocation for programmers accustomed to error messages (from compilers,\nfor instance) indicating the exact point of the problem. Therefore,\nrelocate the ?!AMP?! annotation to the end of the line in order to\nbetter direct the programmer's attention to the source of the problem.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.sed | 8 ++++----\n t/chainlint/arithmetic-expansion.expect | 2 +-\n t/chainlint/block.expect | 2 +-\n t/chainlint/broken-chain.expect | 2 +-\n t/chainlint/case.expect | 4 ++--\n t/chainlint/command-substitution.expect | 2 +-\n t/chainlint/cuddled.expect | 4 ++--\n t/chainlint/for-loop.expect | 4 ++--\n t/chainlint/here-doc-multi-line-command-subst.expect | 2 +-\n t/chainlint/here-doc-multi-line-string.expect | 2 +-\n t/chainlint/if-in-loop.expect | 6 +++---\n t/chainlint/if-then-else.expect | 4 ++--\n t/chainlint/inline-comment.expect | 2 +-\n t/chainlint/loop-in-if.expect | 6 +++---\n t/chainlint/multi-line-string.expect | 2 +-\n t/chainlint/nested-cuddled-subshell.expect | 6 +++---\n t/chainlint/nested-here-doc.expect | 2 +-\n t/chainlint/nested-subshell-comment.expect | 2 +-\n t/chainlint/pipe.expect | 2 +-\n t/chainlint/semicolon.expect | 2 +-\n t/chainlint/subshell-here-doc.expect | 2 +-\n t/chainlint/subshell-one-liner.expect | 4 ++--\n t/chainlint/while-loop.expect | 4 ++--\n 23 files changed, 38 insertions(+), 38 deletions(-)\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex 8a25c5b855..883a2b307c 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -236,7 +236,7 @@ s/.*\\n//\n # line ends with pipe \"...|\" -- valid; not missing \"&&\"\n /|[ \t]*$/bcont\n # missing end-of-line \"&&\" -- mark suspect\n-/&&[ \t]*$/!s/^/?!AMP?!/\n+/&&[ \t]*$/!s/$/ ?!AMP?!/\n :cont\n # retrieve and print previous line\n x\n@@ -303,7 +303,7 @@ bcase\n # that line legitimately lacks \"&&\"\n :else\n x\n-s/?!AMP?!//\n+s/ ?!AMP?!$//\n x\n bcont\n \n@@ -311,7 +311,7 @@ bcont\n # \"suspect\" from final contained line since that line legitimately lacks \"&&\"\n :done\n x\n-s/?!AMP?!//\n+s/ ?!AMP?!$//\n x\n # is 'done' or 'fi' cuddled with \")\" to close subshell?\n /done.*)/bclose\n@@ -354,7 +354,7 @@ bblock\n # since that line legitimately lacks \"&&\" and exit subshell loop\n :clssolo\n x\n-s/?!AMP?!//\n+s/ ?!AMP?!$//\n p\n x\n s/^/>/\ndiff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect\nindex 09457d3196..56cd5b69f5 100644\n--- a/t/chainlint/arithmetic-expansion.expect\n+++ b/t/chainlint/arithmetic-expansion.expect\n@@ -4,6 +4,6 @@\n \tbaz\n >) &&\n (\n-?!AMP?!\tbar=$((42 + 1))\n+\tbar=$((42 + 1)) ?!AMP?!\n \tbaz\n >)\ndiff --git a/t/chainlint/block.expect b/t/chainlint/block.expect\nindex fed7e89ae8..6333237cb2 100644\n--- a/t/chainlint/block.expect\n+++ b/t/chainlint/block.expect\n@@ -7,6 +7,6 @@\n \tbar &&\n \t{\n \t\techo c\n-?!AMP?!\t}\n+\t} ?!AMP?!\n \tbaz\n >)\ndiff --git a/t/chainlint/broken-chain.expect b/t/chainlint/broken-chain.expect\nindex 55b0f42a53..0960a8c7c0 100644\n--- a/t/chainlint/broken-chain.expect\n+++ b/t/chainlint/broken-chain.expect\n@@ -1,6 +1,6 @@\n (\n \tfoo &&\n-?!AMP?!\tbar\n+\tbar ?!AMP?!\n \tbaz &&\n \twop\n >)\ndiff --git a/t/chainlint/case.expect b/t/chainlint/case.expect\nindex 41f121fbbf..a4b92d4613 100644\n--- a/t/chainlint/case.expect\n+++ b/t/chainlint/case.expect\n@@ -9,11 +9,11 @@\n \tcase \"$x\" in\n \tx) foo ;;\n \t*) bar ;;\n-?!AMP?!\tesac\n+\tesac ?!AMP?!\n \tfoobar\n >) &&\n (\n \tcase \"$x\" in 1) true;; esac &&\n-?!AMP?!\tcase \"$y\" in 2) false;; esac\n+\tcase \"$y\" in 2) false;; esac ?!AMP?!\n \tfoobar\n >)\ndiff --git a/t/chainlint/command-substitution.expect b/t/chainlint/command-substitution.expect\nindex ad4118e537..f276067b7b 100644\n--- a/t/chainlint/command-substitution.expect\n+++ b/t/chainlint/command-substitution.expect\n@@ -4,6 +4,6 @@\n \tbaz\n >) &&\n (\n-?!AMP?!\tbar=$(gobble blocks)\n+\tbar=$(gobble blocks) ?!AMP?!\n \tbaz\n >)\ndiff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect\nindex b506d46221..b6c4ed90a9 100644\n--- a/t/chainlint/cuddled.expect\n+++ b/t/chainlint/cuddled.expect\n@@ -4,7 +4,7 @@ cd foo &&\n >) &&\n \n (\n-?!AMP?!cd foo\n+cd foo ?!AMP?!\n \tbar\n >) &&\n \n@@ -17,5 +17,5 @@ cd foo &&\n >\tbar) &&\n \n (\n-?!AMP?!cd foo\n+cd foo ?!AMP?!\n >\tbar)\ndiff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect\nindex c33cf56ee7..dc209e21bd 100644\n--- a/t/chainlint/for-loop.expect\n+++ b/t/chainlint/for-loop.expect\n@@ -1,9 +1,9 @@\n (\n \tfor i in a b c\n \tdo\n-?!AMP?!\t\techo $i\n+\t\techo $i ?!AMP?!\n \t\tcat\n-?!AMP?!\tdone\n+\tdone ?!AMP?!\n \tfor i in a b c; do\n \t\techo $i &&\n \t\tcat $i\ndiff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect\nindex e5fb752d2f..3a35bb014c 100644\n--- a/t/chainlint/here-doc-multi-line-command-subst.expect\n+++ b/t/chainlint/here-doc-multi-line-command-subst.expect\n@@ -1,5 +1,5 @@\n (\n \tx=$(bobble &&\n-?!AMP?!>>\t\twiffle)\n+>>\t\twiffle) ?!AMP?!\n \techo $x\n >)\ndiff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect\nindex 32038a070c..a3b9a5472d 100644\n--- a/t/chainlint/here-doc-multi-line-string.expect\n+++ b/t/chainlint/here-doc-multi-line-string.expect\n@@ -1,4 +1,4 @@\n (\n-?!AMP?!\tcat && echo \"multi-line\tstring\"\n+\tcat && echo \"multi-line\tstring\" ?!AMP?!\n \tbap\n >)\ndiff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect\nindex 03d3ceb22d..7d91837269 100644\n--- a/t/chainlint/if-in-loop.expect\n+++ b/t/chainlint/if-in-loop.expect\n@@ -3,10 +3,10 @@\n \tdo\n \t\tif false\n \t\tthen\n-?!AMP?!\t\t\techo \"err\"\n+\t\t\techo \"err\" ?!AMP?!\n \t\t\texit 1\n-?!AMP?!\t\tfi\n+\t\tfi ?!AMP?!\n \t\tfoo\n-?!AMP?!\tdone\n+\tdone ?!AMP?!\n \tbar\n >)\ndiff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect\nindex a80f5e6c75..3055d5606c 100644\n--- a/t/chainlint/if-then-else.expect\n+++ b/t/chainlint/if-then-else.expect\n@@ -1,7 +1,7 @@\n (\n \tif test -n \"\"\n \tthen\n-?!AMP?!\t\techo very\n+\t\techo very ?!AMP?!\n \t\techo empty\n \telif test -z \"\"\n \tthen\n@@ -9,7 +9,7 @@\n \telse\n \t\techo foo &&\n \t\tcat\n-?!AMP?!\tfi\n+\tfi ?!AMP?!\n \techo poodle\n >) &&\n (\ndiff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect\nindex fc9f250ac4..3d655a32b0 100644\n--- a/t/chainlint/inline-comment.expect\n+++ b/t/chainlint/inline-comment.expect\n@@ -1,6 +1,6 @@\n (\n \tfoobar &&\n-?!AMP?!\tbarfoo\n+\tbarfoo ?!AMP?!\n \tflibble \"not a # comment\"\n >) &&\n \ndiff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect\nindex 088e622c31..cebd3ae95e 100644\n--- a/t/chainlint/loop-in-if.expect\n+++ b/t/chainlint/loop-in-if.expect\n@@ -3,10 +3,10 @@\n \tthen\n \t\twhile true\n \t\tdo\n-?!AMP?!\t\t\techo \"pop\"\n+\t\t\techo \"pop\" ?!AMP?!\n \t\t\techo \"glup\"\n-?!AMP?!\t\tdone\n+\t\tdone ?!AMP?!\n \t\tfoo\n-?!AMP?!\tfi\n+\tfi ?!AMP?!\n \tbar\n >)\ndiff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect\nindex 2829516495..f1be2baf0a 100644\n--- a/t/chainlint/multi-line-string.expect\n+++ b/t/chainlint/multi-line-string.expect\n@@ -1,6 +1,6 @@\n (\n \tx=\"line 1\t\tline 2\t\tline 3\" &&\n-?!AMP?!\ty=\"line 1\t\tline2\"\n+\ty=\"line 1\t\tline2\" ?!AMP?!\n \tfoobar\n >) &&\n (\ndiff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect\nindex c2a59ffc33..aa522658ed 100644\n--- a/t/chainlint/nested-cuddled-subshell.expect\n+++ b/t/chainlint/nested-cuddled-subshell.expect\n@@ -4,16 +4,16 @@\n >>\t) &&\n \t(cd foo &&\n \t\tbar\n-?!AMP?!>>\t)\n+>>\t) ?!AMP?!\n \t(\n \t\tcd foo &&\n >>\t\tbar) &&\n \t(\n \t\tcd foo &&\n-?!AMP?!>>\t\tbar)\n+>>\t\tbar) ?!AMP?!\n \t(cd foo &&\n >>\t\tbar) &&\n \t(cd foo &&\n-?!AMP?!>>\t\tbar)\n+>>\t\tbar) ?!AMP?!\n \tfoobar\n >)\ndiff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect\nindex 0c9ef1cfc6..f9604d3fac 100644\n--- a/t/chainlint/nested-here-doc.expect\n+++ b/t/chainlint/nested-here-doc.expect\n@@ -2,6 +2,6 @@ cat >foop &&\n \n (\n \tcat &&\n-?!AMP?!\tcat\n+\tcat ?!AMP?!\n \tfoobar\n >)\ndiff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect\nindex 15b68d4373..925e49bae9 100644\n--- a/t/chainlint/nested-subshell-comment.expect\n+++ b/t/chainlint/nested-subshell-comment.expect\n@@ -6,6 +6,6 @@\n \t\t# minor numbers of cows (or do they?)\n \t\tbaz &&\n \t\tsnaff\n-?!AMP?!>>\t)\n+>>\t) ?!AMP?!\n \tfuzzy\n >)\ndiff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect\nindex 211b901dbc..ede6bcc607 100644\n--- a/t/chainlint/pipe.expect\n+++ b/t/chainlint/pipe.expect\n@@ -3,6 +3,6 @@\n \tbar |\n \tbaz &&\n \tfish |\n-?!AMP?!\tcow\n+\tcow ?!AMP?!\n \tsunder\n >)\ndiff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect\nindex 1d79384606..ffc87bdffb 100644\n--- a/t/chainlint/semicolon.expect\n+++ b/t/chainlint/semicolon.expect\n@@ -1,5 +1,5 @@\n (\n-?!AMP?!?!SEMI?!\tcat foo ; echo bar\n+?!SEMI?!\tcat foo ; echo bar ?!AMP?!\n ?!SEMI?!\tcat foo ; echo bar\n >) &&\n (\ndiff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect\nindex 7e057aee42..9d3f25b3f5 100644\n--- a/t/chainlint/subshell-here-doc.expect\n+++ b/t/chainlint/subshell-here-doc.expect\n@@ -1,6 +1,6 @@\n (\n \techo wobba \t gorgo snoot \t wafta snurb &&\n-?!AMP?!\tcat >bip\n+\tcat >bip ?!AMP?!\n \techo >bop\n >) &&\n (\ndiff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect\nindex 51162821d7..ec77aa5b95 100644\n--- a/t/chainlint/subshell-one-liner.expect\n+++ b/t/chainlint/subshell-one-liner.expect\n@@ -8,7 +8,7 @@\n \t(foo || exit 1) &&\n \t(foo || exit 1) |\n \t(foo || exit 1) >baz &&\n-?!AMP?!\t(foo && bar)\n-?!AMP?!?!SEMI?!\t(foo && bar; baz)\n+\t(foo && bar) ?!AMP?!\n+?!SEMI?!\t(foo && bar; baz) ?!AMP?!\n \tfoobar\n >)\ndiff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect\nindex 13cff2c0a5..f8b9fcf62b 100644\n--- a/t/chainlint/while-loop.expect\n+++ b/t/chainlint/while-loop.expect\n@@ -1,9 +1,9 @@\n (\n \twhile true\n \tdo\n-?!AMP?!\t\techo foo\n+\t\techo foo ?!AMP?!\n \t\tcat\n-?!AMP?!\tdone\n+\tdone ?!AMP?!\n \twhile true; do\n \t\techo foo &&\n \t\tcat bar\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443953","messageId":"20211213063059.19424-8-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 07/15] chainlint.sed: improve ?!SEMI?! placement accuracy","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:51Z","receivedAt":"2021-12-13T06:32:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When chainlint.sed detects commands separated by a semicolon rather than\nby `&&`, it places a ?!SEMI?! annotation at the beginning of the line.\nHowever, this is an unusual location for programmers accustomed to error\nmessages (from compilers, for instance) indicating the exact point of\nthe problem. Therefore, relocate the ?!SEMI?! annotation to the location\nof the semicolon in order to better direct the programmer's attention to\nthe source of the problem.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.sed | 4 ++--\n t/chainlint/negated-one-liner.expect | 4 ++--\n t/chainlint/one-liner.expect | 6 +++---\n t/chainlint/semicolon.expect | 14 +++++++-------\n t/chainlint/subshell-one-liner.expect | 8 ++++----\n 5 files changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex 883a2b307c..60c2099c18 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -126,7 +126,7 @@ b\n # \"&&\" (but not \";\" in a string)\n :oneline\n /;/{\n-\t/\"[^\"]*;[^\"]*\"/!s/^/?!SEMI?!/\n+\t/\"[^\"]*;[^\"]*\"/!s/;/; ?!SEMI?!/\n }\n b\n \n@@ -230,7 +230,7 @@ s/.*\\n//\n # string and not \";;\" in one-liner \"case...esac\")\n /;/{\n \t/;;/!{\n-\t\t/\"[^\"]*;[^\"]*\"/!s/^/?!SEMI?!/\n+\t\t/\"[^\"]*;[^\"]*\"/!s/;/; ?!SEMI?!/\n \t}\n }\n # line ends with pipe \"...|\" -- valid; not missing \"&&\"\ndiff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect\nindex cf18429d03..60baf84b7a 100644\n--- a/t/chainlint/negated-one-liner.expect\n+++ b/t/chainlint/negated-one-liner.expect\n@@ -1,5 +1,5 @@\n ! (foo && bar) &&\n ! (foo && bar) >baz &&\n \n-?!SEMI?!! (foo; bar) &&\n-?!SEMI?!! (foo; bar) >baz\n+! (foo; ?!SEMI?! bar) &&\n+! (foo; ?!SEMI?! bar) >baz\ndiff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect\nindex c64058f7af..3b46554728 100644\n--- a/t/chainlint/one-liner.expect\n+++ b/t/chainlint/one-liner.expect\n@@ -2,8 +2,8 @@\n (foo && bar) |\n (foo && bar) >baz &&\n \n-?!SEMI?!(foo; bar) &&\n-?!SEMI?!(foo; bar) |\n-?!SEMI?!(foo; bar) >baz &&\n+(foo; ?!SEMI?! bar) &&\n+(foo; ?!SEMI?! bar) |\n+(foo; ?!SEMI?! bar) >baz &&\n \n (foo \"bar; baz\")\ndiff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect\nindex ffc87bdffb..d2d804f5b0 100644\n--- a/t/chainlint/semicolon.expect\n+++ b/t/chainlint/semicolon.expect\n@@ -1,20 +1,20 @@\n (\n-?!SEMI?!\tcat foo ; echo bar ?!AMP?!\n-?!SEMI?!\tcat foo ; echo bar\n+\tcat foo ; ?!SEMI?! echo bar ?!AMP?!\n+\tcat foo ; ?!SEMI?! echo bar\n >) &&\n (\n-?!SEMI?!\tcat foo ; echo bar &&\n-?!SEMI?!\tcat foo ; echo bar\n+\tcat foo ; ?!SEMI?! echo bar &&\n+\tcat foo ; ?!SEMI?! echo bar\n >) &&\n (\n \techo \"foo; bar\" &&\n-?!SEMI?!\tcat foo; echo bar\n+\tcat foo; ?!SEMI?! echo bar\n >) &&\n (\n-?!SEMI?!\tfoo;\n+\tfoo; ?!SEMI?!\n >) &&\n (\n cd foo &&\n \tfor i in a b c; do\n-?!SEMI?!\t\techo;\n+\t\techo; ?!SEMI?!\n >\tdone)\ndiff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect\nindex ec77aa5b95..432217801b 100644\n--- a/t/chainlint/subshell-one-liner.expect\n+++ b/t/chainlint/subshell-one-liner.expect\n@@ -2,13 +2,13 @@\n \t(foo && bar) &&\n \t(foo && bar) |\n \t(foo && bar) >baz &&\n-?!SEMI?!\t(foo; bar) &&\n-?!SEMI?!\t(foo; bar) |\n-?!SEMI?!\t(foo; bar) >baz &&\n+\t(foo; ?!SEMI?! bar) &&\n+\t(foo; ?!SEMI?! bar) |\n+\t(foo; ?!SEMI?! bar) >baz &&\n \t(foo || exit 1) &&\n \t(foo || exit 1) |\n \t(foo || exit 1) >baz &&\n \t(foo && bar) ?!AMP?!\n-?!SEMI?!\t(foo && bar; baz) ?!AMP?!\n+\t(foo && bar; ?!SEMI?! baz) ?!AMP?!\n \tfoobar\n >)\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443954","messageId":"20211213063059.19424-9-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 08/15] chainlint.sed: tolerate harmless \";\" at end of last line in block","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:52Z","receivedAt":"2021-12-13T06:32:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"chainlint.sed flags \";\" when used as a command terminator since it\nbreaks the &&-chain, thus can allow failures to go undetected. However,\nwhen a command terminated by \";\" is the last command in the body of a\ncompound statement, such as `command-2` in:\n\n if test $# -gt 1\n then\n command-1 &&\n command-2;\n fi\n\nthen the \";\" is harmless and the exit code from `command-2` is passed\nthrough untouched and becomes the exit code of the compound statement,\nas if the \";\" was not present. Therefore, tolerate a trailing \";\" in\nthis position rather than complaining about broken &&-chain.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.sed | 11 ++++++-----\n t/chainlint/semicolon.expect | 4 ++--\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex 60c2099c18..91077b6e26 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -47,8 +47,9 @@\n # \"?!AMP?!\" violation is removed from the \"bar\" line (retrieved from the \"hold\"\n # area) since the final statement of a subshell must not end with \"&&\". The\n # final line of a subshell may still break the &&-chain by using \";\" internally\n-# to chain commands together rather than \"&&\", so \"?!SEMI?!\" is never removed\n-# from a line (even though \"?!AMP?!\" might be).\n+# to chain commands together rather than \"&&\", so \"?!SEMI?!\" is not removed\n+# from such a line; however, if the line ends with \"?!SEMI?!\", then the \";\" is\n+# harmless and the annotation is removed.\n #\n # Care is taken to recognize the last _statement_ of a multi-line subshell, not\n # necessarily the last textual _line_ within the subshell, since &&-chaining\n@@ -303,7 +304,7 @@ bcase\n # that line legitimately lacks \"&&\"\n :else\n x\n-s/ ?!AMP?!$//\n+s/\\( ?!SEMI?!\\)* ?!AMP?!$//\n x\n bcont\n \n@@ -311,7 +312,7 @@ bcont\n # \"suspect\" from final contained line since that line legitimately lacks \"&&\"\n :done\n x\n-s/ ?!AMP?!$//\n+s/\\( ?!SEMI?!\\)* ?!AMP?!$//\n x\n # is 'done' or 'fi' cuddled with \")\" to close subshell?\n /done.*)/bclose\n@@ -354,7 +355,7 @@ bblock\n # since that line legitimately lacks \"&&\" and exit subshell loop\n :clssolo\n x\n-s/ ?!AMP?!$//\n+s/\\( ?!SEMI?!\\)* ?!AMP?!$//\n p\n x\n s/^/>/\ndiff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect\nindex d2d804f5b0..0e6389f532 100644\n--- a/t/chainlint/semicolon.expect\n+++ b/t/chainlint/semicolon.expect\n@@ -11,10 +11,10 @@\n \tcat foo; ?!SEMI?! echo bar\n >) &&\n (\n-\tfoo; ?!SEMI?!\n+\tfoo;\n >) &&\n (\n cd foo &&\n \tfor i in a b c; do\n-\t\techo; ?!SEMI?!\n+\t\techo;\n >\tdone)\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443955","messageId":"20211213063059.19424-10-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 09/15] chainlint.sed: drop unnecessary distinction between ?!AMP?! and ?!SEMI?!","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:53Z","receivedAt":"2021-12-13T06:32:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From inception, when chainlint.sed encountered a line using semicolon to\nseparate commands rather than `&&`, it would insert a ?!SEMI?!\nannotation at the beginning of the line rather ?!AMP?! even though the\n&&-chain is also broken by the semicolon. Given a line such as:\n\n ?!SEMI?! cmd1; cmd2 &&\n\nthe ?!SEMI?! annotation makes it easier to see what the problem is than\nif the output had been:\n\n ?!AMP?! cmd1; cmd2 &&\n\nwhich might confuse the test author into thinking that the linter is\nbroken (since the line clearly ends with `&&`).\n\nHowever, now that the ?!AMP?! an ?!SEMI?! annotations are inserted at\nthe point of breakage rather than at the beginning of the line, and\ntaking into account that both represent a broken &&-chain, there is\nlittle reason to distinguish between the two. Using ?!AMP?! alone is\nsufficient to point the test author at the problem. For instance, in:\n\n cmd1; ?!AMP?! cmd2 &&\n cmd3\n\nit is clear that the &&-chain is broken between `cmd1` and `cmd2`.\nLikewise, in:\n\n cmd1 && cmd2 ?!AMP?!\n cmd3\n\nit is clear that the &&-chain is broken between `cmd2` and `cmd3`.\nFinally, in:\n\n cmd1; ?!AMP?! cmd2 ?!AMP?!\n cmd3\n\nit is clear that the &&-chain is broken between each command.\n\nHence, there is no longer a good reason to make a distinction between a\nbroken &&-chain due to a semicolon and a broken chain due to a missing\n`&&` at end-of-line. Therefore, drop the ?!SEMI?! annotation and use\n?!AMP?! exclusively.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.sed | 21 ++++++++++-----------\n t/chainlint/negated-one-liner.expect | 4 ++--\n t/chainlint/one-liner.expect | 6 +++---\n t/chainlint/semicolon.expect | 10 +++++-----\n t/chainlint/subshell-one-liner.expect | 8 ++++----\n 5 files changed, 24 insertions(+), 25 deletions(-)\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex 91077b6e26..f5fcca09ca 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -24,9 +24,9 @@\n # in order to avoid misinterpreting the \")\" in constructs such as \"x=$(...)\"\n # and \"case $x in *)\" as ending the subshell.\n #\n-# Lines missing a final \"&&\" are flagged with \"?!AMP?!\", and lines which chain\n-# commands with \";\" internally rather than \"&&\" are flagged \"?!SEMI?!\". A line\n-# may be flagged for both violations.\n+# Lines missing a final \"&&\" are flagged with \"?!AMP?!\", as are lines which\n+# chain commands with \";\" internally rather than \"&&\". A line may be flagged\n+# for both violations.\n #\n # Detection of a missing &&-link in a multi-line subshell is complicated by the\n # fact that the last statement before the closing \")\" must not end with \"&&\".\n@@ -47,9 +47,8 @@\n # \"?!AMP?!\" violation is removed from the \"bar\" line (retrieved from the \"hold\"\n # area) since the final statement of a subshell must not end with \"&&\". The\n # final line of a subshell may still break the &&-chain by using \";\" internally\n-# to chain commands together rather than \"&&\", so \"?!SEMI?!\" is not removed\n-# from such a line; however, if the line ends with \"?!SEMI?!\", then the \";\" is\n-# harmless and the annotation is removed.\n+# to chain commands together rather than \"&&\", but an internal \"?!AMP?!\" is\n+# never removed from a line even though a line-ending \"?!AMP?!\" might be.\n #\n # Care is taken to recognize the last _statement_ of a multi-line subshell, not\n # necessarily the last textual _line_ within the subshell, since &&-chaining\n@@ -127,7 +126,7 @@ b\n # \"&&\" (but not \";\" in a string)\n :oneline\n /;/{\n-\t/\"[^\"]*;[^\"]*\"/!s/;/; ?!SEMI?!/\n+\t/\"[^\"]*;[^\"]*\"/!s/;/; ?!AMP?!/\n }\n b\n \n@@ -231,7 +230,7 @@ s/.*\\n//\n # string and not \";;\" in one-liner \"case...esac\")\n /;/{\n \t/;;/!{\n-\t\t/\"[^\"]*;[^\"]*\"/!s/;/; ?!SEMI?!/\n+\t\t/\"[^\"]*;[^\"]*\"/!s/;/; ?!AMP?!/\n \t}\n }\n # line ends with pipe \"...|\" -- valid; not missing \"&&\"\n@@ -304,7 +303,7 @@ bcase\n # that line legitimately lacks \"&&\"\n :else\n x\n-s/\\( ?!SEMI?!\\)* ?!AMP?!$//\n+s/\\( ?!AMP?!\\)* ?!AMP?!$//\n x\n bcont\n \n@@ -312,7 +311,7 @@ bcont\n # \"suspect\" from final contained line since that line legitimately lacks \"&&\"\n :done\n x\n-s/\\( ?!SEMI?!\\)* ?!AMP?!$//\n+s/\\( ?!AMP?!\\)* ?!AMP?!$//\n x\n # is 'done' or 'fi' cuddled with \")\" to close subshell?\n /done.*)/bclose\n@@ -355,7 +354,7 @@ bblock\n # since that line legitimately lacks \"&&\" and exit subshell loop\n :clssolo\n x\n-s/\\( ?!SEMI?!\\)* ?!AMP?!$//\n+s/\\( ?!AMP?!\\)* ?!AMP?!$//\n p\n x\n s/^/>/\ndiff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect\nindex 60baf84b7a..ad4c2d949e 100644\n--- a/t/chainlint/negated-one-liner.expect\n+++ b/t/chainlint/negated-one-liner.expect\n@@ -1,5 +1,5 @@\n ! (foo && bar) &&\n ! (foo && bar) >baz &&\n \n-! (foo; ?!SEMI?! bar) &&\n-! (foo; ?!SEMI?! bar) >baz\n+! (foo; ?!AMP?! bar) &&\n+! (foo; ?!AMP?! bar) >baz\ndiff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect\nindex 3b46554728..57a7a444c1 100644\n--- a/t/chainlint/one-liner.expect\n+++ b/t/chainlint/one-liner.expect\n@@ -2,8 +2,8 @@\n (foo && bar) |\n (foo && bar) >baz &&\n \n-(foo; ?!SEMI?! bar) &&\n-(foo; ?!SEMI?! bar) |\n-(foo; ?!SEMI?! bar) >baz &&\n+(foo; ?!AMP?! bar) &&\n+(foo; ?!AMP?! bar) |\n+(foo; ?!AMP?! bar) >baz &&\n \n (foo \"bar; baz\")\ndiff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect\nindex 0e6389f532..54a08ce582 100644\n--- a/t/chainlint/semicolon.expect\n+++ b/t/chainlint/semicolon.expect\n@@ -1,14 +1,14 @@\n (\n-\tcat foo ; ?!SEMI?! echo bar ?!AMP?!\n-\tcat foo ; ?!SEMI?! echo bar\n+\tcat foo ; ?!AMP?! echo bar ?!AMP?!\n+\tcat foo ; ?!AMP?! echo bar\n >) &&\n (\n-\tcat foo ; ?!SEMI?! echo bar &&\n-\tcat foo ; ?!SEMI?! echo bar\n+\tcat foo ; ?!AMP?! echo bar &&\n+\tcat foo ; ?!AMP?! echo bar\n >) &&\n (\n \techo \"foo; bar\" &&\n-\tcat foo; ?!SEMI?! echo bar\n+\tcat foo; ?!AMP?! echo bar\n >) &&\n (\n \tfoo;\ndiff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect\nindex 432217801b..4b44632b09 100644\n--- a/t/chainlint/subshell-one-liner.expect\n+++ b/t/chainlint/subshell-one-liner.expect\n@@ -2,13 +2,13 @@\n \t(foo && bar) &&\n \t(foo && bar) |\n \t(foo && bar) >baz &&\n-\t(foo; ?!SEMI?! bar) &&\n-\t(foo; ?!SEMI?! bar) |\n-\t(foo; ?!SEMI?! bar) >baz &&\n+\t(foo; ?!AMP?! bar) &&\n+\t(foo; ?!AMP?! bar) |\n+\t(foo; ?!AMP?! bar) >baz &&\n \t(foo || exit 1) &&\n \t(foo || exit 1) |\n \t(foo || exit 1) >baz &&\n \t(foo && bar) ?!AMP?!\n-\t(foo && bar; ?!SEMI?! baz) ?!AMP?!\n+\t(foo && bar; ?!AMP?! baz) ?!AMP?!\n \tfoobar\n >)\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443956","messageId":"20211213063059.19424-11-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 10/15] chainlint.sed: drop subshell-closing \">\" annotation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:54Z","receivedAt":"2021-12-13T06:32:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"chainlint.sed inserts a \">\" annotation at the beginning of a line to\nsignal that its heuristics have identified an end-of-subshell. This was\nuseful as a debugging aid during development of the script, but it has\nno value to test writers and might even confuse them into thinking that\nthe linter is misbehaving by inserting line-noise into the shell code it\nis validating. Moreover, its presence also potentially makes it\ndifficult to reuse the chainlint self-test \"expect\" output should a more\ncapable linter ever be developed. Therefore, drop the \">\" annotation.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.sed | 9 ---------\n t/chainlint/arithmetic-expansion.expect | 4 ++--\n t/chainlint/bash-array.expect | 4 ++--\n t/chainlint/blank-line.expect | 2 +-\n t/chainlint/block.expect | 2 +-\n t/chainlint/broken-chain.expect | 2 +-\n t/chainlint/case.expect | 6 +++---\n .../close-nested-and-parent-together.expect | 2 +-\n t/chainlint/close-subshell.expect | 16 ++++++++--------\n t/chainlint/command-substitution.expect | 4 ++--\n t/chainlint/comment.expect | 2 +-\n t/chainlint/complex-if-in-cuddled-loop.expect | 2 +-\n t/chainlint/cuddled-if-then-else.expect | 2 +-\n t/chainlint/cuddled-loop.expect | 2 +-\n t/chainlint/cuddled.expect | 10 +++++-----\n t/chainlint/exit-loop.expect | 6 +++---\n t/chainlint/exit-subshell.expect | 2 +-\n t/chainlint/for-loop.expect | 2 +-\n t/chainlint/here-doc-close-subshell.expect | 2 +-\n .../here-doc-multi-line-command-subst.expect | 4 ++--\n t/chainlint/here-doc-multi-line-string.expect | 2 +-\n t/chainlint/if-in-loop.expect | 2 +-\n t/chainlint/if-then-else.expect | 4 ++--\n t/chainlint/incomplete-line.expect | 2 +-\n t/chainlint/inline-comment.expect | 4 ++--\n t/chainlint/loop-in-if.expect | 2 +-\n ...multi-line-nested-command-substitution.expect | 10 +++++-----\n t/chainlint/multi-line-string.expect | 4 ++--\n t/chainlint/nested-cuddled-subshell.expect | 14 +++++++-------\n t/chainlint/nested-here-doc.expect | 2 +-\n t/chainlint/nested-subshell-comment.expect | 4 ++--\n t/chainlint/nested-subshell.expect | 6 +++---\n t/chainlint/p4-filespec.expect | 2 +-\n t/chainlint/pipe.expect | 2 +-\n t/chainlint/semicolon.expect | 10 +++++-----\n t/chainlint/subshell-here-doc.expect | 4 ++--\n t/chainlint/subshell-one-liner.expect | 2 +-\n t/chainlint/t7900-subtree.expect | 6 +++---\n t/chainlint/while-loop.expect | 2 +-\n 39 files changed, 80 insertions(+), 89 deletions(-)\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex f5fcca09ca..2689e13636 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -76,12 +76,6 @@\n # newline, thus the closing here-doc tag has been found. The closing tag line\n # and the \"<...>\" prefix on the target line are then discarded, leaving just\n # the target line \"cat >out\".\n-#\n-# To facilitate regression testing (and manual debugging), a \">\" annotation is\n-# applied to the line containing \")\" which closes a subshell, \">>\" to a line\n-# closing a nested subshell, and \">>>\" to a line closing both at once. This\n-# makes it easy to detect whether the heuristics correctly identify\n-# end-of-subshell.\n #------------------------------------------------------------------------------\n \n # incomplete line -- slurp up next line\n@@ -337,7 +331,6 @@ n\n x\n bnstslrp\n :nstcl\n-s/^/>>/\n # is it \"))\" which closes nested and parent subshells?\n /)[ \t]*)/bslurp\n bchkchn\n@@ -357,7 +350,6 @@ x\n s/\\( ?!AMP?!\\)* ?!AMP?!$//\n p\n x\n-s/^/>/\n b\n \n # found closing \"...)\" -- exit subshell loop\n@@ -365,5 +357,4 @@ b\n x\n p\n x\n-s/^/>/\n b\ndiff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect\nindex 56cd5b69f5..46ee1046af 100644\n--- a/t/chainlint/arithmetic-expansion.expect\n+++ b/t/chainlint/arithmetic-expansion.expect\n@@ -2,8 +2,8 @@\n \tfoo &&\n \tbar=$((42 + 1)) &&\n \tbaz\n->) &&\n+) &&\n (\n \tbar=$((42 + 1)) ?!AMP?!\n \tbaz\n->)\n+)\ndiff --git a/t/chainlint/bash-array.expect b/t/chainlint/bash-array.expect\nindex c4a830d1c1..4c34eaee45 100644\n--- a/t/chainlint/bash-array.expect\n+++ b/t/chainlint/bash-array.expect\n@@ -2,9 +2,9 @@\n \tfoo &&\n \tbar=(gumbo stumbo wumbo) &&\n \tbaz\n->) &&\n+) &&\n (\n \tfoo &&\n \tbar=${#bar[@]} &&\n \tbaz\n->)\n+)\ndiff --git a/t/chainlint/blank-line.expect b/t/chainlint/blank-line.expect\nindex 3be939ed38..f76fde1ffb 100644\n--- a/t/chainlint/blank-line.expect\n+++ b/t/chainlint/blank-line.expect\n@@ -1,4 +1,4 @@\n (\n \tnothing &&\n \tsomething\n->)\n+)\ndiff --git a/t/chainlint/block.expect b/t/chainlint/block.expect\nindex 6333237cb2..da60257ebc 100644\n--- a/t/chainlint/block.expect\n+++ b/t/chainlint/block.expect\n@@ -9,4 +9,4 @@\n \t\techo c\n \t} ?!AMP?!\n \tbaz\n->)\n+)\ndiff --git a/t/chainlint/broken-chain.expect b/t/chainlint/broken-chain.expect\nindex 0960a8c7c0..cfb58fb6b9 100644\n--- a/t/chainlint/broken-chain.expect\n+++ b/t/chainlint/broken-chain.expect\n@@ -3,4 +3,4 @@\n \tbar ?!AMP?!\n \tbaz &&\n \twop\n->)\n+)\ndiff --git a/t/chainlint/case.expect b/t/chainlint/case.expect\nindex a4b92d4613..31f280d8ce 100644\n--- a/t/chainlint/case.expect\n+++ b/t/chainlint/case.expect\n@@ -4,16 +4,16 @@\n \t*) bar ;;\n \tesac &&\n \tfoobar\n->) &&\n+) &&\n (\n \tcase \"$x\" in\n \tx) foo ;;\n \t*) bar ;;\n \tesac ?!AMP?!\n \tfoobar\n->) &&\n+) &&\n (\n \tcase \"$x\" in 1) true;; esac &&\n \tcase \"$y\" in 2) false;; esac ?!AMP?!\n \tfoobar\n->)\n+)\ndiff --git a/t/chainlint/close-nested-and-parent-together.expect b/t/chainlint/close-nested-and-parent-together.expect\nindex 2a910f9d66..5ef509eb49 100644\n--- a/t/chainlint/close-nested-and-parent-together.expect\n+++ b/t/chainlint/close-nested-and-parent-together.expect\n@@ -1,4 +1,4 @@\n (\n cd foo &&\n \t(bar &&\n->>>\t\tbaz))\n+\t\tbaz))\ndiff --git a/t/chainlint/close-subshell.expect b/t/chainlint/close-subshell.expect\nindex 184688718a..0f87db9ae6 100644\n--- a/t/chainlint/close-subshell.expect\n+++ b/t/chainlint/close-subshell.expect\n@@ -1,25 +1,25 @@\n (\n \tfoo\n->) &&\n+) &&\n (\n \tbar\n->) >out &&\n+) >out &&\n (\n \tbaz\n->) 2>err &&\n+) 2>err &&\n (\n \tboo\n->) ) | wuzzle &&\n+) | wuzzle &&\n (\n \tbop\n->) | fazz \tfozz &&\n+) | fazz \tfozz &&\n (\n \tbup\n->) |\n+) |\n fuzzle &&\n (\n \tyop\n->)\n+)\ndiff --git a/t/chainlint/command-substitution.expect b/t/chainlint/command-substitution.expect\nindex f276067b7b..c72e4df9e7 100644\n--- a/t/chainlint/command-substitution.expect\n+++ b/t/chainlint/command-substitution.expect\n@@ -2,8 +2,8 @@\n \tfoo &&\n \tbar=$(gobble) &&\n \tbaz\n->) &&\n+) &&\n (\n \tbar=$(gobble blocks) ?!AMP?!\n \tbaz\n->)\n+)\ndiff --git a/t/chainlint/comment.expect b/t/chainlint/comment.expect\nindex 3be939ed38..f76fde1ffb 100644\n--- a/t/chainlint/comment.expect\n+++ b/t/chainlint/comment.expect\n@@ -1,4 +1,4 @@\n (\n \tnothing &&\n \tsomething\n->)\n+)\ndiff --git a/t/chainlint/complex-if-in-cuddled-loop.expect b/t/chainlint/complex-if-in-cuddled-loop.expect\nindex 9674b88cf2..b8aa626ed0 100644\n--- a/t/chainlint/complex-if-in-cuddled-loop.expect\n+++ b/t/chainlint/complex-if-in-cuddled-loop.expect\n@@ -6,5 +6,5 @@ for i in a b c; do\n else\n echo >file\n fi\n-> done) &&\n+ done) &&\n test ! -f file\ndiff --git a/t/chainlint/cuddled-if-then-else.expect b/t/chainlint/cuddled-if-then-else.expect\nindex ab2a026fbc..4e089b087a 100644\n--- a/t/chainlint/cuddled-if-then-else.expect\n+++ b/t/chainlint/cuddled-if-then-else.expect\n@@ -3,5 +3,5 @@ if test -z \"\"; then\n echo empty\n else\n echo bizzy\n-> fi) &&\n+ fi) &&\n echo foobar\ndiff --git a/t/chainlint/cuddled-loop.expect b/t/chainlint/cuddled-loop.expect\nindex 8c0260d7f1..7932303763 100644\n--- a/t/chainlint/cuddled-loop.expect\n+++ b/t/chainlint/cuddled-loop.expect\n@@ -1,5 +1,5 @@\n (\n while read x\n do foobar bop || exit 1\n-> done ) &&\n+) &&\n \n (\n cd foo ?!AMP?!\n \tbar\n->) &&\n+) &&\n \n (\n \tcd foo &&\n->\tbar) &&\n+\tbar) &&\n \n (\n cd foo &&\n->\tbar) &&\n+\tbar) &&\n \n (\n cd foo ?!AMP?!\n->\tbar)\n+\tbar)\ndiff --git a/t/chainlint/exit-loop.expect b/t/chainlint/exit-loop.expect\nindex 84d8bdebc0..f76aa60466 100644\n--- a/t/chainlint/exit-loop.expect\n+++ b/t/chainlint/exit-loop.expect\n@@ -5,7 +5,7 @@\n \t\tbar &&\n \t\tbaz\n \tdone\n->) &&\n+) &&\n (\n \twhile true\n \tdo\n@@ -13,7 +13,7 @@\n \t\tbar &&\n \t\tbaz\n \tdone\n->) &&\n+) &&\n (\n \ti=0 &&\n \twhile test $i -lt 10\n@@ -21,4 +21,4 @@\n \t\techo $i || exit\n \t\ti=$(($i + 1))\n \tdone\n->)\n+)\ndiff --git a/t/chainlint/exit-subshell.expect b/t/chainlint/exit-subshell.expect\nindex bf78454f74..da80339f78 100644\n--- a/t/chainlint/exit-subshell.expect\n+++ b/t/chainlint/exit-subshell.expect\n@@ -2,4 +2,4 @@\n \tfoo || exit 1\n \tbar &&\n \tbaz\n->)\n+)\ndiff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect\nindex dc209e21bd..b74df064c5 100644\n--- a/t/chainlint/for-loop.expect\n+++ b/t/chainlint/for-loop.expect\n@@ -8,4 +8,4 @@\n \t\techo $i &&\n \t\tcat $i\n \tdone\n->)\n+)\ndiff --git a/t/chainlint/here-doc-close-subshell.expect b/t/chainlint/here-doc-close-subshell.expect\nindex f011e335e5..e748526570 100644\n--- a/t/chainlint/here-doc-close-subshell.expect\n+++ b/t/chainlint/here-doc-close-subshell.expect\n@@ -1,2 +1,2 @@\n (\n->\tcat)\n+\tcat)\ndiff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect\nindex 3a35bb014c..f1248f8ade 100644\n--- a/t/chainlint/here-doc-multi-line-command-subst.expect\n+++ b/t/chainlint/here-doc-multi-line-command-subst.expect\n@@ -1,5 +1,5 @@\n (\n \tx=$(bobble &&\n->>\t\twiffle) ?!AMP?!\n+\t\twiffle) ?!AMP?!\n \techo $x\n->)\n+)\ndiff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect\nindex a3b9a5472d..7e883b252e 100644\n--- a/t/chainlint/here-doc-multi-line-string.expect\n+++ b/t/chainlint/here-doc-multi-line-string.expect\n@@ -1,4 +1,4 @@\n (\n \tcat && echo \"multi-line\tstring\" ?!AMP?!\n \tbap\n->)\n+)\ndiff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect\nindex 7d91837269..03b82a3e58 100644\n--- a/t/chainlint/if-in-loop.expect\n+++ b/t/chainlint/if-in-loop.expect\n@@ -9,4 +9,4 @@\n \t\tfoo\n \tdone ?!AMP?!\n \tbar\n->)\n+)\ndiff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect\nindex 3055d5606c..debcf7b756 100644\n--- a/t/chainlint/if-then-else.expect\n+++ b/t/chainlint/if-then-else.expect\n@@ -11,10 +11,10 @@\n \t\tcat\n \tfi ?!AMP?!\n \techo poodle\n->) &&\n+) &&\n (\n \tif test -n \"\"; then\n \t\techo very &&\n \t\techo empty\n \tfi\n->)\n+)\ndiff --git a/t/chainlint/incomplete-line.expect b/t/chainlint/incomplete-line.expect\nindex 2f3ebabdc2..ffac8f9018 100644\n--- a/t/chainlint/incomplete-line.expect\n+++ b/t/chainlint/incomplete-line.expect\n@@ -1,4 +1,4 @@\n line 1 line 2 line 3 line 4 &&\n (\n \tline 5 \tline 6 \tline 7 \tline 8\n->)\n+)\ndiff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect\nindex 3d655a32b0..f6b42757d2 100644\n--- a/t/chainlint/inline-comment.expect\n+++ b/t/chainlint/inline-comment.expect\n@@ -2,8 +2,8 @@\n \tfoobar &&\n \tbarfoo ?!AMP?!\n \tflibble \"not a # comment\"\n->) &&\n+) &&\n \n (\n cd foo &&\n->\tflibble \"not a # comment\")\n+\tflibble \"not a # comment\")\ndiff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect\nindex cebd3ae95e..e1be42376c 100644\n--- a/t/chainlint/loop-in-if.expect\n+++ b/t/chainlint/loop-in-if.expect\n@@ -9,4 +9,4 @@\n \t\tfoo\n \tfi ?!AMP?!\n \tbar\n->)\n+)\ndiff --git a/t/chainlint/multi-line-nested-command-substitution.expect b/t/chainlint/multi-line-nested-command-substitution.expect\nindex 59b6c8b850..300058341b 100644\n--- a/t/chainlint/multi-line-nested-command-substitution.expect\n+++ b/t/chainlint/multi-line-nested-command-substitution.expect\n@@ -3,16 +3,16 @@\n \tx=$(\n \t\techo bar |\n \t\tcat\n->>\t) &&\n+\t) &&\n \techo ok\n->) |\n+) |\n sort &&\n (\n \tbar &&\n \tx=$(echo bar |\n \t\tcat\n->>\t) &&\n+\t) &&\n \ty=$(echo baz |\n->>\t\tfip) &&\n+\t\tfip) &&\n \techo fail\n->)\n+)\ndiff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect\nindex f1be2baf0a..ab0dadf748 100644\n--- a/t/chainlint/multi-line-string.expect\n+++ b/t/chainlint/multi-line-string.expect\n@@ -2,8 +2,8 @@\n \tx=\"line 1\t\tline 2\t\tline 3\" &&\n \ty=\"line 1\t\tline2\" ?!AMP?!\n \tfoobar\n->) &&\n+) &&\n (\n \techo \"xyz\" \"abc\t\tdef\t\tghi\" &&\n \tbarfoo\n->)\n+)\ndiff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect\nindex aa522658ed..2a86885ee6 100644\n--- a/t/chainlint/nested-cuddled-subshell.expect\n+++ b/t/chainlint/nested-cuddled-subshell.expect\n@@ -1,19 +1,19 @@\n (\n \t(cd foo &&\n \t\tbar\n->>\t) &&\n+\t) &&\n \t(cd foo &&\n \t\tbar\n->>\t) ?!AMP?!\n+\t) ?!AMP?!\n \t(\n \t\tcd foo &&\n->>\t\tbar) &&\n+\t\tbar) &&\n \t(\n \t\tcd foo &&\n->>\t\tbar) ?!AMP?!\n+\t\tbar) ?!AMP?!\n \t(cd foo &&\n->>\t\tbar) &&\n+\t\tbar) &&\n \t(cd foo &&\n->>\t\tbar) ?!AMP?!\n+\t\tbar) ?!AMP?!\n \tfoobar\n->)\n+)\ndiff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect\nindex f9604d3fac..2a51205d32 100644\n--- a/t/chainlint/nested-here-doc.expect\n+++ b/t/chainlint/nested-here-doc.expect\n@@ -4,4 +4,4 @@ cat >foop &&\n \tcat &&\n \tcat ?!AMP?!\n \tfoobar\n->)\n+)\ndiff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect\nindex 925e49bae9..9138cf386d 100644\n--- a/t/chainlint/nested-subshell-comment.expect\n+++ b/t/chainlint/nested-subshell-comment.expect\n@@ -6,6 +6,6 @@\n \t\t# minor numbers of cows (or do they?)\n \t\tbaz &&\n \t\tsnaff\n->>\t) ?!AMP?!\n+\t) ?!AMP?!\n \tfuzzy\n->)\n+)\ndiff --git a/t/chainlint/nested-subshell.expect b/t/chainlint/nested-subshell.expect\nindex c8165ad19e..41a48adaa2 100644\n--- a/t/chainlint/nested-subshell.expect\n+++ b/t/chainlint/nested-subshell.expect\n@@ -3,10 +3,10 @@\n \t(\n \t\techo a &&\n \t\techo b\n->>\t) >file &&\n+\t) >file &&\n \tcd foo &&\n \t(\n \t\techo a\n \t\techo b\n->>\t) >file\n->)\n+\t) >file\n+)\ndiff --git a/t/chainlint/p4-filespec.expect b/t/chainlint/p4-filespec.expect\nindex 98b3d881fd..1290fd1ff2 100644\n--- a/t/chainlint/p4-filespec.expect\n+++ b/t/chainlint/p4-filespec.expect\n@@ -1,4 +1,4 @@\n (\n \tp4 print -1 //depot/fiddle#42 >file &&\n \tfoobar\n->)\n+)\ndiff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect\nindex ede6bcc607..2cfc028297 100644\n--- a/t/chainlint/pipe.expect\n+++ b/t/chainlint/pipe.expect\n@@ -5,4 +5,4 @@\n \tfish |\n \tcow ?!AMP?!\n \tsunder\n->)\n+)\ndiff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect\nindex 54a08ce582..05141a96cf 100644\n--- a/t/chainlint/semicolon.expect\n+++ b/t/chainlint/semicolon.expect\n@@ -1,20 +1,20 @@\n (\n \tcat foo ; ?!AMP?! echo bar ?!AMP?!\n \tcat foo ; ?!AMP?! echo bar\n->) &&\n+) &&\n (\n \tcat foo ; ?!AMP?! echo bar &&\n \tcat foo ; ?!AMP?! echo bar\n->) &&\n+) &&\n (\n \techo \"foo; bar\" &&\n \tcat foo; ?!AMP?! echo bar\n->) &&\n+) &&\n (\n \tfoo;\n->) &&\n+) &&\n (\n cd foo &&\n \tfor i in a b c; do\n \t\techo;\n->\tdone)\n+\tdone)\ndiff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect\nindex 9d3f25b3f5..b7250ca753 100644\n--- a/t/chainlint/subshell-here-doc.expect\n+++ b/t/chainlint/subshell-here-doc.expect\n@@ -2,9 +2,9 @@\n \techo wobba \t gorgo snoot \t wafta snurb &&\n \tcat >bip ?!AMP?!\n \techo >bop\n->) &&\n+) &&\n (\n \tcat >bup &&\n \tcat >bup3 &&\n \tmeep\n->)\n+)\ndiff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect\nindex 4b44632b09..b7015361bf 100644\n--- a/t/chainlint/subshell-one-liner.expect\n+++ b/t/chainlint/subshell-one-liner.expect\n@@ -11,4 +11,4 @@\n \t(foo && bar) ?!AMP?!\n \t(foo && bar; ?!AMP?! baz) ?!AMP?!\n \tfoobar\n->)\n+)\ndiff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect\nindex f769244ef6..215aca01c2 100644\n--- a/t/chainlint/t7900-subtree.expect\n+++ b/t/chainlint/t7900-subtree.expect\n@@ -1,10 +1,10 @@\n (\n \tchks=\"sub1sub2sub3sub4\" &&\n \tchks_sub=$(cat | sed \"s,^,sub dir/,\"\n->>) &&\n+) &&\n \tchkms=\"main-sub1main-sub2main-sub3main-sub4\" &&\n \tchkms_sub=$(cat | sed \"s,^,sub dir/,\"\n->>) &&\n+) &&\n \tsubfiles=$(git ls-files) &&\n \tcheck_equal \"$subfiles\" \"$chkms$chks\"\n->)\n+)\ndiff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect\nindex f8b9fcf62b..e2813b378e 100644\n--- a/t/chainlint/while-loop.expect\n+++ b/t/chainlint/while-loop.expect\n@@ -8,4 +8,4 @@\n \t\techo foo &&\n \t\tcat bar\n \tdone\n->)\n+)\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443957","messageId":"20211213063059.19424-12-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 11/15] chainlint.sed: make here-doc \"<<-\" operator recognition more POSIX-like","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:55Z","receivedAt":"2021-12-13T06:32:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"According to POSIX, \"<<\" and \"<<-\" are distinct shell operators. For the\nlatter to be recognized, no whitespace is allowed before the \"-\", though\nwhitespace is allowed after the operator. However, the chainlint\npatterns which identify here-docs are both too loose and too tight,\nincorrectly allowing whitespace between \"<<\" and \"-\" but disallowing it\nbetween \"-\" and the here-doc tag. Fix the patterns to better match\nPOSIX.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.sed | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex 2689e13636..b382746526 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -88,8 +88,8 @@\n \n # here-doc -- swallow it to avoid false hits within its body (but keep the\n # command to which it was attached)\n-/<<[ \t]*[-\\\\'\"]*[A-Za-z0-9_]/ {\n-\ts/^\\(.*\\)<<[ \t]*[-\\\\'\"]*\\([A-Za-z0-9_][A-Za-z0-9_]*\\)['\"]*/<\\2>\\1<\\1<\\1<\\1<>>>>>> theirs\"\n\nThis problem went unnoticed because chainlint.sed is not a real parser,\nbut rather applies heuristics to pretend to understand shell code. In\nthis case, it saw what it thought was a here-doc operator (`<< ours`),\nand fell off the end of the test looking for the closing tag \"ours\"\nwhich it never found, thus swallowed the remainder of the test without\nchecking it for &&-chain breakage.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.sed | 8 ++++++--\n t/chainlint/not-heredoc.expect | 14 ++++++++++++++\n t/chainlint/not-heredoc.test | 16 ++++++++++++++++\n 3 files changed, 36 insertions(+), 2 deletions(-)\n create mode 100644 t/chainlint/not-heredoc.expect\n create mode 100644 t/chainlint/not-heredoc.test\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex b382746526..2f786f890d 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -89,6 +89,7 @@\n # here-doc -- swallow it to avoid false hits within its body (but keep the\n # command to which it was attached)\n /<<-*[ \t]*[\\\\'\"]*[A-Za-z0-9_]/ {\n+\t/\"[^\"]*<<[^\"]*\"/bnotdoc\n \ts/^\\(.*\\)<<-*[ \t]*[\\\\'\"]*\\([A-Za-z0-9_][A-Za-z0-9_]*\\)['\"]*/<\\2>\\1<]*>//\n \ts/\\n.*$//\n }\n+:notdoc\n \n # one-liner \"(...) &&\"\n /^[ \t]*!*[ \t]*(..*)[ \t]*&&[ \t]*$/boneline\n@@ -151,8 +153,10 @@ s/.*\\n//\n \t/\"[^'\"]*'[^'\"]*\"/!bsqstr\n }\n :folded\n-# here-doc -- swallow it\n-/<<-*[ \t]*[\\\\'\"]*[A-Za-z0-9_]/bheredoc\n+# here-doc -- swallow it (but not \"<<\" in a string)\n+/<<-*[ \t]*[\\\\'\"]*[A-Za-z0-9_]/{\n+\t/\"[^\"]*<<[^\"]*\"/!bheredoc\n+}\n # comment or empty line -- discard since final non-comment, non-empty line\n # before closing \")\", \"done\", \"elsif\", \"else\", or \"fi\" will need to be\n # re-visited to drop \"suspect\" marking since final line of those constructs\ndiff --git a/t/chainlint/not-heredoc.expect b/t/chainlint/not-heredoc.expect\nnew file mode 100644\nindex 0000000000..2e9bb135fe\n--- /dev/null\n+++ b/t/chainlint/not-heredoc.expect\n@@ -0,0 +1,14 @@\n+echo \"<<<<<<< ours\" &&\n+echo ourside &&\n+echo \"=======\" &&\n+echo theirside &&\n+echo \">>>>>>> theirs\" &&\n+\n+(\n+\techo \"<<<<<<< ours\" &&\n+\techo ourside &&\n+\techo \"=======\" &&\n+\techo theirside &&\n+\techo \">>>>>>> theirs\" ?!AMP?!\n+\tpoodle\n+) >merged\ndiff --git a/t/chainlint/not-heredoc.test b/t/chainlint/not-heredoc.test\nnew file mode 100644\nindex 0000000000..9aa57346cd\n--- /dev/null\n+++ b/t/chainlint/not-heredoc.test\n@@ -0,0 +1,16 @@\n+# LINT: \"<< ours\" inside string is not here-doc\n+echo \"<<<<<<< ours\" &&\n+echo ourside &&\n+echo \"=======\" &&\n+echo theirside &&\n+echo \">>>>>>> theirs\" &&\n+\n+(\n+# LINT: \"<< ours\" inside string is not here-doc\n+\techo \"<<<<<<< ours\" &&\n+\techo ourside &&\n+\techo \"=======\" &&\n+\techo theirside &&\n+\techo \">>>>>>> theirs\"\n+\tpoodle\n+) >merged\n-- \n2.34.1.397.gfae76fe5da\n\n"},{"id":"443959","messageId":"20211213063059.19424-14-sunshine@sunshineco.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"[PATCH 13/15] chainlint.sed: stop throwing away here-doc tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T06:30:57Z","receivedAt":"2021-12-13T06:32:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The purpose of chainlint is to highlight problems it finds in test code\nby inserting annotations at the location of each problem. Arbitrarily\neliding bits of the code it is checking is not helpful, yet this is\nexactly what chainlint.sed does by cavalierly and unnecessarily dropping\nthe here-doc operator and tag; i.e. `cat <\n---\n t/chainlint.sed | 24 ++++++++++++-------\n t/chainlint/for-loop.expect | 2 +-\n t/chainlint/here-doc-close-subshell.expect | 2 +-\n .../here-doc-multi-line-command-subst.expect | 2 +-\n t/chainlint/here-doc-multi-line-string.expect | 2 +-\n t/chainlint/here-doc.expect | 8 +++----\n t/chainlint/if-then-else.expect | 2 +-\n t/chainlint/nested-here-doc.expect | 6 ++---\n t/chainlint/subshell-here-doc.expect | 10 ++++----\n t/chainlint/t7900-subtree.expect | 4 ++--\n t/chainlint/while-loop.expect | 2 +-\n 11 files changed, 35 insertions(+), 29 deletions(-)\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex 2f786f890d..0e575c0c62 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -62,20 +62,20 @@\n # receives similar treatment.\n #\n # Swallowing here-docs with arbitrary tags requires a bit of finesse. When a\n-# line such as \"cat <out\" is seen, the here-doc tag is moved to the front\n-# of the line enclosed in angle brackets as a sentinel, giving \"cat >out\".\n+# line such as \"cat <cat <\\n\\1$/ is attempted to see if\n # the content inside \"<...>\" matches the entirety of the newly-read line. For\n # instance, if the next line read is \"some data\", when concatenated with the\n-# target line, it becomes \"cat >out\\nsome data\", and a match is attempted\n+# target line, it becomes \"cat <cat >out\\nEOF\",\n+# encountered, it is appended to the target line giving \"cat <\" does match the text following the\n # newline, thus the closing here-doc tag has been found. The closing tag line\n # and the \"<...>\" prefix on the target line are then discarded, leaving just\n-# the target line \"cat >out\".\n+# the target line \"cat <\\1<\\1\\2/\n \t:hered\n \tN\n \t/^<\\([^>]*\\)>.*\\n[ \t]*\\1[ \t]*$/!{\n@@ -238,6 +237,7 @@ s/.*\\n//\n :cont\n # retrieve and print previous line\n x\n+s/?!HERE?!/<\\1<\\1?!HERE?!\\2\\3/\n :hdocsub\n N\n /^<\\([^>]*\\)>.*\\n[ \t]*\\1[ \t]*$/!{\n@@ -293,6 +292,7 @@ bfolded\n # found \"case ... in\" -- pass through untouched\n :case\n x\n+s/?!HERE?!/<foo &&\n+cat <<-Arbitrary_Tag_42 >foo &&\n \n-cat >boo &&\n+cat <boo &&\n \n-horticulture\n+horticulture <foop &&\n+cat <foop &&\n \n (\n-\tcat &&\n-\tcat ?!AMP?!\n+\tcat <<-INPUT_END &&\n+\tcat <<-EOT ?!AMP?!\n \tfoobar\n )\ndiff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect\nindex b7250ca753..029d129299 100644\n--- a/t/chainlint/subshell-here-doc.expect\n+++ b/t/chainlint/subshell-here-doc.expect\n@@ -1,10 +1,10 @@\n (\n-\techo wobba \t gorgo snoot \t wafta snurb &&\n-\tcat >bip ?!AMP?!\n-\techo >bop\n+\techo wobba \t gorgo snoot \t wafta snurb <<-EOF &&\n+\tcat <bip ?!AMP?!\n+\techo <<-EOF >bop\n ) &&\n (\n-\tcat >bup &&\n-\tcat >bup3 &&\n+\tcat <<-ARBITRARY >bup &&\n+\tcat <<-ARBITRARY3 >bup3 &&\n \tmeep\n )\ndiff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect\nindex 215aca01c2..1cccc7bf7e 100644\n--- a/t/chainlint/t7900-subtree.expect\n+++ b/t/chainlint/t7900-subtree.expect\n@@ -1,9 +1,9 @@\n (\n \tchks=\"sub1sub2sub3sub4\" &&\n-\tchks_sub=$(cat | sed \"s,^,sub dir/,\"\n+\tchks_sub=$(cat <\n---\n t/chainlint.sed | 21 +++++++++++++++++++--\n t/chainlint/block-comment.expect | 6 ++++++\n t/chainlint/block-comment.test | 8 ++++++++\n t/chainlint/case-comment.expect | 8 ++++++++\n t/chainlint/case-comment.test | 11 +++++++++++\n t/chainlint/nested-subshell-comment.expect | 2 --\n 6 files changed, 52 insertions(+), 4 deletions(-)\n create mode 100644 t/chainlint/block-comment.expect\n create mode 100644 t/chainlint/block-comment.test\n create mode 100644 t/chainlint/case-comment.expect\n create mode 100644 t/chainlint/case-comment.test\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex 0e575c0c62..b1505ef2cd 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -294,6 +294,12 @@ bfolded\n x\n s/?!HERE?!/<\n---\n t/Makefile | 4 +-\n t/chainlint.sed | 38 ++++++++++++-------\n .../close-nested-and-parent-together.expect | 3 +-\n t/chainlint/complex-if-in-cuddled-loop.expect | 3 +-\n t/chainlint/cuddled-if-then-else.expect | 3 +-\n t/chainlint/cuddled-loop.expect | 3 +-\n t/chainlint/cuddled.expect | 12 ++----\n t/chainlint/inline-comment.expect | 3 +-\n t/chainlint/semicolon.expect | 3 +-\n 9 files changed, 37 insertions(+), 35 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex f4ae40be46..46cd5fc527 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -72,8 +72,8 @@ clean-chainlint:\n check-chainlint:\n \t@mkdir -p '$(CHAINLINTTMP_SQ)' && \\\n \tsed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n-\tcat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n-\t$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \\\n+\tsed -e '/^[ \t]*$$/d' $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n+\t$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests | grep -v '^[\t]*$$' >'$(CHAINLINTTMP_SQ)'/actual && \\\n \tdiff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n \n test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \\\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\nindex b1505ef2cd..dc4ce37cb5 100644\n--- a/t/chainlint.sed\n+++ b/t/chainlint.sed\n@@ -131,11 +131,15 @@ b\n \th\n \tbnextln\n }\n-# \"(...\" line -- split off and stash \"(\", then process \"...\" as its own line\n+# \"(...\" line -- \"(\" opening subshell cuddled with command; temporarily replace\n+# \"(\" with sentinel \"^\" and process the line as if \"(\" had been seen solo on\n+# the preceding line; this temporary replacement prevents several rules from\n+# accidentally thinking \"(\" introduces a nested subshell; \"^\" is changed back\n+# to \"(\" at output time\n x\n-s/.*/(/\n+s/.*//\n x\n-s/(//\n+s/(/^/\n bslurp\n \n :nextln\n@@ -168,12 +172,12 @@ s/.*\\n//\n \t/\"[^\"]*#[^\"]*\"/!s/[ \t]#.*$//\n }\n # one-liner \"case ... esac\"\n-/^[ \t]*case[ \t]*..*esac/bchkchn\n+/^[ \t^]*case[ \t]*..*esac/bchkchn\n # multi-line \"case ... esac\"\n-/^[ \t]*case[ \t]..*[ \t]in/bcase\n+/^[ \t^]*case[ \t]..*[ \t]in/bcase\n # multi-line \"for ... done\" or \"while ... done\"\n-/^[ \t]*for[ \t]..*[ \t]in/bcont\n-/^[ \t]*while[ \t]/bcont\n+/^[ \t^]*for[ \t]..*[ \t]in/bcont\n+/^[ \t^]*while[ \t]/bcont\n /^[ \t]*do[ \t]/bcont\n /^[ \t]*do[ \t]*$/bcont\n /;[ \t]*do/bcont\n@@ -184,7 +188,7 @@ s/.*\\n//\n /||[ \t]*exit[ \t]/bcont\n /||[ \t]*exit[ \t]*$/bcont\n # multi-line \"if...elsif...else...fi\"\n-/^[ \t]*if[ \t]/bcont\n+/^[ \t^]*if[ \t]/bcont\n /^[ \t]*then[ \t]/bcont\n /^[ \t]*then[ \t]*$/bcont\n /;[ \t]*then/bcont\n@@ -197,15 +201,15 @@ s/.*\\n//\n /^[ \t]*fi[ \t]*[<>|]/bdone\n /^[ \t]*fi[ \t]*)/bdone\n # nested one-liner \"(...) &&\"\n-/^[ \t]*(.*)[ \t]*&&[ \t]*$/bchkchn\n+/^[ \t^]*(.*)[ \t]*&&[ \t]*$/bchkchn\n # nested one-liner \"(...)\"\n-/^[ \t]*(.*)[ \t]*$/bchkchn\n+/^[ \t^]*(.*)[ \t]*$/bchkchn\n # nested one-liner \"(...) >x\" (or \"2>x\" or \"|]/bchkchn\n+/^[ \t^]*(.*)[ \t]*[0-9]*[<>|]/bchkchn\n # nested multi-line \"(...\\n...)\"\n-/^[ \t]*(/bnest\n+/^[ \t^]*(/bnest\n # multi-line \"{...\\n...}\"\n-/^[ \t]*{/bblock\n+/^[ \t^]*{/bblock\n # closing \")\" on own line -- exit subshell\n /^[ \t]*)/bclssolo\n # \"$((...))\" -- arithmetic expansion; not closing \")\"\n@@ -237,6 +241,7 @@ s/.*\\n//\n :cont\n # retrieve and print previous line\n x\n+s/^\\([ \t]*\\)^/\\1(/\n s/?!HERE?!/<Rather than running `chainlint` and `diff` once per self-test -- which\n>may become expensive as more tests are added -- instead run `chainlint`\n>a single time over all tests bodies collectively and compare the result\n>to the collective \"expected\" output.\n>\n>Signed-off-by: Eric Sunshine \n>---\n> t/Makefile | 10 ++++------\n> 1 file changed, 4 insertions(+), 6 deletions(-)\n>\n>diff --git a/t/Makefile b/t/Makefile\n>index 882d26eee3..f4ae40be46 100644\n>--- a/t/Makefile\n>+++ b/t/Makefile\n>@@ -71,12 +71,10 @@ clean-chainlint:\n>\n> check-chainlint:\n> \t@mkdir -p '$(CHAINLINTTMP_SQ)' && \\\n>-\terr=0 && \\\n>-\tfor i in $(CHAINLINTTESTS); do \\\n>-\t\t$(CHAINLINT) -\t\tsed -e '/^# LINT: /d' >'$(CHAINLINTTMP_SQ)'/$$i.actual && \\\n>-\t\tdiff -u chainlint/$$i.expect '$(CHAINLINTTMP_SQ)'/$$i.actual || err=1; \\\n>-\tdone && exit $$err\n>+\tsed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n>+\tcat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n>+\t$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \\\n>+\tdiff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n>\n\nIf I read this right you are relying on the order of the .test & .expect \nfiles to match. I did something similar in a test prereq which resulted in a \nbug when setting the test_output_dir to something residing in /dev/shm, \nsince the order of files in /dev/shm is reversed (at least on some \nplatforms). Even though this should work as is I could see this leading to a \nsimilar bug in the future.\n"},{"id":"443973","messageId":"RFC-patch-1.1-bb3f1577829-20211213T095456Z-avarab@gmail.com","threadId":"57072","inReplyTo":"20211213063059.19424-6-sunshine@sunshineco.com","subject":"[RFC PATCH] t/Makefile: use dependency graph for \"check-chainlint\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-13T10:09:42Z","receivedAt":"2021-12-13T10:36:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This gets rid of this fixed cost when running \"make T=\":\n\n$ git hyperfine -L rev HEAD~1,HEAD~0 -s 'make -C t check-chainlint' 'make -C t check-chainlint'\nBenchmark 1: make -C t check-chainlint' in 'HEAD~1\n Time (mean ± σ): 111.3 ms ± 0.5 ms [User: 116.9 ms, System: 26.2 ms]\n Range (min … max): 110.5 ms … 112.5 ms 26 runs\n\nBenchmark 2: make -C t check-chainlint' in 'HEAD~0\n Time (mean ± σ): 12.5 ms ± 0.2 ms [User: 11.5 ms, System: 1.0 ms]\n Range (min … max): 12.1 ms … 13.2 ms 223 runs\n\nSummary\n 'make -C t check-chainlint' in 'HEAD~0' ran\n 8.92 ± 0.16 times faster than 'make -C t check-chainlint' in 'HEAD~1'\n---\nOn Mon, Dec 13 2021, Eric Sunshine wrote:\n\n> Rather than running `chainlint` and `diff` once per self-test -- which\n> may become expensive as more tests are added -- instead run `chainlint`\n> a single time over all tests bodies collectively and compare the result\n> to the collective \"expected\" output.\n\nI think that \"optimizing\" things like this is an anti-pattern. I.e. we\nhave N chainlint test files, and N potential outputs from that (ok or\nnot, and with/without error). If one of the chainlint tests changes\nwe'd like to re-run it, if not we can re-use an earlier run.\n\nThis is something make's dependency logic is perfectly suited for, and\nwill be faster than any optimization of turning a for-loop into a\n\"sed\" command we run every time, since we'll only need to \"stat\" a few\nthings to see that there's nothing to do.\n\nI've had the below as part of my local build for a while. It\nconflicts/would be improved by my in-flight ab/make-dependency, which\nyou also ran into conflicts with.\n\nThis also has the advantage that you'll see what specific test failed\nfrom the Makefile output. Aside from the \".build\" rule needing to be\nfixed up to use the \"mkdir\" trick in ab/make-dependency it should also\nuse $(QUIET_GEN) or whatever.\n\nI'm not quite happy with the below, the wildcard/patsubst is a bit\nmessy, and it doesn't properly work around \"make test\" and how it runs\n\"clean\" after its run, but you'll find that \"make chainlint\" will\nproperly cache things with this.\n\n t/.gitignore | 1 +\n t/Makefile | 29 +++++++++++++++--------------\n 2 files changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/t/.gitignore b/t/.gitignore\nindex 91cf5772fe5..f142d6d42fd 100644\n--- a/t/.gitignore\n+++ b/t/.gitignore\n@@ -1,3 +1,4 @@\n+/.build/\n /trash directory*\n /test-results\n /.prove\ndiff --git a/t/Makefile b/t/Makefile\nindex 882d26eee30..9bac0e683c9 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -18,10 +18,8 @@ TEST_LINT ?= test-lint\n \n ifdef TEST_OUTPUT_DIRECTORY\n TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results\n-CHAINLINTTMP = $(TEST_OUTPUT_DIRECTORY)/chainlinttmp\n else\n TEST_RESULTS_DIRECTORY = test-results\n-CHAINLINTTMP = chainlinttmp\n endif\n \n # Shell quote;\n@@ -29,13 +27,12 @@ SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n TEST_SHELL_PATH_SQ = $(subst ','\\'',$(TEST_SHELL_PATH))\n PERL_PATH_SQ = $(subst ','\\'',$(PERL_PATH))\n TEST_RESULTS_DIRECTORY_SQ = $(subst ','\\'',$(TEST_RESULTS_DIRECTORY))\n-CHAINLINTTMP_SQ = $(subst ','\\'',$(CHAINLINTTMP))\n \n T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))\n TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n-CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n+CHAINLINTTESTS = $(wildcard chainlint/*.test)\n CHAINLINT = sed -f chainlint.sed\n \n all: $(DEFAULT_TEST_TARGET)\n@@ -67,16 +64,20 @@ clean: clean-except-prove-cache\n \t$(RM) .prove\n \n clean-chainlint:\n-\t$(RM) -r '$(CHAINLINTTMP_SQ)'\n-\n-check-chainlint:\n-\t@mkdir -p '$(CHAINLINTTMP_SQ)' && \\\n-\terr=0 && \\\n-\tfor i in $(CHAINLINTTESTS); do \\\n-\t\t$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/$$i.actual && \\\n-\t\tdiff -u chainlint/$$i.expect '$(CHAINLINTTMP_SQ)'/$$i.actual || err=1; \\\n-\tdone && exit $$err\n+\t$(RM) -r .build/chainlint\n+\n+BUILT_CHAINLINTTESTS = $(patsubst %,.build/%.actual,$(CHAINLINTTESTS))\n+\n+.build/chainlint:\n+\tmkdir -p $@\n+\n+$(BUILT_CHAINLINTTESTS): | .build/chainlint\n+$(BUILT_CHAINLINTTESTS): .build/%.actual: %\n+\t$(CHAINLINT) <$< | \\\n+\tsed -e '/^# LINT: /d' >$@ && \\\n+\tdiff -u $(basename $<).expect $@\n+\n+check-chainlint: $(BUILT_CHAINLINTTESTS)\n \n test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \\\n \ttest-lint-filenames\n-- \n2.34.1.1025.g9a0c3a30920\n\n"},{"id":"443981","messageId":"CAPig+cSKn6wdPKc=b8Xjqy5D=bVdu6FQtYKJuwN2VoV7pEEgHw@mail.gmail.com","threadId":"57072","inReplyTo":"20211213102224.y5psbojmivlxe5px@fs","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T14:27:48Z","receivedAt":"2021-12-13T14:28:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 13, 2021 at 5:22 AM Fabian Stelzer wrote:\n> On 13.12.2021 01:30, Eric Sunshine wrote:\n> > check-chainlint:\n> >+ sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n> >+ cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n> >+ $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \\\n> >+ diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n>\n> If I read this right you are relying on the order of the .test & .expect\n> files to match. I did something similar in a test prereq which resulted in a\n> bug when setting the test_output_dir to something residing in /dev/shm,\n> since the order of files in /dev/shm is reversed (at least on some\n> platforms). Even though this should work as is I could see this leading to a\n> similar bug in the future.\n\nIt's not seen in the patch context, but earlier in the file we have:\n\n CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))\n\nwhich provides stability via `sort`, thus ensures that the order of\nthe \".test\" and \".expect\" match.\n\nI think that addresses your concern (unless I misunderstand your observation).\n"},{"id":"443986","messageId":"20211213154327.pmhopjbdlkz7dgjh@fs","threadId":"57072","inReplyTo":"CAPig+cSKn6wdPKc=b8Xjqy5D=bVdu6FQtYKJuwN2VoV7pEEgHw@mail.gmail.com","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-13T15:43:27Z","receivedAt":"2021-12-13T15:43:32Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 13.12.2021 09:27, Eric Sunshine wrote:\n>On Mon, Dec 13, 2021 at 5:22 AM Fabian Stelzer wrote:\n>> On 13.12.2021 01:30, Eric Sunshine wrote:\n>> > check-chainlint:\n>> >+ sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n>> >+ cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n>> >+ $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \\\n>> >+ diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n>>\n>> If I read this right you are relying on the order of the .test & .expect\n>> files to match. I did something similar in a test prereq which resulted in a\n>> bug when setting the test_output_dir to something residing in /dev/shm,\n>> since the order of files in /dev/shm is reversed (at least on some\n>> platforms). Even though this should work as is I could see this leading to a\n>> similar bug in the future.\n>\n>It's not seen in the patch context, but earlier in the file we have:\n>\n> CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))\n>\n>which provides stability via `sort`, thus ensures that the order of\n>the \".test\" and \".expect\" match.\n>\n>I think that addresses your concern (unless I misunderstand your observation).\n\nYes, thats what i meant. I didn't realize $CHAINLINTTESTS is already the \nsorted glob. Thanks for clarifying.\n\nPersonally i find the initial for loop variant to be the most readable. \nÆvars makefile targets could be very nice too, but especially:\n\n+$(BUILT_CHAINLINTTESTS): | .build/chainlint\n+$(BUILT_CHAINLINTTESTS): .build/%.actual: %\n+ $(CHAINLINT) <$< | \\\n+\t sed -e '/^# LINT: /d' >$@ && \\\n+ diff -u $(basename $<).expect $@\n\ni find very hard to grasp :/\nI have no idea what is going on here: `<$< |` ?\n"},{"id":"443999","messageId":"CAPig+cSXHBMgOUycL0cXuVCb_PJ2=x2w4wUkc7eQQueyk=0Uzw@mail.gmail.com","threadId":"57072","inReplyTo":"20211213154327.pmhopjbdlkz7dgjh@fs","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T16:02:36Z","receivedAt":"2021-12-13T16:02:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer wrote:\n> Personally i find the initial for loop variant to be the most readable.\n> Ævars makefile targets could be very nice too, but especially:\n>\n> +$(BUILT_CHAINLINTTESTS): | .build/chainlint\n> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %\n> + $(CHAINLINT) <$< | \\\n> + sed -e '/^# LINT: /d' >$@ && \\\n> + diff -u $(basename $<).expect $@\n>\n> i find very hard to grasp :/\n> I have no idea what is going on here: `<$< |` ?\n\nYa, that line-noise is an unfortunate combination of shell and\nMakefile gobbledygook. The `$<` is effectively the source file (the\nfile being passed into chainlint.sed), and the rest of it is just\nnormal shell. `<` is redirection (using the source file `$<` as\nstdin), and `|` is the pipe operator (sending the output of\nchainlint.sed to another `sed`), and capturing it all via shell `>`\nredirection in `$@` which is the Makefile variable for the target\nfile.\n\nAnyhow, although the commit message tries to sell this change as some\nsort of optimization, it's really in preparation for the new chainlint\nwhich wants to check all tests in all files with a single invocation\nrather than being invoked over and over and over. The self-test files\nalso require more preprocessing to work with the new chainlint, so the\nimplementation of `check-chainlint` gets rather more complex once the\nend state is reached. I'll think about it a bit, but at the moment,\nI'm still leaning toward this intermediate step as being beneficial to\nreaching the end state. However, my opinion could change since the way\nthis is done here was probably influenced by an earlier iteration of\nthe new chainlint, but now that the implementation of the new\nchainlint is concrete, it may not be especially important to do it\nthis way.\n"},{"id":"444000","messageId":"20211213161419.cwamlunrgz3adrhh@fs","threadId":"57072","inReplyTo":"CAPig+cSXHBMgOUycL0cXuVCb_PJ2=x2w4wUkc7eQQueyk=0Uzw@mail.gmail.com","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-13T16:14:19Z","receivedAt":"2021-12-13T16:14:24Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 13.12.2021 11:02, Eric Sunshine wrote:\n>On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer wrote:\n>> Personally i find the initial for loop variant to be the most readable.\n>> Ævars makefile targets could be very nice too, but especially:\n>>\n>> +$(BUILT_CHAINLINTTESTS): | .build/chainlint\n>> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %\n>> + $(CHAINLINT) <$< | \\\n>> + sed -e '/^# LINT: /d' >$@ && \\\n>> + diff -u $(basename $<).expect $@\n>>\n>> i find very hard to grasp :/\n>> I have no idea what is going on here: `<$< |` ?\n>\n>Ya, that line-noise is an unfortunate combination of shell and\n>Makefile gobbledygook. The `$<` is effectively the source file (the\n>file being passed into chainlint.sed), and the rest of it is just\n>normal shell. `<` is redirection (using the source file `$<` as\n>stdin), and `|` is the pipe operator (sending the output of\n>chainlint.sed to another `sed`), and capturing it all via shell `>`\n>redirection in `$@` which is the Makefile variable for the target\n>file.\n>\n\nThanks, that explains it nicely. I'm not familiar enough with makefile \nsyntax.\n\n>Anyhow, although the commit message tries to sell this change as some\n>sort of optimization, it's really in preparation for the new chainlint\n>which wants to check all tests in all files with a single invocation\n>rather than being invoked over and over and over. The self-test files\n>also require more preprocessing to work with the new chainlint, so the\n>implementation of `check-chainlint` gets rather more complex once the\n>end state is reached. I'll think about it a bit, but at the moment,\n>I'm still leaning toward this intermediate step as being beneficial to\n>reaching the end state. However, my opinion could change since the way\n>this is done here was probably influenced by an earlier iteration of\n>the new chainlint, but now that the implementation of the new\n>chainlint is concrete, it may not be especially important to do it\n>this way.\n\nI don't mind much either way and i tend to favor the efficient variant as \nwell. On the other hand i can also see some bug mixing up the contents of \nthese files producing a huge diff with no good indication for the developer \nof what has happened.\n"},{"id":"444001","messageId":"211213.86tufc8oop.gmgdl@evledraar.gmail.com","threadId":"57072","inReplyTo":"CAPig+cSXHBMgOUycL0cXuVCb_PJ2=x2w4wUkc7eQQueyk=0Uzw@mail.gmail.com","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-13T16:11:18Z","receivedAt":"2021-12-13T16:17:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 13 2021, Eric Sunshine wrote:\n\n> On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer wrote:\n>> Personally i find the initial for loop variant to be the most readable.\n>> Ævars makefile targets could be very nice too, but especially:\n>>\n>> +$(BUILT_CHAINLINTTESTS): | .build/chainlint\n>> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %\n>> + $(CHAINLINT) <$< | \\\n>> + sed -e '/^# LINT: /d' >$@ && \\\n>> + diff -u $(basename $<).expect $@\n>>\n>> i find very hard to grasp :/\n>> I have no idea what is going on here: `<$< |` ?\n>\n> Ya, that line-noise is an unfortunate combination of shell and\n> Makefile gobbledygook. The `$<` is effectively the source file (the\n> file being passed into chainlint.sed), and the rest of it is just\n> normal shell. `<` is redirection (using the source file `$<` as\n> stdin), and `|` is the pipe operator (sending the output of\n> chainlint.sed to another `sed`), and capturing it all via shell `>`\n> redirection in `$@` which is the Makefile variable for the target\n> file.\n\nTo add to that;\nhttps://www.gnu.org/software/make/manual/html_node/Rules.html#Rules and\nother relevant parts of the GNU make manual are very helpful here.\n\n> Anyhow, although the commit message tries to sell this change as some\n> sort of optimization, it's really in preparation for the new chainlint\n> which wants to check all tests in all files with a single invocation\n> rather than being invoked over and over and over. The self-test files\n> also require more preprocessing to work with the new chainlint, so the\n> implementation of `check-chainlint` gets rather more complex once the\n> end state is reached. I'll think about it a bit, but at the moment,\n> I'm still leaning toward this intermediate step as being beneficial to\n> reaching the end state. However, my opinion could change since the way\n> this is done here was probably influenced by an earlier iteration of\n> the new chainlint, but now that the implementation of the new\n> chainlint is concrete, it may not be especially important to do it\n> this way.\n\nI don't really care about the details of whether it's invoked once or N\ntimes, although I think the N times with proper dependencies tends to\ngive you better error messages, but maybe you'll be changing it\nsignificantly enough that the current map between chainlint files and\napproximately what sort of thing they check won't be there anymore.\n\nIn any case, I'd think that a rule that used $< now (i.e. 1=1 file->out\nprereq) would be better for the current state, and could just be changed\nto use one of $^ or $+ later.\n\nI.e. you can declare a \"check.done\" that depends on {1..10}.todo, and\nget a list of all of those {1..10}.todo files if one changes, or just\nthe ones whose mtime is newer than a \"check.done\".\n\nThe reason I looked at this to begin with is that it takes it ~100-150ms\nto run now, which adds up if you're e.g. using \"make test T=\" in\n\"git rebase -i --exec\".\n"},{"id":"444011","messageId":"CAPig+cR5Q2q=wdXuVBeP52=pAfvh6_4z__g-0JUGWq_7uor_Hg@mail.gmail.com","threadId":"57072","inReplyTo":"211213.86tufc8oop.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T17:05:34Z","receivedAt":"2021-12-13T17:05:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 13, 2021 at 11:17 AM Ævar Arnfjörð Bjarmason\n wrote:\n> On Mon, Dec 13 2021, Eric Sunshine wrote:\n> > On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer wrote:\n> >> I have no idea what is going on here: `<$< |` ?\n> >\n> > Ya, that line-noise is an unfortunate combination of shell and\n> > Makefile gobbledygook. The `$<` is effectively the source file (the\n> > file being passed into chainlint.sed), and the rest of it is just\n> > normal shell. `<` is redirection (using the source file `$<` as\n> > stdin), and `|` is the pipe operator (sending the output of\n> > chainlint.sed to another `sed`), and capturing it all via shell `>`\n> > redirection in `$@` which is the Makefile variable for the target\n> > file.\n>\n> To add to that;\n> https://www.gnu.org/software/make/manual/html_node/Rules.html#Rules and\n> other relevant parts of the GNU make manual are very helpful here.\n\nAnd the Makefile variables $< and $@, in particular, are documented here:\nhttps://www.gnu.org/software/make/manual/html_node/Automatic-Variables.html\n\n> I don't really care about the details of whether it's invoked once or N\n> times, although I think the N times with proper dependencies tends to\n> give you better error messages, but maybe you'll be changing it\n> significantly enough that the current map between chainlint files and\n> approximately what sort of thing they check won't be there anymore.\n>\n> In any case, I'd think that a rule that used $< now (i.e. 1=1 file->out\n> prereq) would be better for the current state, and could just be changed\n> to use one of $^ or $+ later.\n>\n> I.e. you can declare a \"check.done\" that depends on {1..10}.todo, and\n> get a list of all of those {1..10}.todo files if one changes, or just\n> the ones whose mtime is newer than a \"check.done\".\n>\n> The reason I looked at this to begin with is that it takes it ~100-150ms\n> to run now, which adds up if you're e.g. using \"make test T=\" in\n> \"git rebase -i --exec\".\n\nRegarding this last point, one idea I strongly considered (and have\nnot rejected) is to stop making `check-chainlin` a dependency of\n`test` and `prove`. Unlike most of the test suite, in which a change\nto any part of the Git source code could potentially cause any test to\nfail -- thus, it is important to run the full test suite for any\nsource code change -- the `check-chainlint` target is completely\nisolated from everything else; it only checks whether `chainlint`\nitself functions correctly. The only time it really makes sense to run\n`check-chainlint` is when chainlint itself is changed in order to\nverify that it still functions as expected. Considering how\ninfrequently (i.e. never) chainlint is modified, it seems somewhat\nsilly for every `make test` or `make prove` invoked by anybody\nanywhere to repeatedly and forever validate chainlint[*]. Instead, it\ncould be the responsibility of the person modifying chainlint to run\nthe `check-chainlint` self-tests.\n\n[*]: There is at least one exception. Various implementations of `sed`\ncould behave differently, thus impacting the behavior of\nchainlint.sed. This is not just a theoretical concern. I did all the\ndevelopment of this series on macOS, where everything worked as\nintended. Shortly before sending the series to the list, I subjected\nit to other platforms via CI and found that it failed on Linux due to\nminor behavioral differences in `sed` on Linux (though, very oddly, it\nworked just fine on Windows). I might not have caught this problem if\n`check-chainlint` had not been run automatically by `make test`.\n"},{"id":"444013","messageId":"CAPig+cRkC=-dV_i1LJg3kZVVpR778YNyhpO6maHnH5zFH9zS6w@mail.gmail.com","threadId":"57072","inReplyTo":"CAPig+cR5Q2q=wdXuVBeP52=pAfvh6_4z__g-0JUGWq_7uor_Hg@mail.gmail.com","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T17:25:30Z","receivedAt":"2021-12-13T17:25:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 13, 2021 at 12:05 PM Eric Sunshine wrote:\n> On Mon, Dec 13, 2021 at 11:17 AM Ævar Arnfjörð Bjarmason\n> wrote:\n> > The reason I looked at this to begin with is that it takes it ~100-150ms\n> > to run now, which adds up if you're e.g. using \"make test T=\" in\n> > \"git rebase -i --exec\".\n>\n> Regarding this last point, one idea I strongly considered (and have\n> not rejected) is to stop making `check-chainlin` a dependency of\n> `test` and `prove`. [...]\n\nAnother less sledge-hammer approach would be to make t/Makefile\nrespect GIT_TEST_CHAIN_LINT so that it doesn't run `check-chainlint`\nfor `test` and `prove` when that variable is `0`. That would allow\nyour `git rebase -i --exec` case to avoid the wasted extra overhead of\n`check-chainlint` (and chainlint in general).\n"},{"id":"444022","messageId":"211213.86zgp470wa.gmgdl@evledraar.gmail.com","threadId":"57072","inReplyTo":"CAPig+cRkC=-dV_i1LJg3kZVVpR778YNyhpO6maHnH5zFH9zS6w@mail.gmail.com","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-13T19:33:36Z","receivedAt":"2021-12-13T19:36:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 13 2021, Eric Sunshine wrote:\n\n> On Mon, Dec 13, 2021 at 12:05 PM Eric Sunshine wrote:\n>> On Mon, Dec 13, 2021 at 11:17 AM Ævar Arnfjörð Bjarmason\n>> wrote:\n>> > The reason I looked at this to begin with is that it takes it ~100-150ms\n>> > to run now, which adds up if you're e.g. using \"make test T=\" in\n>> > \"git rebase -i --exec\".\n>>\n>> Regarding this last point, one idea I strongly considered (and have\n>> not rejected) is to stop making `check-chainlin` a dependency of\n>> `test` and `prove`. [...]\n>\n> Another less sledge-hammer approach would be to make t/Makefile\n> respect GIT_TEST_CHAIN_LINT so that it doesn't run `check-chainlint`\n> for `test` and `prove` when that variable is `0`. That would allow\n> your `git rebase -i --exec` case to avoid the wasted extra overhead of\n> `check-chainlint` (and chainlint in general).\n\nYes, but I just don't see why it's needed.\n\nWe need to build e.g. t/helpers/ to run the tests, and doing that is\nprobably always going to take eons of compilation times compared to\nthese assertions.\n\nBut it's a one-off eon because we declare the dependency DAG and don't\nrecompile them unless the sources or their dependencies change. Likewise\nfor these chainlint tests we can (and maybe should) make them optional,\nbut as long as we're not needlessly running them when no changes have\nhappened...\n\n"},{"id":"444031","messageId":"CAPig+cRrX396RRGGhjMDB6ybNa0WxcAOY-BfW4FTE84R+ucLkQ@mail.gmail.com","threadId":"57072","inReplyTo":"211213.86zgp470wa.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-13T21:37:55Z","receivedAt":"2021-12-13T21:38:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 13, 2021 at 2:36 PM Ævar Arnfjörð Bjarmason\n wrote:\n> On Mon, Dec 13 2021, Eric Sunshine wrote:\n> > Another less sledge-hammer approach would be to make t/Makefile\n> > respect GIT_TEST_CHAIN_LINT so that it doesn't run `check-chainlint`\n> > for `test` and `prove` when that variable is `0`. That would allow\n> > your `git rebase -i --exec` case to avoid the wasted extra overhead of\n> > `check-chainlint` (and chainlint in general).\n>\n> Yes, but I just don't see why it's needed.\n>\n> We need to build e.g. t/helpers/ to run the tests, and doing that is\n> probably always going to take eons of compilation times compared to\n> these assertions.\n>\n> But it's a one-off eon because we declare the dependency DAG and don't\n> recompile them unless the sources or their dependencies change. Likewise\n> for these chainlint tests we can (and maybe should) make them optional,\n> but as long as we're not needlessly running them when no changes have\n> happened...\n\nThat's a good point, and the same observation applies to\nt/check-non-portable-shell.pl which is run unconditionally, as well,\nwhenever `test` and `prove` are run.\n"},{"id":"444083","messageId":"CAPig+cSFtpt6ExbVDbcx3tZodrKFuM-r2GMW4TQ2tJmLvHBFtQ@mail.gmail.com","threadId":"57072","inReplyTo":"RFC-patch-1.1-bb3f1577829-20211213T095456Z-avarab@gmail.com","subject":"Re: [RFC PATCH] t/Makefile: use dependency graph for \"check-chainlint\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-14T07:44:25Z","receivedAt":"2021-12-14T07:44:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 13, 2021 at 5:09 AM Ævar Arnfjörð Bjarmason\n wrote:\n> On Mon, Dec 13 2021, Eric Sunshine wrote:\n> > Rather than running `chainlint` and `diff` once per self-test -- which\n> > may become expensive as more tests are added -- instead run `chainlint`\n> > a single time over all tests bodies collectively and compare the result\n> > to the collective \"expected\" output.\n>\n> I think that \"optimizing\" things like this is an anti-pattern. I.e. we\n> have N chainlint test files, and N potential outputs from that (ok or\n> not, and with/without error). If one of the chainlint tests changes\n> we'd like to re-run it, if not we can re-use an earlier run.\n\nAs mentioned in a reply elsewhere, the commit message sells this as an\noptimization, but that's not the real reason for the change, which is\nthat the rewritten `check-chainlint` target for the upcoming new\nchainlint really wants to have a composite file of the \"test\" input\nand a composite of the \"expect\" output. I didn't know how to sell that\nchange in this preparatory patch series, so I weakly framed it as an\noptimization. The reason for making this a preparatory step is that it\nmakes for a less noisy patch later on when the new chainlint is\nactually plugged into the `check-chainlint` target. At least, it was\nless noisy originally... in the final implementation, I think it ends\nup being noisy anyhow. So, maybe it makes sense to drop this patch\naltogether(?).\n\n> This is something make's dependency logic is perfectly suited for, and\n> will be faster than any optimization of turning a for-loop into a\n> \"sed\" command we run every time, since we'll only need to \"stat\" a few\n> things to see that there's nothing to do.\n>\n> +BUILT_CHAINLINTTESTS = $(patsubst %,.build/%.actual,$(CHAINLINTTESTS))\n> +\n> +.build/chainlint:\n> + mkdir -p $@\n> +\n> +$(BUILT_CHAINLINTTESTS): | .build/chainlint\n> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %\n> + $(CHAINLINT) <$< | \\\n> + sed -e '/^# LINT: /d' >$@ && \\\n> + diff -u $(basename $<).expect $@\n> +\n> +check-chainlint: $(BUILT_CHAINLINTTESTS)\n\nThis sort of optimization makes sense (I think) even with the new\nchainlint preferring to see composite \"test\" and \"expect\" files rather\nthan the individual files. The individual files would be prerequisites\nof the composite files, thus the composites would only be regenerated\nif the individual files change. And the composite files would be\nprerequisites of the `check-chainlint` target, so it would only run if\nthe composite files change (or if chainlint itself changes).\n\nIn fact, with the new chainlint checking all tests in all scripts at\nonce, this technique should apply nicely to it, as well, since the\nnames of test scripts (t????-*.sh) are fed to it as command-line\narguments. Thus, the t????-*.sh files could be prerequisites of the\nchainlint rule which would use $? to check only test scripts which\nhave changed since the previous run.\n\nHaving said all that, I don't think think the changes in this series\nor the upcoming new chainlint series make the situation any worse\n(Makefile-wise) than its current state, and the sort of optimizations\ndiscussed here can easily be made after those series land. (And, as my\nGit time is rather limited these days, I'd really like to focus on\ngetting the core components landed.)\n"},{"id":"444099","messageId":"211214.86tufbbbu3.gmgdl@evledraar.gmail.com","threadId":"57072","inReplyTo":"CAPig+cSFtpt6ExbVDbcx3tZodrKFuM-r2GMW4TQ2tJmLvHBFtQ@mail.gmail.com","subject":"Re: [RFC PATCH] t/Makefile: use dependency graph for \"check-chainlint\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-14T12:34:53Z","receivedAt":"2021-12-14T12:38:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 14 2021, Eric Sunshine wrote:\n\n> On Mon, Dec 13, 2021 at 5:09 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> On Mon, Dec 13 2021, Eric Sunshine wrote:\n>> > Rather than running `chainlint` and `diff` once per self-test -- which\n>> > may become expensive as more tests are added -- instead run `chainlint`\n>> > a single time over all tests bodies collectively and compare the result\n>> > to the collective \"expected\" output.\n>>\n>> I think that \"optimizing\" things like this is an anti-pattern. I.e. we\n>> have N chainlint test files, and N potential outputs from that (ok or\n>> not, and with/without error). If one of the chainlint tests changes\n>> we'd like to re-run it, if not we can re-use an earlier run.\n>\n> As mentioned in a reply elsewhere, the commit message sells this as an\n> optimization, but that's not the real reason for the change, which is\n> that the rewritten `check-chainlint` target for the upcoming new\n> chainlint really wants to have a composite file of the \"test\" input\n> and a composite of the \"expect\" output. I didn't know how to sell that\n> change in this preparatory patch series, so I weakly framed it as an\n> optimization. The reason for making this a preparatory step is that it\n> makes for a less noisy patch later on when the new chainlint is\n> actually plugged into the `check-chainlint` target. At least, it was\n> less noisy originally... in the final implementation, I think it ends\n> up being noisy anyhow. So, maybe it makes sense to drop this patch\n> altogether(?).\n\nI think you should do whatever you think makes sense here, I was just\npointing out that alternate Makefile approach in case it was helpful.\n\n>> This is something make's dependency logic is perfectly suited for, and\n>> will be faster than any optimization of turning a for-loop into a\n>> \"sed\" command we run every time, since we'll only need to \"stat\" a few\n>> things to see that there's nothing to do.\n>>\n>> +BUILT_CHAINLINTTESTS = $(patsubst %,.build/%.actual,$(CHAINLINTTESTS))\n>> +\n>> +.build/chainlint:\n>> + mkdir -p $@\n>> +\n>> +$(BUILT_CHAINLINTTESTS): | .build/chainlint\n>> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %\n>> + $(CHAINLINT) <$< | \\\n>> + sed -e '/^# LINT: /d' >$@ && \\\n>> + diff -u $(basename $<).expect $@\n>> +\n>> +check-chainlint: $(BUILT_CHAINLINTTESTS)\n>\n> This sort of optimization makes sense (I think) even with the new\n> chainlint preferring to see composite \"test\" and \"expect\" files rather\n> than the individual files. The individual files would be prerequisites\n> of the composite files, thus the composites would only be regenerated\n> if the individual files change. And the composite files would be\n> prerequisites of the `check-chainlint` target, so it would only run if\n> the composite files change (or if chainlint itself changes).\n>\n> In fact, with the new chainlint checking all tests in all scripts at\n> once, this technique should apply nicely to it, as well, since the\n> names of test scripts (t????-*.sh) are fed to it as command-line\n> arguments. Thus, the t????-*.sh files could be prerequisites of the\n> chainlint rule which would use $? to check only test scripts which\n> have changed since the previous run.\n\n*nod*\n\n> Having said all that, I don't think think the changes in this series\n> or the upcoming new chainlint series make the situation any worse\n> (Makefile-wise) than its current state, and the sort of optimizations\n> discussed here can easily be made after those series land. (And, as my\n> Git time is rather limited these days, I'd really like to focus on\n> getting the core components landed.)\n\nSounds good to me. We have plenty of these \"doesn't have deps\" targets\n(e.g. the check shell syntax one you noted), we can just fix/change them\nsome other time.\n"},{"id":"444137","messageId":"CABPp-BFNEPkEaY-MT1Ot7EzHkW=7FHGbJxU=pr226M43BvHwTw@mail.gmail.com","threadId":"57072","inReplyTo":"20211213063059.19424-1-sunshine@sunshineco.com","subject":"Re: [PATCH 00/15] generalize chainlint self-tests","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-15T00:00:37Z","receivedAt":"2021-12-15T00:00:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Dec 12, 2021 at 10:31 PM Eric Sunshine wrote:\n>\n> A while back, Peff successfully nerd-sniped[1] me into tackling a\n> long-brewing idea I had about (possibly) improving `chainlint`\n> performance by linting all tests in all scripts with a single command\n> invocation instead of running `sed` 25300+ times (once for each test).\n> A description of some of the important features of the new linter can be\n> found at [2].\n>\n> Although the new chainlint implementation has been complete for several\n> months, I'm still working out how to organize its patch series. In the\n> meantime, _this_ patch series makes it possible for the new linter to\n> re-use the existing chainlint self-tests. It does so by cleansing the\n> self-test \".test\" and \".expect\" files of implementation details and\n> limitations specific to chainlint.sed.\n\nI read through the patches in this series, and didn't note any\nproblems. However, my knowledge of sed is just the basics. Even in a\nfew cases where regexes were all that were really involved, the\nregexes were lengthy enough that my eyes just glazed over. So, my\nreview is kind of superficial, but the preparatory patches certainly\nseem good to me, and the commit messages are well explained, and the\nnon-sed changes are consistent with the described changes, and the\neasier sed stuff looked good. It's clear you put a lot of care into\ncarefully explaining and dividing up the patches in a nice and logical\nmanner.\n"},{"id":"444139","messageId":"CAPig+cQrzA9Op=mAAWfuCyDmiFPFopX2KutOg4DHQm2bnPdvPQ@mail.gmail.com","threadId":"57072","inReplyTo":"CABPp-BFNEPkEaY-MT1Ot7EzHkW=7FHGbJxU=pr226M43BvHwTw@mail.gmail.com","subject":"Re: [PATCH 00/15] generalize chainlint self-tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-15T03:15:00Z","receivedAt":"2021-12-15T03:15:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 14, 2021 at 7:00 PM Elijah Newren wrote:\n> On Sun, Dec 12, 2021 at 10:31 PM Eric Sunshine wrote:\n> > Although the new chainlint implementation has been complete for several\n> > months, I'm still working out how to organize its patch series. In the\n> > meantime, _this_ patch series makes it possible for the new linter to\n> > re-use the existing chainlint self-tests. It does so by cleansing the\n> > self-test \".test\" and \".expect\" files of implementation details and\n> > limitations specific to chainlint.sed.\n>\n> I read through the patches in this series, and didn't note any\n> problems. However, my knowledge of sed is just the basics. Even in a\n> few cases where regexes were all that were really involved, the\n> regexes were lengthy enough that my eyes just glazed over. So, my\n> review is kind of superficial, but the preparatory patches certainly\n> seem good to me, and the commit messages are well explained, and the\n> non-sed changes are consistent with the described changes, and the\n> easier sed stuff looked good. It's clear you put a lot of care into\n> carefully explaining and dividing up the patches in a nice and logical\n> manner.\n\nThanks, Elijah, for reading through the series. I wasn't necessarily\nexpecting anyone to read the patches carefully, especially the\n`sed`-specific changes[*] since it's such an out-of-the-way part of\nthe project, but it's nice to know that the time I put into organizing\nthe series and writing the commit messages wasn't wasted.\n\n[*] With the comments stripped out, the entire chainlint.sed script\ndoes quite a good job of emulating gobs of line-noise burped up by an\nold dial-up modem, so it's no surprise your eyes glazed over.\n"},{"id":"444207","messageId":"211216.86zgp0adls.gmgdl@evledraar.gmail.com","threadId":"57072","inReplyTo":"20211213154327.pmhopjbdlkz7dgjh@fs","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-16T13:17:58Z","receivedAt":"2021-12-16T13:22:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 13 2021, Fabian Stelzer wrote:\n\n> On 13.12.2021 09:27, Eric Sunshine wrote:\n>>On Mon, Dec 13, 2021 at 5:22 AM Fabian Stelzer wrote:\n>>> On 13.12.2021 01:30, Eric Sunshine wrote:\n>>> > check-chainlint:\n>>> >+ sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n>>> >+ cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n>>> >+ $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \\\n>>> >+ diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n>>>\n>>> If I read this right you are relying on the order of the .test & .expect\n>>> files to match. I did something similar in a test prereq which resulted in a\n>>> bug when setting the test_output_dir to something residing in /dev/shm,\n>>> since the order of files in /dev/shm is reversed (at least on some\n>>> platforms). Even though this should work as is I could see this leading to a\n>>> similar bug in the future.\n>>\n>>It's not seen in the patch context, but earlier in the file we have:\n>>\n>> CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))\n>>\n>>which provides stability via `sort`, thus ensures that the order of\n>>the \".test\" and \".expect\" match.\n>>\n>>I think that addresses your concern (unless I misunderstand your observation).\n>\n> Yes, thats what i meant. I didn't realize $CHAINLINTTESTS is already\n> the sorted glob. Thanks for clarifying.\n>\n> Personally i find the initial for loop variant to be the most\n> readable. Ævars makefile targets could be very nice too, but\n> especially:\n>\n> +$(BUILT_CHAINLINTTESTS): | .build/chainlint\n> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %\n> + $(CHAINLINT) <$< | \\\n> +\t sed -e '/^# LINT: /d' >$@ && \\\n> + diff -u $(basename $<).expect $@\n>\n> i find very hard to grasp :/\n> I have no idea what is going on here: `<$< |` ?\n\nIt's a minor point, and not relevant to this series proceeding.\n\nBut just FWIW I think both of you are wrong about the potenital for a\n\".test\" and \".expect\" bug here.\n\nI.e. yes the CHAINLINTTESTS variable is sorted:\n\n CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))\n\nBut in Eric's patch we just have this relevant to this concern of\n(paraphrased) \"would it not be sorted break it?\":\n\n\t+\tsed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n\t+\tcat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n\nSo it doesn't matter if it's sorted our not.\n\nI.e. we've got {A,B,C}.{test,expect} files in a directory, and we're\nconstructing a \"A.test\" and \"A.expect\" via \"$(patsubst)\".\n\nSo if it's \"A B C\", \"C B A\", \"A C B\" etc. won't matter. We'll always get\n\".test\" files corresponding to \".expect\".\n\nIf it's not sorted we'll get failure output in an unexpected order, but\nit won't matter to whether we detect a failure or not.\n"},{"id":"444233","messageId":"CAPig+cT_38g51RtcMAf184jjB3Zr67gv=rO0oEY1DG7asnyUJg@mail.gmail.com","threadId":"57072","inReplyTo":"211216.86zgp0adls.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-12-16T15:47:46Z","receivedAt":"2021-12-16T15:48:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 16, 2021 at 8:22 AM Ævar Arnfjörð Bjarmason\n wrote:\n> > On 13.12.2021 09:27, Eric Sunshine wrote:\n> >>It's not seen in the patch context, but earlier in the file we have:\n> >>\n> >> CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))\n> >>\n> >>which provides stability via `sort`, thus ensures that the order of\n> >>the \".test\" and \".expect\" match.\n> >>\n> >>I think that addresses your concern (unless I misunderstand your observation).\n>\n> But just FWIW I think both of you are wrong about the potenital for a\n> \".test\" and \".expect\" bug here.\n>\n> I.e. yes the CHAINLINTTESTS variable is sorted:\n>\n> But in Eric's patch we just have this relevant to this concern of\n> (paraphrased) \"would it not be sorted break it?\":\n>\n> + sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n> + cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n>\n> So it doesn't matter if it's sorted our not.\n>\n> I.e. we've got {A,B,C}.{test,expect} files in a directory, and we're\n> constructing a \"A.test\" and \"A.expect\" via \"$(patsubst)\".\n>\n> So if it's \"A B C\", \"C B A\", \"A C B\" etc. won't matter. We'll always get\n> \".test\" files corresponding to \".expect\".\n\nYes, sorry, I meant to say something along these lines in my reply, in\naddition to mentioning `sort`, but forgot. Taking a look at this\nagain, though, makes me wonder if the CHAINLINTTESTS assignment should\nbe done with `:=` rather than `=` (unless GNU make is smart enough to\nonly invoke the `wildcard` operation only once, in which case it\nwouldn't particularly matter).\n"},{"id":"444256","messageId":"211216.86r1ac9uqc.gmgdl@evledraar.gmail.com","threadId":"57072","inReplyTo":"CAPig+cT_38g51RtcMAf184jjB3Zr67gv=rO0oEY1DG7asnyUJg@mail.gmail.com","subject":"Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-16T19:26:12Z","receivedAt":"2021-12-16T20:10:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 16 2021, Eric Sunshine wrote:\n\n> On Thu, Dec 16, 2021 at 8:22 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> > On 13.12.2021 09:27, Eric Sunshine wrote:\n>> >>It's not seen in the patch context, but earlier in the file we have:\n>> >>\n>> >> CHAINLINTTESTS = $(sort $(...,$(wildcard chainlint/*.test)))\n>> >>\n>> >>which provides stability via `sort`, thus ensures that the order of\n>> >>the \".test\" and \".expect\" match.\n>> >>\n>> >>I think that addresses your concern (unless I misunderstand your observation).\n>>\n>> But just FWIW I think both of you are wrong about the potenital for a\n>> \".test\" and \".expect\" bug here.\n>>\n>> I.e. yes the CHAINLINTTESTS variable is sorted:\n>>\n>> But in Eric's patch we just have this relevant to this concern of\n>> (paraphrased) \"would it not be sorted break it?\":\n>>\n>> + sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n>> + cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n>>\n>> So it doesn't matter if it's sorted our not.\n>>\n>> I.e. we've got {A,B,C}.{test,expect} files in a directory, and we're\n>> constructing a \"A.test\" and \"A.expect\" via \"$(patsubst)\".\n>>\n>> So if it's \"A B C\", \"C B A\", \"A C B\" etc. won't matter. We'll always get\n>> \".test\" files corresponding to \".expect\".\n>\n> Yes, sorry, I meant to say something along these lines in my reply, in\n> addition to mentioning `sort`, but forgot. Taking a look at this\n> again, though, makes me wonder if the CHAINLINTTESTS assignment should\n> be done with `:=` rather than `=` (unless GNU make is smart enough to\n> only invoke the `wildcard` operation only once, in which case it\n> wouldn't particularly matter).\n\nIt appears to be smart enough to do that, i.e. it'll invoke a $(shell)\nassignment N times for -jN unless you use simply-expanded variables, but\nnot for a simple wildcard.\n\nBut I really don't think that'll matter, doing that I/O is trivial\ncompared to other things involved there.\n\nIf for some weird reson (e.g. hundreds of thousands of dependency files)\nyou find yourself trying to optimize make runs on this basis, this is\nthe wrong way to go about it.\n\nInstead you'd have a rule depending on the contain directory, whose\nmtime will be updated should anything there change. You shouldn't need\nthat trickery for anything the size of the git codebase, but that\napproach will beat trying to optimize the O(n) lstat() calls you'll\notherwise be doing on the contents of the directory.\n"}]}