From 925d7304d2a5f5dda9fbe3f541338bb87083f6b2 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:45 -0500 Subject: [PATCH 01/15] t/chainlint/*.test: don't use invalid shell syntax The chainlint self-test code snippets are supposed to represent the body of a test_expect_success() or test_expect_failure(), yet the contents of these tests would have caused the shell to report syntax errors had they been real test bodies. Although chainlint.sed, with its simplistic heuristics, is blind to these syntactic problems, a future more robust chainlint implementation might not have such a limitation, so make these snippets syntactically valid. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint/if-then-else.expect | 5 +++-- t/chainlint/if-then-else.test | 3 ++- t/chainlint/subshell-here-doc.test | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect index 5953c7bfbc..a80f5e6c75 100644 --- a/t/chainlint/if-then-else.expect +++ b/t/chainlint/if-then-else.expect @@ -4,6 +4,7 @@ ?!AMP?! echo very echo empty elif test -z "" + then echo foo else echo foo && @@ -14,6 +15,6 @@ ( if test -n ""; then echo very && -?!AMP?! echo empty - if + echo empty + fi >) diff --git a/t/chainlint/if-then-else.test b/t/chainlint/if-then-else.test index 9bd8e9a4c6..d2b03ca6b4 100644 --- a/t/chainlint/if-then-else.test +++ b/t/chainlint/if-then-else.test @@ -7,6 +7,7 @@ # LINT: last statement before 'elif' does not need "&&" echo empty elif test -z "" + then # LINT: last statement before 'else' does not need "&&" echo foo else @@ -24,5 +25,5 @@ if test -n ""; then echo very && echo empty - if + fi ) diff --git a/t/chainlint/subshell-here-doc.test b/t/chainlint/subshell-here-doc.test index f6b3ba4214..0cce907ba8 100644 --- a/t/chainlint/subshell-here-doc.test +++ b/t/chainlint/subshell-here-doc.test @@ -11,7 +11,7 @@ # LINT: missing "&&" on 'cat' cat <bip fish fly high - EOF +EOF # LINT: swallow here-doc (EOF is last line of subshell) echo <<-\EOF >bop From 5459bc1bbb54536df18b034afd390f899bda37be Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:46 -0500 Subject: [PATCH 02/15] t/chainlint/*.test: fix invalid test cases due to mixing quote types The chainlint self-test code snippets are supposed to represent the body of a test_expect_success() or test_expect_failure(), yet the contents of a few tests would have caused the shell to report syntax errors had they been real test bodies due to the mix of single- and double-quotes. Although chainlint.sed, with its simplistic heuristics, is blind to this problem, a future more robust chainlint implementation might not have such a limitation. Therefore, stop mixing quote types haphazardly in those tests and unify quoting throughout. While at it, drop chunks of tests which merely repeat what is already tested elsewhere but with alternative quotes. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint/broken-chain.test | 2 +- t/chainlint/case.test | 6 +++--- t/chainlint/complex-if-in-cuddled-loop.test | 2 +- t/chainlint/cuddled-if-then-else.test | 2 +- t/chainlint/cuddled-loop.test | 2 +- t/chainlint/for-loop.test | 8 ++++---- t/chainlint/here-doc.expect | 2 -- t/chainlint/here-doc.test | 7 ------- t/chainlint/if-in-loop.test | 6 +++--- t/chainlint/if-then-else.test | 14 +++++++------- t/chainlint/loop-in-if.test | 6 +++--- t/chainlint/multi-line-string.expect | 8 +------- t/chainlint/multi-line-string.test | 16 ++-------------- t/chainlint/nested-subshell-comment.test | 2 +- t/chainlint/pipe.test | 2 +- t/chainlint/subshell-here-doc.expect | 1 - t/chainlint/subshell-here-doc.test | 6 +----- t/chainlint/t7900-subtree.expect | 4 ++-- t/chainlint/t7900-subtree.test | 4 ++-- t/chainlint/while-loop.test | 8 ++++---- 20 files changed, 38 insertions(+), 70 deletions(-) diff --git a/t/chainlint/broken-chain.test b/t/chainlint/broken-chain.test index 3cc67b65d0..2a44aa73b7 100644 --- a/t/chainlint/broken-chain.test +++ b/t/chainlint/broken-chain.test @@ -1,6 +1,6 @@ ( foo && -# LINT: missing "&&" from 'bar' +# LINT: missing "&&" from "bar" bar baz && # LINT: final statement before closing ")" legitimately lacks "&&" diff --git a/t/chainlint/case.test b/t/chainlint/case.test index 5ef6ff7db5..4cb086bf87 100644 --- a/t/chainlint/case.test +++ b/t/chainlint/case.test @@ -1,5 +1,5 @@ ( -# LINT: "...)" arms in 'case' not misinterpreted as subshell-closing ")" +# LINT: "...)" arms in "case" not misinterpreted as subshell-closing ")" case "$x" in x) foo ;; *) bar ;; @@ -7,7 +7,7 @@ foobar ) && ( -# LINT: missing "&&" on 'esac' +# LINT: missing "&&" on "esac" case "$x" in x) foo ;; *) bar ;; @@ -15,7 +15,7 @@ foobar ) && ( -# LINT: "...)" arm in one-liner 'case' not misinterpreted as closing ")" +# LINT: "...)" arm in one-liner "case" not misinterpreted as closing ")" case "$x" in 1) true;; esac && # LINT: same but missing "&&" case "$y" in 2) false;; esac diff --git a/t/chainlint/complex-if-in-cuddled-loop.test b/t/chainlint/complex-if-in-cuddled-loop.test index 571bbd85cd..5efeda58b2 100644 --- a/t/chainlint/complex-if-in-cuddled-loop.test +++ b/t/chainlint/complex-if-in-cuddled-loop.test @@ -1,4 +1,4 @@ -# LINT: 'for' loop cuddled with "(" and ")" and nested 'if' with complex +# LINT: "for" loop cuddled with "(" and ")" and nested "if" with complex # LINT: multi-line condition; indented with spaces, not tabs (for i in a b c; do if test "$(echo $(waffle bat))" = "eleventeen" && diff --git a/t/chainlint/cuddled-if-then-else.test b/t/chainlint/cuddled-if-then-else.test index eed774a9d6..7c53f4efe3 100644 --- a/t/chainlint/cuddled-if-then-else.test +++ b/t/chainlint/cuddled-if-then-else.test @@ -1,4 +1,4 @@ -# LINT: 'if' cuddled with "(" and ")"; indented with spaces, not tabs +# LINT: "if" cuddled with "(" and ")"; indented with spaces, not tabs (if test -z ""; then echo empty else diff --git a/t/chainlint/cuddled-loop.test b/t/chainlint/cuddled-loop.test index a841d781f0..3c2a62f751 100644 --- a/t/chainlint/cuddled-loop.test +++ b/t/chainlint/cuddled-loop.test @@ -1,4 +1,4 @@ -# LINT: 'while' loop cuddled with "(" and ")", with embedded (allowed) +# LINT: "while" loop cuddled with "(" and ")", with embedded (allowed) # LINT: "|| exit {n}" to exit loop early, and using redirection "<" to feed # LINT: loop; indented with spaces, not tabs ( while read x diff --git a/t/chainlint/for-loop.test b/t/chainlint/for-loop.test index 7db76262bc..6cb3428158 100644 --- a/t/chainlint/for-loop.test +++ b/t/chainlint/for-loop.test @@ -1,17 +1,17 @@ ( -# LINT: 'for', 'do', 'done' do not need "&&" +# LINT: "for", "do", "done" do not need "&&" for i in a b c do -# LINT: missing "&&" on 'echo' +# LINT: missing "&&" on "echo" echo $i # LINT: last statement of while does not need "&&" cat <<-\EOF bar EOF -# LINT: missing "&&" on 'done' +# LINT: missing "&&" on "done" done -# LINT: 'do' on same line as 'for' +# LINT: "do" on same line as "for" for i in a b c; do echo $i && cat $i diff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect index 534b065e38..8449eb2e02 100644 --- a/t/chainlint/here-doc.expect +++ b/t/chainlint/here-doc.expect @@ -2,8 +2,6 @@ boodle wobba gorgo snoot wafta snurb && cat >foo && -cat >bar && - cat >boo && horticulture diff --git a/t/chainlint/here-doc.test b/t/chainlint/here-doc.test index ad4ce8afd9..3f5f92cad3 100644 --- a/t/chainlint/here-doc.test +++ b/t/chainlint/here-doc.test @@ -14,13 +14,6 @@ boz woz Arbitrary_Tag_42 -# LINT: swallow 'quoted' here-doc -cat <<'FUMP' >bar && -snoz -boz -woz -FUMP - # LINT: swallow "quoted" here-doc cat <<"zump" >boo && snoz diff --git a/t/chainlint/if-in-loop.test b/t/chainlint/if-in-loop.test index daf22da164..f0cf19cfad 100644 --- a/t/chainlint/if-in-loop.test +++ b/t/chainlint/if-in-loop.test @@ -3,13 +3,13 @@ do if false then -# LINT: missing "&&" on 'echo' +# LINT: missing "&&" on "echo" echo "err" exit 1 -# LINT: missing "&&" on 'fi' +# LINT: missing "&&" on "fi" fi foo -# LINT: missing "&&" on 'done' +# LINT: missing "&&" on "done" done bar ) diff --git a/t/chainlint/if-then-else.test b/t/chainlint/if-then-else.test index d2b03ca6b4..2055336c2b 100644 --- a/t/chainlint/if-then-else.test +++ b/t/chainlint/if-then-else.test @@ -1,27 +1,27 @@ ( -# LINT: 'if', 'then', 'elif', 'else', 'fi' do not need "&&" +# LINT: "if", "then", "elif", "else", "fi" do not need "&&" if test -n "" then -# LINT: missing "&&" on 'echo' +# LINT: missing "&&" on "echo" echo very -# LINT: last statement before 'elif' does not need "&&" +# LINT: last statement before "elif" does not need "&&" echo empty elif test -z "" then -# LINT: last statement before 'else' does not need "&&" +# LINT: last statement before "else" does not need "&&" echo foo else echo foo && -# LINT: last statement before 'fi' does not need "&&" +# LINT: last statement before "fi" does not need "&&" cat <<-\EOF bar EOF -# LINT: missing "&&" on 'fi' +# LINT: missing "&&" on "fi" fi echo poodle ) && ( -# LINT: 'then' on same line as 'if' +# LINT: "then" on same line as "if" if test -n ""; then echo very && echo empty diff --git a/t/chainlint/loop-in-if.test b/t/chainlint/loop-in-if.test index 93e8ba8e4d..dfcc3f98fb 100644 --- a/t/chainlint/loop-in-if.test +++ b/t/chainlint/loop-in-if.test @@ -3,13 +3,13 @@ then while true do -# LINT: missing "&&" on 'echo' +# LINT: missing "&&" on "echo" echo "pop" echo "glup" -# LINT: missing "&&" on 'done' +# LINT: missing "&&" on "done" done foo -# LINT: missing "&&" on 'fi' +# LINT: missing "&&" on "fi" fi bar ) diff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect index 170cb59993..2829516495 100644 --- a/t/chainlint/multi-line-string.expect +++ b/t/chainlint/multi-line-string.expect @@ -1,15 +1,9 @@ ( x="line 1 line 2 line 3" && -?!AMP?! y='line 1 line2' +?!AMP?! y="line 1 line2" foobar >) && -( - echo "there's nothing to see here" && - exit ->) && ( echo "xyz" "abc def ghi" && - echo 'xyz' 'abc def ghi' && - echo 'xyz' "abc def ghi" && barfoo >) diff --git a/t/chainlint/multi-line-string.test b/t/chainlint/multi-line-string.test index 287ab89705..4a0af2107d 100644 --- a/t/chainlint/multi-line-string.test +++ b/t/chainlint/multi-line-string.test @@ -3,25 +3,13 @@ line 2 line 3" && # LINT: missing "&&" on assignment - y='line 1 - line2' + y="line 1 + line2" foobar ) && -( -# LINT: apostrophe (in a contraction) within string not misinterpreted as -# LINT: starting multi-line single-quoted string - echo "there's nothing to see here" && - exit -) && ( echo "xyz" "abc def ghi" && - echo 'xyz' 'abc - def - ghi' && - echo 'xyz' "abc - def - ghi" && barfoo ) diff --git a/t/chainlint/nested-subshell-comment.test b/t/chainlint/nested-subshell-comment.test index 0ff136ab3c..0215cdb192 100644 --- a/t/chainlint/nested-subshell-comment.test +++ b/t/chainlint/nested-subshell-comment.test @@ -7,7 +7,7 @@ # minor numbers of cows (or do they?) baz && snaff -# LINT: missing "&&" on ')' +# LINT: missing "&&" on ")" ) fuzzy ) diff --git a/t/chainlint/pipe.test b/t/chainlint/pipe.test index e6af4de916..dd82534c66 100644 --- a/t/chainlint/pipe.test +++ b/t/chainlint/pipe.test @@ -4,7 +4,7 @@ bar | baz && -# LINT: final line of pipe sequence ('cow') lacking "&&" +# LINT: final line of pipe sequence ("cow") lacking "&&" fish | cow diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect index 74723e7340..7e057aee42 100644 --- a/t/chainlint/subshell-here-doc.expect +++ b/t/chainlint/subshell-here-doc.expect @@ -5,7 +5,6 @@ >) && ( cat >bup && - cat >bup2 && cat >bup3 && meep >) diff --git a/t/chainlint/subshell-here-doc.test b/t/chainlint/subshell-here-doc.test index 0cce907ba8..d40eb65583 100644 --- a/t/chainlint/subshell-here-doc.test +++ b/t/chainlint/subshell-here-doc.test @@ -8,7 +8,7 @@ nevermore... EOF -# LINT: missing "&&" on 'cat' +# LINT: missing "&&" on "cat" cat <bip fish fly high EOF @@ -27,10 +27,6 @@ EOF glink FIZZ ARBITRARY - cat <<-'ARBITRARY2' >bup2 && - glink - FIZZ - ARBITRARY2 cat <<-"ARBITRARY3" >bup3 && glink FIZZ diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect index c9913429e6..f769244ef6 100644 --- a/t/chainlint/t7900-subtree.expect +++ b/t/chainlint/t7900-subtree.expect @@ -1,9 +1,9 @@ ( chks="sub1sub2sub3sub4" && - chks_sub=$(cat | sed 's,^,sub dir/,' + chks_sub=$(cat | sed "s,^,sub dir/," >>) && chkms="main-sub1main-sub2main-sub3main-sub4" && - chkms_sub=$(cat | sed 's,^,sub dir/,' + chkms_sub=$(cat | sed "s,^,sub dir/," >>) && subfiles=$(git ls-files) && check_equal "$subfiles" "$chkms$chks" diff --git a/t/chainlint/t7900-subtree.test b/t/chainlint/t7900-subtree.test index 277d8358df..02f3129232 100644 --- a/t/chainlint/t7900-subtree.test +++ b/t/chainlint/t7900-subtree.test @@ -3,7 +3,7 @@ sub2 sub3 sub4" && - chks_sub=$(cat < Date: Mon, 13 Dec 2021 01:30:47 -0500 Subject: [PATCH 03/15] t/chainlint/*.test: generalize self-test commentary The purpose of chainlint.sed is to detect &&-chain breakage only within subshells (one level deep); it doesn't bother checking for top-level &&-chain breakage since the &&-chain checker built into t/test-lib.sh should detect broken &&-chains outside of subshells by making them magically exit with code 117. However, this division of labor may not always be the case if a more capable chainlint implementation is ever developed. Beyond that, due to being sed-based and due to its use of heuristics, chainlint.sed has several limitations (such as being unable to detect &&-chain breakage in subshells more than one level deep since it only manually emulates recursion into a subshell). Some of the comments in the chainlint self-tests unnecessarily reflect the limitations of chainlint.sed even though those limitations are not what is being tested. Therefore, simplify and generalize the comments to explain only what is being tested, thus ensuring that they won't become outdated if a more capable chainlint is ever developed. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint/blank-line.test | 2 +- t/chainlint/block.test | 3 +-- t/chainlint/cuddled.test | 3 +-- t/chainlint/nested-subshell.test | 1 - t/chainlint/one-liner.test | 2 +- t/chainlint/semicolon.test | 4 ++-- 6 files changed, 6 insertions(+), 9 deletions(-) diff --git a/t/chainlint/blank-line.test b/t/chainlint/blank-line.test index f6dd14302b..0fdf15b3e1 100644 --- a/t/chainlint/blank-line.test +++ b/t/chainlint/blank-line.test @@ -3,7 +3,7 @@ nothing && something -# LINT: swallow blank lines since final _statement_ before subshell end is +# LINT: ignore blank lines since final _statement_ before subshell end is # LINT: significant to "&&"-check, not final _line_ (which might be blank) diff --git a/t/chainlint/block.test b/t/chainlint/block.test index d859151af1..0a82fd579f 100644 --- a/t/chainlint/block.test +++ b/t/chainlint/block.test @@ -1,6 +1,5 @@ ( -# LINT: missing "&&" in block not currently detected (for consistency with -# LINT: --chain-lint at top level and to provide escape hatch if needed) +# LINT: missing "&&" after first "echo" foo && { echo a diff --git a/t/chainlint/cuddled.test b/t/chainlint/cuddled.test index 0499fa4180..257b5b5eed 100644 --- a/t/chainlint/cuddled.test +++ b/t/chainlint/cuddled.test @@ -1,5 +1,4 @@ -# LINT: first subshell statement cuddled with opening "("; for implementation -# LINT: simplicity, "(..." is split into two lines, "(" and "..." +# LINT: first subshell statement cuddled with opening "(" (cd foo && bar ) && diff --git a/t/chainlint/nested-subshell.test b/t/chainlint/nested-subshell.test index 998b05a47d..440ee9992d 100644 --- a/t/chainlint/nested-subshell.test +++ b/t/chainlint/nested-subshell.test @@ -7,7 +7,6 @@ cd foo && ( -# LINT: nested multi-line subshell not presently checked for missing "&&" echo a echo b ) >file diff --git a/t/chainlint/one-liner.test b/t/chainlint/one-liner.test index ec9acb9825..69796d7505 100644 --- a/t/chainlint/one-liner.test +++ b/t/chainlint/one-liner.test @@ -3,7 +3,7 @@ (foo && bar) | (foo && bar) >baz && -# LINT: top-level one-liner subshell missing internal "&&" +# LINT: top-level one-liner subshell missing internal "&&" and broken &&-chain (foo; bar) && (foo; bar) | (foo; bar) >baz diff --git a/t/chainlint/semicolon.test b/t/chainlint/semicolon.test index d82c8ebbc0..67e1192c50 100644 --- a/t/chainlint/semicolon.test +++ b/t/chainlint/semicolon.test @@ -15,11 +15,11 @@ cat foo; echo bar ) && ( -# LINT: unnecessary terminating semicolon +# LINT: semicolon unnecessary but legitimate foo; ) && (cd foo && for i in a b c; do -# LINT: unnecessary terminating semicolon +# LINT: semicolon unnecessary but legitimate echo; done) From 0cca54c706128338ee79efb55e9c0ddb6be723dc Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:48 -0500 Subject: [PATCH 04/15] t/chainlint/one-liner: avoid overly intimate chainlint.sed knowledge The purpose of chainlint.sed is to detect &&-chain breakage only within subshells (one level deep); it doesn't bother checking for top-level &&-chain breakage since the &&-chain checker built into t/test-lib.sh should detect broken &&-chains outside of subshells by making them magically exit with code 117. Unfortunately, one of the chainlint.sed self-tests has overly intimate knowledge of this particular division of responsibilities and only cares about what chainlint.sed itself will produce, while ignoring the fact that a more all-encompassing linter would complain about a broken &&-chain outside the subshell. This makes it difficult to re-use the test with a more capable chainlint implementation should one ever be developed. Therefore, adjust the test and its "expected" output to avoid being specific to the tunnel-vision of this one implementation. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint/one-liner.expect | 2 +- t/chainlint/one-liner.test | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect index 237f227349..c64058f7af 100644 --- a/t/chainlint/one-liner.expect +++ b/t/chainlint/one-liner.expect @@ -4,6 +4,6 @@ ?!SEMI?!(foo; bar) && ?!SEMI?!(foo; bar) | -?!SEMI?!(foo; bar) >baz +?!SEMI?!(foo; bar) >baz && (foo "bar; baz") diff --git a/t/chainlint/one-liner.test b/t/chainlint/one-liner.test index 69796d7505..be9858fa29 100644 --- a/t/chainlint/one-liner.test +++ b/t/chainlint/one-liner.test @@ -6,7 +6,7 @@ # LINT: top-level one-liner subshell missing internal "&&" and broken &&-chain (foo; bar) && (foo; bar) | -(foo; bar) >baz +(foo; bar) >baz && # LINT: ";" in string not misinterpreted as broken &&-chain (foo "bar; baz") From f30c1d5eb1ceeec460ea4cd2089ece63156f5eb0 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:49 -0500 Subject: [PATCH 05/15] t/Makefile: optimize chainlint self-test Rather than running `chainlint` and `diff` once per self-test -- which may become expensive as more tests are added -- instead run `chainlint` a single time over all tests bodies collectively and compare the result to the collective "expected" output. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/Makefile | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/t/Makefile b/t/Makefile index 882d26eee3..f4ae40be46 100644 --- a/t/Makefile +++ b/t/Makefile @@ -71,12 +71,10 @@ clean-chainlint: check-chainlint: @mkdir -p '$(CHAINLINTTMP_SQ)' && \ - err=0 && \ - for i in $(CHAINLINTTESTS); do \ - $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/$$i.actual && \ - diff -u chainlint/$$i.expect '$(CHAINLINTTMP_SQ)'/$$i.actual || err=1; \ - done && exit $$err + sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \ + cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \ + $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \ + diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \ test-lint-filenames From db8c7a1cc02e545dd75b55e2ccbab2de51cd06ae Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:50 -0500 Subject: [PATCH 06/15] chainlint.sed: improve ?!AMP?! placement accuracy When chainlint.sed detects a broken &&-chain, it places an ?!AMP?! annotation at the beginning of the line. However, this is an unusual location for programmers accustomed to error messages (from compilers, for instance) indicating the exact point of the problem. Therefore, relocate the ?!AMP?! annotation to the end of the line in order to better direct the programmer's attention to the source of the problem. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint.sed | 8 ++++---- t/chainlint/arithmetic-expansion.expect | 2 +- t/chainlint/block.expect | 2 +- t/chainlint/broken-chain.expect | 2 +- t/chainlint/case.expect | 4 ++-- t/chainlint/command-substitution.expect | 2 +- t/chainlint/cuddled.expect | 4 ++-- t/chainlint/for-loop.expect | 4 ++-- t/chainlint/here-doc-multi-line-command-subst.expect | 2 +- t/chainlint/here-doc-multi-line-string.expect | 2 +- t/chainlint/if-in-loop.expect | 6 +++--- t/chainlint/if-then-else.expect | 4 ++-- t/chainlint/inline-comment.expect | 2 +- t/chainlint/loop-in-if.expect | 6 +++--- t/chainlint/multi-line-string.expect | 2 +- t/chainlint/nested-cuddled-subshell.expect | 6 +++--- t/chainlint/nested-here-doc.expect | 2 +- t/chainlint/nested-subshell-comment.expect | 2 +- t/chainlint/pipe.expect | 2 +- t/chainlint/semicolon.expect | 2 +- t/chainlint/subshell-here-doc.expect | 2 +- t/chainlint/subshell-one-liner.expect | 4 ++-- t/chainlint/while-loop.expect | 4 ++-- 23 files changed, 38 insertions(+), 38 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 8a25c5b855..883a2b307c 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -236,7 +236,7 @@ s/.*\n// # line ends with pipe "...|" -- valid; not missing "&&" /|[ ]*$/bcont # missing end-of-line "&&" -- mark suspect -/&&[ ]*$/!s/^/?!AMP?!/ +/&&[ ]*$/!s/$/ ?!AMP?!/ :cont # retrieve and print previous line x @@ -303,7 +303,7 @@ bcase # that line legitimately lacks "&&" :else x -s/?!AMP?!// +s/ ?!AMP?!$// x bcont @@ -311,7 +311,7 @@ bcont # "suspect" from final contained line since that line legitimately lacks "&&" :done x -s/?!AMP?!// +s/ ?!AMP?!$// x # is 'done' or 'fi' cuddled with ")" to close subshell? /done.*)/bclose @@ -354,7 +354,7 @@ bblock # since that line legitimately lacks "&&" and exit subshell loop :clssolo x -s/?!AMP?!// +s/ ?!AMP?!$// p x s/^/>/ diff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect index 09457d3196..56cd5b69f5 100644 --- a/t/chainlint/arithmetic-expansion.expect +++ b/t/chainlint/arithmetic-expansion.expect @@ -4,6 +4,6 @@ baz >) && ( -?!AMP?! bar=$((42 + 1)) + bar=$((42 + 1)) ?!AMP?! baz >) diff --git a/t/chainlint/block.expect b/t/chainlint/block.expect index fed7e89ae8..6333237cb2 100644 --- a/t/chainlint/block.expect +++ b/t/chainlint/block.expect @@ -7,6 +7,6 @@ bar && { echo c -?!AMP?! } + } ?!AMP?! baz >) diff --git a/t/chainlint/broken-chain.expect b/t/chainlint/broken-chain.expect index 55b0f42a53..0960a8c7c0 100644 --- a/t/chainlint/broken-chain.expect +++ b/t/chainlint/broken-chain.expect @@ -1,6 +1,6 @@ ( foo && -?!AMP?! bar + bar ?!AMP?! baz && wop >) diff --git a/t/chainlint/case.expect b/t/chainlint/case.expect index 41f121fbbf..a4b92d4613 100644 --- a/t/chainlint/case.expect +++ b/t/chainlint/case.expect @@ -9,11 +9,11 @@ case "$x" in x) foo ;; *) bar ;; -?!AMP?! esac + esac ?!AMP?! foobar >) && ( case "$x" in 1) true;; esac && -?!AMP?! case "$y" in 2) false;; esac + case "$y" in 2) false;; esac ?!AMP?! foobar >) diff --git a/t/chainlint/command-substitution.expect b/t/chainlint/command-substitution.expect index ad4118e537..f276067b7b 100644 --- a/t/chainlint/command-substitution.expect +++ b/t/chainlint/command-substitution.expect @@ -4,6 +4,6 @@ baz >) && ( -?!AMP?! bar=$(gobble blocks) + bar=$(gobble blocks) ?!AMP?! baz >) diff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect index b506d46221..b6c4ed90a9 100644 --- a/t/chainlint/cuddled.expect +++ b/t/chainlint/cuddled.expect @@ -4,7 +4,7 @@ cd foo && >) && ( -?!AMP?!cd foo +cd foo ?!AMP?! bar >) && @@ -17,5 +17,5 @@ cd foo && > bar) && ( -?!AMP?!cd foo +cd foo ?!AMP?! > bar) diff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect index c33cf56ee7..dc209e21bd 100644 --- a/t/chainlint/for-loop.expect +++ b/t/chainlint/for-loop.expect @@ -1,9 +1,9 @@ ( for i in a b c do -?!AMP?! echo $i + echo $i ?!AMP?! cat -?!AMP?! done + done ?!AMP?! for i in a b c; do echo $i && cat $i diff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect index e5fb752d2f..3a35bb014c 100644 --- a/t/chainlint/here-doc-multi-line-command-subst.expect +++ b/t/chainlint/here-doc-multi-line-command-subst.expect @@ -1,5 +1,5 @@ ( x=$(bobble && -?!AMP?!>> wiffle) +>> wiffle) ?!AMP?! echo $x >) diff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect index 32038a070c..a3b9a5472d 100644 --- a/t/chainlint/here-doc-multi-line-string.expect +++ b/t/chainlint/here-doc-multi-line-string.expect @@ -1,4 +1,4 @@ ( -?!AMP?! cat && echo "multi-line string" + cat && echo "multi-line string" ?!AMP?! bap >) diff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect index 03d3ceb22d..7d91837269 100644 --- a/t/chainlint/if-in-loop.expect +++ b/t/chainlint/if-in-loop.expect @@ -3,10 +3,10 @@ do if false then -?!AMP?! echo "err" + echo "err" ?!AMP?! exit 1 -?!AMP?! fi + fi ?!AMP?! foo -?!AMP?! done + done ?!AMP?! bar >) diff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect index a80f5e6c75..3055d5606c 100644 --- a/t/chainlint/if-then-else.expect +++ b/t/chainlint/if-then-else.expect @@ -1,7 +1,7 @@ ( if test -n "" then -?!AMP?! echo very + echo very ?!AMP?! echo empty elif test -z "" then @@ -9,7 +9,7 @@ else echo foo && cat -?!AMP?! fi + fi ?!AMP?! echo poodle >) && ( diff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect index fc9f250ac4..3d655a32b0 100644 --- a/t/chainlint/inline-comment.expect +++ b/t/chainlint/inline-comment.expect @@ -1,6 +1,6 @@ ( foobar && -?!AMP?! barfoo + barfoo ?!AMP?! flibble "not a # comment" >) && diff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect index 088e622c31..cebd3ae95e 100644 --- a/t/chainlint/loop-in-if.expect +++ b/t/chainlint/loop-in-if.expect @@ -3,10 +3,10 @@ then while true do -?!AMP?! echo "pop" + echo "pop" ?!AMP?! echo "glup" -?!AMP?! done + done ?!AMP?! foo -?!AMP?! fi + fi ?!AMP?! bar >) diff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect index 2829516495..f1be2baf0a 100644 --- a/t/chainlint/multi-line-string.expect +++ b/t/chainlint/multi-line-string.expect @@ -1,6 +1,6 @@ ( x="line 1 line 2 line 3" && -?!AMP?! y="line 1 line2" + y="line 1 line2" ?!AMP?! foobar >) && ( diff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect index c2a59ffc33..aa522658ed 100644 --- a/t/chainlint/nested-cuddled-subshell.expect +++ b/t/chainlint/nested-cuddled-subshell.expect @@ -4,16 +4,16 @@ >> ) && (cd foo && bar -?!AMP?!>> ) +>> ) ?!AMP?! ( cd foo && >> bar) && ( cd foo && -?!AMP?!>> bar) +>> bar) ?!AMP?! (cd foo && >> bar) && (cd foo && -?!AMP?!>> bar) +>> bar) ?!AMP?! foobar >) diff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect index 0c9ef1cfc6..f9604d3fac 100644 --- a/t/chainlint/nested-here-doc.expect +++ b/t/chainlint/nested-here-doc.expect @@ -2,6 +2,6 @@ cat >foop && ( cat && -?!AMP?! cat + cat ?!AMP?! foobar >) diff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect index 15b68d4373..925e49bae9 100644 --- a/t/chainlint/nested-subshell-comment.expect +++ b/t/chainlint/nested-subshell-comment.expect @@ -6,6 +6,6 @@ # minor numbers of cows (or do they?) baz && snaff -?!AMP?!>> ) +>> ) ?!AMP?! fuzzy >) diff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect index 211b901dbc..ede6bcc607 100644 --- a/t/chainlint/pipe.expect +++ b/t/chainlint/pipe.expect @@ -3,6 +3,6 @@ bar | baz && fish | -?!AMP?! cow + cow ?!AMP?! sunder >) diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect index 1d79384606..ffc87bdffb 100644 --- a/t/chainlint/semicolon.expect +++ b/t/chainlint/semicolon.expect @@ -1,5 +1,5 @@ ( -?!AMP?!?!SEMI?! cat foo ; echo bar +?!SEMI?! cat foo ; echo bar ?!AMP?! ?!SEMI?! cat foo ; echo bar >) && ( diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect index 7e057aee42..9d3f25b3f5 100644 --- a/t/chainlint/subshell-here-doc.expect +++ b/t/chainlint/subshell-here-doc.expect @@ -1,6 +1,6 @@ ( echo wobba gorgo snoot wafta snurb && -?!AMP?! cat >bip + cat >bip ?!AMP?! echo >bop >) && ( diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect index 51162821d7..ec77aa5b95 100644 --- a/t/chainlint/subshell-one-liner.expect +++ b/t/chainlint/subshell-one-liner.expect @@ -8,7 +8,7 @@ (foo || exit 1) && (foo || exit 1) | (foo || exit 1) >baz && -?!AMP?! (foo && bar) -?!AMP?!?!SEMI?! (foo && bar; baz) + (foo && bar) ?!AMP?! +?!SEMI?! (foo && bar; baz) ?!AMP?! foobar >) diff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect index 13cff2c0a5..f8b9fcf62b 100644 --- a/t/chainlint/while-loop.expect +++ b/t/chainlint/while-loop.expect @@ -1,9 +1,9 @@ ( while true do -?!AMP?! echo foo + echo foo ?!AMP?! cat -?!AMP?! done + done ?!AMP?! while true; do echo foo && cat bar From fbd992b61b3acb6ce235d678bfe65a60c503dddd Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:51 -0500 Subject: [PATCH 07/15] chainlint.sed: improve ?!SEMI?! placement accuracy When chainlint.sed detects commands separated by a semicolon rather than by `&&`, it places a ?!SEMI?! annotation at the beginning of the line. However, this is an unusual location for programmers accustomed to error messages (from compilers, for instance) indicating the exact point of the problem. Therefore, relocate the ?!SEMI?! annotation to the location of the semicolon in order to better direct the programmer's attention to the source of the problem. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint.sed | 4 ++-- t/chainlint/negated-one-liner.expect | 4 ++-- t/chainlint/one-liner.expect | 6 +++--- t/chainlint/semicolon.expect | 14 +++++++------- t/chainlint/subshell-one-liner.expect | 8 ++++---- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 883a2b307c..60c2099c18 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -126,7 +126,7 @@ b # "&&" (but not ";" in a string) :oneline /;/{ - /"[^"]*;[^"]*"/!s/^/?!SEMI?!/ + /"[^"]*;[^"]*"/!s/;/; ?!SEMI?!/ } b @@ -230,7 +230,7 @@ s/.*\n// # string and not ";;" in one-liner "case...esac") /;/{ /;;/!{ - /"[^"]*;[^"]*"/!s/^/?!SEMI?!/ + /"[^"]*;[^"]*"/!s/;/; ?!SEMI?!/ } } # line ends with pipe "...|" -- valid; not missing "&&" diff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect index cf18429d03..60baf84b7a 100644 --- a/t/chainlint/negated-one-liner.expect +++ b/t/chainlint/negated-one-liner.expect @@ -1,5 +1,5 @@ ! (foo && bar) && ! (foo && bar) >baz && -?!SEMI?!! (foo; bar) && -?!SEMI?!! (foo; bar) >baz +! (foo; ?!SEMI?! bar) && +! (foo; ?!SEMI?! bar) >baz diff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect index c64058f7af..3b46554728 100644 --- a/t/chainlint/one-liner.expect +++ b/t/chainlint/one-liner.expect @@ -2,8 +2,8 @@ (foo && bar) | (foo && bar) >baz && -?!SEMI?!(foo; bar) && -?!SEMI?!(foo; bar) | -?!SEMI?!(foo; bar) >baz && +(foo; ?!SEMI?! bar) && +(foo; ?!SEMI?! bar) | +(foo; ?!SEMI?! bar) >baz && (foo "bar; baz") diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect index ffc87bdffb..d2d804f5b0 100644 --- a/t/chainlint/semicolon.expect +++ b/t/chainlint/semicolon.expect @@ -1,20 +1,20 @@ ( -?!SEMI?! cat foo ; echo bar ?!AMP?! -?!SEMI?! cat foo ; echo bar + cat foo ; ?!SEMI?! echo bar ?!AMP?! + cat foo ; ?!SEMI?! echo bar >) && ( -?!SEMI?! cat foo ; echo bar && -?!SEMI?! cat foo ; echo bar + cat foo ; ?!SEMI?! echo bar && + cat foo ; ?!SEMI?! echo bar >) && ( echo "foo; bar" && -?!SEMI?! cat foo; echo bar + cat foo; ?!SEMI?! echo bar >) && ( -?!SEMI?! foo; + foo; ?!SEMI?! >) && ( cd foo && for i in a b c; do -?!SEMI?! echo; + echo; ?!SEMI?! > done) diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect index ec77aa5b95..432217801b 100644 --- a/t/chainlint/subshell-one-liner.expect +++ b/t/chainlint/subshell-one-liner.expect @@ -2,13 +2,13 @@ (foo && bar) && (foo && bar) | (foo && bar) >baz && -?!SEMI?! (foo; bar) && -?!SEMI?! (foo; bar) | -?!SEMI?! (foo; bar) >baz && + (foo; ?!SEMI?! bar) && + (foo; ?!SEMI?! bar) | + (foo; ?!SEMI?! bar) >baz && (foo || exit 1) && (foo || exit 1) | (foo || exit 1) >baz && (foo && bar) ?!AMP?! -?!SEMI?! (foo && bar; baz) ?!AMP?! + (foo && bar; ?!SEMI?! baz) ?!AMP?! foobar >) From 3865a7e36dd762f57860faac0d1a1aa9c52d9404 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:52 -0500 Subject: [PATCH 08/15] chainlint.sed: tolerate harmless ";" at end of last line in block chainlint.sed flags ";" when used as a command terminator since it breaks the &&-chain, thus can allow failures to go undetected. However, when a command terminated by ";" is the last command in the body of a compound statement, such as `command-2` in: if test $# -gt 1 then command-1 && command-2; fi then the ";" is harmless and the exit code from `command-2` is passed through untouched and becomes the exit code of the compound statement, as if the ";" was not present. Therefore, tolerate a trailing ";" in this position rather than complaining about broken &&-chain. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint.sed | 11 ++++++----- t/chainlint/semicolon.expect | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 60c2099c18..91077b6e26 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -47,8 +47,9 @@ # "?!AMP?!" violation is removed from the "bar" line (retrieved from the "hold" # area) since the final statement of a subshell must not end with "&&". The # final line of a subshell may still break the &&-chain by using ";" internally -# to chain commands together rather than "&&", so "?!SEMI?!" is never removed -# from a line (even though "?!AMP?!" might be). +# to chain commands together rather than "&&", so "?!SEMI?!" is not removed +# from such a line; however, if the line ends with "?!SEMI?!", then the ";" is +# harmless and the annotation is removed. # # Care is taken to recognize the last _statement_ of a multi-line subshell, not # necessarily the last textual _line_ within the subshell, since &&-chaining @@ -303,7 +304,7 @@ bcase # that line legitimately lacks "&&" :else x -s/ ?!AMP?!$// +s/\( ?!SEMI?!\)* ?!AMP?!$// x bcont @@ -311,7 +312,7 @@ bcont # "suspect" from final contained line since that line legitimately lacks "&&" :done x -s/ ?!AMP?!$// +s/\( ?!SEMI?!\)* ?!AMP?!$// x # is 'done' or 'fi' cuddled with ")" to close subshell? /done.*)/bclose @@ -354,7 +355,7 @@ bblock # since that line legitimately lacks "&&" and exit subshell loop :clssolo x -s/ ?!AMP?!$// +s/\( ?!SEMI?!\)* ?!AMP?!$// p x s/^/>/ diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect index d2d804f5b0..0e6389f532 100644 --- a/t/chainlint/semicolon.expect +++ b/t/chainlint/semicolon.expect @@ -11,10 +11,10 @@ cat foo; ?!SEMI?! echo bar >) && ( - foo; ?!SEMI?! + foo; >) && ( cd foo && for i in a b c; do - echo; ?!SEMI?! + echo; > done) From 0d7131763e7762dc34c407f32e3c63123001a292 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:53 -0500 Subject: [PATCH 09/15] chainlint.sed: drop unnecessary distinction between ?!AMP?! and ?!SEMI?! >From inception, when chainlint.sed encountered a line using semicolon to separate commands rather than `&&`, it would insert a ?!SEMI?! annotation at the beginning of the line rather ?!AMP?! even though the &&-chain is also broken by the semicolon. Given a line such as: ?!SEMI?! cmd1; cmd2 && the ?!SEMI?! annotation makes it easier to see what the problem is than if the output had been: ?!AMP?! cmd1; cmd2 && which might confuse the test author into thinking that the linter is broken (since the line clearly ends with `&&`). However, now that the ?!AMP?! an ?!SEMI?! annotations are inserted at the point of breakage rather than at the beginning of the line, and taking into account that both represent a broken &&-chain, there is little reason to distinguish between the two. Using ?!AMP?! alone is sufficient to point the test author at the problem. For instance, in: cmd1; ?!AMP?! cmd2 && cmd3 it is clear that the &&-chain is broken between `cmd1` and `cmd2`. Likewise, in: cmd1 && cmd2 ?!AMP?! cmd3 it is clear that the &&-chain is broken between `cmd2` and `cmd3`. Finally, in: cmd1; ?!AMP?! cmd2 ?!AMP?! cmd3 it is clear that the &&-chain is broken between each command. Hence, there is no longer a good reason to make a distinction between a broken &&-chain due to a semicolon and a broken chain due to a missing `&&` at end-of-line. Therefore, drop the ?!SEMI?! annotation and use ?!AMP?! exclusively. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint.sed | 21 ++++++++++----------- t/chainlint/negated-one-liner.expect | 4 ++-- t/chainlint/one-liner.expect | 6 +++--- t/chainlint/semicolon.expect | 10 +++++----- t/chainlint/subshell-one-liner.expect | 8 ++++---- 5 files changed, 24 insertions(+), 25 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 91077b6e26..f5fcca09ca 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -24,9 +24,9 @@ # in order to avoid misinterpreting the ")" in constructs such as "x=$(...)" # and "case $x in *)" as ending the subshell. # -# Lines missing a final "&&" are flagged with "?!AMP?!", and lines which chain -# commands with ";" internally rather than "&&" are flagged "?!SEMI?!". A line -# may be flagged for both violations. +# Lines missing a final "&&" are flagged with "?!AMP?!", as are lines which +# chain commands with ";" internally rather than "&&". A line may be flagged +# for both violations. # # Detection of a missing &&-link in a multi-line subshell is complicated by the # fact that the last statement before the closing ")" must not end with "&&". @@ -47,9 +47,8 @@ # "?!AMP?!" violation is removed from the "bar" line (retrieved from the "hold" # area) since the final statement of a subshell must not end with "&&". The # final line of a subshell may still break the &&-chain by using ";" internally -# to chain commands together rather than "&&", so "?!SEMI?!" is not removed -# from such a line; however, if the line ends with "?!SEMI?!", then the ";" is -# harmless and the annotation is removed. +# to chain commands together rather than "&&", but an internal "?!AMP?!" is +# never removed from a line even though a line-ending "?!AMP?!" might be. # # Care is taken to recognize the last _statement_ of a multi-line subshell, not # necessarily the last textual _line_ within the subshell, since &&-chaining @@ -127,7 +126,7 @@ b # "&&" (but not ";" in a string) :oneline /;/{ - /"[^"]*;[^"]*"/!s/;/; ?!SEMI?!/ + /"[^"]*;[^"]*"/!s/;/; ?!AMP?!/ } b @@ -231,7 +230,7 @@ s/.*\n// # string and not ";;" in one-liner "case...esac") /;/{ /;;/!{ - /"[^"]*;[^"]*"/!s/;/; ?!SEMI?!/ + /"[^"]*;[^"]*"/!s/;/; ?!AMP?!/ } } # line ends with pipe "...|" -- valid; not missing "&&" @@ -304,7 +303,7 @@ bcase # that line legitimately lacks "&&" :else x -s/\( ?!SEMI?!\)* ?!AMP?!$// +s/\( ?!AMP?!\)* ?!AMP?!$// x bcont @@ -312,7 +311,7 @@ bcont # "suspect" from final contained line since that line legitimately lacks "&&" :done x -s/\( ?!SEMI?!\)* ?!AMP?!$// +s/\( ?!AMP?!\)* ?!AMP?!$// x # is 'done' or 'fi' cuddled with ")" to close subshell? /done.*)/bclose @@ -355,7 +354,7 @@ bblock # since that line legitimately lacks "&&" and exit subshell loop :clssolo x -s/\( ?!SEMI?!\)* ?!AMP?!$// +s/\( ?!AMP?!\)* ?!AMP?!$// p x s/^/>/ diff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect index 60baf84b7a..ad4c2d949e 100644 --- a/t/chainlint/negated-one-liner.expect +++ b/t/chainlint/negated-one-liner.expect @@ -1,5 +1,5 @@ ! (foo && bar) && ! (foo && bar) >baz && -! (foo; ?!SEMI?! bar) && -! (foo; ?!SEMI?! bar) >baz +! (foo; ?!AMP?! bar) && +! (foo; ?!AMP?! bar) >baz diff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect index 3b46554728..57a7a444c1 100644 --- a/t/chainlint/one-liner.expect +++ b/t/chainlint/one-liner.expect @@ -2,8 +2,8 @@ (foo && bar) | (foo && bar) >baz && -(foo; ?!SEMI?! bar) && -(foo; ?!SEMI?! bar) | -(foo; ?!SEMI?! bar) >baz && +(foo; ?!AMP?! bar) && +(foo; ?!AMP?! bar) | +(foo; ?!AMP?! bar) >baz && (foo "bar; baz") diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect index 0e6389f532..54a08ce582 100644 --- a/t/chainlint/semicolon.expect +++ b/t/chainlint/semicolon.expect @@ -1,14 +1,14 @@ ( - cat foo ; ?!SEMI?! echo bar ?!AMP?! - cat foo ; ?!SEMI?! echo bar + cat foo ; ?!AMP?! echo bar ?!AMP?! + cat foo ; ?!AMP?! echo bar >) && ( - cat foo ; ?!SEMI?! echo bar && - cat foo ; ?!SEMI?! echo bar + cat foo ; ?!AMP?! echo bar && + cat foo ; ?!AMP?! echo bar >) && ( echo "foo; bar" && - cat foo; ?!SEMI?! echo bar + cat foo; ?!AMP?! echo bar >) && ( foo; diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect index 432217801b..4b44632b09 100644 --- a/t/chainlint/subshell-one-liner.expect +++ b/t/chainlint/subshell-one-liner.expect @@ -2,13 +2,13 @@ (foo && bar) && (foo && bar) | (foo && bar) >baz && - (foo; ?!SEMI?! bar) && - (foo; ?!SEMI?! bar) | - (foo; ?!SEMI?! bar) >baz && + (foo; ?!AMP?! bar) && + (foo; ?!AMP?! bar) | + (foo; ?!AMP?! bar) >baz && (foo || exit 1) && (foo || exit 1) | (foo || exit 1) >baz && (foo && bar) ?!AMP?! - (foo && bar; ?!SEMI?! baz) ?!AMP?! + (foo && bar; ?!AMP?! baz) ?!AMP?! foobar >) From 5be30d0cd301f50e5f4dd992bc11ce75f457cf46 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:54 -0500 Subject: [PATCH 10/15] chainlint.sed: drop subshell-closing ">" annotation chainlint.sed inserts a ">" annotation at the beginning of a line to signal that its heuristics have identified an end-of-subshell. This was useful as a debugging aid during development of the script, but it has no value to test writers and might even confuse them into thinking that the linter is misbehaving by inserting line-noise into the shell code it is validating. Moreover, its presence also potentially makes it difficult to reuse the chainlint self-test "expect" output should a more capable linter ever be developed. Therefore, drop the ">" annotation. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint.sed | 9 --------- t/chainlint/arithmetic-expansion.expect | 4 ++-- t/chainlint/bash-array.expect | 4 ++-- t/chainlint/blank-line.expect | 2 +- t/chainlint/block.expect | 2 +- t/chainlint/broken-chain.expect | 2 +- t/chainlint/case.expect | 6 +++--- .../close-nested-and-parent-together.expect | 2 +- t/chainlint/close-subshell.expect | 16 ++++++++-------- t/chainlint/command-substitution.expect | 4 ++-- t/chainlint/comment.expect | 2 +- t/chainlint/complex-if-in-cuddled-loop.expect | 2 +- t/chainlint/cuddled-if-then-else.expect | 2 +- t/chainlint/cuddled-loop.expect | 2 +- t/chainlint/cuddled.expect | 10 +++++----- t/chainlint/exit-loop.expect | 6 +++--- t/chainlint/exit-subshell.expect | 2 +- t/chainlint/for-loop.expect | 2 +- t/chainlint/here-doc-close-subshell.expect | 2 +- .../here-doc-multi-line-command-subst.expect | 4 ++-- t/chainlint/here-doc-multi-line-string.expect | 2 +- t/chainlint/if-in-loop.expect | 2 +- t/chainlint/if-then-else.expect | 4 ++-- t/chainlint/incomplete-line.expect | 2 +- t/chainlint/inline-comment.expect | 4 ++-- t/chainlint/loop-in-if.expect | 2 +- ...multi-line-nested-command-substitution.expect | 10 +++++----- t/chainlint/multi-line-string.expect | 4 ++-- t/chainlint/nested-cuddled-subshell.expect | 14 +++++++------- t/chainlint/nested-here-doc.expect | 2 +- t/chainlint/nested-subshell-comment.expect | 4 ++-- t/chainlint/nested-subshell.expect | 6 +++--- t/chainlint/p4-filespec.expect | 2 +- t/chainlint/pipe.expect | 2 +- t/chainlint/semicolon.expect | 10 +++++----- t/chainlint/subshell-here-doc.expect | 4 ++-- t/chainlint/subshell-one-liner.expect | 2 +- t/chainlint/t7900-subtree.expect | 6 +++--- t/chainlint/while-loop.expect | 2 +- 39 files changed, 80 insertions(+), 89 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index f5fcca09ca..2689e13636 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -76,12 +76,6 @@ # newline, thus the closing here-doc tag has been found. The closing tag line # and the "<...>" prefix on the target line are then discarded, leaving just # the target line "cat >out". -# -# To facilitate regression testing (and manual debugging), a ">" annotation is -# applied to the line containing ")" which closes a subshell, ">>" to a line -# closing a nested subshell, and ">>>" to a line closing both at once. This -# makes it easy to detect whether the heuristics correctly identify -# end-of-subshell. #------------------------------------------------------------------------------ # incomplete line -- slurp up next line @@ -337,7 +331,6 @@ n x bnstslrp :nstcl -s/^/>>/ # is it "))" which closes nested and parent subshells? /)[ ]*)/bslurp bchkchn @@ -357,7 +350,6 @@ x s/\( ?!AMP?!\)* ?!AMP?!$// p x -s/^/>/ b # found closing "...)" -- exit subshell loop @@ -365,5 +357,4 @@ b x p x -s/^/>/ b diff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect index 56cd5b69f5..46ee1046af 100644 --- a/t/chainlint/arithmetic-expansion.expect +++ b/t/chainlint/arithmetic-expansion.expect @@ -2,8 +2,8 @@ foo && bar=$((42 + 1)) && baz ->) && +) && ( bar=$((42 + 1)) ?!AMP?! baz ->) +) diff --git a/t/chainlint/bash-array.expect b/t/chainlint/bash-array.expect index c4a830d1c1..4c34eaee45 100644 --- a/t/chainlint/bash-array.expect +++ b/t/chainlint/bash-array.expect @@ -2,9 +2,9 @@ foo && bar=(gumbo stumbo wumbo) && baz ->) && +) && ( foo && bar=${#bar[@]} && baz ->) +) diff --git a/t/chainlint/blank-line.expect b/t/chainlint/blank-line.expect index 3be939ed38..f76fde1ffb 100644 --- a/t/chainlint/blank-line.expect +++ b/t/chainlint/blank-line.expect @@ -1,4 +1,4 @@ ( nothing && something ->) +) diff --git a/t/chainlint/block.expect b/t/chainlint/block.expect index 6333237cb2..da60257ebc 100644 --- a/t/chainlint/block.expect +++ b/t/chainlint/block.expect @@ -9,4 +9,4 @@ echo c } ?!AMP?! baz ->) +) diff --git a/t/chainlint/broken-chain.expect b/t/chainlint/broken-chain.expect index 0960a8c7c0..cfb58fb6b9 100644 --- a/t/chainlint/broken-chain.expect +++ b/t/chainlint/broken-chain.expect @@ -3,4 +3,4 @@ bar ?!AMP?! baz && wop ->) +) diff --git a/t/chainlint/case.expect b/t/chainlint/case.expect index a4b92d4613..31f280d8ce 100644 --- a/t/chainlint/case.expect +++ b/t/chainlint/case.expect @@ -4,16 +4,16 @@ *) bar ;; esac && foobar ->) && +) && ( case "$x" in x) foo ;; *) bar ;; esac ?!AMP?! foobar ->) && +) && ( case "$x" in 1) true;; esac && case "$y" in 2) false;; esac ?!AMP?! foobar ->) +) diff --git a/t/chainlint/close-nested-and-parent-together.expect b/t/chainlint/close-nested-and-parent-together.expect index 2a910f9d66..5ef509eb49 100644 --- a/t/chainlint/close-nested-and-parent-together.expect +++ b/t/chainlint/close-nested-and-parent-together.expect @@ -1,4 +1,4 @@ ( cd foo && (bar && ->>> baz)) + baz)) diff --git a/t/chainlint/close-subshell.expect b/t/chainlint/close-subshell.expect index 184688718a..0f87db9ae6 100644 --- a/t/chainlint/close-subshell.expect +++ b/t/chainlint/close-subshell.expect @@ -1,25 +1,25 @@ ( foo ->) && +) && ( bar ->) >out && +) >out && ( baz ->) 2>err && +) 2>err && ( boo ->) ) | wuzzle && +) | wuzzle && ( bop ->) | fazz fozz && +) | fazz fozz && ( bup ->) | +) | fuzzle && ( yop ->) +) diff --git a/t/chainlint/command-substitution.expect b/t/chainlint/command-substitution.expect index f276067b7b..c72e4df9e7 100644 --- a/t/chainlint/command-substitution.expect +++ b/t/chainlint/command-substitution.expect @@ -2,8 +2,8 @@ foo && bar=$(gobble) && baz ->) && +) && ( bar=$(gobble blocks) ?!AMP?! baz ->) +) diff --git a/t/chainlint/comment.expect b/t/chainlint/comment.expect index 3be939ed38..f76fde1ffb 100644 --- a/t/chainlint/comment.expect +++ b/t/chainlint/comment.expect @@ -1,4 +1,4 @@ ( nothing && something ->) +) diff --git a/t/chainlint/complex-if-in-cuddled-loop.expect b/t/chainlint/complex-if-in-cuddled-loop.expect index 9674b88cf2..b8aa626ed0 100644 --- a/t/chainlint/complex-if-in-cuddled-loop.expect +++ b/t/chainlint/complex-if-in-cuddled-loop.expect @@ -6,5 +6,5 @@ for i in a b c; do else echo >file fi -> done) && + done) && test ! -f file diff --git a/t/chainlint/cuddled-if-then-else.expect b/t/chainlint/cuddled-if-then-else.expect index ab2a026fbc..4e089b087a 100644 --- a/t/chainlint/cuddled-if-then-else.expect +++ b/t/chainlint/cuddled-if-then-else.expect @@ -3,5 +3,5 @@ if test -z ""; then echo empty else echo bizzy -> fi) && + fi) && echo foobar diff --git a/t/chainlint/cuddled-loop.expect b/t/chainlint/cuddled-loop.expect index 8c0260d7f1..7932303763 100644 --- a/t/chainlint/cuddled-loop.expect +++ b/t/chainlint/cuddled-loop.expect @@ -1,5 +1,5 @@ ( while read x do foobar bop || exit 1 -> done ) && +) && ( cd foo ?!AMP?! bar ->) && +) && ( cd foo && -> bar) && + bar) && ( cd foo && -> bar) && + bar) && ( cd foo ?!AMP?! -> bar) + bar) diff --git a/t/chainlint/exit-loop.expect b/t/chainlint/exit-loop.expect index 84d8bdebc0..f76aa60466 100644 --- a/t/chainlint/exit-loop.expect +++ b/t/chainlint/exit-loop.expect @@ -5,7 +5,7 @@ bar && baz done ->) && +) && ( while true do @@ -13,7 +13,7 @@ bar && baz done ->) && +) && ( i=0 && while test $i -lt 10 @@ -21,4 +21,4 @@ echo $i || exit i=$(($i + 1)) done ->) +) diff --git a/t/chainlint/exit-subshell.expect b/t/chainlint/exit-subshell.expect index bf78454f74..da80339f78 100644 --- a/t/chainlint/exit-subshell.expect +++ b/t/chainlint/exit-subshell.expect @@ -2,4 +2,4 @@ foo || exit 1 bar && baz ->) +) diff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect index dc209e21bd..b74df064c5 100644 --- a/t/chainlint/for-loop.expect +++ b/t/chainlint/for-loop.expect @@ -8,4 +8,4 @@ echo $i && cat $i done ->) +) diff --git a/t/chainlint/here-doc-close-subshell.expect b/t/chainlint/here-doc-close-subshell.expect index f011e335e5..e748526570 100644 --- a/t/chainlint/here-doc-close-subshell.expect +++ b/t/chainlint/here-doc-close-subshell.expect @@ -1,2 +1,2 @@ ( -> cat) + cat) diff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect index 3a35bb014c..f1248f8ade 100644 --- a/t/chainlint/here-doc-multi-line-command-subst.expect +++ b/t/chainlint/here-doc-multi-line-command-subst.expect @@ -1,5 +1,5 @@ ( x=$(bobble && ->> wiffle) ?!AMP?! + wiffle) ?!AMP?! echo $x ->) +) diff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect index a3b9a5472d..7e883b252e 100644 --- a/t/chainlint/here-doc-multi-line-string.expect +++ b/t/chainlint/here-doc-multi-line-string.expect @@ -1,4 +1,4 @@ ( cat && echo "multi-line string" ?!AMP?! bap ->) +) diff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect index 7d91837269..03b82a3e58 100644 --- a/t/chainlint/if-in-loop.expect +++ b/t/chainlint/if-in-loop.expect @@ -9,4 +9,4 @@ foo done ?!AMP?! bar ->) +) diff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect index 3055d5606c..debcf7b756 100644 --- a/t/chainlint/if-then-else.expect +++ b/t/chainlint/if-then-else.expect @@ -11,10 +11,10 @@ cat fi ?!AMP?! echo poodle ->) && +) && ( if test -n ""; then echo very && echo empty fi ->) +) diff --git a/t/chainlint/incomplete-line.expect b/t/chainlint/incomplete-line.expect index 2f3ebabdc2..ffac8f9018 100644 --- a/t/chainlint/incomplete-line.expect +++ b/t/chainlint/incomplete-line.expect @@ -1,4 +1,4 @@ line 1 line 2 line 3 line 4 && ( line 5 line 6 line 7 line 8 ->) +) diff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect index 3d655a32b0..f6b42757d2 100644 --- a/t/chainlint/inline-comment.expect +++ b/t/chainlint/inline-comment.expect @@ -2,8 +2,8 @@ foobar && barfoo ?!AMP?! flibble "not a # comment" ->) && +) && ( cd foo && -> flibble "not a # comment") + flibble "not a # comment") diff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect index cebd3ae95e..e1be42376c 100644 --- a/t/chainlint/loop-in-if.expect +++ b/t/chainlint/loop-in-if.expect @@ -9,4 +9,4 @@ foo fi ?!AMP?! bar ->) +) diff --git a/t/chainlint/multi-line-nested-command-substitution.expect b/t/chainlint/multi-line-nested-command-substitution.expect index 59b6c8b850..300058341b 100644 --- a/t/chainlint/multi-line-nested-command-substitution.expect +++ b/t/chainlint/multi-line-nested-command-substitution.expect @@ -3,16 +3,16 @@ x=$( echo bar | cat ->> ) && + ) && echo ok ->) | +) | sort && ( bar && x=$(echo bar | cat ->> ) && + ) && y=$(echo baz | ->> fip) && + fip) && echo fail ->) +) diff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect index f1be2baf0a..ab0dadf748 100644 --- a/t/chainlint/multi-line-string.expect +++ b/t/chainlint/multi-line-string.expect @@ -2,8 +2,8 @@ x="line 1 line 2 line 3" && y="line 1 line2" ?!AMP?! foobar ->) && +) && ( echo "xyz" "abc def ghi" && barfoo ->) +) diff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect index aa522658ed..2a86885ee6 100644 --- a/t/chainlint/nested-cuddled-subshell.expect +++ b/t/chainlint/nested-cuddled-subshell.expect @@ -1,19 +1,19 @@ ( (cd foo && bar ->> ) && + ) && (cd foo && bar ->> ) ?!AMP?! + ) ?!AMP?! ( cd foo && ->> bar) && + bar) && ( cd foo && ->> bar) ?!AMP?! + bar) ?!AMP?! (cd foo && ->> bar) && + bar) && (cd foo && ->> bar) ?!AMP?! + bar) ?!AMP?! foobar ->) +) diff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect index f9604d3fac..2a51205d32 100644 --- a/t/chainlint/nested-here-doc.expect +++ b/t/chainlint/nested-here-doc.expect @@ -4,4 +4,4 @@ cat >foop && cat && cat ?!AMP?! foobar ->) +) diff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect index 925e49bae9..9138cf386d 100644 --- a/t/chainlint/nested-subshell-comment.expect +++ b/t/chainlint/nested-subshell-comment.expect @@ -6,6 +6,6 @@ # minor numbers of cows (or do they?) baz && snaff ->> ) ?!AMP?! + ) ?!AMP?! fuzzy ->) +) diff --git a/t/chainlint/nested-subshell.expect b/t/chainlint/nested-subshell.expect index c8165ad19e..41a48adaa2 100644 --- a/t/chainlint/nested-subshell.expect +++ b/t/chainlint/nested-subshell.expect @@ -3,10 +3,10 @@ ( echo a && echo b ->> ) >file && + ) >file && cd foo && ( echo a echo b ->> ) >file ->) + ) >file +) diff --git a/t/chainlint/p4-filespec.expect b/t/chainlint/p4-filespec.expect index 98b3d881fd..1290fd1ff2 100644 --- a/t/chainlint/p4-filespec.expect +++ b/t/chainlint/p4-filespec.expect @@ -1,4 +1,4 @@ ( p4 print -1 //depot/fiddle#42 >file && foobar ->) +) diff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect index ede6bcc607..2cfc028297 100644 --- a/t/chainlint/pipe.expect +++ b/t/chainlint/pipe.expect @@ -5,4 +5,4 @@ fish | cow ?!AMP?! sunder ->) +) diff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect index 54a08ce582..05141a96cf 100644 --- a/t/chainlint/semicolon.expect +++ b/t/chainlint/semicolon.expect @@ -1,20 +1,20 @@ ( cat foo ; ?!AMP?! echo bar ?!AMP?! cat foo ; ?!AMP?! echo bar ->) && +) && ( cat foo ; ?!AMP?! echo bar && cat foo ; ?!AMP?! echo bar ->) && +) && ( echo "foo; bar" && cat foo; ?!AMP?! echo bar ->) && +) && ( foo; ->) && +) && ( cd foo && for i in a b c; do echo; -> done) + done) diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect index 9d3f25b3f5..b7250ca753 100644 --- a/t/chainlint/subshell-here-doc.expect +++ b/t/chainlint/subshell-here-doc.expect @@ -2,9 +2,9 @@ echo wobba gorgo snoot wafta snurb && cat >bip ?!AMP?! echo >bop ->) && +) && ( cat >bup && cat >bup3 && meep ->) +) diff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect index 4b44632b09..b7015361bf 100644 --- a/t/chainlint/subshell-one-liner.expect +++ b/t/chainlint/subshell-one-liner.expect @@ -11,4 +11,4 @@ (foo && bar) ?!AMP?! (foo && bar; ?!AMP?! baz) ?!AMP?! foobar ->) +) diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect index f769244ef6..215aca01c2 100644 --- a/t/chainlint/t7900-subtree.expect +++ b/t/chainlint/t7900-subtree.expect @@ -1,10 +1,10 @@ ( chks="sub1sub2sub3sub4" && chks_sub=$(cat | sed "s,^,sub dir/," ->>) && +) && chkms="main-sub1main-sub2main-sub3main-sub4" && chkms_sub=$(cat | sed "s,^,sub dir/," ->>) && +) && subfiles=$(git ls-files) && check_equal "$subfiles" "$chkms$chks" ->) +) diff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect index f8b9fcf62b..e2813b378e 100644 --- a/t/chainlint/while-loop.expect +++ b/t/chainlint/while-loop.expect @@ -8,4 +8,4 @@ echo foo && cat bar done ->) +) From 2d53614210919c2a4eac4a03e9d34da7b9e7ff31 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:55 -0500 Subject: [PATCH 11/15] chainlint.sed: make here-doc "<<-" operator recognition more POSIX-like According to POSIX, "<<" and "<<-" are distinct shell operators. For the latter to be recognized, no whitespace is allowed before the "-", though whitespace is allowed after the operator. However, the chainlint patterns which identify here-docs are both too loose and too tight, incorrectly allowing whitespace between "<<" and "-" but disallowing it between "-" and the here-doc tag. Fix the patterns to better match POSIX. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint.sed | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 2689e13636..b382746526 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -88,8 +88,8 @@ # here-doc -- swallow it to avoid false hits within its body (but keep the # command to which it was attached) -/<<[ ]*[-\\'"]*[A-Za-z0-9_]/ { - s/^\(.*\)<<[ ]*[-\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<\1<\1<\1< Date: Mon, 13 Dec 2021 01:30:56 -0500 Subject: [PATCH 12/15] chainlint.sed: don't mistake `<< word` in string as here-doc operator Tighten here-doc recognition to prevent it from being fooled by text which looks like a here-doc operator but happens merely to be the content of a string, such as this real-world case from t7201: echo "<<<<<<< ours" && echo ourside && echo "=======" && echo theirside && echo ">>>>>>> theirs" This problem went unnoticed because chainlint.sed is not a real parser, but rather applies heuristics to pretend to understand shell code. In this case, it saw what it thought was a here-doc operator (`<< ours`), and fell off the end of the test looking for the closing tag "ours" which it never found, thus swallowed the remainder of the test without checking it for &&-chain breakage. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint.sed | 8 ++++++-- t/chainlint/not-heredoc.expect | 14 ++++++++++++++ t/chainlint/not-heredoc.test | 16 ++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 t/chainlint/not-heredoc.expect create mode 100644 t/chainlint/not-heredoc.test diff --git a/t/chainlint.sed b/t/chainlint.sed index b382746526..2f786f890d 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -89,6 +89,7 @@ # here-doc -- swallow it to avoid false hits within its body (but keep the # command to which it was attached) /<<-*[ ]*[\\'"]*[A-Za-z0-9_]/ { + /"[^"]*<<[^"]*"/bnotdoc s/^\(.*\)<<-*[ ]*[\\'"]*\([A-Za-z0-9_][A-Za-z0-9_]*\)['"]*/<\2>\1<]*>// s/\n.*$// } +:notdoc # one-liner "(...) &&" /^[ ]*!*[ ]*(..*)[ ]*&&[ ]*$/boneline @@ -151,8 +153,10 @@ s/.*\n// /"[^'"]*'[^'"]*"/!bsqstr } :folded -# here-doc -- swallow it -/<<-*[ ]*[\\'"]*[A-Za-z0-9_]/bheredoc +# here-doc -- swallow it (but not "<<" in a string) +/<<-*[ ]*[\\'"]*[A-Za-z0-9_]/{ + /"[^"]*<<[^"]*"/!bheredoc +} # comment or empty line -- discard since final non-comment, non-empty line # before closing ")", "done", "elsif", "else", or "fi" will need to be # re-visited to drop "suspect" marking since final line of those constructs diff --git a/t/chainlint/not-heredoc.expect b/t/chainlint/not-heredoc.expect new file mode 100644 index 0000000000..2e9bb135fe --- /dev/null +++ b/t/chainlint/not-heredoc.expect @@ -0,0 +1,14 @@ +echo "<<<<<<< ours" && +echo ourside && +echo "=======" && +echo theirside && +echo ">>>>>>> theirs" && + +( + echo "<<<<<<< ours" && + echo ourside && + echo "=======" && + echo theirside && + echo ">>>>>>> theirs" ?!AMP?! + poodle +) >merged diff --git a/t/chainlint/not-heredoc.test b/t/chainlint/not-heredoc.test new file mode 100644 index 0000000000..9aa57346cd --- /dev/null +++ b/t/chainlint/not-heredoc.test @@ -0,0 +1,16 @@ +# LINT: "<< ours" inside string is not here-doc +echo "<<<<<<< ours" && +echo ourside && +echo "=======" && +echo theirside && +echo ">>>>>>> theirs" && + +( +# LINT: "<< ours" inside string is not here-doc + echo "<<<<<<< ours" && + echo ourside && + echo "=======" && + echo theirside && + echo ">>>>>>> theirs" + poodle +) >merged From 34ba05c2966f9e96b74ea984251e3bda802d6a7a Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 13 Dec 2021 01:30:57 -0500 Subject: [PATCH 13/15] chainlint.sed: stop throwing away here-doc tags The purpose of chainlint is to highlight problems it finds in test code by inserting annotations at the location of each problem. Arbitrarily eliding bits of the code it is checking is not helpful, yet this is exactly what chainlint.sed does by cavalierly and unnecessarily dropping the here-doc operator and tag; i.e. `cat < Signed-off-by: Junio C Hamano --- t/chainlint.sed | 24 ++++++++++++------- t/chainlint/for-loop.expect | 2 +- t/chainlint/here-doc-close-subshell.expect | 2 +- .../here-doc-multi-line-command-subst.expect | 2 +- t/chainlint/here-doc-multi-line-string.expect | 2 +- t/chainlint/here-doc.expect | 8 +++---- t/chainlint/if-then-else.expect | 2 +- t/chainlint/nested-here-doc.expect | 6 ++--- t/chainlint/subshell-here-doc.expect | 10 ++++---- t/chainlint/t7900-subtree.expect | 4 ++-- t/chainlint/while-loop.expect | 2 +- 11 files changed, 35 insertions(+), 29 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 2f786f890d..0e575c0c62 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -62,20 +62,20 @@ # receives similar treatment. # # Swallowing here-docs with arbitrary tags requires a bit of finesse. When a -# line such as "cat <out" is seen, the here-doc tag is moved to the front -# of the line enclosed in angle brackets as a sentinel, giving "cat >out". +# line such as "cat <cat <\n\1$/ is attempted to see if # the content inside "<...>" matches the entirety of the newly-read line. For # instance, if the next line read is "some data", when concatenated with the -# target line, it becomes "cat >out\nsome data", and a match is attempted +# target line, it becomes "cat <cat >out\nEOF", +# encountered, it is appended to the target line giving "cat <" does match the text following the # newline, thus the closing here-doc tag has been found. The closing tag line # and the "<...>" prefix on the target line are then discarded, leaving just -# the target line "cat >out". +# the target line "cat <\1<\1\2/ :hered N /^<\([^>]*\)>.*\n[ ]*\1[ ]*$/!{ @@ -238,6 +237,7 @@ s/.*\n// :cont # retrieve and print previous line x +s/?!HERE?!/<\1<\1?!HERE?!\2\3/ :hdocsub N /^<\([^>]*\)>.*\n[ ]*\1[ ]*$/!{ @@ -293,6 +292,7 @@ bfolded # found "case ... in" -- pass through untouched :case x +s/?!HERE?!/<foo && +cat <<-Arbitrary_Tag_42 >foo && -cat >boo && +cat <boo && -horticulture +horticulture <foop && +cat <foop && ( - cat && - cat ?!AMP?! + cat <<-INPUT_END && + cat <<-EOT ?!AMP?! foobar ) diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect index b7250ca753..029d129299 100644 --- a/t/chainlint/subshell-here-doc.expect +++ b/t/chainlint/subshell-here-doc.expect @@ -1,10 +1,10 @@ ( - echo wobba gorgo snoot wafta snurb && - cat >bip ?!AMP?! - echo >bop + echo wobba gorgo snoot wafta snurb <<-EOF && + cat <bip ?!AMP?! + echo <<-EOF >bop ) && ( - cat >bup && - cat >bup3 && + cat <<-ARBITRARY >bup && + cat <<-ARBITRARY3 >bup3 && meep ) diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect index 215aca01c2..1cccc7bf7e 100644 --- a/t/chainlint/t7900-subtree.expect +++ b/t/chainlint/t7900-subtree.expect @@ -1,9 +1,9 @@ ( chks="sub1sub2sub3sub4" && - chks_sub=$(cat | sed "s,^,sub dir/," + chks_sub=$(cat < Date: Mon, 13 Dec 2021 01:30:58 -0500 Subject: [PATCH 14/15] chainlint.sed: swallow comments consistently When checking for broken a &&-chain, chainlint.sed knows that the final statement in a subshell should not end with `&&`, so it takes care to make a distinction between the final line which is an actual statement and any lines which may be mere comments preceding the closing ')'. As such, it swallows comment lines so that they do not interfere with the &&-chain check. However, since `sed` does not provide any sort of real recursion, chainlint.sed only checks &&-chains in subshells one level deep; it doesn't do any checking in deeper subshells or in `{...}` blocks within subshells. Furthermore, on account of potential implementation complexity, it doesn't check &&-chains within `case` arms. Due to an oversight, it also doesn't swallow comments inside deep subshells, `{...}` blocks, or `case` statements, which makes its output inconsistent (swallowing comments in some cases but not others). Unfortunately, this inconsistency seeps into the chainlint self-test "expect" files, which potentially makes it difficult to reuse the self-tests should a more capable chainlint ever be developed. Therefore, teach chainlint.sed to consistently swallow comments in all cases. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/chainlint.sed | 21 +++++++++++++++++++-- t/chainlint/block-comment.expect | 6 ++++++ t/chainlint/block-comment.test | 8 ++++++++ t/chainlint/case-comment.expect | 8 ++++++++ t/chainlint/case-comment.test | 11 +++++++++++ t/chainlint/nested-subshell-comment.expect | 2 -- 6 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 t/chainlint/block-comment.expect create mode 100644 t/chainlint/block-comment.test create mode 100644 t/chainlint/case-comment.expect create mode 100644 t/chainlint/case-comment.test diff --git a/t/chainlint.sed b/t/chainlint.sed index 0e575c0c62..b1505ef2cd 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -294,6 +294,12 @@ bfolded x s/?!HERE?!/< Date: Mon, 13 Dec 2021 01:30:59 -0500 Subject: [PATCH 15/15] chainlint.sed: stop splitting "(..." into separate lines "(" and "..." Because `sed` is line-oriented, for ease of implementation, when chainlint.sed encounters an opening subshell in which the first command is cuddled with the "(", it splits the line into two lines: one containing only "(", and the other containing whatever follows "(". This allows chainlint.sed to get by with a single set of regular expressions for matching shell statements rather than having to duplicate each expression (one set for matching non-cuddled statements, and one set for matching cuddled statements). However, although syntactically and semantically immaterial, this transformation has no value to test authors and might even confuse them into thinking that the linter is misbehaving by inserting (whitespace) line-noise into the shell code it is validating. Moreover, it also allows an implementation detail of chainlint.sed to seep into the chainlint self-test "expect" files, which potentially makes it difficult to reuse the self-tests should a more capable chainlint ever be developed. To address these concerns, stop splitting cuddled "(..." into two lines. Note that, as an implementation artifact, due to sed's line-oriented nature, this change inserts a blank line at output time just before the "(..." line is emitted. It would be possible to suppress this blank line but doing so would add a fair bit of complexity to chainlint.sed. Therefore, rather than suppressing the extra blank line, the Makefile's `check-chainlint` target which runs the chainlint self-tests is instead modified to ignore blank lines when comparing chainlint output against the self-test "expect" output. This is a reasonable compromise for two reasons. First, the purpose of the chainlint self-tests is to verify that the ?!AMP?! annotations are being correctly added; precise whitespace is immaterial. Second, by necessity, chainlint.sed itself already throws away all blank lines within subshells since, when checking for a broken &&-chain, it needs to check the final _statement_ in a subshell, not the final _line_ (which might be blank), thus it has never made any attempt to precisely reproduce blank lines in its output. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/Makefile | 4 +- t/chainlint.sed | 38 ++++++++++++------- .../close-nested-and-parent-together.expect | 3 +- t/chainlint/complex-if-in-cuddled-loop.expect | 3 +- t/chainlint/cuddled-if-then-else.expect | 3 +- t/chainlint/cuddled-loop.expect | 3 +- t/chainlint/cuddled.expect | 12 ++---- t/chainlint/inline-comment.expect | 3 +- t/chainlint/semicolon.expect | 3 +- 9 files changed, 37 insertions(+), 35 deletions(-) diff --git a/t/Makefile b/t/Makefile index f4ae40be46..46cd5fc527 100644 --- a/t/Makefile +++ b/t/Makefile @@ -72,8 +72,8 @@ clean-chainlint: check-chainlint: @mkdir -p '$(CHAINLINTTMP_SQ)' && \ sed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \ - cat $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \ - $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual && \ + sed -e '/^[ ]*$$/d' $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \ + $(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests | grep -v '^[ ]*$$' >'$(CHAINLINTTMP_SQ)'/actual && \ diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \ diff --git a/t/chainlint.sed b/t/chainlint.sed index b1505ef2cd..dc4ce37cb5 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -131,11 +131,15 @@ b h bnextln } -# "(..." line -- split off and stash "(", then process "..." as its own line +# "(..." line -- "(" opening subshell cuddled with command; temporarily replace +# "(" with sentinel "^" and process the line as if "(" had been seen solo on +# the preceding line; this temporary replacement prevents several rules from +# accidentally thinking "(" introduces a nested subshell; "^" is changed back +# to "(" at output time x -s/.*/(/ +s/.*// x -s/(// +s/(/^/ bslurp :nextln @@ -168,12 +172,12 @@ s/.*\n// /"[^"]*#[^"]*"/!s/[ ]#.*$// } # one-liner "case ... esac" -/^[ ]*case[ ]*..*esac/bchkchn +/^[ ^]*case[ ]*..*esac/bchkchn # multi-line "case ... esac" -/^[ ]*case[ ]..*[ ]in/bcase +/^[ ^]*case[ ]..*[ ]in/bcase # multi-line "for ... done" or "while ... done" -/^[ ]*for[ ]..*[ ]in/bcont -/^[ ]*while[ ]/bcont +/^[ ^]*for[ ]..*[ ]in/bcont +/^[ ^]*while[ ]/bcont /^[ ]*do[ ]/bcont /^[ ]*do[ ]*$/bcont /;[ ]*do/bcont @@ -184,7 +188,7 @@ s/.*\n// /||[ ]*exit[ ]/bcont /||[ ]*exit[ ]*$/bcont # multi-line "if...elsif...else...fi" -/^[ ]*if[ ]/bcont +/^[ ^]*if[ ]/bcont /^[ ]*then[ ]/bcont /^[ ]*then[ ]*$/bcont /;[ ]*then/bcont @@ -197,15 +201,15 @@ s/.*\n// /^[ ]*fi[ ]*[<>|]/bdone /^[ ]*fi[ ]*)/bdone # nested one-liner "(...) &&" -/^[ ]*(.*)[ ]*&&[ ]*$/bchkchn +/^[ ^]*(.*)[ ]*&&[ ]*$/bchkchn # nested one-liner "(...)" -/^[ ]*(.*)[ ]*$/bchkchn +/^[ ^]*(.*)[ ]*$/bchkchn # nested one-liner "(...) >x" (or "2>x" or "|]/bchkchn +/^[ ^]*(.*)[ ]*[0-9]*[<>|]/bchkchn # nested multi-line "(...\n...)" -/^[ ]*(/bnest +/^[ ^]*(/bnest # multi-line "{...\n...}" -/^[ ]*{/bblock +/^[ ^]*{/bblock # closing ")" on own line -- exit subshell /^[ ]*)/bclssolo # "$((...))" -- arithmetic expansion; not closing ")" @@ -237,6 +241,7 @@ s/.*\n// :cont # retrieve and print previous line x +s/^\([ ]*\)^/\1(/ s/?!HERE?!/<