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.
