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

Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Mar 29, 2023, 03:13 UTC
Message-ID
<CAPig+cQexJ9s1f6tLLaq7baeVCKhazPBi_xQ6uybkQcJQ97geg@mail.gmail.com>
In-Reply-To
<20230329030432.GA1801645@coredump.intra.peff.net>
On Tue, Mar 28, 2023 at 11:04 PM Jeff King <peff@peff.net> wrote:
Show 23 quoted lines
> On Tue, Mar 28, 2023 at 10:37:02PM -0400, Jeff King wrote:
> > 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.

Show 18 quoted lines
> 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.

Previous: Jeff KingNext: Eric Sunshine
Message 13 of 32 in “some chainlint fixes and performance improvements”
  1. 0/4 some chainlint fixes and performance improvementsJeff King, Mar 28, 2023
  2. 1/4 tests: run internal chain-linter under "make test"Jeff King, Mar 28, 2023
  3. Ævar Arnfjörð BjarmasonMar 29, 2023
  4. Junio C HamanoMar 29, 2023
  5. Jeff KingMar 29, 2023
  6. Junio C HamanoMar 30, 2023
  7. 2/4 tests: replace chainlint subshell with a functionJeff King, Mar 28, 2023
  8. Junio C HamanoMar 28, 2023
  9. 3/4 tests: drop here-doc check from internal chain-linterJeff King, Mar 28, 2023
  10. Junio C HamanoMar 28, 2023
  11. Jeff KingMar 29, 2023
  12. Jeff KingMar 29, 2023
  13. Eric SunshineMar 29, 2023
  14. Eric SunshineMar 29, 2023
  15. Eric SunshineMar 29, 2023
  16. Jeff KingMar 29, 2023
  17. Eric SunshineMar 29, 2023
  18. Eric SunshineMar 29, 2023
  19. Jeff KingMar 29, 2023
  20. 4/4 tests: skip test_eval_ in internal chain-lintJeff King, Mar 28, 2023
  21. Jeff KingMar 28, 2023
  22. Jeff KingMar 30, 2023
  23. Junio C HamanoMar 30, 2023
  24. 0/5 some chainlint fixes and performance improvementsJeff King, Mar 30, 2023
  25. 1/5 tests: run internal chain-linter under "make test"Jeff King, Mar 30, 2023
  26. 2/5 tests: replace chainlint subshell with a functionJeff King, Mar 30, 2023
  27. 3/5 tests: diagnose unclosed here-doc in chainlint.plJeff King, Mar 30, 2023
  28. Eric SunshineMar 30, 2023
  29. 4/5 tests: drop here-doc check from internal chain-linterJeff King, Mar 30, 2023
  30. 5/5 tests: skip test_eval_ in internal chain-lintJeff King, Mar 30, 2023
  31. Junio C HamanoMar 30, 2023
  32. Jeff KingMar 30, 2023

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

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