{"thread":{"id":"62018","subject":"[PATCH 0/2] make chainlint output more newcomer-friendly","startedAt":"2024-08-29T09:18:25Z","lastAt":"2024-09-10T22:18:06Z","messageCount":29,"participants":["Eric Sunshine","Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"501804","messageId":"20240829091625.41297-1-ericsunshine@charter.net","threadId":"62018","inReplyTo":null,"subject":"[PATCH 0/2] make chainlint output more newcomer-friendly","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2024-08-29T09:16:23Z","receivedAt":"2024-08-29T09:18:25Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nFor the sake of newcomers to the project, I have several times over the\nlast couple years thought to update t/README to explain the rather\ncryptic and terse problem annotations emitted by chainlint (i.e.\n\"?!FOO?!\"), but I never got around to it. However, a review comment[*] I\nposted recently suggesting an update to CodingGuidelines reminded me of\nthe need to update t/README.\n\nAs such, I set about to do so but quickly realized that it would be far\nmore useful to newcomers for chainlint to emit friendly problem\ndescriptions rather than expecting users to know to consult t/README to\ninterpret the existing cryptic annotations. This patch series, which\nimproves chainlint's output, is the result of that epiphany.\n\n[*]: https://lore.kernel.org/git/CAPig+cQLr+vAzkt8UJNVCeE8osGEcEfFunG36oqxa0k8JamJzQ@mail.gmail.com/\n\nEric Sunshine (2):\n chainlint: make error messages self-explanatory\n chainlint: reduce annotation noise-factor\n\n t/chainlint.pl | 33 ++++++++++++++-----\n t/chainlint/arithmetic-expansion.expect | 2 +-\n t/chainlint/block.expect | 8 ++---\n t/chainlint/broken-chain.expect | 2 +-\n t/chainlint/case.expect | 4 +--\n t/chainlint/chain-break-false.expect | 2 +-\n t/chainlint/chained-block.expect | 2 +-\n t/chainlint/chained-subshell.expect | 4 +--\n t/chainlint/command-substitution.expect | 2 +-\n t/chainlint/complex-if-in-cuddled-loop.expect | 2 +-\n t/chainlint/cuddled.expect | 4 +--\n t/chainlint/for-loop.expect | 8 ++---\n t/chainlint/function.expect | 4 +--\n t/chainlint/here-doc-body-indent.expect | 2 +-\n t/chainlint/here-doc-body-pathological.expect | 4 +--\n t/chainlint/here-doc-body.expect | 4 +--\n t/chainlint/here-doc-double.expect | 2 +-\n t/chainlint/here-doc-indent-operator.expect | 2 +-\n .../here-doc-multi-line-command-subst.expect | 2 +-\n t/chainlint/here-doc-multi-line-string.expect | 2 +-\n t/chainlint/if-condition-split.expect | 2 +-\n t/chainlint/if-in-loop.expect | 4 +--\n t/chainlint/if-then-else.expect | 4 +--\n t/chainlint/inline-comment.expect | 2 +-\n t/chainlint/loop-detect-failure.expect | 2 +-\n t/chainlint/loop-in-if.expect | 8 ++---\n t/chainlint/multi-line-string.expect | 2 +-\n t/chainlint/negated-one-liner.expect | 4 +--\n t/chainlint/nested-cuddled-subshell.expect | 6 ++--\n t/chainlint/nested-here-doc.expect | 2 +-\n t/chainlint/nested-loop-detect-failure.expect | 6 ++--\n t/chainlint/nested-subshell-comment.expect | 2 +-\n t/chainlint/nested-subshell.expect | 2 +-\n t/chainlint/not-heredoc.expect | 2 +-\n t/chainlint/one-liner-for-loop.expect | 2 +-\n t/chainlint/one-liner.expect | 6 ++--\n t/chainlint/pipe.expect | 2 +-\n t/chainlint/semicolon.expect | 12 +++----\n t/chainlint/subshell-here-doc.expect | 2 +-\n t/chainlint/subshell-one-liner.expect | 10 +++---\n t/chainlint/token-pasting.expect | 8 ++---\n t/chainlint/unclosed-here-doc-indent.expect | 2 +-\n t/chainlint/unclosed-here-doc.expect | 2 +-\n t/chainlint/while-loop.expect | 8 ++---\n t/test-lib.sh | 2 +-\n 45 files changed, 108 insertions(+), 91 deletions(-)\n\n-- \n2.46.0\n\n"},{"id":"501805","messageId":"20240829091625.41297-2-ericsunshine@charter.net","threadId":"62018","inReplyTo":"20240829091625.41297-1-ericsunshine@charter.net","subject":"[PATCH 1/2] chainlint: make error messages self-explanatory","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2024-08-29T09:16:24Z","receivedAt":"2024-08-29T09:18:25Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nThe annotations emitted by chainlint to indicate detected problems are\noverly terse, so much so that developers new to the project -- those who\nshould most benefit from the linting -- may find them baffling. For\ninstance, although the author of chainlint and seasoned Git developers\nmay understand that \"?!AMP?!\" is an abbreviation of \"ampersand\" and\nindicates a break in the &&-chain, this may not be obvious to newcomers.\n\nSimilarly, although the annotation \"?!LOOP?!\" is understood by project\nregulars to indicate a missing `|| return 1` (or `|| exit 1` in a\nsubshell), newcomers may find it more than a little perplexing. The\n\"?!LOOP?!\" case is particularly serious since it is likely that some\nnewcomers are unaware that shell loops do not terminate automatically\nupon error, and it is more difficult for a newcomer to figure out how to\ncorrect the problem by examining surrounding code since `|| return 1`\nappears in test scrips relatively infrequently (compared, for instance,\nwith &&-chaining).\n\nAddress these shortcomings by emitting human-consumable messages which\nboth explain the problem and give a strong hint about how to correct it.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 26 ++++++++++++++-----\n t/chainlint/arithmetic-expansion.expect | 2 +-\n t/chainlint/block.expect | 8 +++---\n t/chainlint/broken-chain.expect | 2 +-\n t/chainlint/case.expect | 4 +--\n t/chainlint/chain-break-false.expect | 2 +-\n t/chainlint/chained-block.expect | 2 +-\n t/chainlint/chained-subshell.expect | 4 +--\n t/chainlint/command-substitution.expect | 2 +-\n t/chainlint/complex-if-in-cuddled-loop.expect | 2 +-\n t/chainlint/cuddled.expect | 4 +--\n t/chainlint/for-loop.expect | 8 +++---\n t/chainlint/function.expect | 4 +--\n t/chainlint/here-doc-body-indent.expect | 2 +-\n t/chainlint/here-doc-body-pathological.expect | 4 +--\n t/chainlint/here-doc-body.expect | 4 +--\n t/chainlint/here-doc-double.expect | 2 +-\n t/chainlint/here-doc-indent-operator.expect | 2 +-\n .../here-doc-multi-line-command-subst.expect | 2 +-\n t/chainlint/here-doc-multi-line-string.expect | 2 +-\n t/chainlint/if-condition-split.expect | 2 +-\n t/chainlint/if-in-loop.expect | 4 +--\n t/chainlint/if-then-else.expect | 4 +--\n t/chainlint/inline-comment.expect | 2 +-\n t/chainlint/loop-detect-failure.expect | 2 +-\n t/chainlint/loop-in-if.expect | 8 +++---\n t/chainlint/multi-line-string.expect | 2 +-\n t/chainlint/negated-one-liner.expect | 4 +--\n t/chainlint/nested-cuddled-subshell.expect | 6 ++---\n t/chainlint/nested-here-doc.expect | 2 +-\n t/chainlint/nested-loop-detect-failure.expect | 6 ++---\n t/chainlint/nested-subshell-comment.expect | 2 +-\n t/chainlint/nested-subshell.expect | 2 +-\n t/chainlint/not-heredoc.expect | 2 +-\n t/chainlint/one-liner-for-loop.expect | 2 +-\n t/chainlint/one-liner.expect | 6 ++---\n t/chainlint/pipe.expect | 2 +-\n t/chainlint/semicolon.expect | 12 ++++-----\n t/chainlint/subshell-here-doc.expect | 2 +-\n t/chainlint/subshell-one-liner.expect | 10 +++----\n t/chainlint/token-pasting.expect | 8 +++---\n t/chainlint/unclosed-here-doc-indent.expect | 2 +-\n t/chainlint/unclosed-here-doc.expect | 2 +-\n t/chainlint/while-loop.expect | 8 +++---\n 44 files changed, 102 insertions(+), 88 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 5361f23b1d..d79f183dfd 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -9,7 +9,7 @@\n # Input arguments are pathnames of shell scripts containing test definitions,\n # or globs referencing a collection of scripts. For each problem discovered,\n # the pathname of the script containing the test is printed along with the test\n-# name and the test body with a `?!FOO?!` annotation at the location of each\n+# name and the test body with a `?!ERR?!` annotation at the location of each\n # detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n # &&-chain. Returns zero if no problems are discovered, otherwise non-zero.\n \n@@ -181,7 +181,7 @@ sub swallow_heredocs {\n \t\t\t$self->{lineno} += () = $body =~ /\\n/sg;\n \t\t\tnext;\n \t\t}\n-\t\tpush(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]);\n+\t\tpush(@{$self->{parser}->{problems}}, ['HEREDOC', $tag]);\n \t\t$$b =~ /(?:\\G|\\n).*\\z/gc; # consume rest of input\n \t\tmy $body = substr($$b, $start, pos($$b) - $start);\n \t\t$self->{lineno} += () = $body =~ /\\n/sg;\n@@ -238,6 +238,7 @@ sub new {\n \t\tstop => [],\n \t\toutput => [],\n \t\theredocs => {},\n+\t\tinsubshell => 0,\n \t} => $class;\n \t$self->{lexer} = Lexer->new($self, $s);\n \treturn $self;\n@@ -296,8 +297,11 @@ sub parse_group {\n \n sub parse_subshell {\n \tmy $self = shift @_;\n-\treturn ($self->parse(qr/^\\)$/),\n-\t\t$self->expect(')'));\n+\t$self->{insubshell}++;\n+\tmy @tokens = ($self->parse(qr/^\\)$/),\n+\t\t $self->expect(')'));\n+\t$self->{insubshell}--;\n+\treturn @tokens;\n }\n \n sub parse_case_pattern {\n@@ -528,7 +532,7 @@ sub parse_loop_body {\n \treturn @tokens if ends_with(\\@tokens, [qr/^\\|\\|$/, \"\\n\", qr/^echo$/, qr/^.+$/]);\n \t# flag missing \"return/exit\" handling explicit failure in loop body\n \tmy $n = find_non_nl(\\@tokens);\n-\tpush(@{$self->{problems}}, ['LOOP', $tokens[$n]]);\n+\tpush(@{$self->{problems}}, [$self->{insubshell} ? 'LOOPEXIT' : 'LOOPRETURN', $tokens[$n]]);\n \treturn @tokens;\n }\n \n@@ -619,6 +623,15 @@ sub unwrap {\n \treturn $s\n }\n \n+sub format_problem {\n+\tlocal $_ = shift;\n+\t/^AMP$/ && return \"missing '&&'\";\n+\t/^LOOPRETURN$/ && return \"missing '|| return 1'\";\n+\t/^LOOPEXIT$/ && return \"missing '|| exit 1'\";\n+\t/^HEREDOC$/ && return 'unclosed heredoc';\n+\tdie(\"unrecognized problem type '$_'\\n\");\n+}\n+\n sub check_test {\n \tmy $self = shift @_;\n \tmy $title = unwrap(shift @_);\n@@ -641,7 +654,8 @@ sub check_test {\n \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n \t\tmy ($label, $token) = @$_;\n \t\tmy $pos = $token->[2];\n-\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!$label?! \";\n+\t\tmy $err = format_problem($label, $token);\n+\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!ERR $err?! \";\n \t\t$start = $pos;\n \t}\n \t$checked .= substr($body, $start);\ndiff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect\nindex 338ecd5861..2efd65dcbd 100644\n--- a/t/chainlint/arithmetic-expansion.expect\n+++ b/t/chainlint/arithmetic-expansion.expect\n@@ -4,6 +4,6 @@\n 5 \tbaz\n 6 ) &&\n 7 (\n-8 \tbar=$((42 + 1)) ?!AMP?!\n+8 \tbar=$((42 + 1)) ?!ERR missing '&&'?!\n 9 \tbaz\n 10 )\ndiff --git a/t/chainlint/block.expect b/t/chainlint/block.expect\nindex b62e3d58c3..28410b33ef 100644\n--- a/t/chainlint/block.expect\n+++ b/t/chainlint/block.expect\n@@ -1,20 +1,20 @@\n 2 (\n 3 \tfoo &&\n 4 \t{\n-5 \t\techo a ?!AMP?!\n+5 \t\techo a ?!ERR missing '&&'?!\n 6 \t\techo b\n 7 \t} &&\n 8 \tbar &&\n 9 \t{\n 10 \t\techo c\n-11 \t} ?!AMP?!\n+11 \t} ?!ERR missing '&&'?!\n 12 \tbaz\n 13 ) &&\n 14 \n 15 {\n-16 \techo a; ?!AMP?! echo b\n+16 \techo a; ?!ERR missing '&&'?! echo b\n 17 } &&\n-18 { echo a; ?!AMP?! echo b; } &&\n+18 { echo a; ?!ERR missing '&&'?! echo b; } &&\n 19 \n 20 {\n 21 \techo \"${var}9\" &&\ndiff --git a/t/chainlint/broken-chain.expect b/t/chainlint/broken-chain.expect\nindex 9a1838736f..2a209df0a7 100644\n--- a/t/chainlint/broken-chain.expect\n+++ b/t/chainlint/broken-chain.expect\n@@ -1,6 +1,6 @@\n 2 (\n 3 \tfoo &&\n-4 \tbar ?!AMP?!\n+4 \tbar ?!ERR missing '&&'?!\n 5 \tbaz &&\n 6 \twop\n 7 )\ndiff --git a/t/chainlint/case.expect b/t/chainlint/case.expect\nindex c04c61ff36..d00b67b766 100644\n--- a/t/chainlint/case.expect\n+++ b/t/chainlint/case.expect\n@@ -9,11 +9,11 @@\n 10 \tcase \"$x\" in\n 11 \tx) foo ;;\n 12 \t*) bar ;;\n-13 \tesac ?!AMP?!\n+13 \tesac ?!ERR missing '&&'?!\n 14 \tfoobar\n 15 ) &&\n 16 (\n 17 \tcase \"$x\" in 1) true;; esac &&\n-18 \tcase \"$y\" in 2) false;; esac ?!AMP?!\n+18 \tcase \"$y\" in 2) false;; esac ?!ERR missing '&&'?!\n 19 \tfoobar\n 20 )\ndiff --git a/t/chainlint/chain-break-false.expect b/t/chainlint/chain-break-false.expect\nindex 4f815f8e14..bfccc0d90f 100644\n--- a/t/chainlint/chain-break-false.expect\n+++ b/t/chainlint/chain-break-false.expect\n@@ -4,6 +4,6 @@\n 5 \techo failed!\n 6 \tfalse\n 7 else\n-8 \techo it went okay ?!AMP?!\n+8 \techo it went okay ?!ERR missing '&&'?!\n 9 \tcongratulate user\n 10 fi\ndiff --git a/t/chainlint/chained-block.expect b/t/chainlint/chained-block.expect\nindex a546b714a6..293d9eac42 100644\n--- a/t/chainlint/chained-block.expect\n+++ b/t/chainlint/chained-block.expect\n@@ -1,5 +1,5 @@\n 2 echo nobody home && {\n-3 \ttest the doohicky ?!AMP?!\n+3 \ttest the doohicky ?!ERR missing '&&'?!\n 4 \tright now\n 5 } &&\n 6 \ndiff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect\nindex f78b268291..2f5de4fead 100644\n--- a/t/chainlint/chained-subshell.expect\n+++ b/t/chainlint/chained-subshell.expect\n@@ -1,10 +1,10 @@\n 2 mkdir sub && (\n 3 \tcd sub &&\n-4 \tfoo the bar ?!AMP?!\n+4 \tfoo the bar ?!ERR missing '&&'?!\n 5 \tnuff said\n 6 ) &&\n 7 \n 8 cut \"-d \" -f actual | (read s1 s2 s3 &&\n-9 test -f $s1 ?!AMP?!\n+9 test -f $s1 ?!ERR missing '&&'?!\n 10 test $(cat $s2) = tree2path1 &&\n 11 test $(cat $s3) = tree3path1)\ndiff --git a/t/chainlint/command-substitution.expect b/t/chainlint/command-substitution.expect\nindex 5e31b36db6..511c918cb5 100644\n--- a/t/chainlint/command-substitution.expect\n+++ b/t/chainlint/command-substitution.expect\n@@ -4,6 +4,6 @@\n 5 \tbaz\n 6 ) &&\n 7 (\n-8 \tbar=$(gobble blocks) ?!AMP?!\n+8 \tbar=$(gobble blocks) ?!ERR missing '&&'?!\n 9 \tbaz\n 10 )\ndiff --git a/t/chainlint/complex-if-in-cuddled-loop.expect b/t/chainlint/complex-if-in-cuddled-loop.expect\nindex 3a740103db..eb855378a1 100644\n--- a/t/chainlint/complex-if-in-cuddled-loop.expect\n+++ b/t/chainlint/complex-if-in-cuddled-loop.expect\n@@ -4,6 +4,6 @@\n 5 :\n 6 else\n 7 echo >file\n-8 fi ?!LOOP?!\n+8 fi ?!ERR missing '|| exit 1'?!\n 9 done) &&\n 10 test ! -f file\ndiff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect\nindex b06d638311..65825c6879 100644\n--- a/t/chainlint/cuddled.expect\n+++ b/t/chainlint/cuddled.expect\n@@ -2,7 +2,7 @@\n 3 \tbar\n 4 ) &&\n 5 \n-6 (cd foo ?!AMP?!\n+6 (cd foo ?!ERR missing '&&'?!\n 7 \tbar\n 8 ) &&\n 9 \n@@ -13,5 +13,5 @@\n 14 (cd foo &&\n 15 \tbar) &&\n 16 \n-17 (cd foo ?!AMP?!\n+17 (cd foo ?!ERR missing '&&'?!\n 18 \tbar)\ndiff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect\nindex 908aeedf96..df6fc1a35f 100644\n--- a/t/chainlint/for-loop.expect\n+++ b/t/chainlint/for-loop.expect\n@@ -1,14 +1,14 @@\n 2 (\n 3 \tfor i in a b c\n 4 \tdo\n-5 \t\techo $i ?!AMP?!\n-6 \t\tcat <<-\\EOF ?!LOOP?!\n+5 \t\techo $i ?!ERR missing '&&'?!\n+6 \t\tcat <<-\\EOF ?!ERR missing '|| exit 1'?!\n 7 \t\tbar\n 8 \t\tEOF\n-9 \tdone ?!AMP?!\n+9 \tdone ?!ERR missing '&&'?!\n 10 \n 11 \tfor i in a b c; do\n 12 \t\techo $i &&\n-13 \t\tcat $i ?!LOOP?!\n+13 \t\tcat $i ?!ERR missing '|| exit 1'?!\n 14 \tdone\n 15 )\ndiff --git a/t/chainlint/function.expect b/t/chainlint/function.expect\nindex c226246b25..7a15be745b 100644\n--- a/t/chainlint/function.expect\n+++ b/t/chainlint/function.expect\n@@ -4,8 +4,8 @@\n 5 \n 6 remove_object() {\n 7 \tfile=$(sha1_file \"$*\") &&\n-8 \ttest -e \"$file\" ?!AMP?!\n+8 \ttest -e \"$file\" ?!ERR missing '&&'?!\n 9 \trm -f \"$file\"\n-10 } ?!AMP?!\n+10 } ?!ERR missing '&&'?!\n 11 \n 12 sha1_file arg && remove_object arg\ndiff --git a/t/chainlint/here-doc-body-indent.expect b/t/chainlint/here-doc-body-indent.expect\nindex 4323acc93d..1d7298c8ad 100644\n--- a/t/chainlint/here-doc-body-indent.expect\n+++ b/t/chainlint/here-doc-body-indent.expect\n@@ -1,2 +1,2 @@\n-2 \techo \"we should find this\" ?!AMP?!\n+2 \techo \"we should find this\" ?!ERR missing '&&'?!\n 3 \techo \"even though our heredoc has its indent stripped\"\ndiff --git a/t/chainlint/here-doc-body-pathological.expect b/t/chainlint/here-doc-body-pathological.expect\nindex a93a1fa3aa..828f232616 100644\n--- a/t/chainlint/here-doc-body-pathological.expect\n+++ b/t/chainlint/here-doc-body-pathological.expect\n@@ -1,7 +1,7 @@\n-2 \techo \"outer here-doc does not allow indented end-tag\" ?!AMP?!\n+2 \techo \"outer here-doc does not allow indented end-tag\" ?!ERR missing '&&'?!\n 3 \tcat >file <<-\\EOF &&\n 4 \tbut this inner here-doc\n 5 \tdoes allow indented EOF\n 6 \tEOF\n-7 \techo \"missing chain after\" ?!AMP?!\n+7 \techo \"missing chain after\" ?!ERR missing '&&'?!\n 8 \techo \"but this line is OK because it's the end\"\ndiff --git a/t/chainlint/here-doc-body.expect b/t/chainlint/here-doc-body.expect\nindex ddf1c412af..79b9603c1e 100644\n--- a/t/chainlint/here-doc-body.expect\n+++ b/t/chainlint/here-doc-body.expect\n@@ -1,7 +1,7 @@\n-2 \techo \"missing chain before\" ?!AMP?!\n+2 \techo \"missing chain before\" ?!ERR missing '&&'?!\n 3 \tcat >file <<-\\EOF &&\n 4 \tinside inner here-doc\n 5 \tthese are not shell commands\n 6 \tEOF\n-7 \techo \"missing chain after\" ?!AMP?!\n+7 \techo \"missing chain after\" ?!ERR missing '&&'?!\n 8 \techo \"but this line is OK because it's the end\"\ndiff --git a/t/chainlint/here-doc-double.expect b/t/chainlint/here-doc-double.expect\nindex 20dba4b452..9cb1a1a5e3 100644\n--- a/t/chainlint/here-doc-double.expect\n+++ b/t/chainlint/here-doc-double.expect\n@@ -1,2 +1,2 @@\n-8 \techo \"actual test commands\" ?!AMP?!\n+8 \techo \"actual test commands\" ?!ERR missing '&&'?!\n 9 \techo \"that should be checked\"\ndiff --git a/t/chainlint/here-doc-indent-operator.expect b/t/chainlint/here-doc-indent-operator.expect\nindex 277a11202d..2d61e5f49d 100644\n--- a/t/chainlint/here-doc-indent-operator.expect\n+++ b/t/chainlint/here-doc-indent-operator.expect\n@@ -4,7 +4,7 @@\n 5 chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data\n 6 EOF\n 7 \n-8 cat >expect << -EOF ?!AMP?!\n+8 cat >expect << -EOF ?!ERR missing '&&'?!\n 9 this is not indented\n 10 -EOF\n 11 \ndiff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect\nindex 41b55f6437..881e4d2098 100644\n--- a/t/chainlint/here-doc-multi-line-command-subst.expect\n+++ b/t/chainlint/here-doc-multi-line-command-subst.expect\n@@ -3,6 +3,6 @@\n 4 \t\tfossil\n 5 \t\tvegetable\n 6 \t\tEND\n-7 \t\twiffle) ?!AMP?!\n+7 \t\twiffle) ?!ERR missing '&&'?!\n 8 \techo $x\n 9 )\ndiff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect\nindex c71828589e..06c791e0a4 100644\n--- a/t/chainlint/here-doc-multi-line-string.expect\n+++ b/t/chainlint/here-doc-multi-line-string.expect\n@@ -1,6 +1,6 @@\n 2 (\n 3 \tcat <<-\\TXT && echo \"multi-line\n-4 \tstring\" ?!AMP?!\n+4 \tstring\" ?!ERR missing '&&'?!\n 5 \tfizzle\n 6 \tTXT\n 7 \tbap\ndiff --git a/t/chainlint/if-condition-split.expect b/t/chainlint/if-condition-split.expect\nindex 9daf3d294a..5688d93a4f 100644\n--- a/t/chainlint/if-condition-split.expect\n+++ b/t/chainlint/if-condition-split.expect\n@@ -2,6 +2,6 @@\n 3 marcia ||\n 4 kevin\n 5 then\n-6 \techo \"nomads\" ?!AMP?!\n+6 \techo \"nomads\" ?!ERR missing '&&'?!\n 7 \techo \"for sure\"\n 8 fi\ndiff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect\nindex ff8c60dbdb..253b461f87 100644\n--- a/t/chainlint/if-in-loop.expect\n+++ b/t/chainlint/if-in-loop.expect\n@@ -5,8 +5,8 @@\n 6 \t\tthen\n 7 \t\t\techo \"err\"\n 8 \t\t\texit 1\n-9 \t\tfi ?!AMP?!\n+9 \t\tfi ?!ERR missing '&&'?!\n 10 \t\tfoo\n-11 \tdone ?!AMP?!\n+11 \tdone ?!ERR missing '&&'?!\n 12 \tbar\n 13 )\ndiff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect\nindex 965d7e41a2..1b3162759f 100644\n--- a/t/chainlint/if-then-else.expect\n+++ b/t/chainlint/if-then-else.expect\n@@ -1,7 +1,7 @@\n 2 (\n 3 \tif test -n \"\"\n 4 \tthen\n-5 \t\techo very ?!AMP?!\n+5 \t\techo very ?!ERR missing '&&'?!\n 6 \t\techo empty\n 7 \telif test -z \"\"\n 8 \tthen\n@@ -11,7 +11,7 @@\n 12 \t\tcat <<-\\EOF\n 13 \t\tbar\n 14 \t\tEOF\n-15 \tfi ?!AMP?!\n+15 \tfi ?!ERR missing '&&'?!\n 16 \techo poodle\n 17 ) &&\n 18 (\ndiff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect\nindex 0285c0b22c..fedc059a0c 100644\n--- a/t/chainlint/inline-comment.expect\n+++ b/t/chainlint/inline-comment.expect\n@@ -1,6 +1,6 @@\n 2 (\n 3 \tfoobar && # comment 1\n-4 \tbarfoo ?!AMP?! # wrong position for &&\n+4 \tbarfoo ?!ERR missing '&&'?! # wrong position for &&\n 5 \tflibble \"not a # comment\"\n 6 ) &&\n 7 \ndiff --git a/t/chainlint/loop-detect-failure.expect b/t/chainlint/loop-detect-failure.expect\nindex 40c06f0d53..2d46f6d2eb 100644\n--- a/t/chainlint/loop-detect-failure.expect\n+++ b/t/chainlint/loop-detect-failure.expect\n@@ -11,5 +11,5 @@\n 12 do\n 13 \tprintf \"%\"$n\"s\" X > r2/large.$n &&\n 14 \tgit -C r2 add large.$n &&\n-15 \tgit -C r2 commit -m \"$n\" ?!LOOP?!\n+15 \tgit -C r2 commit -m \"$n\" ?!ERR missing '|| return 1'?!\n 16 done\ndiff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect\nindex 4e8c67c914..8936d7ff2d 100644\n--- a/t/chainlint/loop-in-if.expect\n+++ b/t/chainlint/loop-in-if.expect\n@@ -3,10 +3,10 @@\n 4 \tthen\n 5 \t\twhile true\n 6 \t\tdo\n-7 \t\t\techo \"pop\" ?!AMP?!\n-8 \t\t\techo \"glup\" ?!LOOP?!\n-9 \t\tdone ?!AMP?!\n+7 \t\t\techo \"pop\" ?!ERR missing '&&'?!\n+8 \t\t\techo \"glup\" ?!ERR missing '|| exit 1'?!\n+9 \t\tdone ?!ERR missing '&&'?!\n 10 \t\tfoo\n-11 \tfi ?!AMP?!\n+11 \tfi ?!ERR missing '&&'?!\n 12 \tbar\n 13 )\ndiff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect\nindex 62c54e3a5e..3c3a1de75c 100644\n--- a/t/chainlint/multi-line-string.expect\n+++ b/t/chainlint/multi-line-string.expect\n@@ -3,7 +3,7 @@\n 4 \t\tline 2\n 5 \t\tline 3\" &&\n 6 \ty=\"line 1\n-7 \t\tline2\" ?!AMP?!\n+7 \t\tline2\" ?!ERR missing '&&'?!\n 8 \tfoobar\n 9 ) &&\n 10 (\ndiff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect\nindex a6ce52a1da..12bd65264a 100644\n--- a/t/chainlint/negated-one-liner.expect\n+++ b/t/chainlint/negated-one-liner.expect\n@@ -1,5 +1,5 @@\n 2 ! (foo && bar) &&\n 3 ! (foo && bar) >baz &&\n 4 \n-5 ! (foo; ?!AMP?! bar) &&\n-6 ! (foo; ?!AMP?! bar) >baz\n+5 ! (foo; ?!ERR missing '&&'?! bar) &&\n+6 ! (foo; ?!ERR missing '&&'?! bar) >baz\ndiff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect\nindex 0191c9c294..3e947ea5e1 100644\n--- a/t/chainlint/nested-cuddled-subshell.expect\n+++ b/t/chainlint/nested-cuddled-subshell.expect\n@@ -5,7 +5,7 @@\n 6 \n 7 \t(cd foo &&\n 8 \t\tbar\n-9 \t) ?!AMP?!\n+9 \t) ?!ERR missing '&&'?!\n 10 \n 11 \t(\n 12 \t\tcd foo &&\n@@ -13,13 +13,13 @@\n 14 \n 15 \t(\n 16 \t\tcd foo &&\n-17 \t\tbar) ?!AMP?!\n+17 \t\tbar) ?!ERR missing '&&'?!\n 18 \n 19 \t(cd foo &&\n 20 \t\tbar) &&\n 21 \n 22 \t(cd foo &&\n-23 \t\tbar) ?!AMP?!\n+23 \t\tbar) ?!ERR missing '&&'?!\n 24 \n 25 \tfoobar\n 26 )\ndiff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect\nindex 70d9b68dc9..107e5afb01 100644\n--- a/t/chainlint/nested-here-doc.expect\n+++ b/t/chainlint/nested-here-doc.expect\n@@ -18,7 +18,7 @@\n 19 \ttoink\n 20 \tINPUT_END\n 21 \n-22 \tcat <<-\\EOT ?!AMP?!\n+22 \tcat <<-\\EOT ?!ERR missing '&&'?!\n 23 \ttext goes here\n 24 \tdata <\"path$i$j\" ?!LOOP?!\n-7 \tdone ?!LOOP?!\n+6 \t\techo \"$i$j\" >\"path$i$j\" ?!ERR missing '|| return 1'?!\n+7 \tdone ?!ERR missing '|| return 1'?!\n 8 done &&\n 9 \n 10 for i in 0 1 2 3 4 5 6 7 8 9;\n@@ -18,7 +18,7 @@\n 19 do\n 20 \tfor j in 0 1 2 3 4 5 6 7 8 9;\n 21 \tdo\n-22 \t\techo \"$i$j\" >\"path$i$j\" ?!LOOP?!\n+22 \t\techo \"$i$j\" >\"path$i$j\" ?!ERR missing '|| return 1'?!\n 23 \tdone || return 1\n 24 done &&\n 25 \ndiff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect\nindex f89a8d03a8..c6891919c0 100644\n--- a/t/chainlint/nested-subshell-comment.expect\n+++ b/t/chainlint/nested-subshell-comment.expect\n@@ -6,6 +6,6 @@\n 7 \t\t# minor numbers of cows (or do they?)\n 8 \t\tbaz &&\n 9 \t\tsnaff\n-10 \t) ?!AMP?!\n+10 \t) ?!ERR missing '&&'?!\n 11 \tfuzzy\n 12 )\ndiff --git a/t/chainlint/nested-subshell.expect b/t/chainlint/nested-subshell.expect\nindex 811e8a7912..b98d723edf 100644\n--- a/t/chainlint/nested-subshell.expect\n+++ b/t/chainlint/nested-subshell.expect\n@@ -7,7 +7,7 @@\n 8 \n 9 \tcd foo &&\n 10 \t(\n-11 \t\techo a ?!AMP?!\n+11 \t\techo a ?!ERR missing '&&'?!\n 12 \t\techo b\n 13 \t) >file\n 14 )\ndiff --git a/t/chainlint/not-heredoc.expect b/t/chainlint/not-heredoc.expect\nindex 611b7b75cb..9910621103 100644\n--- a/t/chainlint/not-heredoc.expect\n+++ b/t/chainlint/not-heredoc.expect\n@@ -9,6 +9,6 @@\n 10 \techo ourside &&\n 11 \techo \"=======\" &&\n 12 \techo theirside &&\n-13 \techo \">>>>>>> theirs\" ?!AMP?!\n+13 \techo \">>>>>>> theirs\" ?!ERR missing '&&'?!\n 14 \tpoodle\n 15 ) >merged\ndiff --git a/t/chainlint/one-liner-for-loop.expect b/t/chainlint/one-liner-for-loop.expect\nindex 49dcf065ef..2eb2d5fcaf 100644\n--- a/t/chainlint/one-liner-for-loop.expect\n+++ b/t/chainlint/one-liner-for-loop.expect\n@@ -3,7 +3,7 @@\n 4 \tcd dir-rename-and-content &&\n 5 \ttest_write_lines 1 2 3 4 5 >foo &&\n 6 \tmkdir olddir &&\n-7 \tfor i in a b c; do echo $i >olddir/$i; ?!LOOP?! done ?!AMP?!\n+7 \tfor i in a b c; do echo $i >olddir/$i; ?!ERR missing '|| exit 1'?! done ?!ERR missing '&&'?!\n 8 \tgit add foo olddir &&\n 9 \tgit commit -m \"original\" &&\n 10 )\ndiff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect\nindex 9861811283..2c5826e6c4 100644\n--- a/t/chainlint/one-liner.expect\n+++ b/t/chainlint/one-liner.expect\n@@ -2,8 +2,8 @@\n 3 (foo && bar) |\n 4 (foo && bar) >baz &&\n 5 \n-6 (foo; ?!AMP?! bar) &&\n-7 (foo; ?!AMP?! bar) |\n-8 (foo; ?!AMP?! bar) >baz &&\n+6 (foo; ?!ERR missing '&&'?! bar) &&\n+7 (foo; ?!ERR missing '&&'?! bar) |\n+8 (foo; ?!ERR missing '&&'?! bar) >baz &&\n 9 \n 10 (foo \"bar; baz\")\ndiff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect\nindex 1bbe5a2ce1..a198d5bdb2 100644\n--- a/t/chainlint/pipe.expect\n+++ b/t/chainlint/pipe.expect\n@@ -4,7 +4,7 @@\n 5 \tbaz &&\n 6 \n 7 \tfish |\n-8 \tcow ?!AMP?!\n+8 \tcow ?!ERR missing '&&'?!\n 9 \n 10 \tsunder\n 11 )\ndiff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect\nindex 866438310c..e22920bf2c 100644\n--- a/t/chainlint/semicolon.expect\n+++ b/t/chainlint/semicolon.expect\n@@ -1,19 +1,19 @@\n 2 (\n-3 \tcat foo ; ?!AMP?! echo bar ?!AMP?!\n-4 \tcat foo ; ?!AMP?! echo bar\n+3 \tcat foo ; ?!ERR missing '&&'?! echo bar ?!ERR missing '&&'?!\n+4 \tcat foo ; ?!ERR missing '&&'?! echo bar\n 5 ) &&\n 6 (\n-7 \tcat foo ; ?!AMP?! echo bar &&\n-8 \tcat foo ; ?!AMP?! echo bar\n+7 \tcat foo ; ?!ERR missing '&&'?! echo bar &&\n+8 \tcat foo ; ?!ERR missing '&&'?! echo bar\n 9 ) &&\n 10 (\n 11 \techo \"foo; bar\" &&\n-12 \tcat foo; ?!AMP?! echo bar\n+12 \tcat foo; ?!ERR missing '&&'?! echo bar\n 13 ) &&\n 14 (\n 15 \tfoo;\n 16 ) &&\n 17 (cd foo &&\n 18 \tfor i in a b c; do\n-19 \t\techo; ?!LOOP?!\n+19 \t\techo; ?!ERR missing '|| exit 1'?!\n 20 \tdone)\ndiff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect\nindex 5647500c82..953d8084e5 100644\n--- a/t/chainlint/subshell-here-doc.expect\n+++ b/t/chainlint/subshell-here-doc.expect\n@@ -6,7 +6,7 @@\n 7 \tnevermore...\n 8 \tEOF\n 9 \n-10 \tcat <bip ?!AMP?!\n+10 \tcat <bip ?!ERR missing '&&'?!\n 11 \tfish fly high\n 12 EOF\n 13 \ndiff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect\nindex 214316c6a0..f82296db66 100644\n--- a/t/chainlint/subshell-one-liner.expect\n+++ b/t/chainlint/subshell-one-liner.expect\n@@ -3,17 +3,17 @@\n 4 \t(foo && bar) |\n 5 \t(foo && bar) >baz &&\n 6 \n-7 \t(foo; ?!AMP?! bar) &&\n-8 \t(foo; ?!AMP?! bar) |\n-9 \t(foo; ?!AMP?! bar) >baz &&\n+7 \t(foo; ?!ERR missing '&&'?! bar) &&\n+8 \t(foo; ?!ERR missing '&&'?! bar) |\n+9 \t(foo; ?!ERR missing '&&'?! bar) >baz &&\n 10 \n 11 \t(foo || exit 1) &&\n 12 \t(foo || exit 1) |\n 13 \t(foo || exit 1) >baz &&\n 14 \n-15 \t(foo && bar) ?!AMP?!\n+15 \t(foo && bar) ?!ERR missing '&&'?!\n 16 \n-17 \t(foo && bar; ?!AMP?! baz) ?!AMP?!\n+17 \t(foo && bar; ?!ERR missing '&&'?! baz) ?!ERR missing '&&'?!\n 18 \n 19 \tfoobar\n 20 )\ndiff --git a/t/chainlint/token-pasting.expect b/t/chainlint/token-pasting.expect\nindex 64f3235d26..aa64cf75f3 100644\n--- a/t/chainlint/token-pasting.expect\n+++ b/t/chainlint/token-pasting.expect\n@@ -2,13 +2,13 @@\n 3 git config filter.rot13.clean ./rot13.sh &&\n 4 \n 5 {\n-6 echo \"*.t filter=rot13\" ?!AMP?!\n+6 echo \"*.t filter=rot13\" ?!ERR missing '&&'?!\n 7 echo \"*.i ident\"\n 8 } >.gitattributes &&\n 9 \n 10 {\n-11 echo a b c d e f g h i j k l m ?!AMP?!\n-12 echo n o p q r s t u v w x y z ?!AMP?!\n+11 echo a b c d e f g h i j k l m ?!ERR missing '&&'?!\n+12 echo n o p q r s t u v w x y z ?!ERR missing '&&'?!\n 13 echo '$Id$'\n 14 } >test &&\n 15 cat test >test.t &&\n@@ -19,7 +19,7 @@\n 20 git checkout -- test test.t test.i &&\n 21 \n 22 echo \"content-test2\" >test2.o &&\n-23 echo \"content-test3 - filename with special characters\" >\"test3 'sq',$x=.o\" ?!AMP?!\n+23 echo \"content-test3 - filename with special characters\" >\"test3 'sq',$x=.o\" ?!ERR missing '&&'?!\n 24 \n 25 downstream_url_for_sed=$(\n 26 \tprintf \"%sn\" \"$downstream_url\" |\ndiff --git a/t/chainlint/unclosed-here-doc-indent.expect b/t/chainlint/unclosed-here-doc-indent.expect\nindex f78e23cb63..d5b9ab52ee 100644\n--- a/t/chainlint/unclosed-here-doc-indent.expect\n+++ b/t/chainlint/unclosed-here-doc-indent.expect\n@@ -1,4 +1,4 @@\n 2 command_which_is_run &&\n-3 cat >expect <<-\\EOF ?!UNCLOSED-HEREDOC?! &&\n+3 cat >expect <<-\\EOF ?!ERR unclosed heredoc?! &&\n 4 we forget to end the here-doc\n 5 command_which_is_gobbled\ndiff --git a/t/chainlint/unclosed-here-doc.expect b/t/chainlint/unclosed-here-doc.expect\nindex 51304672cf..8f6d260544 100644\n--- a/t/chainlint/unclosed-here-doc.expect\n+++ b/t/chainlint/unclosed-here-doc.expect\n@@ -1,5 +1,5 @@\n 2 command_which_is_run &&\n-3 cat >expect <<\\EOF ?!UNCLOSED-HEREDOC?! &&\n+3 cat >expect <<\\EOF ?!ERR unclosed heredoc?! &&\n 4 \twe try to end the here-doc below,\n 5 \tbut the indentation throws us off\n 6 \tsince the operator is not \"<<-\".\ndiff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect\nindex 5ffabd5a93..1cfd17b3c2 100644\n--- a/t/chainlint/while-loop.expect\n+++ b/t/chainlint/while-loop.expect\n@@ -1,14 +1,14 @@\n 2 (\n 3 \twhile true\n 4 \tdo\n-5 \t\techo foo ?!AMP?!\n-6 \t\tcat <<-\\EOF ?!LOOP?!\n+5 \t\techo foo ?!ERR missing '&&'?!\n+6 \t\tcat <<-\\EOF ?!ERR missing '|| exit 1'?!\n 7 \t\tbar\n 8 \t\tEOF\n-9 \tdone ?!AMP?!\n+9 \tdone ?!ERR missing '&&'?!\n 10 \n 11 \twhile true; do\n 12 \t\techo foo &&\n-13 \t\tcat bar ?!LOOP?!\n+13 \t\tcat bar ?!ERR missing '|| exit 1'?!\n 14 \tdone\n 15 )\n-- \n2.46.0\n\n"},{"id":"501806","messageId":"20240829091625.41297-3-ericsunshine@charter.net","threadId":"62018","inReplyTo":"20240829091625.41297-1-ericsunshine@charter.net","subject":"[PATCH 2/2] chainlint: reduce annotation noise-factor","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2024-08-29T09:16:25Z","receivedAt":"2024-08-29T09:18:25Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nWhen chainlint detects a problem in a test definition, it highlights the\noffending code with an \"?!ERR ...?!\" annotation. The rather curious \"?!\"\ndelimiter was chosen to draw the reader's attention to the problem area.\n\nLater, chainlint learned to color its output when sent to a terminal.\nProblem annotations are colored with a red background which stands out\nwell from surrounding text, thus easily draws the reader's attention. As\nsuch, the additional \"?!\" decoration became superfluous (when output is\ncolored), however the decoration was retained since it serves as a good\nneedle when using the terminal's search feature to \"jump\" to the next\nproblem.\n\nNevertheless, the \"?!\" decoration is noisy and ugly and makes it\nunnecessarily difficult for the reader to pluck the problem description\nfrom the annotation. For instance, it is easier to see at a glance what\nthe problem is in:\n\n ERR missing '&&'\n\nthan in the noisier:\n\n ?!ERR missing '&&'?!\n\nTherefore drop the \"!?\" decoration when output is colored (but retain it\notherwise).\n\nNote that the preceding change gave all problem annotations a uniform\n\"ERR\" prefix which serves as a reasonably suitable replacement needle\nwhen searching in a terminal, so loss of \"?!\" in the output should not\nbe overly problematic.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 7 +++++--\n t/test-lib.sh | 2 +-\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex d79f183dfd..971ab9212a 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -591,6 +591,7 @@ sub new {\n \tmy $class = shift @_;\n \tmy $self = $class->SUPER::new(@_);\n \t$self->{ntests} = 0;\n+\t$self->{nerrs} = 0;\n \treturn $self;\n }\n \n@@ -647,8 +648,10 @@ sub check_test {\n \tmy $parser = TestParser->new(\\$body);\n \tmy @tokens = $parser->parse();\n \tmy $problems = $parser->{problems};\n+\t$self->{nerrs} += @$problems;\n \treturn unless $emit_all || @$problems;\n \tmy $c = main::fd_colors(1);\n+\tmy ($erropen, $errclose) = -t 1 ? (\"$c->{rev}$c->{red}\", $c->{reset}) : ('?!', '?!');\n \tmy $start = 0;\n \tmy $checked = '';\n \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n@@ -663,7 +666,7 @@ sub check_test {\n \t$checked =~ s/^\\d+ \\n//;\n \t$checked =~ s/(\\s) \\?!/$1?!/mg;\n \t$checked =~ s/\\?! (\\s)/?!$1/mg;\n-\t$checked =~ s/(\\?![^?]+\\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;\n+\t$checked =~ s/\\?!([^?]+)\\?!/$erropen$1$errclose/mg;\n \t$checked =~ s/^\\d+/$c->{dim}$&$c->{reset}/mg;\n \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n \tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n@@ -805,9 +808,9 @@ sub check_script {\n \t\t\tmy $c = fd_colors(1);\n \t\t\tmy $s = join('', @{$parser->{output}});\n \t\t\t$emit->(\"$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\\n\" . $s);\n-\t\t\t$nerrs += () = $s =~ /\\?![^?]+\\?!/g;\n \t\t}\n \t\t$ntests += $parser->{ntests};\n+\t\t$nerrs += $parser->{nerrs};\n \t}\n \treturn [$id, $nscripts, $ntests, $nerrs];\n }\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 54247604cb..b652cb98cd 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1606,7 +1606,7 @@ if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0 &&\n test \"${GIT_TEST_EXT_CHAIN_LINT:-1}\" != 0\n then\n \t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n-\t\tBUG \"lint error (see '?!...!? annotations above)\"\n+\t\tBUG \"lint error (see 'ERR' annotations above)\"\n fi\n \n # Last-minute variable setup\n-- \n2.46.0\n\n"},{"id":"501829","messageId":"ZtBHbftK7vdTEz93@tanuki","threadId":"62018","inReplyTo":"20240829091625.41297-2-ericsunshine@charter.net","subject":"Re: [PATCH 1/2] chainlint: make error messages self-explanatory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-29T10:03:33Z","receivedAt":"2024-08-29T10:03:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 29, 2024 at 05:16:24AM -0400, Eric Sunshine wrote:\n> From: Eric Sunshine \n> \n> The annotations emitted by chainlint to indicate detected problems are\n> overly terse, so much so that developers new to the project -- those who\n> should most benefit from the linting -- may find them baffling. For\n> instance, although the author of chainlint and seasoned Git developers\n> may understand that \"?!AMP?!\" is an abbreviation of \"ampersand\" and\n> indicates a break in the &&-chain, this may not be obvious to newcomers.\n> \n> Similarly, although the annotation \"?!LOOP?!\" is understood by project\n> regulars to indicate a missing `|| return 1` (or `|| exit 1` in a\n> subshell), newcomers may find it more than a little perplexing. The\n> \"?!LOOP?!\" case is particularly serious since it is likely that some\n> newcomers are unaware that shell loops do not terminate automatically\n> upon error, and it is more difficult for a newcomer to figure out how to\n> correct the problem by examining surrounding code since `|| return 1`\n> appears in test scrips relatively infrequently (compared, for instance,\n> with &&-chaining).\n> \n> Address these shortcomings by emitting human-consumable messages which\n> both explain the problem and give a strong hint about how to correct it.\n\nA worthwhile goal indeed. As you say, especially figuring out how to fix\nthe loop annotations is not exactly straight forward.\n\n[snip]\n> diff --git a/t/chainlint.pl b/t/chainlint.pl\n> index 5361f23b1d..d79f183dfd 100755\n> --- a/t/chainlint.pl\n> +++ b/t/chainlint.pl\n> @@ -9,7 +9,7 @@\n> # Input arguments are pathnames of shell scripts containing test definitions,\n> # or globs referencing a collection of scripts. For each problem discovered,\n> # the pathname of the script containing the test is printed along with the test\n> -# name and the test body with a `?!FOO?!` annotation at the location of each\n> +# name and the test body with a `?!ERR?!` annotation at the location of each\n> # detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n> # &&-chain. Returns zero if no problems are discovered, otherwise non-zero.\n> \n> @@ -181,7 +181,7 @@ sub swallow_heredocs {\n> \t\t\t$self->{lineno} += () = $body =~ /\\n/sg;\n> \t\t\tnext;\n> \t\t}\n> -\t\tpush(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]);\n> +\t\tpush(@{$self->{parser}->{problems}}, ['HEREDOC', $tag]);\n> \t\t$$b =~ /(?:\\G|\\n).*\\z/gc; # consume rest of input\n> \t\tmy $body = substr($$b, $start, pos($$b) - $start);\n> \t\t$self->{lineno} += () = $body =~ /\\n/sg;\n\nI was wondering why this is being changed here, as I found the old name\nto be easier to understand. Then I saw further down that you essentially\nuse those as identifiers for the actual problem.\n\nIs there a specific reason why we now have the separate translation\nstep? Couldn't we instead push the translated message here, directly?\n\n> @@ -296,8 +297,11 @@ sub parse_group {\n> \n> sub parse_subshell {\n> \tmy $self = shift @_;\n> -\treturn ($self->parse(qr/^\\)$/),\n> -\t\t$self->expect(')'));\n> +\t$self->{insubshell}++;\n> +\tmy @tokens = ($self->parse(qr/^\\)$/),\n> +\t\t $self->expect(')'));\n> +\t$self->{insubshell}--;\n> +\treturn @tokens;\n> }\n> \n> sub parse_case_pattern {\n\nOkay. The subshell recursion level tracking here is required such that\nwe can discern LOOPEXIT vs LOOPRETURN cases. Makes sense.\n\n> @@ -641,7 +654,8 @@ sub check_test {\n> \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n> \t\tmy ($label, $token) = @$_;\n> \t\tmy $pos = $token->[2];\n> -\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!$label?! \";\n> +\t\tmy $err = format_problem($label, $token);\n> +\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!ERR $err?! \";\n> \t\t$start = $pos;\n> \t}\n> \t$checked .= substr($body, $start);\n> diff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect\n> index 338ecd5861..2efd65dcbd 100644\n> --- a/t/chainlint/arithmetic-expansion.expect\n> +++ b/t/chainlint/arithmetic-expansion.expect\n> @@ -4,6 +4,6 @@\n> 5 \tbaz\n> 6 ) &&\n> 7 (\n> -8 \tbar=$((42 + 1)) ?!AMP?!\n> +8 \tbar=$((42 + 1)) ?!ERR missing '&&'?!\n> 9 \tbaz\n> 10 )\n\nI find the resulting error messages a bit confusing: to me it reads as\nif \"ERR\" is missing the ampersands. Is it actually useful to have the\nERR prefix in the first place? We do not output anything but errors, so\nit feels somewhat redundant.\n\nPatrick\n"},{"id":"501830","messageId":"ZtBHecRkFQkSAF6C@tanuki","threadId":"62018","inReplyTo":"20240829091625.41297-3-ericsunshine@charter.net","subject":"Re: [PATCH 2/2] chainlint: reduce annotation noise-factor","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-29T10:03:37Z","receivedAt":"2024-08-29T10:03:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 29, 2024 at 05:16:25AM -0400, Eric Sunshine wrote:\n> From: Eric Sunshine \n> \n> When chainlint detects a problem in a test definition, it highlights the\n> offending code with an \"?!ERR ...?!\" annotation. The rather curious \"?!\"\n> delimiter was chosen to draw the reader's attention to the problem area.\n> \n> Later, chainlint learned to color its output when sent to a terminal.\n> Problem annotations are colored with a red background which stands out\n> well from surrounding text, thus easily draws the reader's attention. As\n> such, the additional \"?!\" decoration became superfluous (when output is\n> colored), however the decoration was retained since it serves as a good\n> needle when using the terminal's search feature to \"jump\" to the next\n> problem.\n> \n> Nevertheless, the \"?!\" decoration is noisy and ugly and makes it\n> unnecessarily difficult for the reader to pluck the problem description\n> from the annotation. For instance, it is easier to see at a glance what\n> the problem is in:\n> \n> ERR missing '&&'\n> \n> than in the noisier:\n> \n> ?!ERR missing '&&'?!\n> \n> Therefore drop the \"!?\" decoration when output is colored (but retain it\n> otherwise).\n> \n> Note that the preceding change gave all problem annotations a uniform\n> \"ERR\" prefix which serves as a reasonably suitable replacement needle\n> when searching in a terminal, so loss of \"?!\" in the output should not\n> be overly problematic.\n\nOkay, now the \"ERR\" prefix becomes a bit more important because we drop\nthe other punctuation. I'm still not much of a fan of it, though. Makes\nme wonder whether we want to take a clue from how compilers nowadays\nformat this, e.g. by using \"pointers\".\n\nSo this:\n\n 2 (\n 3 \tfoo |\n 4 \tbar |\n 5 \tbaz &&\n 6 \n 7 \tfish |\n 8 \tcow ?!AMP?!\n 9 \n 10 \tsunder\n 11 )\n\nWould become this:\n\n t/chainlint/pipe.actual:8: error: expected ampersands (&&)\n 7 \tfish |\n 8 \tcow \n ^\n 9\n\nWhile this would be neat, I guess it would also be way more work than\nthe current series you have posted. And whether that work is ultimately\nreally worth it may be another question. Probably not.\n\nSo overall, I'm fine with the direction that your patch series takes.\n\nPatrick\n"},{"id":"501837","messageId":"xmqq7cbzxrry.fsf@gitster.g","threadId":"62018","inReplyTo":"20240829091625.41297-2-ericsunshine@charter.net","subject":"Re: [PATCH 1/2] chainlint: make error messages self-explanatory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-29T15:39:13Z","receivedAt":"2024-08-29T15:39:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> \"?!LOOP?!\" case is particularly serious since it is likely that some\n> newcomers are unaware that shell loops do not terminate automatically\n> upon error, and it is more difficult for a newcomer to figure out how to\n> correct the problem by examining surrounding code since `|| return 1`\n> appears in test scrips relatively infrequently (compared, for instance,\n> with &&-chaining).\n\n\"scrips\" -> \"scripts\"\n\nI'd prefer to see \"some newcomes are unaware that\" part rewritten\nand toned down, as it is not our primary business to help total\nnewbies to learn shells, it certainly is not what the chain lint\nchecker should bend over backwards to do.\n\n ... particularly serious, as it does not convey that returning\n control with \"|| return 1\" (or \"|| exit 1\" from a subshell)\n immediately after we detect an error is the canonical way we\n chose in this project to handle errors in a loop. Because it\n happens relatively infrequently, this norm is harder to figure\n out for a new person on their own than other patterns (like\n &&-chaining).\n\n> Address these shortcomings by emitting human-consumable messages which\n> both explain the problem and give a strong hint about how to correct it.\n\n\"consumable\" -> \"readable\".\n\n>\n> Signed-off-by: Eric Sunshine \n> ...\n> # Input arguments are pathnames of shell scripts containing test definitions,\n> # or globs referencing a collection of scripts. For each problem discovered,\n> # the pathname of the script containing the test is printed along with the test\n> -# name and the test body with a `?!FOO?!` annotation at the location of each\n> +# name and the test body with a `?!ERR?!` annotation at the location of each\n> # detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n\n\"FOO\" -> \"ERR\"?\n\n> @@ -619,6 +623,15 @@ sub unwrap {\n> \treturn $s\n> }\n> \n> +sub format_problem {\n> +\tlocal $_ = shift;\n> +\t/^AMP$/ && return \"missing '&&'\";\n> +\t/^LOOPRETURN$/ && return \"missing '|| return 1'\";\n> +\t/^LOOPEXIT$/ && return \"missing '|| exit 1'\";\n> +\t/^HEREDOC$/ && return 'unclosed heredoc';\n> +\tdie(\"unrecognized problem type '$_'\\n\");\n> +}\n> +\n> sub check_test {\n> \tmy $self = shift @_;\n> \tmy $title = unwrap(shift @_);\n> @@ -641,7 +654,8 @@ sub check_test {\n> \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n> \t\tmy ($label, $token) = @$_;\n> \t\tmy $pos = $token->[2];\n> -\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!$label?! \";\n> +\t\tmy $err = format_problem($label, $token);\n> +\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!ERR $err?! \";\n> \t\t$start = $pos;\n> \t}\n> \t$checked .= substr($body, $start);\n\nWith the hunks omitted before the above two that let us tell between\nRETURN vs EXIT, the above two makes the problems much easier to\nread.\n\nAll the \"examples\" (self tests) and changes to them looked sensible.\n\nThanks.\n"},{"id":"501839","messageId":"xmqqv7zjwcgq.fsf@gitster.g","threadId":"62018","inReplyTo":"20240829091625.41297-3-ericsunshine@charter.net","subject":"Re: [PATCH 2/2] chainlint: reduce annotation noise-factor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-29T15:55:17Z","receivedAt":"2024-08-29T15:55:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> From: Eric Sunshine \n>\n> When chainlint detects a problem in a test definition, it highlights the\n> offending code with an \"?!ERR ...?!\" annotation. The rather curious \"?!\"\n> delimiter was chosen to draw the reader's attention to the problem area.\n>\n> Later, chainlint learned to color its output when sent to a terminal.\n> Problem annotations are colored with a red background which stands out\n> well from surrounding text, thus easily draws the reader's attention. As\n> such, the additional \"?!\" decoration became superfluous (when output is\n> colored), however the decoration was retained since it serves as a good\n> needle when using the terminal's search feature to \"jump\" to the next\n> problem.\n>\n> Nevertheless, the \"?!\" decoration is noisy and ugly and makes it\n> unnecessarily difficult for the reader to pluck the problem description\n> from the annotation. For instance, it is easier to see at a glance what\n> the problem is in:\n>\n> ERR missing '&&'\n>\n> than in the noisier:\n>\n> ?!ERR missing '&&'?!\n>\n> Therefore drop the \"!?\" decoration when output is colored (but retain it\n> otherwise).\n\nWait. That does not qualify \"Therefore\".\n\nWe talked about a \"good needle\" and then complained how ugly the\nstring that was happened to be chosen as good needle is. That is\nnot enough to explain why it is justified to \"lose\" the needle. The\nonly thing you justified is to move away from the ugly pattern, as a\ntypical \"terminal's search feature\" does not give us an easy way to\n\"jump to the next text painted yellow\".\n\n> Note that the preceding change gave all problem annotations a uniform\n> \"ERR\" prefix which serves as a reasonably suitable replacement needle\n> when searching in a terminal, so loss of \"?!\" in the output should not\n> be overly problematic.\n\nDrop this separate paragraph, promote its contents up from \"Note\"\nstatus and as a proper part of the previous sentence in its rewrite,\nsomething like:\n\n Since the errors are all uniformly prefixed with \"ERR\", which\n can be used as the \"good needle\" instead, lose the \"!?\"\n decoration when output is colored.\n\nto replace \"Therefore\" and everything that follow.\n\n> @@ -663,7 +666,7 @@ sub check_test {\n> \t$checked =~ s/^\\d+ \\n//;\n> \t$checked =~ s/(\\s) \\?!/$1?!/mg;\n> \t$checked =~ s/\\?! (\\s)/?!$1/mg;\n> -\t$checked =~ s/(\\?![^?]+\\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;\n> +\t$checked =~ s/\\?!([^?]+)\\?!/$erropen$1$errclose/mg;\n\nHmph. With $erropen and $errclose, I was hoping that we can shed\nthe reliance on the \"?!\" mark even internally. This is especially\ntrue that in the early part of this sub, the problem description was\nvery much structured piece of data, not something the consuming code\nneed to pick out of an already formatted text like this, risking to\nget confused by the payload (i.e. the text that came from the\nproblematic test script inside \"substr($body, $start, $pos-$start)\"\nmay contain anything, including \"?!\", right?).\n\n> \t$checked =~ s/^\\d+/$c->{dim}$&$c->{reset}/mg;\n> \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n> \tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n> @@ -805,9 +808,9 @@ sub check_script {\n> \t\t\tmy $c = fd_colors(1);\n> \t\t\tmy $s = join('', @{$parser->{output}});\n> \t\t\t$emit->(\"$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\\n\" . $s);\n> -\t\t\t$nerrs += () = $s =~ /\\?![^?]+\\?!/g;\n> \t\t}\n> \t\t$ntests += $parser->{ntests};\n> +\t\t$nerrs += $parser->{nerrs};\n> \t}\n> \treturn [$id, $nscripts, $ntests, $nerrs];\n> }\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 54247604cb..b652cb98cd 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1606,7 +1606,7 @@ if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0 &&\n> test \"${GIT_TEST_EXT_CHAIN_LINT:-1}\" != 0\n> then\n> \t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n> -\t\tBUG \"lint error (see '?!...!? annotations above)\"\n> +\t\tBUG \"lint error (see 'ERR' annotations above)\"\n> fi\n> \n> # Last-minute variable setup\n\nOverall the two patches looked great.\nThanks.\n"},{"id":"501842","messageId":"20240829170712.GA405209@coredump.intra.peff.net","threadId":"62018","inReplyTo":"ZtBHbftK7vdTEz93@tanuki","subject":"Re: [PATCH 1/2] chainlint: make error messages self-explanatory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-29T17:07:12Z","receivedAt":"2024-08-29T17:07:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 29, 2024 at 12:03:33PM +0200, Patrick Steinhardt wrote:\n\n> > diff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect\n> > index 338ecd5861..2efd65dcbd 100644\n> > --- a/t/chainlint/arithmetic-expansion.expect\n> > +++ b/t/chainlint/arithmetic-expansion.expect\n> > @@ -4,6 +4,6 @@\n> > 5 \tbaz\n> > 6 ) &&\n> > 7 (\n> > -8 \tbar=$((42 + 1)) ?!AMP?!\n> > +8 \tbar=$((42 + 1)) ?!ERR missing '&&'?!\n> > 9 \tbaz\n> > 10 )\n> \n> I find the resulting error messages a bit confusing: to me it reads as\n> if \"ERR\" is missing the ampersands. Is it actually useful to have the\n> ERR prefix in the first place? We do not output anything but errors, so\n> it feels somewhat redundant.\n\nI wonder if coloring \"ERR\" differently, or perhaps even adding a colon,\nlike \"ERR: \", would make it stand out more.\n\nFWIW, I find the existing error messages pretty readable, but that is\nprobably a sign that my mind has been poisoned by using chainlint too\nmuch already. ;)\n\n-Peff\n"},{"id":"501843","messageId":"20240829171001.GB405209@coredump.intra.peff.net","threadId":"62018","inReplyTo":"ZtBHecRkFQkSAF6C@tanuki","subject":"Re: [PATCH 2/2] chainlint: reduce annotation noise-factor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-29T17:10:01Z","receivedAt":"2024-08-29T17:10:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 29, 2024 at 12:03:37PM +0200, Patrick Steinhardt wrote:\n\n> Okay, now the \"ERR\" prefix becomes a bit more important because we drop\n> the other punctuation. I'm still not much of a fan of it, though. Makes\n> me wonder whether we want to take a clue from how compilers nowadays\n> format this, e.g. by using \"pointers\".\n> \n> So this:\n> \n> 2 (\n> 3 \tfoo |\n> 4 \tbar |\n> 5 \tbaz &&\n> 6 \n> 7 \tfish |\n> 8 \tcow ?!AMP?!\n> 9 \n> 10 \tsunder\n> 11 )\n> \n> Would become this:\n> \n> t/chainlint/pipe.actual:8: error: expected ampersands (&&)\n> 7 \tfish |\n> 8 \tcow \n> ^\n> 9\n> \n> While this would be neat, I guess it would also be way more work than\n> the current series you have posted. And whether that work is ultimately\n> really worth it may be another question. Probably not.\n\nI think that output is quite readable. One bonus is that it follows the\nusual \"quickfix\" format, so there's editor support for jumping to the\nproblematic spot.\n\nIt probably is more verbose if you have multiple errors right next to\neach other (since now we just show the annotated source text). But that\nis going to be relatively rare compared to single mistakes, I'd think.\n\n-Peff\n"},{"id":"501846","messageId":"CAPig+cRnEkS2CbAtao8vGki1tsMGmJ992eDn3rnrtPZYnMvk8A@mail.gmail.com","threadId":"62018","inReplyTo":"ZtBHbftK7vdTEz93@tanuki","subject":"Re: [PATCH 1/2] chainlint: make error messages self-explanatory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-29T18:01:37Z","receivedAt":"2024-08-29T18:01:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 29, 2024 at 6:03 AM Patrick Steinhardt wrote:\n> On Thu, Aug 29, 2024 at 05:16:24AM -0400, Eric Sunshine wrote:\n> > - push(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]);\n> > + push(@{$self->{parser}->{problems}}, ['HEREDOC', $tag]);\n> > $$b =~ /(?:\\G|\\n).*\\z/gc; # consume rest of input\n> > my $body = substr($$b, $start, pos($$b) - $start);\n> > $self->{lineno} += () = $body =~ /\\n/sg;\n>\n> I was wondering why this is being changed here, as I found the old name\n> to be easier to understand. Then I saw further down that you essentially\n> use those as identifiers for the actual problem.\n\nPeff chose[1] the longer \"UNCLOSED-HEREDOC\" over the (perhaps too)\nterse \"HERE\" I had chosen[2], however, now that this is an internal\ndetail of the script -- not part of the user-facing output -- such\nverbosity is unneeded. As programmers, just as we choose shorter\nvariable names (say, \"i\" instead of \"record_index\" in a for-loop), I\nfind \"HEREDOC\" easier to read in a code context than the longer\n\"UNCLOSED-HEREDOC\", hence this (admittedly unnecessary) change.\n\n[1]: https://lore.kernel.org/git/20230330193031.GC27989@coredump.intra.peff.net/\n[2]: https://lore.kernel.org/git/CAPig+cQiOGrDSUc34jHEBp87Rx-dnXNcPcF76bu0SJoOzD+1hw@mail.gmail.com/\n\n> Is there a specific reason why we now have the separate translation\n> step? Couldn't we instead push the translated message here, directly?\n\nI considered that but, although this instance is a simple \"push\"\noperation, some heuristics scan and modify the `problems` array by\nlooking for and removing specific items. There are numerous instances\nin (older) scripts similar to this:\n\n if condition not satisified\n then\n echo it did not work...\n echo failed!\n return 1\n fi\n\nwhich prints an error message and then explicitly signals failure with\n`return 1` (or `exit 1` or `false`) as the final command in an `if`\nbranch or `case` arm. In these cases, the tests don't bother\nmaintaining the &&-chain between `echo` and the explicit \"test failed\"\nindicator.\n\nAs chainlint processes the token stream, it correctly pushes \"AMP\"\nannotations onto the `problems` array for each of the `echo` lines,\nbut when it encounters the explicit `return 1`, the heuristic kicks in\nand notices that the broken &&-chain leading up to `return 1` is\nimmaterial since the construct is manually signaling failure, thus the\n&&-chain breakage is legitimate and safe. Requiring test authors to\nadd \"&&\" to each such line would just be making busy-work for them.\nHence, the heuristic actively removes the preceding \"AMP\" annotations\nfrom `problems`. For the removal, it's easier to search `problems` for\na simple token such as \"AMP\" than to search for a user-facing message\nsuch as \"ERR missing '?!'\".\n\n> > -8 bar=$((42 + 1)) ?!AMP?!\n> > +8 bar=$((42 + 1)) ?!ERR missing '&&'?!\n>\n> I find the resulting error messages a bit confusing: to me it reads as\n> if \"ERR\" is missing the ampersands. Is it actually useful to have the\n> ERR prefix in the first place? We do not output anything but errors, so\n> it feels somewhat redundant.\n\nAs you mentioned in your review of [2/2], the \"ERR\" prefix serves as a\nuseful target for searches in a terminal.\n\nRegarding possible confusion, my first draft placed a colon after the\nprefix, i.e.:\n\n ERR: missing '&&'\n\nbut it seemed unnecessarily noisy, so I dropped the colon since:\n\n ERR missing '&&'\n\nseemed clear enough. However, I don't feel too strongly about it and\ncan add the colon back if people think it would make the message\nclearer.\n"},{"id":"501848","messageId":"CAPig+cSWoQ8pkdy3gyRQFUwoQ5ZytK9HjobHd3EJxdFRJhDWxQ@mail.gmail.com","threadId":"62018","inReplyTo":"20240829170712.GA405209@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] chainlint: make error messages self-explanatory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-29T18:10:36Z","receivedAt":"2024-08-29T18:10:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 29, 2024 at 1:07 PM Jeff King wrote:\n> On Thu, Aug 29, 2024 at 12:03:33PM +0200, Patrick Steinhardt wrote:\n> > I find the resulting error messages a bit confusing: to me it reads as\n> > if \"ERR\" is missing the ampersands. Is it actually useful to have the\n> > ERR prefix in the first place? We do not output anything but errors, so\n> > it feels somewhat redundant.\n>\n> I wonder if coloring \"ERR\" differently, or perhaps even adding a colon,\n> like \"ERR: \", would make it stand out more.\n\nI considered both of these ideas. Coloring \"ERR\" was dismissed almost\nimmediately due to the (admittedly tiny) bit of extra complexity and\nthe minimal color palette available (and since I couldn't trust myself\nto not waste an inordinate amount of time trying to arrive at the\nperfect color combination).\n\nMy first draft did place a colon after \"ERR\", but it seemed\nunnecessarily noisy, so I dropped it. However, I don't feel overly\nstrongly about it and can add it back if people think it would be\nhelpful.\n\n> FWIW, I find the existing error messages pretty readable, but that is\n> probably a sign that my mind has been poisoned by using chainlint too\n> much already. ;)\n\nGoal achieved.\n"},{"id":"501851","messageId":"CAPig+cTTXmaAjJrOOSKKDKvMAE+yD9wfoYii5C21jGpq=sqtyA@mail.gmail.com","threadId":"62018","inReplyTo":"ZtBHecRkFQkSAF6C@tanuki","subject":"Re: [PATCH 2/2] chainlint: reduce annotation noise-factor","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-29T18:28:17Z","receivedAt":"2024-08-29T18:28:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 29, 2024 at 6:03 AM Patrick Steinhardt wrote:\n> On Thu, Aug 29, 2024 at 05:16:25AM -0400, Eric Sunshine wrote:\n> > Note that the preceding change gave all problem annotations a uniform\n> > \"ERR\" prefix which serves as a reasonably suitable replacement needle\n> > when searching in a terminal, so loss of \"?!\" in the output should not\n> > be overly problematic.\n>\n> Okay, now the \"ERR\" prefix becomes a bit more important because we drop\n> the other punctuation. I'm still not much of a fan of it, though. Makes\n> me wonder whether we want to take a clue from how compilers nowadays\n> format this, e.g. by using \"pointers\".\n>\n> So this:\n> 7 fish |\n> 8 cow ?!AMP?!\n>\n> Would become this:\n> t/chainlint/pipe.actual:8: error: expected ampersands (&&)\n> 7 fish |\n> 8 cow\n> ^\n>\n> While this would be neat, I guess it would also be way more work than\n> the current series you have posted. And whether that work is ultimately\n> really worth it may be another question. Probably not.\n\nInterestingly, I'm not always a fan of the sort of compiler output you\nsuggest since I often have more difficulty interpreting the output and\nlocating the actual problem[*] than if the annotation was merely\ninline, sitting immediately next to the problem itself.\n\nAlso, the vast majority of the time, chainlint will be flagging a\nmissing \"&&\" at the end of line, so with the inline annotation, it's\nvery easy to see (especially when colored) exactly where the problem\nis at a glance.\n\nHence, the cost of implementing \"^\" doesn't feel particularly\nworthwhile (and, with my limited Git time these days, I'm unlikely to\ndo so).\n\n[*] This is especially so when dealing with foreign code which is\nwider than my 80-column terminal or 80-column editor window, in which\nthe source text and the \"^\" may wrap over multiple lines.\n"},{"id":"501852","messageId":"CAPig+cRLeQSP6ybfVwo889kMNi8yvRvwLpHNjn-SpU0qWbe=Kw@mail.gmail.com","threadId":"62018","inReplyTo":"20240829171001.GB405209@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] chainlint: reduce annotation noise-factor","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-29T18:37:57Z","receivedAt":"2024-08-29T18:38:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 29, 2024 at 1:10 PM Jeff King wrote:\n> On Thu, Aug 29, 2024 at 12:03:37PM +0200, Patrick Steinhardt wrote:\n> > Would become this:\n> >\n> > t/chainlint/pipe.actual:8: error: expected ampersands (&&)\n> > 7 fish |\n> > 8 cow\n> > ^\n> > 9\n> >\n> > While this would be neat, I guess it would also be way more work than\n> > the current series you have posted. And whether that work is ultimately\n> > really worth it may be another question. Probably not.\n>\n> I think that output is quite readable. One bonus is that it follows the\n> usual \"quickfix\" format, so there's editor support for jumping to the\n> problematic spot.\n\nThe \"quickfix\" notation has more appeal (to me) than the \"^\"\nannotation since it is immediately useful in an editor and doesn't\nrequire extra cogitation. A couple concerns which come to mind are\nthat it makes the output even more noisy (which could be distracting),\nand that the path, if relative, might not correspond to the editor's\n\"cwd\" or to a path in the editor's search list, so the promised \"jump\nto next problem\" automation may not materialize.\n\n> It probably is more verbose if you have multiple errors right next to\n> each other (since now we just show the annotated source text). But that\n> is going to be relatively rare compared to single mistakes, I'd think.\n\nMaybe. Maybe not. A first-draft test by a newcomer might very well\nbreak the &&-chain in many spots.\n\nNevertheless, I'm not likely to implement this any time soon (or at all).\n"},{"id":"501878","messageId":"CAPig+cQ+6am7-BSnWZz5=C0Q1Vyng0T4goB+ZE9TKJMrpi_Jpg@mail.gmail.com","threadId":"62018","inReplyTo":"xmqq7cbzxrry.fsf@gitster.g","subject":"Re: [PATCH 1/2] chainlint: make error messages self-explanatory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-29T22:04:43Z","receivedAt":"2024-08-29T22:04:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 29, 2024 at 11:39 AM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > \"?!LOOP?!\" case is particularly serious since it is likely that some\n> > newcomers are unaware that shell loops do not terminate automatically\n> > upon error, and it is more difficult for a newcomer to figure out how to\n> > correct the problem by examining surrounding code since `|| return 1`\n> > appears in test scrips relatively infrequently (compared, for instance,\n> > with &&-chaining).\n>\n> I'd prefer to see \"some newcomes are unaware that\" part rewritten\n> and toned down, as it is not our primary business to help total\n> newbies to learn shells, it certainly is not what the chain lint\n> checker should bend over backwards to do.\n>\n> ... particularly serious, as it does not convey that returning\n> control with \"|| return 1\" (or \"|| exit 1\" from a subshell)\n> immediately after we detect an error is the canonical way we\n> chose in this project to handle errors in a loop. Because it\n> happens relatively infrequently, this norm is harder to figure\n> out for a new person on their own than other patterns (like\n> &&-chaining).\n\nHow about this?\n\n The \"?!LOOP?!\" case is particularly serious because that terse\n single word does nothing to convey that the loop body should end\n with \"|| return 1\" (or \"|| exit 1\" in a subshell) to ensure that a\n failing command in the body aborts the loop immediately, which is\n important since a shell loop does not automatically terminate when\n an error occurs within its body. Moreover, unlike &&-chaining\n which is ubiquitous in Git tests, the \"|| return 1\" idiom is\n relatively infrequent, thus may be harder for a newcomer to\n discover by consulting nearby code.\n\n> > -# name and the test body with a `?!FOO?!` annotation at the location of each\n> > +# name and the test body with a `?!ERR?!` annotation at the location of each\n> > # detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n>\n> \"FOO\" -> \"ERR\"?\n\nYep. Sharp eyes.\n"},{"id":"501930","messageId":"xmqqv7zhrgzf.fsf@gitster.g","threadId":"62018","inReplyTo":"CAPig+cQ+6am7-BSnWZz5=C0Q1Vyng0T4goB+ZE9TKJMrpi_Jpg@mail.gmail.com","subject":"Re: [PATCH 1/2] chainlint: make error messages self-explanatory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-30T18:41:08Z","receivedAt":"2024-08-30T18:41:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> How about this?\n>\n> The \"?!LOOP?!\" case is particularly serious because that terse\n> single word does nothing to convey that the loop body should end\n> with \"|| return 1\" (or \"|| exit 1\" in a subshell) to ensure that a\n> failing command in the body aborts the loop immediately, which is\n> important since a shell loop does not automatically terminate when\n> an error occurs within its body. Moreover, unlike &&-chaining\n> which is ubiquitous in Git tests, the \"|| return 1\" idiom is\n> relatively infrequent, thus may be harder for a newcomer to\n> discover by consulting nearby code.\n\nStrike \", which is important since .*\\ its body.\" and the above\nreads perfect.\n\n>> > -# name and the test body with a `?!FOO?!` annotation at the location of each\n>> > +# name and the test body with a `?!ERR?!` annotation at the location of each\n>> > # detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n>>\n>> \"FOO\" -> \"ERR\"?\n>\n> Yep. Sharp eyes.\n\nOK. I'll mark the topic to be expecting a reroll for these small\nmessaging plus \"ERR\" -> \"ERR:\" but without other larger changes\nmentioned in the thread.\n\nThanks.\n"},{"id":"501946","messageId":"CAPig+cSZ8Sot9oq+rmzBTmQU-Fnay92roTO=Mk0uT+-JUzMcXw@mail.gmail.com","threadId":"62018","inReplyTo":"xmqqv7zjwcgq.fsf@gitster.g","subject":"Re: [PATCH 2/2] chainlint: reduce annotation noise-factor","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-30T23:30:09Z","receivedAt":"2024-08-30T23:30:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 29, 2024 at 11:55 AM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > When chainlint detects a problem in a test definition, it highlights the\n> > offending code with an \"?!ERR ...?!\" annotation. The rather curious \"?!\"\n> > delimiter was chosen to draw the reader's attention to the problem area.\n> >\n> > Later, chainlint learned to color its output when sent to a terminal.\n> > Problem annotations are colored with a red background which stands out\n> > well from surrounding text, thus easily draws the reader's attention. As\n> > such, the additional \"?!\" decoration became superfluous (when output is\n> > colored), however the decoration was retained since it serves as a good\n> > needle when using the terminal's search feature to \"jump\" to the next\n> > problem.\n> >\n> > Nevertheless, the \"?!\" decoration is noisy and ugly and makes it\n> > unnecessarily difficult for the reader to pluck the problem description\n> > from the annotation. For instance, it is easier to see at a glance what\n> > the problem is in:\n> > ERR missing '&&'\n> > than in the noisier:\n> > ?!ERR missing '&&'?!\n> > Therefore drop the \"!?\" decoration when output is colored (but retain it\n> > otherwise).\n>\n> Wait. That does not qualify \"Therefore\".\n>\n> We talked about a \"good needle\" and then complained how ugly the\n> string that was happened to be chosen as good needle is. That is\n> not enough to explain why it is justified to \"lose\" the needle. The\n> only thing you justified is to move away from the ugly pattern, as a\n> typical \"terminal's search feature\" does not give us an easy way to\n> \"jump to the next text painted yellow\".\n>\n> > Note that the preceding change gave all problem annotations a uniform\n> > \"ERR\" prefix which serves as a reasonably suitable replacement needle\n> > when searching in a terminal, so loss of \"?!\" in the output should not\n> > be overly problematic.\n>\n> Drop this separate paragraph, promote its contents up from \"Note\"\n> status and as a proper part of the previous sentence in its rewrite,\n> something like:\n>\n> Since the errors are all uniformly prefixed with \"ERR\", which\n> can be used as the \"good needle\" instead, lose the \"!?\"\n> decoration when output is colored.\n>\n> to replace \"Therefore\" and everything that follow.\n\nPerhaps the following would make for a more palatable commit message?\n\n When chainlint detects a problem in a test definition, it\n highlights the offending code with a \"?!...?!\" annotation. The\n rather curious \"?!\" decoration was chosen to draw the reader's\n attention to the problem area and to act as a good \"needle\" when\n using the terminal's search feature to \"jump\" to the next problem.\n\n Later, chainlint learned to color its output when sent to a\n terminal. Problem annotations are colored with a red background\n which stands out well from surrounding text, thus easily draws the\n reader's attention. Together with the preceding change which gave\n all problem annotations a uniform \"ERR\" prefix, the noisy \"?!\"\n decoration has become superfluous as a search \"needle\" so omit it\n when output is colored.\n\n> > @@ -663,7 +666,7 @@ sub check_test {\n> > $checked =~ s/(\\s) \\?!/$1?!/mg;\n> > $checked =~ s/\\?! (\\s)/?!$1/mg;\n> > - $checked =~ s/(\\?![^?]+\\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;\n> > + $checked =~ s/\\?!([^?]+)\\?!/$erropen$1$errclose/mg;\n>\n> Hmph. With $erropen and $errclose, I was hoping that we can shed\n> the reliance on the \"?!\" mark even internally.\n\nGood point. Just above the shown context:\n\n $checked .= substr($body, $start, $pos - $start) . \" ?!ERR $err?! \";\n\nunconditionally adds the \"?!\" decorations even when the output is\ncolored, and then the:\n\n $checked =~ s/\\?!([^?]+)\\?!/$erropen$1$errclose/mg;\n\nremoves them. It would be nice to add the \"?!\" decoration only when\nnot coloring output, however, together with the explicit space added\nbefore and after \"?!...?!\" by:\n\n $checked .= substr($body, $start, $pos - $start) . \" ?!ERR $err?! \";\n\nthe related:\n\n $checked =~ s/(\\s) \\?!/$1?!/mg;\n $checked =~ s/\\?! (\\s)/?!$1/mg;\n\nensure, for aesthetic reasons, that there is one, and only one, space\nbefore and after the annotation.\n\nIt may be possible to do something like this instead (untested), but\nI'm not sure it's worth the complexity:\n\n $checked .= substr($body, $start, $pos - $start);\n $checked .= ' ' unless $checked =~ /\\s$/;\n $checked .= \"$erropenERR $err$errclose\";\n $checked .= ' ' unless $pos + 1 >= length($body) ||\n substr($body, $pos + 1, 1) =~ /\\s/;\n\n> This is especially\n> true that in the early part of this sub, the problem description was\n> very much structured piece of data, not something the consuming code\n> need to pick out of an already formatted text like this, risking to\n> get confused by the payload (i.e. the text that came from the\n> problematic test script inside \"substr($body, $start, $pos-$start)\"\n> may contain anything, including \"?!\", right?).\n\nAs first implemented, there was no structured \"problem description\".\nchainlint originally just output a stream of raw parse tokens (not the\noriginal test text), and when a problem was discovered the \"?!...?!\"\nannotations were embedded directly in the output stream. This was\nstill the case even when colored output was implemented[1]; in fact,\nthe annotations were colored after-the-fact by searching for \"?!...?!\"\nin the output stream. It was only when chainlint was taught to output\nthe original test text verbatim[2] that problem descriptions became\nstructured data.\n\nI was never overly concerned about \"?!\" appearing as part of the\nactual payload, partly because it is an unusual character sequence to\nbe present in shell code anyhow (indeed, it only appears in t5510),\npartly because it requires \"?!\" to be doubled up (i.e. \"?!...?!\"), and\npartly because this processing kicks in only when a linting problem is\nactually discovered,\n\n[1]: 7c04aa7390 (chainlint: colorize problem annotations and test\ndelimiters, 2022-09-13)\n[2]: 73c768dae9 (chainlint: annotate original test definition rather\nthan token stream, 2022-11-08)\n"},{"id":"501947","messageId":"xmqqle0dpo1p.fsf@gitster.g","threadId":"62018","inReplyTo":"CAPig+cSZ8Sot9oq+rmzBTmQU-Fnay92roTO=Mk0uT+-JUzMcXw@mail.gmail.com","subject":"Re: [PATCH 2/2] chainlint: reduce annotation noise-factor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-30T23:51:30Z","receivedAt":"2024-08-30T23:51:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> It may be possible to do something like this instead (untested), but\n> I'm not sure it's worth the complexity:\n>\n> $checked .= substr($body, $start, $pos - $start);\n> $checked .= ' ' unless $checked =~ /\\s$/;\n> $checked .= \"$erropenERR $err$errclose\";\n> $checked .= ' ' unless $pos + 1 >= length($body) ||\n> substr($body, $pos + 1, 1) =~ /\\s/;\n\nI think the complexity you mention is the updates to existing code\nto get to the above end state? Using some setup like ...\n\n\t($erropen, errclose) = \n\t\t$colored_output ? (\"?!\", \"?!\") : (\"\", \"\");\n\n... and then using a code like the above would be quite\nstraightforward and the end result cannot become simpler than that\n;-)\n\n> As first implemented, there was no structured \"problem description\".\n> chainlint originally just output a stream of raw parse tokens (not the\n> original test text), and when a problem was discovered the \"?!...?!\"\n> annotations were embedded directly in the output stream. This was\n> still the case even when colored output was implemented[1]; in fact,\n> the annotations were colored after-the-fact by searching for \"?!...?!\"\n> in the output stream. It was only when chainlint was taught to output\n> the original test text verbatim[2] that problem descriptions became\n> structured data.\n\nExactly.\n\nThanks.\n"},{"id":"502526","messageId":"20240910041013.68948-2-ericsunshine@charter.net","threadId":"62018","inReplyTo":"20240910041013.68948-1-ericsunshine@charter.net","subject":"[PATCH v2 1/3] chainlint: don't be fooled by \"?!...?!\" in test body","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2024-09-10T04:10:11Z","receivedAt":"2024-09-10T04:12:10Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nAs originally implemented, chainlint did not collect structured\ninformation about detected problems. Instead, it merely emitted raw\nparse tokens (not the original test text), along with a \"?!...?!\"\nannotation directly into the output stream each time a problem was\ndiscovered. In order to report statistics (in --stats mode) and to\nadjust its exit code to indicate success or failure, it merely counts\nthe number of times \"?!...?!\" appears in the output stream. An obvious\nshortcoming of this approach is that it can be fooled by a legitimate\n\"?!...?!\" sequence in the body of a test (though, only if an actual\nproblem is detected in the test).\n\nThe situation did not improve when 7c04aa7390 (chainlint: colorize\nproblem annotations and test delimiters, 2022-09-13) colored the\nannotations after-the-fact by searching for \"?!...?!\" in the output\nstream and inserting color codes. As above, a shortcoming is that this\napproach can incorrectly color a legitimate \"?!...?!\" sequence in a test\nbody as if it is an error.\n\nHowever, when 73c768dae9 (chainlint: annotate original test definition\nrather than token stream, 2022-11-08) taught chainlint to output the\noriginal test text verbatim, it started collecting structured\ninformation about detected problems.\n\nNow that it is available, take advantage of the structured problem\ninformation to deterministically count the number of problems detected\nand to color the annotations directly, rather than scanning the output\nstream for \"?!...?!\" and performing these operations after-the-fact.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 5361f23b1d..1a7611ad43 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -587,6 +587,7 @@ sub new {\n \tmy $class = shift @_;\n \tmy $self = $class->SUPER::new(@_);\n \t$self->{ntests} = 0;\n+\t$self->{nerrs} = 0;\n \treturn $self;\n }\n \n@@ -634,6 +635,7 @@ sub check_test {\n \tmy $parser = TestParser->new(\\$body);\n \tmy @tokens = $parser->parse();\n \tmy $problems = $parser->{problems};\n+\t$self->{nerrs} += @$problems;\n \treturn unless $emit_all || @$problems;\n \tmy $c = main::fd_colors(1);\n \tmy $start = 0;\n@@ -641,15 +643,16 @@ sub check_test {\n \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n \t\tmy ($label, $token) = @$_;\n \t\tmy $pos = $token->[2];\n-\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!$label?! \";\n+\t\t$checked .= substr($body, $start, $pos - $start);\n+\t\t$checked .= ' ' unless $checked =~ /\\s$/;\n+\t\t$checked .= \"$c->{rev}$c->{red}?!$label?!$c->{reset}\";\n+\t\t$checked .= ' ' unless $pos >= length($body) ||\n+\t\t substr($body, $pos, 1) =~ /^\\s/;\n \t\t$start = $pos;\n \t}\n \t$checked .= substr($body, $start);\n \t$checked =~ s/^/$lineno++ . ' '/mge;\n \t$checked =~ s/^\\d+ \\n//;\n-\t$checked =~ s/(\\s) \\?!/$1?!/mg;\n-\t$checked =~ s/\\?! (\\s)/?!$1/mg;\n-\t$checked =~ s/(\\?![^?]+\\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;\n \t$checked =~ s/^\\d+/$c->{dim}$&$c->{reset}/mg;\n \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n \tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n@@ -791,9 +794,9 @@ sub check_script {\n \t\t\tmy $c = fd_colors(1);\n \t\t\tmy $s = join('', @{$parser->{output}});\n \t\t\t$emit->(\"$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\\n\" . $s);\n-\t\t\t$nerrs += () = $s =~ /\\?![^?]+\\?!/g;\n \t\t}\n \t\t$ntests += $parser->{ntests};\n+\t\t$nerrs += $parser->{nerrs};\n \t}\n \treturn [$id, $nscripts, $ntests, $nerrs];\n }\n-- \n2.46.0\n\n"},{"id":"502527","messageId":"20240910041013.68948-3-ericsunshine@charter.net","threadId":"62018","inReplyTo":"20240910041013.68948-1-ericsunshine@charter.net","subject":"[PATCH v2 2/3] chainlint: make error messages self-explanatory","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2024-09-10T04:10:12Z","receivedAt":"2024-09-10T04:12:10Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nThe annotations emitted by chainlint to indicate detected problems are\noverly terse, so much so that developers new to the project -- those who\nshould most benefit from the linting -- may find them baffling. For\ninstance, although the author of chainlint and seasoned Git developers\nmay understand that \"?!AMP?!\" is an abbreviation of \"ampersand\" and\nindicates a break in the &&-chain, this may not be obvious to newcomers.\n\nThe \"?!LOOP?!\" case is particularly serious because that terse single\nword does nothing to convey that the loop body should end with\n\"|| return 1\" (or \"|| exit 1\" in a subshell) to ensure that a failing\ncommand in the body aborts the loop immediately. Moreover, unlike\n&&-chaining which is ubiquitous in Git tests, the \"|| return 1\" idiom is\nrelatively infrequent, thus may be harder for a newcomer to discover by\nconsulting nearby code.\n\nAddress these shortcomings by emitting human-readable messages which\nboth explain the problem and give a strong hint about how to correct it.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 30 ++++++++++++++-----\n t/chainlint/arithmetic-expansion.expect | 2 +-\n t/chainlint/block.expect | 8 ++---\n t/chainlint/broken-chain.expect | 2 +-\n t/chainlint/case.expect | 4 +--\n t/chainlint/chain-break-false.expect | 2 +-\n t/chainlint/chained-block.expect | 2 +-\n t/chainlint/chained-subshell.expect | 4 +--\n t/chainlint/command-substitution.expect | 2 +-\n t/chainlint/complex-if-in-cuddled-loop.expect | 2 +-\n t/chainlint/cuddled.expect | 4 +--\n t/chainlint/for-loop.expect | 8 ++---\n t/chainlint/function.expect | 4 +--\n t/chainlint/here-doc-body-indent.expect | 2 +-\n t/chainlint/here-doc-body-pathological.expect | 4 +--\n t/chainlint/here-doc-body.expect | 4 +--\n t/chainlint/here-doc-double.expect | 2 +-\n t/chainlint/here-doc-indent-operator.expect | 2 +-\n .../here-doc-multi-line-command-subst.expect | 2 +-\n t/chainlint/here-doc-multi-line-string.expect | 2 +-\n t/chainlint/if-condition-split.expect | 2 +-\n t/chainlint/if-in-loop.expect | 4 +--\n t/chainlint/if-then-else.expect | 4 +--\n t/chainlint/inline-comment.expect | 2 +-\n t/chainlint/loop-detect-failure.expect | 2 +-\n t/chainlint/loop-in-if.expect | 8 ++---\n t/chainlint/multi-line-string.expect | 2 +-\n t/chainlint/negated-one-liner.expect | 4 +--\n t/chainlint/nested-cuddled-subshell.expect | 6 ++--\n t/chainlint/nested-here-doc.expect | 2 +-\n t/chainlint/nested-loop-detect-failure.expect | 6 ++--\n t/chainlint/nested-subshell-comment.expect | 2 +-\n t/chainlint/nested-subshell.expect | 2 +-\n t/chainlint/not-heredoc.expect | 2 +-\n t/chainlint/one-liner-for-loop.expect | 2 +-\n t/chainlint/one-liner.expect | 6 ++--\n t/chainlint/pipe.expect | 2 +-\n t/chainlint/semicolon.expect | 12 ++++----\n t/chainlint/subshell-here-doc.expect | 2 +-\n t/chainlint/subshell-one-liner.expect | 10 +++----\n t/chainlint/token-pasting.expect | 8 ++---\n t/chainlint/unclosed-here-doc-indent.expect | 2 +-\n t/chainlint/unclosed-here-doc.expect | 2 +-\n t/chainlint/while-loop.expect | 8 ++---\n 44 files changed, 104 insertions(+), 90 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 1a7611ad43..ad26499478 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -9,9 +9,9 @@\n # Input arguments are pathnames of shell scripts containing test definitions,\n # or globs referencing a collection of scripts. For each problem discovered,\n # the pathname of the script containing the test is printed along with the test\n-# name and the test body with a `?!FOO?!` annotation at the location of each\n-# detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n-# &&-chain. Returns zero if no problems are discovered, otherwise non-zero.\n+# name and the test body with a `?!LINT: ...?!` annotation at the location of\n+# each detected problem, where \"...\" is an explanation of the problem. Returns\n+# zero if no problems are discovered, otherwise non-zero.\n \n use warnings;\n use strict;\n@@ -181,7 +181,7 @@ sub swallow_heredocs {\n \t\t\t$self->{lineno} += () = $body =~ /\\n/sg;\n \t\t\tnext;\n \t\t}\n-\t\tpush(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]);\n+\t\tpush(@{$self->{parser}->{problems}}, ['HEREDOC', $tag]);\n \t\t$$b =~ /(?:\\G|\\n).*\\z/gc; # consume rest of input\n \t\tmy $body = substr($$b, $start, pos($$b) - $start);\n \t\t$self->{lineno} += () = $body =~ /\\n/sg;\n@@ -238,6 +238,7 @@ sub new {\n \t\tstop => [],\n \t\toutput => [],\n \t\theredocs => {},\n+\t\tinsubshell => 0,\n \t} => $class;\n \t$self->{lexer} = Lexer->new($self, $s);\n \treturn $self;\n@@ -296,8 +297,11 @@ sub parse_group {\n \n sub parse_subshell {\n \tmy $self = shift @_;\n-\treturn ($self->parse(qr/^\\)$/),\n-\t\t$self->expect(')'));\n+\t$self->{insubshell}++;\n+\tmy @tokens = ($self->parse(qr/^\\)$/),\n+\t\t $self->expect(')'));\n+\t$self->{insubshell}--;\n+\treturn @tokens;\n }\n \n sub parse_case_pattern {\n@@ -528,7 +532,7 @@ sub parse_loop_body {\n \treturn @tokens if ends_with(\\@tokens, [qr/^\\|\\|$/, \"\\n\", qr/^echo$/, qr/^.+$/]);\n \t# flag missing \"return/exit\" handling explicit failure in loop body\n \tmy $n = find_non_nl(\\@tokens);\n-\tpush(@{$self->{problems}}, ['LOOP', $tokens[$n]]);\n+\tpush(@{$self->{problems}}, [$self->{insubshell} ? 'LOOPEXIT' : 'LOOPRETURN', $tokens[$n]]);\n \treturn @tokens;\n }\n \n@@ -620,6 +624,15 @@ sub unwrap {\n \treturn $s\n }\n \n+sub format_problem {\n+\tlocal $_ = shift;\n+\t/^AMP$/ && return \"missing '&&'\";\n+\t/^LOOPRETURN$/ && return \"missing '|| return 1'\";\n+\t/^LOOPEXIT$/ && return \"missing '|| exit 1'\";\n+\t/^HEREDOC$/ && return 'unclosed heredoc';\n+\tdie(\"unrecognized problem type '$_'\\n\");\n+}\n+\n sub check_test {\n \tmy $self = shift @_;\n \tmy $title = unwrap(shift @_);\n@@ -643,9 +656,10 @@ sub check_test {\n \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n \t\tmy ($label, $token) = @$_;\n \t\tmy $pos = $token->[2];\n+\t\tmy $err = format_problem($label);\n \t\t$checked .= substr($body, $start, $pos - $start);\n \t\t$checked .= ' ' unless $checked =~ /\\s$/;\n-\t\t$checked .= \"$c->{rev}$c->{red}?!$label?!$c->{reset}\";\n+\t\t$checked .= \"$c->{rev}$c->{red}?!LINT: $err?!$c->{reset}\";\n \t\t$checked .= ' ' unless $pos >= length($body) ||\n \t\t substr($body, $pos, 1) =~ /^\\s/;\n \t\t$start = $pos;\ndiff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect\nindex 338ecd5861..5677e16cad 100644\n--- a/t/chainlint/arithmetic-expansion.expect\n+++ b/t/chainlint/arithmetic-expansion.expect\n@@ -4,6 +4,6 @@\n 5 \tbaz\n 6 ) &&\n 7 (\n-8 \tbar=$((42 + 1)) ?!AMP?!\n+8 \tbar=$((42 + 1)) ?!LINT: missing '&&'?!\n 9 \tbaz\n 10 )\ndiff --git a/t/chainlint/block.expect b/t/chainlint/block.expect\nindex b62e3d58c3..3d3f854c0d 100644\n--- a/t/chainlint/block.expect\n+++ b/t/chainlint/block.expect\n@@ -1,20 +1,20 @@\n 2 (\n 3 \tfoo &&\n 4 \t{\n-5 \t\techo a ?!AMP?!\n+5 \t\techo a ?!LINT: missing '&&'?!\n 6 \t\techo b\n 7 \t} &&\n 8 \tbar &&\n 9 \t{\n 10 \t\techo c\n-11 \t} ?!AMP?!\n+11 \t} ?!LINT: missing '&&'?!\n 12 \tbaz\n 13 ) &&\n 14 \n 15 {\n-16 \techo a; ?!AMP?! echo b\n+16 \techo a; ?!LINT: missing '&&'?! echo b\n 17 } &&\n-18 { echo a; ?!AMP?! echo b; } &&\n+18 { echo a; ?!LINT: missing '&&'?! echo b; } &&\n 19 \n 20 {\n 21 \techo \"${var}9\" &&\ndiff --git a/t/chainlint/broken-chain.expect b/t/chainlint/broken-chain.expect\nindex 9a1838736f..b7b1ce8509 100644\n--- a/t/chainlint/broken-chain.expect\n+++ b/t/chainlint/broken-chain.expect\n@@ -1,6 +1,6 @@\n 2 (\n 3 \tfoo &&\n-4 \tbar ?!AMP?!\n+4 \tbar ?!LINT: missing '&&'?!\n 5 \tbaz &&\n 6 \twop\n 7 )\ndiff --git a/t/chainlint/case.expect b/t/chainlint/case.expect\nindex c04c61ff36..0a3b09e470 100644\n--- a/t/chainlint/case.expect\n+++ b/t/chainlint/case.expect\n@@ -9,11 +9,11 @@\n 10 \tcase \"$x\" in\n 11 \tx) foo ;;\n 12 \t*) bar ;;\n-13 \tesac ?!AMP?!\n+13 \tesac ?!LINT: missing '&&'?!\n 14 \tfoobar\n 15 ) &&\n 16 (\n 17 \tcase \"$x\" in 1) true;; esac &&\n-18 \tcase \"$y\" in 2) false;; esac ?!AMP?!\n+18 \tcase \"$y\" in 2) false;; esac ?!LINT: missing '&&'?!\n 19 \tfoobar\n 20 )\ndiff --git a/t/chainlint/chain-break-false.expect b/t/chainlint/chain-break-false.expect\nindex 4f815f8e14..f6a0a301e9 100644\n--- a/t/chainlint/chain-break-false.expect\n+++ b/t/chainlint/chain-break-false.expect\n@@ -4,6 +4,6 @@\n 5 \techo failed!\n 6 \tfalse\n 7 else\n-8 \techo it went okay ?!AMP?!\n+8 \techo it went okay ?!LINT: missing '&&'?!\n 9 \tcongratulate user\n 10 fi\ndiff --git a/t/chainlint/chained-block.expect b/t/chainlint/chained-block.expect\nindex a546b714a6..f2501bba90 100644\n--- a/t/chainlint/chained-block.expect\n+++ b/t/chainlint/chained-block.expect\n@@ -1,5 +1,5 @@\n 2 echo nobody home && {\n-3 \ttest the doohicky ?!AMP?!\n+3 \ttest the doohicky ?!LINT: missing '&&'?!\n 4 \tright now\n 5 } &&\n 6 \ndiff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect\nindex f78b268291..93fb1a6578 100644\n--- a/t/chainlint/chained-subshell.expect\n+++ b/t/chainlint/chained-subshell.expect\n@@ -1,10 +1,10 @@\n 2 mkdir sub && (\n 3 \tcd sub &&\n-4 \tfoo the bar ?!AMP?!\n+4 \tfoo the bar ?!LINT: missing '&&'?!\n 5 \tnuff said\n 6 ) &&\n 7 \n 8 cut \"-d \" -f actual | (read s1 s2 s3 &&\n-9 test -f $s1 ?!AMP?!\n+9 test -f $s1 ?!LINT: missing '&&'?!\n 10 test $(cat $s2) = tree2path1 &&\n 11 test $(cat $s3) = tree3path1)\ndiff --git a/t/chainlint/command-substitution.expect b/t/chainlint/command-substitution.expect\nindex 5e31b36db6..73809fd585 100644\n--- a/t/chainlint/command-substitution.expect\n+++ b/t/chainlint/command-substitution.expect\n@@ -4,6 +4,6 @@\n 5 \tbaz\n 6 ) &&\n 7 (\n-8 \tbar=$(gobble blocks) ?!AMP?!\n+8 \tbar=$(gobble blocks) ?!LINT: missing '&&'?!\n 9 \tbaz\n 10 )\ndiff --git a/t/chainlint/complex-if-in-cuddled-loop.expect b/t/chainlint/complex-if-in-cuddled-loop.expect\nindex 3a740103db..e66bb2d5d0 100644\n--- a/t/chainlint/complex-if-in-cuddled-loop.expect\n+++ b/t/chainlint/complex-if-in-cuddled-loop.expect\n@@ -4,6 +4,6 @@\n 5 :\n 6 else\n 7 echo >file\n-8 fi ?!LOOP?!\n+8 fi ?!LINT: missing '|| exit 1'?!\n 9 done) &&\n 10 test ! -f file\ndiff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect\nindex b06d638311..1864b3fc8b 100644\n--- a/t/chainlint/cuddled.expect\n+++ b/t/chainlint/cuddled.expect\n@@ -2,7 +2,7 @@\n 3 \tbar\n 4 ) &&\n 5 \n-6 (cd foo ?!AMP?!\n+6 (cd foo ?!LINT: missing '&&'?!\n 7 \tbar\n 8 ) &&\n 9 \n@@ -13,5 +13,5 @@\n 14 (cd foo &&\n 15 \tbar) &&\n 16 \n-17 (cd foo ?!AMP?!\n+17 (cd foo ?!LINT: missing '&&'?!\n 18 \tbar)\ndiff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect\nindex 908aeedf96..5029eacce3 100644\n--- a/t/chainlint/for-loop.expect\n+++ b/t/chainlint/for-loop.expect\n@@ -1,14 +1,14 @@\n 2 (\n 3 \tfor i in a b c\n 4 \tdo\n-5 \t\techo $i ?!AMP?!\n-6 \t\tcat <<-\\EOF ?!LOOP?!\n+5 \t\techo $i ?!LINT: missing '&&'?!\n+6 \t\tcat <<-\\EOF ?!LINT: missing '|| exit 1'?!\n 7 \t\tbar\n 8 \t\tEOF\n-9 \tdone ?!AMP?!\n+9 \tdone ?!LINT: missing '&&'?!\n 10 \n 11 \tfor i in a b c; do\n 12 \t\techo $i &&\n-13 \t\tcat $i ?!LOOP?!\n+13 \t\tcat $i ?!LINT: missing '|| exit 1'?!\n 14 \tdone\n 15 )\ndiff --git a/t/chainlint/function.expect b/t/chainlint/function.expect\nindex c226246b25..9e46a3554a 100644\n--- a/t/chainlint/function.expect\n+++ b/t/chainlint/function.expect\n@@ -4,8 +4,8 @@\n 5 \n 6 remove_object() {\n 7 \tfile=$(sha1_file \"$*\") &&\n-8 \ttest -e \"$file\" ?!AMP?!\n+8 \ttest -e \"$file\" ?!LINT: missing '&&'?!\n 9 \trm -f \"$file\"\n-10 } ?!AMP?!\n+10 } ?!LINT: missing '&&'?!\n 11 \n 12 sha1_file arg && remove_object arg\ndiff --git a/t/chainlint/here-doc-body-indent.expect b/t/chainlint/here-doc-body-indent.expect\nindex 4323acc93d..4306faee86 100644\n--- a/t/chainlint/here-doc-body-indent.expect\n+++ b/t/chainlint/here-doc-body-indent.expect\n@@ -1,2 +1,2 @@\n-2 \techo \"we should find this\" ?!AMP?!\n+2 \techo \"we should find this\" ?!LINT: missing '&&'?!\n 3 \techo \"even though our heredoc has its indent stripped\"\ndiff --git a/t/chainlint/here-doc-body-pathological.expect b/t/chainlint/here-doc-body-pathological.expect\nindex a93a1fa3aa..2f8ea03a47 100644\n--- a/t/chainlint/here-doc-body-pathological.expect\n+++ b/t/chainlint/here-doc-body-pathological.expect\n@@ -1,7 +1,7 @@\n-2 \techo \"outer here-doc does not allow indented end-tag\" ?!AMP?!\n+2 \techo \"outer here-doc does not allow indented end-tag\" ?!LINT: missing '&&'?!\n 3 \tcat >file <<-\\EOF &&\n 4 \tbut this inner here-doc\n 5 \tdoes allow indented EOF\n 6 \tEOF\n-7 \techo \"missing chain after\" ?!AMP?!\n+7 \techo \"missing chain after\" ?!LINT: missing '&&'?!\n 8 \techo \"but this line is OK because it's the end\"\ndiff --git a/t/chainlint/here-doc-body.expect b/t/chainlint/here-doc-body.expect\nindex ddf1c412af..df8d79bc0a 100644\n--- a/t/chainlint/here-doc-body.expect\n+++ b/t/chainlint/here-doc-body.expect\n@@ -1,7 +1,7 @@\n-2 \techo \"missing chain before\" ?!AMP?!\n+2 \techo \"missing chain before\" ?!LINT: missing '&&'?!\n 3 \tcat >file <<-\\EOF &&\n 4 \tinside inner here-doc\n 5 \tthese are not shell commands\n 6 \tEOF\n-7 \techo \"missing chain after\" ?!AMP?!\n+7 \techo \"missing chain after\" ?!LINT: missing '&&'?!\n 8 \techo \"but this line is OK because it's the end\"\ndiff --git a/t/chainlint/here-doc-double.expect b/t/chainlint/here-doc-double.expect\nindex 20dba4b452..e5e981889f 100644\n--- a/t/chainlint/here-doc-double.expect\n+++ b/t/chainlint/here-doc-double.expect\n@@ -1,2 +1,2 @@\n-8 \techo \"actual test commands\" ?!AMP?!\n+8 \techo \"actual test commands\" ?!LINT: missing '&&'?!\n 9 \techo \"that should be checked\"\ndiff --git a/t/chainlint/here-doc-indent-operator.expect b/t/chainlint/here-doc-indent-operator.expect\nindex 277a11202d..ec0e61505b 100644\n--- a/t/chainlint/here-doc-indent-operator.expect\n+++ b/t/chainlint/here-doc-indent-operator.expect\n@@ -4,7 +4,7 @@\n 5 chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data\n 6 EOF\n 7 \n-8 cat >expect << -EOF ?!AMP?!\n+8 cat >expect << -EOF ?!LINT: missing '&&'?!\n 9 this is not indented\n 10 -EOF\n 11 \ndiff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect\nindex 41b55f6437..8128f15b92 100644\n--- a/t/chainlint/here-doc-multi-line-command-subst.expect\n+++ b/t/chainlint/here-doc-multi-line-command-subst.expect\n@@ -3,6 +3,6 @@\n 4 \t\tfossil\n 5 \t\tvegetable\n 6 \t\tEND\n-7 \t\twiffle) ?!AMP?!\n+7 \t\twiffle) ?!LINT: missing '&&'?!\n 8 \techo $x\n 9 )\ndiff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect\nindex c71828589e..a03a04ff3d 100644\n--- a/t/chainlint/here-doc-multi-line-string.expect\n+++ b/t/chainlint/here-doc-multi-line-string.expect\n@@ -1,6 +1,6 @@\n 2 (\n 3 \tcat <<-\\TXT && echo \"multi-line\n-4 \tstring\" ?!AMP?!\n+4 \tstring\" ?!LINT: missing '&&'?!\n 5 \tfizzle\n 6 \tTXT\n 7 \tbap\ndiff --git a/t/chainlint/if-condition-split.expect b/t/chainlint/if-condition-split.expect\nindex 9daf3d294a..6d2a03dfdb 100644\n--- a/t/chainlint/if-condition-split.expect\n+++ b/t/chainlint/if-condition-split.expect\n@@ -2,6 +2,6 @@\n 3 marcia ||\n 4 kevin\n 5 then\n-6 \techo \"nomads\" ?!AMP?!\n+6 \techo \"nomads\" ?!LINT: missing '&&'?!\n 7 \techo \"for sure\"\n 8 fi\ndiff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect\nindex ff8c60dbdb..7e3ba740de 100644\n--- a/t/chainlint/if-in-loop.expect\n+++ b/t/chainlint/if-in-loop.expect\n@@ -5,8 +5,8 @@\n 6 \t\tthen\n 7 \t\t\techo \"err\"\n 8 \t\t\texit 1\n-9 \t\tfi ?!AMP?!\n+9 \t\tfi ?!LINT: missing '&&'?!\n 10 \t\tfoo\n-11 \tdone ?!AMP?!\n+11 \tdone ?!LINT: missing '&&'?!\n 12 \tbar\n 13 )\ndiff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect\nindex 965d7e41a2..924caa2e4e 100644\n--- a/t/chainlint/if-then-else.expect\n+++ b/t/chainlint/if-then-else.expect\n@@ -1,7 +1,7 @@\n 2 (\n 3 \tif test -n \"\"\n 4 \tthen\n-5 \t\techo very ?!AMP?!\n+5 \t\techo very ?!LINT: missing '&&'?!\n 6 \t\techo empty\n 7 \telif test -z \"\"\n 8 \tthen\n@@ -11,7 +11,7 @@\n 12 \t\tcat <<-\\EOF\n 13 \t\tbar\n 14 \t\tEOF\n-15 \tfi ?!AMP?!\n+15 \tfi ?!LINT: missing '&&'?!\n 16 \techo poodle\n 17 ) &&\n 18 (\ndiff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect\nindex 0285c0b22c..4b4080124e 100644\n--- a/t/chainlint/inline-comment.expect\n+++ b/t/chainlint/inline-comment.expect\n@@ -1,6 +1,6 @@\n 2 (\n 3 \tfoobar && # comment 1\n-4 \tbarfoo ?!AMP?! # wrong position for &&\n+4 \tbarfoo ?!LINT: missing '&&'?! # wrong position for &&\n 5 \tflibble \"not a # comment\"\n 6 ) &&\n 7 \ndiff --git a/t/chainlint/loop-detect-failure.expect b/t/chainlint/loop-detect-failure.expect\nindex 40c06f0d53..7d846b878d 100644\n--- a/t/chainlint/loop-detect-failure.expect\n+++ b/t/chainlint/loop-detect-failure.expect\n@@ -11,5 +11,5 @@\n 12 do\n 13 \tprintf \"%\"$n\"s\" X > r2/large.$n &&\n 14 \tgit -C r2 add large.$n &&\n-15 \tgit -C r2 commit -m \"$n\" ?!LOOP?!\n+15 \tgit -C r2 commit -m \"$n\" ?!LINT: missing '|| return 1'?!\n 16 done\ndiff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect\nindex 4e8c67c914..32e076ad1b 100644\n--- a/t/chainlint/loop-in-if.expect\n+++ b/t/chainlint/loop-in-if.expect\n@@ -3,10 +3,10 @@\n 4 \tthen\n 5 \t\twhile true\n 6 \t\tdo\n-7 \t\t\techo \"pop\" ?!AMP?!\n-8 \t\t\techo \"glup\" ?!LOOP?!\n-9 \t\tdone ?!AMP?!\n+7 \t\t\techo \"pop\" ?!LINT: missing '&&'?!\n+8 \t\t\techo \"glup\" ?!LINT: missing '|| exit 1'?!\n+9 \t\tdone ?!LINT: missing '&&'?!\n 10 \t\tfoo\n-11 \tfi ?!AMP?!\n+11 \tfi ?!LINT: missing '&&'?!\n 12 \tbar\n 13 )\ndiff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect\nindex 62c54e3a5e..9d33297525 100644\n--- a/t/chainlint/multi-line-string.expect\n+++ b/t/chainlint/multi-line-string.expect\n@@ -3,7 +3,7 @@\n 4 \t\tline 2\n 5 \t\tline 3\" &&\n 6 \ty=\"line 1\n-7 \t\tline2\" ?!AMP?!\n+7 \t\tline2\" ?!LINT: missing '&&'?!\n 8 \tfoobar\n 9 ) &&\n 10 (\ndiff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect\nindex a6ce52a1da..0a6f3c29b2 100644\n--- a/t/chainlint/negated-one-liner.expect\n+++ b/t/chainlint/negated-one-liner.expect\n@@ -1,5 +1,5 @@\n 2 ! (foo && bar) &&\n 3 ! (foo && bar) >baz &&\n 4 \n-5 ! (foo; ?!AMP?! bar) &&\n-6 ! (foo; ?!AMP?! bar) >baz\n+5 ! (foo; ?!LINT: missing '&&'?! bar) &&\n+6 ! (foo; ?!LINT: missing '&&'?! bar) >baz\ndiff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect\nindex 0191c9c294..fec2c74274 100644\n--- a/t/chainlint/nested-cuddled-subshell.expect\n+++ b/t/chainlint/nested-cuddled-subshell.expect\n@@ -5,7 +5,7 @@\n 6 \n 7 \t(cd foo &&\n 8 \t\tbar\n-9 \t) ?!AMP?!\n+9 \t) ?!LINT: missing '&&'?!\n 10 \n 11 \t(\n 12 \t\tcd foo &&\n@@ -13,13 +13,13 @@\n 14 \n 15 \t(\n 16 \t\tcd foo &&\n-17 \t\tbar) ?!AMP?!\n+17 \t\tbar) ?!LINT: missing '&&'?!\n 18 \n 19 \t(cd foo &&\n 20 \t\tbar) &&\n 21 \n 22 \t(cd foo &&\n-23 \t\tbar) ?!AMP?!\n+23 \t\tbar) ?!LINT: missing '&&'?!\n 24 \n 25 \tfoobar\n 26 )\ndiff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect\nindex 70d9b68dc9..571f4c9514 100644\n--- a/t/chainlint/nested-here-doc.expect\n+++ b/t/chainlint/nested-here-doc.expect\n@@ -18,7 +18,7 @@\n 19 \ttoink\n 20 \tINPUT_END\n 21 \n-22 \tcat <<-\\EOT ?!AMP?!\n+22 \tcat <<-\\EOT ?!LINT: missing '&&'?!\n 23 \ttext goes here\n 24 \tdata <\"path$i$j\" ?!LOOP?!\n-7 \tdone ?!LOOP?!\n+6 \t\techo \"$i$j\" >\"path$i$j\" ?!LINT: missing '|| return 1'?!\n+7 \tdone ?!LINT: missing '|| return 1'?!\n 8 done &&\n 9 \n 10 for i in 0 1 2 3 4 5 6 7 8 9;\n@@ -18,7 +18,7 @@\n 19 do\n 20 \tfor j in 0 1 2 3 4 5 6 7 8 9;\n 21 \tdo\n-22 \t\techo \"$i$j\" >\"path$i$j\" ?!LOOP?!\n+22 \t\techo \"$i$j\" >\"path$i$j\" ?!LINT: missing '|| return 1'?!\n 23 \tdone || return 1\n 24 done &&\n 25 \ndiff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect\nindex f89a8d03a8..078c6f275f 100644\n--- a/t/chainlint/nested-subshell-comment.expect\n+++ b/t/chainlint/nested-subshell-comment.expect\n@@ -6,6 +6,6 @@\n 7 \t\t# minor numbers of cows (or do they?)\n 8 \t\tbaz &&\n 9 \t\tsnaff\n-10 \t) ?!AMP?!\n+10 \t) ?!LINT: missing '&&'?!\n 11 \tfuzzy\n 12 )\ndiff --git a/t/chainlint/nested-subshell.expect b/t/chainlint/nested-subshell.expect\nindex 811e8a7912..a8d85d5d5b 100644\n--- a/t/chainlint/nested-subshell.expect\n+++ b/t/chainlint/nested-subshell.expect\n@@ -7,7 +7,7 @@\n 8 \n 9 \tcd foo &&\n 10 \t(\n-11 \t\techo a ?!AMP?!\n+11 \t\techo a ?!LINT: missing '&&'?!\n 12 \t\techo b\n 13 \t) >file\n 14 )\ndiff --git a/t/chainlint/not-heredoc.expect b/t/chainlint/not-heredoc.expect\nindex 611b7b75cb..5d51705a7a 100644\n--- a/t/chainlint/not-heredoc.expect\n+++ b/t/chainlint/not-heredoc.expect\n@@ -9,6 +9,6 @@\n 10 \techo ourside &&\n 11 \techo \"=======\" &&\n 12 \techo theirside &&\n-13 \techo \">>>>>>> theirs\" ?!AMP?!\n+13 \techo \">>>>>>> theirs\" ?!LINT: missing '&&'?!\n 14 \tpoodle\n 15 ) >merged\ndiff --git a/t/chainlint/one-liner-for-loop.expect b/t/chainlint/one-liner-for-loop.expect\nindex 49dcf065ef..e1fcbd3639 100644\n--- a/t/chainlint/one-liner-for-loop.expect\n+++ b/t/chainlint/one-liner-for-loop.expect\n@@ -3,7 +3,7 @@\n 4 \tcd dir-rename-and-content &&\n 5 \ttest_write_lines 1 2 3 4 5 >foo &&\n 6 \tmkdir olddir &&\n-7 \tfor i in a b c; do echo $i >olddir/$i; ?!LOOP?! done ?!AMP?!\n+7 \tfor i in a b c; do echo $i >olddir/$i; ?!LINT: missing '|| exit 1'?! done ?!LINT: missing '&&'?!\n 8 \tgit add foo olddir &&\n 9 \tgit commit -m \"original\" &&\n 10 )\ndiff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect\nindex 9861811283..5deeb05070 100644\n--- a/t/chainlint/one-liner.expect\n+++ b/t/chainlint/one-liner.expect\n@@ -2,8 +2,8 @@\n 3 (foo && bar) |\n 4 (foo && bar) >baz &&\n 5 \n-6 (foo; ?!AMP?! bar) &&\n-7 (foo; ?!AMP?! bar) |\n-8 (foo; ?!AMP?! bar) >baz &&\n+6 (foo; ?!LINT: missing '&&'?! bar) &&\n+7 (foo; ?!LINT: missing '&&'?! bar) |\n+8 (foo; ?!LINT: missing '&&'?! bar) >baz &&\n 9 \n 10 (foo \"bar; baz\")\ndiff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect\nindex 1bbe5a2ce1..d947c76584 100644\n--- a/t/chainlint/pipe.expect\n+++ b/t/chainlint/pipe.expect\n@@ -4,7 +4,7 @@\n 5 \tbaz &&\n 6 \n 7 \tfish |\n-8 \tcow ?!AMP?!\n+8 \tcow ?!LINT: missing '&&'?!\n 9 \n 10 \tsunder\n 11 )\ndiff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect\nindex 866438310c..2b499fbe70 100644\n--- a/t/chainlint/semicolon.expect\n+++ b/t/chainlint/semicolon.expect\n@@ -1,19 +1,19 @@\n 2 (\n-3 \tcat foo ; ?!AMP?! echo bar ?!AMP?!\n-4 \tcat foo ; ?!AMP?! echo bar\n+3 \tcat foo ; ?!LINT: missing '&&'?! echo bar ?!LINT: missing '&&'?!\n+4 \tcat foo ; ?!LINT: missing '&&'?! echo bar\n 5 ) &&\n 6 (\n-7 \tcat foo ; ?!AMP?! echo bar &&\n-8 \tcat foo ; ?!AMP?! echo bar\n+7 \tcat foo ; ?!LINT: missing '&&'?! echo bar &&\n+8 \tcat foo ; ?!LINT: missing '&&'?! echo bar\n 9 ) &&\n 10 (\n 11 \techo \"foo; bar\" &&\n-12 \tcat foo; ?!AMP?! echo bar\n+12 \tcat foo; ?!LINT: missing '&&'?! echo bar\n 13 ) &&\n 14 (\n 15 \tfoo;\n 16 ) &&\n 17 (cd foo &&\n 18 \tfor i in a b c; do\n-19 \t\techo; ?!LOOP?!\n+19 \t\techo; ?!LINT: missing '|| exit 1'?!\n 20 \tdone)\ndiff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect\nindex 5647500c82..e450caf948 100644\n--- a/t/chainlint/subshell-here-doc.expect\n+++ b/t/chainlint/subshell-here-doc.expect\n@@ -6,7 +6,7 @@\n 7 \tnevermore...\n 8 \tEOF\n 9 \n-10 \tcat <bip ?!AMP?!\n+10 \tcat <bip ?!LINT: missing '&&'?!\n 11 \tfish fly high\n 12 EOF\n 13 \ndiff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect\nindex 214316c6a0..265d996a21 100644\n--- a/t/chainlint/subshell-one-liner.expect\n+++ b/t/chainlint/subshell-one-liner.expect\n@@ -3,17 +3,17 @@\n 4 \t(foo && bar) |\n 5 \t(foo && bar) >baz &&\n 6 \n-7 \t(foo; ?!AMP?! bar) &&\n-8 \t(foo; ?!AMP?! bar) |\n-9 \t(foo; ?!AMP?! bar) >baz &&\n+7 \t(foo; ?!LINT: missing '&&'?! bar) &&\n+8 \t(foo; ?!LINT: missing '&&'?! bar) |\n+9 \t(foo; ?!LINT: missing '&&'?! bar) >baz &&\n 10 \n 11 \t(foo || exit 1) &&\n 12 \t(foo || exit 1) |\n 13 \t(foo || exit 1) >baz &&\n 14 \n-15 \t(foo && bar) ?!AMP?!\n+15 \t(foo && bar) ?!LINT: missing '&&'?!\n 16 \n-17 \t(foo && bar; ?!AMP?! baz) ?!AMP?!\n+17 \t(foo && bar; ?!LINT: missing '&&'?! baz) ?!LINT: missing '&&'?!\n 18 \n 19 \tfoobar\n 20 )\ndiff --git a/t/chainlint/token-pasting.expect b/t/chainlint/token-pasting.expect\nindex 64f3235d26..387189b6de 100644\n--- a/t/chainlint/token-pasting.expect\n+++ b/t/chainlint/token-pasting.expect\n@@ -2,13 +2,13 @@\n 3 git config filter.rot13.clean ./rot13.sh &&\n 4 \n 5 {\n-6 echo \"*.t filter=rot13\" ?!AMP?!\n+6 echo \"*.t filter=rot13\" ?!LINT: missing '&&'?!\n 7 echo \"*.i ident\"\n 8 } >.gitattributes &&\n 9 \n 10 {\n-11 echo a b c d e f g h i j k l m ?!AMP?!\n-12 echo n o p q r s t u v w x y z ?!AMP?!\n+11 echo a b c d e f g h i j k l m ?!LINT: missing '&&'?!\n+12 echo n o p q r s t u v w x y z ?!LINT: missing '&&'?!\n 13 echo '$Id$'\n 14 } >test &&\n 15 cat test >test.t &&\n@@ -19,7 +19,7 @@\n 20 git checkout -- test test.t test.i &&\n 21 \n 22 echo \"content-test2\" >test2.o &&\n-23 echo \"content-test3 - filename with special characters\" >\"test3 'sq',$x=.o\" ?!AMP?!\n+23 echo \"content-test3 - filename with special characters\" >\"test3 'sq',$x=.o\" ?!LINT: missing '&&'?!\n 24 \n 25 downstream_url_for_sed=$(\n 26 \tprintf \"%sn\" \"$downstream_url\" |\ndiff --git a/t/chainlint/unclosed-here-doc-indent.expect b/t/chainlint/unclosed-here-doc-indent.expect\nindex f78e23cb63..156906c85a 100644\n--- a/t/chainlint/unclosed-here-doc-indent.expect\n+++ b/t/chainlint/unclosed-here-doc-indent.expect\n@@ -1,4 +1,4 @@\n 2 command_which_is_run &&\n-3 cat >expect <<-\\EOF ?!UNCLOSED-HEREDOC?! &&\n+3 cat >expect <<-\\EOF ?!LINT: unclosed heredoc?! &&\n 4 we forget to end the here-doc\n 5 command_which_is_gobbled\ndiff --git a/t/chainlint/unclosed-here-doc.expect b/t/chainlint/unclosed-here-doc.expect\nindex 51304672cf..752c608862 100644\n--- a/t/chainlint/unclosed-here-doc.expect\n+++ b/t/chainlint/unclosed-here-doc.expect\n@@ -1,5 +1,5 @@\n 2 command_which_is_run &&\n-3 cat >expect <<\\EOF ?!UNCLOSED-HEREDOC?! &&\n+3 cat >expect <<\\EOF ?!LINT: unclosed heredoc?! &&\n 4 \twe try to end the here-doc below,\n 5 \tbut the indentation throws us off\n 6 \tsince the operator is not \"<<-\".\ndiff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect\nindex 5ffabd5a93..2ba5582165 100644\n--- a/t/chainlint/while-loop.expect\n+++ b/t/chainlint/while-loop.expect\n@@ -1,14 +1,14 @@\n 2 (\n 3 \twhile true\n 4 \tdo\n-5 \t\techo foo ?!AMP?!\n-6 \t\tcat <<-\\EOF ?!LOOP?!\n+5 \t\techo foo ?!LINT: missing '&&'?!\n+6 \t\tcat <<-\\EOF ?!LINT: missing '|| exit 1'?!\n 7 \t\tbar\n 8 \t\tEOF\n-9 \tdone ?!AMP?!\n+9 \tdone ?!LINT: missing '&&'?!\n 10 \n 11 \twhile true; do\n 12 \t\techo foo &&\n-13 \t\tcat bar ?!LOOP?!\n+13 \t\tcat bar ?!LINT: missing '|| exit 1'?!\n 14 \tdone\n 15 )\n-- \n2.46.0\n\n"},{"id":"502529","messageId":"20240910041013.68948-1-ericsunshine@charter.net","threadId":"62018","inReplyTo":"20240829091625.41297-1-ericsunshine@charter.net","subject":"[PATCH v2 0/3] make chainlint output more newcomer-friendly","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2024-09-10T04:10:10Z","receivedAt":"2024-09-10T04:12:10Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nThis is a reroll of [1] which (hopefully) makes chainlint's problem\nannotations more friendly and helpful for newcomers. In particular, it\nditches the cryptic \"AMP\", \"LOOP\", etc. and replaces them with proper\nerror messages.\n\nChanges since v1:\n\n* new patch [1/3] -- motivated by Junio's observation[2] about\n availability of structured problem information -- takes advantage of\n that information directly rather than post-processing \"?!...?!\"\n sequences in the output stream\n\n* old patch [2/2] (now [3/3]) which drops \"?!\" decorations when emitting\n colored output to a terminal partially justified the change by\n claiming that the new \"ERR\" (or \"ERR:\") prefix is a good \"needle\" for\n a terminal's search feature, thus the noisy \"?!\" is no longer needed;\n however, I realized that \"ERR\" (or \"ERR:\") is, in fact, an awful\n needle since the string \"err\" (or \"err:\") is quite likely to\n legitimately appear in source text, hence I changed the prefix to\n \"LINT:\" (with the colon since Patrick found lack of colon\n confusing[3])\n\n* rewrote commit messages based upon feedback from Junio[2,4]\n\n* dropped an unused argument from the call to format_problem() which was\n an artifact used briefly during development of v1\n\nUnfortunately, the included range-diff is a mess and pretty much useless\nsince half the changes from old patch [2/2] (now [3/3]) migrated to new\npatch [1/3], and range-diff thinks that [1/3] is a rewrite of old [2/2]\nand that old [2/2] is now entirely new [3/3] which is exactly opposite\nwhat really happened, and I wasn't able to convince range-diff to\nreconsider. Hence, I also included an interdiff which is easier to read\nbut doesn't show improvements to the commit messages.\n\n[1]: https://lore.kernel.org/git/20240829091625.41297-1-ericsunshine@charter.net/\n[2]: https://lore.kernel.org/git/xmqqv7zjwcgq.fsf@gitster.g/\n[3]: https://lore.kernel.org/git/ZtBHbftK7vdTEz93@tanuki/\n[4]: https://lore.kernel.org/git/xmqq7cbzxrry.fsf@gitster.g/\n\nEric Sunshine (3):\n chainlint: don't be fooled by \"?!...?!\" in test body\n chainlint: make error messages self-explanatory\n chainlint: reduce annotation noise-factor\n\n t/chainlint.pl | 42 +++++++++++++------\n t/chainlint/arithmetic-expansion.expect | 2 +-\n t/chainlint/block.expect | 8 ++--\n t/chainlint/broken-chain.expect | 2 +-\n t/chainlint/case.expect | 4 +-\n t/chainlint/chain-break-false.expect | 2 +-\n t/chainlint/chained-block.expect | 2 +-\n t/chainlint/chained-subshell.expect | 4 +-\n t/chainlint/command-substitution.expect | 2 +-\n t/chainlint/complex-if-in-cuddled-loop.expect | 2 +-\n t/chainlint/cuddled.expect | 4 +-\n t/chainlint/for-loop.expect | 8 ++--\n t/chainlint/function.expect | 4 +-\n t/chainlint/here-doc-body-indent.expect | 2 +-\n t/chainlint/here-doc-body-pathological.expect | 4 +-\n t/chainlint/here-doc-body.expect | 4 +-\n t/chainlint/here-doc-double.expect | 2 +-\n t/chainlint/here-doc-indent-operator.expect | 2 +-\n .../here-doc-multi-line-command-subst.expect | 2 +-\n t/chainlint/here-doc-multi-line-string.expect | 2 +-\n t/chainlint/if-condition-split.expect | 2 +-\n t/chainlint/if-in-loop.expect | 4 +-\n t/chainlint/if-then-else.expect | 4 +-\n t/chainlint/inline-comment.expect | 2 +-\n t/chainlint/loop-detect-failure.expect | 2 +-\n t/chainlint/loop-in-if.expect | 8 ++--\n t/chainlint/multi-line-string.expect | 2 +-\n t/chainlint/negated-one-liner.expect | 4 +-\n t/chainlint/nested-cuddled-subshell.expect | 6 +--\n t/chainlint/nested-here-doc.expect | 2 +-\n t/chainlint/nested-loop-detect-failure.expect | 6 +--\n t/chainlint/nested-subshell-comment.expect | 2 +-\n t/chainlint/nested-subshell.expect | 2 +-\n t/chainlint/not-heredoc.expect | 2 +-\n t/chainlint/one-liner-for-loop.expect | 2 +-\n t/chainlint/one-liner.expect | 6 +--\n t/chainlint/pipe.expect | 2 +-\n t/chainlint/semicolon.expect | 12 +++---\n t/chainlint/subshell-here-doc.expect | 2 +-\n t/chainlint/subshell-one-liner.expect | 10 ++---\n t/chainlint/token-pasting.expect | 8 ++--\n t/chainlint/unclosed-here-doc-indent.expect | 2 +-\n t/chainlint/unclosed-here-doc.expect | 2 +-\n t/chainlint/while-loop.expect | 8 ++--\n t/test-lib.sh | 2 +-\n 45 files changed, 113 insertions(+), 95 deletions(-)\n\n\nInterdiff against v1:\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 971ab9212a..f0598e3934 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -9,9 +9,9 @@\n # Input arguments are pathnames of shell scripts containing test definitions,\n # or globs referencing a collection of scripts. For each problem discovered,\n # the pathname of the script containing the test is printed along with the test\n-# name and the test body with a `?!ERR?!` annotation at the location of each\n-# detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n-# &&-chain. Returns zero if no problems are discovered, otherwise non-zero.\n+# name and the test body with a `?!LINT: ...?!` annotation at the location of\n+# each detected problem, where \"...\" is an explanation of the problem. Returns\n+# zero if no problems are discovered, otherwise non-zero.\n \n use warnings;\n use strict;\n@@ -657,16 +657,17 @@ sub check_test {\n \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n \t\tmy ($label, $token) = @$_;\n \t\tmy $pos = $token->[2];\n-\t\tmy $err = format_problem($label, $token);\n-\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!ERR $err?! \";\n+\t\tmy $err = format_problem($label);\n+\t\t$checked .= substr($body, $start, $pos - $start);\n+\t\t$checked .= ' ' unless $checked =~ /\\s$/;\n+\t\t$checked .= \"${erropen}LINT: $err$errclose\";\n+\t\t$checked .= ' ' unless $pos >= length($body) ||\n+\t\t substr($body, $pos, 1) =~ /^\\s/;\n \t\t$start = $pos;\n \t}\n \t$checked .= substr($body, $start);\n \t$checked =~ s/^/$lineno++ . ' '/mge;\n \t$checked =~ s/^\\d+ \\n//;\n-\t$checked =~ s/(\\s) \\?!/$1?!/mg;\n-\t$checked =~ s/\\?! (\\s)/?!$1/mg;\n-\t$checked =~ s/\\?!([^?]+)\\?!/$erropen$1$errclose/mg;\n \t$checked =~ s/^\\d+/$c->{dim}$&$c->{reset}/mg;\n \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n \tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\ndiff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect\nindex 2efd65dcbd..5677e16cad 100644\n--- a/t/chainlint/arithmetic-expansion.expect\n+++ b/t/chainlint/arithmetic-expansion.expect\n@@ -4,6 +4,6 @@\n 5 \tbaz\n 6 ) &&\n 7 (\n-8 \tbar=$((42 + 1)) ?!ERR missing '&&'?!\n+8 \tbar=$((42 + 1)) ?!LINT: missing '&&'?!\n 9 \tbaz\n 10 )\ndiff --git a/t/chainlint/block.expect b/t/chainlint/block.expect\nindex 28410b33ef..3d3f854c0d 100644\n--- a/t/chainlint/block.expect\n+++ b/t/chainlint/block.expect\n@@ -1,20 +1,20 @@\n 2 (\n 3 \tfoo &&\n 4 \t{\n-5 \t\techo a ?!ERR missing '&&'?!\n+5 \t\techo a ?!LINT: missing '&&'?!\n 6 \t\techo b\n 7 \t} &&\n 8 \tbar &&\n 9 \t{\n 10 \t\techo c\n-11 \t} ?!ERR missing '&&'?!\n+11 \t} ?!LINT: missing '&&'?!\n 12 \tbaz\n 13 ) &&\n 14 \n 15 {\n-16 \techo a; ?!ERR missing '&&'?! echo b\n+16 \techo a; ?!LINT: missing '&&'?! echo b\n 17 } &&\n-18 { echo a; ?!ERR missing '&&'?! echo b; } &&\n+18 { echo a; ?!LINT: missing '&&'?! echo b; } &&\n 19 \n 20 {\n 21 \techo \"${var}9\" &&\ndiff --git a/t/chainlint/broken-chain.expect b/t/chainlint/broken-chain.expect\nindex 2a209df0a7..b7b1ce8509 100644\n--- a/t/chainlint/broken-chain.expect\n+++ b/t/chainlint/broken-chain.expect\n@@ -1,6 +1,6 @@\n 2 (\n 3 \tfoo &&\n-4 \tbar ?!ERR missing '&&'?!\n+4 \tbar ?!LINT: missing '&&'?!\n 5 \tbaz &&\n 6 \twop\n 7 )\ndiff --git a/t/chainlint/case.expect b/t/chainlint/case.expect\nindex d00b67b766..0a3b09e470 100644\n--- a/t/chainlint/case.expect\n+++ b/t/chainlint/case.expect\n@@ -9,11 +9,11 @@\n 10 \tcase \"$x\" in\n 11 \tx) foo ;;\n 12 \t*) bar ;;\n-13 \tesac ?!ERR missing '&&'?!\n+13 \tesac ?!LINT: missing '&&'?!\n 14 \tfoobar\n 15 ) &&\n 16 (\n 17 \tcase \"$x\" in 1) true;; esac &&\n-18 \tcase \"$y\" in 2) false;; esac ?!ERR missing '&&'?!\n+18 \tcase \"$y\" in 2) false;; esac ?!LINT: missing '&&'?!\n 19 \tfoobar\n 20 )\ndiff --git a/t/chainlint/chain-break-false.expect b/t/chainlint/chain-break-false.expect\nindex bfccc0d90f..f6a0a301e9 100644\n--- a/t/chainlint/chain-break-false.expect\n+++ b/t/chainlint/chain-break-false.expect\n@@ -4,6 +4,6 @@\n 5 \techo failed!\n 6 \tfalse\n 7 else\n-8 \techo it went okay ?!ERR missing '&&'?!\n+8 \techo it went okay ?!LINT: missing '&&'?!\n 9 \tcongratulate user\n 10 fi\ndiff --git a/t/chainlint/chained-block.expect b/t/chainlint/chained-block.expect\nindex 293d9eac42..f2501bba90 100644\n--- a/t/chainlint/chained-block.expect\n+++ b/t/chainlint/chained-block.expect\n@@ -1,5 +1,5 @@\n 2 echo nobody home && {\n-3 \ttest the doohicky ?!ERR missing '&&'?!\n+3 \ttest the doohicky ?!LINT: missing '&&'?!\n 4 \tright now\n 5 } &&\n 6 \ndiff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect\nindex 2f5de4fead..93fb1a6578 100644\n--- a/t/chainlint/chained-subshell.expect\n+++ b/t/chainlint/chained-subshell.expect\n@@ -1,10 +1,10 @@\n 2 mkdir sub && (\n 3 \tcd sub &&\n-4 \tfoo the bar ?!ERR missing '&&'?!\n+4 \tfoo the bar ?!LINT: missing '&&'?!\n 5 \tnuff said\n 6 ) &&\n 7 \n 8 cut \"-d \" -f actual | (read s1 s2 s3 &&\n-9 test -f $s1 ?!ERR missing '&&'?!\n+9 test -f $s1 ?!LINT: missing '&&'?!\n 10 test $(cat $s2) = tree2path1 &&\n 11 test $(cat $s3) = tree3path1)\ndiff --git a/t/chainlint/command-substitution.expect b/t/chainlint/command-substitution.expect\nindex 511c918cb5..73809fd585 100644\n--- a/t/chainlint/command-substitution.expect\n+++ b/t/chainlint/command-substitution.expect\n@@ -4,6 +4,6 @@\n 5 \tbaz\n 6 ) &&\n 7 (\n-8 \tbar=$(gobble blocks) ?!ERR missing '&&'?!\n+8 \tbar=$(gobble blocks) ?!LINT: missing '&&'?!\n 9 \tbaz\n 10 )\ndiff --git a/t/chainlint/complex-if-in-cuddled-loop.expect b/t/chainlint/complex-if-in-cuddled-loop.expect\nindex eb855378a1..e66bb2d5d0 100644\n--- a/t/chainlint/complex-if-in-cuddled-loop.expect\n+++ b/t/chainlint/complex-if-in-cuddled-loop.expect\n@@ -4,6 +4,6 @@\n 5 :\n 6 else\n 7 echo >file\n-8 fi ?!ERR missing '|| exit 1'?!\n+8 fi ?!LINT: missing '|| exit 1'?!\n 9 done) &&\n 10 test ! -f file\ndiff --git a/t/chainlint/cuddled.expect b/t/chainlint/cuddled.expect\nindex 65825c6879..1864b3fc8b 100644\n--- a/t/chainlint/cuddled.expect\n+++ b/t/chainlint/cuddled.expect\n@@ -2,7 +2,7 @@\n 3 \tbar\n 4 ) &&\n 5 \n-6 (cd foo ?!ERR missing '&&'?!\n+6 (cd foo ?!LINT: missing '&&'?!\n 7 \tbar\n 8 ) &&\n 9 \n@@ -13,5 +13,5 @@\n 14 (cd foo &&\n 15 \tbar) &&\n 16 \n-17 (cd foo ?!ERR missing '&&'?!\n+17 (cd foo ?!LINT: missing '&&'?!\n 18 \tbar)\ndiff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect\nindex df6fc1a35f..5029eacce3 100644\n--- a/t/chainlint/for-loop.expect\n+++ b/t/chainlint/for-loop.expect\n@@ -1,14 +1,14 @@\n 2 (\n 3 \tfor i in a b c\n 4 \tdo\n-5 \t\techo $i ?!ERR missing '&&'?!\n-6 \t\tcat <<-\\EOF ?!ERR missing '|| exit 1'?!\n+5 \t\techo $i ?!LINT: missing '&&'?!\n+6 \t\tcat <<-\\EOF ?!LINT: missing '|| exit 1'?!\n 7 \t\tbar\n 8 \t\tEOF\n-9 \tdone ?!ERR missing '&&'?!\n+9 \tdone ?!LINT: missing '&&'?!\n 10 \n 11 \tfor i in a b c; do\n 12 \t\techo $i &&\n-13 \t\tcat $i ?!ERR missing '|| exit 1'?!\n+13 \t\tcat $i ?!LINT: missing '|| exit 1'?!\n 14 \tdone\n 15 )\ndiff --git a/t/chainlint/function.expect b/t/chainlint/function.expect\nindex 7a15be745b..9e46a3554a 100644\n--- a/t/chainlint/function.expect\n+++ b/t/chainlint/function.expect\n@@ -4,8 +4,8 @@\n 5 \n 6 remove_object() {\n 7 \tfile=$(sha1_file \"$*\") &&\n-8 \ttest -e \"$file\" ?!ERR missing '&&'?!\n+8 \ttest -e \"$file\" ?!LINT: missing '&&'?!\n 9 \trm -f \"$file\"\n-10 } ?!ERR missing '&&'?!\n+10 } ?!LINT: missing '&&'?!\n 11 \n 12 sha1_file arg && remove_object arg\ndiff --git a/t/chainlint/here-doc-body-indent.expect b/t/chainlint/here-doc-body-indent.expect\nindex 1d7298c8ad..4306faee86 100644\n--- a/t/chainlint/here-doc-body-indent.expect\n+++ b/t/chainlint/here-doc-body-indent.expect\n@@ -1,2 +1,2 @@\n-2 \techo \"we should find this\" ?!ERR missing '&&'?!\n+2 \techo \"we should find this\" ?!LINT: missing '&&'?!\n 3 \techo \"even though our heredoc has its indent stripped\"\ndiff --git a/t/chainlint/here-doc-body-pathological.expect b/t/chainlint/here-doc-body-pathological.expect\nindex 828f232616..2f8ea03a47 100644\n--- a/t/chainlint/here-doc-body-pathological.expect\n+++ b/t/chainlint/here-doc-body-pathological.expect\n@@ -1,7 +1,7 @@\n-2 \techo \"outer here-doc does not allow indented end-tag\" ?!ERR missing '&&'?!\n+2 \techo \"outer here-doc does not allow indented end-tag\" ?!LINT: missing '&&'?!\n 3 \tcat >file <<-\\EOF &&\n 4 \tbut this inner here-doc\n 5 \tdoes allow indented EOF\n 6 \tEOF\n-7 \techo \"missing chain after\" ?!ERR missing '&&'?!\n+7 \techo \"missing chain after\" ?!LINT: missing '&&'?!\n 8 \techo \"but this line is OK because it's the end\"\ndiff --git a/t/chainlint/here-doc-body.expect b/t/chainlint/here-doc-body.expect\nindex 79b9603c1e..df8d79bc0a 100644\n--- a/t/chainlint/here-doc-body.expect\n+++ b/t/chainlint/here-doc-body.expect\n@@ -1,7 +1,7 @@\n-2 \techo \"missing chain before\" ?!ERR missing '&&'?!\n+2 \techo \"missing chain before\" ?!LINT: missing '&&'?!\n 3 \tcat >file <<-\\EOF &&\n 4 \tinside inner here-doc\n 5 \tthese are not shell commands\n 6 \tEOF\n-7 \techo \"missing chain after\" ?!ERR missing '&&'?!\n+7 \techo \"missing chain after\" ?!LINT: missing '&&'?!\n 8 \techo \"but this line is OK because it's the end\"\ndiff --git a/t/chainlint/here-doc-double.expect b/t/chainlint/here-doc-double.expect\nindex 9cb1a1a5e3..e5e981889f 100644\n--- a/t/chainlint/here-doc-double.expect\n+++ b/t/chainlint/here-doc-double.expect\n@@ -1,2 +1,2 @@\n-8 \techo \"actual test commands\" ?!ERR missing '&&'?!\n+8 \techo \"actual test commands\" ?!LINT: missing '&&'?!\n 9 \techo \"that should be checked\"\ndiff --git a/t/chainlint/here-doc-indent-operator.expect b/t/chainlint/here-doc-indent-operator.expect\nindex 2d61e5f49d..ec0e61505b 100644\n--- a/t/chainlint/here-doc-indent-operator.expect\n+++ b/t/chainlint/here-doc-indent-operator.expect\n@@ -4,7 +4,7 @@\n 5 chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data\n 6 EOF\n 7 \n-8 cat >expect << -EOF ?!ERR missing '&&'?!\n+8 cat >expect << -EOF ?!LINT: missing '&&'?!\n 9 this is not indented\n 10 -EOF\n 11 \ndiff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect\nindex 881e4d2098..8128f15b92 100644\n--- a/t/chainlint/here-doc-multi-line-command-subst.expect\n+++ b/t/chainlint/here-doc-multi-line-command-subst.expect\n@@ -3,6 +3,6 @@\n 4 \t\tfossil\n 5 \t\tvegetable\n 6 \t\tEND\n-7 \t\twiffle) ?!ERR missing '&&'?!\n+7 \t\twiffle) ?!LINT: missing '&&'?!\n 8 \techo $x\n 9 )\ndiff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect\nindex 06c791e0a4..a03a04ff3d 100644\n--- a/t/chainlint/here-doc-multi-line-string.expect\n+++ b/t/chainlint/here-doc-multi-line-string.expect\n@@ -1,6 +1,6 @@\n 2 (\n 3 \tcat <<-\\TXT && echo \"multi-line\n-4 \tstring\" ?!ERR missing '&&'?!\n+4 \tstring\" ?!LINT: missing '&&'?!\n 5 \tfizzle\n 6 \tTXT\n 7 \tbap\ndiff --git a/t/chainlint/if-condition-split.expect b/t/chainlint/if-condition-split.expect\nindex 5688d93a4f..6d2a03dfdb 100644\n--- a/t/chainlint/if-condition-split.expect\n+++ b/t/chainlint/if-condition-split.expect\n@@ -2,6 +2,6 @@\n 3 marcia ||\n 4 kevin\n 5 then\n-6 \techo \"nomads\" ?!ERR missing '&&'?!\n+6 \techo \"nomads\" ?!LINT: missing '&&'?!\n 7 \techo \"for sure\"\n 8 fi\ndiff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect\nindex 253b461f87..7e3ba740de 100644\n--- a/t/chainlint/if-in-loop.expect\n+++ b/t/chainlint/if-in-loop.expect\n@@ -5,8 +5,8 @@\n 6 \t\tthen\n 7 \t\t\techo \"err\"\n 8 \t\t\texit 1\n-9 \t\tfi ?!ERR missing '&&'?!\n+9 \t\tfi ?!LINT: missing '&&'?!\n 10 \t\tfoo\n-11 \tdone ?!ERR missing '&&'?!\n+11 \tdone ?!LINT: missing '&&'?!\n 12 \tbar\n 13 )\ndiff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect\nindex 1b3162759f..924caa2e4e 100644\n--- a/t/chainlint/if-then-else.expect\n+++ b/t/chainlint/if-then-else.expect\n@@ -1,7 +1,7 @@\n 2 (\n 3 \tif test -n \"\"\n 4 \tthen\n-5 \t\techo very ?!ERR missing '&&'?!\n+5 \t\techo very ?!LINT: missing '&&'?!\n 6 \t\techo empty\n 7 \telif test -z \"\"\n 8 \tthen\n@@ -11,7 +11,7 @@\n 12 \t\tcat <<-\\EOF\n 13 \t\tbar\n 14 \t\tEOF\n-15 \tfi ?!ERR missing '&&'?!\n+15 \tfi ?!LINT: missing '&&'?!\n 16 \techo poodle\n 17 ) &&\n 18 (\ndiff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect\nindex fedc059a0c..4b4080124e 100644\n--- a/t/chainlint/inline-comment.expect\n+++ b/t/chainlint/inline-comment.expect\n@@ -1,6 +1,6 @@\n 2 (\n 3 \tfoobar && # comment 1\n-4 \tbarfoo ?!ERR missing '&&'?! # wrong position for &&\n+4 \tbarfoo ?!LINT: missing '&&'?! # wrong position for &&\n 5 \tflibble \"not a # comment\"\n 6 ) &&\n 7 \ndiff --git a/t/chainlint/loop-detect-failure.expect b/t/chainlint/loop-detect-failure.expect\nindex 2d46f6d2eb..7d846b878d 100644\n--- a/t/chainlint/loop-detect-failure.expect\n+++ b/t/chainlint/loop-detect-failure.expect\n@@ -11,5 +11,5 @@\n 12 do\n 13 \tprintf \"%\"$n\"s\" X > r2/large.$n &&\n 14 \tgit -C r2 add large.$n &&\n-15 \tgit -C r2 commit -m \"$n\" ?!ERR missing '|| return 1'?!\n+15 \tgit -C r2 commit -m \"$n\" ?!LINT: missing '|| return 1'?!\n 16 done\ndiff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect\nindex 8936d7ff2d..32e076ad1b 100644\n--- a/t/chainlint/loop-in-if.expect\n+++ b/t/chainlint/loop-in-if.expect\n@@ -3,10 +3,10 @@\n 4 \tthen\n 5 \t\twhile true\n 6 \t\tdo\n-7 \t\t\techo \"pop\" ?!ERR missing '&&'?!\n-8 \t\t\techo \"glup\" ?!ERR missing '|| exit 1'?!\n-9 \t\tdone ?!ERR missing '&&'?!\n+7 \t\t\techo \"pop\" ?!LINT: missing '&&'?!\n+8 \t\t\techo \"glup\" ?!LINT: missing '|| exit 1'?!\n+9 \t\tdone ?!LINT: missing '&&'?!\n 10 \t\tfoo\n-11 \tfi ?!ERR missing '&&'?!\n+11 \tfi ?!LINT: missing '&&'?!\n 12 \tbar\n 13 )\ndiff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect\nindex 3c3a1de75c..9d33297525 100644\n--- a/t/chainlint/multi-line-string.expect\n+++ b/t/chainlint/multi-line-string.expect\n@@ -3,7 +3,7 @@\n 4 \t\tline 2\n 5 \t\tline 3\" &&\n 6 \ty=\"line 1\n-7 \t\tline2\" ?!ERR missing '&&'?!\n+7 \t\tline2\" ?!LINT: missing '&&'?!\n 8 \tfoobar\n 9 ) &&\n 10 (\ndiff --git a/t/chainlint/negated-one-liner.expect b/t/chainlint/negated-one-liner.expect\nindex 12bd65264a..0a6f3c29b2 100644\n--- a/t/chainlint/negated-one-liner.expect\n+++ b/t/chainlint/negated-one-liner.expect\n@@ -1,5 +1,5 @@\n 2 ! (foo && bar) &&\n 3 ! (foo && bar) >baz &&\n 4 \n-5 ! (foo; ?!ERR missing '&&'?! bar) &&\n-6 ! (foo; ?!ERR missing '&&'?! bar) >baz\n+5 ! (foo; ?!LINT: missing '&&'?! bar) &&\n+6 ! (foo; ?!LINT: missing '&&'?! bar) >baz\ndiff --git a/t/chainlint/nested-cuddled-subshell.expect b/t/chainlint/nested-cuddled-subshell.expect\nindex 3e947ea5e1..fec2c74274 100644\n--- a/t/chainlint/nested-cuddled-subshell.expect\n+++ b/t/chainlint/nested-cuddled-subshell.expect\n@@ -5,7 +5,7 @@\n 6 \n 7 \t(cd foo &&\n 8 \t\tbar\n-9 \t) ?!ERR missing '&&'?!\n+9 \t) ?!LINT: missing '&&'?!\n 10 \n 11 \t(\n 12 \t\tcd foo &&\n@@ -13,13 +13,13 @@\n 14 \n 15 \t(\n 16 \t\tcd foo &&\n-17 \t\tbar) ?!ERR missing '&&'?!\n+17 \t\tbar) ?!LINT: missing '&&'?!\n 18 \n 19 \t(cd foo &&\n 20 \t\tbar) &&\n 21 \n 22 \t(cd foo &&\n-23 \t\tbar) ?!ERR missing '&&'?!\n+23 \t\tbar) ?!LINT: missing '&&'?!\n 24 \n 25 \tfoobar\n 26 )\ndiff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect\nindex 107e5afb01..571f4c9514 100644\n--- a/t/chainlint/nested-here-doc.expect\n+++ b/t/chainlint/nested-here-doc.expect\n@@ -18,7 +18,7 @@\n 19 \ttoink\n 20 \tINPUT_END\n 21 \n-22 \tcat <<-\\EOT ?!ERR missing '&&'?!\n+22 \tcat <<-\\EOT ?!LINT: missing '&&'?!\n 23 \ttext goes here\n 24 \tdata <\"path$i$j\" ?!ERR missing '|| return 1'?!\n-7 \tdone ?!ERR missing '|| return 1'?!\n+6 \t\techo \"$i$j\" >\"path$i$j\" ?!LINT: missing '|| return 1'?!\n+7 \tdone ?!LINT: missing '|| return 1'?!\n 8 done &&\n 9 \n 10 for i in 0 1 2 3 4 5 6 7 8 9;\n@@ -18,7 +18,7 @@\n 19 do\n 20 \tfor j in 0 1 2 3 4 5 6 7 8 9;\n 21 \tdo\n-22 \t\techo \"$i$j\" >\"path$i$j\" ?!ERR missing '|| return 1'?!\n+22 \t\techo \"$i$j\" >\"path$i$j\" ?!LINT: missing '|| return 1'?!\n 23 \tdone || return 1\n 24 done &&\n 25 \ndiff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect\nindex c6891919c0..078c6f275f 100644\n--- a/t/chainlint/nested-subshell-comment.expect\n+++ b/t/chainlint/nested-subshell-comment.expect\n@@ -6,6 +6,6 @@\n 7 \t\t# minor numbers of cows (or do they?)\n 8 \t\tbaz &&\n 9 \t\tsnaff\n-10 \t) ?!ERR missing '&&'?!\n+10 \t) ?!LINT: missing '&&'?!\n 11 \tfuzzy\n 12 )\ndiff --git a/t/chainlint/nested-subshell.expect b/t/chainlint/nested-subshell.expect\nindex b98d723edf..a8d85d5d5b 100644\n--- a/t/chainlint/nested-subshell.expect\n+++ b/t/chainlint/nested-subshell.expect\n@@ -7,7 +7,7 @@\n 8 \n 9 \tcd foo &&\n 10 \t(\n-11 \t\techo a ?!ERR missing '&&'?!\n+11 \t\techo a ?!LINT: missing '&&'?!\n 12 \t\techo b\n 13 \t) >file\n 14 )\ndiff --git a/t/chainlint/not-heredoc.expect b/t/chainlint/not-heredoc.expect\nindex 9910621103..5d51705a7a 100644\n--- a/t/chainlint/not-heredoc.expect\n+++ b/t/chainlint/not-heredoc.expect\n@@ -9,6 +9,6 @@\n 10 \techo ourside &&\n 11 \techo \"=======\" &&\n 12 \techo theirside &&\n-13 \techo \">>>>>>> theirs\" ?!ERR missing '&&'?!\n+13 \techo \">>>>>>> theirs\" ?!LINT: missing '&&'?!\n 14 \tpoodle\n 15 ) >merged\ndiff --git a/t/chainlint/one-liner-for-loop.expect b/t/chainlint/one-liner-for-loop.expect\nindex 2eb2d5fcaf..e1fcbd3639 100644\n--- a/t/chainlint/one-liner-for-loop.expect\n+++ b/t/chainlint/one-liner-for-loop.expect\n@@ -3,7 +3,7 @@\n 4 \tcd dir-rename-and-content &&\n 5 \ttest_write_lines 1 2 3 4 5 >foo &&\n 6 \tmkdir olddir &&\n-7 \tfor i in a b c; do echo $i >olddir/$i; ?!ERR missing '|| exit 1'?! done ?!ERR missing '&&'?!\n+7 \tfor i in a b c; do echo $i >olddir/$i; ?!LINT: missing '|| exit 1'?! done ?!LINT: missing '&&'?!\n 8 \tgit add foo olddir &&\n 9 \tgit commit -m \"original\" &&\n 10 )\ndiff --git a/t/chainlint/one-liner.expect b/t/chainlint/one-liner.expect\nindex 2c5826e6c4..5deeb05070 100644\n--- a/t/chainlint/one-liner.expect\n+++ b/t/chainlint/one-liner.expect\n@@ -2,8 +2,8 @@\n 3 (foo && bar) |\n 4 (foo && bar) >baz &&\n 5 \n-6 (foo; ?!ERR missing '&&'?! bar) &&\n-7 (foo; ?!ERR missing '&&'?! bar) |\n-8 (foo; ?!ERR missing '&&'?! bar) >baz &&\n+6 (foo; ?!LINT: missing '&&'?! bar) &&\n+7 (foo; ?!LINT: missing '&&'?! bar) |\n+8 (foo; ?!LINT: missing '&&'?! bar) >baz &&\n 9 \n 10 (foo \"bar; baz\")\ndiff --git a/t/chainlint/pipe.expect b/t/chainlint/pipe.expect\nindex a198d5bdb2..d947c76584 100644\n--- a/t/chainlint/pipe.expect\n+++ b/t/chainlint/pipe.expect\n@@ -4,7 +4,7 @@\n 5 \tbaz &&\n 6 \n 7 \tfish |\n-8 \tcow ?!ERR missing '&&'?!\n+8 \tcow ?!LINT: missing '&&'?!\n 9 \n 10 \tsunder\n 11 )\ndiff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect\nindex e22920bf2c..2b499fbe70 100644\n--- a/t/chainlint/semicolon.expect\n+++ b/t/chainlint/semicolon.expect\n@@ -1,19 +1,19 @@\n 2 (\n-3 \tcat foo ; ?!ERR missing '&&'?! echo bar ?!ERR missing '&&'?!\n-4 \tcat foo ; ?!ERR missing '&&'?! echo bar\n+3 \tcat foo ; ?!LINT: missing '&&'?! echo bar ?!LINT: missing '&&'?!\n+4 \tcat foo ; ?!LINT: missing '&&'?! echo bar\n 5 ) &&\n 6 (\n-7 \tcat foo ; ?!ERR missing '&&'?! echo bar &&\n-8 \tcat foo ; ?!ERR missing '&&'?! echo bar\n+7 \tcat foo ; ?!LINT: missing '&&'?! echo bar &&\n+8 \tcat foo ; ?!LINT: missing '&&'?! echo bar\n 9 ) &&\n 10 (\n 11 \techo \"foo; bar\" &&\n-12 \tcat foo; ?!ERR missing '&&'?! echo bar\n+12 \tcat foo; ?!LINT: missing '&&'?! echo bar\n 13 ) &&\n 14 (\n 15 \tfoo;\n 16 ) &&\n 17 (cd foo &&\n 18 \tfor i in a b c; do\n-19 \t\techo; ?!ERR missing '|| exit 1'?!\n+19 \t\techo; ?!LINT: missing '|| exit 1'?!\n 20 \tdone)\ndiff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect\nindex 953d8084e5..e450caf948 100644\n--- a/t/chainlint/subshell-here-doc.expect\n+++ b/t/chainlint/subshell-here-doc.expect\n@@ -6,7 +6,7 @@\n 7 \tnevermore...\n 8 \tEOF\n 9 \n-10 \tcat <bip ?!ERR missing '&&'?!\n+10 \tcat <bip ?!LINT: missing '&&'?!\n 11 \tfish fly high\n 12 EOF\n 13 \ndiff --git a/t/chainlint/subshell-one-liner.expect b/t/chainlint/subshell-one-liner.expect\nindex f82296db66..265d996a21 100644\n--- a/t/chainlint/subshell-one-liner.expect\n+++ b/t/chainlint/subshell-one-liner.expect\n@@ -3,17 +3,17 @@\n 4 \t(foo && bar) |\n 5 \t(foo && bar) >baz &&\n 6 \n-7 \t(foo; ?!ERR missing '&&'?! bar) &&\n-8 \t(foo; ?!ERR missing '&&'?! bar) |\n-9 \t(foo; ?!ERR missing '&&'?! bar) >baz &&\n+7 \t(foo; ?!LINT: missing '&&'?! bar) &&\n+8 \t(foo; ?!LINT: missing '&&'?! bar) |\n+9 \t(foo; ?!LINT: missing '&&'?! bar) >baz &&\n 10 \n 11 \t(foo || exit 1) &&\n 12 \t(foo || exit 1) |\n 13 \t(foo || exit 1) >baz &&\n 14 \n-15 \t(foo && bar) ?!ERR missing '&&'?!\n+15 \t(foo && bar) ?!LINT: missing '&&'?!\n 16 \n-17 \t(foo && bar; ?!ERR missing '&&'?! baz) ?!ERR missing '&&'?!\n+17 \t(foo && bar; ?!LINT: missing '&&'?! baz) ?!LINT: missing '&&'?!\n 18 \n 19 \tfoobar\n 20 )\ndiff --git a/t/chainlint/token-pasting.expect b/t/chainlint/token-pasting.expect\nindex aa64cf75f3..387189b6de 100644\n--- a/t/chainlint/token-pasting.expect\n+++ b/t/chainlint/token-pasting.expect\n@@ -2,13 +2,13 @@\n 3 git config filter.rot13.clean ./rot13.sh &&\n 4 \n 5 {\n-6 echo \"*.t filter=rot13\" ?!ERR missing '&&'?!\n+6 echo \"*.t filter=rot13\" ?!LINT: missing '&&'?!\n 7 echo \"*.i ident\"\n 8 } >.gitattributes &&\n 9 \n 10 {\n-11 echo a b c d e f g h i j k l m ?!ERR missing '&&'?!\n-12 echo n o p q r s t u v w x y z ?!ERR missing '&&'?!\n+11 echo a b c d e f g h i j k l m ?!LINT: missing '&&'?!\n+12 echo n o p q r s t u v w x y z ?!LINT: missing '&&'?!\n 13 echo '$Id$'\n 14 } >test &&\n 15 cat test >test.t &&\n@@ -19,7 +19,7 @@\n 20 git checkout -- test test.t test.i &&\n 21 \n 22 echo \"content-test2\" >test2.o &&\n-23 echo \"content-test3 - filename with special characters\" >\"test3 'sq',$x=.o\" ?!ERR missing '&&'?!\n+23 echo \"content-test3 - filename with special characters\" >\"test3 'sq',$x=.o\" ?!LINT: missing '&&'?!\n 24 \n 25 downstream_url_for_sed=$(\n 26 \tprintf \"%sn\" \"$downstream_url\" |\ndiff --git a/t/chainlint/unclosed-here-doc-indent.expect b/t/chainlint/unclosed-here-doc-indent.expect\nindex d5b9ab52ee..156906c85a 100644\n--- a/t/chainlint/unclosed-here-doc-indent.expect\n+++ b/t/chainlint/unclosed-here-doc-indent.expect\n@@ -1,4 +1,4 @@\n 2 command_which_is_run &&\n-3 cat >expect <<-\\EOF ?!ERR unclosed heredoc?! &&\n+3 cat >expect <<-\\EOF ?!LINT: unclosed heredoc?! &&\n 4 we forget to end the here-doc\n 5 command_which_is_gobbled\ndiff --git a/t/chainlint/unclosed-here-doc.expect b/t/chainlint/unclosed-here-doc.expect\nindex 8f6d260544..752c608862 100644\n--- a/t/chainlint/unclosed-here-doc.expect\n+++ b/t/chainlint/unclosed-here-doc.expect\n@@ -1,5 +1,5 @@\n 2 command_which_is_run &&\n-3 cat >expect <<\\EOF ?!ERR unclosed heredoc?! &&\n+3 cat >expect <<\\EOF ?!LINT: unclosed heredoc?! &&\n 4 \twe try to end the here-doc below,\n 5 \tbut the indentation throws us off\n 6 \tsince the operator is not \"<<-\".\ndiff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect\nindex 1cfd17b3c2..2ba5582165 100644\n--- a/t/chainlint/while-loop.expect\n+++ b/t/chainlint/while-loop.expect\n@@ -1,14 +1,14 @@\n 2 (\n 3 \twhile true\n 4 \tdo\n-5 \t\techo foo ?!ERR missing '&&'?!\n-6 \t\tcat <<-\\EOF ?!ERR missing '|| exit 1'?!\n+5 \t\techo foo ?!LINT: missing '&&'?!\n+6 \t\tcat <<-\\EOF ?!LINT: missing '|| exit 1'?!\n 7 \t\tbar\n 8 \t\tEOF\n-9 \tdone ?!ERR missing '&&'?!\n+9 \tdone ?!LINT: missing '&&'?!\n 10 \n 11 \twhile true; do\n 12 \t\techo foo &&\n-13 \t\tcat bar ?!ERR missing '|| exit 1'?!\n+13 \t\tcat bar ?!LINT: missing '|| exit 1'?!\n 14 \tdone\n 15 )\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex b652cb98cd..278d1215f1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1606,7 +1606,7 @@ if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0 &&\n test \"${GIT_TEST_EXT_CHAIN_LINT:-1}\" != 0\n then\n \t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n-\t\tBUG \"lint error (see 'ERR' annotations above)\"\n+\t\tBUG \"lint error (see 'LINT' annotations above)\"\n fi\n \n # Last-minute variable setup\n\n\nRange-diff against v1:\n2: 93305d0bdf ! 1: 260a877ce1 chainlint: reduce annotation noise-factor\n @@ Metadata\n Author: Eric Sunshine \n \n ## Commit message ##\n - chainlint: reduce annotation noise-factor\n + chainlint: don't be fooled by \"?!...?!\" in test body\n \n - When chainlint detects a problem in a test definition, it highlights the\n - offending code with an \"?!ERR ...?!\" annotation. The rather curious \"?!\"\n - delimiter was chosen to draw the reader's attention to the problem area.\n + As originally implemented, chainlint did not collect structured\n + information about detected problems. Instead, it merely emitted raw\n + parse tokens (not the original test text), along with a \"?!...?!\"\n + annotation directly into the output stream each time a problem was\n + discovered. In order to report statistics (in --stats mode) and to\n + adjust its exit code to indicate success or failure, it merely counts\n + the number of times \"?!...?!\" appears in the output stream. An obvious\n + shortcoming of this approach is that it can be fooled by a legitimate\n + \"?!...?!\" sequence in the body of a test (though, only if an actual\n + problem is detected in the test).\n \n - Later, chainlint learned to color its output when sent to a terminal.\n - Problem annotations are colored with a red background which stands out\n - well from surrounding text, thus easily draws the reader's attention. As\n - such, the additional \"?!\" decoration became superfluous (when output is\n - colored), however the decoration was retained since it serves as a good\n - needle when using the terminal's search feature to \"jump\" to the next\n - problem.\n + The situation did not improve when 7c04aa7390 (chainlint: colorize\n + problem annotations and test delimiters, 2022-09-13) colored the\n + annotations after-the-fact by searching for \"?!...?!\" in the output\n + stream and inserting color codes. As above, a shortcoming is that this\n + approach can incorrectly color a legitimate \"?!...?!\" sequence in a test\n + body as if it is an error.\n \n - Nevertheless, the \"?!\" decoration is noisy and ugly and makes it\n - unnecessarily difficult for the reader to pluck the problem description\n - from the annotation. For instance, it is easier to see at a glance what\n - the problem is in:\n + However, when 73c768dae9 (chainlint: annotate original test definition\n + rather than token stream, 2022-11-08) taught chainlint to output the\n + original test text verbatim, it started collecting structured\n + information about detected problems.\n \n - ERR missing '&&'\n -\n - than in the noisier:\n -\n - ?!ERR missing '&&'?!\n -\n - Therefore drop the \"!?\" decoration when output is colored (but retain it\n - otherwise).\n -\n - Note that the preceding change gave all problem annotations a uniform\n - \"ERR\" prefix which serves as a reasonably suitable replacement needle\n - when searching in a terminal, so loss of \"?!\" in the output should not\n - be overly problematic.\n + Now that it is available, take advantage of the structured problem\n + information to deterministically count the number of problems detected\n + and to color the annotations directly, rather than scanning the output\n + stream for \"?!...?!\" and performing these operations after-the-fact.\n \n Signed-off-by: Eric Sunshine \n \n @@ t/chainlint.pl: sub check_test {\n +\t$self->{nerrs} += @$problems;\n \treturn unless $emit_all || @$problems;\n \tmy $c = main::fd_colors(1);\n -+\tmy ($erropen, $errclose) = -t 1 ? (\"$c->{rev}$c->{red}\", $c->{reset}) : ('?!', '?!');\n \tmy $start = 0;\n - \tmy $checked = '';\n - \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n @@ t/chainlint.pl: sub check_test {\n + \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n + \t\tmy ($label, $token) = @$_;\n + \t\tmy $pos = $token->[2];\n +-\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!$label?! \";\n ++\t\t$checked .= substr($body, $start, $pos - $start);\n ++\t\t$checked .= ' ' unless $checked =~ /\\s$/;\n ++\t\t$checked .= \"$c->{rev}$c->{red}?!$label?!$c->{reset}\";\n ++\t\t$checked .= ' ' unless $pos >= length($body) ||\n ++\t\t substr($body, $pos, 1) =~ /^\\s/;\n + \t\t$start = $pos;\n + \t}\n + \t$checked .= substr($body, $start);\n + \t$checked =~ s/^/$lineno++ . ' '/mge;\n \t$checked =~ s/^\\d+ \\n//;\n - \t$checked =~ s/(\\s) \\?!/$1?!/mg;\n - \t$checked =~ s/\\?! (\\s)/?!$1/mg;\n +-\t$checked =~ s/(\\s) \\?!/$1?!/mg;\n +-\t$checked =~ s/\\?! (\\s)/?!$1/mg;\n -\t$checked =~ s/(\\?![^?]+\\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg;\n -+\t$checked =~ s/\\?!([^?]+)\\?!/$erropen$1$errclose/mg;\n \t$checked =~ s/^\\d+/$c->{dim}$&$c->{reset}/mg;\n \t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n \tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\n @@ t/chainlint.pl: sub check_script {\n \t}\n \treturn [$id, $nscripts, $ntests, $nerrs];\n }\n -\n - ## t/test-lib.sh ##\n -@@ t/test-lib.sh: if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0 &&\n - test \"${GIT_TEST_EXT_CHAIN_LINT:-1}\" != 0\n - then\n - \t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n --\t\tBUG \"lint error (see '?!...!? annotations above)\"\n -+\t\tBUG \"lint error (see 'ERR' annotations above)\"\n - fi\n - \n - # Last-minute variable setup\n1: 0530fc13c6 ! 2: 5a4c1bd31a chainlint: make error messages self-explanatory\n @@ Commit message\n may understand that \"?!AMP?!\" is an abbreviation of \"ampersand\" and\n indicates a break in the &&-chain, this may not be obvious to newcomers.\n \n - Similarly, although the annotation \"?!LOOP?!\" is understood by project\n - regulars to indicate a missing `|| return 1` (or `|| exit 1` in a\n - subshell), newcomers may find it more than a little perplexing. The\n - \"?!LOOP?!\" case is particularly serious since it is likely that some\n - newcomers are unaware that shell loops do not terminate automatically\n - upon error, and it is more difficult for a newcomer to figure out how to\n - correct the problem by examining surrounding code since `|| return 1`\n - appears in test scrips relatively infrequently (compared, for instance,\n - with &&-chaining).\n + The \"?!LOOP?!\" case is particularly serious because that terse single\n + word does nothing to convey that the loop body should end with\n + \"|| return 1\" (or \"|| exit 1\" in a subshell) to ensure that a failing\n + command in the body aborts the loop immediately. Moreover, unlike\n + &&-chaining which is ubiquitous in Git tests, the \"|| return 1\" idiom is\n + relatively infrequent, thus may be harder for a newcomer to discover by\n + consulting nearby code.\n \n - Address these shortcomings by emitting human-consumable messages which\n + Address these shortcomings by emitting human-readable messages which\n both explain the problem and give a strong hint about how to correct it.\n \n Signed-off-by: Eric Sunshine \n @@ t/chainlint.pl\n # or globs referencing a collection of scripts. For each problem discovered,\n # the pathname of the script containing the test is printed along with the test\n -# name and the test body with a `?!FOO?!` annotation at the location of each\n -+# name and the test body with a `?!ERR?!` annotation at the location of each\n - # detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n - # &&-chain. Returns zero if no problems are discovered, otherwise non-zero.\n +-# detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n +-# &&-chain. Returns zero if no problems are discovered, otherwise non-zero.\n ++# name and the test body with a `?!LINT: ...?!` annotation at the location of\n ++# each detected problem, where \"...\" is an explanation of the problem. Returns\n ++# zero if no problems are discovered, otherwise non-zero.\n \n + use warnings;\n + use strict;\n @@ t/chainlint.pl: sub swallow_heredocs {\n \t\t\t$self->{lineno} += () = $body =~ /\\n/sg;\n \t\t\tnext;\n @@ t/chainlint.pl: sub check_test {\n \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n \t\tmy ($label, $token) = @$_;\n \t\tmy $pos = $token->[2];\n --\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!$label?! \";\n -+\t\tmy $err = format_problem($label, $token);\n -+\t\t$checked .= substr($body, $start, $pos - $start) . \" ?!ERR $err?! \";\n ++\t\tmy $err = format_problem($label);\n + \t\t$checked .= substr($body, $start, $pos - $start);\n + \t\t$checked .= ' ' unless $checked =~ /\\s$/;\n +-\t\t$checked .= \"$c->{rev}$c->{red}?!$label?!$c->{reset}\";\n ++\t\t$checked .= \"$c->{rev}$c->{red}?!LINT: $err?!$c->{reset}\";\n + \t\t$checked .= ' ' unless $pos >= length($body) ||\n + \t\t substr($body, $pos, 1) =~ /^\\s/;\n \t\t$start = $pos;\n - \t}\n - \t$checked .= substr($body, $start);\n \n ## t/chainlint/arithmetic-expansion.expect ##\n @@\n @@ t/chainlint/arithmetic-expansion.expect\n 6 ) &&\n 7 (\n -8 \tbar=$((42 + 1)) ?!AMP?!\n -+8 \tbar=$((42 + 1)) ?!ERR missing '&&'?!\n ++8 \tbar=$((42 + 1)) ?!LINT: missing '&&'?!\n 9 \tbaz\n 10 )\n \n @@ t/chainlint/block.expect\n 3 \tfoo &&\n 4 \t{\n -5 \t\techo a ?!AMP?!\n -+5 \t\techo a ?!ERR missing '&&'?!\n ++5 \t\techo a ?!LINT: missing '&&'?!\n 6 \t\techo b\n 7 \t} &&\n 8 \tbar &&\n 9 \t{\n 10 \t\techo c\n -11 \t} ?!AMP?!\n -+11 \t} ?!ERR missing '&&'?!\n ++11 \t} ?!LINT: missing '&&'?!\n 12 \tbaz\n 13 ) &&\n 14 \n 15 {\n -16 \techo a; ?!AMP?! echo b\n -+16 \techo a; ?!ERR missing '&&'?! echo b\n ++16 \techo a; ?!LINT: missing '&&'?! echo b\n 17 } &&\n -18 { echo a; ?!AMP?! echo b; } &&\n -+18 { echo a; ?!ERR missing '&&'?! echo b; } &&\n ++18 { echo a; ?!LINT: missing '&&'?! echo b; } &&\n 19 \n 20 {\n 21 \techo \"${var}9\" &&\n @@ t/chainlint/broken-chain.expect\n 2 (\n 3 \tfoo &&\n -4 \tbar ?!AMP?!\n -+4 \tbar ?!ERR missing '&&'?!\n ++4 \tbar ?!LINT: missing '&&'?!\n 5 \tbaz &&\n 6 \twop\n 7 )\n @@ t/chainlint/case.expect\n 11 \tx) foo ;;\n 12 \t*) bar ;;\n -13 \tesac ?!AMP?!\n -+13 \tesac ?!ERR missing '&&'?!\n ++13 \tesac ?!LINT: missing '&&'?!\n 14 \tfoobar\n 15 ) &&\n 16 (\n 17 \tcase \"$x\" in 1) true;; esac &&\n -18 \tcase \"$y\" in 2) false;; esac ?!AMP?!\n -+18 \tcase \"$y\" in 2) false;; esac ?!ERR missing '&&'?!\n ++18 \tcase \"$y\" in 2) false;; esac ?!LINT: missing '&&'?!\n 19 \tfoobar\n 20 )\n \n @@ t/chainlint/chain-break-false.expect\n 6 \tfalse\n 7 else\n -8 \techo it went okay ?!AMP?!\n -+8 \techo it went okay ?!ERR missing '&&'?!\n ++8 \techo it went okay ?!LINT: missing '&&'?!\n 9 \tcongratulate user\n 10 fi\n \n @@ t/chainlint/chained-block.expect\n @@\n 2 echo nobody home && {\n -3 \ttest the doohicky ?!AMP?!\n -+3 \ttest the doohicky ?!ERR missing '&&'?!\n ++3 \ttest the doohicky ?!LINT: missing '&&'?!\n 4 \tright now\n 5 } &&\n 6 \n @@ t/chainlint/chained-subshell.expect\n 2 mkdir sub && (\n 3 \tcd sub &&\n -4 \tfoo the bar ?!AMP?!\n -+4 \tfoo the bar ?!ERR missing '&&'?!\n ++4 \tfoo the bar ?!LINT: missing '&&'?!\n 5 \tnuff said\n 6 ) &&\n 7 \n 8 cut \"-d \" -f actual | (read s1 s2 s3 &&\n -9 test -f $s1 ?!AMP?!\n -+9 test -f $s1 ?!ERR missing '&&'?!\n ++9 test -f $s1 ?!LINT: missing '&&'?!\n 10 test $(cat $s2) = tree2path1 &&\n 11 test $(cat $s3) = tree3path1)\n \n @@ t/chainlint/command-substitution.expect\n 6 ) &&\n 7 (\n -8 \tbar=$(gobble blocks) ?!AMP?!\n -+8 \tbar=$(gobble blocks) ?!ERR missing '&&'?!\n ++8 \tbar=$(gobble blocks) ?!LINT: missing '&&'?!\n 9 \tbaz\n 10 )\n \n @@ t/chainlint/complex-if-in-cuddled-loop.expect\n 6 else\n 7 echo >file\n -8 fi ?!LOOP?!\n -+8 fi ?!ERR missing '|| exit 1'?!\n ++8 fi ?!LINT: missing '|| exit 1'?!\n 9 done) &&\n 10 test ! -f file\n \n @@ t/chainlint/cuddled.expect\n 4 ) &&\n 5 \n -6 (cd foo ?!AMP?!\n -+6 (cd foo ?!ERR missing '&&'?!\n ++6 (cd foo ?!LINT: missing '&&'?!\n 7 \tbar\n 8 ) &&\n 9 \n @@ t/chainlint/cuddled.expect\n 15 \tbar) &&\n 16 \n -17 (cd foo ?!AMP?!\n -+17 (cd foo ?!ERR missing '&&'?!\n ++17 (cd foo ?!LINT: missing '&&'?!\n 18 \tbar)\n \n ## t/chainlint/for-loop.expect ##\n @@ t/chainlint/for-loop.expect\n 4 \tdo\n -5 \t\techo $i ?!AMP?!\n -6 \t\tcat <<-\\EOF ?!LOOP?!\n -+5 \t\techo $i ?!ERR missing '&&'?!\n -+6 \t\tcat <<-\\EOF ?!ERR missing '|| exit 1'?!\n ++5 \t\techo $i ?!LINT: missing '&&'?!\n ++6 \t\tcat <<-\\EOF ?!LINT: missing '|| exit 1'?!\n 7 \t\tbar\n 8 \t\tEOF\n -9 \tdone ?!AMP?!\n -+9 \tdone ?!ERR missing '&&'?!\n ++9 \tdone ?!LINT: missing '&&'?!\n 10 \n 11 \tfor i in a b c; do\n 12 \t\techo $i &&\n -13 \t\tcat $i ?!LOOP?!\n -+13 \t\tcat $i ?!ERR missing '|| exit 1'?!\n ++13 \t\tcat $i ?!LINT: missing '|| exit 1'?!\n 14 \tdone\n 15 )\n \n @@ t/chainlint/function.expect\n 6 remove_object() {\n 7 \tfile=$(sha1_file \"$*\") &&\n -8 \ttest -e \"$file\" ?!AMP?!\n -+8 \ttest -e \"$file\" ?!ERR missing '&&'?!\n ++8 \ttest -e \"$file\" ?!LINT: missing '&&'?!\n 9 \trm -f \"$file\"\n -10 } ?!AMP?!\n -+10 } ?!ERR missing '&&'?!\n ++10 } ?!LINT: missing '&&'?!\n 11 \n 12 sha1_file arg && remove_object arg\n \n ## t/chainlint/here-doc-body-indent.expect ##\n @@\n -2 \techo \"we should find this\" ?!AMP?!\n -+2 \techo \"we should find this\" ?!ERR missing '&&'?!\n ++2 \techo \"we should find this\" ?!LINT: missing '&&'?!\n 3 \techo \"even though our heredoc has its indent stripped\"\n \n ## t/chainlint/here-doc-body-pathological.expect ##\n @@\n -2 \techo \"outer here-doc does not allow indented end-tag\" ?!AMP?!\n -+2 \techo \"outer here-doc does not allow indented end-tag\" ?!ERR missing '&&'?!\n ++2 \techo \"outer here-doc does not allow indented end-tag\" ?!LINT: missing '&&'?!\n 3 \tcat >file <<-\\EOF &&\n 4 \tbut this inner here-doc\n 5 \tdoes allow indented EOF\n 6 \tEOF\n -7 \techo \"missing chain after\" ?!AMP?!\n -+7 \techo \"missing chain after\" ?!ERR missing '&&'?!\n ++7 \techo \"missing chain after\" ?!LINT: missing '&&'?!\n 8 \techo \"but this line is OK because it's the end\"\n \n ## t/chainlint/here-doc-body.expect ##\n @@\n -2 \techo \"missing chain before\" ?!AMP?!\n -+2 \techo \"missing chain before\" ?!ERR missing '&&'?!\n ++2 \techo \"missing chain before\" ?!LINT: missing '&&'?!\n 3 \tcat >file <<-\\EOF &&\n 4 \tinside inner here-doc\n 5 \tthese are not shell commands\n 6 \tEOF\n -7 \techo \"missing chain after\" ?!AMP?!\n -+7 \techo \"missing chain after\" ?!ERR missing '&&'?!\n ++7 \techo \"missing chain after\" ?!LINT: missing '&&'?!\n 8 \techo \"but this line is OK because it's the end\"\n \n ## t/chainlint/here-doc-double.expect ##\n @@\n -8 \techo \"actual test commands\" ?!AMP?!\n -+8 \techo \"actual test commands\" ?!ERR missing '&&'?!\n ++8 \techo \"actual test commands\" ?!LINT: missing '&&'?!\n 9 \techo \"that should be checked\"\n \n ## t/chainlint/here-doc-indent-operator.expect ##\n @@ t/chainlint/here-doc-indent-operator.expect\n 6 EOF\n 7 \n -8 cat >expect << -EOF ?!AMP?!\n -+8 cat >expect << -EOF ?!ERR missing '&&'?!\n ++8 cat >expect << -EOF ?!LINT: missing '&&'?!\n 9 this is not indented\n 10 -EOF\n 11 \n @@ t/chainlint/here-doc-multi-line-command-subst.expect\n 5 \t\tvegetable\n 6 \t\tEND\n -7 \t\twiffle) ?!AMP?!\n -+7 \t\twiffle) ?!ERR missing '&&'?!\n ++7 \t\twiffle) ?!LINT: missing '&&'?!\n 8 \techo $x\n 9 )\n \n @@ t/chainlint/here-doc-multi-line-string.expect\n 2 (\n 3 \tcat <<-\\TXT && echo \"multi-line\n -4 \tstring\" ?!AMP?!\n -+4 \tstring\" ?!ERR missing '&&'?!\n ++4 \tstring\" ?!LINT: missing '&&'?!\n 5 \tfizzle\n 6 \tTXT\n 7 \tbap\n @@ t/chainlint/if-condition-split.expect\n 4 kevin\n 5 then\n -6 \techo \"nomads\" ?!AMP?!\n -+6 \techo \"nomads\" ?!ERR missing '&&'?!\n ++6 \techo \"nomads\" ?!LINT: missing '&&'?!\n 7 \techo \"for sure\"\n 8 fi\n \n @@ t/chainlint/if-in-loop.expect\n 7 \t\t\techo \"err\"\n 8 \t\t\texit 1\n -9 \t\tfi ?!AMP?!\n -+9 \t\tfi ?!ERR missing '&&'?!\n ++9 \t\tfi ?!LINT: missing '&&'?!\n 10 \t\tfoo\n -11 \tdone ?!AMP?!\n -+11 \tdone ?!ERR missing '&&'?!\n ++11 \tdone ?!LINT: missing '&&'?!\n 12 \tbar\n 13 )\n \n @@ t/chainlint/if-then-else.expect\n 3 \tif test -n \"\"\n 4 \tthen\n -5 \t\techo very ?!AMP?!\n -+5 \t\techo very ?!ERR missing '&&'?!\n ++5 \t\techo very ?!LINT: missing '&&'?!\n 6 \t\techo empty\n 7 \telif test -z \"\"\n 8 \tthen\n @@ t/chainlint/if-then-else.expect\n 13 \t\tbar\n 14 \t\tEOF\n -15 \tfi ?!AMP?!\n -+15 \tfi ?!ERR missing '&&'?!\n ++15 \tfi ?!LINT: missing '&&'?!\n 16 \techo poodle\n 17 ) &&\n 18 (\n @@ t/chainlint/inline-comment.expect\n 2 (\n 3 \tfoobar && # comment 1\n -4 \tbarfoo ?!AMP?! # wrong position for &&\n -+4 \tbarfoo ?!ERR missing '&&'?! # wrong position for &&\n ++4 \tbarfoo ?!LINT: missing '&&'?! # wrong position for &&\n 5 \tflibble \"not a # comment\"\n 6 ) &&\n 7 \n @@ t/chainlint/loop-detect-failure.expect\n 13 \tprintf \"%\"$n\"s\" X > r2/large.$n &&\n 14 \tgit -C r2 add large.$n &&\n -15 \tgit -C r2 commit -m \"$n\" ?!LOOP?!\n -+15 \tgit -C r2 commit -m \"$n\" ?!ERR missing '|| return 1'?!\n ++15 \tgit -C r2 commit -m \"$n\" ?!LINT: missing '|| return 1'?!\n 16 done\n \n ## t/chainlint/loop-in-if.expect ##\n @@ t/chainlint/loop-in-if.expect\n -7 \t\t\techo \"pop\" ?!AMP?!\n -8 \t\t\techo \"glup\" ?!LOOP?!\n -9 \t\tdone ?!AMP?!\n -+7 \t\t\techo \"pop\" ?!ERR missing '&&'?!\n -+8 \t\t\techo \"glup\" ?!ERR missing '|| exit 1'?!\n -+9 \t\tdone ?!ERR missing '&&'?!\n ++7 \t\t\techo \"pop\" ?!LINT: missing '&&'?!\n ++8 \t\t\techo \"glup\" ?!LINT: missing '|| exit 1'?!\n ++9 \t\tdone ?!LINT: missing '&&'?!\n 10 \t\tfoo\n -11 \tfi ?!AMP?!\n -+11 \tfi ?!ERR missing '&&'?!\n ++11 \tfi ?!LINT: missing '&&'?!\n 12 \tbar\n 13 )\n \n @@ t/chainlint/multi-line-string.expect\n 5 \t\tline 3\" &&\n 6 \ty=\"line 1\n -7 \t\tline2\" ?!AMP?!\n -+7 \t\tline2\" ?!ERR missing '&&'?!\n ++7 \t\tline2\" ?!LINT: missing '&&'?!\n 8 \tfoobar\n 9 ) &&\n 10 (\n @@ t/chainlint/negated-one-liner.expect\n 4 \n -5 ! (foo; ?!AMP?! bar) &&\n -6 ! (foo; ?!AMP?! bar) >baz\n -+5 ! (foo; ?!ERR missing '&&'?! bar) &&\n -+6 ! (foo; ?!ERR missing '&&'?! bar) >baz\n ++5 ! (foo; ?!LINT: missing '&&'?! bar) &&\n ++6 ! (foo; ?!LINT: missing '&&'?! bar) >baz\n \n ## t/chainlint/nested-cuddled-subshell.expect ##\n @@\n @@ t/chainlint/nested-cuddled-subshell.expect\n 7 \t(cd foo &&\n 8 \t\tbar\n -9 \t) ?!AMP?!\n -+9 \t) ?!ERR missing '&&'?!\n ++9 \t) ?!LINT: missing '&&'?!\n 10 \n 11 \t(\n 12 \t\tcd foo &&\n @@ t/chainlint/nested-cuddled-subshell.expect\n 15 \t(\n 16 \t\tcd foo &&\n -17 \t\tbar) ?!AMP?!\n -+17 \t\tbar) ?!ERR missing '&&'?!\n ++17 \t\tbar) ?!LINT: missing '&&'?!\n 18 \n 19 \t(cd foo &&\n 20 \t\tbar) &&\n 21 \n 22 \t(cd foo &&\n -23 \t\tbar) ?!AMP?!\n -+23 \t\tbar) ?!ERR missing '&&'?!\n ++23 \t\tbar) ?!LINT: missing '&&'?!\n 24 \n 25 \tfoobar\n 26 )\n @@ t/chainlint/nested-here-doc.expect\n 20 \tINPUT_END\n 21 \n -22 \tcat <<-\\EOT ?!AMP?!\n -+22 \tcat <<-\\EOT ?!ERR missing '&&'?!\n ++22 \tcat <<-\\EOT ?!LINT: missing '&&'?!\n 23 \ttext goes here\n 24 \tdata <\"path$i$j\" ?!LOOP?!\n -7 \tdone ?!LOOP?!\n -+6 \t\techo \"$i$j\" >\"path$i$j\" ?!ERR missing '|| return 1'?!\n -+7 \tdone ?!ERR missing '|| return 1'?!\n ++6 \t\techo \"$i$j\" >\"path$i$j\" ?!LINT: missing '|| return 1'?!\n ++7 \tdone ?!LINT: missing '|| return 1'?!\n 8 done &&\n 9 \n 10 for i in 0 1 2 3 4 5 6 7 8 9;\n @@ t/chainlint/nested-loop-detect-failure.expect\n 20 \tfor j in 0 1 2 3 4 5 6 7 8 9;\n 21 \tdo\n -22 \t\techo \"$i$j\" >\"path$i$j\" ?!LOOP?!\n -+22 \t\techo \"$i$j\" >\"path$i$j\" ?!ERR missing '|| return 1'?!\n ++22 \t\techo \"$i$j\" >\"path$i$j\" ?!LINT: missing '|| return 1'?!\n 23 \tdone || return 1\n 24 done &&\n 25 \n @@ t/chainlint/nested-subshell-comment.expect\n 8 \t\tbaz &&\n 9 \t\tsnaff\n -10 \t) ?!AMP?!\n -+10 \t) ?!ERR missing '&&'?!\n ++10 \t) ?!LINT: missing '&&'?!\n 11 \tfuzzy\n 12 )\n \n @@ t/chainlint/nested-subshell.expect\n 9 \tcd foo &&\n 10 \t(\n -11 \t\techo a ?!AMP?!\n -+11 \t\techo a ?!ERR missing '&&'?!\n ++11 \t\techo a ?!LINT: missing '&&'?!\n 12 \t\techo b\n 13 \t) >file\n 14 )\n @@ t/chainlint/not-heredoc.expect\n 11 \techo \"=======\" &&\n 12 \techo theirside &&\n -13 \techo \">>>>>>> theirs\" ?!AMP?!\n -+13 \techo \">>>>>>> theirs\" ?!ERR missing '&&'?!\n ++13 \techo \">>>>>>> theirs\" ?!LINT: missing '&&'?!\n 14 \tpoodle\n 15 ) >merged\n \n @@ t/chainlint/one-liner-for-loop.expect\n 5 \ttest_write_lines 1 2 3 4 5 >foo &&\n 6 \tmkdir olddir &&\n -7 \tfor i in a b c; do echo $i >olddir/$i; ?!LOOP?! done ?!AMP?!\n -+7 \tfor i in a b c; do echo $i >olddir/$i; ?!ERR missing '|| exit 1'?! done ?!ERR missing '&&'?!\n ++7 \tfor i in a b c; do echo $i >olddir/$i; ?!LINT: missing '|| exit 1'?! done ?!LINT: missing '&&'?!\n 8 \tgit add foo olddir &&\n 9 \tgit commit -m \"original\" &&\n 10 )\n @@ t/chainlint/one-liner.expect\n -6 (foo; ?!AMP?! bar) &&\n -7 (foo; ?!AMP?! bar) |\n -8 (foo; ?!AMP?! bar) >baz &&\n -+6 (foo; ?!ERR missing '&&'?! bar) &&\n -+7 (foo; ?!ERR missing '&&'?! bar) |\n -+8 (foo; ?!ERR missing '&&'?! bar) >baz &&\n ++6 (foo; ?!LINT: missing '&&'?! bar) &&\n ++7 (foo; ?!LINT: missing '&&'?! bar) |\n ++8 (foo; ?!LINT: missing '&&'?! bar) >baz &&\n 9 \n 10 (foo \"bar; baz\")\n \n @@ t/chainlint/pipe.expect\n 6 \n 7 \tfish |\n -8 \tcow ?!AMP?!\n -+8 \tcow ?!ERR missing '&&'?!\n ++8 \tcow ?!LINT: missing '&&'?!\n 9 \n 10 \tsunder\n 11 )\n @@ t/chainlint/semicolon.expect\n 2 (\n -3 \tcat foo ; ?!AMP?! echo bar ?!AMP?!\n -4 \tcat foo ; ?!AMP?! echo bar\n -+3 \tcat foo ; ?!ERR missing '&&'?! echo bar ?!ERR missing '&&'?!\n -+4 \tcat foo ; ?!ERR missing '&&'?! echo bar\n ++3 \tcat foo ; ?!LINT: missing '&&'?! echo bar ?!LINT: missing '&&'?!\n ++4 \tcat foo ; ?!LINT: missing '&&'?! echo bar\n 5 ) &&\n 6 (\n -7 \tcat foo ; ?!AMP?! echo bar &&\n -8 \tcat foo ; ?!AMP?! echo bar\n -+7 \tcat foo ; ?!ERR missing '&&'?! echo bar &&\n -+8 \tcat foo ; ?!ERR missing '&&'?! echo bar\n ++7 \tcat foo ; ?!LINT: missing '&&'?! echo bar &&\n ++8 \tcat foo ; ?!LINT: missing '&&'?! echo bar\n 9 ) &&\n 10 (\n 11 \techo \"foo; bar\" &&\n -12 \tcat foo; ?!AMP?! echo bar\n -+12 \tcat foo; ?!ERR missing '&&'?! echo bar\n ++12 \tcat foo; ?!LINT: missing '&&'?! echo bar\n 13 ) &&\n 14 (\n 15 \tfoo;\n @@ t/chainlint/semicolon.expect\n 17 (cd foo &&\n 18 \tfor i in a b c; do\n -19 \t\techo; ?!LOOP?!\n -+19 \t\techo; ?!ERR missing '|| exit 1'?!\n ++19 \t\techo; ?!LINT: missing '|| exit 1'?!\n 20 \tdone)\n \n ## t/chainlint/subshell-here-doc.expect ##\n @@ t/chainlint/subshell-here-doc.expect\n 8 \tEOF\n 9 \n -10 \tcat <bip ?!AMP?!\n -+10 \tcat <bip ?!ERR missing '&&'?!\n ++10 \tcat <bip ?!LINT: missing '&&'?!\n 11 \tfish fly high\n 12 EOF\n 13 \n @@ t/chainlint/subshell-one-liner.expect\n -7 \t(foo; ?!AMP?! bar) &&\n -8 \t(foo; ?!AMP?! bar) |\n -9 \t(foo; ?!AMP?! bar) >baz &&\n -+7 \t(foo; ?!ERR missing '&&'?! bar) &&\n -+8 \t(foo; ?!ERR missing '&&'?! bar) |\n -+9 \t(foo; ?!ERR missing '&&'?! bar) >baz &&\n ++7 \t(foo; ?!LINT: missing '&&'?! bar) &&\n ++8 \t(foo; ?!LINT: missing '&&'?! bar) |\n ++9 \t(foo; ?!LINT: missing '&&'?! bar) >baz &&\n 10 \n 11 \t(foo || exit 1) &&\n 12 \t(foo || exit 1) |\n 13 \t(foo || exit 1) >baz &&\n 14 \n -15 \t(foo && bar) ?!AMP?!\n -+15 \t(foo && bar) ?!ERR missing '&&'?!\n ++15 \t(foo && bar) ?!LINT: missing '&&'?!\n 16 \n -17 \t(foo && bar; ?!AMP?! baz) ?!AMP?!\n -+17 \t(foo && bar; ?!ERR missing '&&'?! baz) ?!ERR missing '&&'?!\n ++17 \t(foo && bar; ?!LINT: missing '&&'?! baz) ?!LINT: missing '&&'?!\n 18 \n 19 \tfoobar\n 20 )\n @@ t/chainlint/token-pasting.expect\n 4 \n 5 {\n -6 echo \"*.t filter=rot13\" ?!AMP?!\n -+6 echo \"*.t filter=rot13\" ?!ERR missing '&&'?!\n ++6 echo \"*.t filter=rot13\" ?!LINT: missing '&&'?!\n 7 echo \"*.i ident\"\n 8 } >.gitattributes &&\n 9 \n 10 {\n -11 echo a b c d e f g h i j k l m ?!AMP?!\n -12 echo n o p q r s t u v w x y z ?!AMP?!\n -+11 echo a b c d e f g h i j k l m ?!ERR missing '&&'?!\n -+12 echo n o p q r s t u v w x y z ?!ERR missing '&&'?!\n ++11 echo a b c d e f g h i j k l m ?!LINT: missing '&&'?!\n ++12 echo n o p q r s t u v w x y z ?!LINT: missing '&&'?!\n 13 echo '$Id$'\n 14 } >test &&\n 15 cat test >test.t &&\n @@ t/chainlint/token-pasting.expect\n 21 \n 22 echo \"content-test2\" >test2.o &&\n -23 echo \"content-test3 - filename with special characters\" >\"test3 'sq',$x=.o\" ?!AMP?!\n -+23 echo \"content-test3 - filename with special characters\" >\"test3 'sq',$x=.o\" ?!ERR missing '&&'?!\n ++23 echo \"content-test3 - filename with special characters\" >\"test3 'sq',$x=.o\" ?!LINT: missing '&&'?!\n 24 \n 25 downstream_url_for_sed=$(\n 26 \tprintf \"%sn\" \"$downstream_url\" |\n @@ t/chainlint/unclosed-here-doc-indent.expect\n @@\n 2 command_which_is_run &&\n -3 cat >expect <<-\\EOF ?!UNCLOSED-HEREDOC?! &&\n -+3 cat >expect <<-\\EOF ?!ERR unclosed heredoc?! &&\n ++3 cat >expect <<-\\EOF ?!LINT: unclosed heredoc?! &&\n 4 we forget to end the here-doc\n 5 command_which_is_gobbled\n \n @@ t/chainlint/unclosed-here-doc.expect\n @@\n 2 command_which_is_run &&\n -3 cat >expect <<\\EOF ?!UNCLOSED-HEREDOC?! &&\n -+3 cat >expect <<\\EOF ?!ERR unclosed heredoc?! &&\n ++3 cat >expect <<\\EOF ?!LINT: unclosed heredoc?! &&\n 4 \twe try to end the here-doc below,\n 5 \tbut the indentation throws us off\n 6 \tsince the operator is not \"<<-\".\n @@ t/chainlint/while-loop.expect\n 4 \tdo\n -5 \t\techo foo ?!AMP?!\n -6 \t\tcat <<-\\EOF ?!LOOP?!\n -+5 \t\techo foo ?!ERR missing '&&'?!\n -+6 \t\tcat <<-\\EOF ?!ERR missing '|| exit 1'?!\n ++5 \t\techo foo ?!LINT: missing '&&'?!\n ++6 \t\tcat <<-\\EOF ?!LINT: missing '|| exit 1'?!\n 7 \t\tbar\n 8 \t\tEOF\n -9 \tdone ?!AMP?!\n -+9 \tdone ?!ERR missing '&&'?!\n ++9 \tdone ?!LINT: missing '&&'?!\n 10 \n 11 \twhile true; do\n 12 \t\techo foo &&\n -13 \t\tcat bar ?!LOOP?!\n -+13 \t\tcat bar ?!ERR missing '|| exit 1'?!\n ++13 \t\tcat bar ?!LINT: missing '|| exit 1'?!\n 14 \tdone\n 15 )\n-: ---------- > 3: eee1e7fac7 chainlint: reduce annotation noise-factor\n-- \n2.46.0\n\n"},{"id":"502528","messageId":"20240910041013.68948-4-ericsunshine@charter.net","threadId":"62018","inReplyTo":"20240910041013.68948-1-ericsunshine@charter.net","subject":"[PATCH v2 3/3] chainlint: reduce annotation noise-factor","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2024-09-10T04:10:13Z","receivedAt":"2024-09-10T04:12:12Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nWhen chainlint detects a problem in a test definition, it highlights the\noffending code with a \"?!...?!\" annotation. The rather curious \"?!\"\ndecoration was chosen to draw the reader's attention to the problem area\nand to act as a good \"needle\" when using the terminal's search feature\nto \"jump\" to the next problem.\n\nLater, chainlint learned to color its output when sent to a terminal.\nProblem annotations are colored with a red background which stands out\nwell from surrounding text, thus easily draws the reader's attention.\nTogether with the preceding change which gave all problem annotations a\nuniform \"LINT:\" prefix, the noisy \"?!\" decoration has become superfluous\nas a search \"needle\" so omit it when output is colored.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 3 ++-\n t/test-lib.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex ad26499478..f0598e3934 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -651,6 +651,7 @@ sub check_test {\n \t$self->{nerrs} += @$problems;\n \treturn unless $emit_all || @$problems;\n \tmy $c = main::fd_colors(1);\n+\tmy ($erropen, $errclose) = -t 1 ? (\"$c->{rev}$c->{red}\", $c->{reset}) : ('?!', '?!');\n \tmy $start = 0;\n \tmy $checked = '';\n \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n@@ -659,7 +660,7 @@ sub check_test {\n \t\tmy $err = format_problem($label);\n \t\t$checked .= substr($body, $start, $pos - $start);\n \t\t$checked .= ' ' unless $checked =~ /\\s$/;\n-\t\t$checked .= \"$c->{rev}$c->{red}?!LINT: $err?!$c->{reset}\";\n+\t\t$checked .= \"${erropen}LINT: $err$errclose\";\n \t\t$checked .= ' ' unless $pos >= length($body) ||\n \t\t substr($body, $pos, 1) =~ /^\\s/;\n \t\t$start = $pos;\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 54247604cb..278d1215f1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1606,7 +1606,7 @@ if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0 &&\n test \"${GIT_TEST_EXT_CHAIN_LINT:-1}\" != 0\n then\n \t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n-\t\tBUG \"lint error (see '?!...!? annotations above)\"\n+\t\tBUG \"lint error (see 'LINT' annotations above)\"\n fi\n \n # Last-minute variable setup\n-- \n2.46.0\n\n"},{"id":"502544","messageId":"20240910064441.GE1459778@coredump.intra.peff.net","threadId":"62018","inReplyTo":"20240910041013.68948-1-ericsunshine@charter.net","subject":"Re: [PATCH v2 0/3] make chainlint output more newcomer-friendly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-10T06:44:41Z","receivedAt":"2024-09-10T06:44:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 10, 2024 at 12:10:10AM -0400, Eric Sunshine wrote:\n\n> Changes since v1:\n> \n> * new patch [1/3] -- motivated by Junio's observation[2] about\n> availability of structured problem information -- takes advantage of\n> that information directly rather than post-processing \"?!...?!\"\n> sequences in the output stream\n> \n> * old patch [2/2] (now [3/3]) which drops \"?!\" decorations when emitting\n> colored output to a terminal partially justified the change by\n> claiming that the new \"ERR\" (or \"ERR:\") prefix is a good \"needle\" for\n> a terminal's search feature, thus the noisy \"?!\" is no longer needed;\n> however, I realized that \"ERR\" (or \"ERR:\") is, in fact, an awful\n> needle since the string \"err\" (or \"err:\") is quite likely to\n> legitimately appear in source text, hence I changed the prefix to\n> \"LINT:\" (with the colon since Patrick found lack of colon\n> confusing[3])\n> \n> * rewrote commit messages based upon feedback from Junio[2,4]\n> \n> * dropped an unused argument from the call to format_problem() which was\n> an artifact used briefly during development of v1\n\nVery nice. I think the \"LINT:\" prefix does a good job of standing out,\nafter spot-checking the output of a few of the tests.\n\nI read through the commits themselves and didn't have any suggestions.\n\n-Peff\n"},{"id":"502549","messageId":"Zt_5xyDwG6YE1rFl@pks.im","threadId":"62018","inReplyTo":"20240910041013.68948-3-ericsunshine@charter.net","subject":"Re: [PATCH v2 2/3] chainlint: make error messages self-explanatory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-10T07:48:23Z","receivedAt":"2024-09-10T07:48:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 10, 2024 at 12:10:12AM -0400, Eric Sunshine wrote:\n> diff --git a/t/chainlint/arithmetic-expansion.expect b/t/chainlint/arithmetic-expansion.expect\n> index 338ecd5861..5677e16cad 100644\n> --- a/t/chainlint/arithmetic-expansion.expect\n> +++ b/t/chainlint/arithmetic-expansion.expect\n> @@ -4,6 +4,6 @@\n> 5 \tbaz\n> 6 ) &&\n> 7 (\n> -8 \tbar=$((42 + 1)) ?!AMP?!\n> +8 \tbar=$((42 + 1)) ?!LINT: missing '&&'?!\n> 9 \tbaz\n> 10 )\n\nThis looks a lot nicer than both the old state and the first iteration.\nI certainly like it! I'm not really able to comment on the Perl code,\nwhich mostly looks like gibberish to me (which isn't your fault). I'll\nleave it to others to comment on that.\n\nPatrick\n"},{"id":"502550","messageId":"Zt_5zMiu4QRka5x3@pks.im","threadId":"62018","inReplyTo":"20240910041013.68948-4-ericsunshine@charter.net","subject":"Re: [PATCH v2 3/3] chainlint: reduce annotation noise-factor","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-10T07:48:28Z","receivedAt":"2024-09-10T07:48:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 10, 2024 at 12:10:13AM -0400, Eric Sunshine wrote:\n> From: Eric Sunshine \n> \n> When chainlint detects a problem in a test definition, it highlights the\n> offending code with a \"?!...?!\" annotation. The rather curious \"?!\"\n> decoration was chosen to draw the reader's attention to the problem area\n> and to act as a good \"needle\" when using the terminal's search feature\n> to \"jump\" to the next problem.\n> \n> Later, chainlint learned to color its output when sent to a terminal.\n> Problem annotations are colored with a red background which stands out\n> well from surrounding text, thus easily draws the reader's attention.\n> Together with the preceding change which gave all problem annotations a\n> uniform \"LINT:\" prefix, the noisy \"?!\" decoration has become superfluous\n> as a search \"needle\" so omit it when output is colored.\n> \n> Signed-off-by: Eric Sunshine \n> ---\n> t/chainlint.pl | 3 ++-\n> t/test-lib.sh | 2 +-\n> 2 files changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/chainlint.pl b/t/chainlint.pl\n> index ad26499478..f0598e3934 100755\n> --- a/t/chainlint.pl\n> +++ b/t/chainlint.pl\n> @@ -651,6 +651,7 @@ sub check_test {\n> \t$self->{nerrs} += @$problems;\n> \treturn unless $emit_all || @$problems;\n> \tmy $c = main::fd_colors(1);\n> +\tmy ($erropen, $errclose) = -t 1 ? (\"$c->{rev}$c->{red}\", $c->{reset}) : ('?!', '?!');\n> \tmy $start = 0;\n> \tmy $checked = '';\n> \tfor (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {\n\nI was first wondering why we didn't have to change our tests. But this\nseems to use either coloring or the `?!` decorations based on whether or\nnot we output to a terminal. And as our tests output to a non-terminal\nthey indeed see the old format, and as such they don't have to change.\n\nOne thing I don't like about this is that we now have different output\ndepending on whether or not you happen to pipe output to e.g. less(1),\nwhich I do quite frequently. So I'd propose to just drop the markers\nunconditionally.\n\nPatrick\n"},{"id":"502552","messageId":"CAPig+cQZhrG+0BJkDbmKY11jxSspod2Xp8tSQq-DGOO9qMbR_w@mail.gmail.com","threadId":"62018","inReplyTo":"Zt_5zMiu4QRka5x3@pks.im","subject":"Re: [PATCH v2 3/3] chainlint: reduce annotation noise-factor","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-09-10T08:14:38Z","receivedAt":"2024-09-10T08:14:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 10, 2024 at 3:48 AM Patrick Steinhardt wrote:\n> On Tue, Sep 10, 2024 at 12:10:13AM -0400, Eric Sunshine wrote:\n> > [...]\n> > Later, chainlint learned to color its output when sent to a terminal.\n> > Problem annotations are colored with a red background which stands out\n> > well from surrounding text, thus easily draws the reader's attention.\n> > Together with the preceding change which gave all problem annotations a\n> > uniform \"LINT:\" prefix, the noisy \"?!\" decoration has become superfluous\n> > as a search \"needle\" so omit it when output is colored.\n> >\n> > Signed-off-by: Eric Sunshine \n> > ---\n> > + my ($erropen, $errclose) = -t 1 ? (\"$c->{rev}$c->{red}\", $c->{reset}) : ('?!', '?!');\n>\n> I was first wondering why we didn't have to change our tests. But this\n> seems to use either coloring or the `?!` decorations based on whether or\n> not we output to a terminal. And as our tests output to a non-terminal\n> they indeed see the old format, and as such they don't have to change.\n\nCorrect.\n\n> One thing I don't like about this is that we now have different output\n> depending on whether or not you happen to pipe output to e.g. less(1),\n> which I do quite frequently. So I'd propose to just drop the markers\n> unconditionally.\n\nMy knee-jerk reaction is that the \"?!\" decoration is still handy for\ndrawing the eye when scanning non-colored output visually (not using a\nsearch feature), so I'm hesitant to drop it. However, on reflection,\nI'm not sure I feel very strongly about it. What do others think?\n"},{"id":"502563","messageId":"xmqqjzfjms6j.fsf@gitster.g","threadId":"62018","inReplyTo":"CAPig+cQZhrG+0BJkDbmKY11jxSspod2Xp8tSQq-DGOO9qMbR_w@mail.gmail.com","subject":"Re: [PATCH v2 3/3] chainlint: reduce annotation noise-factor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-10T15:42:12Z","receivedAt":"2024-09-10T15:42:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> One thing I don't like about this is that we now have different output\n>> depending on whether or not you happen to pipe output to e.g. less(1),\n>> which I do quite frequently. So I'd propose to just drop the markers\n>> unconditionally.\n>\n> My knee-jerk reaction is that the \"?!\" decoration is still handy for\n> drawing the eye when scanning non-colored output visually (not using a\n> search feature), so I'm hesitant to drop it. However, on reflection,\n> I'm not sure I feel very strongly about it. What do others think?\n\nUnlike ERR, LINT is distinct enough, even when mixed with snippets\ntaken from the test scripts that are full of words that hints\nerrors, checking, etc., so I'd expect that new readers who have\nnever seen the \"?!\" eye-magnets would not find the output too hard\nto read. For those of us whose eyes are so used to, we might miss\nthem for a while, but I do not see much upside in keeping it.\n\nThanks.\n"},{"id":"502575","messageId":"xmqqttenjvzv.fsf@gitster.g","threadId":"62018","inReplyTo":"20240910041013.68948-2-ericsunshine@charter.net","subject":"Re: [PATCH v2 1/3] chainlint: don't be fooled by \"?!...?!\" in test body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-10T16:48:04Z","receivedAt":"2024-09-10T16:48:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> However, when 73c768dae9 (chainlint: annotate original test definition\n> rather than token stream, 2022-11-08) taught chainlint to output the\n> original test text verbatim, it started collecting structured\n> information about detected problems.\n>\n> Now that it is available, take advantage of the structured problem\n> information to deterministically count the number of problems detected\n> and to color the annotations directly, rather than scanning the output\n> stream for \"?!...?!\" and performing these operations after-the-fact.\n\nMakes sense. Nicely done.\n\nWill queue. Thanks.\n"},{"id":"502577","messageId":"xmqqfrq7fmat.fsf@gitster.g","threadId":"62018","inReplyTo":"20240910041013.68948-1-ericsunshine@charter.net","subject":"Re: [PATCH v2 0/3] make chainlint output more newcomer-friendly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-10T17:31:06Z","receivedAt":"2024-09-10T17:31:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> * new patch [1/3] -- motivated by Junio's observation[2] about\n> availability of structured problem information -- takes advantage of\n> that information directly rather than post-processing \"?!...?!\"\n> sequences in the output stream\n\n;-).\n\n> * old patch [2/2] (now [3/3]) which drops \"?!\" decorations when emitting\n> colored output to a terminal partially justified the change by\n> claiming that the new \"ERR\" (or \"ERR:\") prefix is a good \"needle\" for\n> a terminal's search feature, thus the noisy \"?!\" is no longer needed;\n> however, I realized that \"ERR\" (or \"ERR:\") is, in fact, an awful\n> needle since the string \"err\" (or \"err:\") is quite likely to\n> legitimately appear in source text, hence I changed the prefix to\n> \"LINT:\" (with the colon since Patrick found lack of colon\n> confusing[3])\n\nNice; I prefer LINT over ERR quite a lot.\n\n> Unfortunately, the included range-diff is a mess and pretty much useless\n\nThat's expected and OK after a large update of any series, which\noften deserves to be read from cover to cover anyway.\n\n> -\t$checked =~ s/(\\s) \\?!/$1?!/mg;\n> -\t$checked =~ s/\\?! (\\s)/?!$1/mg;\n> -\t$checked =~ s/\\?!([^?]+)\\?!/$erropen$1$errclose/mg;\n\n;-)\n"},{"id":"502601","messageId":"CAPig+cRuAVc=zzhbXFx0LkO6Q93fMEVyMiJxJ41eVAp48iCX8w@mail.gmail.com","threadId":"62018","inReplyTo":"xmqqjzfjms6j.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] chainlint: reduce annotation noise-factor","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-09-10T22:17:52Z","receivedAt":"2024-09-10T22:18:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 10, 2024 at 11:42 AM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > On Tue, Sep 10, 2024 at 3:48 AM Patrick Steinhardt wrote:\n> >> One thing I don't like about this is that we now have different output\n> >> depending on whether or not you happen to pipe output to e.g. less(1),\n> >> which I do quite frequently. So I'd propose to just drop the markers\n> >> unconditionally.\n> >\n> > My knee-jerk reaction is that the \"?!\" decoration is still handy for\n> > drawing the eye when scanning non-colored output visually (not using a\n> > search feature), so I'm hesitant to drop it. However, on reflection,\n> > I'm not sure I feel very strongly about it. What do others think?\n>\n> Unlike ERR, LINT is distinct enough, even when mixed with snippets\n> taken from the test scripts that are full of words that hints\n> errors, checking, etc., so I'd expect that new readers who have\n> never seen the \"?!\" eye-magnets would not find the output too hard\n> to read. For those of us whose eyes are so used to, we might miss\n> them for a while, but I do not see much upside in keeping it.\n\nOkay, thanks for weighing in. I'll reroll and make [3/3] drop the \"?!\"\ndecoration unconditionally.\n"}]}