t: add lint-style.pl and convert grep to test_grep by mmontalbo · Pull Request #2135 · gitgitgadget/git

mmontalbo · 2026-06-02T21:21:30Z

test_grep is a wrapper around grep for test assertions that prints the
file contents on failure for easier debugging. Bare grep fails
silently, making it hard to diagnose what went wrong. Many tests still
use bare grep for assertions.

This series documents test_grep, fixes three pre-existing bugs where
grep assertions were missing file arguments, mechanically converts
assertion greps to test_grep across the suite, and adds check-grep.pl
to prevent new bare grep assertions from being introduced.

check-grep.pl is a line-oriented lint tool following the same pattern as
check-non-portable-shell.pl. It uses heuristics to distinguish
assertion greps from filter greps (pipes, redirects, command
substitutions, conditionals) and also catches the '! test_grep' mistake
where shell negation suppresses the diagnostic output. It is wired into
the test-lint target as test-lint-grep.

Patch 1 adds a t/README entry for test_grep.

Patch 2 fixes three greps missing file arguments (t2402, t7507,
t7700); they were reading empty stdin and passing vacuously.

Patch 3 mechanically converts 2887 assertion greps to test_grep
and rewrites 4 existing '! test_grep' to 'test_grep !'.

Patch 4 adds check-grep.pl and wires it into test-lint as
test-lint-grep.

Changes since v1:

Replaced large lint-style.pl tool with check-grep.pl, a small
line-oriented checker matching the style of
check-non-portable-shell.pl.
Dropped --fix mode. The conversion commit documents how it was
produced, following the pattern from 6789275 (tests: teach
callers of test_i18ngrep to use test_grep).

cc: "D. Ben Knoble" ben.knoble@gmail.com

test_grep is a wrapper around grep for test assertions that prints the file contents on failure for easier debugging. It also accepts '!' as its first argument for negation, which preserves the diagnostic output that '! test_grep' would suppress. Despite being widely used (and the preferred replacement for bare grep in assertions), test_grep has no entry in t/README alongside the other documented helpers like test_cmp and test_line_count. Add one. Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>

mmontalbo · 2026-06-04T04:27:44Z

/preview

gitgitgadget · 2026-06-04T04:28:59Z

Preview email sent as pull.2135.git.1780547320.gitgitgadget@gmail.com

mmontalbo · 2026-06-04T07:44:54Z

/submit

gitgitgadget · 2026-06-04T07:46:30Z

Submitted as pull.2135.git.1780559158.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2135/mmontalbo/mm/test-grep-docs-v1

To fetch this version to local tag pr-2135/mmontalbo/mm/test-grep-docs-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2135/mmontalbo/mm/test-grep-docs-v1

gitgitgadget · 2026-06-04T20:19:24Z

