| Message ID | 20211209051115.52629-12-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c576868eaff8cc4c7fb5cf8b787756b16fde268b |
| Headers | show |
| Series | tests: fix broken &&-chains & abort loops on error | expand |
On Wed, Dec 8, 2021 at 11:39 PM Eric Sunshine <sunshine@sunshineco.com> 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 `$(...)` command substitutions in order to > reduce the number of possible lurking bugs. > > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > contrib/subtree/t/t7900-subtree.sh | 2 +- > t/t0005-signals.sh | 2 +- > t/t0060-path-utils.sh | 4 ++-- > t/t1006-cat-file.sh | 10 +++++----- > t/t3600-rm.sh | 2 +- > t/t7010-setup.sh | 2 +- > 6 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh > index 4153b65321..1c1f76f04a 100755 > --- a/contrib/subtree/t/t7900-subtree.sh > +++ b/contrib/subtree/t/t7900-subtree.sh > @@ -1445,7 +1445,7 @@ test_expect_success 'subtree descendant check' ' > ) && > test_create_commit "$test_count" folder_subtree/0 && > test_create_commit "$test_count" folder_subtree/b && > - cherry=$(cd "$test_count"; git rev-parse HEAD) && > + cherry=$(cd "$test_count" && git rev-parse HEAD) && > ( > cd "$test_count" && > git checkout branch > diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh > index a5ec6a0315..eba75a2490 100755 > --- a/t/t0005-signals.sh > +++ b/t/t0005-signals.sh > @@ -48,7 +48,7 @@ test_expect_success !MINGW 'a constipated git dies with SIGPIPE' ' > ' > > test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' ' > - OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) && > + OUT=$( ((trap "" PIPE && large_git; echo $? 1>&3) | :) 3>&1 ) && Shouldn't the second ';' be replaced with '&&' as well? > test_match_signal 13 "$OUT" > ' > > diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh > index 34d1061f32..71a5d370cc 100755 > --- a/t/t0060-path-utils.sh > +++ b/t/t0060-path-utils.sh > @@ -216,7 +216,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' > mkdir second && > ln -s ../first second/other && > mkdir third && > - dir="$(cd .git; pwd -P)" && > + dir="$(cd .git && pwd -P)" && > dir2=third/../second/other/.git && > test "$dir" = "$(test-tool path-utils real_path $dir2)" && > file="$dir"/index && > @@ -224,7 +224,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' > basename=blub && > test "$dir/$basename" = "$(cd .git && test-tool path-utils real_path "$basename")" && > ln -s ../first/file .git/syml && > - sym="$(cd first; pwd -P)"/file && > + sym="$(cd first && pwd -P)"/file && > test "$sym" = "$(test-tool path-utils real_path "$dir2/syml")" > ' > > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh > index 658628375c..67a3f64c2d 100755 > --- a/t/t1006-cat-file.sh > +++ b/t/t1006-cat-file.sh > @@ -211,14 +211,14 @@ done > test_expect_success "--batch-check for a non-existent named object" ' > test "foobar42 missing > foobar84 missing" = \ > - "$( ( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)" > + "$( ( echo foobar42 && echo_without_newline foobar84 ) | git cat-file --batch-check)" > ' > > test_expect_success "--batch-check for a non-existent hash" ' > test "0000000000000000000000000000000000000042 missing > 0000000000000000000000000000000000000084 missing" = \ > - "$( ( echo 0000000000000000000000000000000000000042; > - echo_without_newline 0000000000000000000000000000000000000084; ) | > + "$( ( echo 0000000000000000000000000000000000000042 && > + echo_without_newline 0000000000000000000000000000000000000084 ) | > git cat-file --batch-check)" > ' > > @@ -226,8 +226,8 @@ test_expect_success "--batch for an existent and a non-existent hash" ' > test "$tag_sha1 tag $tag_size > $tag_content > 0000000000000000000000000000000000000000 missing" = \ > - "$( ( echo $tag_sha1; > - echo_without_newline 0000000000000000000000000000000000000000; ) | > + "$( ( echo $tag_sha1 && > + echo_without_newline 0000000000000000000000000000000000000000 ) | > git cat-file --batch)" > ' > > diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh > index bb9ef35dac..ed3952eb98 100755 > --- a/t/t3600-rm.sh > +++ b/t/t3600-rm.sh > @@ -265,7 +265,7 @@ test_expect_success 'choking "git rm" should not let it die with cruft (induce S > > test_expect_success !MINGW 'choking "git rm" should not let it die with cruft (induce and check SIGPIPE)' ' > choke_git_rm_setup && > - OUT=$( ((trap "" PIPE; git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) && > + OUT=$( ((trap "" PIPE && git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) && Same here; shouldn't the second ';' be replaced with '&&' as well? > test_match_signal 13 "$OUT" && > test_path_is_missing .git/index.lock > ' > diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh > index 0335a9a158..520f96d09f 100755 > --- a/t/t7010-setup.sh > +++ b/t/t7010-setup.sh > @@ -137,7 +137,7 @@ test_expect_success 'setup deeper work tree' ' > > test_expect_success 'add a directory outside the work tree' '( > cd tester && > - d1="$(cd .. ; pwd)" && > + d1="$(cd .. && pwd)" && > test_must_fail git add "$d1" > )' > > -- > 2.34.1.307.g9b7440fafd >
On Thu, Dec 9, 2021 at 11:44 AM Elijah Newren <newren@gmail.com> wrote: > On Wed, Dec 8, 2021 at 11:39 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' ' > > - OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) && > > + OUT=$( ((trap "" PIPE && large_git; echo $? 1>&3) | :) 3>&1 ) && > > Shouldn't the second ';' be replaced with '&&' as well? Thanks for reading so carefully. In this case, the answer is "no", the semicolon is correct. This code legitimately wants to capture in the OUT variable the numeric exit status of the command preceding `echo $?`. If the semicolon is replaced with `&&`, then the echo won't be executed if the exit status is non-zero, but we want `echo` to be executed regardless of the exit status. So, the code is correct with the semicolon, and would be incorrect with `&&`. (I hope I'm explaining this well enough to make sense.) It's this sort of special case which accounts for why the new linter (as mentioned in the cover letter) has special understanding that a broken &&-chain can be legitimate in certain circumstances, such as explicit handling of `$?`. > > - OUT=$( ((trap "" PIPE; git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) && > > + OUT=$( ((trap "" PIPE && git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) && > > Same here; shouldn't the second ';' be replaced with '&&' as well? Same answer as above.
On Thu, Dec 9, 2021 at 8:53 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Thu, Dec 9, 2021 at 11:44 AM Elijah Newren <newren@gmail.com> wrote: > > On Wed, Dec 8, 2021 at 11:39 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > > test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' ' > > > - OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) && > > > + OUT=$( ((trap "" PIPE && large_git; echo $? 1>&3) | :) 3>&1 ) && > > > > Shouldn't the second ';' be replaced with '&&' as well? > > Thanks for reading so carefully. In this case, the answer is "no", the > semicolon is correct. This code legitimately wants to capture in the > OUT variable the numeric exit status of the command preceding `echo > $?`. If the semicolon is replaced with `&&`, then the echo won't be > executed if the exit status is non-zero, but we want `echo` to be > executed regardless of the exit status. So, the code is correct with > the semicolon, and would be incorrect with `&&`. (I hope I'm > explaining this well enough to make sense.) > > It's this sort of special case which accounts for why the new linter > (as mentioned in the cover letter) has special understanding that a > broken &&-chain can be legitimate in certain circumstances, such as > explicit handling of `$?`. Ah, right, you had mentioned this in the cover letter. Short term memory on my part, I guess. Thanks for explaining...again. :-) > > > - OUT=$( ((trap "" PIPE; git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) && > > > + OUT=$( ((trap "" PIPE && git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) && > > > > Same here; shouldn't the second ';' be replaced with '&&' as well? > > Same answer as above.
On Thu, Dec 09, 2021 at 11:53:47AM -0500, Eric Sunshine wrote: > On Thu, Dec 9, 2021 at 11:44 AM Elijah Newren <newren@gmail.com> wrote: > > On Wed, Dec 8, 2021 at 11:39 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > > test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' ' > > > - OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) && > > > + OUT=$( ((trap "" PIPE && large_git; echo $? 1>&3) | :) 3>&1 ) && > > > > Shouldn't the second ';' be replaced with '&&' as well? > > Thanks for reading so carefully. In this case, the answer is "no", the > semicolon is correct. This code legitimately wants to capture in the > OUT variable the numeric exit status of the command preceding `echo > $?`. If the semicolon is replaced with `&&`, then the echo won't be > executed if the exit status is non-zero, but we want `echo` to be > executed regardless of the exit status. So, the code is correct with > the semicolon, and would be incorrect with `&&`. (I hope I'm > explaining this well enough to make sense.) That makes sense to me. I wondered why it was even worth changing the earlier semi-colon in that case, then, but... > It's this sort of special case which accounts for why the new linter > (as mentioned in the cover letter) has special understanding that a > broken &&-chain can be legitimate in certain circumstances, such as > explicit handling of `$?`. ...your unseen magic script explains it. :) All of the changes here look reasonable. We'd either want to know about failure (e.g., "cd") or don't expect it to fail (e.g., "echo"). These "trap" calls are probably fine. I can't imagine why they'd fail, but being a weird shell builtin I wonder if it's possible for them to fail in odd circumstances. I'm happy to leave that as a hypothetical until we see it in practice. -Peff
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh index 4153b65321..1c1f76f04a 100755 --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -1445,7 +1445,7 @@ test_expect_success 'subtree descendant check' ' ) && test_create_commit "$test_count" folder_subtree/0 && test_create_commit "$test_count" folder_subtree/b && - cherry=$(cd "$test_count"; git rev-parse HEAD) && + cherry=$(cd "$test_count" && git rev-parse HEAD) && ( cd "$test_count" && git checkout branch diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh index a5ec6a0315..eba75a2490 100755 --- a/t/t0005-signals.sh +++ b/t/t0005-signals.sh @@ -48,7 +48,7 @@ test_expect_success !MINGW 'a constipated git dies with SIGPIPE' ' ' test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' ' - OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) && + OUT=$( ((trap "" PIPE && large_git; echo $? 1>&3) | :) 3>&1 ) && test_match_signal 13 "$OUT" ' diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 34d1061f32..71a5d370cc 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -216,7 +216,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' mkdir second && ln -s ../first second/other && mkdir third && - dir="$(cd .git; pwd -P)" && + dir="$(cd .git && pwd -P)" && dir2=third/../second/other/.git && test "$dir" = "$(test-tool path-utils real_path $dir2)" && file="$dir"/index && @@ -224,7 +224,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' basename=blub && test "$dir/$basename" = "$(cd .git && test-tool path-utils real_path "$basename")" && ln -s ../first/file .git/syml && - sym="$(cd first; pwd -P)"/file && + sym="$(cd first && pwd -P)"/file && test "$sym" = "$(test-tool path-utils real_path "$dir2/syml")" ' diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 658628375c..67a3f64c2d 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -211,14 +211,14 @@ done test_expect_success "--batch-check for a non-existent named object" ' test "foobar42 missing foobar84 missing" = \ - "$( ( echo foobar42; echo_without_newline foobar84; ) | git cat-file --batch-check)" + "$( ( echo foobar42 && echo_without_newline foobar84 ) | git cat-file --batch-check)" ' test_expect_success "--batch-check for a non-existent hash" ' test "0000000000000000000000000000000000000042 missing 0000000000000000000000000000000000000084 missing" = \ - "$( ( echo 0000000000000000000000000000000000000042; - echo_without_newline 0000000000000000000000000000000000000084; ) | + "$( ( echo 0000000000000000000000000000000000000042 && + echo_without_newline 0000000000000000000000000000000000000084 ) | git cat-file --batch-check)" ' @@ -226,8 +226,8 @@ test_expect_success "--batch for an existent and a non-existent hash" ' test "$tag_sha1 tag $tag_size $tag_content 0000000000000000000000000000000000000000 missing" = \ - "$( ( echo $tag_sha1; - echo_without_newline 0000000000000000000000000000000000000000; ) | + "$( ( echo $tag_sha1 && + echo_without_newline 0000000000000000000000000000000000000000 ) | git cat-file --batch)" ' diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index bb9ef35dac..ed3952eb98 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -265,7 +265,7 @@ test_expect_success 'choking "git rm" should not let it die with cruft (induce S test_expect_success !MINGW 'choking "git rm" should not let it die with cruft (induce and check SIGPIPE)' ' choke_git_rm_setup && - OUT=$( ((trap "" PIPE; git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) && + OUT=$( ((trap "" PIPE && git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) && test_match_signal 13 "$OUT" && test_path_is_missing .git/index.lock ' diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh index 0335a9a158..520f96d09f 100755 --- a/t/t7010-setup.sh +++ b/t/t7010-setup.sh @@ -137,7 +137,7 @@ test_expect_success 'setup deeper work tree' ' test_expect_success 'add a directory outside the work tree' '( cd tester && - d1="$(cd .. ; pwd)" && + d1="$(cd .. && pwd)" && test_must_fail git add "$d1" )'
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 `$(...)` command substitutions in order to reduce the number of possible lurking bugs. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- contrib/subtree/t/t7900-subtree.sh | 2 +- t/t0005-signals.sh | 2 +- t/t0060-path-utils.sh | 4 ++-- t/t1006-cat-file.sh | 10 +++++----- t/t3600-rm.sh | 2 +- t/t7010-setup.sh | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-)