| Message ID | 20211209051115.52629-15-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 03949e33f58223dd2d8465f4dd8042e5e581fcef |
| Headers | show |
| Series | tests: fix broken &&-chains & abort loops on error | expand |
On Thu, Dec 09, 2021 at 12:11:10AM -0500, Eric Sunshine wrote: > diff --git a/t/t1050-large.sh b/t/t1050-large.sh > index 99ff2866b7..0e4267c723 100755 > --- a/t/t1050-large.sh > +++ b/t/t1050-large.sh > @@ -51,27 +51,21 @@ EOF > test_expect_success 'add a large file or two' ' > git add large1 huge large2 && > # make sure we got a single packfile and no loose objects > - bad= count=0 idx= && > + count=0 idx= && > for p in .git/objects/pack/pack-*.pack > do > count=$(( $count + 1 )) && > - if test_path_is_file "$p" && > - idx=${p%.pack}.idx && test_path_is_file "$idx" > - then > - continue > - fi > - bad=t > + test_path_is_file "$p" && > + idx=${p%.pack}.idx && > + test_path_is_file "$idx" || return 1 > done && > - test -z "$bad" && Thanks goodness. I had to read the original loop several times to understand what it was trying to do. The post-image is much nicer. > diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh > index 17f988edd2..a6a73effde 100755 > --- a/t/t9400-git-cvsserver-server.sh > +++ b/t/t9400-git-cvsserver-server.sh > @@ -350,10 +350,9 @@ test_expect_success 'cvs update (subdirectories)' \ > test_cmp "$dir/$filename" "../$dir/$filename"; then > : > else > - echo >failure > + exit 1 > fi > - done) && > - test ! -f failure' > + done)' These all look good. I had to blink a few times to see the subshell in this one (it's finished by the closing paren after "done", for other reviewers). -Peff
diff --git a/t/t1050-large.sh b/t/t1050-large.sh index 99ff2866b7..0e4267c723 100755 --- a/t/t1050-large.sh +++ b/t/t1050-large.sh @@ -51,27 +51,21 @@ EOF test_expect_success 'add a large file or two' ' git add large1 huge large2 && # make sure we got a single packfile and no loose objects - bad= count=0 idx= && + count=0 idx= && for p in .git/objects/pack/pack-*.pack do count=$(( $count + 1 )) && - if test_path_is_file "$p" && - idx=${p%.pack}.idx && test_path_is_file "$idx" - then - continue - fi - bad=t + test_path_is_file "$p" && + idx=${p%.pack}.idx && + test_path_is_file "$idx" || return 1 done && - test -z "$bad" && test $count = 1 && cnt=$(git show-index <"$idx" | wc -l) && test $cnt = 2 && for l in .git/objects/$OIDPATH_REGEX do - test_path_is_file "$l" || continue - bad=t + test_path_is_missing "$l" || return 1 done && - test -z "$bad" && # attempt to add another copy of the same git add large3 && @@ -79,14 +73,10 @@ test_expect_success 'add a large file or two' ' for p in .git/objects/pack/pack-*.pack do count=$(( $count + 1 )) && - if test_path_is_file "$p" && - idx=${p%.pack}.idx && test_path_is_file "$idx" - then - continue - fi - bad=t + test_path_is_file "$p" && + idx=${p%.pack}.idx && + test_path_is_file "$idx" || return 1 done && - test -z "$bad" && test $count = 1 ' diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index e6e3c8f552..5ef8db481c 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1332,7 +1332,6 @@ test_expect_success 'unqualified <dst> refspec DWIM and advice' ' ( cd test && git tag -a -m "Some tag" some-tag main && - exit_with=true && for type in commit tag tree blob do if test "$type" = "blob" @@ -1348,9 +1347,8 @@ test_expect_success 'unqualified <dst> refspec DWIM and advice' ' push origin $oid:dst 2>err && test_i18ngrep "error: The destination you" err && test_i18ngrep ! "hint: Did you mean" err || - exit_with=false - done && - $exit_with + exit 1 + done ) ' diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index 17f988edd2..a6a73effde 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -350,10 +350,9 @@ test_expect_success 'cvs update (subdirectories)' \ test_cmp "$dir/$filename" "../$dir/$filename"; then : else - echo >failure + exit 1 fi - done) && - test ! -f failure' + done)' cd "$WORKDIR" test_expect_success 'cvs update (delete file)' \
Rather than maintaining a flag indicating a failure within a loop and aborting the test when the loop ends if the flag is set, modern practice is to signal the failure immediately by exiting the loop early via `return 1` (or `exit 1` if inside a subshell). Simplify these loops by following the modern idiom. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- t/t1050-large.sh | 26 ++++++++------------------ t/t5505-remote.sh | 6 ++---- t/t9400-git-cvsserver-server.sh | 5 ++--- 3 files changed, 12 insertions(+), 25 deletions(-)