{"thread":{"id":"58385","subject":"[PATCH 00/18] make test \"linting\" more comprehensive","startedAt":"2022-09-01T00:30:05Z","lastAt":"2022-11-22T00:51:40Z","messageCount":51,"participants":["Eric Sunshine via GitGitGadget","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Eric Sunshine","Jeff King","Junio C Hamano","Elijah Newren","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":18},"messages":[{"id":"462388","messageId":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":null,"subject":"[PATCH 00/18] make test \"linting\" more comprehensive","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:38Z","receivedAt":"2022-09-01T00:30:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"A while back, Peff successfully nerd-sniped[1] me into tackling a\nlong-brewing idea I had about (possibly) improving \"chainlint\" performance\nby linting all tests in all scripts with a single command invocation instead\nof running \"sed\" 26800+ times (once for each test). The new linter\nintroduced by this series can check all test definitions in the entire\nproject in a single invocation, and each test definition is checked only\nonce no matter how many times the test is actually run (unlike chainlint.sed\nwhich will check a test repeatedly if, for instance, the test is run in a\nloop). Moreover, all test definitions in the project are \"linted\" even if\nsome of those tests would not run on a particular platform or under a\ncertain configuration (unlike chainlint.sed which only lints tests which\nactually run).\n\nThe new linter is a good deal smarter than chainlint.sed and understands not\njust shell syntax but also some semantics of test construction, unlike\nchainlint.sed which is merely heuristics-based. For instance, the new linter\nrecognizes cases when a broken &&-chain is legitimate, such as when \"$?\" is\nhandled explicitly or when a failure is signaled directly with \"false\", in\nwhich case the &&-chain leading up to the \"false\" is immaterial, as well as\nother cases. Unlike chainlint.sed, it recognizes that a semicolon after the\nlast command in a compound statement is harmless, thus won't interpret the\nsemicolon as breaking the &&-chain.\n\nThe new linter also provides considerably better coverage for broken\n&&-chains. The \"magic exit code 117\" &&-chain checker built into test-lib.sh\nonly works for top-level command invocations; it doesn't work within \"{...}\"\ngroups, \"(...)\" subshells, \"$(...)\" substitutions, or within bodies of\ncompound statements, such as \"if\", \"for\", \"while\", \"case\", etc.\nchainlint.sed partly fills the gap by catching broken &&-chains in \"(...)\"\nsubshells one level deep, but bugs can still lurk behind broken &&-chains in\nthe other cases. The new linter catches broken &&-chains within all those\nconstructs to any depth.\n\nAnother important improvement is that the new linter understands that shell\nloops do not terminate automatically when a command in the loop body fails,\nand that the condition needs to be handled explicitly by the test author by\nusing \"|| return 1\" (or \"|| exit 1\" in a subshell) to signal failure.\nConsequently, the new linter will complain when a loop is lacking \"|| return\n1\" (or \"|| exit 1\").\n\nFinally, unlike chainlint.sed which (not surprisingly) is implemented in\n\"sed\", the new linter is written in Perl, thus should be more accessible to\na wider audience, and is structured as a traditional top-down parser which\nmakes it much easier to reason about.\n\nThe new linter could eventually subsume other linting tasks such as\ncheck-nonportable-shell.pl (which itself takes a couple seconds to run on my\nmachine), though it probably should be renamed to something other than\n\"chainlint\" since it is no longer targeted only at spotting &&-chain breaks,\nbut that can wait for another day.\n\nÆvar offered some sensible comments[2,3] about optimizing the Makefile rules\nrelated to chainlint, but those optimizations are not tackled here for a few\nreasons: (1) this series is already quite long, (2) I'd like to keep the\nseries focused on its primary goal of installing a new and improved linter,\n(3) these patches do not make the Makefile situation any worse[4], and (4)\nthose optimizations can easily be done atop this series[5].\n\nJunio: This series is nominally atop es/t4301-sed-portability-fix which is\nin \"next\", and es/fix-chained-tests, es/test-chain-lint, and es/chainlint,\nall of which are already in \"master\".\n\nDscho: This series conflicts with some patches carried only by the Git for\nWindows project; the resolutions are obvious and simple. The new linter also\nidentifies some problems in tests carried only by the Git for Windows\nproject.\n\n[1] https://lore.kernel.org/git/YJzGcZpZ+E9R0gYd@coredump.intra.peff.net/\n[2]\nhttps://lore.kernel.org/git/RFC-patch-1.1-bb3f1577829-20211213T095456Z-avarab@gmail.com/\n[3] https://lore.kernel.org/git/211213.86tufc8oop.gmgdl@evledraar.gmail.com/\n[4]\nhttps://lore.kernel.org/git/CAPig+cSFtpt6ExbVDbcx3tZodrKFuM-r2GMW4TQ2tJmLvHBFtQ@mail.gmail.com/\n[5] https://lore.kernel.org/git/211214.86tufbbbu3.gmgdl@evledraar.gmail.com/\n\nEric Sunshine (18):\n t: add skeleton chainlint.pl\n chainlint.pl: add POSIX shell lexical analyzer\n chainlint.pl: add POSIX shell parser\n chainlint.pl: add parser to validate tests\n chainlint.pl: add parser to identify test definitions\n chainlint.pl: validate test scripts in parallel\n chainlint.pl: don't require `return|exit|continue` to end with `&&`\n t/Makefile: apply chainlint.pl to existing self-tests\n chainlint.pl: don't require `&` background command to end with `&&`\n chainlint.pl: don't flag broken &&-chain if `$?` handled explicitly\n chainlint.pl: don't flag broken &&-chain if failure indicated\n explicitly\n chainlint.pl: complain about loops lacking explicit failure handling\n chainlint.pl: allow `|| echo` to signal failure upstream of a pipe\n t/chainlint: add more chainlint.pl self-tests\n test-lib: retire \"lint harder\" optimization hack\n test-lib: replace chainlint.sed with chainlint.pl\n t/Makefile: teach `make test` and `make prove` to run chainlint.pl\n t: retire unused chainlint.sed\n\n contrib/buildsystems/CMakeLists.txt | 2 +-\n t/Makefile | 49 +-\n t/README | 5 -\n t/chainlint.pl | 730 ++++++++++++++++++\n t/chainlint.sed | 399 ----------\n t/chainlint/blank-line-before-esac.expect | 18 +\n t/chainlint/blank-line-before-esac.test | 19 +\n t/chainlint/block.expect | 15 +-\n t/chainlint/block.test | 15 +-\n t/chainlint/chain-break-background.expect | 9 +\n t/chainlint/chain-break-background.test | 10 +\n t/chainlint/chain-break-continue.expect | 12 +\n t/chainlint/chain-break-continue.test | 13 +\n t/chainlint/chain-break-false.expect | 9 +\n t/chainlint/chain-break-false.test | 10 +\n t/chainlint/chain-break-return-exit.expect | 19 +\n t/chainlint/chain-break-return-exit.test | 23 +\n t/chainlint/chain-break-status.expect | 9 +\n t/chainlint/chain-break-status.test | 11 +\n t/chainlint/chained-block.expect | 9 +\n t/chainlint/chained-block.test | 11 +\n t/chainlint/chained-subshell.expect | 10 +\n t/chainlint/chained-subshell.test | 13 +\n .../command-substitution-subsubshell.expect | 2 +\n .../command-substitution-subsubshell.test | 3 +\n t/chainlint/complex-if-in-cuddled-loop.expect | 2 +-\n t/chainlint/double-here-doc.expect | 2 +\n t/chainlint/double-here-doc.test | 12 +\n t/chainlint/dqstring-line-splice.expect | 3 +\n t/chainlint/dqstring-line-splice.test | 7 +\n t/chainlint/dqstring-no-interpolate.expect | 11 +\n t/chainlint/dqstring-no-interpolate.test | 15 +\n t/chainlint/empty-here-doc.expect | 3 +\n t/chainlint/empty-here-doc.test | 5 +\n t/chainlint/exclamation.expect | 4 +\n t/chainlint/exclamation.test | 8 +\n t/chainlint/for-loop-abbreviated.expect | 5 +\n t/chainlint/for-loop-abbreviated.test | 6 +\n t/chainlint/for-loop.expect | 4 +-\n t/chainlint/function.expect | 11 +\n t/chainlint/function.test | 13 +\n t/chainlint/here-doc-indent-operator.expect | 5 +\n t/chainlint/here-doc-indent-operator.test | 13 +\n t/chainlint/here-doc-multi-line-string.expect | 3 +-\n t/chainlint/if-condition-split.expect | 7 +\n t/chainlint/if-condition-split.test | 8 +\n t/chainlint/if-in-loop.expect | 2 +-\n t/chainlint/if-in-loop.test | 2 +-\n t/chainlint/loop-detect-failure.expect | 15 +\n t/chainlint/loop-detect-failure.test | 17 +\n t/chainlint/loop-detect-status.expect | 18 +\n t/chainlint/loop-detect-status.test | 19 +\n t/chainlint/loop-in-if.expect | 2 +-\n t/chainlint/loop-upstream-pipe.expect | 10 +\n t/chainlint/loop-upstream-pipe.test | 11 +\n t/chainlint/multi-line-string.expect | 11 +-\n t/chainlint/nested-loop-detect-failure.expect | 31 +\n t/chainlint/nested-loop-detect-failure.test | 35 +\n t/chainlint/nested-subshell.expect | 2 +-\n t/chainlint/one-liner-for-loop.expect | 9 +\n t/chainlint/one-liner-for-loop.test | 10 +\n t/chainlint/return-loop.expect | 5 +\n t/chainlint/return-loop.test | 6 +\n t/chainlint/semicolon.expect | 2 +-\n t/chainlint/sqstring-in-sqstring.expect | 4 +\n t/chainlint/sqstring-in-sqstring.test | 5 +\n t/chainlint/t7900-subtree.expect | 13 +-\n t/chainlint/token-pasting.expect | 27 +\n t/chainlint/token-pasting.test | 32 +\n t/chainlint/while-loop.expect | 4 +-\n t/t0027-auto-crlf.sh | 7 +-\n t/t3070-wildmatch.sh | 5 -\n t/test-lib.sh | 12 +-\n 73 files changed, 1439 insertions(+), 449 deletions(-)\n create mode 100755 t/chainlint.pl\n delete mode 100644 t/chainlint.sed\n create mode 100644 t/chainlint/blank-line-before-esac.expect\n create mode 100644 t/chainlint/blank-line-before-esac.test\n create mode 100644 t/chainlint/chain-break-background.expect\n create mode 100644 t/chainlint/chain-break-background.test\n create mode 100644 t/chainlint/chain-break-continue.expect\n create mode 100644 t/chainlint/chain-break-continue.test\n create mode 100644 t/chainlint/chain-break-false.expect\n create mode 100644 t/chainlint/chain-break-false.test\n create mode 100644 t/chainlint/chain-break-return-exit.expect\n create mode 100644 t/chainlint/chain-break-return-exit.test\n create mode 100644 t/chainlint/chain-break-status.expect\n create mode 100644 t/chainlint/chain-break-status.test\n create mode 100644 t/chainlint/chained-block.expect\n create mode 100644 t/chainlint/chained-block.test\n create mode 100644 t/chainlint/chained-subshell.expect\n create mode 100644 t/chainlint/chained-subshell.test\n create mode 100644 t/chainlint/command-substitution-subsubshell.expect\n create mode 100644 t/chainlint/command-substitution-subsubshell.test\n create mode 100644 t/chainlint/double-here-doc.expect\n create mode 100644 t/chainlint/double-here-doc.test\n create mode 100644 t/chainlint/dqstring-line-splice.expect\n create mode 100644 t/chainlint/dqstring-line-splice.test\n create mode 100644 t/chainlint/dqstring-no-interpolate.expect\n create mode 100644 t/chainlint/dqstring-no-interpolate.test\n create mode 100644 t/chainlint/empty-here-doc.expect\n create mode 100644 t/chainlint/empty-here-doc.test\n create mode 100644 t/chainlint/exclamation.expect\n create mode 100644 t/chainlint/exclamation.test\n create mode 100644 t/chainlint/for-loop-abbreviated.expect\n create mode 100644 t/chainlint/for-loop-abbreviated.test\n create mode 100644 t/chainlint/function.expect\n create mode 100644 t/chainlint/function.test\n create mode 100644 t/chainlint/here-doc-indent-operator.expect\n create mode 100644 t/chainlint/here-doc-indent-operator.test\n create mode 100644 t/chainlint/if-condition-split.expect\n create mode 100644 t/chainlint/if-condition-split.test\n create mode 100644 t/chainlint/loop-detect-failure.expect\n create mode 100644 t/chainlint/loop-detect-failure.test\n create mode 100644 t/chainlint/loop-detect-status.expect\n create mode 100644 t/chainlint/loop-detect-status.test\n create mode 100644 t/chainlint/loop-upstream-pipe.expect\n create mode 100644 t/chainlint/loop-upstream-pipe.test\n create mode 100644 t/chainlint/nested-loop-detect-failure.expect\n create mode 100644 t/chainlint/nested-loop-detect-failure.test\n create mode 100644 t/chainlint/one-liner-for-loop.expect\n create mode 100644 t/chainlint/one-liner-for-loop.test\n create mode 100644 t/chainlint/return-loop.expect\n create mode 100644 t/chainlint/return-loop.test\n create mode 100644 t/chainlint/sqstring-in-sqstring.expect\n create mode 100644 t/chainlint/sqstring-in-sqstring.test\n create mode 100644 t/chainlint/token-pasting.expect\n create mode 100644 t/chainlint/token-pasting.test\n\n\nbase-commit: d42b38dfb5edf1a7fddd9542d722f91038407819\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1322%2Fsunshineco%2Fchainlintperl-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1322/sunshineco/chainlintperl-v1\nPull-Request: https://github.com/git/git/pull/1322\n-- \ngitgitgadget\n"},{"id":"462389","messageId":"3423df94bd6035640828a2508968cf8e1f5b4dda.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 01/18] t: add skeleton chainlint.pl","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:39Z","receivedAt":"2022-09-01T00:30:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nAlthough chainlint.sed usefully identifies broken &&-chains in tests, it\nhas several shortcomings which include:\n\n * only detects &&-chain breakage in subshells (one-level deep)\n\n * does not check for broken top-level &&-chains; that task is left to\n the \"magic exit code 117\" checker built into test-lib.sh, however,\n that detection does not extend to `{...}` blocks, `$(...)`\n expressions, or compound statements such as `if...fi`,\n `while...done`, `case...esac`\n\n * uses heuristics, which makes it (potentially) fallible and difficult\n to tweak to handle additional real-world cases\n\n * written in `sed` and employs advanced `sed` operators which are\n probably not well-known to many programmers, thus the pool of people\n who can maintain it is likely small\n\n * manually simulates recursion into subshells which makes it much more\n difficult to reason about than, say, a traditional top-down parser\n\n * checks each test as the test is run, which can get expensive for\n tests which are run repeatedly by functions or loops since their\n bodies will be checked over and over (tens or hundreds of times)\n unnecessarily\n\nTo address these shortcomings, begin implementing a more functional and\nprecise test linter which understands shell syntax and semantics rather\nthan employing heuristics, thus is able to recognize structural problems\nwith tests beyond broken &&-chains.\n\nThe new linter is written in Perl, thus should be more accessible to a\nwider audience, and is structured as a traditional top-down parser which\nmakes it much easier to reason about, and allows it to inspect compound\nstatements within test bodies to any depth.\n\nFurthermore, it can check all test definitions in the entire project in\na single invocation rather than having to be invoked once per test, and\neach test definition is checked only once no matter how many times the\ntest is actually run.\n\nAt this stage, the new linter is just a skeleton containing boilerplate\nwhich handles command-line options, collects and reports statistics, and\nfeeds its arguments -- paths of test scripts -- to a (presently)\ndo-nothing script parser for validation. Subsequent changes will flesh\nout the functionality.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 115 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 115 insertions(+)\n create mode 100755 t/chainlint.pl\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nnew file mode 100755\nindex 00000000000..e8ab95c7858\n--- /dev/null\n+++ b/t/chainlint.pl\n@@ -0,0 +1,115 @@\n+#!/usr/bin/env perl\n+#\n+# Copyright (c) 2021-2022 Eric Sunshine \n+#\n+# This tool scans shell scripts for test definitions and checks those tests for\n+# problems, such as broken &&-chains, which might hide bugs in the tests\n+# themselves or in behaviors being exercised by the tests.\n+#\n+# Input arguments are pathnames of shell scripts containing test definitions,\n+# or globs referencing a collection of scripts. For each problem discovered,\n+# the pathname of the script containing the test is printed along with the test\n+# name and the test body with a `?!FOO?!` annotation at the location of each\n+# detected problem, where \"FOO\" is a tag such as \"AMP\" which indicates a broken\n+# &&-chain. Returns zero if no problems are discovered, otherwise non-zero.\n+\n+use warnings;\n+use strict;\n+use File::Glob;\n+use Getopt::Long;\n+\n+my $show_stats;\n+my $emit_all;\n+\n+package ScriptParser;\n+\n+sub new {\n+\tmy $class = shift @_;\n+\tmy $self = bless {} => $class;\n+\t$self->{output} = [];\n+\t$self->{ntests} = 0;\n+\treturn $self;\n+}\n+\n+sub parse_cmd {\n+\treturn undef;\n+}\n+\n+# main contains high-level functionality for processing command-line switches,\n+# feeding input test scripts to ScriptParser, and reporting results.\n+package main;\n+\n+my $getnow = sub { return time(); };\n+my $interval = sub { return time() - shift; };\n+if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {\n+\t$getnow = sub { return [Time::HiRes::gettimeofday()]; };\n+\t$interval = sub { return Time::HiRes::tv_interval(shift); };\n+}\n+\n+sub show_stats {\n+\tmy ($start_time, $stats) = @_;\n+\tmy $walltime = $interval->($start_time);\n+\tmy ($usertime) = times();\n+\tmy ($total_workers, $total_scripts, $total_tests, $total_errs) = (0, 0, 0, 0);\n+\tfor (@$stats) {\n+\t\tmy ($worker, $nscripts, $ntests, $nerrs) = @$_;\n+\t\tprint(STDERR \"worker $worker: $nscripts scripts, $ntests tests, $nerrs errors\\n\");\n+\t\t$total_workers++;\n+\t\t$total_scripts += $nscripts;\n+\t\t$total_tests += $ntests;\n+\t\t$total_errs += $nerrs;\n+\t}\n+\tprintf(STDERR \"total: %d workers, %d scripts, %d tests, %d errors, %.2fs/%.2fs (wall/user)\\n\", $total_workers, $total_scripts, $total_tests, $total_errs, $walltime, $usertime);\n+}\n+\n+sub check_script {\n+\tmy ($id, $next_script, $emit) = @_;\n+\tmy ($nscripts, $ntests, $nerrs) = (0, 0, 0);\n+\twhile (my $path = $next_script->()) {\n+\t\t$nscripts++;\n+\t\tmy $fh;\n+\t\tunless (open($fh, \"<\", $path)) {\n+\t\t\t$emit->(\"?!ERR?! $path: $!\\n\");\n+\t\t\tnext;\n+\t\t}\n+\t\tmy $s = do { local $/; <$fh> };\n+\t\tclose($fh);\n+\t\tmy $parser = ScriptParser->new(\\$s);\n+\t\t1 while $parser->parse_cmd();\n+\t\tif (@{$parser->{output}}) {\n+\t\t\tmy $s = join('', @{$parser->{output}});\n+\t\t\t$emit->(\"# chainlint: $path\\n\" . $s);\n+\t\t\t$nerrs += () = $s =~ /\\?![^?]+\\?!/g;\n+\t\t}\n+\t\t$ntests += $parser->{ntests};\n+\t}\n+\treturn [$id, $nscripts, $ntests, $nerrs];\n+}\n+\n+sub exit_code {\n+\tmy $stats = shift @_;\n+\tfor (@$stats) {\n+\t\tmy ($worker, $nscripts, $ntests, $nerrs) = @$_;\n+\t\treturn 1 if $nerrs;\n+\t}\n+\treturn 0;\n+}\n+\n+Getopt::Long::Configure(qw{bundling});\n+GetOptions(\n+\t\"emit-all!\" => \\$emit_all,\n+\t\"stats|show-stats!\" => \\$show_stats) or die(\"option error\\n\");\n+\n+my $start_time = $getnow->();\n+my @stats;\n+\n+my @scripts;\n+push(@scripts, File::Glob::bsd_glob($_)) for (@ARGV);\n+unless (@scripts) {\n+\tshow_stats($start_time, \\@stats) if $show_stats;\n+\texit;\n+}\n+\n+push(@stats, check_script(1, sub { shift(@scripts); }, sub { print(@_); }));\n+show_stats($start_time, \\@stats) if $show_stats;\n+exit(exit_code(\\@stats));\n-- \ngitgitgadget\n\n"},{"id":"462390","messageId":"c1042b9bcd94b9ecb0bf73dfbd4334b9f30ba99a.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 02/18] chainlint.pl: add POSIX shell lexical analyzer","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:40Z","receivedAt":"2022-09-01T00:30:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nBegin fleshing out chainlint.pl by adding a lexical analyzer for the\nPOSIX shell command language. The sole entry point Lexer::scan_token()\nreturns the next token from the input. It will be called by the upcoming\nshell language parser.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 177 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 177 insertions(+)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex e8ab95c7858..81ffbf28bf3 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -21,6 +21,183 @@ use Getopt::Long;\n my $show_stats;\n my $emit_all;\n \n+# Lexer tokenizes POSIX shell scripts. It is roughly modeled after section 2.3\n+# \"Token Recognition\" of POSIX chapter 2 \"Shell Command Language\". Although\n+# similar to lexical analyzers for other languages, this one differs in a few\n+# substantial ways due to quirks of the shell command language.\n+#\n+# For instance, in many languages, newline is just whitespace like space or\n+# TAB, but in shell a newline is a command separator, thus a distinct lexical\n+# token. A newline is significant and returned as a distinct token even at the\n+# end of a shell comment.\n+#\n+# In other languages, `1+2` would typically be scanned as three tokens\n+# (`1`, `+`, and `2`), but in shell it is a single token. However, the similar\n+# `1 + 2`, which embeds whitepace, is scanned as three token in shell, as well.\n+# In shell, several characters with special meaning lose that meaning when not\n+# surrounded by whitespace. For instance, the negation operator `!` is special\n+# when standing alone surrounded by whitespace; whereas in `foo!uucp` it is\n+# just a plain character in the longer token \"foo!uucp\". In many other\n+# languages, `\"string\"/foo:'string'` might be scanned as five tokens (\"string\",\n+# `/`, `foo`, `:`, and 'string'), but in shell, it is just a single token.\n+#\n+# The lexical analyzer for the shell command language is also somewhat unusual\n+# in that it recursively invokes the parser to handle the body of `$(...)`\n+# expressions which can contain arbitrary shell code. Such expressions may be\n+# encountered both inside and outside of double-quoted strings.\n+#\n+# The lexical analyzer is responsible for consuming shell here-doc bodies which\n+# extend from the line following a `< $parser,\n+\t\tbuff => $s,\n+\t\theretags => []\n+\t} => $class;\n+}\n+\n+sub scan_heredoc_tag {\n+\tmy $self = shift @_;\n+\t${$self->{buff}} =~ /\\G(-?)/gc;\n+\tmy $indented = $1;\n+\tmy $tag = $self->scan_token();\n+\t$tag =~ s/['\"\\\\]//g;\n+\tpush(@{$self->{heretags}}, $indented ? \"\\t$tag\" : \"$tag\");\n+\treturn \"<<$indented$tag\";\n+}\n+\n+sub scan_op {\n+\tmy ($self, $c) = @_;\n+\tmy $b = $self->{buff};\n+\treturn $c unless $$b =~ /\\G(.)/sgc;\n+\tmy $cc = $c . $1;\n+\treturn scan_heredoc_tag($self) if $cc eq '<<';\n+\treturn $cc if $cc =~ /^(?:&&|\\|\\||>>|;;|<&|>&|<>|>\\|)$/;\n+\tpos($$b)--;\n+\treturn $c;\n+}\n+\n+sub scan_sqstring {\n+\tmy $self = shift @_;\n+\t${$self->{buff}} =~ /\\G([^']*'|.*\\z)/sgc;\n+\treturn \"'\" . $1;\n+}\n+\n+sub scan_dqstring {\n+\tmy $self = shift @_;\n+\tmy $b = $self->{buff};\n+\tmy $s = '\"';\n+\twhile (1) {\n+\t\t# slurp up non-special characters\n+\t\t$s .= $1 if $$b =~ /\\G([^\"\\$\\\\]+)/gc;\n+\t\t# handle special characters\n+\t\tlast unless $$b =~ /\\G(.)/sgc;\n+\t\tmy $c = $1;\n+\t\t$s .= '\"', last if $c eq '\"';\n+\t\t$s .= '$' . $self->scan_dollar(), next if $c eq '$';\n+\t\tif ($c eq '\\\\') {\n+\t\t\t$s .= '\\\\', last unless $$b =~ /\\G(.)/sgc;\n+\t\t\t$c = $1;\n+\t\t\tnext if $c eq \"\\n\"; # line splice\n+\t\t\t# backslash escapes only $, `, \", \\ in dq-string\n+\t\t\t$s .= '\\\\' unless $c =~ /^[\\$`\"\\\\]$/;\n+\t\t\t$s .= $c;\n+\t\t\tnext;\n+\t\t}\n+\t\tdie(\"internal error scanning dq-string '$c'\\n\");\n+\t}\n+\treturn $s;\n+}\n+\n+sub scan_balanced {\n+\tmy ($self, $c1, $c2) = @_;\n+\tmy $b = $self->{buff};\n+\tmy $depth = 1;\n+\tmy $s = $c1;\n+\twhile ($$b =~ /\\G([^\\Q$c1$c2\\E]*(?:[\\Q$c1$c2\\E]|\\z))/gc) {\n+\t\t$s .= $1;\n+\t\t$depth++, next if $s =~ /\\Q$c1\\E$/;\n+\t\t$depth--;\n+\t\tlast if $depth == 0;\n+\t}\n+\treturn $s;\n+}\n+\n+sub scan_subst {\n+\tmy $self = shift @_;\n+\tmy @tokens = $self->{parser}->parse(qr/^\\)$/);\n+\t$self->{parser}->next_token(); # closing \")\"\n+\treturn @tokens;\n+}\n+\n+sub scan_dollar {\n+\tmy $self = shift @_;\n+\tmy $b = $self->{buff};\n+\treturn $self->scan_balanced('(', ')') if $$b =~ /\\G\\((?=\\()/gc; # $((...))\n+\treturn '(' . join(' ', $self->scan_subst()) . ')' if $$b =~ /\\G\\(/gc; # $(...)\n+\treturn $self->scan_balanced('{', '}') if $$b =~ /\\G\\{/gc; # ${...}\n+\treturn $1 if $$b =~ /\\G(\\w+)/gc; # $var\n+\treturn $1 if $$b =~ /\\G([@*#?$!0-9-])/gc; # $*, $1, $$, etc.\n+\treturn '';\n+}\n+\n+sub swallow_heredocs {\n+\tmy $self = shift @_;\n+\tmy $b = $self->{buff};\n+\tmy $tags = $self->{heretags};\n+\twhile (my $tag = shift @$tags) {\n+\t\tmy $indent = $tag =~ s/^\\t// ? '\\\\s*' : '';\n+\t\t$$b =~ /(?:\\G|\\n)$indent\\Q$tag\\E(?:\\n|\\z)/gc;\n+\t}\n+}\n+\n+sub scan_token {\n+\tmy $self = shift @_;\n+\tmy $b = $self->{buff};\n+\tmy $token = '';\n+RESTART:\n+\t$$b =~ /\\G[ \\t]+/gc; # skip whitespace (but not newline)\n+\treturn \"\\n\" if $$b =~ /\\G#[^\\n]*(?:\\n|\\z)/gc; # comment\n+\twhile (1) {\n+\t\t# slurp up non-special characters\n+\t\t$token .= $1 if $$b =~ /\\G([^\\\\;&|<>(){}'\"\\$\\s]+)/gc;\n+\t\t# handle special characters\n+\t\tlast unless $$b =~ /\\G(.)/sgc;\n+\t\tmy $c = $1;\n+\t\tlast if $c =~ /^[ \\t]$/; # whitespace ends token\n+\t\tpos($$b)--, last if length($token) && $c =~ /^[;&|<>(){}\\n]$/;\n+\t\t$token .= $self->scan_sqstring(), next if $c eq \"'\";\n+\t\t$token .= $self->scan_dqstring(), next if $c eq '\"';\n+\t\t$token .= $c . $self->scan_dollar(), next if $c eq '$';\n+\t\t$self->swallow_heredocs(), $token = $c, last if $c eq \"\\n\";\n+\t\t$token = $self->scan_op($c), last if $c =~ /^[;&|<>]$/;\n+\t\t$token = $c, last if $c =~ /^[(){}]$/;\n+\t\tif ($c eq '\\\\') {\n+\t\t\t$token .= '\\\\', last unless $$b =~ /\\G(.)/sgc;\n+\t\t\t$c = $1;\n+\t\t\tnext if $c eq \"\\n\" && length($token); # line splice\n+\t\t\tgoto RESTART if $c eq \"\\n\"; # line splice\n+\t\t\t$token .= '\\\\' . $c;\n+\t\t\tnext;\n+\t\t}\n+\t\tdie(\"internal error scanning character '$c'\\n\");\n+\t}\n+\treturn length($token) ? $token : undef;\n+}\n+\n package ScriptParser;\n \n sub new {\n-- \ngitgitgadget\n\n"},{"id":"462391","messageId":"cbd94b343cb0e180d9333f0ecd285d7c7deb7904.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 04/18] chainlint.pl: add parser to validate tests","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:42Z","receivedAt":"2022-09-01T00:30:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nContinue fleshing out chainlint.pl by adding TestParser, a parser with\nspecial knowledge about how Git tests should be written; for instance,\nit knows that commands within a test body should be chained together\nwith `&&`. An upcoming parser which plucks test definitions from test\nscripts will invoke TestParser for each test body it encounters.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex cdf136896be..ad257106e56 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -441,6 +441,52 @@ DONE:\n \treturn @tokens;\n }\n \n+# TestParser is a subclass of ShellParser which, beyond parsing shell script\n+# code, is also imbued with semantic knowledge of test construction, and checks\n+# tests for common problems (such as broken &&-chains) which might hide bugs in\n+# the tests themselves or in behaviors being exercised by the tests. As such,\n+# TestParser is only called upon to parse test bodies, not the top-level\n+# scripts in which the tests are defined.\n+package TestParser;\n+\n+use base 'ShellParser';\n+\n+sub find_non_nl {\n+\tmy $tokens = shift @_;\n+\tmy $n = shift @_;\n+\t$n = $#$tokens if !defined($n);\n+\t$n-- while $n >= 0 && $$tokens[$n] eq \"\\n\";\n+\treturn $n;\n+}\n+\n+sub ends_with {\n+\tmy ($tokens, $needles) = @_;\n+\tmy $n = find_non_nl($tokens);\n+\tfor my $needle (reverse(@$needles)) {\n+\t\treturn undef if $n < 0;\n+\t\t$n = find_non_nl($tokens, $n), next if $needle eq \"\\n\";\n+\t\treturn undef if $$tokens[$n] !~ $needle;\n+\t\t$n--;\n+\t}\n+\treturn 1;\n+}\n+\n+sub accumulate {\n+\tmy ($self, $tokens, $cmd) = @_;\n+\tgoto DONE unless @$tokens;\n+\tgoto DONE if @$cmd == 1 && $$cmd[0] eq \"\\n\";\n+\n+\t# did previous command end with \"&&\", \"||\", \"|\"?\n+\tgoto DONE if ends_with($tokens, [qr/^(?:&&|\\|\\||\\|)$/]);\n+\n+\t# flag missing \"&&\" at end of previous command\n+\tmy $n = find_non_nl($tokens);\n+\tsplice(@$tokens, $n + 1, 0, '?!AMP?!') unless $n < 0;\n+\n+DONE:\n+\t$self->SUPER::accumulate($tokens, $cmd);\n+}\n+\n package ScriptParser;\n \n sub new {\n-- \ngitgitgadget\n\n"},{"id":"462392","messageId":"a71bb11185bc5890964639b7a2ee002fde325d20.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 03/18] chainlint.pl: add POSIX shell parser","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:41Z","receivedAt":"2022-09-01T00:30:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nContinue fleshing out chainlint.pl by adding a general purpose recursive\ndescent parser for the POSIX shell command language. Although never\ninvoked directly, upcoming parser subclasses will extend its\nfunctionality for specific purposes, such as plucking test definitions\nfrom input scripts and applying domain-specific knowledge to perform\ntest validation.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 243 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 243 insertions(+)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 81ffbf28bf3..cdf136896be 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -198,6 +198,249 @@ RESTART:\n \treturn length($token) ? $token : undef;\n }\n \n+# ShellParser parses POSIX shell scripts (with minor extensions for Bash). It\n+# is a recursive descent parser very roughly modeled after section 2.10 \"Shell\n+# Grammar\" of POSIX chapter 2 \"Shell Command Language\".\n+package ShellParser;\n+\n+sub new {\n+\tmy ($class, $s) = @_;\n+\tmy $self = bless {\n+\t\tbuff => [],\n+\t\tstop => [],\n+\t\toutput => []\n+\t} => $class;\n+\t$self->{lexer} = Lexer->new($self, $s);\n+\treturn $self;\n+}\n+\n+sub next_token {\n+\tmy $self = shift @_;\n+\treturn pop(@{$self->{buff}}) if @{$self->{buff}};\n+\treturn $self->{lexer}->scan_token();\n+}\n+\n+sub untoken {\n+\tmy $self = shift @_;\n+\tpush(@{$self->{buff}}, @_);\n+}\n+\n+sub peek {\n+\tmy $self = shift @_;\n+\tmy $token = $self->next_token();\n+\treturn undef unless defined($token);\n+\t$self->untoken($token);\n+\treturn $token;\n+}\n+\n+sub stop_at {\n+\tmy ($self, $token) = @_;\n+\treturn 1 unless defined($token);\n+\tmy $stop = ${$self->{stop}}[-1] if @{$self->{stop}};\n+\treturn defined($stop) && $token =~ $stop;\n+}\n+\n+sub expect {\n+\tmy ($self, $expect) = @_;\n+\tmy $token = $self->next_token();\n+\treturn $token if defined($token) && $token eq $expect;\n+\tpush(@{$self->{output}}, \"?!ERR?! expected '$expect' but found '\" . (defined($token) ? $token : \"\") . \"'\\n\");\n+\t$self->untoken($token) if defined($token);\n+\treturn ();\n+}\n+\n+sub optional_newlines {\n+\tmy $self = shift @_;\n+\tmy @tokens;\n+\twhile (my $token = $self->peek()) {\n+\t\tlast unless $token eq \"\\n\";\n+\t\tpush(@tokens, $self->next_token());\n+\t}\n+\treturn @tokens;\n+}\n+\n+sub parse_group {\n+\tmy $self = shift @_;\n+\treturn ($self->parse(qr/^}$/),\n+\t\t$self->expect('}'));\n+}\n+\n+sub parse_subshell {\n+\tmy $self = shift @_;\n+\treturn ($self->parse(qr/^\\)$/),\n+\t\t$self->expect(')'));\n+}\n+\n+sub parse_case_pattern {\n+\tmy $self = shift @_;\n+\tmy @tokens;\n+\twhile (defined(my $token = $self->next_token())) {\n+\t\tpush(@tokens, $token);\n+\t\tlast if $token eq ')';\n+\t}\n+\treturn @tokens;\n+}\n+\n+sub parse_case {\n+\tmy $self = shift @_;\n+\tmy @tokens;\n+\tpush(@tokens,\n+\t $self->next_token(), # subject\n+\t $self->optional_newlines(),\n+\t $self->expect('in'),\n+\t $self->optional_newlines());\n+\twhile (1) {\n+\t\tmy $token = $self->peek();\n+\t\tlast unless defined($token) && $token ne 'esac';\n+\t\tpush(@tokens,\n+\t\t $self->parse_case_pattern(),\n+\t\t $self->optional_newlines(),\n+\t\t $self->parse(qr/^(?:;;|esac)$/)); # item body\n+\t\t$token = $self->peek();\n+\t\tlast unless defined($token) && $token ne 'esac';\n+\t\tpush(@tokens,\n+\t\t $self->expect(';;'),\n+\t\t $self->optional_newlines());\n+\t}\n+\tpush(@tokens, $self->expect('esac'));\n+\treturn @tokens;\n+}\n+\n+sub parse_for {\n+\tmy $self = shift @_;\n+\tmy @tokens;\n+\tpush(@tokens,\n+\t $self->next_token(), # variable\n+\t $self->optional_newlines());\n+\tmy $token = $self->peek();\n+\tif (defined($token) && $token eq 'in') {\n+\t\tpush(@tokens,\n+\t\t $self->expect('in'),\n+\t\t $self->optional_newlines());\n+\t}\n+\tpush(@tokens,\n+\t $self->parse(qr/^do$/), # items\n+\t $self->expect('do'),\n+\t $self->optional_newlines(),\n+\t $self->parse_loop_body(),\n+\t $self->expect('done'));\n+\treturn @tokens;\n+}\n+\n+sub parse_if {\n+\tmy $self = shift @_;\n+\tmy @tokens;\n+\twhile (1) {\n+\t\tpush(@tokens,\n+\t\t $self->parse(qr/^then$/), # if/elif condition\n+\t\t $self->expect('then'),\n+\t\t $self->optional_newlines(),\n+\t\t $self->parse(qr/^(?:elif|else|fi)$/)); # if/elif body\n+\t\tmy $token = $self->peek();\n+\t\tlast unless defined($token) && $token eq 'elif';\n+\t\tpush(@tokens, $self->expect('elif'));\n+\t}\n+\tmy $token = $self->peek();\n+\tif (defined($token) && $token eq 'else') {\n+\t\tpush(@tokens,\n+\t\t $self->expect('else'),\n+\t\t $self->optional_newlines(),\n+\t\t $self->parse(qr/^fi$/)); # else body\n+\t}\n+\tpush(@tokens, $self->expect('fi'));\n+\treturn @tokens;\n+}\n+\n+sub parse_loop_body {\n+\tmy $self = shift @_;\n+\treturn $self->parse(qr/^done$/);\n+}\n+\n+sub parse_loop {\n+\tmy $self = shift @_;\n+\treturn ($self->parse(qr/^do$/), # condition\n+\t\t$self->expect('do'),\n+\t\t$self->optional_newlines(),\n+\t\t$self->parse_loop_body(),\n+\t\t$self->expect('done'));\n+}\n+\n+sub parse_func {\n+\tmy $self = shift @_;\n+\treturn ($self->expect('('),\n+\t\t$self->expect(')'),\n+\t\t$self->optional_newlines(),\n+\t\t$self->parse_cmd()); # body\n+}\n+\n+sub parse_bash_array_assignment {\n+\tmy $self = shift @_;\n+\tmy @tokens = $self->expect('(');\n+\twhile (defined(my $token = $self->next_token())) {\n+\t\tpush(@tokens, $token);\n+\t\tlast if $token eq ')';\n+\t}\n+\treturn @tokens;\n+}\n+\n+my %compound = (\n+\t'{' => \\&parse_group,\n+\t'(' => \\&parse_subshell,\n+\t'case' => \\&parse_case,\n+\t'for' => \\&parse_for,\n+\t'if' => \\&parse_if,\n+\t'until' => \\&parse_loop,\n+\t'while' => \\&parse_loop);\n+\n+sub parse_cmd {\n+\tmy $self = shift @_;\n+\tmy $cmd = $self->next_token();\n+\treturn () unless defined($cmd);\n+\treturn $cmd if $cmd eq \"\\n\";\n+\n+\tmy $token;\n+\tmy @tokens = $cmd;\n+\tif ($cmd eq '!') {\n+\t\tpush(@tokens, $self->parse_cmd());\n+\t\treturn @tokens;\n+\t} elsif (my $f = $compound{$cmd}) {\n+\t\tpush(@tokens, $self->$f());\n+\t} elsif (defined($token = $self->peek()) && $token eq '(') {\n+\t\tif ($cmd !~ /\\w=$/) {\n+\t\t\tpush(@tokens, $self->parse_func());\n+\t\t\treturn @tokens;\n+\t\t}\n+\t\t$tokens[-1] .= join(' ', $self->parse_bash_array_assignment());\n+\t}\n+\n+\twhile (defined(my $token = $self->next_token())) {\n+\t\t$self->untoken($token), last if $self->stop_at($token);\n+\t\tpush(@tokens, $token);\n+\t\tlast if $token =~ /^(?:[;&\\n|]|&&|\\|\\|)$/;\n+\t}\n+\tpush(@tokens, $self->next_token()) if $tokens[-1] ne \"\\n\" && defined($token = $self->peek()) && $token eq \"\\n\";\n+\treturn @tokens;\n+}\n+\n+sub accumulate {\n+\tmy ($self, $tokens, $cmd) = @_;\n+\tpush(@$tokens, @$cmd);\n+}\n+\n+sub parse {\n+\tmy ($self, $stop) = @_;\n+\tpush(@{$self->{stop}}, $stop);\n+\tgoto DONE if $self->stop_at($self->peek());\n+\tmy @tokens;\n+\twhile (my @cmd = $self->parse_cmd()) {\n+\t\t$self->accumulate(\\@tokens, \\@cmd);\n+\t\tlast if $self->stop_at($self->peek());\n+\t}\n+DONE:\n+\tpop(@{$self->{stop}});\n+\treturn @tokens;\n+}\n+\n package ScriptParser;\n \n sub new {\n-- \ngitgitgadget\n\n"},{"id":"462393","messageId":"62fc652eb47a4df83d88a197e376f28dbbab3b52.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:44Z","receivedAt":"2022-09-01T00:30:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nAlthough chainlint.pl has undergone a good deal of optimization during\nits development -- increasing in speed significantly -- parsing and\nvalidating 1050+ scripts and 16500+ tests via Perl is not exactly\ninstantaneous. However, perceived performance can be improved by taking\nadvantage of the fact that there is no interdependence between test\nscripts or test definitions, thus parsing and validating can be done in\nparallel. The number of available cores is determined automatically but\ncan be overridden via the --jobs option.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 49 insertions(+), 1 deletion(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex d526723ac00..898573a9100 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -15,9 +15,11 @@\n \n use warnings;\n use strict;\n+use Config;\n use File::Glob;\n use Getopt::Long;\n \n+my $jobs = -1;\n my $show_stats;\n my $emit_all;\n \n@@ -569,6 +571,16 @@ if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {\n \t$interval = sub { return Time::HiRes::tv_interval(shift); };\n }\n \n+sub ncores {\n+\t# Windows\n+\treturn $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});\n+\t# Linux / MSYS2 / Cygwin / WSL\n+\tdo { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor\\s*:/, <>)); } if -r '/proc/cpuinfo';\n+\t# macOS & BSD\n+\treturn qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;\n+\treturn 1;\n+}\n+\n sub show_stats {\n \tmy ($start_time, $stats) = @_;\n \tmy $walltime = $interval->($start_time);\n@@ -621,7 +633,9 @@ sub exit_code {\n Getopt::Long::Configure(qw{bundling});\n GetOptions(\n \t\"emit-all!\" => \\$emit_all,\n+\t\"jobs|j=i\" => \\$jobs,\n \t\"stats|show-stats!\" => \\$show_stats) or die(\"option error\\n\");\n+$jobs = ncores() if $jobs < 1;\n \n my $start_time = $getnow->();\n my @stats;\n@@ -633,6 +647,40 @@ unless (@scripts) {\n \texit;\n }\n \n-push(@stats, check_script(1, sub { shift(@scripts); }, sub { print(@_); }));\n+unless ($Config{useithreads} && eval {\n+\trequire threads; threads->import();\n+\trequire Thread::Queue; Thread::Queue->import();\n+\t1;\n+\t}) {\n+\tpush(@stats, check_script(1, sub { shift(@scripts); }, sub { print(@_); }));\n+\tshow_stats($start_time, \\@stats) if $show_stats;\n+\texit(exit_code(\\@stats));\n+}\n+\n+my $script_queue = Thread::Queue->new();\n+my $output_queue = Thread::Queue->new();\n+\n+sub next_script { return $script_queue->dequeue(); }\n+sub emit { $output_queue->enqueue(@_); }\n+\n+sub monitor {\n+\twhile (my $s = $output_queue->dequeue()) {\n+\t\tprint($s);\n+\t}\n+}\n+\n+my $mon = threads->create({'context' => 'void'}, \\&monitor);\n+threads->create({'context' => 'list'}, \\&check_script, $_, \\&next_script, \\&emit) for 1..$jobs;\n+\n+$script_queue->enqueue(@scripts);\n+$script_queue->end();\n+\n+for (threads->list()) {\n+\tpush(@stats, $_->join()) unless $_ == $mon;\n+}\n+\n+$output_queue->end();\n+$mon->join();\n+\n show_stats($start_time, \\@stats) if $show_stats;\n exit(exit_code(\\@stats));\n-- \ngitgitgadget\n\n"},{"id":"462394","messageId":"0de14477a42f2c18efb4b1e0ba52155645a7f0e2.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 05/18] chainlint.pl: add parser to identify test definitions","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:43Z","receivedAt":"2022-09-01T00:30:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nFinish fleshing out chainlint.pl by adding ScriptParser, a parser which\nscans shell scripts for tests defined by test_expect_success() and\ntest_expect_failure(), plucks the test body from each definition, and\npasses it to TestParser for validation. It recognizes test definitions\nnot only at the top-level of test scripts but also tests synthesized\nwithin compound commands such as loops and function.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 63 +++++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 60 insertions(+), 3 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex ad257106e56..d526723ac00 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -487,18 +487,75 @@ DONE:\n \t$self->SUPER::accumulate($tokens, $cmd);\n }\n \n+# ScriptParser is a subclass of ShellParser which identifies individual test\n+# definitions within test scripts, and passes each test body through TestParser\n+# to identify possible problems. ShellParser detects test definitions not only\n+# at the top-level of test scripts but also within compound commands such as\n+# loops and function definitions.\n package ScriptParser;\n \n+use base 'ShellParser';\n+\n sub new {\n \tmy $class = shift @_;\n-\tmy $self = bless {} => $class;\n-\t$self->{output} = [];\n+\tmy $self = $class->SUPER::new(@_);\n \t$self->{ntests} = 0;\n \treturn $self;\n }\n \n+# extract the raw content of a token, which may be a single string or a\n+# composition of multiple strings and non-string character runs; for instance,\n+# `\"test body\"` unwraps to `test body`; `word\"a b\"42'c d'` to `worda b42c d`\n+sub unwrap {\n+\tmy $token = @_ ? shift @_ : $_;\n+\t# simple case: 'sqstring' or \"dqstring\"\n+\treturn $token if $token =~ s/^'([^']*)'$/$1/;\n+\treturn $token if $token =~ s/^\"([^\"]*)\"$/$1/;\n+\n+\t# composite case\n+\tmy ($s, $q, $escaped);\n+\twhile (1) {\n+\t\t# slurp up non-special characters\n+\t\t$s .= $1 if $token =~ /\\G([^\\\\'\"]*)/gc;\n+\t\t# handle special characters\n+\t\tlast unless $token =~ /\\G(.)/sgc;\n+\t\tmy $c = $1;\n+\t\t$q = undef, next if defined($q) && $c eq $q;\n+\t\t$q = $c, next if !defined($q) && $c =~ /^['\"]$/;\n+\t\tif ($c eq '\\\\') {\n+\t\t\tlast unless $token =~ /\\G(.)/sgc;\n+\t\t\t$c = $1;\n+\t\t\t$s .= '\\\\' if $c eq \"\\n\"; # preserve line splice\n+\t\t}\n+\t\t$s .= $c;\n+\t}\n+\treturn $s\n+}\n+\n+sub check_test {\n+\tmy $self = shift @_;\n+\tmy ($title, $body) = map(unwrap, @_);\n+\t$self->{ntests}++;\n+\tmy $parser = TestParser->new(\\$body);\n+\tmy @tokens = $parser->parse();\n+\treturn unless $emit_all || grep(/\\?![^?]+\\?!/, @tokens);\n+\tmy $checked = join(' ', @tokens);\n+\t$checked =~ s/^\\n//;\n+\t$checked =~ s/^ //mg;\n+\t$checked =~ s/ $//mg;\n+\t$checked .= \"\\n\" unless $checked =~ /\\n$/;\n+\tpush(@{$self->{output}}, \"# chainlint: $title\\n$checked\");\n+}\n+\n sub parse_cmd {\n-\treturn undef;\n+\tmy $self = shift @_;\n+\tmy @tokens = $self->SUPER::parse_cmd();\n+\treturn @tokens unless @tokens && $tokens[0] =~ /^test_expect_(?:success|failure)$/;\n+\tmy $n = $#tokens;\n+\t$n-- while $n >= 0 && $tokens[$n] =~ /^(?:[;&\\n|]|&&|\\|\\|)$/;\n+\t$self->check_test($tokens[1], $tokens[2]) if $n == 2; # title body\n+\t$self->check_test($tokens[2], $tokens[3]) if $n > 2; # prereq title body\n+\treturn @tokens;\n }\n \n # main contains high-level functionality for processing command-line switches,\n-- \ngitgitgadget\n\n"},{"id":"462395","messageId":"ee627a09719a4a7347c97783c1bf8f9cb9ddbf89.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 07/18] chainlint.pl: don't require `return|exit|continue` to end with `&&`","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:45Z","receivedAt":"2022-09-01T00:30:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nIn order to check for &&-chain breakage, each time TestParser encounters\na new command, it checks whether the previous command ends with `&&`,\nand -- with a couple exceptions -- signals breakage if it does not. The\nfirst exception is that a command may validly end with `||`, which is\ncommonly employed as `command || return 1` at the very end of a loop\nbody to terminate the loop early. The second is that piping one\ncommand's output with `|` to another command does not constitute a\n&&-chain break (the exit status of the pipe is the exit status of the\nfinal command in the pipe).\n\nHowever, it turns out that there are a few additional cases found in the\nwild in which it is likely safe for `&&` to be missing even when other\ncommands follow. For instance:\n\n while {condition-1}\n do\n test {condition-2} || return 1 # or `exit 1` within a subshell\n more-commands\n done\n\n while {condition-1}\n do\n test {condition-2} || continue\n more-commands\n done\n\nSuch cases indicate deliberate thought about failure modes by the test\nauthor, thus flagging them as breaking the &&-chain is not helpful.\nTherefore, take these special cases into consideration when checking for\n&&-chain breakage.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 20 ++++++++++++++++++--\n t/chainlint/chain-break-continue.expect | 12 ++++++++++++\n t/chainlint/chain-break-continue.test | 13 +++++++++++++\n t/chainlint/chain-break-return-exit.expect | 4 ++++\n t/chainlint/chain-break-return-exit.test | 5 +++++\n t/chainlint/return-loop.expect | 5 +++++\n t/chainlint/return-loop.test | 6 ++++++\n 7 files changed, 63 insertions(+), 2 deletions(-)\n create mode 100644 t/chainlint/chain-break-continue.expect\n create mode 100644 t/chainlint/chain-break-continue.test\n create mode 100644 t/chainlint/chain-break-return-exit.expect\n create mode 100644 t/chainlint/chain-break-return-exit.test\n create mode 100644 t/chainlint/return-loop.expect\n create mode 100644 t/chainlint/return-loop.test\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 898573a9100..31c444067ce 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -473,13 +473,29 @@ sub ends_with {\n \treturn 1;\n }\n \n+sub match_ending {\n+\tmy ($tokens, $endings) = @_;\n+\tfor my $needles (@$endings) {\n+\t\tnext if @$tokens < scalar(grep {$_ ne \"\\n\"} @$needles);\n+\t\treturn 1 if ends_with($tokens, $needles);\n+\t}\n+\treturn undef;\n+}\n+\n+my @safe_endings = (\n+\t[qr/^(?:&&|\\|\\||\\|)$/],\n+\t[qr/^(?:exit|return)$/, qr/^(?:\\d+|\\$\\?)$/],\n+\t[qr/^(?:exit|return)$/, qr/^(?:\\d+|\\$\\?)$/, qr/^;$/],\n+\t[qr/^(?:exit|return|continue)$/],\n+\t[qr/^(?:exit|return|continue)$/, qr/^;$/]);\n+\n sub accumulate {\n \tmy ($self, $tokens, $cmd) = @_;\n \tgoto DONE unless @$tokens;\n \tgoto DONE if @$cmd == 1 && $$cmd[0] eq \"\\n\";\n \n-\t# did previous command end with \"&&\", \"||\", \"|\"?\n-\tgoto DONE if ends_with($tokens, [qr/^(?:&&|\\|\\||\\|)$/]);\n+\t# did previous command end with \"&&\", \"|\", \"|| return\" or similar?\n+\tgoto DONE if match_ending($tokens, \\@safe_endings);\n \n \t# flag missing \"&&\" at end of previous command\n \tmy $n = find_non_nl($tokens);\ndiff --git a/t/chainlint/chain-break-continue.expect b/t/chainlint/chain-break-continue.expect\nnew file mode 100644\nindex 00000000000..47a34577100\n--- /dev/null\n+++ b/t/chainlint/chain-break-continue.expect\n@@ -0,0 +1,12 @@\n+git ls-tree --name-only -r refs/notes/many_notes |\n+while read path\n+do\n+\ttest \"$path\" = \"foobar/non-note.txt\" && continue\n+\ttest \"$path\" = \"deadbeef\" && continue\n+\ttest \"$path\" = \"de/adbeef\" && continue\n+\n+\tif test $(expr length \"$path\") -ne $hexsz\n+\tthen\n+\t\treturn 1\n+\tfi\n+done\ndiff --git a/t/chainlint/chain-break-continue.test b/t/chainlint/chain-break-continue.test\nnew file mode 100644\nindex 00000000000..f0af71d8bd9\n--- /dev/null\n+++ b/t/chainlint/chain-break-continue.test\n@@ -0,0 +1,13 @@\n+git ls-tree --name-only -r refs/notes/many_notes |\n+while read path\n+do\n+# LINT: broken &&-chain okay if explicit \"continue\"\n+\ttest \"$path\" = \"foobar/non-note.txt\" && continue\n+\ttest \"$path\" = \"deadbeef\" && continue\n+\ttest \"$path\" = \"de/adbeef\" && continue\n+\n+\tif test $(expr length \"$path\") -ne $hexsz\n+\tthen\n+\t\treturn 1\n+\tfi\n+done\ndiff --git a/t/chainlint/chain-break-return-exit.expect b/t/chainlint/chain-break-return-exit.expect\nnew file mode 100644\nindex 00000000000..dba292ee89b\n--- /dev/null\n+++ b/t/chainlint/chain-break-return-exit.expect\n@@ -0,0 +1,4 @@\n+for i in 1 2 3 4 ; do\n+\tgit checkout main -b $i || return $?\n+\ttest_commit $i $i $i tag$i || return $?\n+done\ndiff --git a/t/chainlint/chain-break-return-exit.test b/t/chainlint/chain-break-return-exit.test\nnew file mode 100644\nindex 00000000000..e2b059933aa\n--- /dev/null\n+++ b/t/chainlint/chain-break-return-exit.test\n@@ -0,0 +1,5 @@\n+for i in 1 2 3 4 ; do\n+# LINT: broken &&-chain okay if explicit \"return $?\" signals failure\n+\tgit checkout main -b $i || return $?\n+\ttest_commit $i $i $i tag$i || return $?\n+done\ndiff --git a/t/chainlint/return-loop.expect b/t/chainlint/return-loop.expect\nnew file mode 100644\nindex 00000000000..cfc0549befe\n--- /dev/null\n+++ b/t/chainlint/return-loop.expect\n@@ -0,0 +1,5 @@\n+while test $i -lt $((num - 5))\n+do\n+\tgit notes add -m \"notes for commit$i\" HEAD~$i || return 1\n+\ti=$((i + 1))\n+done\ndiff --git a/t/chainlint/return-loop.test b/t/chainlint/return-loop.test\nnew file mode 100644\nindex 00000000000..f90b1713005\n--- /dev/null\n+++ b/t/chainlint/return-loop.test\n@@ -0,0 +1,6 @@\n+while test $i -lt $((num - 5))\n+do\n+# LINT: \"|| return {n}\" valid loop escape outside subshell; no \"&&\" needed\n+\tgit notes add -m \"notes for commit$i\" HEAD~$i || return 1\n+\ti=$((i + 1))\n+done\n-- \ngitgitgadget\n\n"},{"id":"462396","messageId":"7df396ddea4cdaf9d014bb90a38da010676c1ce8.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 10/18] chainlint.pl: don't flag broken &&-chain if `$?` handled explicitly","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:48Z","receivedAt":"2022-09-01T00:30:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nThere are cases in which tests capture and check a command's exit code\nexplicitly without employing test_expect_code(). They do so by\nintentionally breaking the &&-chain since it would be impossible to\ncapture \"$?\" in the failing case if the `status=$?` assignment was part\nof the &&-chain. Since such constructs are manually checking the exit\ncode, their &&-chain breakage is legitimate and safe, thus should not be\nflagged. Therefore, stop flagging &&-chain breakage in such cases.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 6 ++++++\n t/chainlint/chain-break-status.expect | 9 +++++++++\n t/chainlint/chain-break-status.test | 11 +++++++++++\n 3 files changed, 26 insertions(+)\n create mode 100644 t/chainlint/chain-break-status.expect\n create mode 100644 t/chainlint/chain-break-status.test\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex ba3fcb0c8e6..14e1db3519a 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -497,6 +497,12 @@ sub accumulate {\n \t# did previous command end with \"&&\", \"|\", \"|| return\" or similar?\n \tgoto DONE if match_ending($tokens, \\@safe_endings);\n \n+\t# if this command handles \"$?\" specially, then okay for previous\n+\t# command to be missing \"&&\"\n+\tfor my $token (@$cmd) {\n+\t\tgoto DONE if $token =~ /\\$\\?/;\n+\t}\n+\n \t# flag missing \"&&\" at end of previous command\n \tmy $n = find_non_nl($tokens);\n \tsplice(@$tokens, $n + 1, 0, '?!AMP?!') unless $n < 0;\ndiff --git a/t/chainlint/chain-break-status.expect b/t/chainlint/chain-break-status.expect\nnew file mode 100644\nindex 00000000000..f4bada94632\n--- /dev/null\n+++ b/t/chainlint/chain-break-status.expect\n@@ -0,0 +1,9 @@\n+OUT=$(( ( large_git ; echo $? 1 >& 3 ) | : ) 3 >& 1) &&\n+test_match_signal 13 \"$OUT\" &&\n+\n+{ test-tool sigchain > actual ; ret=$? ; } &&\n+{\n+\ttest_match_signal 15 \"$ret\" ||\n+\ttest \"$ret\" = 3\n+} &&\n+test_cmp expect actual\ndiff --git a/t/chainlint/chain-break-status.test b/t/chainlint/chain-break-status.test\nnew file mode 100644\nindex 00000000000..a6602a7b99c\n--- /dev/null\n+++ b/t/chainlint/chain-break-status.test\n@@ -0,0 +1,11 @@\n+# LINT: broken &&-chain okay if next command handles \"$?\" explicitly\n+OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&\n+test_match_signal 13 \"$OUT\" &&\n+\n+# LINT: broken &&-chain okay if next command handles \"$?\" explicitly\n+{ test-tool sigchain >actual; ret=$?; } &&\n+{\n+\ttest_match_signal 15 \"$ret\" ||\n+\ttest \"$ret\" = 3\n+} &&\n+test_cmp expect actual\n-- \ngitgitgadget\n\n"},{"id":"462397","messageId":"1049172aaca8e13f64201dc0ebf04ba5c655c0ce.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 12/18] chainlint.pl: complain about loops lacking explicit failure handling","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:50Z","receivedAt":"2022-09-01T00:30:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nShell `for` and `while` loops do not terminate automatically just\nbecause a command fails within the loop body. Instead, the loop\ncontinues to iterate and eventually returns the exit status of the final\ncommand of the final iteration, which may not be the command which\nfailed, thus it is possible for failures to go undetected. Consequently,\nit is important for test authors to explicitly handle failure within the\nloop body by terminating the loop manually upon failure. This can be\ndone by returning a non-zero exit code from within the loop body\n(i.e. `|| return 1`) or exiting (i.e. `|| exit 1`) if the loop is within\na subshell, or by manually checking `$?` and taking some appropriate\naction. Therefore, add logic to detect and complain about loops which\nlack explicit `return` or `exit`, or `$?` check.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 11 ++++++\n t/chainlint/complex-if-in-cuddled-loop.expect | 2 +-\n t/chainlint/for-loop.expect | 4 +--\n t/chainlint/loop-detect-failure.expect | 15 ++++++++\n t/chainlint/loop-detect-failure.test | 17 +++++++++\n t/chainlint/loop-detect-status.expect | 18 ++++++++++\n t/chainlint/loop-detect-status.test | 19 ++++++++++\n t/chainlint/loop-in-if.expect | 2 +-\n t/chainlint/nested-loop-detect-failure.expect | 31 ++++++++++++++++\n t/chainlint/nested-loop-detect-failure.test | 35 +++++++++++++++++++\n t/chainlint/semicolon.expect | 2 +-\n t/chainlint/while-loop.expect | 4 +--\n 12 files changed, 153 insertions(+), 7 deletions(-)\n create mode 100644 t/chainlint/loop-detect-failure.expect\n create mode 100644 t/chainlint/loop-detect-failure.test\n create mode 100644 t/chainlint/loop-detect-status.expect\n create mode 100644 t/chainlint/loop-detect-status.test\n create mode 100644 t/chainlint/nested-loop-detect-failure.expect\n create mode 100644 t/chainlint/nested-loop-detect-failure.test\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex a76a09ecf5e..674b3ddf696 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -482,6 +482,17 @@ sub match_ending {\n \treturn undef;\n }\n \n+sub parse_loop_body {\n+\tmy $self = shift @_;\n+\tmy @tokens = $self->SUPER::parse_loop_body(@_);\n+\t# did loop signal failure via \"|| return\" or \"|| exit\"?\n+\treturn @tokens if !@tokens || grep(/^(?:return|exit|\\$\\?)$/, @tokens);\n+\t# flag missing \"return/exit\" handling explicit failure in loop body\n+\tmy $n = find_non_nl(\\@tokens);\n+\tsplice(@tokens, $n + 1, 0, '?!LOOP?!');\n+\treturn @tokens;\n+}\n+\n my @safe_endings = (\n \t[qr/^(?:&&|\\|\\||\\||&)$/],\n \t[qr/^(?:exit|return)$/, qr/^(?:\\d+|\\$\\?)$/],\ndiff --git a/t/chainlint/complex-if-in-cuddled-loop.expect b/t/chainlint/complex-if-in-cuddled-loop.expect\nindex 2fca1834095..dac2d0fd1d9 100644\n--- a/t/chainlint/complex-if-in-cuddled-loop.expect\n+++ b/t/chainlint/complex-if-in-cuddled-loop.expect\n@@ -4,6 +4,6 @@\n :\n else\n echo >file\n- fi\n+ fi ?!LOOP?!\n done) &&\n test ! -f file\ndiff --git a/t/chainlint/for-loop.expect b/t/chainlint/for-loop.expect\nindex 6671b8cd842..a5810c9bddd 100644\n--- a/t/chainlint/for-loop.expect\n+++ b/t/chainlint/for-loop.expect\n@@ -2,10 +2,10 @@\n \tfor i in a b c\n \tdo\n \t\techo $i ?!AMP?!\n-\t\tcat <<-EOF\n+\t\tcat <<-EOF ?!LOOP?!\n \tdone ?!AMP?!\n \tfor i in a b c; do\n \t\techo $i &&\n-\t\tcat $i\n+\t\tcat $i ?!LOOP?!\n \tdone\n )\ndiff --git a/t/chainlint/loop-detect-failure.expect b/t/chainlint/loop-detect-failure.expect\nnew file mode 100644\nindex 00000000000..a66025c39d4\n--- /dev/null\n+++ b/t/chainlint/loop-detect-failure.expect\n@@ -0,0 +1,15 @@\n+git init r1 &&\n+for n in 1 2 3 4 5\n+do\n+\techo \"This is file: $n\" > r1/file.$n &&\n+\tgit -C r1 add file.$n &&\n+\tgit -C r1 commit -m \"$n\" || return 1\n+done &&\n+\n+git init r2 &&\n+for n in 1000 10000\n+do\n+\tprintf \"%\"$n\"s\" X > r2/large.$n &&\n+\tgit -C r2 add large.$n &&\n+\tgit -C r2 commit -m \"$n\" ?!LOOP?!\n+done\ndiff --git a/t/chainlint/loop-detect-failure.test b/t/chainlint/loop-detect-failure.test\nnew file mode 100644\nindex 00000000000..b9791cc802e\n--- /dev/null\n+++ b/t/chainlint/loop-detect-failure.test\n@@ -0,0 +1,17 @@\n+git init r1 &&\n+# LINT: loop handles failure explicitly with \"|| return 1\"\n+for n in 1 2 3 4 5\n+do\n+\techo \"This is file: $n\" > r1/file.$n &&\n+\tgit -C r1 add file.$n &&\n+\tgit -C r1 commit -m \"$n\" || return 1\n+done &&\n+\n+git init r2 &&\n+# LINT: loop fails to handle failure explicitly with \"|| return 1\"\n+for n in 1000 10000\n+do\n+\tprintf \"%\"$n\"s\" X > r2/large.$n &&\n+\tgit -C r2 add large.$n &&\n+\tgit -C r2 commit -m \"$n\"\n+done\ndiff --git a/t/chainlint/loop-detect-status.expect b/t/chainlint/loop-detect-status.expect\nnew file mode 100644\nindex 00000000000..0ad23bb35e4\n--- /dev/null\n+++ b/t/chainlint/loop-detect-status.expect\n@@ -0,0 +1,18 @@\n+( while test $i -le $blobcount\n+do\n+\tprintf \"Generating blob $i/$blobcount\\r\" >& 2 &&\n+\tprintf \"blob\\nmark :$i\\ndata $blobsize\\n\" &&\n+\n+\tprintf \"%-${blobsize}s\" $i &&\n+\techo \"M 100644 :$i $i\" >> commit &&\n+\ti=$(($i+1)) ||\n+\techo $? > exit-status\n+done &&\n+echo \"commit refs/heads/main\" &&\n+echo \"author A U Thor 123456789 +0000\" &&\n+echo \"committer C O Mitter 123456789 +0000\" &&\n+echo \"data 5\" &&\n+echo \">2gb\" &&\n+cat commit ) |\n+git fast-import --big-file-threshold=2 &&\n+test ! -f exit-status\ndiff --git a/t/chainlint/loop-detect-status.test b/t/chainlint/loop-detect-status.test\nnew file mode 100644\nindex 00000000000..1c6c23cfc9e\n--- /dev/null\n+++ b/t/chainlint/loop-detect-status.test\n@@ -0,0 +1,19 @@\n+# LINT: \"$?\" handled explicitly within loop body\n+(while test $i -le $blobcount\n+ do\n+\tprintf \"Generating blob $i/$blobcount\\r\" >&2 &&\n+\tprintf \"blob\\nmark :$i\\ndata $blobsize\\n\" &&\n+\t#test-tool genrandom $i $blobsize &&\n+\tprintf \"%-${blobsize}s\" $i &&\n+\techo \"M 100644 :$i $i\" >> commit &&\n+\ti=$(($i+1)) ||\n+\techo $? > exit-status\n+ done &&\n+ echo \"commit refs/heads/main\" &&\n+ echo \"author A U Thor 123456789 +0000\" &&\n+ echo \"committer C O Mitter 123456789 +0000\" &&\n+ echo \"data 5\" &&\n+ echo \">2gb\" &&\n+ cat commit) |\n+git fast-import --big-file-threshold=2 &&\n+test ! -f exit-status\ndiff --git a/t/chainlint/loop-in-if.expect b/t/chainlint/loop-in-if.expect\nindex e1be42376c5..6c5d6e5b243 100644\n--- a/t/chainlint/loop-in-if.expect\n+++ b/t/chainlint/loop-in-if.expect\n@@ -4,7 +4,7 @@\n \t\twhile true\n \t\tdo\n \t\t\techo \"pop\" ?!AMP?!\n-\t\t\techo \"glup\"\n+\t\t\techo \"glup\" ?!LOOP?!\n \t\tdone ?!AMP?!\n \t\tfoo\n \tfi ?!AMP?!\ndiff --git a/t/chainlint/nested-loop-detect-failure.expect b/t/chainlint/nested-loop-detect-failure.expect\nnew file mode 100644\nindex 00000000000..4793a0e8e12\n--- /dev/null\n+++ b/t/chainlint/nested-loop-detect-failure.expect\n@@ -0,0 +1,31 @@\n+for i in 0 1 2 3 4 5 6 7 8 9 ;\n+do\n+\tfor j in 0 1 2 3 4 5 6 7 8 9 ;\n+\tdo\n+\t\techo \"$i$j\" > \"path$i$j\" ?!LOOP?!\n+\tdone ?!LOOP?!\n+done &&\n+\n+for i in 0 1 2 3 4 5 6 7 8 9 ;\n+do\n+\tfor j in 0 1 2 3 4 5 6 7 8 9 ;\n+\tdo\n+\t\techo \"$i$j\" > \"path$i$j\" || return 1\n+\tdone\n+done &&\n+\n+for i in 0 1 2 3 4 5 6 7 8 9 ;\n+do\n+\tfor j in 0 1 2 3 4 5 6 7 8 9 ;\n+\tdo\n+\t\techo \"$i$j\" > \"path$i$j\" ?!LOOP?!\n+\tdone || return 1\n+done &&\n+\n+for i in 0 1 2 3 4 5 6 7 8 9 ;\n+do\n+\tfor j in 0 1 2 3 4 5 6 7 8 9 ;\n+\tdo\n+\t\techo \"$i$j\" > \"path$i$j\" || return 1\n+\tdone || return 1\n+done\ndiff --git a/t/chainlint/nested-loop-detect-failure.test b/t/chainlint/nested-loop-detect-failure.test\nnew file mode 100644\nindex 00000000000..e6f0c1acfb8\n--- /dev/null\n+++ b/t/chainlint/nested-loop-detect-failure.test\n@@ -0,0 +1,35 @@\n+# LINT: neither loop handles failure explicitly with \"|| return 1\"\n+for i in 0 1 2 3 4 5 6 7 8 9;\n+do\n+\tfor j in 0 1 2 3 4 5 6 7 8 9;\n+\tdo\n+\t\techo \"$i$j\" >\"path$i$j\"\n+\tdone\n+done &&\n+\n+# LINT: inner loop handles failure explicitly with \"|| return 1\"\n+for i in 0 1 2 3 4 5 6 7 8 9;\n+do\n+\tfor j in 0 1 2 3 4 5 6 7 8 9;\n+\tdo\n+\t\techo \"$i$j\" >\"path$i$j\" || return 1\n+\tdone\n+done &&\n+\n+# LINT: outer loop handles failure explicitly with \"|| return 1\"\n+for i in 0 1 2 3 4 5 6 7 8 9;\n+do\n+\tfor j in 0 1 2 3 4 5 6 7 8 9;\n+\tdo\n+\t\techo \"$i$j\" >\"path$i$j\"\n+\tdone || return 1\n+done &&\n+\n+# LINT: inner & outer loops handles failure explicitly with \"|| return 1\"\n+for i in 0 1 2 3 4 5 6 7 8 9;\n+do\n+\tfor j in 0 1 2 3 4 5 6 7 8 9;\n+\tdo\n+\t\techo \"$i$j\" >\"path$i$j\" || return 1\n+\tdone || return 1\n+done\ndiff --git a/t/chainlint/semicolon.expect b/t/chainlint/semicolon.expect\nindex ed0b3707ae9..3aa2259f36c 100644\n--- a/t/chainlint/semicolon.expect\n+++ b/t/chainlint/semicolon.expect\n@@ -15,5 +15,5 @@\n ) &&\n (cd foo &&\n \tfor i in a b c; do\n-\t\techo;\n+\t\techo; ?!LOOP?!\n \tdone)\ndiff --git a/t/chainlint/while-loop.expect b/t/chainlint/while-loop.expect\nindex 0d3a9b3d128..f272aa21fee 100644\n--- a/t/chainlint/while-loop.expect\n+++ b/t/chainlint/while-loop.expect\n@@ -2,10 +2,10 @@\n \twhile true\n \tdo\n \t\techo foo ?!AMP?!\n-\t\tcat <<-EOF\n+\t\tcat <<-EOF ?!LOOP?!\n \tdone ?!AMP?!\n \twhile true; do\n \t\techo foo &&\n-\t\tcat bar\n+\t\tcat bar ?!LOOP?!\n \tdone\n )\n-- \ngitgitgadget\n\n"},{"id":"462398","messageId":"86a718bfa5585c826fc98f8844a52faabb45fa55.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 09/18] chainlint.pl: don't require `&` background command to end with `&&`","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:47Z","receivedAt":"2022-09-01T00:30:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nThe exit status of the `&` asynchronous operator which starts a command\nin the background is unconditionally zero, and the few places in the\ntest scripts which launch commands asynchronously are not interested in\nthe exit status of the `&` operator (though they often capture the\nbackground command's PID). As such, there is little value in complaining\nabout broken &&-chain for a command launched in the background, and\ndoing so would only make busy-work for test authors. Therefore, take\nthis special case into account when checking for &&-chain breakage.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 2 +-\n t/chainlint/chain-break-background.expect | 9 +++++++++\n t/chainlint/chain-break-background.test | 10 ++++++++++\n 3 files changed, 20 insertions(+), 1 deletion(-)\n create mode 100644 t/chainlint/chain-break-background.expect\n create mode 100644 t/chainlint/chain-break-background.test\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 31c444067ce..ba3fcb0c8e6 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -483,7 +483,7 @@ sub match_ending {\n }\n \n my @safe_endings = (\n-\t[qr/^(?:&&|\\|\\||\\|)$/],\n+\t[qr/^(?:&&|\\|\\||\\||&)$/],\n \t[qr/^(?:exit|return)$/, qr/^(?:\\d+|\\$\\?)$/],\n \t[qr/^(?:exit|return)$/, qr/^(?:\\d+|\\$\\?)$/, qr/^;$/],\n \t[qr/^(?:exit|return|continue)$/],\ndiff --git a/t/chainlint/chain-break-background.expect b/t/chainlint/chain-break-background.expect\nnew file mode 100644\nindex 00000000000..28f9114f42d\n--- /dev/null\n+++ b/t/chainlint/chain-break-background.expect\n@@ -0,0 +1,9 @@\n+JGIT_DAEMON_PID= &&\n+git init --bare empty.git &&\n+> empty.git/git-daemon-export-ok &&\n+mkfifo jgit_daemon_output &&\n+{\n+\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . > jgit_daemon_output &\n+\tJGIT_DAEMON_PID=$!\n+} &&\n+test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git\ndiff --git a/t/chainlint/chain-break-background.test b/t/chainlint/chain-break-background.test\nnew file mode 100644\nindex 00000000000..e10f656b055\n--- /dev/null\n+++ b/t/chainlint/chain-break-background.test\n@@ -0,0 +1,10 @@\n+JGIT_DAEMON_PID= &&\n+git init --bare empty.git &&\n+>empty.git/git-daemon-export-ok &&\n+mkfifo jgit_daemon_output &&\n+{\n+# LINT: exit status of \"&\" is always 0 so &&-chaining immaterial\n+\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . >jgit_daemon_output &\n+\tJGIT_DAEMON_PID=$!\n+} &&\n+test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git\n-- \ngitgitgadget\n\n"},{"id":"462399","messageId":"737d666bf9e309686f95e4909998c33200c1e0a4.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 08/18] t/Makefile: apply chainlint.pl to existing self-tests","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:46Z","receivedAt":"2022-09-01T00:30:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nNow that chainlint.pl is functional, take advantage of the existing\nchainlint self-tests to validate its operation. (While at it, stop\nvalidating chainlint.sed against the self-tests since it will soon be\nretired.)\n\nDue to chainlint.sed implementation limitations leaking into the\nself-test \"expect\" files, a few of them require minor adjustment to make\nthem compatible with chainlint.pl which does not share those\nlimitations.\n\nFirst, because `sed` does not provide any sort of real recursion,\nchainlint.sed only emulates recursion into subshells, and each level of\nrecursion leads to a multiplicative increase in complexity of the `sed`\nrules. To avoid substantial complexity, chainlint.sed, therefore, only\nemulates subshell recursion one level deep. Any subshell deeper than\nthat is passed through as-is, which means that &&-chains are not checked\nin deeper subshells. chainlint.pl, on the other hand, employs a proper\nrecursive descent parser, thus checks subshells to any depth and\ncorrectly flags broken &&-chains in deep subshells.\n\nSecond, due to sed's line-oriented nature, chainlint.sed, by necessity,\nfolds multi-line quoted strings into a single line. chainlint.pl, on the\nother hand, employs a proper lexical analyzer which preserves quoted\nstrings as-is, including embedded newlines.\n\nFurthermore, the output of chainlint.sed and chainlint.pl do not match\nprecisely in terms of whitespace. However, since the purpose of the\nself-checks is to verify that the ?!AMP?! annotations are being\ncorrectly added, minor whitespace differences are immaterial. For this\nreason, rather than adjusting whitespace in all existing self-test\n\"expect\" files to match the new linter's output, the `check-chainlint`\ntarget ignores whitespace differences. Since `diff -w` is not POSIX,\n`check-chainlint` attempts to employ `git diff -w`, and only falls back\nto non-POSIX `diff -w` (and `-u`) if `git diff` is not available.\n\nSigned-off-by: Eric Sunshine \n---\n t/Makefile | 29 +++++++++++++++----\n t/chainlint/block.expect | 2 +-\n t/chainlint/here-doc-multi-line-string.expect | 3 +-\n t/chainlint/multi-line-string.expect | 11 +++++--\n t/chainlint/nested-subshell.expect | 2 +-\n t/chainlint/t7900-subtree.expect | 13 +++++++--\n 6 files changed, 46 insertions(+), 14 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 1c80c0c79a0..11f276774ea 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -38,7 +38,7 @@ T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))\n TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n-CHAINLINT = sed -f chainlint.sed\n+CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n \n all: $(DEFAULT_TEST_TARGET)\n \n@@ -73,10 +73,29 @@ clean-chainlint:\n \n check-chainlint:\n \t@mkdir -p '$(CHAINLINTTMP_SQ)' && \\\n-\tsed -e '/^# LINT: /d' $(patsubst %,chainlint/%.test,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/tests && \\\n-\tsed -e '/^[ \t]*$$/d' $(patsubst %,chainlint/%.expect,$(CHAINLINTTESTS)) >'$(CHAINLINTTMP_SQ)'/expect && \\\n-\t$(CHAINLINT) '$(CHAINLINTTMP_SQ)'/tests | grep -v '^[\t]*$$' >'$(CHAINLINTTMP_SQ)'/actual && \\\n-\tdiff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n+\tfor i in $(CHAINLINTTESTS); do \\\n+\t\techo \"test_expect_success '$$i' '\" && \\\n+\t\tsed -e '/^# LINT: /d' chainlint/$$i.test && \\\n+\t\techo \"'\"; \\\n+\tdone >'$(CHAINLINTTMP_SQ)'/tests && \\\n+\t{ \\\n+\t\techo \"# chainlint: $(CHAINLINTTMP_SQ)/tests\" && \\\n+\t\tfor i in $(CHAINLINTTESTS); do \\\n+\t\t\techo \"# chainlint: $$i\" && \\\n+\t\t\tsed -e '/^[ \t]*$$/d' chainlint/$$i.expect; \\\n+\t\tdone \\\n+\t} >'$(CHAINLINTTMP_SQ)'/expect && \\\n+\t$(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \\\n+\t\tgrep -v '^[ \t]*$$' >'$(CHAINLINTTMP_SQ)'/actual && \\\n+\tif test -f ../GIT-BUILD-OPTIONS; then \\\n+\t\t. ../GIT-BUILD-OPTIONS; \\\n+\tfi && \\\n+\tif test -x ../git$$X; then \\\n+\t\tDIFFW=\"../git$$X --no-pager diff -w --no-index\"; \\\n+\telse \\\n+\t\tDIFFW=\"diff -w -u\"; \\\n+\tfi && \\\n+\t$$DIFFW '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n \n test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \\\n \ttest-lint-filenames\ndiff --git a/t/chainlint/block.expect b/t/chainlint/block.expect\nindex da60257ebc4..37dbf7d95fa 100644\n--- a/t/chainlint/block.expect\n+++ b/t/chainlint/block.expect\n@@ -1,7 +1,7 @@\n (\n \tfoo &&\n \t{\n-\t\techo a\n+\t\techo a ?!AMP?!\n \t\techo b\n \t} &&\n \tbar &&\ndiff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect\nindex 2578191ca8a..be64b26869a 100644\n--- a/t/chainlint/here-doc-multi-line-string.expect\n+++ b/t/chainlint/here-doc-multi-line-string.expect\n@@ -1,4 +1,5 @@\n (\n-\tcat <<-TXT && echo \"multi-line\tstring\" ?!AMP?!\n+\tcat <<-TXT && echo \"multi-line\n+\tstring\" ?!AMP?!\n \tbap\n )\ndiff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect\nindex ab0dadf748e..27ff95218e7 100644\n--- a/t/chainlint/multi-line-string.expect\n+++ b/t/chainlint/multi-line-string.expect\n@@ -1,9 +1,14 @@\n (\n-\tx=\"line 1\t\tline 2\t\tline 3\" &&\n-\ty=\"line 1\t\tline2\" ?!AMP?!\n+\tx=\"line 1\n+\t\tline 2\n+\t\tline 3\" &&\n+\ty=\"line 1\n+\t\tline2\" ?!AMP?!\n \tfoobar\n ) &&\n (\n-\techo \"xyz\" \"abc\t\tdef\t\tghi\" &&\n+\techo \"xyz\" \"abc\n+\t\tdef\n+\t\tghi\" &&\n \tbarfoo\n )\ndiff --git a/t/chainlint/nested-subshell.expect b/t/chainlint/nested-subshell.expect\nindex 41a48adaa2b..02e0a9f1bb5 100644\n--- a/t/chainlint/nested-subshell.expect\n+++ b/t/chainlint/nested-subshell.expect\n@@ -6,7 +6,7 @@\n \t) >file &&\n \tcd foo &&\n \t(\n-\t\techo a\n+\t\techo a ?!AMP?!\n \t\techo b\n \t) >file\n )\ndiff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect\nindex 1cccc7bf7e1..69167da2f27 100644\n--- a/t/chainlint/t7900-subtree.expect\n+++ b/t/chainlint/t7900-subtree.expect\n@@ -1,10 +1,17 @@\n (\n-\tchks=\"sub1sub2sub3sub4\" &&\n+\tchks=\"sub1\n+sub2\n+sub3\n+sub4\" &&\n \tchks_sub=$(cat <\n\nThere are quite a few tests which print an error messages and then\nexplicitly signal failure with `false`, `return 1`, or `exit 1` as the\nfinal command in an `if` branch. In these cases, the tests don't bother\nmaintaining the &&-chain between `echo` and the explicit \"test failed\"\nindicator. Since such constructs are manually signaling failure, their\n&&-chain breakage is legitimate and safe -- both for the command\nimmediately preceding `false`, `return`, or `exit`, as well as for all\npreceding commands in the `if` branch. Therefore, stop flagging &&-chain\nbreakage in these sorts of cases.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 8 ++++++++\n t/chainlint/chain-break-false.expect | 9 +++++++++\n t/chainlint/chain-break-false.test | 10 ++++++++++\n t/chainlint/chain-break-return-exit.expect | 15 +++++++++++++++\n t/chainlint/chain-break-return-exit.test | 18 ++++++++++++++++++\n t/chainlint/if-in-loop.expect | 2 +-\n t/chainlint/if-in-loop.test | 2 +-\n 7 files changed, 62 insertions(+), 2 deletions(-)\n create mode 100644 t/chainlint/chain-break-false.expect\n create mode 100644 t/chainlint/chain-break-false.test\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 14e1db3519a..a76a09ecf5e 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -503,6 +503,14 @@ sub accumulate {\n \t\tgoto DONE if $token =~ /\\$\\?/;\n \t}\n \n+\t# if this command is \"false\", \"return 1\", or \"exit 1\" (which signal\n+\t# failure explicitly), then okay for all preceding commands to be\n+\t# missing \"&&\"\n+\tif ($$cmd[0] =~ /^(?:false|return|exit)$/) {\n+\t\t@$tokens = grep(!/^\\?!AMP\\?!$/, @$tokens);\n+\t\tgoto DONE;\n+\t}\n+\n \t# flag missing \"&&\" at end of previous command\n \tmy $n = find_non_nl($tokens);\n \tsplice(@$tokens, $n + 1, 0, '?!AMP?!') unless $n < 0;\ndiff --git a/t/chainlint/chain-break-false.expect b/t/chainlint/chain-break-false.expect\nnew file mode 100644\nindex 00000000000..989766fb856\n--- /dev/null\n+++ b/t/chainlint/chain-break-false.expect\n@@ -0,0 +1,9 @@\n+if condition not satisified\n+then\n+\techo it did not work...\n+\techo failed!\n+\tfalse\n+else\n+\techo it went okay ?!AMP?!\n+\tcongratulate user\n+fi\ndiff --git a/t/chainlint/chain-break-false.test b/t/chainlint/chain-break-false.test\nnew file mode 100644\nindex 00000000000..a5aaff8c8a4\n--- /dev/null\n+++ b/t/chainlint/chain-break-false.test\n@@ -0,0 +1,10 @@\n+# LINT: broken &&-chain okay if explicit \"false\" signals failure\n+if condition not satisified\n+then\n+\techo it did not work...\n+\techo failed!\n+\tfalse\n+else\n+\techo it went okay\n+\tcongratulate user\n+fi\ndiff --git a/t/chainlint/chain-break-return-exit.expect b/t/chainlint/chain-break-return-exit.expect\nindex dba292ee89b..1732d221c32 100644\n--- a/t/chainlint/chain-break-return-exit.expect\n+++ b/t/chainlint/chain-break-return-exit.expect\n@@ -1,3 +1,18 @@\n+case \"$(git ls-files)\" in\n+one ) echo pass one ;;\n+* ) echo bad one ; return 1 ;;\n+esac &&\n+(\n+\tcase \"$(git ls-files)\" in\n+\ttwo ) echo pass two ;;\n+\t* ) echo bad two ; exit 1 ;;\n+esac\n+) &&\n+case \"$(git ls-files)\" in\n+dir/two\"$LF\"one ) echo pass both ;;\n+* ) echo bad ; return 1 ;;\n+esac &&\n+\n for i in 1 2 3 4 ; do\n \tgit checkout main -b $i || return $?\n \ttest_commit $i $i $i tag$i || return $?\ndiff --git a/t/chainlint/chain-break-return-exit.test b/t/chainlint/chain-break-return-exit.test\nindex e2b059933aa..46542edf881 100644\n--- a/t/chainlint/chain-break-return-exit.test\n+++ b/t/chainlint/chain-break-return-exit.test\n@@ -1,3 +1,21 @@\n+case \"$(git ls-files)\" in\n+one) echo pass one ;;\n+# LINT: broken &&-chain okay if explicit \"return 1\" signals failuire\n+*) echo bad one; return 1 ;;\n+esac &&\n+(\n+\tcase \"$(git ls-files)\" in\n+\ttwo) echo pass two ;;\n+# LINT: broken &&-chain okay if explicit \"exit 1\" signals failuire\n+\t*) echo bad two; exit 1 ;;\n+\tesac\n+) &&\n+case \"$(git ls-files)\" in\n+dir/two\"$LF\"one) echo pass both ;;\n+# LINT: broken &&-chain okay if explicit \"return 1\" signals failuire\n+*) echo bad; return 1 ;;\n+esac &&\n+\n for i in 1 2 3 4 ; do\n # LINT: broken &&-chain okay if explicit \"return $?\" signals failure\n \tgit checkout main -b $i || return $?\ndiff --git a/t/chainlint/if-in-loop.expect b/t/chainlint/if-in-loop.expect\nindex 03b82a3e58c..d6514ae7492 100644\n--- a/t/chainlint/if-in-loop.expect\n+++ b/t/chainlint/if-in-loop.expect\n@@ -3,7 +3,7 @@\n \tdo\n \t\tif false\n \t\tthen\n-\t\t\techo \"err\" ?!AMP?!\n+\t\t\techo \"err\"\n \t\t\texit 1\n \t\tfi ?!AMP?!\n \t\tfoo\ndiff --git a/t/chainlint/if-in-loop.test b/t/chainlint/if-in-loop.test\nindex f0cf19cfada..90c23976fec 100644\n--- a/t/chainlint/if-in-loop.test\n+++ b/t/chainlint/if-in-loop.test\n@@ -3,7 +3,7 @@\n \tdo\n \t\tif false\n \t\tthen\n-# LINT: missing \"&&\" on \"echo\"\n+# LINT: missing \"&&\" on \"echo\" okay since \"exit 1\" signals error explicitly\n \t\t\techo \"err\"\n \t\t\texit 1\n # LINT: missing \"&&\" on \"fi\"\n-- \ngitgitgadget\n\n"},{"id":"462401","messageId":"7179972013825de50bddb6a68a17f0b83d5f35a8.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 13/18] chainlint.pl: allow `|| echo` to signal failure upstream of a pipe","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:51Z","receivedAt":"2022-09-01T00:30:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nThe use of `|| return` (or `|| exit`) to signal failure within a loop\nisn't effective when the loop is upstream of a pipe since the pipe\nswallows all upstream exit codes and returns only the exit code of the\nfinal command in the pipeline.\n\nTo work around this limitation, tests may adopt an alternative strategy\nof signaling failure by emitting text which would never be emitted in\nthe non-failing case. For instance:\n\n while condition\n do\n command1 &&\n command2 ||\n echo \"impossible text\"\n done |\n sort >actual &&\n\nSuch usage indicates deliberate thought about failure cases by the test\nauthor, thus flagging them as missing `|| return` (or `|| exit`) is not\nhelpful. Therefore, take this case into consideration when checking for\nexplicit loop termination.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 3 +++\n t/chainlint/loop-upstream-pipe.expect | 10 ++++++++++\n t/chainlint/loop-upstream-pipe.test | 11 +++++++++++\n 3 files changed, 24 insertions(+)\n create mode 100644 t/chainlint/loop-upstream-pipe.expect\n create mode 100644 t/chainlint/loop-upstream-pipe.test\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex 674b3ddf696..386999ce65d 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -487,6 +487,9 @@ sub parse_loop_body {\n \tmy @tokens = $self->SUPER::parse_loop_body(@_);\n \t# did loop signal failure via \"|| return\" or \"|| exit\"?\n \treturn @tokens if !@tokens || grep(/^(?:return|exit|\\$\\?)$/, @tokens);\n+\t# did loop upstream of a pipe signal failure via \"|| echo 'impossible\n+\t# text'\" as the final command in the loop body?\n+\treturn @tokens if ends_with(\\@tokens, [qr/^\\|\\|$/, \"\\n\", qr/^echo$/, qr/^.+$/]);\n \t# flag missing \"return/exit\" handling explicit failure in loop body\n \tmy $n = find_non_nl(\\@tokens);\n \tsplice(@tokens, $n + 1, 0, '?!LOOP?!');\ndiff --git a/t/chainlint/loop-upstream-pipe.expect b/t/chainlint/loop-upstream-pipe.expect\nnew file mode 100644\nindex 00000000000..0b82ecc4b96\n--- /dev/null\n+++ b/t/chainlint/loop-upstream-pipe.expect\n@@ -0,0 +1,10 @@\n+(\n+\tgit rev-list --objects --no-object-names base..loose |\n+\twhile read oid\n+\tdo\n+\t\tpath=\"$objdir/$(test_oid_to_path \"$oid\")\" &&\n+\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\" ||\n+\t\techo \"object list generation failed for $oid\"\n+\tdone |\n+\tsort -k1\n+) >expect &&\ndiff --git a/t/chainlint/loop-upstream-pipe.test b/t/chainlint/loop-upstream-pipe.test\nnew file mode 100644\nindex 00000000000..efb77da897c\n--- /dev/null\n+++ b/t/chainlint/loop-upstream-pipe.test\n@@ -0,0 +1,11 @@\n+(\n+\tgit rev-list --objects --no-object-names base..loose |\n+\twhile read oid\n+\tdo\n+# LINT: \"|| echo\" signals failure in loop upstream of a pipe\n+\t\tpath=\"$objdir/$(test_oid_to_path \"$oid\")\" &&\n+\t\tprintf \"%s %d\\n\" \"$oid\" \"$(test-tool chmtime --get \"$path\")\" ||\n+\t\techo \"object list generation failed for $oid\"\n+\tdone |\n+\tsort -k1\n+) >expect &&\n-- \ngitgitgadget\n\n"},{"id":"462402","messageId":"7814d6a51c47ace670db6e185f71e1640d53aab1.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 14/18] t/chainlint: add more chainlint.pl self-tests","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:52Z","receivedAt":"2022-09-01T00:30:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nDuring the development of chainlint.pl, numerous new self-tests were\ncreated to verify correct functioning beyond the checks already\nrepresented by the existing self-tests. The new checks fall into several\ncategories:\n\n* behavior of the lexical analyzer for complex cases, such as line\n splicing, token pasting, entering and exiting string contexts inside\n and outside of test script bodies; for instance:\n\n test_expect_success 'title' '\n x=$(echo \"something\" |\n sed -e '\\''s/\\\\/\\\\\\\\/g'\\'' -e '\\''s/[[/.*^$]/\\\\&/g'\\''\n '\n\n* behavior of the parser for all compound grammatical constructs, such\n as `if...fi`, `case...esac`, `while...done`, `{...}`, etc., and for\n other legal shell grammatical constructs not covered by existing\n chainlint.sed self-tests, as well as complex cases, such as:\n\n OUT=$( ((large_git 1>&3) | :) 3>&1 ) &&\n\n* detection of problems, such as &&-chain breakage, from top-level to\n any depth since the existing self-tests do not cover any top-level\n context and only cover subshells one level deep due to limitations of\n chainlint.sed\n\n* address blind spots in chainlint.sed (such as not detecting a broken\n &&-chain on a one-line for-loop in a subshell[1]) which chainlint.pl\n correctly detects\n\n* real-world cases which tripped up chainlint.pl during its development\n\n[1]: https://lore.kernel.org/git/dce35a47012fecc6edc11c68e91dbb485c5bc36f.1661663880.git.gitgitgadget@gmail.com/\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint/blank-line-before-esac.expect | 18 +++++++++++\n t/chainlint/blank-line-before-esac.test | 19 +++++++++++\n t/chainlint/block.expect | 13 +++++++-\n t/chainlint/block.test | 15 ++++++++-\n t/chainlint/chained-block.expect | 9 ++++++\n t/chainlint/chained-block.test | 11 +++++++\n t/chainlint/chained-subshell.expect | 10 ++++++\n t/chainlint/chained-subshell.test | 13 ++++++++\n .../command-substitution-subsubshell.expect | 2 ++\n .../command-substitution-subsubshell.test | 3 ++\n t/chainlint/double-here-doc.expect | 2 ++\n t/chainlint/double-here-doc.test | 12 +++++++\n t/chainlint/dqstring-line-splice.expect | 3 ++\n t/chainlint/dqstring-line-splice.test | 7 ++++\n t/chainlint/dqstring-no-interpolate.expect | 11 +++++++\n t/chainlint/dqstring-no-interpolate.test | 15 +++++++++\n t/chainlint/empty-here-doc.expect | 3 ++\n t/chainlint/empty-here-doc.test | 5 +++\n t/chainlint/exclamation.expect | 4 +++\n t/chainlint/exclamation.test | 8 +++++\n t/chainlint/for-loop-abbreviated.expect | 5 +++\n t/chainlint/for-loop-abbreviated.test | 6 ++++\n t/chainlint/function.expect | 11 +++++++\n t/chainlint/function.test | 13 ++++++++\n t/chainlint/here-doc-indent-operator.expect | 5 +++\n t/chainlint/here-doc-indent-operator.test | 13 ++++++++\n t/chainlint/if-condition-split.expect | 7 ++++\n t/chainlint/if-condition-split.test | 8 +++++\n t/chainlint/one-liner-for-loop.expect | 9 ++++++\n t/chainlint/one-liner-for-loop.test | 10 ++++++\n t/chainlint/sqstring-in-sqstring.expect | 4 +++\n t/chainlint/sqstring-in-sqstring.test | 5 +++\n t/chainlint/token-pasting.expect | 27 ++++++++++++++++\n t/chainlint/token-pasting.test | 32 +++++++++++++++++++\n 34 files changed, 336 insertions(+), 2 deletions(-)\n create mode 100644 t/chainlint/blank-line-before-esac.expect\n create mode 100644 t/chainlint/blank-line-before-esac.test\n create mode 100644 t/chainlint/chained-block.expect\n create mode 100644 t/chainlint/chained-block.test\n create mode 100644 t/chainlint/chained-subshell.expect\n create mode 100644 t/chainlint/chained-subshell.test\n create mode 100644 t/chainlint/command-substitution-subsubshell.expect\n create mode 100644 t/chainlint/command-substitution-subsubshell.test\n create mode 100644 t/chainlint/double-here-doc.expect\n create mode 100644 t/chainlint/double-here-doc.test\n create mode 100644 t/chainlint/dqstring-line-splice.expect\n create mode 100644 t/chainlint/dqstring-line-splice.test\n create mode 100644 t/chainlint/dqstring-no-interpolate.expect\n create mode 100644 t/chainlint/dqstring-no-interpolate.test\n create mode 100644 t/chainlint/empty-here-doc.expect\n create mode 100644 t/chainlint/empty-here-doc.test\n create mode 100644 t/chainlint/exclamation.expect\n create mode 100644 t/chainlint/exclamation.test\n create mode 100644 t/chainlint/for-loop-abbreviated.expect\n create mode 100644 t/chainlint/for-loop-abbreviated.test\n create mode 100644 t/chainlint/function.expect\n create mode 100644 t/chainlint/function.test\n create mode 100644 t/chainlint/here-doc-indent-operator.expect\n create mode 100644 t/chainlint/here-doc-indent-operator.test\n create mode 100644 t/chainlint/if-condition-split.expect\n create mode 100644 t/chainlint/if-condition-split.test\n create mode 100644 t/chainlint/one-liner-for-loop.expect\n create mode 100644 t/chainlint/one-liner-for-loop.test\n create mode 100644 t/chainlint/sqstring-in-sqstring.expect\n create mode 100644 t/chainlint/sqstring-in-sqstring.test\n create mode 100644 t/chainlint/token-pasting.expect\n create mode 100644 t/chainlint/token-pasting.test\n\ndiff --git a/t/chainlint/blank-line-before-esac.expect b/t/chainlint/blank-line-before-esac.expect\nnew file mode 100644\nindex 00000000000..48ed4eb1246\n--- /dev/null\n+++ b/t/chainlint/blank-line-before-esac.expect\n@@ -0,0 +1,18 @@\n+test_done ( ) {\n+\tcase \"$test_failure\" in\n+\t0 )\n+\t\ttest_at_end_hook_\n+\n+\t\texit 0 ;;\n+\n+\t* )\n+\t\tif test $test_external_has_tap -eq 0\n+\t\tthen\n+\t\t\tsay_color error \"# failed $test_failure among $msg\"\n+\t\t\tsay \"1..$test_count\"\n+\t\tfi\n+\n+\t\texit 1 ;;\n+\n+\t\tesac\n+}\ndiff --git a/t/chainlint/blank-line-before-esac.test b/t/chainlint/blank-line-before-esac.test\nnew file mode 100644\nindex 00000000000..cecccad19f5\n--- /dev/null\n+++ b/t/chainlint/blank-line-before-esac.test\n@@ -0,0 +1,19 @@\n+# LINT: blank line before \"esac\"\n+test_done () {\n+\tcase \"$test_failure\" in\n+\t0)\n+\t\ttest_at_end_hook_\n+\n+\t\texit 0 ;;\n+\n+\t*)\n+\t\tif test $test_external_has_tap -eq 0\n+\t\tthen\n+\t\t\tsay_color error \"# failed $test_failure among $msg\"\n+\t\t\tsay \"1..$test_count\"\n+\t\tfi\n+\n+\t\texit 1 ;;\n+\n+\tesac\n+}\ndiff --git a/t/chainlint/block.expect b/t/chainlint/block.expect\nindex 37dbf7d95fa..a3bcea492a9 100644\n--- a/t/chainlint/block.expect\n+++ b/t/chainlint/block.expect\n@@ -9,4 +9,15 @@\n \t\techo c\n \t} ?!AMP?!\n \tbaz\n-)\n+) &&\n+\n+{\n+\techo a ; ?!AMP?! echo b\n+} &&\n+{ echo a ; ?!AMP?! echo b ; } &&\n+\n+{\n+\techo \"${var}9\" &&\n+\techo \"done\"\n+} &&\n+finis\ndiff --git a/t/chainlint/block.test b/t/chainlint/block.test\nindex 0a82fd579f6..4ab69a4afc4 100644\n--- a/t/chainlint/block.test\n+++ b/t/chainlint/block.test\n@@ -11,4 +11,17 @@\n \t\techo c\n \t}\n \tbaz\n-)\n+) &&\n+\n+# LINT: \";\" not allowed in place of \"&&\"\n+{\n+\techo a; echo b\n+} &&\n+{ echo a; echo b; } &&\n+\n+# LINT: \"}\" inside string not mistaken as end of block\n+{\n+\techo \"${var}9\" &&\n+\techo \"done\"\n+} &&\n+finis\ndiff --git a/t/chainlint/chained-block.expect b/t/chainlint/chained-block.expect\nnew file mode 100644\nindex 00000000000..574cdceb071\n--- /dev/null\n+++ b/t/chainlint/chained-block.expect\n@@ -0,0 +1,9 @@\n+echo nobody home && {\n+\ttest the doohicky ?!AMP?!\n+\tright now\n+} &&\n+\n+GIT_EXTERNAL_DIFF=echo git diff | {\n+\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\ttest \"z$oh\" = \"z$oldhex\"\n+}\ndiff --git a/t/chainlint/chained-block.test b/t/chainlint/chained-block.test\nnew file mode 100644\nindex 00000000000..86f81ece639\n--- /dev/null\n+++ b/t/chainlint/chained-block.test\n@@ -0,0 +1,11 @@\n+# LINT: start of block chained to preceding command\n+echo nobody home && {\n+\ttest the doohicky\n+\tright now\n+} &&\n+\n+# LINT: preceding command pipes to block on same line\n+GIT_EXTERNAL_DIFF=echo git diff | {\n+\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\ttest \"z$oh\" = \"z$oldhex\"\n+}\ndiff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect\nnew file mode 100644\nindex 00000000000..af0369d3285\n--- /dev/null\n+++ b/t/chainlint/chained-subshell.expect\n@@ -0,0 +1,10 @@\n+mkdir sub && (\n+\tcd sub &&\n+\tfoo the bar ?!AMP?!\n+\tnuff said\n+) &&\n+\n+cut \"-d \" -f actual | ( read s1 s2 s3 &&\n+test -f $s1 ?!AMP?!\n+test $(cat $s2) = tree2path1 &&\n+test $(cat $s3) = tree3path1 )\ndiff --git a/t/chainlint/chained-subshell.test b/t/chainlint/chained-subshell.test\nnew file mode 100644\nindex 00000000000..4ff6ddd8cbd\n--- /dev/null\n+++ b/t/chainlint/chained-subshell.test\n@@ -0,0 +1,13 @@\n+# LINT: start of subshell chained to preceding command\n+mkdir sub && (\n+\tcd sub &&\n+\tfoo the bar\n+\tnuff said\n+) &&\n+\n+# LINT: preceding command pipes to subshell on same line\n+cut \"-d \" -f actual | (read s1 s2 s3 &&\n+test -f $s1\n+test $(cat $s2) = tree2path1 &&\n+# LINT: closing subshell \")\" correctly detected on same line as \"$(...)\"\n+test $(cat $s3) = tree3path1)\ndiff --git a/t/chainlint/command-substitution-subsubshell.expect b/t/chainlint/command-substitution-subsubshell.expect\nnew file mode 100644\nindex 00000000000..ab2f79e8457\n--- /dev/null\n+++ b/t/chainlint/command-substitution-subsubshell.expect\n@@ -0,0 +1,2 @@\n+OUT=$(( ( large_git 1 >& 3 ) | : ) 3 >& 1) &&\n+test_match_signal 13 \"$OUT\"\ndiff --git a/t/chainlint/command-substitution-subsubshell.test b/t/chainlint/command-substitution-subsubshell.test\nnew file mode 100644\nindex 00000000000..321de2951ce\n--- /dev/null\n+++ b/t/chainlint/command-substitution-subsubshell.test\n@@ -0,0 +1,3 @@\n+# LINT: subshell nested in subshell nested in command substitution\n+OUT=$( ((large_git 1>&3) | :) 3>&1 ) &&\n+test_match_signal 13 \"$OUT\"\ndiff --git a/t/chainlint/double-here-doc.expect b/t/chainlint/double-here-doc.expect\nnew file mode 100644\nindex 00000000000..75477bb1add\n--- /dev/null\n+++ b/t/chainlint/double-here-doc.expect\n@@ -0,0 +1,2 @@\n+run_sub_test_lib_test_err run-inv-range-start \"--run invalid range start\" --run=\"a-5\" <<-EOF &&\n+check_sub_test_lib_test_err run-inv-range-start <<-EOF_OUT 3 <<-EOF_ERR\ndiff --git a/t/chainlint/double-here-doc.test b/t/chainlint/double-here-doc.test\nnew file mode 100644\nindex 00000000000..cd584a43573\n--- /dev/null\n+++ b/t/chainlint/double-here-doc.test\n@@ -0,0 +1,12 @@\n+run_sub_test_lib_test_err run-inv-range-start \\\n+\t\"--run invalid range start\" \\\n+\t--run=\"a-5\" <<-\\EOF &&\n+test_expect_success \"passing test #1\" \"true\"\n+test_done\n+EOF\n+check_sub_test_lib_test_err run-inv-range-start \\\n+\t<<-\\EOF_OUT 3<<-EOF_ERR\n+> FATAL: Unexpected exit with code 1\n+EOF_OUT\n+> error: --run: invalid non-numeric in range start: ${SQ}a-5${SQ}\n+EOF_ERR\ndiff --git a/t/chainlint/dqstring-line-splice.expect b/t/chainlint/dqstring-line-splice.expect\nnew file mode 100644\nindex 00000000000..bf9ced60d4c\n--- /dev/null\n+++ b/t/chainlint/dqstring-line-splice.expect\n@@ -0,0 +1,3 @@\n+echo 'fatal: reword option of --fixup is mutually exclusive with' '--patch/--interactive/--all/--include/--only' > expect &&\n+test_must_fail git commit --fixup=reword:HEAD~ $1 2 > actual &&\n+test_cmp expect actual\ndiff --git a/t/chainlint/dqstring-line-splice.test b/t/chainlint/dqstring-line-splice.test\nnew file mode 100644\nindex 00000000000..b40714439f6\n--- /dev/null\n+++ b/t/chainlint/dqstring-line-splice.test\n@@ -0,0 +1,7 @@\n+# LINT: line-splice within DQ-string\n+'\"\n+echo 'fatal: reword option of --fixup is mutually exclusive with'\\\n+\t'--patch/--interactive/--all/--include/--only' >expect &&\n+test_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&\n+test_cmp expect actual\n+\"'\ndiff --git a/t/chainlint/dqstring-no-interpolate.expect b/t/chainlint/dqstring-no-interpolate.expect\nnew file mode 100644\nindex 00000000000..10724987a5f\n--- /dev/null\n+++ b/t/chainlint/dqstring-no-interpolate.expect\n@@ -0,0 +1,11 @@\n+grep \"^ ! [rejected][ ]*$BRANCH -> $BRANCH (non-fast-forward)$\" out &&\n+\n+grep \"^\\.git$\" output.txt &&\n+\n+\n+(\n+\tcd client$version &&\n+\tGIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. $(cat ../input)\n+) > output &&\n+\tcut -d ' ' -f 2 < output | sort > actual &&\n+\ttest_cmp expect actual\ndiff --git a/t/chainlint/dqstring-no-interpolate.test b/t/chainlint/dqstring-no-interpolate.test\nnew file mode 100644\nindex 00000000000..d2f4219cbbb\n--- /dev/null\n+++ b/t/chainlint/dqstring-no-interpolate.test\n@@ -0,0 +1,15 @@\n+# LINT: regex dollar-sign eol anchor in double-quoted string not special\n+grep \"^ ! \\[rejected\\][ ]*$BRANCH -> $BRANCH (non-fast-forward)$\" out &&\n+\n+# LINT: escaped \"$\" not mistaken for variable expansion\n+grep \"^\\\\.git\\$\" output.txt &&\n+\n+'\"\n+(\n+\tcd client$version &&\n+# LINT: escaped dollar-sign in double-quoted test body\n+\tGIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. \\$(cat ../input)\n+) >output &&\n+\tcut -d ' ' -f 2 actual &&\n+\ttest_cmp expect actual\n+\"'\ndiff --git a/t/chainlint/empty-here-doc.expect b/t/chainlint/empty-here-doc.expect\nnew file mode 100644\nindex 00000000000..f42f2d41ba8\n--- /dev/null\n+++ b/t/chainlint/empty-here-doc.expect\n@@ -0,0 +1,3 @@\n+git ls-tree $tree path > current &&\n+cat > expected <current &&\n+# LINT: empty here-doc\n+cat >expected <<\\EOF &&\n+EOF\n+test_output\ndiff --git a/t/chainlint/exclamation.expect b/t/chainlint/exclamation.expect\nnew file mode 100644\nindex 00000000000..2d961a58c66\n--- /dev/null\n+++ b/t/chainlint/exclamation.expect\n@@ -0,0 +1,4 @@\n+if ! condition ; then echo nope ; else yep ; fi &&\n+test_prerequisite !MINGW &&\n+mail uucp!address &&\n+echo !whatever!\ndiff --git a/t/chainlint/exclamation.test b/t/chainlint/exclamation.test\nnew file mode 100644\nindex 00000000000..323595b5bd8\n--- /dev/null\n+++ b/t/chainlint/exclamation.test\n@@ -0,0 +1,8 @@\n+# LINT: \"! word\" is two tokens\n+if ! condition; then echo nope; else yep; fi &&\n+# LINT: \"!word\" is single token, not two tokens \"!\" and \"word\"\n+test_prerequisite !MINGW &&\n+# LINT: \"word!word\" is single token, not three tokens \"word\", \"!\", and \"word\"\n+mail uucp!address &&\n+# LINT: \"!word!\" is single token, not three tokens \"!\", \"word\", and \"!\"\n+echo !whatever!\ndiff --git a/t/chainlint/for-loop-abbreviated.expect b/t/chainlint/for-loop-abbreviated.expect\nnew file mode 100644\nindex 00000000000..a21007a63f1\n--- /dev/null\n+++ b/t/chainlint/for-loop-abbreviated.expect\n@@ -0,0 +1,5 @@\n+for it\n+do\n+\tpath=$(expr \"$it\" : ( [^:]*) ) &&\n+\tgit update-index --add \"$path\" || exit\n+done\ndiff --git a/t/chainlint/for-loop-abbreviated.test b/t/chainlint/for-loop-abbreviated.test\nnew file mode 100644\nindex 00000000000..1084eccb89c\n--- /dev/null\n+++ b/t/chainlint/for-loop-abbreviated.test\n@@ -0,0 +1,6 @@\n+# LINT: for-loop lacking optional \"in [word...]\" before \"do\"\n+for it\n+do\n+\tpath=$(expr \"$it\" : '\\([^:]*\\)') &&\n+\tgit update-index --add \"$path\" || exit\n+done\ndiff --git a/t/chainlint/function.expect b/t/chainlint/function.expect\nnew file mode 100644\nindex 00000000000..a14388e6b9f\n--- /dev/null\n+++ b/t/chainlint/function.expect\n@@ -0,0 +1,11 @@\n+sha1_file ( ) {\n+\techo \"$*\" | sed \"s#..#.git/objects/&/#\"\n+} &&\n+\n+remove_object ( ) {\n+\tfile=$(sha1_file \"$*\") &&\n+\ttest -e \"$file\" ?!AMP?!\n+\trm -f \"$file\"\n+} ?!AMP?!\n+\n+sha1_file arg && remove_object arg\ndiff --git a/t/chainlint/function.test b/t/chainlint/function.test\nnew file mode 100644\nindex 00000000000..5ee59562c93\n--- /dev/null\n+++ b/t/chainlint/function.test\n@@ -0,0 +1,13 @@\n+# LINT: \"()\" in function definition not mistaken for subshell\n+sha1_file() {\n+\techo \"$*\" | sed \"s#..#.git/objects/&/#\"\n+} &&\n+\n+# LINT: broken &&-chain in function and after function\n+remove_object() {\n+\tfile=$(sha1_file \"$*\") &&\n+\ttest -e \"$file\"\n+\trm -f \"$file\"\n+}\n+\n+sha1_file arg && remove_object arg\ndiff --git a/t/chainlint/here-doc-indent-operator.expect b/t/chainlint/here-doc-indent-operator.expect\nnew file mode 100644\nindex 00000000000..fb6cf7285d0\n--- /dev/null\n+++ b/t/chainlint/here-doc-indent-operator.expect\n@@ -0,0 +1,5 @@\n+cat > expect <<-EOF &&\n+\n+cat > expect <<-EOF ?!AMP?!\n+\n+cleanup\ndiff --git a/t/chainlint/here-doc-indent-operator.test b/t/chainlint/here-doc-indent-operator.test\nnew file mode 100644\nindex 00000000000..c8a6f18eb45\n--- /dev/null\n+++ b/t/chainlint/here-doc-indent-operator.test\n@@ -0,0 +1,13 @@\n+# LINT: whitespace between operator \"<<-\" and tag legal\n+cat >expect <<- EOF &&\n+header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0\n+num_commits: $1\n+chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data\n+EOF\n+\n+# LINT: not an indented here-doc; just a plain here-doc with tag named \"-EOF\"\n+cat >expect << -EOF\n+this is not indented\n+-EOF\n+\n+cleanup\ndiff --git a/t/chainlint/if-condition-split.expect b/t/chainlint/if-condition-split.expect\nnew file mode 100644\nindex 00000000000..ee745ef8d7f\n--- /dev/null\n+++ b/t/chainlint/if-condition-split.expect\n@@ -0,0 +1,7 @@\n+if bob &&\n+ marcia ||\n+ kevin\n+then\n+\techo \"nomads\" ?!AMP?!\n+\techo \"for sure\"\n+fi\ndiff --git a/t/chainlint/if-condition-split.test b/t/chainlint/if-condition-split.test\nnew file mode 100644\nindex 00000000000..240daa9fd5d\n--- /dev/null\n+++ b/t/chainlint/if-condition-split.test\n@@ -0,0 +1,8 @@\n+# LINT: \"if\" condition split across multiple lines at \"&&\" or \"||\"\n+if bob &&\n+ marcia ||\n+ kevin\n+then\n+\techo \"nomads\"\n+\techo \"for sure\"\n+fi\ndiff --git a/t/chainlint/one-liner-for-loop.expect b/t/chainlint/one-liner-for-loop.expect\nnew file mode 100644\nindex 00000000000..51a3dc7c544\n--- /dev/null\n+++ b/t/chainlint/one-liner-for-loop.expect\n@@ -0,0 +1,9 @@\n+git init dir-rename-and-content &&\n+(\n+\tcd dir-rename-and-content &&\n+\ttest_write_lines 1 2 3 4 5 >foo &&\n+\tmkdir olddir &&\n+\tfor i in a b c; do echo $i >olddir/$i; ?!LOOP?! done ?!AMP?!\n+\tgit add foo olddir &&\n+\tgit commit -m \"original\" &&\n+)\ndiff --git a/t/chainlint/one-liner-for-loop.test b/t/chainlint/one-liner-for-loop.test\nnew file mode 100644\nindex 00000000000..4bd8c066c79\n--- /dev/null\n+++ b/t/chainlint/one-liner-for-loop.test\n@@ -0,0 +1,10 @@\n+git init dir-rename-and-content &&\n+(\n+\tcd dir-rename-and-content &&\n+\ttest_write_lines 1 2 3 4 5 >foo &&\n+\tmkdir olddir &&\n+# LINT: one-liner for-loop missing \"|| exit\"; also broken &&-chain\n+\tfor i in a b c; do echo $i >olddir/$i; done\n+\tgit add foo olddir &&\n+\tgit commit -m \"original\" &&\n+)\ndiff --git a/t/chainlint/sqstring-in-sqstring.expect b/t/chainlint/sqstring-in-sqstring.expect\nnew file mode 100644\nindex 00000000000..cf0b591cf7d\n--- /dev/null\n+++ b/t/chainlint/sqstring-in-sqstring.expect\n@@ -0,0 +1,4 @@\n+perl -e '\n+\tdefined($_ = -s $_) or die for @ARGV;\n+\texit 1 if $ARGV[0] <= $ARGV[1];\n+' test-2-$packname_2.pack test-3-$packname_3.pack\ndiff --git a/t/chainlint/sqstring-in-sqstring.test b/t/chainlint/sqstring-in-sqstring.test\nnew file mode 100644\nindex 00000000000..77a425e0c79\n--- /dev/null\n+++ b/t/chainlint/sqstring-in-sqstring.test\n@@ -0,0 +1,5 @@\n+# LINT: SQ-string Perl code fragment within SQ-string\n+perl -e '\\''\n+\tdefined($_ = -s $_) or die for @ARGV;\n+\texit 1 if $ARGV[0] <= $ARGV[1];\n+'\\'' test-2-$packname_2.pack test-3-$packname_3.pack\ndiff --git a/t/chainlint/token-pasting.expect b/t/chainlint/token-pasting.expect\nnew file mode 100644\nindex 00000000000..342360bcd05\n--- /dev/null\n+++ b/t/chainlint/token-pasting.expect\n@@ -0,0 +1,27 @@\n+git config filter.rot13.smudge ./rot13.sh &&\n+git config filter.rot13.clean ./rot13.sh &&\n+\n+{\n+ echo \"*.t filter=rot13\" ?!AMP?!\n+ echo \"*.i ident\"\n+} > .gitattributes &&\n+\n+{\n+ echo a b c d e f g h i j k l m ?!AMP?!\n+ echo n o p q r s t u v w x y z ?!AMP?!\n+ echo '$Id$'\n+} > test &&\n+cat test > test.t &&\n+cat test > test.o &&\n+cat test > test.i &&\n+git add test test.t test.i &&\n+rm -f test test.t test.i &&\n+git checkout -- test test.t test.i &&\n+\n+echo \"content-test2\" > test2.o &&\n+echo \"content-test3 - filename with special characters\" > \"test3 'sq',$x=.o\" ?!AMP?!\n+\n+downstream_url_for_sed=$(\n+\tprintf \"%sn\" \"$downstream_url\" |\n+\tsed -e 's/\\/\\\\/g' -e 's/[[/.*^$]/\\&/g'\n+)\ndiff --git a/t/chainlint/token-pasting.test b/t/chainlint/token-pasting.test\nnew file mode 100644\nindex 00000000000..b4610ce815a\n--- /dev/null\n+++ b/t/chainlint/token-pasting.test\n@@ -0,0 +1,32 @@\n+# LINT: single token; composite of multiple strings\n+git config filter.rot13.smudge ./rot13.sh &&\n+git config filter.rot13.clean ./rot13.sh &&\n+\n+{\n+ echo \"*.t filter=rot13\"\n+ echo \"*.i ident\"\n+} >.gitattributes &&\n+\n+{\n+ echo a b c d e f g h i j k l m\n+ echo n o p q r s t u v w x y z\n+# LINT: exit/enter string context and escaped-quote outside of string\n+ echo '\\''$Id$'\\''\n+} >test &&\n+cat test >test.t &&\n+cat test >test.o &&\n+cat test >test.i &&\n+git add test test.t test.i &&\n+rm -f test test.t test.i &&\n+git checkout -- test test.t test.i &&\n+\n+echo \"content-test2\" >test2.o &&\n+# LINT: exit/enter string context and escaped-quote outside of string\n+echo \"content-test3 - filename with special characters\" >\"test3 '\\''sq'\\'',\\$x=.o\"\n+\n+# LINT: single token; composite of multiple strings\n+downstream_url_for_sed=$(\n+\tprintf \"%s\\n\" \"$downstream_url\" |\n+# LINT: exit/enter string context; \"&\" inside string not command terminator\n+\tsed -e '\\''s/\\\\/\\\\\\\\/g'\\'' -e '\\''s/[[/.*^$]/\\\\&/g'\\''\n+)\n-- \ngitgitgadget\n\n"},{"id":"462403","messageId":"32992926ba2f3e8682f6dd91819ec66b82365c3b.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 15/18] test-lib: retire \"lint harder\" optimization hack","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:53Z","receivedAt":"2022-09-01T00:30:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\n`test_run_` in test-lib.sh \"lints\" the body of a test by sending it down\na `sed chainlint.sed | grep` pipeline; this happens once for each test\nrun by a test script. Although this pipeline may seem relatively cheap\nin isolation, it can become expensive when invoked 26800+ times by `make\ntest`, once for each test run, despite the existence of only 16500+ test\ndefinitions across all tests scripts.\n\nThis difference in the number of tests defined in the scripts (16500+)\nand the number of tests actually run by `make test` (26800+) is\nexplained by the fact that some test scripts run a very large number of\nsmall tests, all driven by a series of functions/loops which fill in the\ntest bodies. This means that certain test definitions are being linted\nrepeatedly (tens or hundreds of times) unnecessarily. To avoid such\nunnecessary work, 2d86a96220 (t: avoid sed-based chain-linting in some\nexpensive cases, 2021-05-13) added an optimization hack which allows\nindividual scripts to manually suppress the unnecessary repeated linting\nof the same test definition.\n\nHowever, unlike chainlint.sed which checks a test body as the test is\nrun, chainlint.pl checks each test definition just once, no matter how\nmany times the test is run, thus the sort of optimization hack\nintroduced by 2d86a96220 is no longer needed and can be retired.\nTherefore, revert 2d86a96220.\n\nSigned-off-by: Eric Sunshine \n---\n t/README | 5 -----\n t/t0027-auto-crlf.sh | 7 +------\n t/t3070-wildmatch.sh | 5 -----\n t/test-lib.sh | 7 ++-----\n 4 files changed, 3 insertions(+), 21 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 2f439f96589..979b2d4833d 100644\n--- a/t/README\n+++ b/t/README\n@@ -196,11 +196,6 @@ appropriately before running \"make\". Short options can be bundled, i.e.\n \tthis feature by setting the GIT_TEST_CHAIN_LINT environment\n \tvariable to \"1\" or \"0\", respectively.\n \n-\tA few test scripts disable some of the more advanced\n-\tchain-linting detection in the name of efficiency. You can\n-\toverride this by setting the GIT_TEST_CHAIN_LINT_HARDER\n-\tenvironment variable to \"1\".\n-\n --stress::\n \tRun the test script repeatedly in multiple parallel jobs until\n \tone of them fails. Useful for reproducing rare failures in\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex a22e0e1382c..a94ac1eae37 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -387,9 +387,7 @@ test_expect_success 'setup main' '\n \ttest_tick\n '\n \n-# Disable extra chain-linting for the next set of tests. There are many\n-# auto-generated ones that are not worth checking over and over.\n-GIT_TEST_CHAIN_LINT_HARDER_DEFAULT=0\n+\n \n warn_LF_CRLF=\"LF will be replaced by CRLF\"\n warn_CRLF_LF=\"CRLF will be replaced by LF\"\n@@ -606,9 +604,6 @@ do\n \tcheckout_files \"\" \"$id\" \"crlf\" true \"\" CRLF CRLF CRLF CRLF_mix_CR CRLF_nul\n done\n \n-# The rest of the tests are unique; do the usual linting.\n-unset GIT_TEST_CHAIN_LINT_HARDER_DEFAULT\n-\n # Should be the last test case: remove some files from the worktree\n test_expect_success 'ls-files --eol -d -z' '\n \trm crlf_false_attr__CRLF.txt crlf_false_attr__CRLF_mix_LF.txt crlf_false_attr__LF.txt .gitattributes &&\ndiff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh\nindex f9539968e4c..5d871fde960 100755\n--- a/t/t3070-wildmatch.sh\n+++ b/t/t3070-wildmatch.sh\n@@ -5,11 +5,6 @@ test_description='wildmatch tests'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-# Disable expensive chain-lint tests; all of the tests in this script\n-# are variants of a few trivial test-tool invocations, and there are a lot of\n-# them.\n-GIT_TEST_CHAIN_LINT_HARDER_DEFAULT=0\n-\n should_create_test_file() {\n \tfile=$1\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 377cc1c1203..dc0d0591095 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1091,11 +1091,8 @@ test_run_ () {\n \t\ttrace=\n \t\t# 117 is magic because it is unlikely to match the exit\n \t\t# code of other programs\n-\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\" ||\n-\t\t {\n-\t\t\ttest \"${GIT_TEST_CHAIN_LINT_HARDER:-${GIT_TEST_CHAIN_LINT_HARDER_DEFAULT:-1}}\" != 0 &&\n-\t\t\t$(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!')\n-\t\t }\n+\t\tif $(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!') ||\n+\t\t\ttest \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n \t\tthen\n \t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n \t\tfi\n-- \ngitgitgadget\n\n"},{"id":"462404","messageId":"9589f2a6e495034cc4f45bd0bce80dedfcd30f16.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 16/18] test-lib: replace chainlint.sed with chainlint.pl","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:54Z","receivedAt":"2022-09-01T00:30:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nBy automatically invoking chainlint.sed upon each test it runs,\n`test_run_` in test-lib.sh ensures that broken &&-chains will be\ndetected early as tests are modified or new are tests created since it\nis typical to run a test script manually (i.e. `./t1234-test-script.sh`)\nduring test development. Now that the implementation of chainlint.pl is\ncomplete, modify test-lib.sh to invoke it automatically instead of\nchainlint.sed each time a test script is run.\n\nThis change reduces the number of \"linter\" invocations from 26800+ (once\nper test run) down to 1050+ (once per test script), however, a\nsubsequent change will drop the number of invocations to 1 per `make\ntest`, thus fully realizing the benefit of the new linter.\n\nNote that the \"magic exit code 117\" &&-chain checker added by bb79af9d09\n(t/test-lib: introduce --chain-lint option, 2015-03-20) which is built\ninto t/test-lib.sh is retained since it has near zero-cost and\n(theoretically) may catch a broken &&-chain not caught by chainlint.pl.\n\nSigned-off-by: Eric Sunshine \n---\n contrib/buildsystems/CMakeLists.txt | 2 +-\n t/test-lib.sh | 9 +++++++--\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 2237109b57f..ca358a21a5f 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1076,7 +1076,7 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n \t\t\"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n \t\t\"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n \t#misc copies\n-\tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n+\tfile(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.pl DESTINATION ${CMAKE_BINARY_DIR}/t/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n \tfile(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex dc0d0591095..a65df2fd220 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1091,8 +1091,7 @@ test_run_ () {\n \t\ttrace=\n \t\t# 117 is magic because it is unlikely to match the exit\n \t\t# code of other programs\n-\t\tif $(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!') ||\n-\t\t\ttest \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n+\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n \t\tthen\n \t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n \t\tfi\n@@ -1588,6 +1587,12 @@ then\n \tBAIL_OUT_ENV_NEEDS_SANITIZE_LEAK \"GIT_TEST_SANITIZE_LEAK_LOG=true\"\n fi\n \n+if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0\n+then\n+\t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n+\t\tBUG \"lint error (see '?!...!? annotations above)\"\n+fi\n+\n # Last-minute variable setup\n USER_HOME=\"$HOME\"\n HOME=\"$TRASH_DIRECTORY\"\n-- \ngitgitgadget\n\n"},{"id":"462405","messageId":"f5dbcbf78db127d738c11a1aca416201298426cf.1661992197.git.gitgitgadget@gmail.com","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"[PATCH 18/18] t: retire unused chainlint.sed","fromName":"Eric Sunshine via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T00:29:56Z","receivedAt":"2022-09-01T00:31:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"From: Eric Sunshine \n\nRetire chainlint.sed since it has been replaced by a more accurate and\nfunctional &&-chain \"linter\", thus is no longer used.\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.sed | 399 ------------------------------------------------\n 1 file changed, 399 deletions(-)\n delete mode 100644 t/chainlint.sed\n\ndiff --git a/t/chainlint.sed b/t/chainlint.sed\ndeleted file mode 100644\nindex dc4ce37cb51..00000000000\n--- a/t/chainlint.sed\n+++ /dev/null\n@@ -1,399 +0,0 @@\n-#------------------------------------------------------------------------------\n-# Detect broken &&-chains in tests.\n-#\n-# At present, only &&-chains in subshells are examined by this linter;\n-# top-level &&-chains are instead checked directly by the test framework. Like\n-# the top-level &&-chain linter, the subshell linter (intentionally) does not\n-# check &&-chains within {...} blocks.\n-#\n-# Checking for &&-chain breakage is done line-by-line by pure textual\n-# inspection.\n-#\n-# Incomplete lines (those ending with \"\\\") are stitched together with following\n-# lines to simplify processing, particularly of \"one-liner\" statements.\n-# Top-level here-docs are swallowed to avoid false positives within the\n-# here-doc body, although the statement to which the here-doc is attached is\n-# retained.\n-#\n-# Heuristics are used to detect end-of-subshell when the closing \")\" is cuddled\n-# with the final subshell statement on the same line:\n-#\n-# (cd foo &&\n-# bar)\n-#\n-# in order to avoid misinterpreting the \")\" in constructs such as \"x=$(...)\"\n-# and \"case $x in *)\" as ending the subshell.\n-#\n-# Lines missing a final \"&&\" are flagged with \"?!AMP?!\", as are lines which\n-# chain commands with \";\" internally rather than \"&&\". A line may be flagged\n-# for both violations.\n-#\n-# Detection of a missing &&-link in a multi-line subshell is complicated by the\n-# fact that the last statement before the closing \")\" must not end with \"&&\".\n-# Since processing is line-by-line, it is not known whether a missing \"&&\" is\n-# legitimate or not until the _next_ line is seen. To accommodate this, within\n-# multi-line subshells, each line is stored in sed's \"hold\" area until after\n-# the next line is seen and processed. If the next line is a stand-alone \")\",\n-# then a missing \"&&\" on the previous line is legitimate; otherwise a missing\n-# \"&&\" is a break in the &&-chain.\n-#\n-# (\n-# cd foo &&\n-# bar\n-# )\n-#\n-# In practical terms, when \"bar\" is encountered, it is flagged with \"?!AMP?!\",\n-# but when the stand-alone \")\" line is seen which closes the subshell, the\n-# \"?!AMP?!\" violation is removed from the \"bar\" line (retrieved from the \"hold\"\n-# area) since the final statement of a subshell must not end with \"&&\". The\n-# final line of a subshell may still break the &&-chain by using \";\" internally\n-# to chain commands together rather than \"&&\", but an internal \"?!AMP?!\" is\n-# never removed from a line even though a line-ending \"?!AMP?!\" might be.\n-#\n-# Care is taken to recognize the last _statement_ of a multi-line subshell, not\n-# necessarily the last textual _line_ within the subshell, since &&-chaining\n-# applies to statements, not to lines. Consequently, blank lines, comment\n-# lines, and here-docs are swallowed (but not the command to which the here-doc\n-# is attached), leaving the last statement in the \"hold\" area, not the last\n-# line, thus simplifying &&-link checking.\n-#\n-# The final statement before \"done\" in for- and while-loops, and before \"elif\",\n-# \"else\", and \"fi\" in if-then-else likewise must not end with \"&&\", thus\n-# receives similar treatment.\n-#\n-# Swallowing here-docs with arbitrary tags requires a bit of finesse. When a\n-# line such as \"cat <cat <\\n\\1$/ is attempted to see if\n-# the content inside \"<...>\" matches the entirety of the newly-read line. For\n-# instance, if the next line read is \"some data\", when concatenated with the\n-# target line, it becomes \"cat <cat <\" does match the text following the\n-# newline, thus the closing here-doc tag has been found. The closing tag line\n-# and the \"<...>\" prefix on the target line are then discarded, leaving just\n-# the target line \"cat <\\1\\2/\n-\t:hered\n-\tN\n-\t/^<\\([^>]*\\)>.*\\n[ \t]*\\1[ \t]*$/!{\n-\t\ts/\\n.*$//\n-\t\tbhered\n-\t}\n-\ts/^<[^>]*>//\n-\ts/\\n.*$//\n-}\n-:notdoc\n-\n-# one-liner \"(...) &&\"\n-/^[ \t]*!*[ \t]*(..*)[ \t]*&&[ \t]*$/boneline\n-\n-# same as above but without trailing \"&&\"\n-/^[ \t]*!*[ \t]*(..*)[ \t]*$/boneline\n-\n-# one-liner \"(...) >x\" (or \"2>x\" or \"|&]/boneline\n-\n-# multi-line \"(...\\n...)\"\n-/^[ \t]*(/bsubsh\n-\n-# innocuous line -- print it and advance to next line\n-b\n-\n-# found one-liner \"(...)\" -- mark suspect if it uses \";\" internally rather than\n-# \"&&\" (but not \";\" in a string)\n-:oneline\n-/;/{\n-\t/\"[^\"]*;[^\"]*\"/!s/;/; ?!AMP?!/\n-}\n-b\n-\n-:subsh\n-# bare \"(\" line? -- stash for later printing\n-/^[ \t]*([\t]*$/ {\n-\th\n-\tbnextln\n-}\n-# \"(...\" line -- \"(\" opening subshell cuddled with command; temporarily replace\n-# \"(\" with sentinel \"^\" and process the line as if \"(\" had been seen solo on\n-# the preceding line; this temporary replacement prevents several rules from\n-# accidentally thinking \"(\" introduces a nested subshell; \"^\" is changed back\n-# to \"(\" at output time\n-x\n-s/.*//\n-x\n-s/(/^/\n-bslurp\n-\n-:nextln\n-N\n-s/.*\\n//\n-\n-:slurp\n-# incomplete line \"...\\\"\n-/\\\\$/bicmplte\n-# multi-line quoted string \"...\\n...\"?\n-/\"/bdqstr\n-# multi-line quoted string '...\\n...'? (but not contraction in string \"it's\")\n-/'/{\n-\t/\"[^'\"]*'[^'\"]*\"/!bsqstr\n-}\n-:folded\n-# here-doc -- swallow it (but not \"<<\" in a string)\n-/<<-*[ \t]*[\\\\'\"]*[A-Za-z0-9_]/{\n-\t/\"[^\"]*<<[^\"]*\"/!bheredoc\n-}\n-# comment or empty line -- discard since final non-comment, non-empty line\n-# before closing \")\", \"done\", \"elsif\", \"else\", or \"fi\" will need to be\n-# re-visited to drop \"suspect\" marking since final line of those constructs\n-# legitimately lacks \"&&\", so \"suspect\" mark must be removed\n-/^[ \t]*#/bnextln\n-/^[ \t]*$/bnextln\n-# in-line comment -- strip it (but not \"#\" in a string, Bash ${#...} array\n-# length, or Perforce \"//depot/path#42\" revision in filespec)\n-/[ \t]#/{\n-\t/\"[^\"]*#[^\"]*\"/!s/[ \t]#.*$//\n-}\n-# one-liner \"case ... esac\"\n-/^[ \t^]*case[ \t]*..*esac/bchkchn\n-# multi-line \"case ... esac\"\n-/^[ \t^]*case[ \t]..*[ \t]in/bcase\n-# multi-line \"for ... done\" or \"while ... done\"\n-/^[ \t^]*for[ \t]..*[ \t]in/bcont\n-/^[ \t^]*while[ \t]/bcont\n-/^[ \t]*do[ \t]/bcont\n-/^[ \t]*do[ \t]*$/bcont\n-/;[ \t]*do/bcont\n-/^[ \t]*done[ \t]*&&[ \t]*$/bdone\n-/^[ \t]*done[ \t]*$/bdone\n-/^[ \t]*done[ \t]*[<>|]/bdone\n-/^[ \t]*done[ \t]*)/bdone\n-/||[ \t]*exit[ \t]/bcont\n-/||[ \t]*exit[ \t]*$/bcont\n-# multi-line \"if...elsif...else...fi\"\n-/^[ \t^]*if[ \t]/bcont\n-/^[ \t]*then[ \t]/bcont\n-/^[ \t]*then[ \t]*$/bcont\n-/;[ \t]*then/bcont\n-/^[ \t]*elif[ \t]/belse\n-/^[ \t]*elif[ \t]*$/belse\n-/^[ \t]*else[ \t]/belse\n-/^[ \t]*else[ \t]*$/belse\n-/^[ \t]*fi[ \t]*&&[ \t]*$/bdone\n-/^[ \t]*fi[ \t]*$/bdone\n-/^[ \t]*fi[ \t]*[<>|]/bdone\n-/^[ \t]*fi[ \t]*)/bdone\n-# nested one-liner \"(...) &&\"\n-/^[ \t^]*(.*)[ \t]*&&[ \t]*$/bchkchn\n-# nested one-liner \"(...)\"\n-/^[ \t^]*(.*)[ \t]*$/bchkchn\n-# nested one-liner \"(...) >x\" (or \"2>x\" or \"|]/bchkchn\n-# nested multi-line \"(...\\n...)\"\n-/^[ \t^]*(/bnest\n-# multi-line \"{...\\n...}\"\n-/^[ \t^]*{/bblock\n-# closing \")\" on own line -- exit subshell\n-/^[ \t]*)/bclssolo\n-# \"$((...))\" -- arithmetic expansion; not closing \")\"\n-/\\$(([^)][^)]*))[^)]*$/bchkchn\n-# \"$(...)\" -- command substitution; not closing \")\"\n-/\\$([^)][^)]*)[^)]*$/bchkchn\n-# multi-line \"$(...\\n...)\" -- command substitution; treat as nested subshell\n-/\\$([^)]*$/bnest\n-# \"=(...)\" -- Bash array assignment; not closing \")\"\n-/=(/bchkchn\n-# closing \"...) &&\"\n-/)[ \t]*&&[ \t]*$/bclose\n-# closing \"...)\"\n-/)[ \t]*$/bclose\n-# closing \"...) >x\" (or \"2>x\" or \"|]/bclose\n-:chkchn\n-# mark suspect if line uses \";\" internally rather than \"&&\" (but not \";\" in a\n-# string and not \";;\" in one-liner \"case...esac\")\n-/;/{\n-\t/;;/!{\n-\t\t/\"[^\"]*;[^\"]*\"/!s/;/; ?!AMP?!/\n-\t}\n-}\n-# line ends with pipe \"...|\" -- valid; not missing \"&&\"\n-/|[ \t]*$/bcont\n-# missing end-of-line \"&&\" -- mark suspect\n-/&&[ \t]*$/!s/$/ ?!AMP?!/\n-:cont\n-# retrieve and print previous line\n-x\n-s/^\\([ \t]*\\)^/\\1(/\n-s/?!HERE?!/<\\1?!HERE?!\\2\\3/\n-:hdocsub\n-N\n-/^<\\([^>]*\\)>.*\\n[ \t]*\\1[ \t]*$/!{\n-\ts/\\n.*$//\n-\tbhdocsub\n-}\n-s/^<[^>]*>//\n-s/\\n.*$//\n-bfolded\n-\n-# found \"case ... in\" -- pass through untouched\n-:case\n-x\n-s/^\\([ \t]*\\)^/\\1(/\n-s/?!HERE?!/<\n\nUnlike chainlint.sed which \"lints\" a single test body at a time, thus is\ninvoked once per test, chainlint.pl can check all test bodies in all\ntest scripts with a single invocation. As such, it is akin to other bulk\n\"linters\" run by the Makefile, such as `test-lint-shell-syntax`,\n`test-lint-duplicates`, etc.\n\nTherefore, teach `make test` and `make prove` to invoke chainlint.pl\nalong with the other bulk linters. Also, since the single chainlint.pl\ninvocation by `make test` or `make prove` has already checked all tests\nin all scripts, instruct the individual test scripts not to run\nchainlint.pl on themselves unnecessarily.\n\nSigned-off-by: Eric Sunshine \n---\n t/Makefile | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 11f276774ea..3db48c0cb64 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -36,14 +36,21 @@ CHAINLINTTMP_SQ = $(subst ','\\'',$(CHAINLINTTMP))\n \n T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))\n+TLIBS = $(sort $(wildcard lib-*.sh)) annotate-tests.sh\n TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n+TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))\n CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n \n+# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n+# checks all tests in all scripts via a single invocation, so tell individual\n+# scripts not to \"chainlint\" themselves\n+CHAINLINTSUPPRESS = GIT_TEST_CHAIN_LINT=0 && export GIT_TEST_CHAIN_LINT &&\n+\n all: $(DEFAULT_TEST_TARGET)\n \n test: pre-clean check-chainlint $(TEST_LINT)\n-\t$(MAKE) aggregate-results-and-cleanup\n+\t$(CHAINLINTSUPPRESS) $(MAKE) aggregate-results-and-cleanup\n \n failed:\n \t@failed=$$(cd '$(TEST_RESULTS_DIRECTORY_SQ)' && \\\n@@ -52,7 +59,7 @@ failed:\n \ttest -z \"$$failed\" || $(MAKE) $$failed\n \n prove: pre-clean check-chainlint $(TEST_LINT)\n-\t@echo \"*** prove ***\"; $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)\n+\t@echo \"*** prove ***\"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)\n \t$(MAKE) clean-except-prove-cache\n \n $(T):\n@@ -99,6 +106,9 @@ check-chainlint:\n \n test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \\\n \ttest-lint-filenames\n+ifneq ($(GIT_TEST_CHAIN_LINT),0)\n+test-lint: test-chainlint\n+endif\n \n test-lint-duplicates:\n \t@dups=`echo $(T) $(TPERF) | tr ' ' '\\n' | sed 's/-.*//' | sort | uniq -d` && \\\n@@ -121,6 +131,9 @@ test-lint-filenames:\n \t\ttest -z \"$$bad\" || { \\\n \t\techo >&2 \"non-portable file name(s): $$bad\"; exit 1; }\n \n+test-chainlint:\n+\t@$(CHAINLINT) $(T) $(TLIBS) $(TPERF) $(TINTEROP)\n+\n aggregate-results-and-cleanup: $(T)\n \t$(MAKE) aggregate-results\n \t$(MAKE) clean\n@@ -136,4 +149,5 @@ valgrind:\n perf:\n \t$(MAKE) -C perf/ all\n \n-.PHONY: pre-clean $(T) aggregate-results clean valgrind perf check-chainlint clean-chainlint\n+.PHONY: pre-clean $(T) aggregate-results clean valgrind perf \\\n+\tcheck-chainlint clean-chainlint test-chainlint\n-- \ngitgitgadget\n\n"},{"id":"462437","messageId":"220901.86k06njmvq.gmgdl@evledraar.gmail.com","threadId":"58385","inReplyTo":"3423df94bd6035640828a2508968cf8e1f5b4dda.1661992197.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 01/18] t: add skeleton chainlint.pl","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-01T12:27:47Z","receivedAt":"2022-09-01T12:32:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 01 2022, Eric Sunshine via GitGitGadget wrote:\n\n> From: Eric Sunshine \n> [...]\n> diff --git a/t/chainlint.pl b/t/chainlint.pl\n\nI really like this overall direction...\n\n> +use warnings;\n> +use strict;\n\nI think that in general we're way overdue for at least a :\n\n\tuse v5.10.1;\n\nOr even something more aggresive, I think we can definitely depend on a\nnewer version for this bit of dev tooling.\n\nThat makes a lot of things in this series more pleasing to look\nat. E.g. you could use named $+{} variables for regexes.\n\n> +package ScriptParser;\n\nI really wish this could be changed to just put this in\nt/chainlint/ScriptParser.pm early on, we could set @INC appropriately\nand \"use\" these, which...\n\n> +my $getnow = sub { return time(); };\n> +my $interval = sub { return time() - shift; };\n\nWould eliminate any scoping concerns about this sort of thing.\n\n> +if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {\n> +\t$getnow = sub { return [Time::HiRes::gettimeofday()]; };\n> +\t$interval = sub { return Time::HiRes::tv_interval(shift); };\n> +}\n\nIs this \"require\" even needed, Time::HiRes is there since 5.7.* says\n\"corelist -l Time::HIRes\".\n\n> [...]\n> +sub check_script {\n> +\tmy ($id, $next_script, $emit) = @_;\n> +\tmy ($nscripts, $ntests, $nerrs) = (0, 0, 0);\n> +\twhile (my $path = $next_script->()) {\n> +\t\t$nscripts++;\n> +\t\tmy $fh;\n> +\t\tunless (open($fh, \"<\", $path)) {\n> +\t\t\t$emit->(\"?!ERR?! $path: $!\\n\");\n\nIf we can depend on v5.10.1 this can surely become:\n\n\tuse autodie qw(open close);\n\nNo?\n\n> +\t\t\t$nerrs += () = $s =~ /\\?![^?]+\\?!/g;\n\ny'know if we add some whitespace there we can conform to\nhttps://metacpan.org/dist/perlsecret/view/lib/perlsecret.pod >:) (not\nserious...)\n"},{"id":"462438","messageId":"220901.86fshbjmqj.gmgdl@evledraar.gmail.com","threadId":"58385","inReplyTo":"c1042b9bcd94b9ecb0bf73dfbd4334b9f30ba99a.1661992197.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 02/18] chainlint.pl: add POSIX shell lexical analyzer","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-01T12:32:49Z","receivedAt":"2022-09-01T12:35:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 01 2022, Eric Sunshine via GitGitGadget wrote:\n\n> From: Eric Sunshine \n\nJust generally on this series:\n\n> +\t$tag =~ s/['\"\\\\]//g;\n\nI think this would be a *lot* easier to read if all of these little\nregex decls could be split out into some \"grammar\" class, or other\nhelper module/namespace. So e.g.:\n\n\tmy $SCRIPT_QUOTE_RX = qr/['\"\\\\]/;\n\nThen:\n\n> +\treturn $cc if $cc =~ /^(?:&&|\\|\\||>>|;;|<&|>&|<>|>\\|)$/;\n\n\tmy $SCRIPT_WHATEVER_RX = qr/\n\t\t^(?:\n\t\t&&\n\t\t|\n\t\t\\|\\|\n\t\t[...]\n\t/x;\n\netc., i.e. we could then make use of /x to add inline comments to these.\n"},{"id":"462439","messageId":"220901.86bkrzjm6e.gmgdl@evledraar.gmail.com","threadId":"58385","inReplyTo":"62fc652eb47a4df83d88a197e376f28dbbab3b52.1661992197.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-01T12:36:57Z","receivedAt":"2022-09-01T12:48:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 01 2022, Eric Sunshine via GitGitGadget wrote:\n\n> From: Eric Sunshine \n>\n> Although chainlint.pl has undergone a good deal of optimization during\n> its development -- increasing in speed significantly -- parsing and\n> validating 1050+ scripts and 16500+ tests via Perl is not exactly\n> instantaneous. However, perceived performance can be improved by taking\n> advantage of the fact that there is no interdependence between test\n> scripts or test definitions, thus parsing and validating can be done in\n> parallel. The number of available cores is determined automatically but\n> can be overridden via the --jobs option.\n\nPer your CL:\n\t\n\tÆvar offered some sensible comments[2,3] about optimizing the Makefile rules\n\trelated to chainlint, but those optimizations are not tackled here for a few\n\treasons: (1) this series is already quite long, (2) I'd like to keep the\n\tseries focused on its primary goal of installing a new and improved linter,\n\t(3) these patches do not make the Makefile situation any worse[4], and (4)\n\tthose optimizations can easily be done atop this series[5].\n\nI have been running with those t/Makefile changesg locally, but didn't\nsubmit them. FWIW that's here:\n\n\thttps://github.com/git/git/compare/master...avar:git:avar/t-Makefile-use-dependency-graph-for-check-chainlint\n\nWhich I'm not entirely sure I'm happy about, and it's jeust about the\nchainlint tests, but...\n\n> +sub ncores {\n> +\t# Windows\n> +\treturn $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});\n> +\t# Linux / MSYS2 / Cygwin / WSL\n> +\tdo { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor\\s*:/, <>)); } if -r '/proc/cpuinfo';\n> +\t# macOS & BSD\n> +\treturn qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;\n> +\treturn 1;\n> +}\n> +\n> sub show_stats {\n> \tmy ($start_time, $stats) = @_;\n> \tmy $walltime = $interval->($start_time);\n> @@ -621,7 +633,9 @@ sub exit_code {\n> Getopt::Long::Configure(qw{bundling});\n> GetOptions(\n> \t\"emit-all!\" => \\$emit_all,\n> +\t\"jobs|j=i\" => \\$jobs,\n> \t\"stats|show-stats!\" => \\$show_stats) or die(\"option error\\n\");\n> +$jobs = ncores() if $jobs < 1;\n> \n> my $start_time = $getnow->();\n> my @stats;\n> @@ -633,6 +647,40 @@ unless (@scripts) {\n> \texit;\n> }\n> \n> -push(@stats, check_script(1, sub { shift(@scripts); }, sub { print(@_); }));\n> +unless ($Config{useithreads} && eval {\n> +\trequire threads; threads->import();\n> +\trequire Thread::Queue; Thread::Queue->import();\n> +\t1;\n> +\t}) {\n> +\tpush(@stats, check_script(1, sub { shift(@scripts); }, sub { print(@_); }));\n> +\tshow_stats($start_time, \\@stats) if $show_stats;\n> +\texit(exit_code(\\@stats));\n> +}\n> +\n> +my $script_queue = Thread::Queue->new();\n> +my $output_queue = Thread::Queue->new();\n> +\n> +sub next_script { return $script_queue->dequeue(); }\n> +sub emit { $output_queue->enqueue(@_); }\n> +\n> +sub monitor {\n> +\twhile (my $s = $output_queue->dequeue()) {\n> +\t\tprint($s);\n> +\t}\n> +}\n> +\n> +my $mon = threads->create({'context' => 'void'}, \\&monitor);\n> +threads->create({'context' => 'list'}, \\&check_script, $_, \\&next_script, \\&emit) for 1..$jobs;\n> +\n> +$script_queue->enqueue(@scripts);\n> +$script_queue->end();\n> +\n> +for (threads->list()) {\n> +\tpush(@stats, $_->join()) unless $_ == $mon;\n> +}\n> +\n> +$output_queue->end();\n> +$mon->join();\n\nMaybe I'm misunderstanding this whole thing, but this really seems like\nthe wrong direction in an otherwise fantastic direction of a series.\n\nI.e. it's *great* that we can do chain-lint without needing to actually\nexecute the *.sh file, this series adds a lint parser that can parse\nthose *.sh \"at rest\".\n\nBut in your 16/18 you then do:\n\t\n\t+if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0\n\t+then\n\t+\t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n\t+\t\tBUG \"lint error (see '?!...!? annotations above)\"\n\t+fi\n\t\nI may just be missing something here, but why not instead just borrow\nwhat I did for \"lint-docs\" in 8650c6298c1 (doc lint: make \"lint-docs\"\nnon-.PHONY, 2021-10-15)?\n\nI.e. if we can run against t0001-init.sh or whatever *once* to see if it\nchain-lints OK then surely we could have a rule like:\n\n\tt0001-init.sh.chainlint-ok: t0001-init.sh\n\t\tperl chainlint.pl $< >$@\n\nThen whenever you change t0001-init.sh we refresh that\nt0001-init.sh.chainlint-ok, if the chainlint.pl exits non-zero we'll\nfail to make it, and will unlink that t0001-init.sh.chainlint-ok.\n\nThat way you wouldn't need any parallelism in the Perl script, because\nyou'd have \"make\" take care of it, and the common case of re-testing\nwhere the speed matters would be that we woudln't need to run this at\nall, or would only re-run it for the test scripts that changed.\n\n(Obviously a \"real\" implementation would want to create that \".ok\" file\nin t/.build/chainlint\" or whatever)\n\nA drawback is that you'd probably be slower on the initial run, as you'd\nspwn N chainlint.pl. You could use $? instead of $< to get around that,\nbut that requires some re-structuring, and I've found it to generally\nnot be worth it.\n\nIt would also have the drawback that a:\n\n\t./t0001-init.sh\n\nwouldn't run the chain-lint, but this would:\n\n\tmake T=t0001-init.sh\n\nBut if want the former to work we could carry some\n\"GIT_TEST_VIA_MAKEFILE\" variable or whatever, and only run the\ntest-via-test-lib.sh if it isn't set.\n"},{"id":"462525","messageId":"9on60586-rr40-onn0-907s-53816r61qn07@tzk.qr","threadId":"58385","inReplyTo":"f5dbcbf78db127d738c11a1aca416201298426cf.1661992197.git.gitgitgadget@gmail.com","subject":"Re: several messages","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-02T12:42:18Z","receivedAt":"2022-09-02T13:07:17Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Thu, 1 Sep 2022, Eric Sunshine via GitGitGadget wrote:\n\n> contrib/buildsystems/CMakeLists.txt | 2 +-\n> t/Makefile | 49 +-\n> t/README | 5 -\n> t/chainlint.pl | 730 ++++++++++++++++++\n> t/chainlint.sed | 399 ----------\n> t/chainlint/blank-line-before-esac.expect | 18 +\n> t/chainlint/blank-line-before-esac.test | 19 +\n> t/chainlint/block.expect | 15 +-\n> t/chainlint/block.test | 15 +-\n> t/chainlint/chain-break-background.expect | 9 +\n> t/chainlint/chain-break-background.test | 10 +\n> t/chainlint/chain-break-continue.expect | 12 +\n> t/chainlint/chain-break-continue.test | 13 +\n> t/chainlint/chain-break-false.expect | 9 +\n> t/chainlint/chain-break-false.test | 10 +\n> t/chainlint/chain-break-return-exit.expect | 19 +\n> t/chainlint/chain-break-return-exit.test | 23 +\n> t/chainlint/chain-break-status.expect | 9 +\n> t/chainlint/chain-break-status.test | 11 +\n> t/chainlint/chained-block.expect | 9 +\n> t/chainlint/chained-block.test | 11 +\n> t/chainlint/chained-subshell.expect | 10 +\n> t/chainlint/chained-subshell.test | 13 +\n> .../command-substitution-subsubshell.expect | 2 +\n> .../command-substitution-subsubshell.test | 3 +\n> t/chainlint/complex-if-in-cuddled-loop.expect | 2 +-\n> t/chainlint/double-here-doc.expect | 2 +\n> t/chainlint/double-here-doc.test | 12 +\n> t/chainlint/dqstring-line-splice.expect | 3 +\n> t/chainlint/dqstring-line-splice.test | 7 +\n> t/chainlint/dqstring-no-interpolate.expect | 11 +\n> t/chainlint/dqstring-no-interpolate.test | 15 +\n> t/chainlint/empty-here-doc.expect | 3 +\n> t/chainlint/empty-here-doc.test | 5 +\n> t/chainlint/exclamation.expect | 4 +\n> t/chainlint/exclamation.test | 8 +\n> t/chainlint/for-loop-abbreviated.expect | 5 +\n> t/chainlint/for-loop-abbreviated.test | 6 +\n> t/chainlint/for-loop.expect | 4 +-\n> t/chainlint/function.expect | 11 +\n> t/chainlint/function.test | 13 +\n> t/chainlint/here-doc-indent-operator.expect | 5 +\n> t/chainlint/here-doc-indent-operator.test | 13 +\n> t/chainlint/here-doc-multi-line-string.expect | 3 +-\n> t/chainlint/if-condition-split.expect | 7 +\n> t/chainlint/if-condition-split.test | 8 +\n> t/chainlint/if-in-loop.expect | 2 +-\n> t/chainlint/if-in-loop.test | 2 +-\n> t/chainlint/loop-detect-failure.expect | 15 +\n> t/chainlint/loop-detect-failure.test | 17 +\n> t/chainlint/loop-detect-status.expect | 18 +\n> t/chainlint/loop-detect-status.test | 19 +\n> t/chainlint/loop-in-if.expect | 2 +-\n> t/chainlint/loop-upstream-pipe.expect | 10 +\n> t/chainlint/loop-upstream-pipe.test | 11 +\n> t/chainlint/multi-line-string.expect | 11 +-\n> t/chainlint/nested-loop-detect-failure.expect | 31 +\n> t/chainlint/nested-loop-detect-failure.test | 35 +\n> t/chainlint/nested-subshell.expect | 2 +-\n> t/chainlint/one-liner-for-loop.expect | 9 +\n> t/chainlint/one-liner-for-loop.test | 10 +\n> t/chainlint/return-loop.expect | 5 +\n> t/chainlint/return-loop.test | 6 +\n> t/chainlint/semicolon.expect | 2 +-\n> t/chainlint/sqstring-in-sqstring.expect | 4 +\n> t/chainlint/sqstring-in-sqstring.test | 5 +\n> t/chainlint/t7900-subtree.expect | 13 +-\n> t/chainlint/token-pasting.expect | 27 +\n> t/chainlint/token-pasting.test | 32 +\n> t/chainlint/while-loop.expect | 4 +-\n> t/t0027-auto-crlf.sh | 7 +-\n> t/t3070-wildmatch.sh | 5 -\n> t/test-lib.sh | 12 +-\n> 73 files changed, 1439 insertions(+), 449 deletions(-)\n> create mode 100755 t/chainlint.pl\n> delete mode 100644 t/chainlint.sed\n> create mode 100644 t/chainlint/blank-line-before-esac.expect\n> create mode 100644 t/chainlint/blank-line-before-esac.test\n> create mode 100644 t/chainlint/chain-break-background.expect\n> create mode 100644 t/chainlint/chain-break-background.test\n> create mode 100644 t/chainlint/chain-break-continue.expect\n> create mode 100644 t/chainlint/chain-break-continue.test\n> create mode 100644 t/chainlint/chain-break-false.expect\n> create mode 100644 t/chainlint/chain-break-false.test\n> create mode 100644 t/chainlint/chain-break-return-exit.expect\n> create mode 100644 t/chainlint/chain-break-return-exit.test\n> create mode 100644 t/chainlint/chain-break-status.expect\n> create mode 100644 t/chainlint/chain-break-status.test\n> create mode 100644 t/chainlint/chained-block.expect\n> create mode 100644 t/chainlint/chained-block.test\n> create mode 100644 t/chainlint/chained-subshell.expect\n> create mode 100644 t/chainlint/chained-subshell.test\n> create mode 100644 t/chainlint/command-substitution-subsubshell.expect\n> create mode 100644 t/chainlint/command-substitution-subsubshell.test\n> create mode 100644 t/chainlint/double-here-doc.expect\n> create mode 100644 t/chainlint/double-here-doc.test\n> create mode 100644 t/chainlint/dqstring-line-splice.expect\n> create mode 100644 t/chainlint/dqstring-line-splice.test\n> create mode 100644 t/chainlint/dqstring-no-interpolate.expect\n> create mode 100644 t/chainlint/dqstring-no-interpolate.test\n> create mode 100644 t/chainlint/empty-here-doc.expect\n> create mode 100644 t/chainlint/empty-here-doc.test\n> create mode 100644 t/chainlint/exclamation.expect\n> create mode 100644 t/chainlint/exclamation.test\n> create mode 100644 t/chainlint/for-loop-abbreviated.expect\n> create mode 100644 t/chainlint/for-loop-abbreviated.test\n> create mode 100644 t/chainlint/function.expect\n> create mode 100644 t/chainlint/function.test\n> create mode 100644 t/chainlint/here-doc-indent-operator.expect\n> create mode 100644 t/chainlint/here-doc-indent-operator.test\n> create mode 100644 t/chainlint/if-condition-split.expect\n> create mode 100644 t/chainlint/if-condition-split.test\n> create mode 100644 t/chainlint/loop-detect-failure.expect\n> create mode 100644 t/chainlint/loop-detect-failure.test\n> create mode 100644 t/chainlint/loop-detect-status.expect\n> create mode 100644 t/chainlint/loop-detect-status.test\n> create mode 100644 t/chainlint/loop-upstream-pipe.expect\n> create mode 100644 t/chainlint/loop-upstream-pipe.test\n> create mode 100644 t/chainlint/nested-loop-detect-failure.expect\n> create mode 100644 t/chainlint/nested-loop-detect-failure.test\n> create mode 100644 t/chainlint/one-liner-for-loop.expect\n> create mode 100644 t/chainlint/one-liner-for-loop.test\n> create mode 100644 t/chainlint/return-loop.expect\n> create mode 100644 t/chainlint/return-loop.test\n> create mode 100644 t/chainlint/sqstring-in-sqstring.expect\n> create mode 100644 t/chainlint/sqstring-in-sqstring.test\n> create mode 100644 t/chainlint/token-pasting.expect\n> create mode 100644 t/chainlint/token-pasting.test\n\nThis looks like it was a lot of work. And that it would be a lot of work\nto review, too, and certainly even more work to maintain.\n\nAre we really sure that we want to burden the Git project with this much\nstuff that is not actually related to Git's core functionality?\n\nIt would be one thing if we could use a well-maintained third-party tool\nto do this job. But adding this to our plate? I hope we can avoid that.\n\nCiao,\nDscho\n"},{"id":"462548","messageId":"CAPig+cRCME=SYyV2bDNoAJjdnHUAWUqSP00aO_v-KWdNvasKpA@mail.gmail.com","threadId":"58385","inReplyTo":"9on60586-rr40-onn0-907s-53816r61qn07@tzk.qr","subject":"Re: several messages","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-02T18:16:21Z","receivedAt":"2022-09-02T18:16:39Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 2, 2022 at 8:42 AM Johannes Schindelin\n wrote:\n> On Thu, 1 Sep 2022, Eric Sunshine via GitGitGadget wrote:\n> > t/chainlint.pl | 730 ++++++++++++++++++\n> > t/chainlint.sed | 399 ----------\n> > t/chainlint/blank-line-before-esac.expect | 18 +\n> > t/chainlint/blank-line-before-esac.test | 19 +\n> > ...\n>\n> This looks like it was a lot of work. And that it would be a lot of work\n> to review, too, and certainly even more work to maintain.\n>\n> Are we really sure that we want to burden the Git project with this much\n> stuff that is not actually related to Git's core functionality?\n>\n> It would be one thing if we could use a well-maintained third-party tool\n> to do this job. But adding this to our plate? I hope we can avoid that.\n\nI understand your concerns about review and maintenance burden, and\nyou're not the first to make such observations; when chainlint.sed was\nsubmitted, it was greeted with similar concerns[1,2], all very\nunderstandable. The key takeaway[3] from that conversation, though,\nwas that, unlike user-facing features which must be reviewed in detail\nand maintained in perpetuity, this is a mere developer aid which can\nbe easily ejected from the project if it ever becomes a maintenance\nburden or shows itself to be unreliable. Potential maintenance burden\naside, a very real benefit of such a tool is that it should help\nprevent bugs from slipping into the project going forward[4], which is\nindeed the aim of all our developer-focused aids.\n\nIn more practical terms, despite initial concerns, in the 4+ years\nsince its introduction, the maintenance cost of chainlint.sed has been\nnearly zero. Very early on, there was a report[5] that chainlint.sed\nwas showing a false-positive in a `contrib` test script; the developer\nquickly responded with a fix[6]. The only other maintenance issues\nwere a couple dead-simple changes[7,8] to shorten \"labels\" to support\nolder versions of `sed`. (As for the chainlint self-tests, the\nmaintenance cost has been exactly zero). My hope is that chainlint.pl\nshould have a similar track-record, but it can easily be dropped from\nthe project if not.\n\n[1]: https://lore.kernel.org/git/xmqqk1q11mkj.fsf@gitster-ct.c.googlers.com/\n[2]: https://lore.kernel.org/git/20180712165608.GA10515@sigill.intra.peff.net/\n[3]: https://lore.kernel.org/git/CAPig+cRmAkiYqFXwRAkQALDoOo-79r2iAumdEJEZhBnETvL-fw@mail.gmail.com/\n[4]: https://lore.kernel.org/git/xmqqin5kw7q3.fsf@gitster-ct.c.googlers.com/\n[5]: https://lore.kernel.org/git/20180730181356.GA156463@aiede.svl.corp.google.com/\n[6]: https://lore.kernel.org/git/20180807082135.60913-1-sunshine@sunshineco.com/\n[7]: https://lore.kernel.org/git/20180824152016.20286-5-avarab@gmail.com/\n[8]: https://lore.kernel.org/git/d15ed626de65c51ef2ba31020eeb2111fb8e091f.1596675905.git.gitgitgadget@gmail.com/\n"},{"id":"462549","messageId":"YxJMzMyjGCyp/b4w@coredump.intra.peff.net","threadId":"58385","inReplyTo":"CAPig+cRCME=SYyV2bDNoAJjdnHUAWUqSP00aO_v-KWdNvasKpA@mail.gmail.com","subject":"Re: several messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-02T18:34:52Z","receivedAt":"2022-09-02T18:35:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2022 at 02:16:21PM -0400, Eric Sunshine wrote:\n\n> > It would be one thing if we could use a well-maintained third-party tool\n> > to do this job. But adding this to our plate? I hope we can avoid that.\n> \n> I understand your concerns about review and maintenance burden, and\n> you're not the first to make such observations; when chainlint.sed was\n> submitted, it was greeted with similar concerns[1,2], all very\n> understandable. The key takeaway[3] from that conversation, though,\n> was that, unlike user-facing features which must be reviewed in detail\n> and maintained in perpetuity, this is a mere developer aid which can\n> be easily ejected from the project if it ever becomes a maintenance\n> burden or shows itself to be unreliable. Potential maintenance burden\n> aside, a very real benefit of such a tool is that it should help\n> prevent bugs from slipping into the project going forward[4], which is\n> indeed the aim of all our developer-focused aids.\n\nThanks for this response and especially the links. My initial gut\nresponse was similar to Dscho's. Which is not surprising, because it\napparently was also my initial response to chainlint.sed back then. ;)\n\nBut I do think that chainlint.sed has proven itself to be both useful\nand not much of a maintenance burden. My only real complaint was the\nadditional runtime in a few corner cases, and that is exactly what\nyou're addressing here.\n\nI'm not excited about carefully reviewing it. At the same time, given\nthe low stakes, I'm kind of willing to accept that between the tests and\nthe results of running it on the current code base, the proof is in the\npudding.\n\n-Peff\n"},{"id":"462550","messageId":"xmqq5yi58vkk.fsf@gitster.g","threadId":"58385","inReplyTo":"YxJMzMyjGCyp/b4w@coredump.intra.peff.net","subject":"Re: several messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-02T18:44:59Z","receivedAt":"2022-09-02T18:45:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Thanks for this response and especially the links. My initial gut\n> response was similar to Dscho's. Which is not surprising, because it\n> apparently was also my initial response to chainlint.sed back then. ;)\n>\n> But I do think that chainlint.sed has proven itself to be both useful\n> and not much of a maintenance burden. My only real complaint was the\n> additional runtime in a few corner cases, and that is exactly what\n> you're addressing here.\n\nI have nothing to add to the above ;-) Thanks all (including Dscho\nwho made us be more explicit in pros-and-cons).\n\n\n"},{"id":"462553","messageId":"CAPig+cT_Z=D2ECJTCC=hosBw9i0vMBZfOAc-+jkPSg3Q519X+w@mail.gmail.com","threadId":"58385","inReplyTo":"220901.86k06njmvq.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 01/18] t: add skeleton chainlint.pl","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-02T18:53:55Z","receivedAt":"2022-09-02T18:54:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 1, 2022 at 8:32 AM Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Sep 01 2022, Eric Sunshine via GitGitGadget wrote:\n> > From: Eric Sunshine \n> > [...]\n> > diff --git a/t/chainlint.pl b/t/chainlint.pl\n>\n> I really like this overall direction...\n\nThanks for running an eye over the patches.\n\n> > +use warnings;\n> > +use strict;\n>\n> I think that in general we're way overdue for at least a :\n>\n> use v5.10.1;\n>\n> Or even something more aggresive, I think we can definitely depend on a\n> newer version for this bit of dev tooling.\n\nBeing stuck with an 11+ year-old primary development machine which\ncan't be upgraded to a newer OS due to vendor end-of-life declaration,\nand with old tools installed, I have little or no interest in bumping\nthe minimum version, especially since older Perl versions are\nperfectly adequate for this task. Undertaking such a version bump\nwould also be outside the scope of this patch series (and I simply\ndon't have the free time or desire to pursue it).\n\n> That makes a lot of things in this series more pleasing to look\n> at. E.g. you could use named $+{} variables for regexes.\n\nPerhaps, but (1) that would not be very relevant for this script which\ntypically only extracts \"$1\", and (2) I've rarely found cases when\nnamed variables help significantly with clarity, but then most of my\nreal-life regexes generally only extract one or two bits of\ninformation, periodically three, and those bits (\"$1\", \"$2\", etc.) are\nimmediately assigned to variables with meaningful names.\n\n> > +package ScriptParser;\n>\n> I really wish this could be changed to just put this in\n> t/chainlint/ScriptParser.pm early on, we could set @INC appropriately\n> and \"use\" these, which...\n\nI intentionally avoided splitting this into multiple modules because I\nwanted it to be easy drop into or adapt to other projects (i.e.\nsharness[1]). Of course, it is effectively a shell parser written in\nPerl, and it's conceivable that the parser part of it could have uses\noutside of Git, so modularizing it might be a good idea, but that's a\ntask for some future date if such a need arises.\n\n[1]: https://github.com/chriscool/sharness\n\n> > +my $getnow = sub { return time(); };\n> > +my $interval = sub { return time() - shift; };\n>\n> Would eliminate any scoping concerns about this sort of thing.\n\nAs above, this is easily addressed if/when someone ever wants to reuse\nthe code outside of Git for some other purpose. I doubt it's worth\nworrying about now.\n\n> > +if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {\n> > + $getnow = sub { return [Time::HiRes::gettimeofday()]; };\n> > + $interval = sub { return Time::HiRes::tv_interval(shift); };\n> > +}\n>\n> Is this \"require\" even needed, Time::HiRes is there since 5.7.* says\n> \"corelist -l Time::HIRes\".\n\nUnfortunately, this is needed. The Windows CI instances the Git\nproject uses don't have Time::HiRes installed (and it's outside the\nscope of this series to address shortcomings in the CI\ninfrastructure).\n\n> > +sub check_script {\n> > + my ($id, $next_script, $emit) = @_;\n> > + my ($nscripts, $ntests, $nerrs) = (0, 0, 0);\n> > + while (my $path = $next_script->()) {\n> > + $nscripts++;\n> > + my $fh;\n> > + unless (open($fh, \"<\", $path)) {\n> > + $emit->(\"?!ERR?! $path: $!\\n\");\n>\n> If we can depend on v5.10.1 this can surely become:\n>\n> use autodie qw(open close);\n>\n> No?\n\nNo. It's clipped in your response, but the full snippet looks like this:\n\n unless (open($fh, \"<\", $path)) {\n $emit->(\"?!ERR?! $path: $!\\n\");\n next;\n }\n\nThe important point is that I _don't_ want the program to \"die\" if it\ncan't open an input file; instead, it should continue processing all\nthe other input files, and the open-failure should be reported as just\nanother error/problem it encountered along the way.\n"},{"id":"462571","messageId":"CABPp-BHAGb9RU2d7_1ZCDbgKj9aB0JkHUD9_mMnVL=EttqZ4Bw@mail.gmail.com","threadId":"58385","inReplyTo":"9589f2a6e495034cc4f45bd0bce80dedfcd30f16.1661992197.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 16/18] test-lib: replace chainlint.sed with chainlint.pl","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-09-03T05:07:33Z","receivedAt":"2022-09-03T05:07:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 31, 2022 at 5:30 PM Eric Sunshine via GitGitGadget\n wrote:\n>\n> From: Eric Sunshine \n>\n> By automatically invoking chainlint.sed upon each test it runs,\n> `test_run_` in test-lib.sh ensures that broken &&-chains will be\n> detected early as tests are modified or new are tests created since it\n\ns/new are tests created/new tests are created/ ?\n\n\n> is typical to run a test script manually (i.e. `./t1234-test-script.sh`)\n> during test development. Now that the implementation of chainlint.pl is\n> complete, modify test-lib.sh to invoke it automatically instead of\n> chainlint.sed each time a test script is run.\n>\n> This change reduces the number of \"linter\" invocations from 26800+ (once\n> per test run) down to 1050+ (once per test script), however, a\n> subsequent change will drop the number of invocations to 1 per `make\n> test`, thus fully realizing the benefit of the new linter.\n>\n> Note that the \"magic exit code 117\" &&-chain checker added by bb79af9d09\n> (t/test-lib: introduce --chain-lint option, 2015-03-20) which is built\n> into t/test-lib.sh is retained since it has near zero-cost and\n> (theoretically) may catch a broken &&-chain not caught by chainlint.pl.\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> contrib/buildsystems/CMakeLists.txt | 2 +-\n> t/test-lib.sh | 9 +++++++--\n> 2 files changed, 8 insertions(+), 3 deletions(-)\n>\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index 2237109b57f..ca358a21a5f 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -1076,7 +1076,7 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})\n> \"string(REPLACE \\\"\\${GIT_BUILD_DIR_REPL}\\\" \\\"GIT_BUILD_DIR=\\\\\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\\\\\"\\\" content \\\"\\${content}\\\")\\n\"\n> \"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \\${content})\")\n> #misc copies\n> - file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> + file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.pl DESTINATION ${CMAKE_BINARY_DIR}/t/)\n> file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)\n> file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)\n> file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index dc0d0591095..a65df2fd220 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1091,8 +1091,7 @@ test_run_ () {\n> trace=\n> # 117 is magic because it is unlikely to match the exit\n> # code of other programs\n> - if $(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!') ||\n> - test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> + if test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> then\n> BUG \"broken &&-chain or run-away HERE-DOC: $1\"\n> fi\n> @@ -1588,6 +1587,12 @@ then\n> BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK \"GIT_TEST_SANITIZE_LEAK_LOG=true\"\n> fi\n>\n> +if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0\n> +then\n> + \"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n> + BUG \"lint error (see '?!...!? annotations above)\"\n> +fi\n> +\n> # Last-minute variable setup\n> USER_HOME=\"$HOME\"\n> HOME=\"$TRASH_DIRECTORY\"\n> --\n> gitgitgadget\n>\n"},{"id":"462572","messageId":"CAPig+cS8Udx+ia7PCEGeGLeX_1pxk8FU+-57=NYkTuHdQH4hJg@mail.gmail.com","threadId":"58385","inReplyTo":"CABPp-BHAGb9RU2d7_1ZCDbgKj9aB0JkHUD9_mMnVL=EttqZ4Bw@mail.gmail.com","subject":"Re: [PATCH 16/18] test-lib: replace chainlint.sed with chainlint.pl","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-03T05:24:46Z","receivedAt":"2022-09-03T05:25:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 3, 2022 at 1:07 AM Elijah Newren wrote:\n> On Wed, Aug 31, 2022 at 5:30 PM Eric Sunshine via GitGitGadget\n> > By automatically invoking chainlint.sed upon each test it runs,\n> > `test_run_` in test-lib.sh ensures that broken &&-chains will be\n> > detected early as tests are modified or new are tests created since it\n>\n> s/new are tests created/new tests are created/ ?\n\nThat does sound better (except perhaps to Yoda).\n"},{"id":"462575","messageId":"CAPig+cTfcz3cJ3-ESW-yUNa7QC0HbjZ_giDQA72gBWp5T4Zb6w@mail.gmail.com","threadId":"58385","inReplyTo":"220901.86fshbjmqj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 02/18] chainlint.pl: add POSIX shell lexical analyzer","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-03T06:00:11Z","receivedAt":"2022-09-03T06:00:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 1, 2022 at 8:35 AM Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Sep 01 2022, Eric Sunshine via GitGitGadget wrote:\n> Just generally on this series:\n>\n> > + $tag =~ s/['\"\\\\]//g;\n>\n> I think this would be a *lot* easier to read if all of these little\n> regex decls could be split out into some \"grammar\" class, or other\n> helper module/namespace. So e.g.:\n>\n> my $SCRIPT_QUOTE_RX = qr/['\"\\\\]/;\n\nTaken out of context (as in the quoted snippet), it may indeed be\ndifficult to understand what that line is doing, however in context\nwith a meaningful function name:\n\n sub scan_heredoc_tag {\n ...\n my $tag = $self->scan_token();\n $tag =~ s/['\"\\\\]//g;\n push(@{$self->{heretags}}, $indented ? \"\\t$tag\" : \"$tag\");\n ...\n }\n\nfor someone who is familiar with common heredoc tag quoting/escaping\n(i.e. <<'EOF', <<\"EOF\", <<\\EOF), I find the inline character class\n`['\"\\\\]` much easier to understand than some opaque name such as\n$SCRIPT_QUOTE_RX, doubly so because the definition of the named regex\nmight be far removed from the actual code which uses it, which would\nrequire going and studying that definition before being able to\nunderstand what this code is doing.\n\nI grasp you made that name up on-the-fly as an example, but that does\nhighlight another reason why I'd be hesitant to try to pluck out and\nname these regexes. Specifically, naming is hard and I don't trust\nthat I could come up with succinct meaningful names which would convey\nwhat a regex does as well as the actual regex itself conveys what it\ndoes. In context within the well-named function, `s/['\"\\\\]//g` is\nobviously stripping quoting/escaping from the tag name; trying to come\nup with a succinct yet accurate name to convey that intention is\ndifficult. And this is just one example. The script is littered with\nlittle regexes like this, and they are almost all unique, thus making\nthe task of inventing succinct meaningful names extra difficult. And,\nas noted above, I'm not at all convinced that plucking the regex out\nof its natural context -- thus making the reader go elsewhere to find\nthe definition of the regex -- would help improve comprehension.\n\n> Then:\n>\n> > + return $cc if $cc =~ /^(?:&&|\\|\\||>>|;;|<&|>&|<>|>\\|)$/;\n>\n> my $SCRIPT_WHATEVER_RX = qr/\n> ^(?:\n> &&\n> |\n> \\|\\|\n> [...]\n> /x;\n>\n> etc., i.e. we could then make use of /x to add inline comments to these.\n\n`/x` does make this slightly easier to grok, and this is a an example\nof a regex which might be easy to name (i.e. $TWO_CHAR_OPERATOR), but\n-- extra mandatory escaping aside -- it's not hard to understand this\none as-is; it's pretty obvious that it's looking for operators `&&`,\n`||`, `>>`, `;;`, `<&`, `>&`, `<>`, and `>|`.\n"},{"id":"462577","messageId":"CAPig+cThSD12whinyLzhHH9qh+bR7W_AH8ea5GT6B=bd87f2RA@mail.gmail.com","threadId":"58385","inReplyTo":"220901.86bkrzjm6e.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-03T07:51:37Z","receivedAt":"2022-09-03T07:51:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 1, 2022 at 8:47 AM Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Sep 01 2022, Eric Sunshine via GitGitGadget wrote:\n> > Although chainlint.pl has undergone a good deal of optimization during\n> > its development -- increasing in speed significantly -- parsing and\n> > validating 1050+ scripts and 16500+ tests via Perl is not exactly\n> > instantaneous. However, perceived performance can be improved by taking\n> > advantage of the fact that there is no interdependence between test\n> > scripts or test definitions, thus parsing and validating can be done in\n> > parallel. The number of available cores is determined automatically but\n> > can be overridden via the --jobs option.\n>\n> Per your CL:\n>\n> Ævar offered some sensible comments[2,3] about optimizing the Makefile rules\n> related to chainlint, but those optimizations are not tackled here for a few\n> reasons: (1) this series is already quite long, (2) I'd like to keep the\n> series focused on its primary goal of installing a new and improved linter,\n> (3) these patches do not make the Makefile situation any worse[4], and (4)\n> those optimizations can easily be done atop this series[5].\n>\n> I have been running with those t/Makefile changesg locally, but didn't\n> submit them. FWIW that's here:\n>\n> https://github.com/git/git/compare/master...avar:git:avar/t-Makefile-use-dependency-graph-for-check-chainlint\n\nThanks for the link. It's nice to see an actual implementation. I\nthink most of what you wrote in the commit message and the patch\nitself are still meaningful following this series.\n\n> > +my $script_queue = Thread::Queue->new();\n> > +my $output_queue = Thread::Queue->new();\n> > +\n> > +my $mon = threads->create({'context' => 'void'}, \\&monitor);\n> > +threads->create({'context' => 'list'}, \\&check_script, $_, \\&next_script, \\&emit) for 1..$jobs;\n>\n> Maybe I'm misunderstanding this whole thing, but this really seems like\n> the wrong direction in an otherwise fantastic direction of a series.\n>\n> I.e. it's *great* that we can do chain-lint without needing to actually\n> execute the *.sh file, this series adds a lint parser that can parse\n> those *.sh \"at rest\".\n>\n> But in your 16/18 you then do:\n>\n> +if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0\n> +then\n> + \"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n> + BUG \"lint error (see '?!...!? annotations above)\"\n> +fi\n>\n> I may just be missing something here, but why not instead just borrow\n> what I did for \"lint-docs\" in 8650c6298c1 (doc lint: make \"lint-docs\"\n> non-.PHONY, 2021-10-15)?\n\nI may be misunderstanding, but regarding patch [16/18], I think you\nanswered your own question at the end of your response when you\npointed out the drawback that you wouldn't get linting when running\nthe test script manually (i.e. `./t1234-test-stuff.sh`). Ensuring that\nthe linter is invoked when running a test script manually is important\n(at least to me) since it's a frequent step when developing a new test\nor modifying an existing test. [16/18] is present to ensure that we\nstill get that behavior.\n\n> I.e. if we can run against t0001-init.sh or whatever *once* to see if it\n> chain-lints OK then surely we could have a rule like:\n>\n> t0001-init.sh.chainlint-ok: t0001-init.sh\n> perl chainlint.pl $< >$@\n>\n> Then whenever you change t0001-init.sh we refresh that\n> t0001-init.sh.chainlint-ok, if the chainlint.pl exits non-zero we'll\n> fail to make it, and will unlink that t0001-init.sh.chainlint-ok.\n>\n> That way you wouldn't need any parallelism in the Perl script, because\n> you'd have \"make\" take care of it, and the common case of re-testing\n> where the speed matters would be that we woudln't need to run this at\n> all, or would only re-run it for the test scripts that changed.\n\nA couple comments regarding parallelism: (1) as mentioned in another\nresponse, when developing the script, I had in mind that it might be\nuseful for other projects (i.e. `sharness`), thus should be able to\nstand on its own without advanced Makefile support, and (2) process\ncreation on Microsoft Windows is _very_ expensive and slow, so on that\nplatform, being able to lint all tests in all script with a single\ninvocation is a big win over running the linter 1050+ times, once for\neach test script.\n\nThat's not to discredit any of your points... I'm just conveying some\nof my thought process.\n\n> (Obviously a \"real\" implementation would want to create that \".ok\" file\n> in t/.build/chainlint\" or whatever)\n>\n> A drawback is that you'd probably be slower on the initial run, as you'd\n> spwn N chainlint.pl. You could use $? instead of $< to get around that,\n> but that requires some re-structuring, and I've found it to generally\n> not be worth it.\n\nThe $? trick might be something Windows folk would appreciate, and\neven those of us in macOS land (at least those of us with old hardware\nand OS).\n\n> It would also have the drawback that a:\n>\n> ./t0001-init.sh\n>\n> wouldn't run the chain-lint, but this would:\n>\n> make T=t0001-init.sh\n>\n> But if want the former to work we could carry some\n> \"GIT_TEST_VIA_MAKEFILE\" variable or whatever, and only run the\n> test-via-test-lib.sh if it isn't set.\n\nI may be misunderstanding, but isn't the GIT_TEST_CHAIN_LINT variable\nuseful for this already, as in [16/18]?\n\nRegarding your observations as a whole, I think the extract from the\ncover letter which you cited above is relevant to my response. I don't\ndisagree with your points about using the Makefile to optimize away\nunnecessary invocations of the linter, or that doing so can be a\nuseful future direction. As mentioned in the cover letter, though, I\nthink that such optimizations are outside the scope of this series\nwhich -- aside from installing an improved linter -- aims to maintain\nthe status quo; in particular, this series ensures that (1) tests get\nlinted as they are being written/modified when the developer runs the\nscript manually `./t1234-test-stuff.sh`, and (2) all tests get linted\nupon `make test`.\n\n(The other reason why I'd prefer to see such optimizations applied\natop this series is that I simply don't have the time these days to\ndevote to major changes of direction in this series, which I think\nmeets its stated goals without making the situation any worse or\nmaking it any more difficult to apply the optimizations you describe.\nAnd the new linter has been languishing on my computer for far too\nlong; the implementation has been complete for well over a year, but\nit took me this long to finish polishing the patch series. I'd like to\nsee the new linter make it into the toolchest of other developers\nsince it can be beneficial; it has already found scores or hundreds[1]\nof possible hiding places for bugs due to broken &&-chain or missing\n`|| return`, and has sniffed out some actual broken tests[2,3].)\n\n[1]: https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/\n[2]: https://lore.kernel.org/git/20211209051115.52629-3-sunshine@sunshineco.com/\n[3]: https://lore.kernel.org/git/7b0784056f3cc0c96e9543ae44d0f5a7b0bf85fa.1661192802.git.gitgitgadget@gmail.com/\n"},{"id":"462673","messageId":"20220906223537.M956576@dcvr","threadId":"58385","inReplyTo":"62fc652eb47a4df83d88a197e376f28dbbab3b52.1661992197.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-09-06T22:35:37Z","receivedAt":"2022-09-06T22:35:42Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Sunshine via GitGitGadget wrote:\n> +unless ($Config{useithreads} && eval {\n> +\trequire threads; threads->import();\n\nFwiw, the threads(3perl) manpage has this since 2014:\n\n The use of interpreter-based threads in perl is officially discouraged.\n\nI was bummed, too :< but I've decided it wasn't worth the\neffort to deal with the problems threads could cause down the\nline in future Perl versions. For example, common libraries\nlike File::Temp will chdir behind-the-scenes which is\nthread-unsafe.\n\n(of course I only care about *BSD and Linux on MMU hardware,\nso I use SOCK_SEQPACKET and fork() freely :>)\n"},{"id":"462674","messageId":"CAPig+cSx661-HEr3JcAD5MuYfgHviGQ1cSAftkgw6gj2FgTQVg@mail.gmail.com","threadId":"58385","inReplyTo":"20220906223537.M956576@dcvr","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-06T22:52:26Z","receivedAt":"2022-09-06T22:52:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 6, 2022 at 6:35 PM Eric Wong wrote:\n> Eric Sunshine via GitGitGadget wrote:\n> > +unless ($Config{useithreads} && eval {\n> > + require threads; threads->import();\n>\n> Fwiw, the threads(3perl) manpage has this since 2014:\n>\n> The use of interpreter-based threads in perl is officially discouraged.\n\nThanks for pointing this out. I did see that, but as no better\nalternative was offered, and since I did want this to work on Windows,\nI went with it.\n\n> I was bummed, too :< but I've decided it wasn't worth the\n> effort to deal with the problems threads could cause down the\n> line in future Perl versions. For example, common libraries\n> like File::Temp will chdir behind-the-scenes which is\n> thread-unsafe.\n>\n> (of course I only care about *BSD and Linux on MMU hardware,\n> so I use SOCK_SEQPACKET and fork() freely :>)\n\nI'm not overly worried about the deprecation at the moment since (1)\nchainlint.pl isn't a widely used script -- it's audience is very\nnarrow; (2) the `$Config{useithreads}` conditional can be seen as an\nautomatic escape-hatch, and (if need be) I can even make `--jobs=1` be\nan explicit escape hatch, and there's already --no-chain-lint for an\nextreme escape-hatch; (3) the script is pretty much standalone -- it\ndoesn't rely upon any libraries like File::Temp or others; (4) Ævar\nhas ideas for using the Makefile for parallelism instead; (5) we can\ncross the deprecation-bridge when/if it actually does become a\nproblem, either by dropping parallelism from chainlint.pl or by\ndropping chainlint.pl itself.\n"},{"id":"462679","messageId":"YxfXQ0IJjq/FT2Uh@coredump.intra.peff.net","threadId":"58385","inReplyTo":"CAPig+cSx661-HEr3JcAD5MuYfgHviGQ1cSAftkgw6gj2FgTQVg@mail.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-06T23:26:59Z","receivedAt":"2022-09-06T23:27:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 06, 2022 at 06:52:26PM -0400, Eric Sunshine wrote:\n\n> On Tue, Sep 6, 2022 at 6:35 PM Eric Wong wrote:\n> > Eric Sunshine via GitGitGadget wrote:\n> > > +unless ($Config{useithreads} && eval {\n> > > + require threads; threads->import();\n> >\n> > Fwiw, the threads(3perl) manpage has this since 2014:\n> >\n> > The use of interpreter-based threads in perl is officially discouraged.\n> \n> Thanks for pointing this out. I did see that, but as no better\n> alternative was offered, and since I did want this to work on Windows,\n> I went with it.\n\nI did some timings the other night, and I found something quite curious\nwith the thread stuff.\n\nHere's a hyperfine run of \"make\" in the t/ directory before any of your\npatches. It uses \"prove\" to do parallelism under the hood:\n\n Benchmark 1: make\n Time (mean ± σ): 68.895 s ± 0.840 s [User: 620.914 s, System: 428.498 s]\n Range (min … max): 67.943 s … 69.531 s 3 runs\n\nSo that gives us a baseline. Now the first thing I wondered is how bad\nit would be to just run chainlint.pl once per script. So I applied up to\nthat patch:\n\n Benchmark 1: make\n Time (mean ± σ): 71.289 s ± 1.302 s [User: 673.300 s, System: 417.912 s]\n Range (min … max): 69.788 s … 72.120 s 3 runs\n\nI was quite surprised that it made things slower! It's nice that we're\nonly calling it once per script instead of once per test, but it seems\nthe startup overhead of the script is really high.\n\nAnd since in this mode we're only feeding it one script at a time, I\ntried reverting the \"chainlint.pl: validate test scripts in parallel\"\ncommit. And indeed, now things are much faster:\n\n Benchmark 1: make\n Time (mean ± σ): 61.544 s ± 3.364 s [User: 556.486 s, System: 384.001 s]\n Range (min … max): 57.660 s … 63.490 s 3 runs\n\nAnd you can see the same thing just running chainlint by itself:\n\n $ time perl chainlint.pl /dev/null\n real\t0m0.069s\n user\t0m0.042s\n sys\t0m0.020s\n\n $ git revert HEAD^{/validate.test.scripts.in.parallel}\n $ time perl chainlint.pl /dev/null\n real\t0m0.014s\n user\t0m0.010s\n sys\t0m0.004s\n\nI didn't track down the source of the slowness. Maybe it's loading extra\nmodules, or maybe it's opening /proc/cpuinfo, or maybe it's the thread\nsetup. But it's a surprising slowdown.\n\nNow of course your intent is to do a single repo-wide invocation. And\nthat is indeed a bit faster. Here it is without the parallel code:\n\n Benchmark 1: make\n Time (mean ± σ): 61.727 s ± 2.140 s [User: 507.712 s, System: 377.753 s]\n Range (min … max): 59.259 s … 63.074 s 3 runs\n\nThe wall-clock time didn't improve much, but the CPU time did. Restoring\nthe parallel code does improve the wall-clock time a bit, but at the\ncost of some extra CPU:\n\n Benchmark 1: make\n Time (mean ± σ): 59.029 s ± 2.851 s [User: 515.690 s, System: 380.369 s]\n Range (min … max): 55.736 s … 60.693 s 3 runs\n\nwhich makes sense. If I do a with/without of just \"make test-chainlint\",\nthe parallelism is buying a few seconds of wall-clock:\n\n Benchmark 1: make test-chainlint\n Time (mean ± σ): 900.1 ms ± 102.9 ms [User: 12049.8 ms, System: 79.7 ms]\n Range (min … max): 704.2 ms … 994.4 ms 10 runs\n\n Benchmark 1: make test-chainlint\n Time (mean ± σ): 3.778 s ± 0.042 s [User: 3.756 s, System: 0.023 s]\n Range (min … max): 3.706 s … 3.833 s 10 runs\n\nI'm not sure what it all means. For Linux, I think I'd be just as happy\nwith a single non-parallelized test-chainlint run for each file. But\nmaybe on Windows the startup overhead is worse? OTOH, the whole test run\nis so much worse there. One process per script is not going to be that\nmuch in relative terms either way.\n\nAnd if we did cache the results and avoid extra invocations via \"make\",\nthen we'd want all the parallelism to move to there anyway.\n\nMaybe that gives you more food for thought about whether perl's \"use\nthreads\" is worth having.\n\n-Peff\n"},{"id":"462911","messageId":"Yx1x5lme2SGBjfia@coredump.intra.peff.net","threadId":"58385","inReplyTo":"pull.1322.git.git.1661992197.gitgitgadget@gmail.com","subject":"Re: [PATCH 00/18] make test \"linting\" more comprehensive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-11T05:28:06Z","receivedAt":"2022-09-11T05:36:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2022 at 12:29:38AM +0000, Eric Sunshine via GitGitGadget wrote:\n\n> A while back, Peff successfully nerd-sniped[1] me into tackling a\n> long-brewing idea I had about (possibly) improving \"chainlint\" performance\n\nOops, sorry. :)\n\nI gave this a read-through, and it looks sensible overall. I have to\nadmit that I did not carefully check all of your regexes. Given the\nrelatively low stakes of the code (as an internal build-time tool only)\nand the set of tests accompanying it, I'm willing to assume it's good\nenough until we see counter-examples.\n\nI posted some timings and thoughts on the use of threads elsewhere. But\nin the end the timings are close enough that I don't care that much\neither way.\n\nI'd also note that I got some first-hand experience with the script as I\nmerged it with all of my other long-brewing topics, and it found a half\ndozen spots, mostly LOOP annotations. At least one was a real \"oops,\nwe'd miss a bug in Git here\" spot. Several were \"we'd probably notice\nthe problem because the loop output wouldn't be as expected\". One was a\n\"we're on the left-hand of a pipe, so the exit code doesn't matter\nanyway\" case, but I am more than happy to fix those if it lets us be\nlinter-clean.\n\nThe output took me a minute to adjust to, just because it feels pretty\njumbled when there are several cases. Mostly this is because the\nscript eats indentation. So it's hard to see the \"# chainlint:\" comment\nstarts, let alone the ?! annotations. Here's an example:\n\n-- >8 --\n# chainlint: t4070-diff-pairs.sh\n# chainlint: split input across multiple diff-pairs\nwrite_script split-raw-diff \"$PERL_PATH\" <<-EOF &&\n\ngit diff-tree -p -M -C -C base new > expect &&\n\ngit diff-tree -r -z -M -C -C base new |\n./split-raw-diff &&\nfor i in diff* ; do\ngit diff-pairs -p < $i ?!LOOP?!\ndone > actual &&\ntest_cmp expect actual\n# chainlint: perf/p5305-pack-limits.sh\n# chainlint: set up delta islands\nhead=$(git rev-parse HEAD) &&\ngit for-each-ref --format=\"delete %(refname)\" |\ngit update-ref --no-deref --stdin &&\n\nn=0 &&\nfork=0 &&\ngit rev-list --first-parent $head |\nwhile read commit ; do\nn=$((n+1)) ?!AMP?!\nif test \"$n\" = 100 ; then\necho \"create refs/forks/$fork/master $commit\" ?!AMP?!\nfork=$((fork+1)) ?!AMP?!\nn=0\nfi ?!LOOP?!\ndone |\ngit update-ref --stdin &&\n\ngit config pack.island \"refs/forks/([0-9]*)/\"\n-- 8< --\n\nIt wasn't too bad once I got the hang of it, but I wonder if a user\nwriting a single test for the first time may get a bit overwhelmed. I\nassume that the indentation is removed as part of the normalization (I\nnotice extra whitespace around \"<\", too). That might be hard to address.\n\nI wonder if color output for \"# chainlint\" and \"?!\" annotations would\nhelp, too. It looks like that may be tricky, though, because the\nannotations re-parsed internally in some cases.\n\n-Peff\n"},{"id":"462912","messageId":"CAPig+cRJVn-mbA6-jOmNfDJtK_nX4ZTw+OcNShvvz8zcQYbCHQ@mail.gmail.com","threadId":"58385","inReplyTo":"Yx1x5lme2SGBjfia@coredump.intra.peff.net","subject":"Re: [PATCH 00/18] make test \"linting\" more comprehensive","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-11T07:01:41Z","receivedAt":"2022-09-11T07:01:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 11, 2022 at 1:28 AM Jeff King wrote:\n> On Thu, Sep 01, 2022 at 12:29:38AM +0000, Eric Sunshine via GitGitGadget wrote:\n> > A while back, Peff successfully nerd-sniped[1] me into tackling a\n> > long-brewing idea I had about (possibly) improving \"chainlint\" performance\n>\n> I gave this a read-through, and it looks sensible overall. I have to\n> admit that I did not carefully check all of your regexes. Given the\n> relatively low stakes of the code (as an internal build-time tool only)\n> and the set of tests accompanying it, I'm willing to assume it's good\n> enough until we see counter-examples.\n\nThanks for the feedback.\n\n> I posted some timings and thoughts on the use of threads elsewhere. But\n> in the end the timings are close enough that I don't care that much\n> either way.\n\nI ran my eye over that message quickly and have been meaning to dig\ninto it and give it a proper response but haven't yet found the time.\n\n> I'd also note that I got some first-hand experience with the script as I\n> merged it with all of my other long-brewing topics, and it found a half\n> dozen spots, mostly LOOP annotations. At least one was a real \"oops,\n> we'd miss a bug in Git here\" spot. Several were \"we'd probably notice\n> the problem because the loop output wouldn't be as expected\". One was a\n> \"we're on the left-hand of a pipe, so the exit code doesn't matter\n> anyway\" case, but I am more than happy to fix those if it lets us be\n> linter-clean.\n\nIndeed, I'm not super happy about the linter complaining about cases\nwhich obviously can't have an impact on the test's outcome, but (as\nmentioned elsewhere in the thread), finally convinced myself that the\nrelatively low number of these was outweighed by the quite large\nnumber of cases caught by the linter which could have let real\nproblems slip though. Perhaps some day the linter can be made smarter\nabout these cases.\n\n> The output took me a minute to adjust to, just because it feels pretty\n> jumbled when there are several cases. Mostly this is because the\n> script eats indentation. So it's hard to see the \"# chainlint:\" comment\n> starts, let alone the ?! annotations. Here's an example:\n> [...snip...]\n> It wasn't too bad once I got the hang of it, but I wonder if a user\n> writing a single test for the first time may get a bit overwhelmed. I\n> assume that the indentation is removed as part of the normalization (I\n> notice extra whitespace around \"<\", too). That might be hard to address.\n\nThe script implements a proper parser and lexer, and the lexer is\ntokenizing the input (throwing away whitespace in the process), thus\nby the time the parser notices something to complain about with a\n\"?!FOO?!\" annotation, the original whitespace is long gone, and it\njust emits the token stream with \"?!FOO?!\" inserted at the correct\nplace. In retrospect, the way this perhaps should have been done would\nhave been for the parser to instruct the lexer to emit a \"?!FOO?!\"\nannotation at the appropriate point in the input stream. But even that\nmight get a bit hairy since there are cases in which the parser\nback-patches by removing some \"?!AMP?!\" annotations when it has\ndecided that it doesn't need to complain about &&-chain breakage. I'm\nsure it's fixable, but don't know how important it is at this point.\n\n> I wonder if color output for \"# chainlint\" and \"?!\" annotations would\n> help, too. It looks like that may be tricky, though, because the\n> annotations re-parsed internally in some cases.\n\nI had the exact same thought about coloring the \"# chainlint:\" lines\nand \"?!FOO?!\" annotations, and how helpful that could be to anyone\n(not just newcomers). Aside from not having much free time these days,\na big reason I didn't tackle it was because doing so properly probably\nmeans relying upon some third-party Perl module, and I intentionally\nwanted to keep the linter independent of add-on modules. Even without\na \"coloring\" module of some sort, if Perl had a standard `curses`\nmodule (which it doesn't), then it would have been easy enough to ask\n`curses` for the proper color codes and apply them as needed. I'm\nold-school, so it doesn't appeal to me, but an alternative would be to\nassume it's safe to use ANSI color codes, but even that may have to be\ndone carefully (i.e. checking TERM and accepting only some whitelisted\nentries, and worrying about about Windows consoles).\n"},{"id":"462920","messageId":"Yx4pg2t6JXR+lsd4@coredump.intra.peff.net","threadId":"58385","inReplyTo":"CAPig+cRJVn-mbA6-jOmNfDJtK_nX4ZTw+OcNShvvz8zcQYbCHQ@mail.gmail.com","subject":"Re: [PATCH 00/18] make test \"linting\" more comprehensive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-11T18:31:31Z","receivedAt":"2022-09-11T18:32:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 11, 2022 at 03:01:41AM -0400, Eric Sunshine wrote:\n\n> > I wonder if color output for \"# chainlint\" and \"?!\" annotations would\n> > help, too. It looks like that may be tricky, though, because the\n> > annotations re-parsed internally in some cases.\n> \n> I had the exact same thought about coloring the \"# chainlint:\" lines\n> and \"?!FOO?!\" annotations, and how helpful that could be to anyone\n> (not just newcomers). Aside from not having much free time these days,\n> a big reason I didn't tackle it was because doing so properly probably\n> means relying upon some third-party Perl module, and I intentionally\n> wanted to keep the linter independent of add-on modules. Even without\n> a \"coloring\" module of some sort, if Perl had a standard `curses`\n> module (which it doesn't), then it would have been easy enough to ask\n> `curses` for the proper color codes and apply them as needed. I'm\n> old-school, so it doesn't appeal to me, but an alternative would be to\n> assume it's safe to use ANSI color codes, but even that may have to be\n> done carefully (i.e. checking TERM and accepting only some whitelisted\n> entries, and worrying about about Windows consoles).\n\nWe're pretty happy to just use ANSI in the rest of Git, but there is a\ncomplication on Windows. See compat/winansi.c where we decode those\ninternally into SetConsoleTextAttribute() calls.\n\nI think we can live with it as-is for now and see how people react. If\nlots of people are getting confused by the output, then that motivates\nfinding a solution. If not, then it's probably not worth the time.\n\n-Peff\n"},{"id":"462952","messageId":"CAPig+cTmosgapa=iUir3-J9k3138DvecHkmX+0QeHJROeTCeeA@mail.gmail.com","threadId":"58385","inReplyTo":"Yx4pg2t6JXR+lsd4@coredump.intra.peff.net","subject":"Re: [PATCH 00/18] make test \"linting\" more comprehensive","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-12T23:17:12Z","receivedAt":"2022-09-12T23:17:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 11, 2022 at 2:31 PM Jeff King wrote:\n> On Sun, Sep 11, 2022 at 03:01:41AM -0400, Eric Sunshine wrote:\n> > > I wonder if color output for \"# chainlint\" and \"?!\" annotations would\n> > > help, too. It looks like that may be tricky, though, because the\n> > > annotations re-parsed internally in some cases.\n> >\n> > I had the exact same thought about coloring the \"# chainlint:\" lines\n> > and \"?!FOO?!\" annotations, and how helpful that could be to anyone\n> > (not just newcomers). Aside from not having much free time these days,\n> > a big reason I didn't tackle it was because doing so properly probably\n> > means relying upon some third-party Perl module, and I intentionally\n> > wanted to keep the linter independent of add-on modules. Even without\n> > a \"coloring\" module of some sort, if Perl had a standard `curses`\n> > module (which it doesn't), then it would have been easy enough to ask\n> > `curses` for the proper color codes and apply them as needed. I'm\n> > old-school, so it doesn't appeal to me, but an alternative would be to\n> > assume it's safe to use ANSI color codes, but even that may have to be\n> > done carefully (i.e. checking TERM and accepting only some whitelisted\n> > entries, and worrying about about Windows consoles).\n>\n> We're pretty happy to just use ANSI in the rest of Git, but there is a\n> complication on Windows. See compat/winansi.c where we decode those\n> internally into SetConsoleTextAttribute() calls.\n>\n> I think we can live with it as-is for now and see how people react. If\n> lots of people are getting confused by the output, then that motivates\n> finding a solution. If not, then it's probably not worth the time.\n\nWell, you nerd-sniped me anyhow. The result is at [1]. Following the\nexample of t/test-lib.sh, it uses `tput` if available to avoid\nhardcoding color codes, and `tput` is invoked lazily, only if it\ndetects problems in the tests, so a normal (non-problematic) run\ndoesn't incur the overhead of shelling out to `tput`.\n\nMy first attempt just assumed ANSI color codes, but then I discovered\nthe precedence set by t/test-lib.sh of using `tput`, so I went with\nthat (since I'm old-school). The ANSI-only version was, of course,\nmuch simpler.\n\n[1]: https://lore.kernel.org/git/pull.1324.git.git.1663023888412.gitgitgadget@gmail.com/\n"},{"id":"462954","messageId":"Yx/JCAfSB4Bv7BPw@coredump.intra.peff.net","threadId":"58385","inReplyTo":"CAPig+cTmosgapa=iUir3-J9k3138DvecHkmX+0QeHJROeTCeeA@mail.gmail.com","subject":"Re: [PATCH 00/18] make test \"linting\" more comprehensive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-13T00:04:24Z","receivedAt":"2022-09-13T00:04:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 12, 2022 at 07:17:12PM -0400, Eric Sunshine wrote:\n\n> > I think we can live with it as-is for now and see how people react. If\n> > lots of people are getting confused by the output, then that motivates\n> > finding a solution. If not, then it's probably not worth the time.\n> \n> Well, you nerd-sniped me anyhow. The result is at [1]. Following the\n\nIt seems we've discovered my true talent. :)\n\n> example of t/test-lib.sh, it uses `tput` if available to avoid\n> hardcoding color codes, and `tput` is invoked lazily, only if it\n> detects problems in the tests, so a normal (non-problematic) run\n> doesn't incur the overhead of shelling out to `tput`.\n\nAh, of course. I didn't think about the fact that the regular tests\nalready had to deal with this problem. Following that lead makes perfect\nsense.\n\n-Peff\n"},{"id":"467654","messageId":"CAPig+cTge7kp9bH+Xd8wpqmEZuuEFE0xQdgqaFP1WAQ-F+xyHA@mail.gmail.com","threadId":"58385","inReplyTo":"YxfXQ0IJjq/FT2Uh@coredump.intra.peff.net","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-21T04:02:54Z","receivedAt":"2022-11-21T04:03:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 6, 2022 at 7:27 PM Jeff King wrote:\n> I did some timings the other night, and I found something quite curious\n> with the thread stuff.\n>\n> I was quite surprised that it made things slower! It's nice that we're\n> only calling it once per script instead of once per test, but it seems\n> the startup overhead of the script is really high.\n>\n> And since in this mode we're only feeding it one script at a time, I\n> tried reverting the \"chainlint.pl: validate test scripts in parallel\"\n> commit. And indeed, now things are much faster:\n>\n> Benchmark 1: make\n> Time (mean ± σ): 61.544 s ± 3.364 s [User: 556.486 s, System: 384.001 s]\n> Range (min … max): 57.660 s … 63.490 s 3 runs\n>\n> And you can see the same thing just running chainlint by itself:\n>\n> $ time perl chainlint.pl /dev/null\n> real 0m0.069s\n> user 0m0.042s\n> sys 0m0.020s\n>\n> $ git revert HEAD^{/validate.test.scripts.in.parallel}\n> $ time perl chainlint.pl /dev/null\n> real 0m0.014s\n> user 0m0.010s\n> sys 0m0.004s\n>\n> I didn't track down the source of the slowness. Maybe it's loading extra\n> modules, or maybe it's opening /proc/cpuinfo, or maybe it's the thread\n> setup. But it's a surprising slowdown.\n\nIt is surprising, and unfortunate. Ditching \"ithreads\" would probably\nbe a good idea. (more on that below)\n\n> Now of course your intent is to do a single repo-wide invocation. And\n> that is indeed a bit faster. Here it is without the parallel code:\n>\n> Benchmark 1: make\n> Time (mean ± σ): 61.727 s ± 2.140 s [User: 507.712 s, System: 377.753 s]\n> Range (min … max): 59.259 s … 63.074 s 3 runs\n>\n> The wall-clock time didn't improve much, but the CPU time did. Restoring\n> the parallel code does improve the wall-clock time a bit, but at the\n> cost of some extra CPU:\n>\n> Benchmark 1: make\n> Time (mean ± σ): 59.029 s ± 2.851 s [User: 515.690 s, System: 380.369 s]\n> Range (min … max): 55.736 s … 60.693 s 3 runs\n>\n> which makes sense. If I do a with/without of just \"make test-chainlint\",\n> the parallelism is buying a few seconds of wall-clock:\n>\n> Benchmark 1: make test-chainlint\n> Time (mean ± σ): 900.1 ms ± 102.9 ms [User: 12049.8 ms, System: 79.7 ms]\n> Range (min … max): 704.2 ms … 994.4 ms 10 runs\n>\n> Benchmark 1: make test-chainlint\n> Time (mean ± σ): 3.778 s ± 0.042 s [User: 3.756 s, System: 0.023 s]\n> Range (min … max): 3.706 s … 3.833 s 10 runs\n>\n> I'm not sure what it all means. For Linux, I think I'd be just as happy\n> with a single non-parallelized test-chainlint run for each file. But\n> maybe on Windows the startup overhead is worse? OTOH, the whole test run\n> is so much worse there. One process per script is not going to be that\n> much in relative terms either way.\n\nSomehow Windows manages to be unbelievably slow no matter what. I\nmentioned elsewhere (after you sent this) that I tested on a five or\nsix year old 8-core dual-boot machine. Booted to Linux, running a\nsingle chainlint.pl invocation using all 8 cores to check all scripts\nin the project took under 1 second walltime. The same machine booted\nto Windows using all 8 cores took just under two minutes(!) walltime\nfor the single Perl invocation to check all scripts in the project.\n\nSo, at this point, I have no hope for making linting fast on Windows;\nit seems to be a lost cause.\n\n> And if we did cache the results and avoid extra invocations via \"make\",\n> then we'd want all the parallelism to move to there anyway.\n>\n> Maybe that gives you more food for thought about whether perl's \"use\n> threads\" is worth having.\n\nI'm not especially happy about the significant overhead of \"ithreads\";\non my (old) machine, although it does improve perceived time\nsignificantly, it eats up quite a bit of additional user-time. As\nsuch, I would not be unhappy to see \"ithreads\" go away, especially\nsince fast linting on Windows seems unattainable (at least with Perl).\n\nOverall, I think Ævar's plan to parallelize linting via \"make\" is\nprobably the way to go.\n"},{"id":"467685","messageId":"221121.86tu2sbfh8.gmgdl@evledraar.gmail.com","threadId":"58385","inReplyTo":"CAPig+cTge7kp9bH+Xd8wpqmEZuuEFE0xQdgqaFP1WAQ-F+xyHA@mail.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-21T13:28:11Z","receivedAt":"2022-11-21T13:32:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 20 2022, Eric Sunshine wrote:\n\n> On Tue, Sep 6, 2022 at 7:27 PM Jeff King wrote:\n>> I did some timings the other night, and I found something quite curious\n>> with the thread stuff.\n>>\n>> I was quite surprised that it made things slower! It's nice that we're\n>> only calling it once per script instead of once per test, but it seems\n>> the startup overhead of the script is really high.\n>>\n>> And since in this mode we're only feeding it one script at a time, I\n>> tried reverting the \"chainlint.pl: validate test scripts in parallel\"\n>> commit. And indeed, now things are much faster:\n>>\n>> Benchmark 1: make\n>> Time (mean ± σ): 61.544 s ± 3.364 s [User: 556.486 s, System: 384.001 s]\n>> Range (min … max): 57.660 s … 63.490 s 3 runs\n>>\n>> And you can see the same thing just running chainlint by itself:\n>>\n>> $ time perl chainlint.pl /dev/null\n>> real 0m0.069s\n>> user 0m0.042s\n>> sys 0m0.020s\n>>\n>> $ git revert HEAD^{/validate.test.scripts.in.parallel}\n>> $ time perl chainlint.pl /dev/null\n>> real 0m0.014s\n>> user 0m0.010s\n>> sys 0m0.004s\n>>\n>> I didn't track down the source of the slowness. Maybe it's loading extra\n>> modules, or maybe it's opening /proc/cpuinfo, or maybe it's the thread\n>> setup. But it's a surprising slowdown.\n>\n> It is surprising, and unfortunate. Ditching \"ithreads\" would probably\n> be a good idea. (more on that below)\n>\n>> Now of course your intent is to do a single repo-wide invocation. And\n>> that is indeed a bit faster. Here it is without the parallel code:\n>>\n>> Benchmark 1: make\n>> Time (mean ± σ): 61.727 s ± 2.140 s [User: 507.712 s, System: 377.753 s]\n>> Range (min … max): 59.259 s … 63.074 s 3 runs\n>>\n>> The wall-clock time didn't improve much, but the CPU time did. Restoring\n>> the parallel code does improve the wall-clock time a bit, but at the\n>> cost of some extra CPU:\n>>\n>> Benchmark 1: make\n>> Time (mean ± σ): 59.029 s ± 2.851 s [User: 515.690 s, System: 380.369 s]\n>> Range (min … max): 55.736 s … 60.693 s 3 runs\n>>\n>> which makes sense. If I do a with/without of just \"make test-chainlint\",\n>> the parallelism is buying a few seconds of wall-clock:\n>>\n>> Benchmark 1: make test-chainlint\n>> Time (mean ± σ): 900.1 ms ± 102.9 ms [User: 12049.8 ms, System: 79.7 ms]\n>> Range (min … max): 704.2 ms … 994.4 ms 10 runs\n>>\n>> Benchmark 1: make test-chainlint\n>> Time (mean ± σ): 3.778 s ± 0.042 s [User: 3.756 s, System: 0.023 s]\n>> Range (min … max): 3.706 s … 3.833 s 10 runs\n>>\n>> I'm not sure what it all means. For Linux, I think I'd be just as happy\n>> with a single non-parallelized test-chainlint run for each file. But\n>> maybe on Windows the startup overhead is worse? OTOH, the whole test run\n>> is so much worse there. One process per script is not going to be that\n>> much in relative terms either way.\n>\n> Somehow Windows manages to be unbelievably slow no matter what. I\n> mentioned elsewhere (after you sent this) that I tested on a five or\n> six year old 8-core dual-boot machine. Booted to Linux, running a\n> single chainlint.pl invocation using all 8 cores to check all scripts\n> in the project took under 1 second walltime. The same machine booted\n> to Windows using all 8 cores took just under two minutes(!) walltime\n> for the single Perl invocation to check all scripts in the project.\n>\n> So, at this point, I have no hope for making linting fast on Windows;\n> it seems to be a lost cause.\n\nI'd be really interested in seeing e.g. the NYTProf output for that run,\ncompared with that on *nix (if you could upload the HTML versions of\nboth somewhere, even better).\n\nMaybe \"chainlint.pl\" is doing something odd, but this goes against the\nusual wisdom about what is and isn't slow in Perl on windows, as I\nunderstand it.\n\nI.e. process star-up etc. is slow there, and I/O's a bit slower, but\nonce you're started up and e.g. slurping up all of those files & parsing\nthem you're just running \"perl-native\" code.\n\nWhich shouldn't be much slower at all. A perl compiled with ithreads is\n(last I checked) around 10-20% slower, and the Windows version is always\ncompiled with that (it's needed for \"fork\" emulation).\n\nBut most *nix versions are compiled with that too, and certainly the one\nyou're using with \"threads\", so that's not the difference.\n\nSo I suspect something odd's going on...\n\n>> And if we did cache the results and avoid extra invocations via \"make\",\n>> then we'd want all the parallelism to move to there anyway.\n>>\n>> Maybe that gives you more food for thought about whether perl's \"use\n>> threads\" is worth having.\n>\n> I'm not especially happy about the significant overhead of \"ithreads\";\n> on my (old) machine, although it does improve perceived time\n> significantly, it eats up quite a bit of additional user-time. As\n> such, I would not be unhappy to see \"ithreads\" go away, especially\n> since fast linting on Windows seems unattainable (at least with Perl).\n>\n> Overall, I think Ævar's plan to parallelize linting via \"make\" is\n> probably the way to go.\n\nYeah, but that seems to me to be orthagonal to why it's this slow on\nWindows, and if it is that wouldn't help much, except for incremental\nre-runs.\n"},{"id":"467690","messageId":"CAPig+cS3Ui=SFuRLPKKugT9RFvtUV3FmO23Wse_Rhih5hgbPmg@mail.gmail.com","threadId":"58385","inReplyTo":"221121.86tu2sbfh8.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-21T14:07:33Z","receivedAt":"2022-11-21T14:11:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 21, 2022 at 8:32 AM Ævar Arnfjörð Bjarmason\n wrote:\n> On Sun, Nov 20 2022, Eric Sunshine wrote:\n> > Somehow Windows manages to be unbelievably slow no matter what. I\n> > mentioned elsewhere (after you sent this) that I tested on a five or\n> > six year old 8-core dual-boot machine. Booted to Linux, running a\n> > single chainlint.pl invocation using all 8 cores to check all scripts\n> > in the project took under 1 second walltime. The same machine booted\n> > to Windows using all 8 cores took just under two minutes(!) walltime\n> > for the single Perl invocation to check all scripts in the project.\n>\n> I'd be really interested in seeing e.g. the NYTProf output for that run,\n> compared with that on *nix (if you could upload the HTML versions of\n> both somewhere, even better).\n\nUnfortunately, I no longer have access to that machine, or usable\nWindows in general. Of course, someone else with access to a dual-boot\nmachine could generate such a report, but whether anyone will offer to\ndo so is a different matter.\n\n> Maybe \"chainlint.pl\" is doing something odd, but this goes against the\n> usual wisdom about what is and isn't slow in Perl on windows, as I\n> understand it.\n>\n> I.e. process star-up etc. is slow there, and I/O's a bit slower, but\n> once you're started up and e.g. slurping up all of those files & parsing\n> them you're just running \"perl-native\" code.\n>\n> Which shouldn't be much slower at all. A perl compiled with ithreads is\n> (last I checked) around 10-20% slower, and the Windows version is always\n> compiled with that (it's needed for \"fork\" emulation).\n>\n> But most *nix versions are compiled with that too, and certainly the one\n> you're using with \"threads\", so that's not the difference.\n>\n> So I suspect something odd's going on...\n\nThis is all my understanding, as well, which is why I was so surprised\nby the difference in speed. Aside from suspecting Windows I/O as the\nculprit, another obvious possible culprit would be whatever\nmechanism/primitives \"ithreads\" is using on Windows for\nlocking/synchronizing and passing messages between threads. I wouldn't\nbe surprised to learn that those mechanisms/primitives have very high\noverhead on that platform.\n\n> > Overall, I think Ævar's plan to parallelize linting via \"make\" is\n> > probably the way to go.\n>\n> Yeah, but that seems to me to be orthagonal to why it's this slow on\n> Windows, and if it is that wouldn't help much, except for incremental\n> re-runs.\n\nOh, I didn't at all mean that `make` parallelism would be helpful on\nWindows; I can't imagine that it ever would be (though I could once\nagain be wrong). What I meant was that `make` parallelism would be a\nnice improvement and simplification (of sorts), in general,\nconsidering that I've given up hope of ever seeing linting be speedy\non Windows.\n"},{"id":"467692","messageId":"221121.86leo4bd91.gmgdl@evledraar.gmail.com","threadId":"58385","inReplyTo":"CAPig+cS3Ui=SFuRLPKKugT9RFvtUV3FmO23Wse_Rhih5hgbPmg@mail.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-21T14:18:49Z","receivedAt":"2022-11-21T14:20:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 21 2022, Eric Sunshine wrote:\n\n> On Mon, Nov 21, 2022 at 8:32 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> On Sun, Nov 20 2022, Eric Sunshine wrote:\n>> > Somehow Windows manages to be unbelievably slow no matter what. I\n>> > mentioned elsewhere (after you sent this) that I tested on a five or\n>> > six year old 8-core dual-boot machine. Booted to Linux, running a\n>> > single chainlint.pl invocation using all 8 cores to check all scripts\n>> > in the project took under 1 second walltime. The same machine booted\n>> > to Windows using all 8 cores took just under two minutes(!) walltime\n>> > for the single Perl invocation to check all scripts in the project.\n>>\n>> I'd be really interested in seeing e.g. the NYTProf output for that run,\n>> compared with that on *nix (if you could upload the HTML versions of\n>> both somewhere, even better).\n>\n> Unfortunately, I no longer have access to that machine, or usable\n> Windows in general. Of course, someone else with access to a dual-boot\n> machine could generate such a report, but whether anyone will offer to\n> do so is a different matter.\n\n:(\n\n>> Maybe \"chainlint.pl\" is doing something odd, but this goes against the\n>> usual wisdom about what is and isn't slow in Perl on windows, as I\n>> understand it.\n>>\n>> I.e. process star-up etc. is slow there, and I/O's a bit slower, but\n>> once you're started up and e.g. slurping up all of those files & parsing\n>> them you're just running \"perl-native\" code.\n>>\n>> Which shouldn't be much slower at all. A perl compiled with ithreads is\n>> (last I checked) around 10-20% slower, and the Windows version is always\n>> compiled with that (it's needed for \"fork\" emulation).\n>>\n>> But most *nix versions are compiled with that too, and certainly the one\n>> you're using with \"threads\", so that's not the difference.\n>>\n>> So I suspect something odd's going on...\n>\n> This is all my understanding, as well, which is why I was so surprised\n> by the difference in speed. Aside from suspecting Windows I/O as the\n> culprit, another obvious possible culprit would be whatever\n> mechanism/primitives \"ithreads\" is using on Windows for\n> locking/synchronizing and passing messages between threads. I wouldn't\n> be surprised to learn that those mechanisms/primitives have very high\n> overhead on that platform.\n\nYeah, that could be, but then...\n\n>> > Overall, I think Ævar's plan to parallelize linting via \"make\" is\n>> > probably the way to go.\n>>\n>> Yeah, but that seems to me to be orthagonal to why it's this slow on\n>> Windows, and if it is that wouldn't help much, except for incremental\n>> re-runs.\n>\n> Oh, I didn't at all mean that `make` parallelism would be helpful on\n> Windows; I can't imagine that it ever would be (though I could once\n> again be wrong). What I meant was that `make` parallelism would be a\n> nice improvement and simplification (of sorts), in general,\n> considering that I've given up hope of ever seeing linting be speedy\n> on Windows.\n\n...that parallelism probably wouldn't be helpful, as it'll run into\nanother thing that's slow.\n\nBut just ditching the \"ithreads\" commit from chainlint.pl should make it\nmuch faster, as sequentially parsing all the files isn't that slow, and\nas that won't use threads should be much faster then.\n\n"},{"id":"467693","messageId":"CAPig+cTTWu+bO3xMZrrKCLxmtBeJoToP-eMdsin7qMm-BC07iw@mail.gmail.com","threadId":"58385","inReplyTo":"221121.86leo4bd91.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-21T14:48:13Z","receivedAt":"2022-11-21T14:58:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 21, 2022 at 9:20 AM Ævar Arnfjörð Bjarmason\n wrote:\n> On Mon, Nov 21 2022, Eric Sunshine wrote:\n> > Oh, I didn't at all mean that `make` parallelism would be helpful on\n> > Windows; I can't imagine that it ever would be (though I could once\n> > again be wrong). What I meant was that `make` parallelism would be a\n> > nice improvement and simplification (of sorts), in general,\n> > considering that I've given up hope of ever seeing linting be speedy\n> > on Windows.\n>\n> But just ditching the \"ithreads\" commit from chainlint.pl should make it\n> much faster, as sequentially parsing all the files isn't that slow, and\n> as that won't use threads should be much faster then.\n\nOn my (old) machine (with spinning hard drive), `make test-chainlint`\nwith \"ithreads\" and warm filesystem cache takes about 3.8 seconds\nwalltime. Without \"ithreads\", it takes about 11.3 seconds. So, the\nimprovement in perceived time is significant. As such, I'm somewhat\nhesitant to see \"ithreads\" dropped from chainlint.pl before `make`\nparallelism is implemented. (I can easily see \"drop ithreads\" as the\nfinal patch of a series which adds `make` parallelism.)\n\nBut perhaps I'm focussing too much on my own experience with my old\nmachine. Maybe linting without \"ithreads\" and without `make`\nparallelism would be \"fast enough\" for developers using beefier modern\nmachines... (genuine question/thought since I don't have access to any\nbeefy modern hardware).\n"},{"id":"467701","messageId":"Y3u9ul1cu+L5d5IZ@coredump.intra.peff.net","threadId":"58385","inReplyTo":"CAPig+cTge7kp9bH+Xd8wpqmEZuuEFE0xQdgqaFP1WAQ-F+xyHA@mail.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-21T18:04:42Z","receivedAt":"2022-11-21T18:04:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 20, 2022 at 11:02:54PM -0500, Eric Sunshine wrote:\n\n> > And if we did cache the results and avoid extra invocations via \"make\",\n> > then we'd want all the parallelism to move to there anyway.\n> >\n> > Maybe that gives you more food for thought about whether perl's \"use\n> > threads\" is worth having.\n> \n> I'm not especially happy about the significant overhead of \"ithreads\";\n> on my (old) machine, although it does improve perceived time\n> significantly, it eats up quite a bit of additional user-time. As\n> such, I would not be unhappy to see \"ithreads\" go away, especially\n> since fast linting on Windows seems unattainable (at least with Perl).\n> \n> Overall, I think Ævar's plan to parallelize linting via \"make\" is\n> probably the way to go.\n\nTBH, I think just running the linter once per test script when the\nscript is run would be sufficient. That is one extra process per script,\nbut they are already shell scripts running a bunch of processes. You get\nparallelism for free because you're already running the tests in\nparallel. You lose out on \"don't bother linting because the file hasn't\nchanged\", but I'm not sure that's really worth the extra complexity\noverall.\n\n-Peff\n"},{"id":"467706","messageId":"CAPig+cQfkkY2Eh=QD47QoUGuAiCEpxSsX24x_8ts2GTKVnV1aw@mail.gmail.com","threadId":"58385","inReplyTo":"Y3u9ul1cu+L5d5IZ@coredump.intra.peff.net","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-21T18:47:42Z","receivedAt":"2022-11-21T18:48:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 21, 2022 at 1:04 PM Jeff King wrote:\n> On Sun, Nov 20, 2022 at 11:02:54PM -0500, Eric Sunshine wrote:\n> > Overall, I think Ævar's plan to parallelize linting via \"make\" is\n> > probably the way to go.\n>\n> TBH, I think just running the linter once per test script when the\n> script is run would be sufficient. That is one extra process per script,\n> but they are already shell scripts running a bunch of processes. You get\n> parallelism for free because you're already running the tests in\n> parallel. You lose out on \"don't bother linting because the file hasn't\n> changed\", but I'm not sure that's really worth the extra complexity\n> overall.\n\nHmm, yes, that's appealing (especially since I've essentially given up\non making linting fast on Windows), and it wouldn't be hard to\nimplement. In fact, it's already implemented by 23a14f3016 (test-lib:\nreplace chainlint.sed with chainlint.pl, 2022-09-01); making it work\nthe way you describe would just involve dropping 69b9924b87\n(t/Makefile: teach `make test` and `make prove` to run chainlint.pl,\n2022-09-01) and 29fb2ec384 (chainlint.pl: validate test scripts in\nparallel, 2022-09-01).\n\nI think Ævar's use-case for `make` parallelization was to speed up\ngit-bisect runs. But thinking about it now, the likelihood of \"lint\"\nproblems cropping up during a git-bisect run is effectively nil, in\nwhich case setting GIT_TEST_CHAIN_LINT=1 should be a perfectly\nappropriate way to take linting out of the equation when bisecting.\n"},{"id":"467707","messageId":"CAPig+cRSoaZJLv3PxT=TKU-Qy8twhAzhgncB=UL5sasgSB3Hiw@mail.gmail.com","threadId":"58385","inReplyTo":"CAPig+cQfkkY2Eh=QD47QoUGuAiCEpxSsX24x_8ts2GTKVnV1aw@mail.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-21T18:50:06Z","receivedAt":"2022-11-21T18:50:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 21, 2022 at 1:47 PM Eric Sunshine wrote:\n> I think Ævar's use-case for `make` parallelization was to speed up\n> git-bisect runs. But thinking about it now, the likelihood of \"lint\"\n> problems cropping up during a git-bisect run is effectively nil, in\n> which case setting GIT_TEST_CHAIN_LINT=1 should be a perfectly\n> appropriate way to take linting out of the equation when bisecting.\n\nI mean \"GIT_TEST_CHAIN_LINT=0\", of course.\n"},{"id":"467709","messageId":"Y3vI99ZiNdXddX8C@coredump.intra.peff.net","threadId":"58385","inReplyTo":"CAPig+cQfkkY2Eh=QD47QoUGuAiCEpxSsX24x_8ts2GTKVnV1aw@mail.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-21T18:52:39Z","receivedAt":"2022-11-21T18:52:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 21, 2022 at 01:47:42PM -0500, Eric Sunshine wrote:\n\n> On Mon, Nov 21, 2022 at 1:04 PM Jeff King wrote:\n> > On Sun, Nov 20, 2022 at 11:02:54PM -0500, Eric Sunshine wrote:\n> > > Overall, I think Ævar's plan to parallelize linting via \"make\" is\n> > > probably the way to go.\n> >\n> > TBH, I think just running the linter once per test script when the\n> > script is run would be sufficient. That is one extra process per script,\n> > but they are already shell scripts running a bunch of processes. You get\n> > parallelism for free because you're already running the tests in\n> > parallel. You lose out on \"don't bother linting because the file hasn't\n> > changed\", but I'm not sure that's really worth the extra complexity\n> > overall.\n> \n> Hmm, yes, that's appealing (especially since I've essentially given up\n> on making linting fast on Windows), and it wouldn't be hard to\n> implement. In fact, it's already implemented by 23a14f3016 (test-lib:\n> replace chainlint.sed with chainlint.pl, 2022-09-01); making it work\n> the way you describe would just involve dropping 69b9924b87\n> (t/Makefile: teach `make test` and `make prove` to run chainlint.pl,\n> 2022-09-01) and 29fb2ec384 (chainlint.pl: validate test scripts in\n> parallel, 2022-09-01).\n\nYes, that was one of the modes I timed in my original email. :)\n\n> I think Ævar's use-case for `make` parallelization was to speed up\n> git-bisect runs. But thinking about it now, the likelihood of \"lint\"\n> problems cropping up during a git-bisect run is effectively nil, in\n> which case setting GIT_TEST_CHAIN_LINT=1 should be a perfectly\n> appropriate way to take linting out of the equation when bisecting.\n\nYes. It's also dumb to run a straight \"make test\" while bisecting in the\nfirst place, because you are going to run a zillion tests that aren't\nrelevant to your bisection. Bisecting on \"cd t && ./test-that-fails\" is\nfaster, at which point you're only running the one lint process (and if\nit really bothers you, you can disable chain lint as you suggest).\n\n-Peff\n"},{"id":"467710","messageId":"CAPig+cQEdidB4YHm9OiyOUe8mbTPBajjX5t-_6ZJVwRykXkqmg@mail.gmail.com","threadId":"58385","inReplyTo":"Y3vI99ZiNdXddX8C@coredump.intra.peff.net","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-21T19:00:41Z","receivedAt":"2022-11-21T19:00:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 21, 2022 at 1:52 PM Jeff King wrote:\n> On Mon, Nov 21, 2022 at 01:47:42PM -0500, Eric Sunshine wrote:\n> > I think Ævar's use-case for `make` parallelization was to speed up\n> > git-bisect runs. But thinking about it now, the likelihood of \"lint\"\n> > problems cropping up during a git-bisect run is effectively nil, in\n> > which case setting GIT_TEST_CHAIN_LINT=1 should be a perfectly\n> > appropriate way to take linting out of the equation when bisecting.\n>\n> Yes. It's also dumb to run a straight \"make test\" while bisecting in the\n> first place, because you are going to run a zillion tests that aren't\n> relevant to your bisection. Bisecting on \"cd t && ./test-that-fails\" is\n> faster, at which point you're only running the one lint process (and if\n> it really bothers you, you can disable chain lint as you suggest).\n\nI think I misspoke. Dredging up old memories, I think Ævar's use-case\nis that he now runs:\n\n git rebase -i --exec 'make test' ...\n\nin order to ensure that the entire test suite passes for _every_ patch\nin a series. (This is due to him having missed a runtime breakage by\nonly running \"make test\" after the final patch in a series was\napplied, when the breakage was only temporary -- added by one patch,\nbut resolved by some other later patch.)\n\nEven so, GIT_TEST_CHAIN_LINT=0 should be appropriate here too.\n"},{"id":"467719","messageId":"Y3vRSBptFTR+AV1f@coredump.intra.peff.net","threadId":"58385","inReplyTo":"CAPig+cQEdidB4YHm9OiyOUe8mbTPBajjX5t-_6ZJVwRykXkqmg@mail.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-21T19:28:08Z","receivedAt":"2022-11-21T19:28:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 21, 2022 at 02:00:41PM -0500, Eric Sunshine wrote:\n\n> On Mon, Nov 21, 2022 at 1:52 PM Jeff King wrote:\n> > On Mon, Nov 21, 2022 at 01:47:42PM -0500, Eric Sunshine wrote:\n> > > I think Ævar's use-case for `make` parallelization was to speed up\n> > > git-bisect runs. But thinking about it now, the likelihood of \"lint\"\n> > > problems cropping up during a git-bisect run is effectively nil, in\n> > > which case setting GIT_TEST_CHAIN_LINT=1 should be a perfectly\n> > > appropriate way to take linting out of the equation when bisecting.\n> >\n> > Yes. It's also dumb to run a straight \"make test\" while bisecting in the\n> > first place, because you are going to run a zillion tests that aren't\n> > relevant to your bisection. Bisecting on \"cd t && ./test-that-fails\" is\n> > faster, at which point you're only running the one lint process (and if\n> > it really bothers you, you can disable chain lint as you suggest).\n> \n> I think I misspoke. Dredging up old memories, I think Ævar's use-case\n> is that he now runs:\n> \n> git rebase -i --exec 'make test' ...\n> \n> in order to ensure that the entire test suite passes for _every_ patch\n> in a series. (This is due to him having missed a runtime breakage by\n> only running \"make test\" after the final patch in a series was\n> applied, when the breakage was only temporary -- added by one patch,\n> but resolved by some other later patch.)\n\nYeah, I do that sometimes, too, especially when heavy refactoring is\ninvolved.\n\n> Even so, GIT_TEST_CHAIN_LINT=0 should be appropriate here too.\n\nAgreed. But also, my original point stands. If you are running 10 CPU\nminutes of tests, then a few CPU seconds of linting is not really that\nimportant.\n\n-Peff\n"},{"id":"467734","messageId":"221122.86cz9fbyln.gmgdl@evledraar.gmail.com","threadId":"58385","inReplyTo":"CAPig+cQEdidB4YHm9OiyOUe8mbTPBajjX5t-_6ZJVwRykXkqmg@mail.gmail.com","subject":"Re: [PATCH 06/18] chainlint.pl: validate test scripts in parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-22T00:11:39Z","receivedAt":"2022-11-22T00:51:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 21 2022, Eric Sunshine wrote:\n\n> On Mon, Nov 21, 2022 at 1:52 PM Jeff King wrote:\n>> On Mon, Nov 21, 2022 at 01:47:42PM -0500, Eric Sunshine wrote:\n>> > I think Ævar's use-case for `make` parallelization was to speed up\n>> > git-bisect runs. But thinking about it now, the likelihood of \"lint\"\n>> > problems cropping up during a git-bisect run is effectively nil, in\n>> > which case setting GIT_TEST_CHAIN_LINT=1 should be a perfectly\n>> > appropriate way to take linting out of the equation when bisecting.\n>>\n>> Yes. It's also dumb to run a straight \"make test\" while bisecting in the\n>> first place, because you are going to run a zillion tests that aren't\n>> relevant to your bisection. Bisecting on \"cd t && ./test-that-fails\" is\n>> faster, at which point you're only running the one lint process (and if\n>> it really bothers you, you can disable chain lint as you suggest).\n>\n> I think I misspoke. Dredging up old memories, I think Ævar's use-case\n> is that he now runs:\n>\n> git rebase -i --exec 'make test' ...\n>\n> in order to ensure that the entire test suite passes for _every_ patch\n> in a series. (This is due to him having missed a runtime breakage by\n> only running \"make test\" after the final patch in a series was\n> applied, when the breakage was only temporary -- added by one patch,\n> but resolved by some other later patch.)\n>\n> Even so, GIT_TEST_CHAIN_LINT=0 should be appropriate here too.\n\nI'd like to make \"make\" fast in terms of avoiding its own overhead\nbefore it gets to actual work mainly because of that use-case, but it\nhelps in general. E.g. if you switch branches we don't compile a file we\ndon't need to, we shouldn't re-run test checks we don't need either.\n\nFor t/ this is:\n\n - Running chainlint.pl on the file, even if it didn't change\n - Ditto check-non-portable-shell.pl\n - Ditto \"non-portable file name(s)\" check\n - Ditto \"test -x\" on all test files\n\nI have a branch where these are all checked using dependencies instead,\ne.g. we run a \"test -x\" on t0071-sort.sh and create a\n\".build/check-executable/t0071-sort.sh.ok\" if that passed, we don't need\nto shell out in the common case.\n\nThe results of that are, and this is a best case in picking one where\nthe test itself is cheap:\n\t\n\t$ git hyperfine -L rev @{u},HEAD~,HEAD -s 'make CFLAGS=-O3' 'make test T=t0071-sort.sh' -w 1\n\tBenchmark 1: make test T=t0071-sort.sh' in '@{u}\n\t Time (mean ± σ): 1.168 s ± 0.074 s [User: 1.534 s, System: 0.082 s]\n\t Range (min … max): 1.096 s … 1.316 s 10 runs\n\t\n\tBenchmark 2: make test T=t0071-sort.sh' in 'HEAD~\n\t Time (mean ± σ): 719.1 ms ± 46.1 ms [User: 910.6 ms, System: 79.7 ms]\n\t Range (min … max): 682.0 ms … 828.2 ms 10 runs\n\t\n\tBenchmark 3: make test T=t0071-sort.sh' in 'HEAD\n\t Time (mean ± σ): 685.0 ms ± 34.2 ms [User: 645.0 ms, System: 56.8 ms]\n\t Range (min … max): 657.6 ms … 773.6 ms 10 runs\n\t\n\tSummary\n\t 'make test T=t0071-sort.sh' in 'HEAD' ran\n\t 1.05 ± 0.09 times faster than 'make test T=t0071-sort.sh' in 'HEAD~'\n\t 1.71 ± 0.14 times faster than 'make test T=t0071-sort.sh' in '@{u}'\n\nThe @{u} being \"master\", HEAD~ is \"incremant without chainlint.pl\", and\n\"HEAD\" is where it's all incremental.\n\nIt's very WIP-quality, but I pushed the chainlint.pl part of it as a POC\njust now, I did the others a while ago:\nhttps://github.com/avar/git/tree/avar/t-Makefile-break-T-to-file-association\n\n"}]}