From: Fabian Stelzer <fs@gigacodes.de>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
Eric Sunshine <ericsunshine@gmail.com>
Subject: Re: [PATCH 12/19] tests: fix broken &&-chains in `{...}` groups
Date: Fri, 10 Dec 2021 10:38:27 +0100 [thread overview]
Message-ID: <20211210093827.a5s5mby3if5f7a4u@fs> (raw)
In-Reply-To: <20211209051115.52629-13-sunshine@sunshineco.com>
On 09.12.2021 00:11, Eric Sunshine wrote:
>The top-level &&-chain checker built into t/test-lib.sh causes tests to
>magically exit with code 117 if the &&-chain is broken. However, it has
>the shortcoming that the magic does not work within `{...}` groups,
>`(...)` subshells, `$(...)` substitutions, or within bodies of compound
>statements, such as `if`, `for`, `while`, `case`, etc. `chainlint.sed`
>partly fills in the gap by catching broken &&-chains in `(...)`
>subshells, but bugs can still lurk behind broken &&-chains in the other
>cases.
>
>Fix broken &&-chains in `{...}` groups in order to reduce the number of
>possible lurking bugs.
>
>Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>---
>
>diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>index 1a1a69ad92..bb3de2701a 100755
>--- a/t/t0021-conversion.sh
>+++ b/t/t0021-conversion.sh
>@@ -76,13 +76,13 @@ test_expect_success setup '
> git config filter.rot13.clean ./rot13.sh &&
>
> {
>- echo "*.t filter=rot13"
>+ echo "*.t filter=rot13" &&
> echo "*.i ident"
> } >.gitattributes &&
>
> {
>- echo a b c d e f g h i j k l m
>- echo n o p q r s t u v w x y z
>+ echo a b c d e f g h i j k l m &&
>+ echo n o p q r s t u v w x y z &&
> echo '\''$Id$'\''
> } >test &&
> cat test >test.t &&
>@@ -159,7 +159,7 @@ test_expect_success expanded_in_repo '
> printf "\$Id: NoTerminatingSymbolAtEOF"
> } >expected-output-crlf &&
> {
>- echo "expanded-keywords ident"
>+ echo "expanded-keywords ident" &&
> echo "expanded-keywords-crlf ident text eol=crlf"
> } >>.gitattributes &&
>
Wouldn't some of these be better off as heredocs as well?
There are a couple more below. I personally don't much mind either way but
since you changed quite a few in an earlier commit why not these?
>diff --git a/t/t0069-oidtree.sh b/t/t0069-oidtree.sh
>index 74cc59bf8a..889db50818 100755
>--- a/t/t0069-oidtree.sh
>+++ b/t/t0069-oidtree.sh
>@@ -28,7 +28,7 @@ test_expect_success 'oidtree insert and contains' '
> EOF
> {
> echoid insert 444 1 2 3 4 5 a b c d e &&
>- echoid contains 44 441 440 444 4440 4444
>+ echoid contains 44 441 440 444 4440 4444 &&
> echo clear
> } | test-tool oidtree >actual &&
> test_cmp expect actual
>@@ -37,11 +37,11 @@ test_expect_success 'oidtree insert and contains' '
> test_expect_success 'oidtree each' '
> echoid "" 123 321 321 >expect &&
> {
>- echoid insert f 9 8 123 321 a b c d e
>- echo each 12300
>- echo each 3211
>- echo each 3210
>- echo each 32100
>+ echoid insert f 9 8 123 321 a b c d e &&
>+ echo each 12300 &&
>+ echo each 3211 &&
>+ echo each 3210 &&
>+ echo each 32100 &&
> echo clear
> } | test-tool oidtree >actual &&
> test_cmp expect actual
>diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
>index 67a3f64c2d..f6f00c7039 100755
>--- a/t/t1006-cat-file.sh
>+++ b/t/t1006-cat-file.sh
>@@ -283,7 +283,7 @@ test_expect_success "--batch-check with multiple sha1s gives correct format" '
>
> test_expect_success 'setup blobs which are likely to delta' '
> test-tool genrandom foo 10240 >foo &&
>- { cat foo; echo plus; } >foo-plus &&
>+ { cat foo && echo plus; } >foo-plus &&
> git add foo foo-plus &&
> git commit -m foo &&
> cat >blobs <<-\EOF
>diff --git a/t/t1300-config.sh b/t/t1300-config.sh
>index 9571649c42..516dd8bfa8 100755
>--- a/t/t1300-config.sh
>+++ b/t/t1300-config.sh
>@@ -901,7 +901,7 @@ test_expect_success 'get --expiry-date' '
> EOF
> : "work around heredoc parsing bug fixed in dash 0.5.7 (in ec2c84d)" &&
> {
>- echo "$rel_out $(git config --expiry-date date.valid1)"
>+ echo "$rel_out $(git config --expiry-date date.valid1)" &&
> git config --expiry-date date.valid2 &&
> git config --expiry-date date.valid3 &&
> git config --expiry-date date.valid4 &&
>diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
>index 17d3cc1405..bbc01aae34 100755
>--- a/t/t1403-show-ref.sh
>+++ b/t/t1403-show-ref.sh
>@@ -78,7 +78,7 @@ test_expect_success 'show-ref --verify -q' '
> test_expect_success 'show-ref -d' '
> {
> echo $(git rev-parse refs/tags/A) refs/tags/A &&
>- echo $(git rev-parse refs/tags/A^0) "refs/tags/A^{}"
>+ echo $(git rev-parse refs/tags/A^0) "refs/tags/A^{}" &&
> echo $(git rev-parse refs/tags/C) refs/tags/C
> } >expect &&
> git show-ref -d A C >actual &&
>@@ -148,7 +148,7 @@ test_expect_success 'show-ref --heads, --tags, --head, pattern' '
>
> {
> echo $(git rev-parse HEAD) HEAD &&
>- echo $(git rev-parse refs/heads/B) refs/heads/B
>+ echo $(git rev-parse refs/heads/B) refs/heads/B &&
> echo $(git rev-parse refs/tags/B) refs/tags/B
> } >expect &&
> git show-ref --head B >actual &&
>@@ -156,8 +156,8 @@ test_expect_success 'show-ref --heads, --tags, --head, pattern' '
>
> {
> echo $(git rev-parse HEAD) HEAD &&
>- echo $(git rev-parse refs/heads/B) refs/heads/B
>- echo $(git rev-parse refs/tags/B) refs/tags/B
>+ echo $(git rev-parse refs/heads/B) refs/heads/B &&
>+ echo $(git rev-parse refs/tags/B) refs/tags/B &&
> echo $(git rev-parse refs/tags/B^0) "refs/tags/B^{}"
> } >expect &&
> git show-ref --head -d B >actual &&
>diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
>index 67b9cc752f..d2ef0041f9 100755
>--- a/t/t2200-add-update.sh
>+++ b/t/t2200-add-update.sh
>@@ -153,10 +153,10 @@ test_expect_success 'add -u resolves unmerged paths' '
> echo "100644 $one 1 $path" &&
> echo "100644 $two 2 $path" &&
> echo "100644 $three 3 $path"
>- done
>- echo "100644 $one 1 path3"
>- echo "100644 $one 1 path4"
>- echo "100644 $one 3 path5"
>+ done &&
>+ echo "100644 $one 1 path3" &&
>+ echo "100644 $one 1 path4" &&
>+ echo "100644 $one 3 path5" &&
> echo "100644 $one 3 path6"
> } |
> git update-index --index-info &&
>@@ -173,8 +173,8 @@ test_expect_success 'add -u resolves unmerged paths' '
> git add -u &&
> git ls-files -s path1 path2 path3 path4 path5 path6 >actual &&
> {
>- echo "100644 $three 0 path1"
>- echo "100644 $two 0 path3"
>+ echo "100644 $three 0 path1" &&
>+ echo "100644 $two 0 path3" &&
> echo "100644 $two 0 path5"
> } >expect &&
> test_cmp expect actual
>diff --git a/t/t2201-add-update-typechange.sh b/t/t2201-add-update-typechange.sh
>index a4eec0a346..687be974d4 100755
>--- a/t/t2201-add-update-typechange.sh
>+++ b/t/t2201-add-update-typechange.sh
>@@ -97,17 +97,17 @@ test_expect_success modify '
> "
> } >expect &&
> {
>- cat expect
>- echo ":100644 160000 $_empty $ZERO_OID T yonk"
>+ cat expect &&
>+ echo ":100644 160000 $_empty $ZERO_OID T yonk" &&
> echo ":100644 000000 $_empty $ZERO_OID D zifmia"
> } >expect-files &&
> {
>- cat expect
>+ cat expect &&
> echo ":000000 160000 $ZERO_OID $ZERO_OID A yonk"
> } >expect-index &&
> {
>- echo "100644 $_empty 0 nitfol"
>- echo "160000 $yomin 0 yomin"
>+ echo "100644 $_empty 0 nitfol" &&
>+ echo "160000 $yomin 0 yomin" &&
> echo "160000 $yonk 0 yonk"
> } >expect-final
> '
>diff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh
>index 47d6f35dcc..7cb9909293 100755
>--- a/t/t4023-diff-rename-typechange.sh
>+++ b/t/t4023-diff-rename-typechange.sh
>@@ -55,7 +55,7 @@ test_expect_success 'cross renames to be detected for regular files' '
>
> git diff-tree five six -r --name-status -B -M | sort >actual &&
> {
>- echo "R100 foo bar"
>+ echo "R100 foo bar" &&
> echo "R100 bar foo"
> } | sort >expect &&
> test_cmp expect actual
>@@ -66,7 +66,7 @@ test_expect_success 'cross renames to be detected for typechange' '
>
> git diff-tree one two -r --name-status -B -M | sort >actual &&
> {
>- echo "R100 foo bar"
>+ echo "R100 foo bar" &&
> echo "R100 bar foo"
> } | sort >expect &&
> test_cmp expect actual
>@@ -78,7 +78,7 @@ test_expect_success 'moves and renames' '
> git diff-tree three four -r --name-status -B -M | sort >actual &&
> {
> # see -B -M (#6) in t4008
>- echo "C100 foo bar"
>+ echo "C100 foo bar" &&
> echo "T100 foo"
> } | sort >expect &&
> test_cmp expect actual
>diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
>index ebff6c6883..ec5c10d2a0 100755
>--- a/t/t4124-apply-ws-rule.sh
>+++ b/t/t4124-apply-ws-rule.sh
>@@ -233,7 +233,7 @@ test_expect_success 'blank at EOF with --whitespace=fix (1)' '
> test_write_lines a b c >one &&
> git add one &&
> test_write_lines a b c >expect &&
>- { cat expect; echo; } >one &&
>+ { cat expect && echo; } >one &&
> git diff -- one >patch &&
>
> git checkout one &&
>diff --git a/t/t4150-am.sh b/t/t4150-am.sh
>index 2aaaa0d7de..103cd39148 100755
>--- a/t/t4150-am.sh
>+++ b/t/t4150-am.sh
>@@ -116,7 +116,7 @@ test_expect_success setup '
> git format-patch --stdout first | sed -e "1d"
> } | append_cr >patch1-crlf.eml &&
> {
>- printf "%255s\\n" ""
>+ printf "%255s\\n" "" &&
> echo "X-Fake-Field: Line One" &&
> echo "X-Fake-Field: Line Two" &&
> echo "X-Fake-Field: Line Three" &&
>diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh
>index 03b952c90d..0244888a5a 100755
>--- a/t/t4212-log-corrupt.sh
>+++ b/t/t4212-log-corrupt.sh
>@@ -20,10 +20,10 @@ test_expect_success 'fsck notices broken commit' '
>
> test_expect_success 'git log with broken author email' '
> {
>- echo commit $(cat broken_email.hash)
>- echo "Author: A U Thor <author@example.com>"
>- echo "Date: Thu Apr 7 15:13:13 2005 -0700"
>- echo
>+ echo commit $(cat broken_email.hash) &&
>+ echo "Author: A U Thor <author@example.com>" &&
>+ echo "Date: Thu Apr 7 15:13:13 2005 -0700" &&
>+ echo &&
> echo " foo"
> } >expect.out &&
>
>diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
>index 759169d074..df524f7b6d 100755
>--- a/t/t5316-pack-delta-depth.sh
>+++ b/t/t5316-pack-delta-depth.sh
>@@ -57,8 +57,11 @@ test_expect_success 'create series of packs' '
> git commit -m $i &&
> cur=$(git rev-parse HEAD^{tree}) &&
> {
>- test -n "$prev" && echo "-$prev"
>- echo $cur
>+ if test -n "$prev"
>+ then
>+ echo "-$prev"
>+ fi &&
>+ echo $cur &&
> echo "$(git rev-parse :file) file"
> } | git pack-objects --stdout >tmp &&
> git index-pack --stdin --fix-thin <tmp || return 1
>diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>index 1892d6615a..01468ce6d8 100755
>--- a/t/t5510-fetch.sh
>+++ b/t/t5510-fetch.sh
>@@ -71,7 +71,7 @@ test_expect_success "fetch test for-merge" '
> main_in_two=$(cd ../two && git rev-parse main) &&
> one_in_two=$(cd ../two && git rev-parse one) &&
> {
>- echo "$one_in_two "
>+ echo "$one_in_two " &&
> echo "$main_in_two not-for-merge"
> } >expected &&
> cut -f -2 .git/FETCH_HEAD >actual &&
>diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
>index 9d440e2821..c69cfd5c64 100755
>--- a/t/t5515-fetch-merge-logic.sh
>+++ b/t/t5515-fetch-merge-logic.sh
>@@ -191,17 +191,17 @@ do
> cp "$expect_r" expect_r &&
> convert_expected expect_r sed_script &&
> {
>- echo "# $cmd"
>- set x $cmd; shift
>- git symbolic-ref HEAD refs/heads/$1 ; shift
>- rm -f .git/FETCH_HEAD
>+ echo "# $cmd" &&
>+ set x $cmd && shift &&
>+ git symbolic-ref HEAD refs/heads/$1 && shift &&
>+ rm -f .git/FETCH_HEAD &&
> git for-each-ref \
> refs/heads refs/remotes/rem refs/tags |
> while read val type refname
> do
> git update-ref -d "$refname" "$val"
>- done
>- git fetch "$@" >/dev/null
>+ done &&
>+ git fetch "$@" >/dev/null &&
> cat .git/FETCH_HEAD
> } >"$actual_f" &&
> git show-ref >"$actual_r" &&
>diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
>index 05a58069b0..b68ec22d3f 100755
>--- a/t/t5562-http-backend-content-length.sh
>+++ b/t/t5562-http-backend-content-length.sh
>@@ -63,7 +63,7 @@ test_expect_success 'setup' '
> hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
> {
> printf "%s %s refs/heads/newbranch\\0report-status object-format=%s\\n" \
>- "$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize_raw
>+ "$ZERO_OID" "$hash_next" "$(test_oid algo)" | packetize_raw &&
> printf 0000 &&
> echo "$hash_next" | git pack-objects --stdout
> } >push_body &&
>diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
>index b87ca06a58..1131503b76 100755
>--- a/t/t5570-git-daemon.sh
>+++ b/t/t5570-git-daemon.sh
>@@ -194,7 +194,7 @@ test_expect_success 'hostname cannot break out of directory' '
>
> test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
> {
>- printf "git-upload-pack /interp.git\n\0host=localhost" | packetize_raw
>+ printf "git-upload-pack /interp.git\n\0host=localhost" | packetize_raw &&
> printf "0000"
> } >input &&
> fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
>diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
>index b043a279f1..80e86d8284 100755
>--- a/t/t5571-pre-push-hook.sh
>+++ b/t/t5571-pre-push-hook.sh
>@@ -114,7 +114,7 @@ test_expect_success 'push to URL' '
>
> test_expect_success 'set up many-ref tests' '
> {
>- nr=1000
>+ nr=1000 &&
> while test $nr -lt 2000
> do
> nr=$(( $nr + 1 )) &&
>diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
>index 04885d0a5e..97f10905d2 100755
>--- a/t/t7513-interpret-trailers.sh
>+++ b/t/t7513-interpret-trailers.sh
>@@ -156,7 +156,7 @@ test_expect_success 'with config option on the command line' '
> Acked-by: Johan
> Reviewed-by: Peff
> EOF
>- { echo; echo "Acked-by: Johan"; } |
>+ { echo && echo "Acked-by: Johan"; } |
> git -c "trailer.Acked-by.ifexists=addifdifferent" interpret-trailers \
> --trailer "Reviewed-by: Peff" --trailer "Acked-by: Johan" >actual &&
> test_cmp expected actual
>diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
>index 5bb302b1ba..ee4fdd8f18 100755
>--- a/t/t8002-blame.sh
>+++ b/t/t8002-blame.sh
>@@ -97,7 +97,7 @@ test_expect_success 'set up abbrev tests' '
> test_commit abbrev &&
> sha1=$(git rev-parse --verify HEAD) &&
> check_abbrev () {
>- expect=$1; shift
>+ expect=$1 && shift &&
> echo $sha1 | cut -c 1-$expect >expect &&
> git blame "$@" abbrev.t >actual &&
> perl -lne "/[0-9a-f]+/ and print \$&" <actual >actual.sha &&
>--
>2.34.1.307.g9b7440fafd
>
next prev parent reply other threads:[~2021-12-10 9:38 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-09 5:10 [PATCH 00/19] tests: fix broken &&-chains & abort loops on error Eric Sunshine
2021-12-09 5:10 ` [PATCH 01/19] t/lib-pager: use sane_unset() to avoid breaking &&-chain Eric Sunshine
2021-12-09 5:10 ` [PATCH 02/19] t1010: fix unnoticed failure on Windows Eric Sunshine
2021-12-09 16:27 ` Elijah Newren
2021-12-09 16:45 ` Eric Sunshine
2021-12-09 5:10 ` [PATCH 03/19] t1020: avoid aborting entire test script when one test fails Eric Sunshine
2021-12-09 5:11 ` [PATCH 04/19] t4202: clarify intent by creating expected content less cleverly Eric Sunshine
2021-12-10 9:09 ` Jeff King
2021-12-09 5:11 ` [PATCH 05/19] t5516: drop unnecessary subshell and command invocation Eric Sunshine
2021-12-10 9:10 ` Jeff King
2021-12-09 5:11 ` [PATCH 06/19] t6300: make `%(raw:size) --shell` test more robust Eric Sunshine
2021-12-10 9:14 ` Jeff King
2021-12-09 5:11 ` [PATCH 07/19] t9107: use shell parameter expansion to avoid breaking &&-chain Eric Sunshine
2021-12-09 5:11 ` [PATCH 08/19] tests: simplify construction of large blocks of text Eric Sunshine
2021-12-09 5:11 ` [PATCH 09/19] tests: use test_write_lines() to generate line-oriented output Eric Sunshine
2021-12-10 9:22 ` Jeff King
2021-12-11 6:59 ` Eric Sunshine
2021-12-09 5:11 ` [PATCH 10/19] tests: fix broken &&-chains in compound statements Eric Sunshine
2021-12-09 5:11 ` [PATCH 11/19] tests: fix broken &&-chains in `$(...)` command substitutions Eric Sunshine
2021-12-09 16:44 ` Elijah Newren
2021-12-09 16:53 ` Eric Sunshine
2021-12-09 16:57 ` Elijah Newren
2021-12-10 9:27 ` Jeff King
2021-12-09 5:11 ` [PATCH 12/19] tests: fix broken &&-chains in `{...}` groups Eric Sunshine
2021-12-10 9:29 ` Jeff King
2021-12-11 7:14 ` Eric Sunshine
2021-12-10 9:38 ` Fabian Stelzer [this message]
2021-12-11 7:32 ` Eric Sunshine
2021-12-09 5:11 ` [PATCH 13/19] tests: apply modern idiom for signaling test failure Eric Sunshine
2021-12-10 9:32 ` Jeff King
2021-12-11 7:47 ` Eric Sunshine
2021-12-09 5:11 ` [PATCH 14/19] tests: apply modern idiom for exiting loop upon failure Eric Sunshine
2021-12-10 9:36 ` Jeff King
2021-12-09 5:11 ` [PATCH 15/19] tests: simplify by dropping unnecessary `for` loops Eric Sunshine
2021-12-09 16:50 ` Elijah Newren
2021-12-09 5:11 ` [PATCH 16/19] t0000-t3999: detect and signal failure within loop Eric Sunshine
2021-12-09 5:11 ` [PATCH 17/19] t4000-t4999: " Eric Sunshine
2021-12-10 9:53 ` Fabian Stelzer
2021-12-11 8:06 ` Eric Sunshine
2021-12-09 5:11 ` [PATCH 18/19] t5000-t5999: " Eric Sunshine
2021-12-09 5:11 ` [PATCH 19/19] t6000-t9999: " Eric Sunshine
2021-12-09 17:02 ` [PATCH 00/19] tests: fix broken &&-chains & abort loops on error Elijah Newren
2021-12-09 19:17 ` Eric Sunshine
2021-12-10 9:38 ` Jeff King
2021-12-10 9:57 ` Fabian Stelzer
2021-12-11 8:16 ` Eric Sunshine
2021-12-11 9:58 ` [PATCH v1.1 2/19] t1010: fix unnoticed failure on Windows Eric Sunshine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211210093827.a5s5mby3if5f7a4u@fs \
--to=fs@gigacodes.de \
--cc=ericsunshine@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).