[PATCH 4/6] t: add lint-style.pl with test_grep negation rule

["Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com>]

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