Re: [PATCH 00/19] tests: fix broken &&-chains & abort loops on error

[Elijah Newren <newren@gmail.com>]

On Wed, Dec 8, 2021 at 11:39 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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>

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