Skip to content

Commit ebe85bf

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 60f4f12 commit ebe85bf

39 files changed

Lines changed: 307 additions & 3 deletions

t/Makefile

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,11 @@ TEST_LINT ?= test-lint
2727
ifdef TEST_OUTPUT_DIRECTORY
2828
TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
2929
CHAINLINTTMP = $(TEST_OUTPUT_DIRECTORY)/chainlinttmp
30+
CHECKGREPTMP = $(TEST_OUTPUT_DIRECTORY)/checkgreptmp
3031
else
3132
TEST_RESULTS_DIRECTORY = test-results
3233
CHAINLINTTMP = chainlinttmp
34+
CHECKGREPTMP = checkgreptmp
3335
endif
3436

3537
# Shell quote;
@@ -38,13 +40,15 @@ TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
3840
PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
3941
TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
4042
CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP))
43+
CHECKGREPTMP_SQ = $(subst ','\'',$(CHECKGREPTMP))
4144

4245
T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
4346
THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
4447
TLIBS = $(sort $(wildcard lib-*.sh)) annotate-tests.sh
4548
TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
4649
TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
4750
CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
51+
CHECKGREPTESTS = $(sort $(wildcard check-grep/*.test))
4852
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
4953
UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
5054
UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
@@ -102,7 +106,7 @@ unit-tests-test-tool:
102106
pre-clean:
103107
$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
104108

105-
clean-except-prove-cache: clean-chainlint
109+
clean-except-prove-cache: clean-chainlint clean-check-grep
106110
$(RM) -r 'trash directory'.*
107111
$(RM) -r valgrind/bin
108112

@@ -139,7 +143,7 @@ check-meson:
139143
test-lint: test-lint-duplicates test-lint-executable \
140144
test-lint-filenames
141145
ifneq ($(PERL_PATH),)
142-
test-lint: test-lint-shell-syntax
146+
test-lint: test-lint-shell-syntax test-lint-grep check-check-grep
143147
else
144148
GIT_TEST_CHAIN_LINT = 0
145149
endif
@@ -160,6 +164,19 @@ test-lint-executable:
160164
test-lint-shell-syntax:
161165
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)
162166

167+
test-lint-grep:
168+
@'$(PERL_PATH_SQ)' check-grep.pl $(T) $(THELPERS) $(TPERF)
169+
170+
check-check-grep:
171+
@mkdir -p '$(CHECKGREPTMP_SQ)' && \
172+
'$(PERL_PATH_SQ)' check-grep-cat.pl '$(CHECKGREPTMP_SQ)' $(CHECKGREPTESTS) && \
173+
'$(PERL_PATH_SQ)' check-grep.pl $(CHECKGREPTESTS) \
174+
>'$(CHECKGREPTMP_SQ)'/actual 2>&1; \
175+
diff -u '$(CHECKGREPTMP_SQ)'/expect '$(CHECKGREPTMP_SQ)'/actual
176+
177+
clean-check-grep:
178+
$(RM) -r '$(CHECKGREPTMP_SQ)'
179+
163180
test-lint-filenames:
164181
@# We do *not* pass a glob to ls-files but use grep instead, to catch
165182
@# non-ASCII characters (which are quoted within double-quotes)
@@ -185,7 +202,8 @@ perf:
185202
$(MAKE) -C perf/ all
186203

187204
.PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
188-
check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS)
205+
check-chainlint clean-chainlint test-chainlint \
206+
check-check-grep clean-check-grep test-lint-grep $(UNIT_TESTS)
189207

190208
.PHONY: libgit-sys-test libgit-rs-test
191209
libgit-sys-test:

t/check-grep-cat.pl

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
#!/usr/bin/env perl
2+
3+
# Assemble expected output for check-check-grep target.
4+
# Usage: check-grep-cat.pl <outdir> <test-name> ...
5+
#
6+
# For each <test-name>, reads check-grep/<test-name>.expect and
7+
# prepends "check-grep/<test-name>.test:" to every non-empty line,
8+
# matching the output format of check-grep.pl when given those
9+
# test files as arguments. Writes the combined expected output
10+
# to <outdir>/expect.
11+
12+
use strict;
13+
use warnings;
14+
15+
my $outdir = shift;
16+
open(my $expect, '>', "$outdir/expect")
17+
or die "unable to open $outdir/expect: $!";
18+
19+
for my $name (@ARGV) {
20+
open(my $fh, '<', "check-grep/$name.expect")
21+
or die "unable to open check-grep/$name.expect: $!";
22+
while (<$fh>) {
23+
chomp;
24+
next if $_ eq '';
25+
print $expect "check-grep/$name.test:$_\n";
26+
}
27+
close $fh;
28+
}
29+
30+
close $expect;

t/check-grep.pl

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
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+
# Tokens that mark the start of a new command.
23+
my %cmd_start;
24+
@cmd_start{qw(&& || ;; do then else elif), "\n", '{', '('} = ();
25+
26+
# Subset of cmd_start that also ends a pipeline.
27+
my %pipe_end;
28+
@pipe_end{qw(&& || ;;)} = ();
29+
30+
# Tokens indicating grep is used as a filter (pipe or redirect).
31+
my %filter_op;
32+
@filter_op{'|', '>', '>>', '<'} = ();
33+
34+
# Check if the raw source line has a lint-ok annotation.
35+
# Also check adjacent lines to handle backslash-continuation
36+
# line number offsets from the parser.
37+
sub lint_ok {
38+
my ($raw_lines, $ln) = @_;
39+
for my $l ($ln - 1, $ln, $ln + 1) {
40+
next if $l < 1 || $l > @$raw_lines;
41+
return 1 if $raw_lines->[$l - 1] =~ /lint-ok/;
42+
}
43+
return 0;
44+
}
45+
46+
# Check if the grep at position $pos pipes its output
47+
# or redirects I/O (i.e. used as a filter, not assertion).
48+
sub is_filter {
49+
my ($tokens, $pos) = @_;
50+
for (my $j = $pos + 1; $j < @$tokens; $j++) {
51+
my $t = $tokens->[$j]->[0];
52+
return 0 if exists $cmd_start{$t};
53+
return 1 if exists $filter_op{$t};
54+
}
55+
return 0;
56+
}
57+
58+
sub check_test {
59+
my $self = shift @_;
60+
my $title = ScriptParser::unwrap(shift @_);
61+
my $body_token = shift @_;
62+
my $lineno = $body_token->[3];
63+
my $body = ScriptParser::unwrap($body_token);
64+
if ($body eq '-') {
65+
my $herebody = shift @_;
66+
if ($herebody) {
67+
$body = $herebody->{content};
68+
$lineno = $herebody->{start_line};
69+
}
70+
}
71+
return unless $body;
72+
73+
# For double-quoted bodies, build a table mapping inner-parser
74+
# line numbers to cumulative line-splice adjustments. Line
75+
# splices (\<newline>) consume a source line without emitting
76+
# a newline into the body text, so the inner parser's line
77+
# count falls behind the source.
78+
my $raw_lines = $self->{raw_lines};
79+
my @splice_adj;
80+
if ($body_token->[0] =~ /^"/) {
81+
my $adj = 0;
82+
my $inner = 0;
83+
for my $src_ln ($lineno .. ($body_token->[4] || $lineno)) {
84+
my $line = $raw_lines->[$src_ln - 1];
85+
if (defined($line) && $line =~ /(\\*)\s*$/ &&
86+
length($1) % 2 == 1) {
87+
$adj++;
88+
} else {
89+
$splice_adj[++$inner] = $adj;
90+
}
91+
}
92+
}
93+
94+
my $parser = ShellParser->new(\$body);
95+
my @tokens = $parser->parse();
96+
97+
my $file = $self->{file};
98+
my $cmd_pos = 1;
99+
my $in_pipe = 0;
100+
101+
for (my $i = 0; $i < @tokens; $i++) {
102+
my $text = $tokens[$i]->[0];
103+
my $rel = $tokens[$i]->[3] || 0;
104+
my $ln = $rel + ($lineno || 1) - 1;
105+
$ln += $splice_adj[$rel] if @splice_adj && $rel <= $#splice_adj;
106+
107+
if (exists $cmd_start{$text}) {
108+
$cmd_pos = 1;
109+
$in_pipe = 0 if exists $pipe_end{$text};
110+
next;
111+
}
112+
if ($text eq '|') {
113+
$cmd_pos = 1;
114+
$in_pipe = 1;
115+
next;
116+
}
117+
if ($text eq '}' || $text eq ')') {
118+
$cmd_pos = 0;
119+
next;
120+
}
121+
122+
next unless $cmd_pos;
123+
124+
# '!' is a negation prefix, stay in command position
125+
if ($text eq '!') {
126+
if ($i + 1 < @tokens &&
127+
$tokens[$i + 1]->[0] eq 'test_grep' &&
128+
!lint_ok($raw_lines, $ln)) {
129+
print "$file:$ln: error: ",
130+
'use "test_grep !" instead of ',
131+
'"! test_grep"', "\n";
132+
$exit_code = 1;
133+
}
134+
next;
135+
}
136+
137+
# bare grep at command position, not in a pipeline
138+
if ($text eq 'grep' && !$in_pipe &&
139+
!is_filter(\@tokens, $i) &&
140+
!lint_ok($raw_lines, $ln)) {
141+
print "$file:$ln: error: ",
142+
"bare grep outside pipeline ",
143+
"(use test_grep)\n";
144+
$exit_code = 1;
145+
}
146+
147+
$cmd_pos = 0;
148+
}
149+
}
150+
151+
package main;
152+
153+
for my $file (@ARGV) {
154+
open(my $fh, '<:unix:crlf', $file) or die "$0: $file: $!\n";
155+
my @raw_lines = <$fh>;
156+
close $fh;
157+
my $s = join('', @raw_lines);
158+
my $parser = CheckGrepParser->new(\$s);
159+
$parser->{file} = $file;
160+
$parser->{raw_lines} = \@raw_lines;
161+
$parser->parse();
162+
}
163+
exit $exit_code;
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
3: error: bare grep outside pipeline (use test_grep)
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
test_expect_success 'grep after && is flagged' '
2+
cmd &&
3+
grep pattern file
4+
'
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
4: error: bare grep outside pipeline (use test_grep)
2+
8: error: bare grep outside pipeline (use test_grep)
3+
15: error: bare grep outside pipeline (use test_grep)
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
test_expect_success 'grep after then/do/else is flagged' '
2+
if true
3+
then
4+
grep pattern file
5+
fi &&
6+
while true
7+
do
8+
grep pattern file &&
9+
break
10+
done &&
11+
if true
12+
then
13+
echo yes
14+
else
15+
grep pattern file
16+
fi
17+
'
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
2: error: bare grep outside pipeline (use test_grep)
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
test_expect_success 'grep -c is flagged (not special-cased)' '
2+
grep -c pattern file
3+
'
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
2: error: bare grep outside pipeline (use test_grep)

0 commit comments

Comments
 (0)