Skip to content

Commit e0ed822

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 e0ed822

39 files changed

Lines changed: 310 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

@@ -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-greplint
143147
else
144148
GIT_TEST_CHAIN_LINT = 0
145149
endif
@@ -160,6 +164,20 @@ test-lint-executable:
160164
test-lint-shell-syntax:
161165
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)
162166

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