On Sat, Jul 06, 2024 at 02:47:57AM -0400, Eric Sunshine wrote: > On Sat, Jul 6, 2024 at 2:11 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > So, the above example simply can't work correctly given the way > > ScriptParser::parse_cmd() calls ScriptParser::check_test() as soon as > > it encounters a `test_expect_success/failure` invocation since it > > doesn't know if the heredocs have been latched at that point. To make > > it properly robust, rather than immediately calling check_test(), it > > would have to continue consuming commands, and saving the ones which > > match `test_expect_success/failure` invocation, until it finally hits > > a `\n`, and only then call check_test() with each command it saved. > > But that's probably overkill at this point considering that we never > > write code like the above, so the submitted patch[2] is probably good > > enough for now. > > Of course, the more I think about it, the more I dislike relying upon > what is effectively an accident of implementation; i.e. that in the > typical case, the heredoc will already have been latched by the time > ScriptParser::parse_cmd() has identified a `test_expect_success` > command, due to the fact that ShellParser::parse_cmd() has that > special case which peeks for `\n` immediately following some other > command terminator. As such, fixing ScriptParser::parse_cmd() to only > call check_test() once it is sure that a '\n' has been encountered is > becoming more appealing, though it is of course a more invasive and > fundamental change than the posted patch. Rats, I just agreed with your earlier email. ;) I am OK with the slightly hacky version we've posted (modulo the fixes I discussed elsewhere). But if you want to take a little time to explore the more robust fix, I am happy to review it. -Peff
