From: Michael Montalbo <redacted>

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 <redacted>
---
 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
+

 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