Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
2 changes: 2 additions & 0 deletions t/.gitattributes
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
t[0-9][0-9][0-9][0-9]/* -whitespace
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/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
Expand Down
37 changes: 35 additions & 2 deletions t/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
test-lint: test-lint-shell-syntax test-lint-style check-lint-style check-shell-parser
else
GIT_TEST_CHAIN_LINT = 0
endif
Expand All @@ -160,6 +161,36 @@ test-lint-executable:
test-lint-shell-syntax:
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)

check-shell-parser:
@'$(PERL_PATH_SQ)' check-shell-parser.pl

TSOURCED = $(sort $(wildcard t[0-9]*/*.sh))

test-lint-style:
@'$(PERL_PATH_SQ)' lint-style.pl $(T) $(THELPERS) $(TPERF) $(TSOURCED)

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)
Expand All @@ -185,7 +216,9 @@ 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-shell-parser \
check-lint-style test-lint-style $(UNIT_TESTS)

.PHONY: libgit-sys-test libgit-rs-test
libgit-sys-test:
Expand Down
21 changes: 21 additions & 0 deletions t/README
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,27 @@ see test-lib-functions.sh for the full list and their options.

Check whether a file has the length it is expected to.

- test_grep [!] [<grep-options>] <pattern> <file>

Check whether <file> contains a line matching <pattern>, or
with '!' that no line matches. Use this instead of bare
'grep <pattern> <file>' in test assertions. On failure,
test_grep prints the contents of <file> for easier debugging,
whereas a bare 'grep' would fail silently.

For negation, pass '!' as the first argument:

test_grep ! "^diff --git" actual

Do not negate by writing '! test_grep', as that suppresses the
diagnostic output.

test_grep should only be used as a test assertion. When grep
is used as a data filter (e.g. 'grep -v "^index" actual >filtered')
or inside a command substitution (e.g. '$(grep -c ...)'), plain
'grep' is the right choice because the exit code is not the
assertion itself.

- test_path_is_file <path>
test_path_is_dir <path>
test_path_is_missing <path>
Expand Down
Loading
Loading