| Message ID | 20230328202819.GC1241631@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | some chainlint fixes and performance improvements | expand |
Jeff King <peff@peff.net> writes: > I'd also be OK dropping this. 12% is nice, but this one test is an > outlier. Picking t4202 somewhat at random as a more realistic test, any > improvement seems to be mostly lost in the noise. > > t/test-lib.sh | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index cfcbd899c5a..0048ec7b6f6 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1101,9 +1101,10 @@ test_run_ () { > trace= > # 117 is magic because it is unlikely to match the exit > # code of other programs > - if test "OK-117" != "$(test_eval_ "fail_117 && $1${LF}${LF}echo OK-\$?" 3>&1)" > + test_eval_ "fail_117 && $1" > + if test $? != 117 > then > - BUG "broken &&-chain or run-away HERE-DOC: $1" > + BUG "broken &&-chain: $1" > fi This "here-doc" linter is a cute trick. With seemingly so little extra code, it catches a breakage in such an unexpected way. Even with a very small code footprint, overhead of an extra process is still there, and it would be very hard to figure out what it does (once you are told what it does, you can probably figure out how it works). These make it a "cute trick". While it is a bit sad to see it lost, the resulting code certainly is easier to follow, I would think. I do not offhand know how valuable detecting unterminated here-doc is, compared to the increased clarity of hte code. Thanks.
On Tue, Mar 28, 2023 at 02:46:12PM -0700, Junio C Hamano wrote: > > - if test "OK-117" != "$(test_eval_ "fail_117 && $1${LF}${LF}echo OK-\$?" 3>&1)" > > + test_eval_ "fail_117 && $1" > > + if test $? != 117 > > then > > - BUG "broken &&-chain or run-away HERE-DOC: $1" > > + BUG "broken &&-chain: $1" > > fi > > This "here-doc" linter is a cute trick. With seemingly so little > extra code, it catches a breakage in such an unexpected way. > > Even with a very small code footprint, overhead of an extra process > is still there, and it would be very hard to figure out what it does > (once you are told what it does, you can probably figure out how it > works). These make it a "cute trick". Yes, I thought it was quite clever (and it is attributed to you), but I agree that I did not quite realize what it was doing until after running git-blame. I only started with "gee, why didn't we write this in the simpler way?", which is often a good starting point for archaeology. :) > While it is a bit sad to see it lost, the resulting code certainly > is easier to follow, I would think. I do not offhand know how > valuable detecting unterminated here-doc is, compared to the > increased clarity of hte code. I think the complexity is merited _if_ we think it is catching useful cases. And I do count unterminated here-doc as a useful case, because, as with broken &&-chains, the failure mode is so nasty (a test will report success, even though part of the test was simply not run!). 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. -Peff
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: diff --git a/t/chainlint.pl b/t/chainlint.pl index e966412999a..6b8c1de5208 100755 --- a/t/chainlint.pl +++ b/t/chainlint.pl @@ -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. 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. -Peff
On Tue, Mar 28, 2023 at 10:37 PM Jeff King <peff@peff.net> wrote: > On Tue, Mar 28, 2023 at 02:46:12PM -0700, Junio C Hamano wrote: > > > - if test "OK-117" != "$(test_eval_ "fail_117 && $1${LF}${LF}echo OK-\$?" 3>&1)" > > > - BUG "broken &&-chain or run-away HERE-DOC: $1" > > > > This "here-doc" linter is a cute trick. With seemingly so little > > extra code, it catches a breakage in such an unexpected way. > > > > Even with a very small code footprint, overhead of an extra process > > is still there, and it would be very hard to figure out what it does > > (once you are told what it does, you can probably figure out how it > > works). These make it a "cute trick". > > Yes, I thought it was quite clever (and it is attributed to you), but I > agree that I did not quite realize what it was doing until after running > git-blame. I only started with "gee, why didn't we write this in the > simpler way?", which is often a good starting point for archaeology. :) I never realized what the "OK-" part of "OK-117" was doing either. I guess I should have gone spelunking through history to find out, though it wasn't high-priority for me to know with regards to my work on chainlint.pl, so I never got around to it. I suspect that the "OK-" trick was discussed and added during the period I was off-list. > > While it is a bit sad to see it lost, the resulting code certainly > > is easier to follow, I would think. I do not offhand know how > > valuable detecting unterminated here-doc is, compared to the > > increased clarity of hte code. > > I think the complexity is merited _if_ we think it is catching useful > cases. And I do count unterminated here-doc as a useful case, because, > as with broken &&-chains, the failure mode is so nasty (a test will > report success, even though part of the test was simply not run!). > > 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. It was an intentional design choice, for the sake of simplicity, _not_ to make chainlint.pl a shell syntax checker, despite the fact that it is genuinely parsing shell code. After all, the shell itself -- by which test code will ultimately be executed -- is more than capable of diagnosing syntax errors, so why burden chainlint.pl with all that extra baggage? Instead, chainlint.pl is focussed on detecting semantic problems -- such as broken &&-chain and missing `return 1` -- which are of importance to _this_ project. So, it was very much deliberate that chainlint.pl does not diagnose an unclosed here-doc. When making that design choice, though, I wasn't aware of 99a64e4b73c (tests: lint for run-away here-doc, 2017-03-22), thus didn't know that our test framework had been allowing such problems to slip through silently[1]. That said, it doesn't look too hard to make chainlint.pl diagnose an unclosed here-doc. [1]: Why is that, by the way? Is `eval` not complaining about the unclosed here-doc? Is that something that can be fixed more generally? Are there other syntactic problems that go unnoticed?
On Tue, Mar 28, 2023 at 11:04 PM Jeff King <peff@peff.net> wrote: > 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. > 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.
On Tue, Mar 28, 2023 at 11:13 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > On Tue, Mar 28, 2023 at 11:04 PM Jeff King <peff@peff.net> wrote: > > So I _think_ it's something like this: > > > > 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. The attached patch seems to do the job. Apologies for Gmail messing up the whitespace. It's also attached to the email. This would probably make a good preparatory patch to your [3/4]. As mentioned earlier in the thread, the changes to scan_heredoc_tag () capture the input-stream position of the here-doc tag itself, which is necessary since it would be too late to do so by the time the error is detected by swallow_heredocs(). I don't now when I'll get time to send this as a proper patch, so feel free to write a commit message and incorporate it into your series if you want to use it. And, of course, you have my sign-off already in the patch. It should be easy to add a test, as well, in t/chainlint, perhaps as unclosed-here-doc.{text,expect}. --- >8 --- From b7103da900dd843aabb17201bc0f9ef0b7a704ba Mon Sep 17 00:00:00 2001 From: Eric Sunshine <sunshine@sunshineco.com> Date: Tue, 28 Mar 2023 23:35:33 -0400 Subject: [PATCH] chainlint: diagnose unclosed here-doc Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- t/chainlint.pl | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/t/chainlint.pl b/t/chainlint.pl index e966412999..3dac033ace 100755 --- a/t/chainlint.pl +++ b/t/chainlint.pl @@ -80,7 +80,8 @@ sub scan_heredoc_tag { return "<<$indented" unless $token; my $tag = $token->[0]; $tag =~ s/['"\\]//g; - push(@{$self->{heretags}}, $indented ? "\t$tag" : "$tag"); + $$token[0] = "\t$tag" if $indented; + push(@{$self->{heretags}}, $token); return "<<$indented$tag"; } @@ -169,10 +170,18 @@ sub swallow_heredocs { my $tags = $self->{heretags}; while (my $tag = shift @$tags) { my $start = pos($$b); - my $indent = $tag =~ s/^\t// ? '\\s*' : ''; - $$b =~ /(?:\G|\n)$indent\Q$tag\E(?:\n|\z)/gc; + my $indent = $$tag[0] =~ s/^\t// ? '\\s*' : ''; + $$b =~ /(?:\G|\n)$indent\Q$$tag[0]\E(?:\n|\z)/gc; + if (pos($$b) > $start) { + my $body = substr($$b, $start, pos($$b) - $start); + $self->{lineno} += () = $body =~ /\n/sg; + next; + } + push(@{$self->{parser}->{problems}}, ['HERE', $tag]); + $$b =~ /(?:\G|\n).*\z/gc; # consume rest of input my $body = substr($$b, $start, pos($$b) - $start); $self->{lineno} += () = $body =~ /\n/sg; + last; } } -- 2.40.0.460.g7fdda0a984 --- >8 ---
On Tue, Mar 28, 2023 at 11:46 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > The attached patch seems to do the job. Apologies for Gmail messing up > the whitespace. It's also attached to the email. > > $tag =~ s/['"\\]//g; > - push(@{$self->{heretags}}, $indented ? "\t$tag" : "$tag"); > + $$token[0] = "\t$tag" if $indented; > + push(@{$self->{heretags}}, $token); > return "<<$indented$tag"; Bah, the `$$token[0] = ...` line is incorrect. It should be: $$token[0] = $indented ? "\t$tag" : "$tag"; which more correctly matches the original code. Without this fix, $$token[0] only gets the cleaned tag name in the `<<-EOF` case but not in the plain `<<EOF` case.
On Tue, Mar 28, 2023 at 11:46:37PM -0400, Eric Sunshine wrote: > > 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. > > The attached patch seems to do the job. Apologies for Gmail messing up > the whitespace. It's also attached to the email. Thanks! I was going to say "please don't consider this urgent", but I see that my nerd-snipe was successful. ;) > This would probably make a good preparatory patch to your [3/4]. As > mentioned earlier in the thread, the changes to scan_heredoc_tag () > capture the input-stream position of the here-doc tag itself, which is > necessary since it would be too late to do so by the time the error is > detected by swallow_heredocs(). I don't now when I'll get time to send > this as a proper patch, so feel free to write a commit message and > incorporate it into your series if you want to use it. And, of course, > you have my sign-off already in the patch. It should be easy to add a > test, as well, in t/chainlint, perhaps as > unclosed-here-doc.{text,expect}. Thanks, I can take it from here (and I agree doing it as prep for 3/4 is good, as I can then omit a bunch of explanations there). Here are the tests I'll squash in (along with your $indent fix): diff --git a/t/chainlint/unclosed-here-doc-indent.expect b/t/chainlint/unclosed-here-doc-indent.expect new file mode 100644 index 00000000000..6e17bb66336 --- /dev/null +++ b/t/chainlint/unclosed-here-doc-indent.expect @@ -0,0 +1,4 @@ +command_which_is_run && +cat >expect <<-\EOF ?!HERE?! && +we forget to end the here-doc +command_which_is_gobbled diff --git a/t/chainlint/unclosed-here-doc-indent.test b/t/chainlint/unclosed-here-doc-indent.test new file mode 100644 index 00000000000..5c841a9dfd4 --- /dev/null +++ b/t/chainlint/unclosed-here-doc-indent.test @@ -0,0 +1,4 @@ +command_which_is_run && +cat >expect <<-\EOF && +we forget to end the here-doc +command_which_is_gobbled diff --git a/t/chainlint/unclosed-here-doc.expect b/t/chainlint/unclosed-here-doc.expect new file mode 100644 index 00000000000..c53b6b794a7 --- /dev/null +++ b/t/chainlint/unclosed-here-doc.expect @@ -0,0 +1,7 @@ +command_which_is_run && +cat >expect <<\EOF ?!HERE?! && + we try to end the here-doc below, + but the indentation throws us off + since the operator is not "<<-". + EOF +command_which_is_gobbled diff --git a/t/chainlint/unclosed-here-doc.test b/t/chainlint/unclosed-here-doc.test new file mode 100644 index 00000000000..69d3786c348 --- /dev/null +++ b/t/chainlint/unclosed-here-doc.test @@ -0,0 +1,7 @@ +command_which_is_run && +cat >expect <<\EOF && + we try to end the here-doc below, + but the indentation throws us off + since the operator is not "<<-". + EOF +command_which_is_gobbled -Peff
On Tue, Mar 28, 2023 at 11:07:17PM -0400, Eric Sunshine 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. > > It was an intentional design choice, for the sake of simplicity, _not_ > to make chainlint.pl a shell syntax checker, despite the fact that it > is genuinely parsing shell code. After all, the shell itself -- by > which test code will ultimately be executed -- is more than capable of > diagnosing syntax errors, so why burden chainlint.pl with all that > extra baggage? Instead, chainlint.pl is focussed on detecting semantic > problems -- such as broken &&-chain and missing `return 1` -- which > are of importance to _this_ project. Yeah, I'm not too surprised to hear any of that, and I almost wrote off chainlint.pl for this purpose (hence the hand-waving in my commit message). But after realizing it has to find here-docs anyway to ignore them, it seemed reasonable to bend it to my will. ;) Thanks again for your patch. > [1]: Why is that, by the way? Is `eval` not complaining about the > unclosed here-doc? Is that something that can be fixed more generally? > Are there other syntactic problems that go unnoticed? The behavior varies from shell to shell. Historically, I suspect it was a deliberate decision to read until EOF, because it lets you stick arbitrary data at the end of a script, like: $ cat foo.sh my_prog <<\EOF 1 2 3 4 5 6 7 8 [imagine kilobytes of data tables here] without having to worry about terminating it. I think I've seen it with something like: { echo "uudecode <<\EOF | tar tf -" tar cf - Documentation | uuencode - } >foo.sh which makes foo.sh a sort of self-extracting tarball. (As an aside, I was disappointed that I did not have uuencode installed by default on my system. How times have changed. ;) ). These days bash will warn about it, but dash will not: $ bash foo.sh | wc -l foo.sh: line 129028: warning: here-document at line 1 delimited by end-of-file (wanted `EOF') 901 $ dash foo.sh | wc -l 901 Bash still has a zero exit code, though, so aside from the stderr cruft (which is hidden unless you are digging in "-v" output), the tests would pass. I can't remember when bash started warning, though "git log -S" in its repo suggests it was bash 4.0 in 2009. -Peff
On Wed, Mar 29, 2023 at 2:07 AM Jeff King <peff@peff.net> wrote: > On Tue, Mar 28, 2023 at 11:46:37PM -0400, Eric Sunshine wrote: > > The attached patch seems to do the job. Apologies for Gmail messing up > > the whitespace. It's also attached to the email. > > Thanks! I was going to say "please don't consider this urgent", but I > see that my nerd-snipe was successful. ;) As always. My nerd-snipe armor is in failure mode. > > This would probably make a good preparatory patch to your [3/4]. As > > mentioned earlier in the thread, the changes to scan_heredoc_tag () > > capture the input-stream position of the here-doc tag itself, which is > > necessary since it would be too late to do so by the time the error is > > detected by swallow_heredocs(). I don't now when I'll get time to send > > this as a proper patch, so feel free to write a commit message and > > incorporate it into your series if you want to use it. And, of course, > > you have my sign-off already in the patch. It should be easy to add a > > test, as well, in t/chainlint, perhaps as > > unclosed-here-doc.{text,expect}. > > Thanks, I can take it from here (and I agree doing it as prep for 3/4 is > good, as I can then omit a bunch of explanations there). Here are the > tests I'll squash in (along with your $indent fix): Thanks for picking this up. > diff --git a/t/chainlint/unclosed-here-doc-indent.expect b/t/chainlint/unclosed-here-doc-indent.expect > diff --git a/t/chainlint/unclosed-here-doc-indent.test b/t/chainlint/unclosed-here-doc-indent.test > diff --git a/t/chainlint/unclosed-here-doc.expect b/t/chainlint/unclosed-here-doc.expect > diff --git a/t/chainlint/unclosed-here-doc.test b/t/chainlint/unclosed-here-doc.test The new tests look fine.
diff --git a/t/test-lib.sh b/t/test-lib.sh index cfcbd899c5a..0048ec7b6f6 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1101,9 +1101,10 @@ test_run_ () { trace= # 117 is magic because it is unlikely to match the exit # code of other programs - if test "OK-117" != "$(test_eval_ "fail_117 && $1${LF}${LF}echo OK-\$?" 3>&1)" + test_eval_ "fail_117 && $1" + if test $? != 117 then - BUG "broken &&-chain or run-away HERE-DOC: $1" + BUG "broken &&-chain: $1" fi trace=$trace_tmp fi
Commit 99a64e4b73c (tests: lint for run-away here-doc, 2017-03-22) tweaked the chain-lint test to catch unclosed here-docs. It works by adding an extra "echo" command after the test snippet, and checking that it is run (if it gets swallowed by a here-doc, naturally it is not run). The downside here is that we introduced an extra $() substitution, which happens in a subshell. This has a measurable performance impact when run for many tests. The tradeoff in safety was undoubtedly worth it when 99a64e4b73c was written. But these days, the external chainlint.pl does a pretty good job of finding these (even though it's not something it specifically tries to flag). For example, if you have a test like: test_expect_success 'should fail linter' ' some_command >actual && cat >expect <<-\EOF && ok # missing EOF line here test_cmp expect actual ' it will see that the here-doc isn't closed, treat it as not-a-here-doc, and complain that the "ok" line does not have an "&&". So in practice we should be catching these via that linter, although: - the error message is not as good as it could be (the real problem is the unclosed here-doc) - it can be fooled if there are no lines in the here-doc: cat >expect <<-\EOF && # missing EOF line here or if every line in the here-doc has &&-chaining (weird, but possible) Those are sufficiently unlikely that they're not worth worrying too much about. And by switching back to a simpler chain-lint, hyperfine reports a measurable speedup on t3070 (which has 1800 tests): 'HEAD' ran 1.12 ± 0.01 times faster than 'HEAD~1' Signed-off-by: Jeff King <peff@peff.net> --- I didn't look at how hard it would be to teach chainlint.pl to complain about the unclosed here-doc. I think it _might_ actually not be that bad, just because it does already understand here-docs in general. If it handled that, then all of the hand-waving in the second half of the commit message could go away. ;) I'd also be OK dropping this. 12% is nice, but this one test is an outlier. Picking t4202 somewhat at random as a more realistic test, any improvement seems to be mostly lost in the noise. t/test-lib.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)