git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Dec 13, 2021, 06:30 UTC
Message-ID
<20211213063059.19424-16-sunshine@sunshineco.com>
In-Reply-To
<20211213063059.19424-1-sunshine@sunshineco.com>

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

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

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

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

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

-- 
2.34.1.397.gfae76fe5da
Previous: Eric SunshineNext: Elijah Newren
Message 32 of 34 in “generalize chainlint self-tests”
  1. 00/15 generalize chainlint self-testsEric Sunshine, Dec 13, 2021
  2. 01/15 t/chainlint/*.test: don't use invalid shell syntaxEric Sunshine, Dec 13, 2021
  3. 03/15 t/chainlint/*.test: generalize self-test commentaryEric Sunshine, Dec 13, 2021
  4. 02/15 t/chainlint/*.test: fix invalid test cases due to mixing quote typesEric Sunshine, Dec 13, 2021
  5. 04/15 t/chainlint/one-liner: avoid overly intimate chainlint.sed knowledgeEric Sunshine, Dec 13, 2021
  6. 05/15 t/Makefile: optimize chainlint self-testEric Sunshine, Dec 13, 2021
  7. Fabian StelzerDec 13, 2021
  8. Eric SunshineDec 13, 2021
  9. Fabian StelzerDec 13, 2021
  10. Eric SunshineDec 13, 2021
  11. Fabian StelzerDec 13, 2021
  12. Ævar Arnfjörð BjarmasonDec 13, 2021
  13. Eric SunshineDec 13, 2021
  14. Eric SunshineDec 13, 2021
  15. Ævar Arnfjörð BjarmasonDec 13, 2021
  16. Eric SunshineDec 13, 2021
  17. Ævar Arnfjörð BjarmasonDec 16, 2021
  18. Eric SunshineDec 16, 2021
  19. Ævar Arnfjörð BjarmasonDec 16, 2021
  20. t/Makefile: use dependency graph for "check-chainlint"Ævar Arnfjörð Bjarmason, Dec 13, 2021
  21. Eric SunshineDec 14, 2021
  22. Ævar Arnfjörð BjarmasonDec 14, 2021
  23. 06/15 chainlint.sed: improve ?!AMP?! placement accuracyEric Sunshine, Dec 13, 2021
  24. 07/15 chainlint.sed: improve ?!SEMI?! placement accuracyEric Sunshine, Dec 13, 2021
  25. 08/15 chainlint.sed: tolerate harmless ";" at end of last line in blockEric Sunshine, Dec 13, 2021
  26. 09/15 chainlint.sed: drop unnecessary distinction between ?!AMP?! and ?!SEMI?!Eric Sunshine, Dec 13, 2021
  27. 10/15 chainlint.sed: drop subshell-closing ">" annotationEric Sunshine, Dec 13, 2021
  28. 11/15 chainlint.sed: make here-doc "<<-" operator recognition more POSIX-likeEric Sunshine, Dec 13, 2021
  29. 12/15 chainlint.sed: don't mistake `<< word` in string as here-doc operatorEric Sunshine, Dec 13, 2021
  30. 13/15 chainlint.sed: stop throwing away here-doc tagsEric Sunshine, Dec 13, 2021
  31. 14/15 chainlint.sed: swallow comments consistentlyEric Sunshine, Dec 13, 2021
  32. 15/15 chainlint.sed: stop splitting "(..." into separate lines "(" and "..."Eric Sunshine, Dec 13, 2021
  33. Elijah NewrenDec 15, 2021
  34. Eric SunshineDec 15, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.