{"thread":{"id":"58769","subject":"[PATCH 1/4] chainlint: add explanatory comments","startedAt":"2022-11-08T19:08:40Z","lastAt":"2022-11-10T02:42:27Z","messageCount":11,"participants":["Eric Sunshine via GitGitGadget","Taylor Blau","Ævar Arnfjörð Bjarmason","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"466879","messageId":"a445304594c6139770439c49cf18f10c6757cbab.1667934510.git.gitgitgadget@gmail.com","threadId":"58769","inReplyTo":"pull.1375.git.git.1667934510.gitgitgadget@gmail.com","subject":"[PATCH 1/4] chainlint: add explanatory comments","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T19:08:27Z","receivedAt":"2022-11-08T19:08:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine <sunshine@sunshineco.com>\n\nThe logic in TestParser::accumulate() for detecting broken &&-chains is\nmostly well-commented, but a couple branches which were deemed obvious\nand straightforward lack comments. In retrospect, though, these cases\nmay give future readers pause, so comment them, as well.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n t/chainlint.pl | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 976db4b8a01..9908de6c758 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -505,7 +505,11 @@ my @safe_endings = (\n \n sub accumulate {\n \tmy ($self, $tokens, $cmd) = @_;\n+\n+\t# no previous command to check for missing \"&&\"\n \tgoto DONE unless @$tokens;\n+\n+\t# new command is empty line; can't yet check if previous is missing \"&&\"\n \tgoto DONE if @$cmd == 1 && $$cmd[0] eq \"\\n\";\n \n \t# did previous command end with \"&&\", \"|\", \"|| return\" or similar?\n-- \ngitgitgadget\n\n"},{"id":"466880","messageId":"pull.1375.git.git.1667934510.gitgitgadget@gmail.com","threadId":"58769","inReplyTo":null,"subject":"[PATCH 0/4] chainlint: improve annotated output","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T19:08:26Z","receivedAt":"2022-11-08T19:08:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When chainlint detects problems in a test, such as a broken &&-chain, it\nprints out the test with \"?!FOO?!\" annotations inserted at each problem\nlocation. However, rather than annotating the original test definition, it\ninstead dumps out a parsed token representation of the test. Since it lacks\ncomments, indentation, here-doc bodies, and so forth, this tokenized\nrepresentation can be difficult for the test author to digest and relate\nback to the original test definition.\n\nAn earlier patch series[1] improved the output somewhat by colorizing the\n\"?!FOO?!\" annotations and the \"# chainlint:\" lines, but the output can still\nbe difficult to digest.\n\nThis patch series further improves the output by instead making chainlint.pl\nannotate the original test definition rather than the parsed token stream,\nthus preserving indentation (and whitespace, in general), here-doc bodies,\netc., which should make it easier for a test author to relate each problem\nback to the source.\n\nThis series was inspired by usability comments from Peff[2] and Ævar[3] and\na bit of discussion which followed[4][5].\n\n(Note to self: Add Ævar to nerd-snipe blacklist alongside Peff.)\n\nFOOTNOTES\n\n[1]\nhttps://lore.kernel.org/git/pull.1324.v2.git.git.1663041707260.gitgitgadget@gmail.com/\n[2] https://lore.kernel.org/git/Yx1x5lme2SGBjfia@coredump.intra.peff.net/\n[3] https://lore.kernel.org/git/221024.865yg9ecsx.gmgdl@evledraar.gmail.com/\n[4]\nhttps://lore.kernel.org/git/CAPig+cRJVn-mbA6-jOmNfDJtK_nX4ZTw+OcNShvvz8zcQYbCHQ@mail.gmail.com/\n[5]\nhttps://lore.kernel.org/git/CAPig+cT=cWYT6kicNWT+6RxfiKKMyVz72H3_9kwkF-f4Vuoe1w@mail.gmail.com/\n\nEric Sunshine (4):\n  chainlint: add explanatory comments\n  chainlint: tighten accuracy when consuming input stream\n  chainlint: latch start/end position of each token\n  chainlint: annotate original test definition rather than token stream\n\n t/chainlint.pl                                | 107 +++++++++++-------\n t/chainlint/block-comment.expect              |   2 +\n t/chainlint/case-comment.expect               |   3 +\n t/chainlint/close-subshell.expect             |   3 +-\n t/chainlint/comment.expect                    |   4 +\n t/chainlint/double-here-doc.expect            |  14 ++-\n t/chainlint/empty-here-doc.expect             |   3 +-\n t/chainlint/for-loop.expect                   |   4 +-\n t/chainlint/here-doc-close-subshell.expect    |   4 +-\n t/chainlint/here-doc-indent-operator.expect   |  10 +-\n .../here-doc-multi-line-command-subst.expect  |   5 +-\n t/chainlint/here-doc-multi-line-string.expect |   4 +-\n t/chainlint/here-doc.expect                   |  24 +++-\n t/chainlint/if-then-else.expect               |   4 +-\n t/chainlint/incomplete-line.expect            |  10 +-\n t/chainlint/inline-comment.expect             |   4 +-\n t/chainlint/loop-detect-status.expect         |   2 +-\n t/chainlint/nested-here-doc.expect            |  27 ++++-\n t/chainlint/nested-subshell-comment.expect    |   2 +\n t/chainlint/subshell-here-doc.expect          |  28 ++++-\n t/chainlint/t7900-subtree.expect              |   4 +\n t/chainlint/while-loop.expect                 |   4 +-\n 22 files changed, 206 insertions(+), 66 deletions(-)\n\n\nbase-commit: 63bba4fdd86d80ef061c449daa97a981a9be0792\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1375%2Fsunshineco%2Fchainlintpreserve-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1375/sunshineco/chainlintpreserve-v1\nPull-Request: https://github.com/git/git/pull/1375\n-- \ngitgitgadget\n"},{"id":"466881","messageId":"31af383fd439c3c0a5003598961acfecfae4018c.1667934510.git.gitgitgadget@gmail.com","threadId":"58769","inReplyTo":"pull.1375.git.git.1667934510.gitgitgadget@gmail.com","subject":"[PATCH 2/4] chainlint: tighten accuracy when consuming input stream","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T19:08:28Z","receivedAt":"2022-11-08T19:08:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine <sunshine@sunshineco.com>\n\nTo extract the next token in the input stream, Lexer::scan_token() finds\nthe start of the token by skipping whitespace, then consumes characters\nbelonging to the token until it encounters a non-token character, such\nas an operator, punctuation, or whitespace. In the case of an operator\nor punctuation which ends a token, before returning the just-scanned\ntoken, it pushes that operator or punctuation character back onto the\ninput stream to ensure that it will be the first character consumed by\nthe next call to scan_token().\n\nHowever, scan_token() is intentionally lax when whitespace ends a token;\nit doesn't bother pushing the whitespace character back onto the token\nstream since it knows that the next call to scan_token() will, as its\nfirst step, skip over whitespace anyhow when looking for the start of\nthe token.\n\nAlthough such laxity is harmless for the proper functioning of the\nlexical analyzer, it does make it difficult to precisely identify the\ntoken's end position in the input stream. Accurate token position\ninformation may be desirable, for instance, to annotate problems or\nhighlight other interesting facets of the input found during the parsing\nphase. To accommodate such possibilities, tighten scan_token() by making\nit push the token-ending whitespace character back onto the input\nstream, just as it does for other token-ending characters.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n t/chainlint.pl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 9908de6c758..1f66c03c593 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -179,7 +179,7 @@ RESTART:\n \t\t# handle special characters\n \t\tlast unless $$b =~ /\\G(.)/sgc;\n \t\tmy $c = $1;\n-\t\tlast if $c =~ /^[ \\t]$/; # whitespace ends token\n+\t\tpos($$b)--, last if $c =~ /^[ \\t]$/; # whitespace ends token\n \t\tpos($$b)--, last if length($token) && $c =~ /^[;&|<>(){}\\n]$/;\n \t\t$token .= $self->scan_sqstring(), next if $c eq \"'\";\n \t\t$token .= $self->scan_dqstring(), next if $c eq '\"';\n-- \ngitgitgadget\n\n"},{"id":"466882","messageId":"fa56128c37280ccc87f8f8f6fd586db221b13ea4.1667934510.git.gitgitgadget@gmail.com","threadId":"58769","inReplyTo":"pull.1375.git.git.1667934510.gitgitgadget@gmail.com","subject":"[PATCH 3/4] chainlint: latch start/end position of each token","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T19:08:29Z","receivedAt":"2022-11-08T19:08:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine <sunshine@sunshineco.com>\n\nWhen chainlint detects problems in a test, such as a broken &&-chain, it\nprints out the test with \"?!FOO?!\" annotations inserted at each problem\nlocation. However, rather than annotating the original test definition,\nit instead dumps out a parsed token representation of the test. Since it\nlacks comments, indentations, here-doc bodies, and so forth, this\ntokenized representation can be difficult for the test author to digest\nand relate back to the original test definition.\n\nTo address this shortcoming, an upcoming change will make it print out\nan annotated copy of the original test definition rather than the\ntokenized representation. In order to do so, it will need to know the\nstart and end positions of each token in the original test definition.\nAs preparation, upgrade TestParser::scan_token() to latch the start and\nend position of the token being scanned, and return that information\nalong with the token itself. A subsequent change will take advantage of\nthis positional information.\n\nIn terms of implementation, TestParser::scan_token() is retrofitted to\nreturn a tuple consisting of the token's lexeme and its start and end\npositions, rather than returning just the lexeme. However, an\nalternative would be to define a class which represents a token:\n\n    package Token;\n\n    sub new {\n        my ($class, $lexeme, $start, $end) = @_;\n        bless [$lexeme, $start, $end] => $class;\n    }\n\n    sub as_string {\n        my $self = shift @_;\n        return $self->[0];\n    }\n\n    sub compare {\n        my ($x, $y) = @_;\n        if (UNIVERSAL::isa($y, 'Token')) {\n            return $x->[0] cmp $y->[0];\n        }\n        return $x->[0] cmp $y;\n    }\n\n    use overload (\n        '\"\"' => 'as_string',\n        'cmp' => 'compare'\n    );\n\nThe major benefit of the class-based approach is that it is entirely\nnon-invasive; it requires no additional changes to the rest of the\nscript since a Token converts automatically to a string, which is what\nscan_token() historically returned.\n\nThe big downside to the Token approach, however, is that it is _slow_;\non this developer's (old) machine, it increases user-time by an\nunacceptable seven seconds when scanning all test scripts in the\nproject. Hence, the simple tuple approach is employed instead since it\nadds only a fraction of a second user-time.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n t/chainlint.pl | 80 +++++++++++++++++++++++++++-----------------------\n 1 file changed, 43 insertions(+), 37 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 1f66c03c593..59aa79babc2 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -75,7 +75,9 @@ sub scan_heredoc_tag {\n \tmy $self = shift @_;\n \t${$self->{buff}} =~ /\\G(-?)/gc;\n \tmy $indented = $1;\n-\tmy $tag = $self->scan_token();\n+\tmy $token = $self->scan_token();\n+\treturn \"<<$indented\" unless $token;\n+\tmy $tag = $token->[0];\n \t$tag =~ s/['\"\\\\]//g;\n \tpush(@{$self->{heretags}}, $indented ? \"\\t$tag\" : \"$tag\");\n \treturn \"<<$indented$tag\";\n@@ -149,7 +151,7 @@ sub scan_dollar {\n \tmy $self = shift @_;\n \tmy $b = $self->{buff};\n \treturn $self->scan_balanced('(', ')') if $$b =~ /\\G\\((?=\\()/gc; # $((...))\n-\treturn '(' . join(' ', $self->scan_subst()) . ')' if $$b =~ /\\G\\(/gc; # $(...)\n+\treturn '(' . join(' ', map {$_->[0]} $self->scan_subst()) . ')' if $$b =~ /\\G\\(/gc; # $(...)\n \treturn $self->scan_balanced('{', '}') if $$b =~ /\\G\\{/gc; # ${...}\n \treturn $1 if $$b =~ /\\G(\\w+)/gc; # $var\n \treturn $1 if $$b =~ /\\G([@*#?$!0-9-])/gc; # $*, $1, $$, etc.\n@@ -170,9 +172,11 @@ sub scan_token {\n \tmy $self = shift @_;\n \tmy $b = $self->{buff};\n \tmy $token = '';\n+\tmy $start;\n RESTART:\n \t$$b =~ /\\G[ \\t]+/gc; # skip whitespace (but not newline)\n-\treturn \"\\n\" if $$b =~ /\\G#[^\\n]*(?:\\n|\\z)/gc; # comment\n+\t$start = pos($$b) || 0;\n+\treturn [\"\\n\", $start, pos($$b)] if $$b =~ /\\G#[^\\n]*(?:\\n|\\z)/gc; # comment\n \twhile (1) {\n \t\t# slurp up non-special characters\n \t\t$token .= $1 if $$b =~ /\\G([^\\\\;&|<>(){}'\"\\$\\s]+)/gc;\n@@ -197,7 +201,7 @@ RESTART:\n \t\t}\n \t\tdie(\"internal error scanning character '$c'\\n\");\n \t}\n-\treturn length($token) ? $token : undef;\n+\treturn length($token) ? [$token, $start, pos($$b)] : undef;\n }\n \n # ShellParser parses POSIX shell scripts (with minor extensions for Bash). It\n@@ -239,14 +243,14 @@ sub stop_at {\n \tmy ($self, $token) = @_;\n \treturn 1 unless defined($token);\n \tmy $stop = ${$self->{stop}}[-1] if @{$self->{stop}};\n-\treturn defined($stop) && $token =~ $stop;\n+\treturn defined($stop) && $token->[0] =~ $stop;\n }\n \n sub expect {\n \tmy ($self, $expect) = @_;\n \tmy $token = $self->next_token();\n-\treturn $token if defined($token) && $token eq $expect;\n-\tpush(@{$self->{output}}, \"?!ERR?! expected '$expect' but found '\" . (defined($token) ? $token : \"<end-of-input>\") . \"'\\n\");\n+\treturn $token if defined($token) && $token->[0] eq $expect;\n+\tpush(@{$self->{output}}, \"?!ERR?! expected '$expect' but found '\" . (defined($token) ? $token->[0] : \"<end-of-input>\") . \"'\\n\");\n \t$self->untoken($token) if defined($token);\n \treturn ();\n }\n@@ -255,7 +259,7 @@ sub optional_newlines {\n \tmy $self = shift @_;\n \tmy @tokens;\n \twhile (my $token = $self->peek()) {\n-\t\tlast unless $token eq \"\\n\";\n+\t\tlast unless $token->[0] eq \"\\n\";\n \t\tpush(@tokens, $self->next_token());\n \t}\n \treturn @tokens;\n@@ -278,7 +282,7 @@ sub parse_case_pattern {\n \tmy @tokens;\n \twhile (defined(my $token = $self->next_token())) {\n \t\tpush(@tokens, $token);\n-\t\tlast if $token eq ')';\n+\t\tlast if $token->[0] eq ')';\n \t}\n \treturn @tokens;\n }\n@@ -293,13 +297,13 @@ sub parse_case {\n \t     $self->optional_newlines());\n \twhile (1) {\n \t\tmy $token = $self->peek();\n-\t\tlast unless defined($token) && $token ne 'esac';\n+\t\tlast unless defined($token) && $token->[0] ne 'esac';\n \t\tpush(@tokens,\n \t\t     $self->parse_case_pattern(),\n \t\t     $self->optional_newlines(),\n \t\t     $self->parse(qr/^(?:;;|esac)$/)); # item body\n \t\t$token = $self->peek();\n-\t\tlast unless defined($token) && $token ne 'esac';\n+\t\tlast unless defined($token) && $token->[0] ne 'esac';\n \t\tpush(@tokens,\n \t\t     $self->expect(';;'),\n \t\t     $self->optional_newlines());\n@@ -315,7 +319,7 @@ sub parse_for {\n \t     $self->next_token(), # variable\n \t     $self->optional_newlines());\n \tmy $token = $self->peek();\n-\tif (defined($token) && $token eq 'in') {\n+\tif (defined($token) && $token->[0] eq 'in') {\n \t\tpush(@tokens,\n \t\t     $self->expect('in'),\n \t\t     $self->optional_newlines());\n@@ -339,11 +343,11 @@ sub parse_if {\n \t\t     $self->optional_newlines(),\n \t\t     $self->parse(qr/^(?:elif|else|fi)$/)); # if/elif body\n \t\tmy $token = $self->peek();\n-\t\tlast unless defined($token) && $token eq 'elif';\n+\t\tlast unless defined($token) && $token->[0] eq 'elif';\n \t\tpush(@tokens, $self->expect('elif'));\n \t}\n \tmy $token = $self->peek();\n-\tif (defined($token) && $token eq 'else') {\n+\tif (defined($token) && $token->[0] eq 'else') {\n \t\tpush(@tokens,\n \t\t     $self->expect('else'),\n \t\t     $self->optional_newlines(),\n@@ -380,7 +384,7 @@ sub parse_bash_array_assignment {\n \tmy @tokens = $self->expect('(');\n \twhile (defined(my $token = $self->next_token())) {\n \t\tpush(@tokens, $token);\n-\t\tlast if $token eq ')';\n+\t\tlast if $token->[0] eq ')';\n \t}\n \treturn @tokens;\n }\n@@ -398,29 +402,31 @@ sub parse_cmd {\n \tmy $self = shift @_;\n \tmy $cmd = $self->next_token();\n \treturn () unless defined($cmd);\n-\treturn $cmd if $cmd eq \"\\n\";\n+\treturn $cmd if $cmd->[0] eq \"\\n\";\n \n \tmy $token;\n \tmy @tokens = $cmd;\n-\tif ($cmd eq '!') {\n+\tif ($cmd->[0] eq '!') {\n \t\tpush(@tokens, $self->parse_cmd());\n \t\treturn @tokens;\n-\t} elsif (my $f = $compound{$cmd}) {\n+\t} elsif (my $f = $compound{$cmd->[0]}) {\n \t\tpush(@tokens, $self->$f());\n-\t} elsif (defined($token = $self->peek()) && $token eq '(') {\n-\t\tif ($cmd !~ /\\w=$/) {\n+\t} elsif (defined($token = $self->peek()) && $token->[0] eq '(') {\n+\t\tif ($cmd->[0] !~ /\\w=$/) {\n \t\t\tpush(@tokens, $self->parse_func());\n \t\t\treturn @tokens;\n \t\t}\n-\t\t$tokens[-1] .= join(' ', $self->parse_bash_array_assignment());\n+\t\tmy @array = $self->parse_bash_array_assignment();\n+\t\t$tokens[-1]->[0] .= join(' ', map {$_->[0]} @array);\n+\t\t$tokens[-1]->[2] = $array[$#array][2] if @array;\n \t}\n \n \twhile (defined(my $token = $self->next_token())) {\n \t\t$self->untoken($token), last if $self->stop_at($token);\n \t\tpush(@tokens, $token);\n-\t\tlast if $token =~ /^(?:[;&\\n|]|&&|\\|\\|)$/;\n+\t\tlast if $token->[0] =~ /^(?:[;&\\n|]|&&|\\|\\|)$/;\n \t}\n-\tpush(@tokens, $self->next_token()) if $tokens[-1] ne \"\\n\" && defined($token = $self->peek()) && $token eq \"\\n\";\n+\tpush(@tokens, $self->next_token()) if $tokens[-1]->[0] ne \"\\n\" && defined($token = $self->peek()) && $token->[0] eq \"\\n\";\n \treturn @tokens;\n }\n \n@@ -457,7 +463,7 @@ sub find_non_nl {\n \tmy $tokens = shift @_;\n \tmy $n = shift @_;\n \t$n = $#$tokens if !defined($n);\n-\t$n-- while $n >= 0 && $$tokens[$n] eq \"\\n\";\n+\t$n-- while $n >= 0 && $$tokens[$n]->[0] eq \"\\n\";\n \treturn $n;\n }\n \n@@ -467,7 +473,7 @@ sub ends_with {\n \tfor my $needle (reverse(@$needles)) {\n \t\treturn undef if $n < 0;\n \t\t$n = find_non_nl($tokens, $n), next if $needle eq \"\\n\";\n-\t\treturn undef if $$tokens[$n] !~ $needle;\n+\t\treturn undef if $$tokens[$n]->[0] !~ $needle;\n \t\t$n--;\n \t}\n \treturn 1;\n@@ -486,13 +492,13 @@ sub parse_loop_body {\n \tmy $self = shift @_;\n \tmy @tokens = $self->SUPER::parse_loop_body(@_);\n \t# did loop signal failure via \"|| return\" or \"|| exit\"?\n-\treturn @tokens if !@tokens || grep(/^(?:return|exit|\\$\\?)$/, @tokens);\n+\treturn @tokens if !@tokens || grep {$_->[0] =~ /^(?:return|exit|\\$\\?)$/} @tokens;\n \t# did loop upstream of a pipe signal failure via \"|| echo 'impossible\n \t# text'\" as the final command in the 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-\tsplice(@tokens, $n + 1, 0, '?!LOOP?!');\n+\tsplice(@tokens, $n + 1, 0, ['?!LOOP?!', $tokens[$n]->[1], $tokens[$n]->[2]]);\n \treturn @tokens;\n }\n \n@@ -510,7 +516,7 @@ sub accumulate {\n \tgoto DONE unless @$tokens;\n \n \t# new command is empty line; can't yet check if previous is missing \"&&\"\n-\tgoto DONE if @$cmd == 1 && $$cmd[0] eq \"\\n\";\n+\tgoto DONE if @$cmd == 1 && $$cmd[0]->[0] eq \"\\n\";\n \n \t# did previous command end with \"&&\", \"|\", \"|| return\" or similar?\n \tgoto DONE if match_ending($tokens, \\@safe_endings);\n@@ -518,20 +524,20 @@ sub accumulate {\n \t# if this command handles \"$?\" specially, then okay for previous\n \t# command to be missing \"&&\"\n \tfor my $token (@$cmd) {\n-\t\tgoto DONE if $token =~ /\\$\\?/;\n+\t\tgoto DONE if $token->[0] =~ /\\$\\?/;\n \t}\n \n \t# if this command is \"false\", \"return 1\", or \"exit 1\" (which signal\n \t# failure explicitly), then okay for all preceding commands to be\n \t# missing \"&&\"\n-\tif ($$cmd[0] =~ /^(?:false|return|exit)$/) {\n-\t\t@$tokens = grep(!/^\\?!AMP\\?!$/, @$tokens);\n+\tif ($$cmd[0]->[0] =~ /^(?:false|return|exit)$/) {\n+\t\t@$tokens = grep {$_->[0] !~ /^\\?!AMP\\?!$/} @$tokens;\n \t\tgoto DONE;\n \t}\n \n \t# flag missing \"&&\" at end of previous command\n \tmy $n = find_non_nl($tokens);\n-\tsplice(@$tokens, $n + 1, 0, '?!AMP?!') unless $n < 0;\n+\tsplice(@$tokens, $n + 1, 0, ['?!AMP?!', $$tokens[$n]->[1], $$tokens[$n]->[2]]) unless $n < 0;\n \n DONE:\n \t$self->SUPER::accumulate($tokens, $cmd);\n@@ -557,7 +563,7 @@ sub new {\n # composition of multiple strings and non-string character runs; for instance,\n # `\"test body\"` unwraps to `test body`; `word\"a b\"42'c d'` to `worda b42c d`\n sub unwrap {\n-\tmy $token = @_ ? shift @_ : $_;\n+\tmy $token = (@_ ? shift @_ : $_)->[0];\n \t# simple case: 'sqstring' or \"dqstring\"\n \treturn $token if $token =~ s/^'([^']*)'$/$1/;\n \treturn $token if $token =~ s/^\"([^\"]*)\"$/$1/;\n@@ -588,9 +594,9 @@ sub check_test {\n \t$self->{ntests}++;\n \tmy $parser = TestParser->new(\\$body);\n \tmy @tokens = $parser->parse();\n-\treturn unless $emit_all || grep(/\\?![^?]+\\?!/, @tokens);\n+\treturn unless $emit_all || grep {$_->[0] =~ /\\?![^?]+\\?!/} @tokens;\n \tmy $c = main::fd_colors(1);\n-\tmy $checked = join(' ', @tokens);\n+\tmy $checked = join(' ', map {$_->[0]} @tokens);\n \t$checked =~ s/^\\n//;\n \t$checked =~ s/^ //mg;\n \t$checked =~ s/ $//mg;\n@@ -602,9 +608,9 @@ sub check_test {\n sub parse_cmd {\n \tmy $self = shift @_;\n \tmy @tokens = $self->SUPER::parse_cmd();\n-\treturn @tokens unless @tokens && $tokens[0] =~ /^test_expect_(?:success|failure)$/;\n+\treturn @tokens unless @tokens && $tokens[0]->[0] =~ /^test_expect_(?:success|failure)$/;\n \tmy $n = $#tokens;\n-\t$n-- while $n >= 0 && $tokens[$n] =~ /^(?:[;&\\n|]|&&|\\|\\|)$/;\n+\t$n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\\n|]|&&|\\|\\|)$/;\n \t$self->check_test($tokens[1], $tokens[2]) if $n == 2; # title body\n \t$self->check_test($tokens[2], $tokens[3]) if $n > 2;  # prereq title body\n \treturn @tokens;\n-- \ngitgitgadget\n\n"},{"id":"466883","messageId":"739c6df1dbcf1963fb426a3c25dc22fa46f7c3e0.1667934510.git.gitgitgadget@gmail.com","threadId":"58769","inReplyTo":"pull.1375.git.git.1667934510.gitgitgadget@gmail.com","subject":"[PATCH 4/4] chainlint: annotate original test definition rather than token stream","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-08T19:08:30Z","receivedAt":"2022-11-08T19:08:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine <sunshine@sunshineco.com>\n\nWhen chainlint detects problems in a test, such as a broken &&-chain, it\nprints out the test with \"?!FOO?!\" annotations inserted at each problem\nlocation. However, rather than annotating the original test definition,\nit instead dumps out a parsed token representation of the test. Since it\nlacks comments, indentations, here-doc bodies, and so forth, this\ntokenized representation can be difficult for the test author to digest\nand relate back to the original test definition.\n\nHowever, now that each parsed token carries positional information, the\nlocation of a detected problem can be pinpointed precisely in the\noriginal test definition. Therefore, take advantage of this information\nto annotate the test definition itself rather than annotating the parsed\ntoken stream, thus making it easier for a test author to relate a\nproblem back to the source.\n\nMaintaining the positional meta-information associated with each\ndetected problem requires a slight change in how the problems are\nmanaged internally. In particular, shell syntax such as:\n\n    msg=\"total: $(cd data; wc -w *.txt) words\"\n\nrequires the lexical analyzer to recursively invoke the parser in order\nto detect problems within the $(...) expression inside the double-quoted\nstring. In this case, the recursive parse context will detect the broken\n&&-chain between the `cd` and `wc` commands, returning the token stream:\n\n    cd data ; ?!AMP?! wc -w *.txt\n\nHowever, the parent parse context will see everything inside the\ndouble-quotes as a single string token:\n\n    \"total: $(cd data ; ?!AMP?! wc -w *.txt) words\"\n\nlosing whatever positional information was attached to the \";\" token\nwhere the problem was detected.\n\nOne way to preserve the positional information of a detected problem in\na recursive parse context within a string would be to attach the\npositional information to the annotation textually; for instance:\n\n    \"total: $(cd data ; ?!AMP:21:22?! wc -w *.txt) words\"\n\nand then extract the positional information when annotating the original\ntest definition.\n\nHowever, a cleaner and much simpler approach is to maintain the list of\ndetected problems separately rather than embedding the problems as\nannotations directly in the parsed token stream. Not only does this\nensure that positional information within recursive parse contexts is\nnot lost, but it keeps the token stream free from non-token pollution,\nwhich may simplify implementation of validations added in the future\nsince they won't have to handle non-token \"?!FOO!?\" items specially.\n\nFinally, the chainlint self-test \"expect\" files need a few mechanical\nadjustments now that the original test definitions are emitted rather\nthan the parsed token stream. In particular, the following items missing\nfrom the historic parsed-token output are now preserved verbatim:\n\n    * indentation (and whitespace, in general)\n\n    * comments\n\n    * here-doc bodies\n\n    * here-doc tag quoting (i.e. \"\\EOF\")\n\n    * line-splices (i.e. \"\\\" at the end of a line)\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n t/chainlint.pl                                | 31 ++++++++++++++-----\n t/chainlint/block-comment.expect              |  2 ++\n t/chainlint/case-comment.expect               |  3 ++\n t/chainlint/close-subshell.expect             |  3 +-\n t/chainlint/comment.expect                    |  4 +++\n t/chainlint/double-here-doc.expect            | 14 +++++++--\n t/chainlint/empty-here-doc.expect             |  3 +-\n t/chainlint/for-loop.expect                   |  4 ++-\n t/chainlint/here-doc-close-subshell.expect    |  4 ++-\n t/chainlint/here-doc-indent-operator.expect   | 10 ++++--\n .../here-doc-multi-line-command-subst.expect  |  5 ++-\n t/chainlint/here-doc-multi-line-string.expect |  4 ++-\n t/chainlint/here-doc.expect                   | 24 ++++++++++++--\n t/chainlint/if-then-else.expect               |  4 ++-\n t/chainlint/incomplete-line.expect            | 10 ++++--\n t/chainlint/inline-comment.expect             |  4 +--\n t/chainlint/loop-detect-status.expect         |  2 +-\n t/chainlint/nested-here-doc.expect            | 27 ++++++++++++++--\n t/chainlint/nested-subshell-comment.expect    |  2 ++\n t/chainlint/subshell-here-doc.expect          | 28 ++++++++++++++---\n t/chainlint/t7900-subtree.expect              |  4 +++\n t/chainlint/while-loop.expect                 |  4 ++-\n 22 files changed, 163 insertions(+), 33 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 59aa79babc2..7972c5bbe6f 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -459,6 +459,13 @@ package TestParser;\n \n use base 'ShellParser';\n \n+sub new {\n+\tmy $class = shift @_;\n+\tmy $self = $class->SUPER::new(@_);\n+\t$self->{problems} = [];\n+\treturn $self;\n+}\n+\n sub find_non_nl {\n \tmy $tokens = shift @_;\n \tmy $n = shift @_;\n@@ -498,7 +505,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-\tsplice(@tokens, $n + 1, 0, ['?!LOOP?!', $tokens[$n]->[1], $tokens[$n]->[2]]);\n+\tpush(@{$self->{problems}}, ['LOOP', $tokens[$n]]);\n \treturn @tokens;\n }\n \n@@ -511,6 +518,7 @@ my @safe_endings = (\n \n sub accumulate {\n \tmy ($self, $tokens, $cmd) = @_;\n+\tmy $problems = $self->{problems};\n \n \t# no previous command to check for missing \"&&\"\n \tgoto DONE unless @$tokens;\n@@ -531,13 +539,13 @@ sub accumulate {\n \t# failure explicitly), then okay for all preceding commands to be\n \t# missing \"&&\"\n \tif ($$cmd[0]->[0] =~ /^(?:false|return|exit)$/) {\n-\t\t@$tokens = grep {$_->[0] !~ /^\\?!AMP\\?!$/} @$tokens;\n+\t\t@$problems = grep {$_->[0] ne 'AMP'} @$problems;\n \t\tgoto DONE;\n \t}\n \n \t# flag missing \"&&\" at end of previous command\n \tmy $n = find_non_nl($tokens);\n-\tsplice(@$tokens, $n + 1, 0, ['?!AMP?!', $$tokens[$n]->[1], $$tokens[$n]->[2]]) unless $n < 0;\n+\tpush(@$problems, ['AMP', $tokens->[$n]]) unless $n < 0;\n \n DONE:\n \t$self->SUPER::accumulate($tokens, $cmd);\n@@ -594,12 +602,21 @@ sub check_test {\n \t$self->{ntests}++;\n \tmy $parser = TestParser->new(\\$body);\n \tmy @tokens = $parser->parse();\n-\treturn unless $emit_all || grep {$_->[0] =~ /\\?![^?]+\\?!/} @tokens;\n+\tmy $problems = $parser->{problems};\n+\treturn unless $emit_all || @$problems;\n \tmy $c = main::fd_colors(1);\n-\tmy $checked = join(' ', map {$_->[0]} @tokens);\n+\tmy $start = 0;\n+\tmy $checked = '';\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$start = $pos;\n+\t}\n+\t$checked .= substr($body, $start);\n \t$checked =~ s/^\\n//;\n-\t$checked =~ s/^ //mg;\n-\t$checked =~ s/ $//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 .= \"\\n\" unless $checked =~ /\\n$/;\n \tpush(@{$self->{output}}, \"$c->{blue}# chainlint: $title$c->{reset}\\n$checked\");\ndiff --git a/t/chainlint/block-comment.expect b/t/chainlint/block-comment.expect\nindex d10b2eeaf27..df2beea8887 100644\n--- a/t/chainlint/block-comment.expect\n+++ b/t/chainlint/block-comment.expect\n@@ -1,6 +1,8 @@\n (\n \t{\n+\t\t# show a\n \t\techo a &&\n+\t\t# show b\n \t\techo b\n \t}\n )\ndiff --git a/t/chainlint/case-comment.expect b/t/chainlint/case-comment.expect\nindex 1e4b054bda0..641c157b98c 100644\n--- a/t/chainlint/case-comment.expect\n+++ b/t/chainlint/case-comment.expect\n@@ -1,7 +1,10 @@\n (\n \tcase \"$x\" in\n+\t# found foo\n \tx) foo ;;\n+\t# found other\n \t*)\n+\t\t# treat it as bar\n \t\tbar\n \t\t;;\n \tesac\ndiff --git a/t/chainlint/close-subshell.expect b/t/chainlint/close-subshell.expect\nindex 0f87db9ae68..2192a2870a1 100644\n--- a/t/chainlint/close-subshell.expect\n+++ b/t/chainlint/close-subshell.expect\n@@ -15,7 +15,8 @@\n ) | wuzzle &&\n (\n \tbop\n-) | fazz \tfozz &&\n+) | fazz \\\n+\tfozz &&\n (\n \tbup\n ) |\ndiff --git a/t/chainlint/comment.expect b/t/chainlint/comment.expect\nindex f76fde1ffba..a68f1f9d7c2 100644\n--- a/t/chainlint/comment.expect\n+++ b/t/chainlint/comment.expect\n@@ -1,4 +1,8 @@\n (\n+\t# comment 1\n \tnothing &&\n+\t# comment 2\n \tsomething\n+\t# comment 3\n+\t# comment 4\n )\ndiff --git a/t/chainlint/double-here-doc.expect b/t/chainlint/double-here-doc.expect\nindex 75477bb1add..cd584a43573 100644\n--- a/t/chainlint/double-here-doc.expect\n+++ b/t/chainlint/double-here-doc.expect\n@@ -1,2 +1,12 @@\n-run_sub_test_lib_test_err run-inv-range-start \"--run invalid range start\" --run=\"a-5\" <<-EOF &&\n-check_sub_test_lib_test_err run-inv-range-start <<-EOF_OUT 3 <<-EOF_ERR\n+run_sub_test_lib_test_err run-inv-range-start \\\n+\t\"--run invalid range start\" \\\n+\t--run=\"a-5\" <<-\\EOF &&\n+test_expect_success \"passing test #1\" \"true\"\n+test_done\n+EOF\n+check_sub_test_lib_test_err run-inv-range-start \\\n+\t<<-\\EOF_OUT 3<<-EOF_ERR\n+> FATAL: Unexpected exit with code 1\n+EOF_OUT\n+> error: --run: invalid non-numeric in range start: ${SQ}a-5${SQ}\n+EOF_ERR\ndiff --git a/t/chainlint/empty-here-doc.expect b/t/chainlint/empty-here-doc.expect\nindex f42f2d41ba8..e8733c97c64 100644\n--- a/t/chainlint/empty-here-doc.expect\n+++ b/t/chainlint/empty-here-doc.expect\n@@ -1,3 +1,4 @@\n git ls-tree $tree path > current &&\n-cat > expected <<EOF &&\n+cat > expected <<\\EOF &&\n+EOF\n test_output\ndiff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect\nindex a5810c9bddd..d65c82129a6 100644\n--- a/t/chainlint/for-loop.expect\n+++ b/t/chainlint/for-loop.expect\n@@ -2,7 +2,9 @@\n \tfor i in a b c\n \tdo\n \t\techo $i ?!AMP?!\n-\t\tcat <<-EOF ?!LOOP?!\n+\t\tcat <<-\\EOF ?!LOOP?!\n+\t\tbar\n+\t\tEOF\n \tdone ?!AMP?!\n \tfor i in a b c; do\n \t\techo $i &&\ndiff --git a/t/chainlint/here-doc-close-subshell.expect b/t/chainlint/here-doc-close-subshell.expect\nindex 2af9ced71cc..7d9c2b56070 100644\n--- a/t/chainlint/here-doc-close-subshell.expect\n+++ b/t/chainlint/here-doc-close-subshell.expect\n@@ -1,2 +1,4 @@\n (\n-\tcat <<-INPUT)\n+\tcat <<-\\INPUT)\n+\tfizz\n+\tINPUT\ndiff --git a/t/chainlint/here-doc-indent-operator.expect b/t/chainlint/here-doc-indent-operator.expect\nindex fb6cf7285d0..f92a7ce9992 100644\n--- a/t/chainlint/here-doc-indent-operator.expect\n+++ b/t/chainlint/here-doc-indent-operator.expect\n@@ -1,5 +1,11 @@\n-cat > expect <<-EOF &&\n+cat >expect <<- EOF &&\n+header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0\n+num_commits: $1\n+chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data\n+EOF\n \n-cat > expect <<-EOF ?!AMP?!\n+cat >expect << -EOF ?!AMP?!\n+this is not indented\n+-EOF\n \n cleanup\ndiff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect\nindex f8b3aa73c4f..b7364c82c89 100644\n--- a/t/chainlint/here-doc-multi-line-command-subst.expect\n+++ b/t/chainlint/here-doc-multi-line-command-subst.expect\n@@ -1,5 +1,8 @@\n (\n-\tx=$(bobble <<-END &&\n+\tx=$(bobble <<-\\END &&\n+\t\tfossil\n+\t\tvegetable\n+\t\tEND\n \t\twiffle) ?!AMP?!\n \techo $x\n )\ndiff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect\nindex be64b26869a..6c13bdcbfb5 100644\n--- a/t/chainlint/here-doc-multi-line-string.expect\n+++ b/t/chainlint/here-doc-multi-line-string.expect\n@@ -1,5 +1,7 @@\n (\n-\tcat <<-TXT && echo \"multi-line\n+\tcat <<-\\TXT && echo \"multi-line\n \tstring\" ?!AMP?!\n+\tfizzle\n+\tTXT\n \tbap\n )\ndiff --git a/t/chainlint/here-doc.expect b/t/chainlint/here-doc.expect\nindex 110059ba584..1df3f782821 100644\n--- a/t/chainlint/here-doc.expect\n+++ b/t/chainlint/here-doc.expect\n@@ -1,7 +1,25 @@\n-boodle wobba        gorgo snoot        wafta snurb <<EOF &&\n+boodle wobba \\\n+\tgorgo snoot \\\n+\twafta snurb <<EOF &&\n+quoth the raven,\n+nevermore...\n+EOF\n \n cat <<-Arbitrary_Tag_42 >foo &&\n+snoz\n+boz\n+woz\n+Arbitrary_Tag_42\n \n-cat <<zump >boo &&\n+cat <<\"zump\" >boo &&\n+snoz\n+boz\n+woz\n+zump\n \n-horticulture <<EOF\n+horticulture <<\\EOF\n+gomez\n+morticia\n+wednesday\n+pugsly\n+EOF\ndiff --git a/t/chainlint/if-then-else.expect b/t/chainlint/if-then-else.expect\nindex 44d86c35976..cbaaf857d47 100644\n--- a/t/chainlint/if-then-else.expect\n+++ b/t/chainlint/if-then-else.expect\n@@ -8,7 +8,9 @@\n \t\techo foo\n \telse\n \t\techo foo &&\n-\t\tcat <<-EOF\n+\t\tcat <<-\\EOF\n+\t\tbar\n+\t\tEOF\n \tfi ?!AMP?!\n \techo poodle\n ) &&\ndiff --git a/t/chainlint/incomplete-line.expect b/t/chainlint/incomplete-line.expect\nindex ffac8f90185..134d3a14f5c 100644\n--- a/t/chainlint/incomplete-line.expect\n+++ b/t/chainlint/incomplete-line.expect\n@@ -1,4 +1,10 @@\n-line 1 line 2 line 3 line 4 &&\n+line 1 \\\n+line 2 \\\n+line 3 \\\n+line 4 &&\n (\n-\tline 5 \tline 6 \tline 7 \tline 8\n+\tline 5 \\\n+\tline 6 \\\n+\tline 7 \\\n+\tline 8\n )\ndiff --git a/t/chainlint/inline-comment.expect b/t/chainlint/inline-comment.expect\nindex dd0dace077f..6bad2185300 100644\n--- a/t/chainlint/inline-comment.expect\n+++ b/t/chainlint/inline-comment.expect\n@@ -1,6 +1,6 @@\n (\n-\tfoobar &&\n-\tbarfoo ?!AMP?!\n+\tfoobar && # comment 1\n+\tbarfoo ?!AMP?! # wrong position for &&\n \tflibble \"not a # comment\"\n ) &&\n \ndiff --git a/t/chainlint/loop-detect-status.expect b/t/chainlint/loop-detect-status.expect\nindex 0ad23bb35e4..24da9e86d59 100644\n--- a/t/chainlint/loop-detect-status.expect\n+++ b/t/chainlint/loop-detect-status.expect\n@@ -2,7 +2,7 @@\n do\n \tprintf \"Generating blob $i/$blobcount\\r\" >& 2 &&\n \tprintf \"blob\\nmark :$i\\ndata $blobsize\\n\" &&\n-\n+\t#test-tool genrandom $i $blobsize &&\n \tprintf \"%-${blobsize}s\" $i &&\n \techo \"M 100644 :$i $i\" >> commit &&\n \ti=$(($i+1)) ||\ndiff --git a/t/chainlint/nested-here-doc.expect b/t/chainlint/nested-here-doc.expect\nindex e3bef63f754..29b3832a986 100644\n--- a/t/chainlint/nested-here-doc.expect\n+++ b/t/chainlint/nested-here-doc.expect\n@@ -1,7 +1,30 @@\n cat <<ARBITRARY >foop &&\n+naddle\n+fub <<EOF\n+\tnozzle\n+\tnoodle\n+EOF\n+formp\n+ARBITRARY\n \n (\n-\tcat <<-INPUT_END &&\n-\tcat <<-EOT ?!AMP?!\n+\tcat <<-\\INPUT_END &&\n+\tfish are mice\n+\tbut geese go slow\n+\tdata <<EOF\n+\t\tperl is lerp\n+\t\tand nothing else\n+\tEOF\n+\ttoink\n+\tINPUT_END\n+\n+\tcat <<-\\EOT ?!AMP?!\n+\ttext goes here\n+\tdata <<EOF\n+\t\tdata goes here\n+\tEOF\n+\tmore test here\n+\tEOT\n+\n \tfoobar\n )\ndiff --git a/t/chainlint/nested-subshell-comment.expect b/t/chainlint/nested-subshell-comment.expect\nindex be4b27a305b..9138cf386d3 100644\n--- a/t/chainlint/nested-subshell-comment.expect\n+++ b/t/chainlint/nested-subshell-comment.expect\n@@ -2,6 +2,8 @@\n \tfoo &&\n \t(\n \t\tbar &&\n+\t\t# bottles wobble while fiddles gobble\n+\t\t# minor numbers of cows (or do they?)\n \t\tbaz &&\n \t\tsnaff\n \t) ?!AMP?!\ndiff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect\nindex 029d129299a..52789278d13 100644\n--- a/t/chainlint/subshell-here-doc.expect\n+++ b/t/chainlint/subshell-here-doc.expect\n@@ -1,10 +1,30 @@\n (\n-\techo wobba \t       gorgo snoot \t       wafta snurb <<-EOF &&\n+\techo wobba \\\n+\t\tgorgo snoot \\\n+\t\twafta snurb <<-EOF &&\n+\tquoth the raven,\n+\tnevermore...\n+\tEOF\n+\n \tcat <<EOF >bip ?!AMP?!\n-\techo <<-EOF >bop\n+\tfish fly high\n+EOF\n+\n+\techo <<-\\EOF >bop\n+\tgomez\n+\tmorticia\n+\twednesday\n+\tpugsly\n+\tEOF\n ) &&\n (\n-\tcat <<-ARBITRARY >bup &&\n-\tcat <<-ARBITRARY3 >bup3 &&\n+\tcat <<-\\ARBITRARY >bup &&\n+\tglink\n+\tFIZZ\n+\tARBITRARY\n+\tcat <<-\"ARBITRARY3\" >bup3 &&\n+\tglink\n+\tFIZZ\n+\tARBITRARY3\n \tmeep\n )\ndiff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect\nindex 69167da2f27..71b3b3bc20e 100644\n--- a/t/chainlint/t7900-subtree.expect\n+++ b/t/chainlint/t7900-subtree.expect\n@@ -4,12 +4,16 @@ sub2\n sub3\n sub4\" &&\n \tchks_sub=$(cat <<TXT | sed \"s,^,sub dir/,\"\n+$chks\n+TXT\n ) &&\n \tchkms=\"main-sub1\n main-sub2\n main-sub3\n main-sub4\" &&\n \tchkms_sub=$(cat <<TXT | sed \"s,^,sub dir/,\"\n+$chkms\n+TXT\n ) &&\n \tsubfiles=$(git ls-files) &&\n \tcheck_equal \"$subfiles\" \"$chkms\ndiff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect\nindex f272aa21fee..1f5eaea0fd5 100644\n--- a/t/chainlint/while-loop.expect\n+++ b/t/chainlint/while-loop.expect\n@@ -2,7 +2,9 @@\n \twhile true\n \tdo\n \t\techo foo ?!AMP?!\n-\t\tcat <<-EOF ?!LOOP?!\n+\t\tcat <<-\\EOF ?!LOOP?!\n+\t\tbar\n+\t\tEOF\n \tdone ?!AMP?!\n \twhile true; do\n \t\techo foo &&\n-- \ngitgitgadget\n"},{"id":"466893","messageId":"Y2q78ofF8fsAX8XU@nand.local","threadId":"58769","inReplyTo":"pull.1375.git.git.1667934510.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] chainlint: improve annotated output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-08T20:28:34Z","receivedAt":"2022-11-08T20:28:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 08, 2022 at 07:08:26PM +0000, Eric Sunshine via GitGitGadget wrote:\n> This patch series further improves the output by instead making chainlint.pl\n> annotate the original test definition rather than the parsed token stream,\n> thus preserving indentation (and whitespace, in general), here-doc bodies,\n> etc., which should make it easier for a test author to relate each problem\n> back to the source.\n\nVery nicely done. The changes all seemed reasonable to me (and, in fact,\nthe approach is pretty straightforward -- the diffstat is misleading\nsince many of changes are to chainlint's expected output).\n\nSo I'm happy with it, but let's hear from some other folks who are more\nfamiliar with this area before we start merging it down.\n\n\nThanks,\nTaylor\n"},{"id":"466910","messageId":"221108.86iljpqdvj.gmgdl@evledraar.gmail.com","threadId":"58769","inReplyTo":"pull.1375.git.git.1667934510.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] chainlint: improve annotated output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T22:17:14Z","receivedAt":"2022-11-08T22:30:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 08 2022, Eric Sunshine via GitGitGadget wrote:\n\n> When chainlint detects problems in a test, such as a broken &&-chain, it\n> prints out the test with \"?!FOO?!\" annotations inserted at each problem\n> location. However, rather than annotating the original test definition, it\n> instead dumps out a parsed token representation of the test. Since it lacks\n> comments, indentation, here-doc bodies, and so forth, this tokenized\n> representation can be difficult for the test author to digest and relate\n> back to the original test definition.\n>\n> An earlier patch series[1] improved the output somewhat by colorizing the\n> \"?!FOO?!\" annotations and the \"# chainlint:\" lines, but the output can still\n> be difficult to digest.\n>\n> This patch series further improves the output by instead making chainlint.pl\n> annotate the original test definition rather than the parsed token stream,\n> thus preserving indentation (and whitespace, in general), here-doc bodies,\n> etc., which should make it easier for a test author to relate each problem\n> back to the source.\n>\n> This series was inspired by usability comments from Peff[2] and Ævar[3] and\n> a bit of discussion which followed[4][5].\n>\n> (Note to self: Add Ævar to nerd-snipe blacklist alongside Peff.)\n\nHeh! It's great to see a follow-up to our discussion the other day, and\nhaving the output verbatim & annotated looks much better, especially for\ncomplex tests.\n\nE.g. (taking one at random, after some grepping/skimming), ruining this one:\n\t\n\tdiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n\tindex dcaab7265f5..c27539a773d 100755\n\t--- a/t/t6300-for-each-ref.sh\n\t+++ b/t/t6300-for-each-ref.sh\n\t@@ -1365,8 +1365,7 @@ test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n\t                do\n\t                        GIT_COMMITTER_EMAIL=\"$email@example.com\" \\\n\t                        git tag -m \"tag $subject\" icase-$(printf %02d $nr) &&\n\t-                       nr=$((nr+1))||\n\t-                       return 1\n\t+                       nr=$((nr+1))\n\t                done\n\t        done &&\n\t        git for-each-ref --ignore-case \\\n\nWould, before emit (correct, but a bit of a token-soup):\n\n\t\n\t$ ./t6300-for-each-ref.sh \n\t# chainlint: ./t6300-for-each-ref.sh\n\t# chainlint: for-each-ref --ignore-case works on multiple sort keys\n\t\n\tnr=0 &&\n\tfor email in a A b B\n\tdo\n\tfor subject in a A b B\n\tdo\n\tGIT_COMMITTER_EMAIL=\"$email@example.com\" git tag -m \"tag $subject\" icase-$(printf %02d $nr) &&\n\tnr=$((nr+1)) ?!LOOP?!\n\tdone ?!LOOP?!\n\tdone &&\n\tgit for-each-ref --ignore-case --format=\"%(taggeremail) %(subject) %(refname)\" --sort=refname --sort=subject --sort=taggeremail refs/tags/icase-* > actual &&\n\tcat > expect <<-EOF &&\n\ttest_cmp expect actual\n\terror: bug in the test script: lint error (see '?!...!? annotations above)\n\nBut now it'll instead emit:\n\t\n\t$ ./t6300-for-each-ref.sh\n\t# chainlint: ./t6300-for-each-ref.sh\n\t# chainlint: for-each-ref --ignore-case works on multiple sort keys\n\t        # name refs numerically to avoid case-insensitive filesystem conflicts\n\t        nr=0 &&\n\t        for email in a A b B\n\t        do\n\t                for subject in a A b B\n\t                do\n\t                        GIT_COMMITTER_EMAIL=\"$email@example.com\" \\\n\t                        git tag -m \"tag $subject\" icase-$(printf %02d $nr) &&\n\t                        nr=$((nr+1)) ?!LOOP?!\n\t                done ?!LOOP?!\n\t        done &&\n\t        git for-each-ref --ignore-case \\\n\t                --format=\"%(taggeremail) %(subject) %(refname)\" \\\n\t                --sort=refname \\\n\t                --sort=subject \\\n\t                --sort=taggeremail \\\n\t                refs/tags/icase-* >actual &&\n\t        cat >expect <<-\\EOF &&\n\t        <a@example.com> tag a refs/tags/icase-00\n\t        <a@example.com> tag A refs/tags/icase-01\n\t        <A@example.com> tag a refs/tags/icase-04\n\t        <A@example.com> tag A refs/tags/icase-05\n\t        <a@example.com> tag b refs/tags/icase-02\n\t        <a@example.com> tag B refs/tags/icase-03\n\t        <A@example.com> tag b refs/tags/icase-06\n\t        <A@example.com> tag B refs/tags/icase-07\n\t        <b@example.com> tag a refs/tags/icase-08\n\t        <b@example.com> tag A refs/tags/icase-09\n\t        <B@example.com> tag a refs/tags/icase-12\n\t        <B@example.com> tag A refs/tags/icase-13\n\t        <b@example.com> tag b refs/tags/icase-10\n\t        <b@example.com> tag B refs/tags/icase-11\n\t        <B@example.com> tag b refs/tags/icase-14\n\t        <B@example.com> tag B refs/tags/icase-15\n\t        EOF\n\t        test_cmp expect actual\n\terror: bug in the test script: lint error (see '?!...!? annotations above)\n\nWhich is so much better, i.e. as you're preserving the whitespace &\ncomments, and the \"?!LOOP?!\" is of course much easier to see with the\ncolored output.\n\nI hadn't noticed before that the contents of here-docs was pruned, but\nthat made sense in the previous parser, but having the content.\n\nAlso, and I guess this is an attempt to evade your blacklist. I *did*\nnotice when playing around with this that if I now expand the \"1 while\"\nloop here:\n\n\tmy $s = do { local $/; <$fh> };\n\tclose($fh);\n\tmy $parser = ScriptParser->new(\\$s);\n\t1 while $parser->parse_cmd();\n\nTo something that \"follows along\" with the parser it shouldn't be too\nhard in the future to add line number annotations now. E.g. for\n\"#!/bin/sh\\n\" you'll emit a token like \"\\n\", but the positions will be\n0, 10.\n\nBut that's all for some hypothetical future, this is already much better\n:)\n"},{"id":"466911","messageId":"CAPig+cSWXYhp95kcRn1EHrPW15o_z7uL+TcHO-hf6owP5FQnNw@mail.gmail.com","threadId":"58769","inReplyTo":"221108.86iljpqdvj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/4] chainlint: improve annotated output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-08T22:43:23Z","receivedAt":"2022-11-08T22:43:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 8, 2022 at 5:29 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> On Tue, Nov 08 2022, Eric Sunshine via GitGitGadget wrote:\n> > (Note to self: Add Ævar to nerd-snipe blacklist alongside Peff.)\n>\n> Also, and I guess this is an attempt to evade your blacklist. I *did*\n> notice when playing around with this that if I now expand the \"1 while\"\n> loop here:\n>    [...]\n> To something that \"follows along\" with the parser it shouldn't be too\n> hard in the future to add line number annotations now. E.g. for\n> \"#!/bin/sh\\n\" you'll emit a token like \"\\n\", but the positions will be\n> 0, 10.\n>\n> But that's all for some hypothetical future, this is already much better\n\nMy nerd-snipe blacklist hasn't fully solidified yet, unfortunately (for me).\n"},{"id":"466920","messageId":"CAPig+cTLoyyzU3+qo8UA7iGCh9o9UbowOz_Z_debN3NXoN8=_w@mail.gmail.com","threadId":"58769","inReplyTo":"CAPig+cSWXYhp95kcRn1EHrPW15o_z7uL+TcHO-hf6owP5FQnNw@mail.gmail.com","subject":"Re: [PATCH 0/4] chainlint: improve annotated output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-08T22:52:55Z","receivedAt":"2022-11-08T22:53:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 8, 2022 at 5:43 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Nov 8, 2022 at 5:29 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> > On Tue, Nov 08 2022, Eric Sunshine via GitGitGadget wrote:\n> > > (Note to self: Add Ævar to nerd-snipe blacklist alongside Peff.)\n> >\n> > Also, and I guess this is an attempt to evade your blacklist. I *did*\n> > notice when playing around with this that if I now expand the \"1 while\"\n> > loop here:\n> >    [...]\n> > To something that \"follows along\" with the parser it shouldn't be too\n> > hard in the future to add line number annotations now. E.g. for\n> > \"#!/bin/sh\\n\" you'll emit a token like \"\\n\", but the positions will be\n> > 0, 10.\n> >\n> > But that's all for some hypothetical future, this is already much better\n>\n> My nerd-snipe blacklist hasn't fully solidified yet, unfortunately (for me).\n\nI forgot to add that if you do manage to penetrate my nerd-snipe\nblacklist, such a feature would be built atop the current series (i.e.\nno reason to hold up this series for the \"hypothetical future\", as you\nsay).\n"},{"id":"466947","messageId":"Y2unEeio8cgmBWCX@coredump.intra.peff.net","threadId":"58769","inReplyTo":"Y2q78ofF8fsAX8XU@nand.local","subject":"Re: [PATCH 0/4] chainlint: improve annotated output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-09T13:11:45Z","receivedAt":"2022-11-09T13:11:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 08, 2022 at 03:28:34PM -0500, Taylor Blau wrote:\n\n> On Tue, Nov 08, 2022 at 07:08:26PM +0000, Eric Sunshine via GitGitGadget wrote:\n> > This patch series further improves the output by instead making chainlint.pl\n> > annotate the original test definition rather than the parsed token stream,\n> > thus preserving indentation (and whitespace, in general), here-doc bodies,\n> > etc., which should make it easier for a test author to relate each problem\n> > back to the source.\n> \n> Very nicely done. The changes all seemed reasonable to me (and, in fact,\n> the approach is pretty straightforward -- the diffstat is misleading\n> since many of changes are to chainlint's expected output).\n> \n> So I'm happy with it, but let's hear from some other folks who are more\n> familiar with this area before we start merging it down.\n\nI don't claim to be _that_ familiar with the code itself, but all of the\npatches look reasonable to me. And most importantly, I dug out the state\nof my tree from early September (via the reflog) before I fixed all of\nthe chainlint problems on my local topics. The improvement in the output\nwith this series is night and day.\n\nI was a little surprised that using a class in patch 3 would cause such\na slowdown. But it's not that hard to believe that the workload is so\nheavy on string comparison and manipulation that the overloaded string\nand comparison functions introduce significant overhead. It has been a\nlong time since I've optimized any perl, but I remember the rule of\nthumb being to minimize the number of lines of perl (because all of the\nbuiltin stuff is blazingly fast C, and all of the perl is byte-code).\n\nAt any rate, the result you came up with doesn't look too bad. The only\nrisk is that you forgot to s/$token/$token->[0]/ somewhere, and I\nsuspect we'd have found that in running the tests.\n\nSo it all seems like a step forward to me.\n\n-Peff\n"},{"id":"467029","messageId":"Y2xlDvmXR4UJgofB@nand.local","threadId":"58769","inReplyTo":"Y2unEeio8cgmBWCX@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] chainlint: improve annotated output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-10T02:42:22Z","receivedAt":"2022-11-10T02:42:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 09, 2022 at 08:11:45AM -0500, Jeff King wrote:\n> So it all seems like a step forward to me.\n\nThanks, all. Let's start merging this topic down :-).\n\n\nThanks,\nTaylor\n"}]}