@@ -1,5 +1,7 @@
 t[0-9][0-9][0-9][0-9]/* -whitespace


"D. Ben Knoble" wrote on the Git mailing list (how to reply to this email):

Hi Michael, This sounds like a neat effort! One drive-by comment… On Thu, Jun 4, 2026 at 3:46 AM Michael Montalbo via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Michael Montalbo <mmontalbo@gmail.com> > > Add a mechanical lint checker for test scripts, similar in spirit to > check-non-portable-shell.pl but focused on test conventions rather > than portability. > > The tool defines LintParser, a subclass of ScriptParser (from the > shared lib-shell-parser.pl module). ScriptParser's > parse_cmd() finds test_expect_success blocks and calls check_test() > for each body; LintParser overrides check_test() to run lint rules > on the parsed commands. A "# lint-ok" comment suppresses all > checks for intentional style violations. > > The first rule detects '! test_grep' and replaces it with > 'test_grep !'. Shell-level negation suppresses the diagnostic > output that test_grep prints on failure; the built-in negation > preserves it. > > Three violations inside test bodies are converted via --fix. One > additional violation in a helper function outside test_expect_success > (t7900's test_geometric_repack_needed) is converted manually, since > the parser only processes test bodies. > > Signed-off-by: Michael Montalbo <mmontalbo@gmail.com> > --- > t/.gitattributes | 2 + > t/Makefile | 32 +++- > t/lint-style.pl | 200 +++++++++++++++++++++ > t/lint-style/heredoc.expect | 3 + > t/lint-style/heredoc.test | 14 ++ > t/lint-style/test-grep-negation-fix.expect | 4 + > t/lint-style/test-grep-negation-fix.test | 4 + > t/lint-style/test-grep-negation.expect | 3 + > t/lint-style/test-grep-negation.test | 4 + > t/t0031-lockfile-pid.sh | 2 +- > t/t5300-pack-object.sh | 2 +- > t/t5319-multi-pack-index.sh | 2 +- > t/t7900-maintenance.sh | 2 +- > 13 files changed, 268 insertions(+), 6 deletions(-) > create mode 100755 t/lint-style.pl > create mode 100644 t/lint-style/heredoc.expect > create mode 100644 t/lint-style/heredoc.test > create mode 100644 t/lint-style/test-grep-negation-fix.expect > create mode 100644 t/lint-style/test-grep-negation-fix.test > create mode 100644 t/lint-style/test-grep-negation.expect > create mode 100644 t/lint-style/test-grep-negation.test > > diff --git a/t/.gitattributes b/t/.gitattributes > index 7664c6e027..aea6889d03 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 > +/lint-style/*.expect eol=lf -whitespace > +/lint-style/*.test eol=lf -whitespace > /t0110/url-* binary > /t3206/* eol=lf > /t3900/*.txt eol=lf > diff --git a/t/Makefile b/t/Makefile > index 25f923fed9..3a5fa4ce37 100644 > --- a/t/Makefile > +++ b/t/Makefile > @@ -46,6 +46,7 @@ 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))) > CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl > +LINT_STYLE_TESTS = $(sort $(wildcard lint-style/*.test)) > UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c) > UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES)) > UNIT_TEST_PROGRAMS += unit-tests/bin/unit-tests$(X) > @@ -139,7 +140,7 @@ check-meson: > test-lint: test-lint-duplicates test-lint-executable \ > test-lint-filenames > ifneq ($(PERL_PATH),) > -test-lint: test-lint-shell-syntax check-shell-parser > +test-lint: test-lint-shell-syntax test-lint-style check-lint-style check-shell-parser > else > GIT_TEST_CHAIN_LINT = 0 > endif > @@ -162,6 +163,32 @@ test-lint-shell-syntax: > > check-shell-parser: > @'$(PERL_PATH_SQ)' check-shell-parser.pl > + > +test-lint-style: > + @'$(PERL_PATH_SQ)' lint-style.pl $(T) $(THELPERS) $(TPERF) > + > +check-lint-style: > + @rc=0; for t in $(LINT_STYLE_TESTS); do \ > + base=$${t%.test}; \ > + case $$base in \ > + *-fix) \ > + cp "$$t" "$$t.tmp" && \ > + '$(PERL_PATH_SQ)' lint-style.pl --fix "$$t.tmp" >/dev/null 2>&1; \ > + fix_rc=$$?; \ > + if test $$fix_rc != 0; then \ > + echo "FAIL: $$t (--fix exit code $$fix_rc)"; rc=1; \ > + elif ! diff -u "$$base.expect" "$$t.tmp"; then \ > + echo "FAIL: $$t (--fix output)"; rc=1; \ > + fi; \ > + rm -f "$$t.tmp" ;; \ > + *) \ > + if ! '$(PERL_PATH_SQ)' lint-style.pl "$$t" 2>&1 | \ > + diff -u "$$base.expect" -; then \ > + echo "FAIL: $$t"; rc=1; \ > + fi ;; \ > + esac; \ > + done; test $$rc = 0 > + …I wonder if it would be easier to maintain this recipe as a separate shell script and have make give LINT_STYLE_TESTS and PERL_PATH (w/o SQ? idk) to the script. That's a lot of inline code otherwise! > 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) > @@ -188,7 +215,8 @@ perf: > > .PHONY: pre-clean $(T) aggregate-results clean valgrind perf \ > check-chainlint clean-chainlint test-chainlint \ > - check-shell-parser $(UNIT_TESTS) > + check-shell-parser \ > + check-lint-style test-lint-style $(UNIT_TESTS) > > .PHONY: libgit-sys-test libgit-rs-test > libgit-sys-test: > diff --git a/t/lint-style.pl b/t/lint-style.pl > new file mode 100755 > index 0000000000..9268577f9b > --- /dev/null > +++ b/t/lint-style.pl > @@ -0,0 +1,200 @@ > +#!/usr/bin/perl > + > +# Check test scripts for style violations that can be detected > +# mechanically, such as using bare 'grep' where test_grep should > +# be used. Use --fix to automatically apply suggested replacements. > +# > +# Detection uses parsed tokens from the shared shell parser for > +# correct handling of heredocs, $(...), pipes, and quoting. > +# Fixes modify the original file text to preserve formatting. > + > +use strict; > +use warnings; > +use File::Basename; > +# Force LF output so check-lint-style's diff against the > +# pre-committed .expect files works on Windows. > +binmode(STDOUT, ':unix'); > +binmode(STDERR, ':unix'); > + > +my $fix_mode = 0; > +if (@ARGV && $ARGV[0] eq '--fix') { > + $fix_mode = 1; > + shift @ARGV; > +} > + > +# Load the shared shell parser (Lexer, ShellParser, ScriptParser). > +my $_lib = dirname($0) . "/lib-shell-parser.pl"; > +$_lib = "./$_lib" unless $_lib =~ m{^/}; > +do $_lib or die "$0: failed to load $_lib: $@$!\n"; > + > +# LintParser is a subclass of ScriptParser which runs lint rules > +# on each test body. Per-file state (file name, raw lines, dirty > +# flag) is stored on the instance before calling parse(). > +# > +# Subroutines defined below (parse_commands, check_test_grep_negation, > +# etc.) are in package main and called with the main:: prefix. > +# File-scoped lexicals ($fix_mode, $has_fixable, etc.) are visible > +# across packages since 'package' does not introduce a new scope. > +package LintParser; > +our @ISA = ('ScriptParser'); > + > +package main; > + > +my $exit_code = 0; > +my $has_fixable = 0; > + > +sub err { > + my ($file, $lineno, $line, $msg, %opts) = @_; > + $line =~ s/^\s+//; > + $line =~ s/\s+$//; > + $line =~ s/\s+/ /g; > + my $prefix = ($fix_mode && $opts{fixable}) ? 'fixed' : 'error'; > + print "$file:$lineno: $prefix: $msg: $line\n"; > + $exit_code = 1 unless $fix_mode && $opts{fixable}; > +} > + > +# Report a lint violation found by a rule. In --fix mode, apply > +# the regex substitution on the raw line and report success. > +# Otherwise just report. Returns 1 if the line was modified. > +sub report_violation { > + my ($file, $cmd, $line_ref, $match, $fix, $from) = @_; > + my $lineno = $cmd->{lineno}; > + my $display = join(' ', @{$cmd->{tokens}}); > + $has_fixable++; # count for the "--fix" hint > + if ($fix_mode) { > + if ($$line_ref =~ s/$match/$fix/) { > + err $file, $lineno, $display, > + "replace '$from' with '$fix'", > + fixable => 1; > + return 1; > + } > + err $file, $lineno, $display, > + "replace '$from' with '$fix' (could not auto-fix)"; > + } else { > + err $file, $lineno, $display, > + "replace '$from' with '$fix'"; > + } > + return 0; > +} > + > +# Split a token stream into commands at &&, ||, ;;, and \n. > +sub parse_commands { > + my ($content) = @_; > + my $parser = ShellParser->new(\$content); > + my @all_tokens = $parser->parse(); > + > + my @commands; > + my @current; > + my $lineno = 1; > + > + for (my $ti = 0; $ti < @all_tokens; $ti++) { > + my $text = $all_tokens[$ti]->[0]; > + if ($text =~ /^(?:&&|\|\||;;|\n)$/) { > + if (@current) { > + push @commands, { > + tokens => [@current], > + lineno => $lineno, > + }; > + @current = (); > + } > + } else { > + $lineno = $all_tokens[$ti]->[3] > + if !@current && defined $all_tokens[$ti]->[3]; > + push @current, $text; > + } > + } > + if (@current) { > + push @commands, { > + tokens => [@current], > + lineno => $lineno, > + }; > + } > + return @commands; > +} > + > +# --- Rule: '! test_grep' should be 'test_grep !' --- > +# Shell-level negation suppresses test_grep's diagnostic output > +# on failure. Built-in negation preserves it. > +sub check_test_grep_negation { > + my ($cmd, $file, $line_ref) = @_; > + my @tokens = @{$cmd->{tokens}}; > + return unless @tokens >= 2 && $tokens[0] eq '!' && $tokens[1] eq 'test_grep'; > + > + return report_violation($file, $cmd, $line_ref, > + qr/!\s*test_grep/, 'test_grep !', '! test_grep'); > +} > + > +# Map parsed commands back to raw file lines for --fix. > +# Detection uses parsed tokens (correct handling of quoting, > +# heredocs, pipes) but fixes must modify the original text > +# to preserve formatting. > +package LintParser; > + > +sub check_test { > + # Called by ScriptParser::parse_cmd for each test_expect_success > + # or test_expect_failure block. > + my $self = shift @_; > + my $title = ScriptParser::unwrap(shift @_); > + > + # Two test body formats: > + # Quoted: test_expect_success 'title' '..body..' > + # Heredoc: test_expect_success 'title' - <<\EOF > + # ..body.. > + # EOF > + # For quoted, the body token is the quoted string. > + # For heredoc, the body token is '-' and the actual > + # code arrives as the next argument from the Lexer. > + my $body_token = shift @_; > + my $lineno_base = $body_token->[3] || 1; > + my $body = ScriptParser::unwrap($body_token); > + > + if ($body eq '-') { > + my $herebody = shift @_; > + if ($herebody) { > + $body = $herebody->{content}; > + $lineno_base = $herebody->{start_line} || 1; > + } > + } > + return unless $body; > + > + # Map each command back to its file line number. > + # $lineno_base is where the body starts in the file; > + # $cmd->{lineno} is relative to the body (starting at 1). > + my $raw_lines = $self->{raw_lines}; > + for my $cmd (main::parse_commands($body)) { > + my $ln = ($cmd->{lineno} || 0) + $lineno_base - 1; > + $cmd->{lineno} = $ln; > + next unless $ln >= 1 && $ln <= @$raw_lines; > + next if $raw_lines->[$ln - 1] =~ /#.*lint-ok/; > + > + if (main::check_test_grep_negation($cmd, $self->{file}, \$raw_lines->[$ln - 1])) { > + $self->{dirty} = 1; > + } > + } > +} > + > +package main; > + > +for my $file (@ARGV) { > + # :unix:crlf strips \r on Windows (same as chainlint.pl) > + open(my $fh, '<:unix:crlf', $file) or die "$0: $file: $!\n"; > + my @raw_lines = <$fh>; > + close $fh; > + > + my $parser = LintParser->new(\join('', @raw_lines)); > + $parser->{file} = $file; > + $parser->{raw_lines} = \@raw_lines; > + $parser->{dirty} = 0; > + $parser->parse(); > + > + if ($fix_mode && $parser->{dirty}) { > + open(my $out, '>', $file) or die "$0: $file: $!\n"; > + print $out @{$parser->{raw_lines}}; > + close $out; > + } > +} > + > +if ($has_fixable && !$fix_mode) { > + print "hint: run with --fix to apply the suggested replacements.\n"; > +} > +exit $exit_code; > diff --git a/t/lint-style/heredoc.expect b/t/lint-style/heredoc.expect > new file mode 100644 > index 0000000000..7ff6d4a52d > --- /dev/null > +++ b/t/lint-style/heredoc.expect > @@ -0,0 +1,3 @@ > +lint-style/heredoc.test:8: error: replace '! test_grep' with 'test_grep !': ! test_grep "after-heredoc-is-caught" actual > +lint-style/heredoc.test:13: error: replace '! test_grep' with 'test_grep !': ! test_grep "not-inside-sed-heredoc" actual > +hint: run with --fix to apply the suggested replacements. > diff --git a/t/lint-style/heredoc.test b/t/lint-style/heredoc.test > new file mode 100644 > index 0000000000..4c05831cfb > --- /dev/null > +++ b/t/lint-style/heredoc.test > @@ -0,0 +1,14 @@ > +test_expect_success 'greps inside heredocs are skipped' ' > + cat <<-EOF && > + grep "inside-strip-tabs" file > + EOF > + cat <<-\EOF && > + grep "inside-no-expand" file > + EOF > + ! test_grep "after-heredoc-is-caught" actual > +' > + > +test_expect_success 'sed with << does not start a heredoc' ' > + sed "s/<< foo/bar/" file && > + ! test_grep "not-inside-sed-heredoc" actual > +' > diff --git a/t/lint-style/test-grep-negation-fix.expect b/t/lint-style/test-grep-negation-fix.expect > new file mode 100644 > index 0000000000..28ecde1073 > --- /dev/null > +++ b/t/lint-style/test-grep-negation-fix.expect > @@ -0,0 +1,4 @@ > +test_expect_success 'negated test_grep' ' > + test_grep ! "pattern" actual && > + test_grep ! -i "insensitive" actual > +' > diff --git a/t/lint-style/test-grep-negation-fix.test b/t/lint-style/test-grep-negation-fix.test > new file mode 100644 > index 0000000000..571c150031 > --- /dev/null > +++ b/t/lint-style/test-grep-negation-fix.test > @@ -0,0 +1,4 @@ > +test_expect_success 'negated test_grep' ' > + ! test_grep "pattern" actual && > + ! test_grep -i "insensitive" actual > +' > diff --git a/t/lint-style/test-grep-negation.expect b/t/lint-style/test-grep-negation.expect > new file mode 100644 > index 0000000000..1fa9e124aa > --- /dev/null > +++ b/t/lint-style/test-grep-negation.expect > @@ -0,0 +1,3 @@ > +lint-style/test-grep-negation.test:2: error: replace '! test_grep' with 'test_grep !': ! test_grep "pattern" actual > +lint-style/test-grep-negation.test:3: error: replace '! test_grep' with 'test_grep !': ! test_grep -i "insensitive" actual > +hint: run with --fix to apply the suggested replacements. > diff --git a/t/lint-style/test-grep-negation.test b/t/lint-style/test-grep-negation.test > new file mode 100644 > index 0000000000..571c150031 > --- /dev/null > +++ b/t/lint-style/test-grep-negation.test > @@ -0,0 +1,4 @@ > +test_expect_success 'negated test_grep' ' > + ! test_grep "pattern" actual && > + ! test_grep -i "insensitive" actual > +' > diff --git a/t/t0031-lockfile-pid.sh b/t/t0031-lockfile-pid.sh > index 8ef87addf5..e9e2f04049 100755 > --- a/t/t0031-lockfile-pid.sh > +++ b/t/t0031-lockfile-pid.sh > @@ -29,7 +29,7 @@ test_expect_success 'PID info not shown by default' ' > test_must_fail git add . 2>err && > # Should not crash, just show normal error without PID > test_grep "Unable to create" err && > - ! test_grep "is held by process" err > + test_grep ! "is held by process" err > ) > ' > > diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh > index 73445782e7..3179b4963e 100755 > --- a/t/t5300-pack-object.sh > +++ b/t/t5300-pack-object.sh > @@ -720,7 +720,7 @@ test_expect_success '--name-hash-version=2 and --write-bitmap-index are incompat > > # --stdout option silently removes --write-bitmap-index > git pack-objects --stdout --all --name-hash-version=2 --write-bitmap-index >out 2>err && > - ! test_grep "currently, --write-bitmap-index requires --name-hash-version=1" err > + test_grep ! "currently, --write-bitmap-index requires --name-hash-version=1" err > ' > > test_expect_success '--path-walk pack everything' ' > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh > index fa0d4046f7..9154d9795f 100755 > --- a/t/t5319-multi-pack-index.sh > +++ b/t/t5319-multi-pack-index.sh > @@ -1175,7 +1175,7 @@ test_expect_success 'load reverse index when missing .idx, .pack' ' > > test_expect_success 'usage shown without sub-command' ' > test_expect_code 129 git multi-pack-index 2>err && > - ! test_grep "unrecognized subcommand" err > + test_grep ! "unrecognized subcommand" err > ' > > test_expect_success 'complains when run outside of a repository' ' > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index d7f82e1bec..9db4a76f67 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -664,7 +664,7 @@ test_geometric_repack_needed () { > true) > test_grep "\[\"git\",\"repack\"," trace2.txt;; > false) > - ! test_grep "\[\"git\",\"repack\"," trace2.txt;; > + test_grep ! "\[\"git\",\"repack\"," trace2.txt;; > *) > BUG "invalid parameter: $NEEDED";; > esac > -- > gitgitgadget > -- D. Ben Knoble

Michael Montalbo wrote on the Git mailing list (how to reply to this email):

On Thu, Jun 4, 2026 at 11:35 AM D. Ben Knoble <ben.knoble@gmail.com> wrote: > > Hi Michael, > > This sounds like a neat effort! > > One drive-by comment… > > On Thu, Jun 4, 2026 at 3:46 AM Michael Montalbo via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > From: Michael Montalbo <mmontalbo@gmail.com> > > > > Add a mechanical lint checker for test scripts, similar in spirit to > > check-non-portable-shell.pl but focused on test conventions rather > > than portability. > > > > The tool defines LintParser, a subclass of ScriptParser (from the > > shared lib-shell-parser.pl module). ScriptParser's > > parse_cmd() finds test_expect_success blocks and calls check_test() > > for each body; LintParser overrides check_test() to run lint rules > > on the parsed commands. A "# lint-ok" comment suppresses all > > checks for intentional style violations. > > > > The first rule detects '! test_grep' and replaces it with > > 'test_grep !'. Shell-level negation suppresses the diagnostic > > output that test_grep prints on failure; the built-in negation > > preserves it. > > > > Three violations inside test bodies are converted via --fix. One > > additional violation in a helper function outside test_expect_success > > (t7900's test_geometric_repack_needed) is converted manually, since > > the parser only processes test bodies. > > > > Signed-off-by: Michael Montalbo <mmontalbo@gmail.com> > > --- > > t/.gitattributes | 2 + > > t/Makefile | 32 +++- > > t/lint-style.pl | 200 +++++++++++++++++++++ > > t/lint-style/heredoc.expect | 3 + > > t/lint-style/heredoc.test | 14 ++ > > t/lint-style/test-grep-negation-fix.expect | 4 + > > t/lint-style/test-grep-negation-fix.test | 4 + > > t/lint-style/test-grep-negation.expect | 3 + > > t/lint-style/test-grep-negation.test | 4 + > > t/t0031-lockfile-pid.sh | 2 +- > > t/t5300-pack-object.sh | 2 +- > > t/t5319-multi-pack-index.sh | 2 +- > > t/t7900-maintenance.sh | 2 +- > > 13 files changed, 268 insertions(+), 6 deletions(-) > > create mode 100755 t/lint-style.pl > > create mode 100644 t/lint-style/heredoc.expect > > create mode 100644 t/lint-style/heredoc.test > > create mode 100644 t/lint-style/test-grep-negation-fix.expect > > create mode 100644 t/lint-style/test-grep-negation-fix.test > > create mode 100644 t/lint-style/test-grep-negation.expect > > create mode 100644 t/lint-style/test-grep-negation.test > > > > diff --git a/t/.gitattributes b/t/.gitattributes > > index 7664c6e027..aea6889d03 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 > > +/lint-style/*.expect eol=lf -whitespace > > +/lint-style/*.test eol=lf -whitespace > > /t0110/url-* binary > > /t3206/* eol=lf > > /t3900/*.txt eol=lf > > diff --git a/t/Makefile b/t/Makefile > > index 25f923fed9..3a5fa4ce37 100644 > > --- a/t/Makefile > > +++ b/t/Makefile > > @@ -46,6 +46,7 @@ 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))) > > CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl > > +LINT_STYLE_TESTS = $(sort $(wildcard lint-style/*.test)) > > UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c) > > UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES)) > > UNIT_TEST_PROGRAMS += unit-tests/bin/unit-tests$(X) > > @@ -139,7 +140,7 @@ check-meson: > > test-lint: test-lint-duplicates test-lint-executable \ > > test-lint-filenames > > ifneq ($(PERL_PATH),) > > -test-lint: test-lint-shell-syntax check-shell-parser > > +test-lint: test-lint-shell-syntax test-lint-style check-lint-style check-shell-parser > > else > > GIT_TEST_CHAIN_LINT = 0 > > endif > > @@ -162,6 +163,32 @@ test-lint-shell-syntax: > > > > check-shell-parser: > > @'$(PERL_PATH_SQ)' check-shell-parser.pl > > + > > +test-lint-style: > > + @'$(PERL_PATH_SQ)' lint-style.pl $(T) $(THELPERS) $(TPERF) > > + > > +check-lint-style: > > + @rc=0; for t in $(LINT_STYLE_TESTS); do \ > > + base=$${t%.test}; \ > > + case $$base in \ > > + *-fix) \ > > + cp "$$t" "$$t.tmp" && \ > > + '$(PERL_PATH_SQ)' lint-style.pl --fix "$$t.tmp" >/dev/null 2>&1; \ > > + fix_rc=$$?; \ > > + if test $$fix_rc != 0; then \ > > + echo "FAIL: $$t (--fix exit code $$fix_rc)"; rc=1; \ > > + elif ! diff -u "$$base.expect" "$$t.tmp"; then \ > > + echo "FAIL: $$t (--fix output)"; rc=1; \ > > + fi; \ > > + rm -f "$$t.tmp" ;; \ > > + *) \ > > + if ! '$(PERL_PATH_SQ)' lint-style.pl "$$t" 2>&1 | \ > > + diff -u "$$base.expect" -; then \ > > + echo "FAIL: $$t"; rc=1; \ > > + fi ;; \ > > + esac; \ > > + done; test $$rc = 0 > > + > > …I wonder if it would be easier to maintain this recipe as a separate > shell script and have make give LINT_STYLE_TESTS and PERL_PATH (w/o > SQ? idk) to the script. That's a lot of inline code otherwise! > Yeah that's a good call out, will fix in a follow-up. Thank you for taking a look! > > 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) > > @@ -188,7 +215,8 @@ perf: > > > > .PHONY: pre-clean $(T) aggregate-results clean valgrind perf \ > > check-chainlint clean-chainlint test-chainlint \ > > - check-shell-parser $(UNIT_TESTS) > > + check-shell-parser \ > > + check-lint-style test-lint-style $(UNIT_TESTS) > > > > .PHONY: libgit-sys-test libgit-rs-test > > libgit-sys-test: > > diff --git a/t/lint-style.pl b/t/lint-style.pl > > new file mode 100755 > > index 0000000000..9268577f9b > > --- /dev/null > > +++ b/t/lint-style.pl > > @@ -0,0 +1,200 @@ > > +#!/usr/bin/perl > > + > > +# Check test scripts for style violations that can be detected > > +# mechanically, such as using bare 'grep' where test_grep should > > +# be used. Use --fix to automatically apply suggested replacements. > > +# > > +# Detection uses parsed tokens from the shared shell parser for > > +# correct handling of heredocs, $(...), pipes, and quoting. > > +# Fixes modify the original file text to preserve formatting. > > + > > +use strict; > > +use warnings; > > +use File::Basename; > > +# Force LF output so check-lint-style's diff against the > > +# pre-committed .expect files works on Windows. > > +binmode(STDOUT, ':unix'); > > +binmode(STDERR, ':unix'); > > + > > +my $fix_mode = 0; > > +if (@ARGV && $ARGV[0] eq '--fix') { > > + $fix_mode = 1; > > + shift @ARGV; > > +} > > + > > +# Load the shared shell parser (Lexer, ShellParser, ScriptParser). > > +my $_lib = dirname($0) . "/lib-shell-parser.pl"; > > +$_lib = "./$_lib" unless $_lib =~ m{^/}; > > +do $_lib or die "$0: failed to load $_lib: $@$!\n"; > > + > > +# LintParser is a subclass of ScriptParser which runs lint rules > > +# on each test body. Per-file state (file name, raw lines, dirty > > +# flag) is stored on the instance before calling parse(). > > +# > > +# Subroutines defined below (parse_commands, check_test_grep_negation, > > +# etc.) are in package main and called with the main:: prefix. > > +# File-scoped lexicals ($fix_mode, $has_fixable, etc.) are visible > > +# across packages since 'package' does not introduce a new scope. > > +package LintParser; > > +our @ISA = ('ScriptParser'); > > + > > +package main; > > + > > +my $exit_code = 0; > > +my $has_fixable = 0; > > + > > +sub err { > > + my ($file, $lineno, $line, $msg, %opts) = @_; > > + $line =~ s/^\s+//; > > + $line =~ s/\s+$//; > > + $line =~ s/\s+/ /g; > > + my $prefix = ($fix_mode && $opts{fixable}) ? 'fixed' : 'error'; > > + print "$file:$lineno: $prefix: $msg: $line\n"; > > + $exit_code = 1 unless $fix_mode && $opts{fixable}; > > +} > > + > > +# Report a lint violation found by a rule. In --fix mode, apply > > +# the regex substitution on the raw line and report success. > > +# Otherwise just report. Returns 1 if the line was modified. > > +sub report_violation { > > + my ($file, $cmd, $line_ref, $match, $fix, $from) = @_; > > + my $lineno = $cmd->{lineno}; > > + my $display = join(' ', @{$cmd->{tokens}}); > > + $has_fixable++; # count for the "--fix" hint > > + if ($fix_mode) { > > + if ($$line_ref =~ s/$match/$fix/) { > > + err $file, $lineno, $display, > > + "replace '$from' with '$fix'", > > + fixable => 1; > > + return 1; > > + } > > + err $file, $lineno, $display, > > + "replace '$from' with '$fix' (could not auto-fix)"; > > + } else { > > + err $file, $lineno, $display, > > + "replace '$from' with '$fix'"; > > + } > > + return 0; > > +} > > + > > +# Split a token stream into commands at &&, ||, ;;, and \n. > > +sub parse_commands { > > + my ($content) = @_; > > + my $parser = ShellParser->new(\$content); > > + my @all_tokens = $parser->parse(); > > + > > + my @commands; > > + my @current; > > + my $lineno = 1; > > + > > + for (my $ti = 0; $ti < @all_tokens; $ti++) { > > + my $text = $all_tokens[$ti]->[0]; > > + if ($text =~ /^(?:&&|\|\||;;|\n)$/) { > > + if (@current) { > > + push @commands, { > > + tokens => [@current], > > + lineno => $lineno, > > + }; > > + @current = (); > > + } > > + } else { > > + $lineno = $all_tokens[$ti]->[3] > > + if !@current && defined $all_tokens[$ti]->[3]; > > + push @current, $text; > > + } > > + } > > + if (@current) { > > + push @commands, { > > + tokens => [@current], > > + lineno => $lineno, > > + }; > > + } > > + return @commands; > > +} > > + > > +# --- Rule: '! test_grep' should be 'test_grep !' --- > > +# Shell-level negation suppresses test_grep's diagnostic output > > +# on failure. Built-in negation preserves it. > > +sub check_test_grep_negation { > > + my ($cmd, $file, $line_ref) = @_; > > + my @tokens = @{$cmd->{tokens}}; > > + return unless @tokens >= 2 && $tokens[0] eq '!' && $tokens[1] eq 'test_grep'; > > + > > + return report_violation($file, $cmd, $line_ref, > > + qr/!\s*test_grep/, 'test_grep !', '! test_grep'); > > +} > > + > > +# Map parsed commands back to raw file lines for --fix. > > +# Detection uses parsed tokens (correct handling of quoting, > > +# heredocs, pipes) but fixes must modify the original text > > +# to preserve formatting. > > +package LintParser; > > + > > +sub check_test { > > + # Called by ScriptParser::parse_cmd for each test_expect_success > > + # or test_expect_failure block. > > + my $self = shift @_; > > + my $title = ScriptParser::unwrap(shift @_); > > + > > + # Two test body formats: > > + # Quoted: test_expect_success 'title' '..body..' > > + # Heredoc: test_expect_success 'title' - <<\EOF > > + # ..body.. > > + # EOF > > + # For quoted, the body token is the quoted string. > > + # For heredoc, the body token is '-' and the actual > > + # code arrives as the next argument from the Lexer. > > + my $body_token = shift @_; > > + my $lineno_base = $body_token->[3] || 1; > > + my $body = ScriptParser::unwrap($body_token); > > + > > + if ($body eq '-') { > > + my $herebody = shift @_; > > + if ($herebody) { > > + $body = $herebody->{content}; > > + $lineno_base = $herebody->{start_line} || 1; > > + } > > + } > > + return unless $body; > > + > > + # Map each command back to its file line number. > > + # $lineno_base is where the body starts in the file; > > + # $cmd->{lineno} is relative to the body (starting at 1). > > + my $raw_lines = $self->{raw_lines}; > > + for my $cmd (main::parse_commands($body)) { > > + my $ln = ($cmd->{lineno} || 0) + $lineno_base - 1; > > + $cmd->{lineno} = $ln; > > + next unless $ln >= 1 && $ln <= @$raw_lines; > > + next if $raw_lines->[$ln - 1] =~ /#.*lint-ok/; > > + > > + if (main::check_test_grep_negation($cmd, $self->{file}, \$raw_lines->[$ln - 1])) { > > + $self->{dirty} = 1; > > + } > > + } > > +} > > + > > +package main; > > + > > +for my $file (@ARGV) { > > + # :unix:crlf strips \r on Windows (same as chainlint.pl) > > + open(my $fh, '<:unix:crlf', $file) or die "$0: $file: $!\n"; > > + my @raw_lines = <$fh>; > > + close $fh; > > + > > + my $parser = LintParser->new(\join('', @raw_lines)); > > + $parser->{file} = $file; > > + $parser->{raw_lines} = \@raw_lines; > > + $parser->{dirty} = 0; > > + $parser->parse(); > > + > > + if ($fix_mode && $parser->{dirty}) { > > + open(my $out, '>', $file) or die "$0: $file: $!\n"; > > + print $out @{$parser->{raw_lines}}; > > + close $out; > > + } > > +} > > + > > +if ($has_fixable && !$fix_mode) { > > + print "hint: run with --fix to apply the suggested replacements.\n"; > > +} > > +exit $exit_code; > > diff --git a/t/lint-style/heredoc.expect b/t/lint-style/heredoc.expect > > new file mode 100644 > > index 0000000000..7ff6d4a52d > > --- /dev/null > > +++ b/t/lint-style/heredoc.expect > > @@ -0,0 +1,3 @@ > > +lint-style/heredoc.test:8: error: replace '! test_grep' with 'test_grep !': ! test_grep "after-heredoc-is-caught" actual > > +lint-style/heredoc.test:13: error: replace '! test_grep' with 'test_grep !': ! test_grep "not-inside-sed-heredoc" actual > > +hint: run with --fix to apply the suggested replacements. > > diff --git a/t/lint-style/heredoc.test b/t/lint-style/heredoc.test > > new file mode 100644 > > index 0000000000..4c05831cfb > > --- /dev/null > > +++ b/t/lint-style/heredoc.test > > @@ -0,0 +1,14 @@ > > +test_expect_success 'greps inside heredocs are skipped' ' > > + cat <<-EOF && > > + grep "inside-strip-tabs" file > > + EOF > > + cat <<-\EOF && > > + grep "inside-no-expand" file > > + EOF > > + ! test_grep "after-heredoc-is-caught" actual > > +' > > + > > +test_expect_success 'sed with << does not start a heredoc' ' > > + sed "s/<< foo/bar/" file && > > + ! test_grep "not-inside-sed-heredoc" actual > > +' > > diff --git a/t/lint-style/test-grep-negation-fix.expect b/t/lint-style/test-grep-negation-fix.expect > > new file mode 100644 > > index 0000000000..28ecde1073 > > --- /dev/null > > +++ b/t/lint-style/test-grep-negation-fix.expect > > @@ -0,0 +1,4 @@ > > +test_expect_success 'negated test_grep' ' > > + test_grep ! "pattern" actual && > > + test_grep ! -i "insensitive" actual > > +' > > diff --git a/t/lint-style/test-grep-negation-fix.test b/t/lint-style/test-grep-negation-fix.test > > new file mode 100644 > > index 0000000000..571c150031 > > --- /dev/null > > +++ b/t/lint-style/test-grep-negation-fix.test > > @@ -0,0 +1,4 @@ > > +test_expect_success 'negated test_grep' ' > > + ! test_grep "pattern" actual && > > + ! test_grep -i "insensitive" actual > > +' > > diff --git a/t/lint-style/test-grep-negation.expect b/t/lint-style/test-grep-negation.expect > > new file mode 100644 > > index 0000000000..1fa9e124aa > > --- /dev/null > > +++ b/t/lint-style/test-grep-negation.expect > > @@ -0,0 +1,3 @@ > > +lint-style/test-grep-negation.test:2: error: replace '! test_grep' with 'test_grep !': ! test_grep "pattern" actual > > +lint-style/test-grep-negation.test:3: error: replace '! test_grep' with 'test_grep !': ! test_grep -i "insensitive" actual > > +hint: run with --fix to apply the suggested replacements. > > diff --git a/t/lint-style/test-grep-negation.test b/t/lint-style/test-grep-negation.test > > new file mode 100644 > > index 0000000000..571c150031 > > --- /dev/null > > +++ b/t/lint-style/test-grep-negation.test > > @@ -0,0 +1,4 @@ > > +test_expect_success 'negated test_grep' ' > > + ! test_grep "pattern" actual && > > + ! test_grep -i "insensitive" actual > > +' > > diff --git a/t/t0031-lockfile-pid.sh b/t/t0031-lockfile-pid.sh > > index 8ef87addf5..e9e2f04049 100755 > > --- a/t/t0031-lockfile-pid.sh > > +++ b/t/t0031-lockfile-pid.sh > > @@ -29,7 +29,7 @@ test_expect_success 'PID info not shown by default' ' > > test_must_fail git add . 2>err && > > # Should not crash, just show normal error without PID > > test_grep "Unable to create" err && > > - ! test_grep "is held by process" err > > + test_grep ! "is held by process" err > > ) > > ' > > > > diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh > > index 73445782e7..3179b4963e 100755 > > --- a/t/t5300-pack-object.sh > > +++ b/t/t5300-pack-object.sh > > @@ -720,7 +720,7 @@ test_expect_success '--name-hash-version=2 and --write-bitmap-index are incompat > > > > # --stdout option silently removes --write-bitmap-index > > git pack-objects --stdout --all --name-hash-version=2 --write-bitmap-index >out 2>err && > > - ! test_grep "currently, --write-bitmap-index requires --name-hash-version=1" err > > + test_grep ! "currently, --write-bitmap-index requires --name-hash-version=1" err > > ' > > > > test_expect_success '--path-walk pack everything' ' > > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh > > index fa0d4046f7..9154d9795f 100755 > > --- a/t/t5319-multi-pack-index.sh > > +++ b/t/t5319-multi-pack-index.sh > > @@ -1175,7 +1175,7 @@ test_expect_success 'load reverse index when missing .idx, .pack' ' > > > > test_expect_success 'usage shown without sub-command' ' > > test_expect_code 129 git multi-pack-index 2>err && > > - ! test_grep "unrecognized subcommand" err > > + test_grep ! "unrecognized subcommand" err > > ' > > > > test_expect_success 'complains when run outside of a repository' ' > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > > index d7f82e1bec..9db4a76f67 100755 > > --- a/t/t7900-maintenance.sh > > +++ b/t/t7900-maintenance.sh > > @@ -664,7 +664,7 @@ test_geometric_repack_needed () { > > true) > > test_grep "\[\"git\",\"repack\"," trace2.txt;; > > false) > > - ! test_grep "\[\"git\",\"repack\"," trace2.txt;; > > + test_grep ! "\[\"git\",\"repack\"," trace2.txt;; > > *) > > BUG "invalid parameter: $NEEDED";; > > esac > > -- > > gitgitgadget > > > > > -- > D. Ben Knoble

Three grep assertions were missing their file arguments, causing them to read from empty stdin instead of the intended file: - t2402: '! grep ...' should read from 'out', matching the grep on the preceding line. - t7507: the closing quote is in the wrong place, making the entire 'diff --git actual' a single pattern with no file argument instead of pattern 'diff --git' and file 'actual'. - t7700: '! grep ...' should read from 'packlist', matching the redirect on the preceding line. Without file arguments these greps always succeed (empty stdin matches nothing), so the assertions were not actually checking anything. All three tests pass with the corrected file arguments, confirming the intended behavior is sound. Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>

gitgitgadget · 2026-06-08T22:46:35Z

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The test suite has a test_grep wrapper that prints file contents on
> assertion failure, making debugging easier. Many tests still use bare 'grep'
> for assertions, which silently swallows context on failure.
>
> This series adds a lint tool (lint-style.pl) to mechanically detect and
> convert these, then applies it across the test suite.

I do not think we want an automated tool that rewrites the source
files.  I was hoping that we would get a patch or two that _adds_ to
existing test-lint framework (i.e., 'test-grep' that 'test-lint'
target depends on in t/Makefile) that gives diagnosis in a similar
fashion as test-lint-shell-syntax and test-chainlint do.

Also some existing uses of "grep" are not end-user facing and should
not be rewritten to "test_grep".

Replace bare grep with test_grep in test assertions across the suite. test_grep prints the contents of the file being searched on failure, making debugging easier than a bare grep which fails silently. Only assertion-style greps are converted: grep used as a filter in pipelines, command substitutions, conditionals, or with redirected I/O is left as-is. Existing '! test_grep' calls are rewritten to 'test_grep !' so that the diagnostic output is preserved on failure. This patch was produced more or less with perl convert-grep.pl t/t[0-9]*.sh t/perf/*.sh where convert-grep.pl applies the same line-oriented heuristics as check-grep.pl (added in the next commit) to identify assertion greps and rewrite them. A good way to sanity check the result is to apply the next commit and run make -C t test-lint-grep which should report no violations. Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>

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. test_grep prints the file contents on failure, making debugging easier. Bare grep fails silently. This lint prevents new bare grep assertions from being introduced and catches the common mistake of writing '! test_grep' instead of 'test_grep !'. The checker uses line-oriented heuristics with stitching for backslash and pipe continuations, plus heredoc tracking. Greps in filter contexts (pipelines, command substitutions, conditionals, redirected I/O) are left alone. Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>

mmontalbo force-pushed the mm/test-grep-docs branch 2 times, most recently from 3258ee2 to cfa4313 Compare June 3, 2026 02:33

mmontalbo force-pushed the mm/test-grep-docs branch 27 times, most recently from 1f8f6e1 to 5d5366a Compare June 3, 2026 11:11

mmontalbo force-pushed the mm/test-grep-docs branch 15 times, most recently from e509df3 to 1527293 Compare June 4, 2026 04:24

gitgitgadget Bot reviewed Jun 4, 2026

View reviewed changes

mmontalbo added 2 commits June 8, 2026 16:00

mmontalbo force-pushed the mm/test-grep-docs branch from 1527293 to d56dc29 Compare June 8, 2026 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

t: add lint-style.pl and convert grep to test_grep#2135

t: add lint-style.pl and convert grep to test_grep#2135
mmontalbo wants to merge 4 commits into
gitgitgadget:masterfrom
mmontalbo:mm/test-grep-docs

mmontalbo commented Jun 2, 2026 •

edited

Loading

Uh oh!

mmontalbo commented Jun 4, 2026

Uh oh!

gitgitgadget Bot commented Jun 4, 2026

Uh oh!

mmontalbo commented Jun 4, 2026

Uh oh!

gitgitgadget Bot commented Jun 4, 2026

Uh oh!

gitgitgadget Bot Jun 4, 2026

Uh oh!

gitgitgadget Bot Jun 4, 2026

Uh oh!

gitgitgadget Bot commented Jun 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mmontalbo commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmontalbo commented Jun 4, 2026

Uh oh!

gitgitgadget Bot commented Jun 4, 2026

Uh oh!

mmontalbo commented Jun 4, 2026

Uh oh!

gitgitgadget Bot commented Jun 4, 2026

Uh oh!

gitgitgadget Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot commented Jun 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mmontalbo commented Jun 2, 2026 •

edited

Loading