Skip to content

Commit e1d6083

Browse files
committed
t: add greplint to detect bare grep assertions
Add greplint.pl, a lint tool that detects bare 'grep' used as a test assertion (where 'test_grep' should be used) and '! test_grep' (where 'test_grep !' should be used). greplint.pl reuses the shared shell parser from lib-shell-parser.pl to tokenize test bodies. The Lexer collapses heredocs, command substitutions, and quoted strings into single tokens, so 'grep' appearing inside these contexts is not flagged. A flat walk over the token stream tracks command position and pipeline state to distinguish assertion greps from filter greps. For double-quoted test bodies, a splice adjustment table compensates for backslash-continuation lines that the Lexer consumes without emitting into the body text, ensuring accurate line numbers. Add test fixtures in greplint/ (modeled on chainlint/) covering detection of bare grep assertions, correct skipping of filters, pipelines, redirects, command substitutions, and lint-ok annotations. Wire into the Makefile as: - test-greplint: runs greplint.pl on $(T) $(THELPERS) $(TPERF) - check-greplint: runs greplint.pl on fixtures, diffs against expected - clean-greplint: removes temp dir Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
1 parent 34ceda3 commit e1d6083

39 files changed

Lines changed: 367 additions & 5 deletions

t/Makefile

Lines changed: 24 additions & 5 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+
GREPLINTTMP = $(TEST_OUTPUT_DIRECTORY)/greplinttmp
3031
else
3132
TEST_RESULTS_DIRECTORY = test-results
3233
CHAINLINTTMP = chainlinttmp
34+
GREPLINTTMP = greplinttmp
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+
GREPLINTTMP_SQ = $(subst ','\'',$(GREPLINTTMP))
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+
GREPLINTTESTS = $(sort $(patsubst greplint/%.test,%,$(wildcard greplint/*.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))
@@ -63,8 +67,8 @@ test: pre-clean check-meson $(TEST_LINT)
6367
$(CHAINLINTSUPPRESS) $(MAKE) aggregate-results-and-cleanup
6468

6569
ifneq ($(PERL_PATH),)
66-
test: check-chainlint
67-
prove: check-chainlint
70+
test: check-chainlint check-greplint
71+
prove: check-chainlint check-greplint
6872
endif
6973

7074
failed:
@@ -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-greplint
106110
$(RM) -r 'trash directory'.*
107111
$(RM) -r valgrind/bin
108112

@@ -120,6 +124,17 @@ check-chainlint:
120124
{ $(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual || true; } && \
121125
diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual
122126

127+
clean-greplint:
128+
$(RM) -r '$(GREPLINTTMP_SQ)'
129+
130+
check-greplint:
131+
@mkdir -p '$(GREPLINTTMP_SQ)' && \
132+
'$(PERL_PATH_SQ)' greplint-cat.pl '$(GREPLINTTMP_SQ)' $(GREPLINTTESTS) && \
133+
{ '$(PERL_PATH_SQ)' greplint.pl \
134+
$(patsubst %,greplint/%.test,$(GREPLINTTESTS)) \
135+
>'$(GREPLINTTMP_SQ)'/actual 2>&1 || true; } && \
136+
diff -u '$(GREPLINTTMP_SQ)'/expect '$(GREPLINTTMP_SQ)'/actual
137+
123138
check-meson:
124139
@# awk acts up when trying to match single quotes, so we use \047 instead.
125140
@mkdir -p mesontmp && \
@@ -139,7 +154,7 @@ check-meson:
139154
test-lint: test-lint-duplicates test-lint-executable \
140155
test-lint-filenames
141156
ifneq ($(PERL_PATH),)
142-
test-lint: test-lint-shell-syntax
157+
test-lint: test-lint-shell-syntax test-greplint
143158
else
144159
GIT_TEST_CHAIN_LINT = 0
145160
endif
@@ -160,6 +175,9 @@ test-lint-executable:
160175
test-lint-shell-syntax:
161176
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)
162177

178+
test-greplint:
179+
@'$(PERL_PATH_SQ)' greplint.pl $(T) $(THELPERS) $(TPERF)
180+
163181
test-lint-filenames:
164182
@# We do *not* pass a glob to ls-files but use grep instead, to catch
165183
@# non-ASCII characters (which are quoted within double-quotes)
@@ -185,7 +203,8 @@ perf:
185203
$(MAKE) -C perf/ all
186204

187205
.PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
188-
check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS)
206+
check-chainlint clean-chainlint test-chainlint \
207+
check-greplint clean-greplint test-greplint $(UNIT_TESTS)
189208

190209
.PHONY: libgit-sys-test libgit-rs-test
191210
libgit-sys-test:

t/greplint-cat.pl

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

t/greplint.pl

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,221 @@
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.
6+
#
7+
# The shared shell parser tokenizes test bodies so that 'grep'
8+
# inside heredocs, command substitutions like $(grep ...), and
9+
# quoted strings is collapsed into a single token and never seen
10+
# by our check. Without the parser, a regex approach would
11+
# false-positive on patterns like:
12+
#
13+
# write_script foo.sh <<-\EOF
14+
# grep pattern file # data, not an assertion
15+
# EOF
16+
#
17+
# cmd 2>&1 |
18+
# grep pattern # continuation of pipeline
19+
#
20+
# The parser handles both by construction.
21+
22+
use strict;
23+
use warnings;
24+
use File::Basename;
25+
do(dirname($0) . "/lib-shell-parser.pl")
26+
or die "$0: failed to load lib-shell-parser.pl: $@$!\n";
27+
28+
my $exit_code = 0;
29+
30+
# GrepLintParser inherits ScriptParser's ability to find
31+
# test_expect_success/failure blocks and call check_test()
32+
# on each body. We override check_test() to walk the token
33+
# stream looking for bare grep assertions.
34+
package GrepLintParser;
35+
36+
our @ISA = ('ScriptParser');
37+
38+
# After these tokens, the next token is a command word.
39+
# For example, in 'echo foo && grep bar file', the 'grep'
40+
# after '&&' is at command position and should be flagged.
41+
my %cmd_start;
42+
@cmd_start{qw(&& || ; ;; do then else elif), "\n", '{', '('} = ();
43+
44+
# Subset of cmd_start that also ends a pipeline.
45+
# 'echo | grep foo && grep bar' -- the second grep is NOT
46+
# in a pipeline because '&&' ended it. But 'echo | \n grep'
47+
# keeps the pipeline across the newline.
48+
my %pipe_end;
49+
@pipe_end{qw(&& || ; ;;)} = ();
50+
51+
# If any of these appear after grep (before the next command
52+
# separator), grep's output is being piped or redirected,
53+
# so it is a filter, not an assertion.
54+
# For example: 'grep pattern file | wc -l' or
55+
# 'grep pattern file >output'
56+
my %filter_op;
57+
@filter_op{'|', '>', '>>', '<'} = ();
58+
59+
# Check if the raw source line has a '# lint-ok' annotation
60+
# that suppresses the warning.
61+
sub lint_ok {
62+
my ($raw_lines, $ln) = @_;
63+
return 0 if $ln < 1 || $ln > @$raw_lines;
64+
return $raw_lines->[$ln - 1] =~ /lint-ok/;
65+
}
66+
67+
# Scan forward from 'grep' to see if its output is piped or
68+
# redirected (making it a filter, not an assertion). Stops
69+
# at the next command separator.
70+
#
71+
# Correctly handles compound-command redirects like
72+
# '{ grep pattern file; } >output' because the scan passes
73+
# through '}' (not in %cmd_start) and finds '>'.
74+
sub is_filter {
75+
my ($tokens, $pos) = @_;
76+
for (my $j = $pos + 1; $j < @$tokens; $j++) {
77+
my $t = $tokens->[$j]->[0];
78+
return 0 if exists $cmd_start{$t};
79+
return 1 if exists $filter_op{$t};
80+
}
81+
return 0;
82+
}
83+
84+
# Called by ScriptParser for each test_expect_success/failure body.
85+
sub check_test {
86+
my $self = shift @_;
87+
my $title = ScriptParser::unwrap(shift @_);
88+
my $body_token = shift @_;
89+
my $lineno = $body_token->[3];
90+
my $body = ScriptParser::unwrap($body_token);
91+
if ($body eq '-') {
92+
my $herebody = shift @_;
93+
if ($herebody) {
94+
$body = $herebody->{content};
95+
$lineno = $herebody->{start_line};
96+
}
97+
}
98+
return unless $body;
99+
100+
# Double-quoted bodies can have line splices (\<newline>)
101+
# that the Lexer consumes without emitting into the body
102+
# text. For example:
103+
#
104+
# test_expect_success "title" "
105+
# x=\$(echo \
106+
# hello) &&
107+
# grep pattern file
108+
# "
109+
#
110+
# The '\' on line 2 splices lines 2-3 into one body line,
111+
# so the inner parser's line count falls behind the source.
112+
# Build a table mapping each inner-parser line to the number
113+
# of splices before it, so we can adjust the reported line.
114+
my $raw_lines = $self->{raw_lines};
115+
my @splice_adj;
116+
if ($body_token->[0] =~ /^"/) {
117+
my $splice_count = 0;
118+
my $body_line = 0;
119+
for my $src_ln ($lineno .. ($body_token->[4] || $lineno)) {
120+
my $line = $raw_lines->[$src_ln - 1];
121+
# Odd trailing backslashes = line splice (\<nl>).
122+
# Even = escaped backslashes (\\), not a splice.
123+
if (defined($line) && $line =~ /(\\*)\s*$/ &&
124+
length($1) % 2 == 1) {
125+
$splice_count++;
126+
} else {
127+
$splice_adj[++$body_line] = $splice_count;
128+
}
129+
}
130+
}
131+
132+
# Re-parse the body text. The Lexer has already collapsed
133+
# heredocs, $(...), and quoted strings into single tokens,
134+
# so 'grep' inside those never appears in this stream.
135+
my $parser = ShellParser->new(\$body);
136+
my @tokens = $parser->parse();
137+
138+
my $file = $self->{file};
139+
my $cmd_pos = 1;
140+
my $in_pipe = 0;
141+
142+
for (my $i = 0; $i < @tokens; $i++) {
143+
my $text = $tokens[$i]->[0];
144+
145+
# Map inner-parser line to file line, adjusting for
146+
# splices in double-quoted bodies.
147+
my $rel = $tokens[$i]->[3] || 0;
148+
my $ln = $rel + ($lineno || 1) - 1;
149+
$ln += $splice_adj[$rel] if @splice_adj && $rel <= $#splice_adj;
150+
151+
# Command separators: next token is a command word.
152+
# 'echo foo && grep bar' -- grep is at cmd position.
153+
if (exists $cmd_start{$text}) {
154+
$cmd_pos = 1;
155+
$in_pipe = 0 if exists $pipe_end{$text};
156+
next;
157+
}
158+
# Pipe: next token is a command word receiving input.
159+
# 'echo foo | grep bar' -- grep is a filter here.
160+
if ($text eq '|') {
161+
$cmd_pos = 1;
162+
$in_pipe = 1;
163+
next;
164+
}
165+
# Close brace/paren: not a command position.
166+
# '{ echo; } && grep' -- grep comes after &&, not }.
167+
if ($text eq '}' || $text eq ')') {
168+
$cmd_pos = 0;
169+
next;
170+
}
171+
172+
# Not at command position -- skip arguments, patterns,
173+
# filenames. 'test_must_fail grep' -- grep is an
174+
# argument to test_must_fail, not a command.
175+
next unless $cmd_pos;
176+
177+
# '!' is a negation prefix; the next token is still
178+
# a command word. Check for the specific anti-pattern
179+
# '! test_grep' which suppresses test_grep's diagnostic
180+
# output on failure.
181+
if ($text eq '!') {
182+
if ($i + 1 < @tokens &&
183+
$tokens[$i + 1]->[0] eq 'test_grep' &&
184+
!lint_ok($raw_lines, $ln)) {
185+
print "$file:$ln: error: ",
186+
'use "test_grep !" instead of ',
187+
'"! test_grep"', "\n";
188+
$exit_code = 1;
189+
}
190+
next;
191+
}
192+
193+
# Flag bare grep at command position that is not
194+
# receiving piped input and not followed by a
195+
# redirect or pipe (i.e., it is an assertion).
196+
if ($text eq 'grep' && !$in_pipe &&
197+
!is_filter(\@tokens, $i) &&
198+
!lint_ok($raw_lines, $ln)) {
199+
print "$file:$ln: error: ",
200+
"bare grep outside pipeline ",
201+
"(use test_grep)\n";
202+
$exit_code = 1;
203+
}
204+
205+
$cmd_pos = 0;
206+
}
207+
}
208+
209+
package main;
210+
211+
for my $file (@ARGV) {
212+
open(my $fh, '<:unix:crlf', $file) or die "$0: $file: $!\n";
213+
my @raw_lines = <$fh>;
214+
close $fh;
215+
my $s = join('', @raw_lines);
216+
my $parser = GrepLintParser->new(\$s);
217+
$parser->{file} = $file;
218+
$parser->{raw_lines} = \@raw_lines;
219+
$parser->parse();
220+
}
221+
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)