Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter
From: Eric Sunshine <hidden>
Date: 2023-03-29 03:13:32
Possibly related (same subject, not in this thread)
- 2023-03-28 · [PATCH 3/4] tests: drop here-doc check from internal chain-linter · Jeff King <hidden>
On Tue, Mar 28, 2023 at 11:04 PM Jeff King [off-list ref] wrote:
quoted hunk ↗ jump to hunk
On Tue, Mar 28, 2023 at 10:37:02PM -0400, Jeff King wrote:quoted
I just think chainlint.pl is doing a good enough job of catching it that we can rely on it. I'll be curious if Eric has input there on whether it can do even better, which would remove all of the caveats from the commit message.So I _think_ it's something like this:@@ -171,6 +171,9 @@ sub swallow_heredocs { my $start = pos($$b); my $indent = $tag =~ s/^\t// ? '\\s*' : ''; $$b =~ /(?:\G|\n)$indent\Q$tag\E(?:\n|\z)/gc; + if (pos($$b) == $start) { + die "oops, we did not find the end of the heredoc"; + } my $body = substr($$b, $start, pos($$b) - $start); $self->{lineno} += () = $body =~ /\n/sg; }But I wasn't sure how to surface a clean error from here, since we're in the Lexer. Maybe we just accumulate a "problems" array here, and then roll those up via the TestParser? I'm not very familiar with the arrangement of that part of the script.
Yes, it would look something like that and you chose the correct spot to detect the problem, but to get a "pretty" error message properly positioned in the input, we need to capture the input stream position of the here-doc tag itself in scan_heredoc_tag(). It doesn't look too difficult, and I even started writing a bit of code to do it, but I'm not sure how soon I can get around to finishing the implementation.
And I say "think" because the thing I was worried about is that we'd do
this lexing at too high a level, and accidentally walk past the end of
the test. Which would mean getting fooled by;
test_expect_success 'this one is broken' '
cat >foo <<\EOF
oops, we are missing our here-doc end
'
test_expect_success 'this one is ok' '
cat >foo <<\EOF
this one is OK, but we would not want to confuse
its closing tag for the missing one
EOF
'
But it looks like Lexer::swallow_heredocs gets to see the individual
test snippets.
Correct. ScriptParser scans the input files for
test_expect_{success,failure} invocations in order to extract the
individual test snippets, which then get passed to TestParser for
semantic analysis.