Skip to content

Commit f749f29

Browse files
committed
t: add check-grep to detect bare grep assertions
Add check-grep.pl, a lint tool that detects bare 'grep' used as a test assertion where 'test_grep' should be used instead, and '! test_grep' where 'test_grep !' should be used (shell negation suppresses the diagnostic output). Wire it into the test-lint target as test-lint-grep. The checker reuses chainlint's shared shell parser to correctly identify command boundaries, so 'grep' appearing as an argument to another command, in heredoc bodies, command substitutions, or strings is not flagged. Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
1 parent 114b565 commit f749f29

2 files changed

Lines changed: 141 additions & 1 deletion

File tree

t/Makefile

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ check-meson:
139139
test-lint: test-lint-duplicates test-lint-executable \
140140
test-lint-filenames
141141
ifneq ($(PERL_PATH),)
142-
test-lint: test-lint-shell-syntax
142+
test-lint: test-lint-shell-syntax test-lint-grep
143143
else
144144
GIT_TEST_CHAIN_LINT = 0
145145
endif
@@ -157,6 +157,9 @@ test-lint-executable:
157157
test -z "$$bad" || { \
158158
echo >&2 "non-executable tests:" $$bad; exit 1; }
159159

160+
test-lint-grep:
161+
@'$(PERL_PATH_SQ)' check-grep.pl $(T) $(TPERF)
162+
160163
test-lint-shell-syntax:
161164
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)
162165

t/check-grep.pl

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
#!/usr/bin/perl
2+
3+
# Detect bare 'grep' used as a test assertion where 'test_grep'
4+
# should be used, and '! test_grep' where 'test_grep !' should
5+
# be used. Uses the shared shell parser to correctly identify
6+
# command boundaries, avoiding false positives from 'grep' in
7+
# arguments, strings, heredocs, or command substitutions.
8+
9+
use strict;
10+
use warnings;
11+
use File::Basename;
12+
13+
my $_lib = dirname($0) . "/lib-shell-parser.pl";
14+
$_lib = "./$_lib" unless $_lib =~ m{^/};
15+
do $_lib or die "$0: failed to load $_lib: $@$!\n";
16+
17+
my $exit_code = 0;
18+
19+
package CheckGrepParser;
20+
our @ISA = ('ScriptParser');
21+
22+
sub check_test {
23+
my $self = shift @_;
24+
my $title = ScriptParser::unwrap(shift @_);
25+
my $body = shift @_;
26+
my $lineno = $body->[3];
27+
$body = ScriptParser::unwrap($body);
28+
if ($body eq '-') {
29+
my $herebody = shift @_;
30+
if ($herebody) {
31+
$body = $herebody->{content};
32+
$lineno = $herebody->{start_line};
33+
}
34+
}
35+
return unless $body;
36+
37+
my $parser = ShellParser->new(\$body);
38+
my @tokens = $parser->parse();
39+
40+
my $raw_lines = $self->{raw_lines};
41+
42+
# Check if the raw source line has a lint-ok annotation.
43+
# Also check adjacent lines to handle backslash-continuation
44+
# line number offsets from the parser.
45+
sub _lint_ok {
46+
my ($raw_lines, $ln) = @_;
47+
for my $l ($ln - 1, $ln, $ln + 1) {
48+
next if $l < 1 || $l > @$raw_lines;
49+
return 1 if $raw_lines->[$l - 1] =~ /lint-ok/;
50+
}
51+
return 0;
52+
}
53+
54+
# Check if the grep at position $pos pipes its output
55+
# or redirects I/O (i.e. used as a filter, not assertion).
56+
sub _is_filter {
57+
my ($tokens, $pos) = @_;
58+
for (my $j = $pos + 1; $j < @$tokens; $j++) {
59+
my $t = $tokens->[$j]->[0];
60+
return 0 if $t =~ /^(?:&&|\|\||;;|\n)$/;
61+
return 1 if $t eq '|';
62+
return 1 if $t =~ /^>{1,2}$/;
63+
return 1 if $t eq '<';
64+
}
65+
return 0;
66+
}
67+
68+
# Walk the token stream. Track command boundaries to
69+
# identify 'grep' as a command word (not an argument).
70+
my $cmd_pos = 1;
71+
my $in_pipe = 0;
72+
my $file = $self->{file};
73+
74+
for (my $i = 0; $i < @tokens; $i++) {
75+
my $text = $tokens[$i]->[0];
76+
my $ln = ($tokens[$i]->[3] || 0) + ($lineno || 1) - 1;
77+
78+
# command separators: next token is a command word
79+
if ($text =~ /^(?:&&|\|\||;;|\n)$/ ||
80+
$text =~ /^[{(]$/ ||
81+
$text =~ /^(?:do|then|else|elif)$/) {
82+
$cmd_pos = 1;
83+
$in_pipe = 0 if $text =~ /^(?:&&|\|\||;;)$/;
84+
next;
85+
}
86+
if ($text eq '|') {
87+
$cmd_pos = 1;
88+
$in_pipe = 1;
89+
next;
90+
}
91+
if ($text =~ /^[})]$/) {
92+
$cmd_pos = 0;
93+
next;
94+
}
95+
96+
next unless $cmd_pos;
97+
98+
# '!' is a negation prefix, stay in command position
99+
if ($text eq '!') {
100+
if ($i + 1 < @tokens &&
101+
$tokens[$i + 1]->[0] eq 'test_grep' &&
102+
!_lint_ok($raw_lines, $ln)) {
103+
print "$file:$ln: error: ",
104+
'use "test_grep !" instead of ',
105+
'"! test_grep"', "\n";
106+
$exit_code = 1;
107+
}
108+
next;
109+
}
110+
111+
# bare grep at command position, not in a pipeline
112+
if ($text eq 'grep' && !$in_pipe &&
113+
!_is_filter(\@tokens, $i) &&
114+
!_lint_ok($raw_lines, $ln)) {
115+
print "$file:$ln: error: ",
116+
"bare grep outside pipeline ",
117+
"(use test_grep)\n";
118+
$exit_code = 1;
119+
}
120+
121+
$cmd_pos = 0;
122+
}
123+
}
124+
125+
package main;
126+
127+
for my $file (@ARGV) {
128+
open(my $fh, '<:unix:crlf', $file) or die "$0: $file: $!\n";
129+
my @raw_lines = <$fh>;
130+
close $fh;
131+
my $s = join('', @raw_lines);
132+
my $parser = CheckGrepParser->new(\$s);
133+
$parser->{file} = $file;
134+
$parser->{raw_lines} = \@raw_lines;
135+
$parser->parse();
136+
}
137+
exit $exit_code;

0 commit comments

Comments
 (0)