t: add greplint to detect bare grep assertions

index 7664c6e027..e867f38c71 100644 --- a/t/.gitattributes +++ b/t/.gitattributes @@ -1,5 +1,7 @@ t[0-9][0-9][0-9][0-9]/* -whitespace /chainlint/*.expect eol=lf -whitespace +/greplint/*.expect eol=lf -whitespace +/greplint/*.test eol=lf -whitespace /t0110/url-* binary /t3206/* eol=lf /t3900/*.txt eol=lf

index ab8a5b54aa..e3249879c4 100644 --- a/t/Makefile +++ b/t/Makefile @@ -27,9 +27,11 @@ TEST_LINT ?= test-lint ifdef TEST_OUTPUT_DIRECTORY TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results CHAINLINTTMP = $(TEST_OUTPUT_DIRECTORY)/chainlinttmp +GREPLINTTMP = $(TEST_OUTPUT_DIRECTORY)/greplinttmp else TEST_RESULTS_DIRECTORY = test-results CHAINLINTTMP = chainlinttmp +GREPLINTTMP = greplinttmp endif # Shell quote; @@ -38,6 +40,7 @@ TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH)) PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH)) TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY)) CHAINLINTTMP_SQ = $(subst ','\'',$(CHAINLINTTMP)) +GREPLINTTMP_SQ = $(subst ','\'',$(GREPLINTTMP)) T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)) THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh))) @@ -45,6 +48,7 @@ TLIBS = $(sort $(wildcard lib-*.sh)) annotate-tests.sh TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh)) TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh)) CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test))) +GREPLINTTESTS = $(sort $(patsubst greplint/%.test,%,$(wildcard greplint/*.test))) CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c) 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) $(CHAINLINTSUPPRESS) $(MAKE) aggregate-results-and-cleanup ifneq ($(PERL_PATH),) -test: check-chainlint -prove: check-chainlint +test: check-chainlint check-greplint +prove: check-chainlint check-greplint endif failed: @@ -102,7 +106,7 @@ unit-tests-test-tool: pre-clean: $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)' -clean-except-prove-cache: clean-chainlint +clean-except-prove-cache: clean-chainlint clean-greplint $(RM) -r 'trash directory'.* $(RM) -r valgrind/bin @@ -120,6 +124,17 @@ check-chainlint: { $(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests >'$(CHAINLINTTMP_SQ)'/actual || true; } && \ diff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual +clean-greplint: + $(RM) -r '$(GREPLINTTMP_SQ)' + +check-greplint: + @mkdir -p '$(GREPLINTTMP_SQ)' && \ + '$(PERL_PATH_SQ)' greplint-cat.pl '$(GREPLINTTMP_SQ)' $(GREPLINTTESTS) && \ + { '$(PERL_PATH_SQ)' greplint.pl \ + $(patsubst %,greplint/%.test,$(GREPLINTTESTS)) \ + >'$(GREPLINTTMP_SQ)'/actual 2>&1 || true; } && \ + diff -u '$(GREPLINTTMP_SQ)'/expect '$(GREPLINTTMP_SQ)'/actual + check-meson: @# awk acts up when trying to match single quotes, so we use \047 instead. @mkdir -p mesontmp && \ @@ -139,7 +154,7 @@ check-meson: test-lint: test-lint-duplicates test-lint-executable \ test-lint-filenames ifneq ($(PERL_PATH),) -test-lint: test-lint-shell-syntax +test-lint: test-lint-shell-syntax test-greplint else GIT_TEST_CHAIN_LINT = 0 endif @@ -160,6 +175,9 @@ test-lint-executable: test-lint-shell-syntax: @'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF) +test-greplint: + @'$(PERL_PATH_SQ)' greplint.pl $(T) $(THELPERS) $(TPERF) + test-lint-filenames: @# We do *not* pass a glob to ls-files but use grep instead, to catch @# non-ASCII characters (which are quoted within double-quotes) @@ -185,7 +203,8 @@ perf: $(MAKE) -C perf/ all .PHONY: pre-clean $(T) aggregate-results clean valgrind perf \ - check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS) + check-chainlint clean-chainlint test-chainlint \ + check-greplint clean-greplint test-greplint $(UNIT_TESTS) .PHONY: libgit-sys-test libgit-rs-test libgit-sys-test:

new file mode 100644 index 0000000000..fa9c3b8aab --- /dev/null +++ b/t/greplint-cat.pl @@ -0,0 +1,27 @@ +#!/usr/bin/env perl + +use strict; +use warnings; + +# Assemble expected output for check-greplint target. +# Usage: greplint-cat.pl <outdir> <test-name> ... +# +# For each <test-name>, reads greplint/<test-name>.expect and +# prepends "greplint/<test-name>.test:" to every non-empty line, +# matching the output format of greplint.pl. Writes combined +# expected output to <outdir>/expect. + +my $outdir = shift; +open(my $expect, '>', "$outdir/expect") + or die "unable to open $outdir/expect: $!"; + +for my $name (@ARGV) { + open(my $fh, '<', "greplint/$name.expect") + or die "unable to open greplint/$name.expect: $!"; + while (<$fh>) { + print $expect "greplint/$name.test:$_"; + } + close $fh; +} + +close $expect;

new file mode 100644 index 0000000000..23efe27511 --- /dev/null +++ b/t/greplint.pl @@ -0,0 +1,258 @@ +#!/usr/bin/env perl + +# Detect bare 'grep' used as a test assertion where 'test_grep' +# should be used, and '! test_grep' where 'test_grep !' should +# be used. +# +# The shared shell parser tokenizes test bodies so that 'grep' +# inside heredocs, command substitutions like $(grep ...), and +# quoted strings is collapsed into a single token and never seen +# by our check. A line-oriented approach would need to track +# heredoc delimiters, nested $() depth, and cross-line pipe +# state to avoid false positives on patterns like: +# +# write_script foo.sh <<-\EOF +# grep pattern file # data, not an assertion +# EOF +# +# The Lexer already handles these. + +use warnings; +use strict; +use File::Basename; +do(dirname($0) . "/lib-shell-parser.pl") + or die "$0: failed to load lib-shell-parser.pl: $@$!\n"; + +my $exit_code = 0; + +# GrepLintParser inherits ScriptParser's ability to find +# test_expect_success/failure blocks and call check_test() +# on each body. We override check_test() to walk the token +# stream looking for bare grep assertions. +package GrepLintParser; + +our @ISA = ('ScriptParser'); + +# After these tokens, the next token is a command word. +# For example, in 'echo foo && grep bar file', the 'grep' +# after '&&' is at command position and should be flagged. +my %cmd_start = map { $_ => 1 } qw(&& || ; ;; do then else elif), "\n", '{', '('; + +# Tokens indicating grep's output is piped or redirected. +my %filter_op = map { $_ => 1 } qw(| > >> <); + +# A token is at "command word" position if the shell would +# interpret it as a program name rather than an argument. +# Only 'grep' at command position is an assertion we should +# flag; 'grep' as an argument ('test_must_fail grep') or +# value ('for cmd in grep sed') is not. +sub is_command_word { + my ($tokens, $pos) = @_; + return 1 if $pos == 0; + for (my $j = $pos - 1; $j >= 0; $j--) { + my $t = $tokens->[$j]->[0]; + # After a separator or pipe, a new command starts. + return 1 if $cmd_start{$t} || $t eq '|'; + # After '}' or ')', what follows is a separator or + # redirect on the compound command, not a new command. + return 0 if $t eq '}' || $t eq ')'; + # '!' is a prefix that does not consume command + # position; keep scanning to find what precedes it. + next if $t eq '!'; + # Any other word means we are past the command word. + return 0; + } + return 1; +} + +# lint_ok() reports whether a bare grep carries a trailing +# '# lint-ok' comment telling this linter to skip it. +# +# In practice this is needed for just one case: a grep acting +# as a data filter whose output is consumed by a redirect or +# pipe on an enclosing compound command (such as a subshell or +# brace group) rather than by grep's own pipeline, e.g. +# +# ( grep ... && # lint-ok +# sed ... ) >out +# +# { grep ... || : # lint-ok +# } >out +# +# is_filter() only scans grep's own pipeline: it stops at the +# separator before the compound command closes and never sees +# the outer redirect, so it would flag such a grep as an +# assertion. A grep that really is an assertion is better +# written as test_grep (or a guarded test_grep when the file's +# presence is conditional) than annotated with lint-ok. +sub lint_ok { + my ($raw_lines, $ln) = @_; + if ($ln < 1 || $ln > @$raw_lines) { + warn "lint_ok: line number $ln out of range (1.." . + scalar(@$raw_lines) . ")\n"; + return 0; + } + return $raw_lines->[$ln - 1] =~ /lint-ok/; +} + +# Grep is a filter (not an assertion) if it receives piped +# input or sends its output to a pipe or redirect. Check +# both directions from grep's position in the token stream. +sub is_filter { + my ($tokens, $pos) = @_; + # Backward: is grep receiving piped input? + # Newlines don't break pipes ('cmd |\n grep' is one + # pipeline), so skip past them. + for (my $j = $pos - 1; $j >= 0; $j--) { + my $t = $tokens->[$j]->[0]; + return 1 if $t eq '|'; + next if $t eq "\n"; + last if $cmd_start{$t} || $t eq '}' || $t eq ')'; + } + # Forward: is grep piping or redirecting output? + # Unlike the backward scan, we do not skip newlines here: + # a bare newline is a command boundary, and redirects or + # pipes must appear on the same line as grep (or after a + # line continuation, which the Lexer consumes). + for (my $j = $pos + 1; $j < @$tokens; $j++) { + my $t = $tokens->[$j]->[0]; + return 0 if $cmd_start{$t}; + return 1 if $filter_op{$t}; + } + return 0; +} + +# Map a body-relative line number to a file line number. +# For double-quoted bodies, backslash-continuation lines +# (\<newline>) are consumed by the Lexer without appearing +# in the body text, so the inner parser sees fewer lines +# than the source file has. We walk the source lines to +# count continuations and adjust accordingly. +sub body_to_file_line { + my ($body_lineno, $body_token, $raw_lines, $body_start) = @_; + my $body_text = $body_token->[0]; + my $body_end_line = $body_token->[4]; + unless ($body_start && $body_start >= 1) { + warn "body_start is not a positive integer\n"; + return $body_lineno; + } + my $file_lineno = $body_lineno + $body_start - 1; + # Only double-quoted bodies have line splices. + return $file_lineno unless $body_text =~ /^"/; + my $adj = 0; + my $lines_seen = 0; + unless ($body_end_line && $body_end_line >= $body_start) { + warn "body_end_line is not set for double-quoted body\n"; + return $file_lineno; + } + my $end = $body_end_line; + if ($end > @$raw_lines) { + warn "body_end_line ($end) exceeds file length (" . + scalar(@$raw_lines) . ")\n"; + return $file_lineno; + } + my $src_ln = $body_start; + while ($src_ln <= $end && $lines_seen < $body_lineno) { + my $line = $raw_lines->[$src_ln - 1]; + # Odd trailing backslashes = continuation (\<nl>). + # Even = escaped backslashes (\\), not a continuation. + if ($line =~ /(\\*)$/ && length($1) % 2 == 1) { + $adj++; + } else { + $lines_seen++; + } + $src_ln++; + } + if ($lines_seen < $body_lineno) { + warn "body_lineno ($body_lineno) not found within body range " . + "($body_start..$end)\n"; + } + return $file_lineno + $adj; +} + +# ScriptParser calls this for each test body found in the script. +sub check_test { + my $self = shift @_; + my $title = ScriptParser::unwrap(shift @_); + my $body_token = shift @_; + my $body_start = $body_token->[3]; + my $body = ScriptParser::unwrap($body_token); + # Handle heredoc-style test bodies: + # test_expect_success 'title' - <<\EOF + # grep pattern file + # EOF + # The '-' signals that the body follows as a heredoc. + if ($body eq '-') { + my $herebody = shift @_; + if ($herebody) { + $body = $herebody->{content}; + $body_start = $herebody->{start_line}; + } + } + return unless $body; + + my $raw_lines = $self->{raw_lines}; + + # The outer parser gives us the body as an opaque string. + # Parse it to get individual tokens with command boundaries. + my $parser = ShellParser->new(\$body); + my @tokens = $parser->parse(); + + my $file = $self->{file}; + + for (my $i = 0; $i < @tokens; $i++) { + my $text = $tokens[$i]->[0]; + next unless is_command_word(\@tokens, $i); + + my $token_lineno = $tokens[$i]->[3]; + unless (defined($token_lineno) && $token_lineno >= 1) { + warn "token has no line number\n"; + next; + } + my $file_lineno = body_to_file_line( + $token_lineno, + $body_token, $raw_lines, $body_start); + + # '!' negates the exit code without consuming command + # position. '! test_grep' is an anti-pattern because + # test_grep only prints diagnostics on grep failure, + # and '!' inverts after that decision is already made. + if ($text eq '!') { + if ($i + 1 < @tokens && + $tokens[$i + 1]->[0] eq 'test_grep' && + !lint_ok($raw_lines, $file_lineno)) { + print "$file:$file_lineno: error: ", + 'use "test_grep !" instead of ', + '"! test_grep"', "\n"; + $exit_code = 1; + } + next; + } + + # Bare grep as a command (not a filter) is a test + # assertion that should use test_grep for better + # failure diagnostics. + if ($text eq 'grep' && + !is_filter(\@tokens, $i) && + !lint_ok($raw_lines, $file_lineno)) { + print "$file:$file_lineno: error: ", + "bare grep outside pipeline ", + "(use test_grep)\n"; + $exit_code = 1; + } + } +} + +package main; + +for my $file (@ARGV) { + open(my $fh, '<:unix:crlf', $file) or die "$0: $file: $!\n"; + my @raw_lines = <$fh>; + close $fh; + my $s = join('', @raw_lines); + my $parser = GrepLintParser->new(\$s); + $parser->{file} = $file; + $parser->{raw_lines} = \@raw_lines; + $parser->parse(); +} +exit $exit_code;

new file mode 100644 index 0000000000..7da1b21aa8 --- /dev/null +++ b/t/greplint/bare-grep-after-and.expect @@ -0,0 +1 @@ +3: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..c5b26e4572 --- /dev/null +++ b/t/greplint/bare-grep-after-and.test @@ -0,0 +1,4 @@ +test_expect_success 'grep after && is flagged' ' + cmd && + grep pattern file +'

new file mode 100644 index 0000000000..7da1b21aa8 --- /dev/null +++ b/t/greplint/bare-grep-after-semicolon.expect @@ -0,0 +1 @@ +3: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..c1e468ddf8 --- /dev/null +++ b/t/greplint/bare-grep-after-semicolon.test @@ -0,0 +1,4 @@ +test_expect_success 'grep after semicolon is flagged' ' + echo hello; + grep pattern file +'

new file mode 100644 index 0000000000..eb6210f0c9 --- /dev/null +++ b/t/greplint/bare-grep-compound-body.expect @@ -0,0 +1,3 @@ +4: error: bare grep outside pipeline (use test_grep) +8: error: bare grep outside pipeline (use test_grep) +15: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..72472edcf6 --- /dev/null +++ b/t/greplint/bare-grep-compound-body.test @@ -0,0 +1,17 @@ +test_expect_success 'grep after then/do/else is flagged' ' + if true + then + grep pattern file + fi && + while true + do + grep pattern file && + break + done && + if true + then + echo yes + else + grep pattern file + fi +'

new file mode 100644 index 0000000000..8922d35b42 --- /dev/null +++ b/t/greplint/bare-grep-count-mode.expect @@ -0,0 +1 @@ +2: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..b9c0f18783 --- /dev/null +++ b/t/greplint/bare-grep-count-mode.test @@ -0,0 +1,3 @@ +test_expect_success 'grep -c is flagged (not special-cased)' ' + grep -c pattern file +'

new file mode 100644 index 0000000000..8922d35b42 --- /dev/null +++ b/t/greplint/bare-grep-explicit-pattern.expect @@ -0,0 +1 @@ +2: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..69d8651019 --- /dev/null +++ b/t/greplint/bare-grep-explicit-pattern.test @@ -0,0 +1,3 @@ +test_expect_success 'grep -e is flagged' ' + grep -e pattern file +'

new file mode 100644 index 0000000000..8922d35b42 --- /dev/null +++ b/t/greplint/bare-grep-flags.expect @@ -0,0 +1 @@ +2: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..9ca0e10233 --- /dev/null +++ b/t/greplint/bare-grep-flags.test @@ -0,0 +1,3 @@ +test_expect_success 'grep -E is flagged' ' + grep -E "pat+ern" file +'

new file mode 100644 index 0000000000..e69de29bb2

new file mode 100644 index 0000000000..335b902a42 --- /dev/null +++ b/t/greplint/bare-grep-lint-ok.test @@ -0,0 +1,4 @@ +test_expect_success 'grep with lint-ok annotation is not flagged' ' + grep pattern file && # lint-ok + echo done +'

new file mode 100644 index 0000000000..8922d35b42 --- /dev/null +++ b/t/greplint/bare-grep-negated.expect @@ -0,0 +1 @@ +2: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..ef789bc8b7 --- /dev/null +++ b/t/greplint/bare-grep-negated.test @@ -0,0 +1,3 @@ +test_expect_success 'negated grep is flagged' ' + ! grep pattern file +'

new file mode 100644 index 0000000000..8922d35b42 --- /dev/null +++ b/t/greplint/bare-grep-pattern-file.expect @@ -0,0 +1 @@ +2: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..d70035e449 --- /dev/null +++ b/t/greplint/bare-grep-pattern-file.test @@ -0,0 +1,3 @@ +test_expect_success 'grep -f is flagged' ' + grep -f patterns.txt file +'

new file mode 100644 index 0000000000..8922d35b42 --- /dev/null +++ b/t/greplint/bare-grep-simple.expect @@ -0,0 +1 @@ +2: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..3a8d5f9bbd --- /dev/null +++ b/t/greplint/bare-grep-simple.test @@ -0,0 +1,3 @@ +test_expect_success 'bare grep is flagged' ' + grep pattern file +'

new file mode 100644 index 0000000000..7da1b21aa8 --- /dev/null +++ b/t/greplint/bare-grep-subshell.expect @@ -0,0 +1 @@ +3: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..1fab2d3363 --- /dev/null +++ b/t/greplint/bare-grep-subshell.test @@ -0,0 +1,5 @@ +test_expect_success 'grep in subshell is flagged' ' + ( + grep pattern file + ) +'

new file mode 100644 index 0000000000..8185bfdca4 --- /dev/null +++ b/t/greplint/dqstring-continuation-offset.expect @@ -0,0 +1 @@ +10: error: bare grep outside pipeline (use test_grep)

new file mode 100644 index 0000000000..b332bd8f3b --- /dev/null +++ b/t/greplint/dqstring-continuation-offset.test @@ -0,0 +1,11 @@ +# Double-quoted test bodies with backslash-continuation lines: +# the splice adjustment in check_test compensates for \<newline> +# lines that the lexer consumes without emitting into the body +# text, so the reported line number matches the source. +test_expect_success 'dqstring continuation offset' " + x=\$(echo \ + hello) && + y=\$(echo \ + world) && + grep pattern file +"

new file mode 100644 index 0000000000..e69de29bb2

new file mode 100644 index 0000000000..bd9a100a5d --- /dev/null +++ b/t/greplint/filter-command-substitution.test @@ -0,0 +1,3 @@ +test_expect_success 'grep in command substitution is not flagged' ' + x=$(grep pattern file) +'

new file mode 100644 index 0000000000..e69de29bb2

new file mode 100644 index 0000000000..cb5c0d9e23 --- /dev/null +++ b/t/greplint/filter-pipe-input.test @@ -0,0 +1,3 @@ +test_expect_success 'grep receiving pipe input is not flagged' ' + cmd | grep pattern +'

new file mode 100644 index 0000000000..e69de29bb2

new file mode 100644 index 0000000000..01340086d7 --- /dev/null +++ b/t/greplint/filter-pipe-output.test @@ -0,0 +1,3 @@ +test_expect_success 'grep piping to another command is not flagged' ' + grep pattern file | wc -l +'

new file mode 100644 index 0000000000..e69de29bb2

new file mode 100644 index 0000000000..ee70ba7cde --- /dev/null +++ b/t/greplint/filter-redirect-output.test @@ -0,0 +1,3 @@ +test_expect_success 'grep with output redirect is not flagged' ' + grep pattern file >output +'

new file mode 100644 index 0000000000..e69de29bb2

new file mode 100644 index 0000000000..292db7766e --- /dev/null +++ b/t/greplint/filter-stdin-redirect.test @@ -0,0 +1,3 @@ +test_expect_success 'grep reading from stdin redirect is not flagged' ' + grep pattern <input +'

new file mode 100644 index 0000000000..e69de29bb2

new file mode 100644 index 0000000000..7157228b60 --- /dev/null +++ b/t/greplint/grep-as-argument.test @@ -0,0 +1,3 @@ +test_expect_success 'grep as argument to another command is not flagged' ' + test_must_fail grep pattern file +'

new file mode 100644 index 0000000000..e69de29bb2

new file mode 100644 index 0000000000..92926f6378 --- /dev/null +++ b/t/greplint/grep-as-value.test @@ -0,0 +1,6 @@ +test_expect_success 'grep as value in for-loop is not flagged' ' + for cmd in grep sed awk + do + echo $cmd + done +'

new file mode 100644 index 0000000000..7dc52f65a8 --- /dev/null +++ b/t/greplint/wrong-negation.expect @@ -0,0 +1 @@ +2: error: use "test_grep !" instead of "! test_grep"

new file mode 100644 index 0000000000..542fbd9b28 --- /dev/null +++ b/t/greplint/wrong-negation.test @@ -0,0 +1,3 @@ +test_expect_success 'wrong negation of test_grep is flagged' ' + ! test_grep pattern file +'