Re: [PATCH 00/19] tests: fix broken &&-chains & abort loops on error
- From
Elijah Newren <newren@gmail.com>
- Date
- Dec 9, 2021, 17:02 UTC
- Message-ID
- <CABPp-BFM5ZbFAzVfvDE3=zm6Q4LN2fWthPP8WH5kbgVPSxomtA@mail.gmail.com>
- In-Reply-To
- <20211209051115.52629-1-sunshine@sunshineco.com>
On Wed, Dec 8, 2021 at 11:39 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 47 quoted lines
>
> A while back, Peff successfully nerd-sniped[1] me into tackling a
> long-brewing idea I had about (possibly) improving `chainlint`
> performance by linting all tests in all scripts with a single command
> invocation instead of running `sed` 25300+ times (once for each test).
>
> Beside the obvious improvement of checking all tests in all scripts at
> once, the new linter is also a good deal smarter than chainlint.sed and
> understands not just shell syntax but also some semantics of test
> construction, unlike chainlint.sed which is merely heuristics-based.
> For instance, the new linter recognizes cases when a broken &&-chain is
> legitimate, such as when `$?` is handled explicitly or when a failure is
> signaled directly with `false`, in which case the &&-chain leading up to
> the `false` is immaterial, as well as other cases. Unlike chainlint.sed,
> it recognizes that a semicolon after the last command in a compound
> statement is harmless, thus won't interpret the semicolon as breaking
> the &&-chain.
>
> The new linter also provides considerably better coverage for broken
> &&-chains. The &&-chain checker built into t/test-lib.sh, which causes
> tests to magically exit with code 117 if the &&-chain is broken, only
> works for top-level command invocations. The magic doesn't work within
> `{...}` groups, `(...)` subshells, `$(...)` substitutions, or within
> bodies of compound statements, such as `if`, `for`, `while`, `case`,
> etc. chainlint.sed partly fills the gap by catching broken &&-chains in
> `(...)` subshells one level deep, but bugs can still lurk behind broken
> &&-chains in the other cases. The new linter catches broken &&-chains
> within all those constructs to any depth.
>
> Another important improvement is that the new linter understands that
> shell loops do not terminate automatically when a command in the loop
> body fails, and that the condition needs to be handled explicitly by the
> test author by using `|| return 1` (or `|| exit 1` in a subshell) within
> the loop body to flag the failure. Consequently, the new linter will
> complain when a loop is lacking `|| return 1` (or `|| exit 1`). There
> are a number of other improvements, as well, but the above are some of
> the more important ones.
>
> Although the new chainlint implementation has been complete for several
> months, I'm still working out how to organize its patch series. In the
> meantime, _this_ patch series fixes problems discovered by the new
> linter due to its improved coverage and extra semantic knowledge about
> Git tests. As much as possible, I resisted the temptation to make
> ancillary cleanups (including indentation fixes) to tests even when such
> cleanups would be obvious improvements. Avoiding such unrelated cleanups
> should make the long patches in this series, which touch a lot of tests,
> easier to review (--color-words helps a lot here).I have to admit to starting to skim once I got to the last four patches, since they were a bit longer and all the same type of change.
You did an excellent job of explaining the changes and presented them in a logical fashion. The few things I thought I caught, you've already answered were already correct. I do think making the second commit message be a bit clearer about the importance of the ordering would be helpful. Anyway:
Reviewed-by: Elijah Newren <newren@gmail.com>
Show 211 quoted lines
> This series merges cleanly with 'next' but conflicts with a couple topics
> in 'seen':
>
> * jh/builtin-fsmonitor-part2
>
> t/perf/p7519-fsmonitor.sh
> simple resolution: keep all changes from jh/builtin-fsmonitor-part2
> (it obviates the need for the fixes made by this series)
>
> * ms/customizable-ident-expansion
>
> t/t0021-conversion.sh
> this is a messy conflict but resolution is simple enough: keep all
> the changes made by ms/customizable-ident-expansion and throw away
> the changes by this series; this will leave a few broken &&-chains
> in t0021-conversion.sh but there are a few other topics in 'seen'
> with such problems already, so it has company; anyhow, "What's
> Cooking" indicates that ms/customizable-ident-expansion is going to
> be discarded, so it may not be worth worrying about it
>
> [1]: https://lore.kernel.org/git/YJzGcZpZ+E9R0gYd@coredump.intra.peff.net/
>
> Eric Sunshine (19):
> t/lib-pager: use sane_unset() to avoid breaking &&-chain
> t1010: fix unnoticed failure on Windows
> t1020: avoid aborting entire test script when one test fails
> t4202: clarify intent by creating expected content less cleverly
> t5516: drop unnecessary subshell and command invocation
> t6300: make `%(raw:size) --shell` test more robust
> t9107: use shell parameter expansion to avoid breaking &&-chain
> tests: simplify construction of large blocks of text
> tests: use test_write_lines() to generate line-oriented output
> tests: fix broken &&-chains in compound statements
> tests: fix broken &&-chains in `$(...)` command substitutions
> tests: fix broken &&-chains in `{...}` groups
> tests: apply modern idiom for signaling test failure
> tests: apply modern idiom for exiting loop upon failure
> tests: simplify by dropping unnecessary `for` loops
> t0000-t3999: detect and signal failure within loop
> t4000-t4999: detect and signal failure within loop
> t5000-t5999: detect and signal failure within loop
> t6000-t9999: detect and signal failure within loop
>
> .../mw-to-git/t/t9365-continuing-queries.sh | 2 +-
> contrib/subtree/t/t7900-subtree.sh | 2 +-
> t/annotate-tests.sh | 2 +-
> t/lib-pager.sh | 2 +-
> t/perf/p0005-status.sh | 12 +-
> t/perf/p0006-read-tree-checkout.sh | 20 +-
> t/perf/p0007-write-cache.sh | 4 +-
> t/perf/p0100-globbing.sh | 4 +-
> t/perf/p1400-update-ref.sh | 4 +-
> t/perf/p1451-fsck-skip-list.sh | 2 +-
> t/perf/p3400-rebase.sh | 2 +-
> t/perf/p5302-pack-index.sh | 4 +-
> t/perf/p5303-many-packs.sh | 10 +-
> t/perf/p7519-fsmonitor.sh | 8 +-
> t/t0005-signals.sh | 2 +-
> t/t0008-ignores.sh | 2 +-
> t/t0011-hashmap.sh | 4 +-
> t/t0020-crlf.sh | 28 +-
> t/t0021-conversion.sh | 60 ++--
> t/t0026-eol-config.sh | 4 +-
> t/t0060-path-utils.sh | 4 +-
> t/t0069-oidtree.sh | 12 +-
> t/t0095-bloom.sh | 4 +-
> t/t0410-partial-clone.sh | 2 +-
> t/t1006-cat-file.sh | 12 +-
> t/t1010-mktree.sh | 4 +-
> t/t1020-subdirectory.sh | 10 +-
> t/t1050-large.sh | 34 +-
> t/t1091-sparse-checkout-builtin.sh | 2 +-
> t/t1300-config.sh | 6 +-
> t/t1400-update-ref.sh | 4 +-
> t/t1403-show-ref.sh | 12 +-
> t/t1410-reflog.sh | 4 +-
> t/t1512-rev-parse-disambiguation.sh | 12 +-
> t/t1700-split-index.sh | 4 +-
> t/t2004-checkout-cache-temp.sh | 4 +-
> t/t2012-checkout-last.sh | 4 +-
> t/t2102-update-index-symlinks.sh | 2 +-
> t/t2103-update-index-ignore-missing.sh | 2 +-
> t/t2200-add-update.sh | 18 +-
> t/t2201-add-update-typechange.sh | 10 +-
> t/t2203-add-intent.sh | 2 +-
> t/t3005-ls-files-relative.sh | 10 +-
> t/t3070-wildmatch.sh | 2 +-
> t/t3202-show-branch.sh | 8 +-
> t/t3303-notes-subtrees.sh | 6 +-
> t/t3305-notes-fanout.sh | 4 +-
> t/t3402-rebase-merge.sh | 8 +-
> t/t3404-rebase-interactive.sh | 4 +-
> t/t3417-rebase-whitespace-fix.sh | 4 +-
> t/t3501-revert-cherry-pick.sh | 2 +-
> t/t3508-cherry-pick-many-commits.sh | 2 +-
> t/t3600-rm.sh | 7 +-
> t/t3700-add.sh | 8 +-
> t/t3920-crlf-messages.sh | 4 +-
> t/t4001-diff-rename.sh | 2 +-
> t/t4012-diff-binary.sh | 2 +-
> t/t4013-diff-various.sh | 22 +-
> t/t4014-format-patch.sh | 32 +-
> t/t4015-diff-whitespace.sh | 4 +-
> t/t4018-diff-funcname.sh | 2 +-
> t/t4019-diff-wserror.sh | 4 +-
> t/t4023-diff-rename-typechange.sh | 6 +-
> t/t4024-diff-optimize-common.sh | 2 +-
> t/t4025-hunk-header.sh | 10 +-
> t/t4038-diff-combined.sh | 2 +-
> t/t4046-diff-unmerged.sh | 10 +-
> t/t4049-diff-stat-count.sh | 2 +-
> t/t4052-stat-output.sh | 2 +-
> t/t4057-diff-combined-paths.sh | 16 +-
> t/t4105-apply-fuzz.sh | 10 +-
> t/t4106-apply-stdin.sh | 5 +-
> t/t4116-apply-reverse.sh | 4 +-
> t/t4117-apply-reject.sh | 20 +-
> t/t4118-apply-empty-context.sh | 6 +-
> t/t4123-apply-shrink.sh | 4 +-
> t/t4124-apply-ws-rule.sh | 58 ++--
> t/t4125-apply-ws-fuzz.sh | 5 +-
> t/t4126-apply-empty.sh | 5 +-
> t/t4127-apply-same-fn.sh | 5 +-
> t/t4138-apply-ws-expansion.sh | 36 +-
> t/t4150-am.sh | 2 +-
> t/t4151-am-abort.sh | 10 +-
> t/t4202-log.sh | 42 +--
> t/t4205-log-pretty-formats.sh | 2 +-
> t/t4211-line-log.sh | 2 +-
> t/t4212-log-corrupt.sh | 8 +-
> t/t4216-log-bloom.sh | 4 +-
> t/t5000-tar-tree.sh | 4 +-
> t/t5003-archive-zip.sh | 2 +-
> t/t5004-archive-corner-cases.sh | 6 +-
> t/t5100-mailinfo.sh | 2 +-
> t/t5300-pack-object.sh | 18 +-
> t/t5302-pack-index.sh | 2 +-
> t/t5306-pack-nobase.sh | 2 +-
> t/t5307-pack-missing-commit.sh | 2 +-
> t/t5310-pack-bitmaps.sh | 2 +-
> t/t5316-pack-delta-depth.sh | 7 +-
> t/t5317-pack-objects-filter-objects.sh | 30 +-
> t/t5318-commit-graph.sh | 6 +-
> t/t5319-multi-pack-index.sh | 10 +-
> t/t5322-pack-objects-sparse.sh | 4 +-
> t/t5325-reverse-index.sh | 2 +-
> t/t5500-fetch-pack.sh | 8 +-
> t/t5502-quickfetch.sh | 2 +-
> t/t5505-remote.sh | 6 +-
> t/t5510-fetch.sh | 14 +-
> t/t5515-fetch-merge-logic.sh | 38 +--
> t/t5516-fetch-push.sh | 5 +-
> t/t5552-skipping-fetch-negotiator.sh | 10 +-
> t/t5562-http-backend-content-length.sh | 2 +-
> t/t5570-git-daemon.sh | 2 +-
> t/t5571-pre-push-hook.sh | 6 +-
> t/t5611-clone-config.sh | 2 +-
> t/t5616-partial-clone.sh | 30 +-
> t/t5702-protocol-v2.sh | 4 +-
> t/t6005-rev-list-count.sh | 8 +-
> t/t6009-rev-list-parent.sh | 6 +-
> t/t6019-rev-list-ancestry-path.sh | 10 +-
> t/t6060-merge-index.sh | 4 +-
> t/t6101-rev-parse-parents.sh | 2 +-
> t/t6112-rev-list-filters-objects.sh | 22 +-
> t/t6120-describe.sh | 13 +-
> t/t6132-pathspec-exclude.sh | 2 +-
> t/t6200-fmt-merge-msg.sh | 2 +-
> t/t6300-for-each-ref.sh | 7 +-
> t/t6406-merge-attr.sh | 8 +-
> t/t6407-merge-binary.sh | 4 +-
> t/t6409-merge-subtree.sh | 6 +-
> t/t6411-merge-filemode.sh | 8 +-
> t/t6412-merge-large-rename.sh | 10 +-
> t/t6416-recursive-corner-cases.sh | 30 +-
> t/t6417-merge-ours-theirs.sh | 5 +-
> t/t6430-merge-recursive.sh | 2 +-
> t/t6600-test-reach.sh | 4 +-
> t/t7004-tag.sh | 9 +-
> t/t7010-setup.sh | 2 +-
> t/t7110-reset-merge.sh | 2 +-
> t/t7501-commit-basic-functionality.sh | 5 +-
> t/t7505-prepare-commit-msg-hook.sh | 2 +-
> t/t7513-interpret-trailers.sh | 2 +-
> t/t7519-status-fsmonitor.sh | 2 +-
> t/t7600-merge.sh | 2 +-
> t/t7602-merge-octopus-many.sh | 4 +-
> t/t7603-merge-reduce-heads.sh | 4 +-
> t/t7700-repack.sh | 2 +-
> t/t7810-grep.sh | 310 +++++++++---------
> t/t8002-blame.sh | 2 +-
> t/t8003-blame-corner-cases.sh | 10 +-
> t/t8014-blame-ignore-fuzzy.sh | 4 +-
> t/t9104-git-svn-follow-parent.sh | 4 +-
> t/t9107-git-svn-migrate.sh | 8 +-
> t/t9130-git-svn-authors-file.sh | 6 +-
> t/t9134-git-svn-ignore-paths.sh | 16 +-
> t/t9138-git-svn-authors-prog.sh | 2 +-
> t/t9146-git-svn-empty-dirs.sh | 4 +-
> t/t9147-git-svn-include-paths.sh | 16 +-
> t/t9152-svn-empty-dirs-after-gc.sh | 2 +-
> t/t9304-fast-import-marks.sh | 2 +-
> t/t9400-git-cvsserver-server.sh | 11 +-
> t/t9800-git-p4-basic.sh | 2 +-
> t/t9818-git-p4-block.sh | 6 +-
> t/t9902-completion.sh | 4 +-
> 163 files changed, 740 insertions(+), 847 deletions(-)
>
> --
> 2.34.1.307.g9b7440fafd
>