| Message ID | 20240706060515.GA700151@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] chainlint.pl: fix line number reporting | expand |
On 7/6/24 2:05 AM, Jeff King wrote: > The previous commit taught chainlint.pl to handle test bodies in > heredocs, but there are two small bugs related to line numbers: > > 2. For an invocation like the one above, if the test_expect_success > line is X, then "test body" would correctly start at X+1, since the > hanging newline at the start of the single-quoted test body > increments the count. But for a here-doc, there is an implicit > newline at the end of the token stream before the here-doc starts. > We have to increment "lineno" to account for this. > > Actually, this is not _quite_ correct, as there could be multiple > here-docs, like: > > test_expect_success "$(cat <<END_OF_TITLE)" - <<END_OF_TEST > this is the title > END_OF_TITLE > this is the test > END_OF_TEST > > in which case we'd need to skip past END_OF_TITLE. Given how > unlikely it is for anybody to do this, and since it would only > affect line numbers, it's probably not worth caring about too much. > The solution would probably be to record the starting line number > of each here-doc section in the lexer/shellparser stage. > > Signed-off-by: Jeff King <peff@peff.net> > --- > I actually suspect the "record the heredoc line number" thing would not > be too hard. I.e., turn ShellParser's "heredoc" hash to point to > hashrefs like: "{ content => ..., lineno => ... }". And that would give > us a good spot to stick an "interpolate" boolean later if we want. It turned out to be quite easy. See below for an implementation atop your patch [1/3] (modulo Gmail whitespace damage). Given how simple this ended up being, it probably makes sense to squash this change in, as well. --- >8 --- diff --git a/t/chainlint.pl b/t/chainlint.pl index c9ab79b6b0..b31cb263f8 100755 --- a/t/chainlint.pl +++ b/t/chainlint.pl @@ -174,8 +174,10 @@ sub swallow_heredocs { $$b =~ /(?:\G|\n)$indent\Q$$tag[0]\E(?:\n|\z)/gc; if (pos($$b) > $start) { my $body = substr($$b, $start, pos($$b) - $start); - $self->{parser}->{heredocs}->{$$tag[0]} = - substr($body, 0, length($body) - length($&)); + $self->{parser}->{heredocs}->{$$tag[0]} = { + content => substr($body, 0, length($body) - length($&)), + start_line => $self->{lineno}, + }; $self->{lineno} += () = $body =~ /\n/sg; next; } @@ -624,8 +626,9 @@ sub check_test { my $lineno = $body->[3]; $body = unwrap($body); if ($body eq '-') { - $body = shift @_; - $lineno++; + my $herebody = shift @_; + $body = $herebody->{content}; + $lineno = $herebody->{start_line}; } $self->{ntests}++; my $parser = TestParser->new(\$body); --
On Mon, Jul 08, 2024 at 01:08:02AM -0400, Eric Sunshine wrote: > > I actually suspect the "record the heredoc line number" thing would not > > be too hard. I.e., turn ShellParser's "heredoc" hash to point to > > hashrefs like: "{ content => ..., lineno => ... }". And that would give > > us a good spot to stick an "interpolate" boolean later if we want. > > It turned out to be quite easy. See below for an implementation atop > your patch [1/3] (modulo Gmail whitespace damage). Given how simple > this ended up being, it probably makes sense to squash this change in, > as well. Very nice! I was hoping it would be something like this. I've squashed this in, and confirmed that it fixes the line numbers in my "double" case: test_expect_success "$(cat <<END_OF_PREREQS)" 'here-doc-double' - <<\EOT SOME PREREQS END_OF_PREREQS echo "actual test commands" echo "that should be checked" EOT The bogus line was incorrectly reported as line 2, because we did not account for the first here-doc. -Peff
diff --git a/t/chainlint.pl b/t/chainlint.pl index eba509b8e1..c9ab79b6b0 100755 --- a/t/chainlint.pl +++ b/t/chainlint.pl @@ -620,15 +620,19 @@ sub unwrap { sub check_test { my $self = shift @_; my $title = unwrap(shift @_); - my $body = unwrap(shift @_); - $body = shift @_ if $body eq '-'; + my $body = shift @_; + my $lineno = $body->[3]; + $body = unwrap($body); + if ($body eq '-') { + $body = shift @_; + $lineno++; + } $self->{ntests}++; my $parser = TestParser->new(\$body); my @tokens = $parser->parse(); my $problems = $parser->{problems}; return unless $emit_all || @$problems; my $c = main::fd_colors(1); - my $lineno = $_[1]->[3]; my $start = 0; my $checked = ''; for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) {
The previous commit taught chainlint.pl to handle test bodies in heredocs, but there are two small bugs related to line numbers: 1. Prior to that commit, we'd leave the title and body untouched in @_. So we could later pull the line number out of the $_[1] array element. Now we shift off the front of the array, so we have to remember that element to grab the line number. This is a regression even for regular: test_expect_success 'title' ' test body ' invocations; the lines for ever test started fresh at 0. 2. For an invocation like the one above, if the test_expect_success line is X, then "test body" would correctly start at X+1, since the hanging newline at the start of the single-quoted test body increments the count. But for a here-doc, there is an implicit newline at the end of the token stream before the here-doc starts. We have to increment "lineno" to account for this. Actually, this is not _quite_ correct, as there could be multiple here-docs, like: test_expect_success "$(cat <<END_OF_TITLE)" - <<END_OF_TEST this is the title END_OF_TITLE this is the test END_OF_TEST in which case we'd need to skip past END_OF_TITLE. Given how unlikely it is for anybody to do this, and since it would only affect line numbers, it's probably not worth caring about too much. The solution would probably be to record the starting line number of each here-doc section in the lexer/shellparser stage. Signed-off-by: Jeff King <peff@peff.net> --- Note to the maintainer: do not worry about applying these yet! The parent message describes where they'd go in the series, but I'll send a full series once Eric and I have worked out the details. Review comments welcome, of course. :) I actually suspect the "record the heredoc line number" thing would not be too hard. I.e., turn ShellParser's "heredoc" hash to point to hashrefs like: "{ content => ..., lineno => ... }". And that would give us a good spot to stick an "interpolate" boolean later if we want. t/chainlint.pl | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)