[PATCHv2 4/6] t7510: exit for loop with test result
When t7510 was introduced, the author made sure that a for loop in a subshell would return with the appropriate error code. Make sure this is true also the for the first line in each loop, which was missed. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- t/t7510-signed-commit.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 5ddac1a..a5ba48e 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in initial second merge fourth-signed fifth-signed sixth-signed master do - git show --pretty=short --show-signature $commit actual + git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual || exit 1 ! grep BAD signature from actual || exit 1 echo $commit OK @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned do - git show --pretty=short --show-signature $commit actual + git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual exit 1 ! grep BAD signature from actual || exit 1 echo $commit OK -- 2.0.0.426.g37dbf84 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote: When t7510 was introduced, the author made sure that a for loop in a subshell would return with the appropriate error code. Make sure this is true also the for the first line in each loop, which was missed. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- t/t7510-signed-commit.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 5ddac1a..a5ba48e 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in initial second merge fourth-signed fifth-signed sixth-signed master do - git show --pretty=short --show-signature $commit actual + git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual || exit 1 Hrm. The original is: X Y || exit 1 Won't that still exit (i.e., it is already correct)? Doing: for X in true false; do for Y in true false; do ($X $Y || exit 1) echo $X/$Y: $? done done yields: true/true: 0 true/false: 1 false/true: 1 false/false: 1 (and should still short-circuit Y, because we go from left-to-right). I do not mind changing it to keep the style of each line consistent, though. I would have written it as a series of -chains, with a single exit at the end, but I think that is just a matter of preference. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
Jeff King venit, vidit, dixit 13.06.2014 13:46: On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote: When t7510 was introduced, the author made sure that a for loop in a subshell would return with the appropriate error code. Make sure this is true also the for the first line in each loop, which was missed. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- t/t7510-signed-commit.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 5ddac1a..a5ba48e 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in initial second merge fourth-signed fifth-signed sixth-signed master do -git show --pretty=short --show-signature $commit actual +git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual || exit 1 Hrm. The original is: X Y || exit 1 Won't that still exit (i.e., it is already correct)? Doing: for X in true false; do for Y in true false; do ($X $Y || exit 1) echo $X/$Y: $? done done yields: true/true: 0 true/false: 1 false/true: 1 false/false: 1 (and should still short-circuit Y, because we go from left-to-right). I do not mind changing it to keep the style of each line consistent, though. I would have written it as a series of -chains, with a single exit at the end, but I think that is just a matter of preference. If I remember correctly, I put something failing on the first line of the original version, and the test succeeded. I think the point is that we have a for loop in a subshell, and we need to make sure that the false of one iteration is not overwritten by the true of the next one - exit 1 makes sure to break the for loop and exit the subshell. (The chain should do that as well, I'll recheck.) Michael -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
Michael J Gruber venit, vidit, dixit 13.06.2014 14:04: Jeff King venit, vidit, dixit 13.06.2014 13:46: On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote: When t7510 was introduced, the author made sure that a for loop in a subshell would return with the appropriate error code. Make sure this is true also the for the first line in each loop, which was missed. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- t/t7510-signed-commit.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 5ddac1a..a5ba48e 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in initial second merge fourth-signed fifth-signed sixth-signed master do - git show --pretty=short --show-signature $commit actual + git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual || exit 1 Hrm. The original is: X Y || exit 1 Won't that still exit (i.e., it is already correct)? Doing: for X in true false; do for Y in true false; do ($X $Y || exit 1) echo $X/$Y: $? done done yields: true/true: 0 true/false: 1 false/true: 1 false/false: 1 (and should still short-circuit Y, because we go from left-to-right). I do not mind changing it to keep the style of each line consistent, though. I would have written it as a series of -chains, with a single exit at the end, but I think that is just a matter of preference. If I remember correctly, I put something failing on the first line of the original version, and the test succeeded. I think the point is that we have a for loop in a subshell, and we need to make sure that the false of one iteration is not overwritten by the true of the next one - exit 1 makes sure to break the for loop and exit the subshell. (The chain should do that as well, I'll recheck.) ... the chain does not, which is the point :) With X Y || exit 1 inside the loop, the loop statement will return false, but the loop will continue (if X returns false), which is exactly the problem that the exit avoids. Make your example iterate over false true instead in the inner loop and you'll see ;) Michael -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
Michael J Gruber venit, vidit, dixit 13.06.2014 14:22: Michael J Gruber venit, vidit, dixit 13.06.2014 14:04: Jeff King venit, vidit, dixit 13.06.2014 13:46: On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote: When t7510 was introduced, the author made sure that a for loop in a subshell would return with the appropriate error code. Make sure this is true also the for the first line in each loop, which was missed. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- t/t7510-signed-commit.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 5ddac1a..a5ba48e 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in initial second merge fourth-signed fifth-signed sixth-signed master do - git show --pretty=short --show-signature $commit actual + git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual || exit 1 Hrm. The original is: X Y || exit 1 Won't that still exit (i.e., it is already correct)? Doing: for X in true false; do for Y in true false; do ($X $Y || exit 1) echo $X/$Y: $? done done yields: true/true: 0 true/false: 1 false/true: 1 false/false: 1 (and should still short-circuit Y, because we go from left-to-right). I do not mind changing it to keep the style of each line consistent, though. I would have written it as a series of -chains, with a single exit at the end, but I think that is just a matter of preference. If I remember correctly, I put something failing on the first line of the original version, and the test succeeded. I think the point is that we have a for loop in a subshell, and we need to make sure that the false of one iteration is not overwritten by the true of the next one - exit 1 makes sure to break the for loop and exit the subshell. (The chain should do that as well, I'll recheck.) ... the chain does not, which is the point :) With X Y || exit 1 inside the loop, the loop statement will return false, but the loop will continue (if X returns false), which is exactly the problem that the exit avoids. Make your example iterate over false true instead in the inner loop and you'll see ;) Michael ... with this loop, sorry: for X in true false; do for Y in false true; do ($X $Y || exit 1) done echo $X/last inner $Y: $? done gives true/last inner true: 0 false/last inner true: 1 even though on both cases we have at least one failure of Y. (failure of one subtest = failure of the test) Looping in the other order: for X in true false; do for Y in true false; do ($X $Y || exit 1) done echo $X/last inner $Y: $? done gives true/last inner false: 1 false/last inner false: 1 as it should be. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
On Fri, Jun 13, 2014 at 02:33:02PM +0200, Michael J Gruber wrote: With X Y || exit 1 inside the loop, the loop statement will return false, but the loop will continue (if X returns false), which is exactly the problem that the exit avoids. Make your example iterate over false true instead in the inner loop and you'll see ;) Michael ... with this loop, sorry: for X in true false; do for Y in false true; do ($X $Y || exit 1) done echo $X/last inner $Y: $? done I'm somewhat confused, as my loops were meant only to expand the truth table, not to simulate a real loop in the code. That is why I have a subshell in the loop around my exit, to make sure we keep looping. In the real code, the subshell surrounds the whole loop (so that an exit leaves the entire loop without leaving the whole script). The actual code is more like: ( for i in a b c; do echo $i: got to first step test $i != b echo $i: got to second step || exit 1 done ) echo overall status: $? which should fail on the second loop iteration. And it does: a: got to first step a: got to second step b: got to first step overall status: 1 That is, we short-circuit to the exit 1 as soon as test $i != b fails. You can replace the use of $? above with more -chaining, of course. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
Am 6/13/2014 14:33, schrieb Michael J Gruber: with this loop, sorry: for X in true false; do for Y in false true; do ($X $Y || exit 1) done echo $X/last inner $Y: $? done gives true/last inner true: 0 false/last inner true: 1 even though on both cases we have at least one failure of Y. (failure of one subtest = failure of the test) Place the loop(s) inside the subshell, and you observe termination on the first failure, and a failure exit code of the subshell. The change proposed in this patch should not be necessary. Something else must be wrong with your tests. Ah, here it is: @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned do - git show --pretty=short --show-signature $commit actual + git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual exit 1 ! grep BAD signature from actual || exit 1 echo $commit OK Notice the ' exit 1'! It should be '|| exit 1', right? -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
Johannes Sixt venit, vidit, dixit 13.06.2014 14:54: Am 6/13/2014 14:33, schrieb Michael J Gruber: with this loop, sorry: for X in true false; do for Y in false true; do ($X $Y || exit 1) done echo $X/last inner $Y: $? done gives true/last inner true: 0 false/last inner true: 1 even though on both cases we have at least one failure of Y. (failure of one subtest = failure of the test) Place the loop(s) inside the subshell, and you observe termination on the first failure, and a failure exit code of the subshell. The change proposed in this patch should not be necessary. Something else must be wrong with your tests. I know I started this (or Jeff did, who knows ;) ), but we keep confusing each other more and more: Ah, here it is: @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned do - git show --pretty=short --show-signature $commit actual + git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual exit 1 This is as in the original, it tests invalid signatures, so Good signature should not be in the response. ! grep BAD signature from actual || exit 1 echo $commit OK Notice the ' exit 1'! It should be '|| exit 1', right? -- Hannes In other words, the original tests already had grep foo exit 1 ! grep bar || exit 1 to test that we have neither foo nor bar. The reason is (supposedly) to have this portion of the test mostly analogous to the previous one, where we want foo and do want bar. So this is completely unrelated. Otoh, it seems the original test could have had a b c || exit 1 or a || exit 1 b || exit 1 c || exit 1 rather than a b || exit 1 c || exit 1 which I thought was incorrect (but I can't recreate the proof right now). I'd say both of the former versions are preferable to the last one unless there is a difference that neither Jeff nor I see. I need a break before looking at this again ;) Michael -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
Am 6/13/2014 15:06, schrieb Michael J Gruber: Johannes Sixt venit, vidit, dixit 13.06.2014 14:54: Am 6/13/2014 14:33, schrieb Michael J Gruber: with this loop, sorry: for X in true false; do for Y in false true; do ($X $Y || exit 1) done echo $X/last inner $Y: $? done gives true/last inner true: 0 false/last inner true: 1 even though on both cases we have at least one failure of Y. (failure of one subtest = failure of the test) Place the loop(s) inside the subshell, and you observe termination on the first failure, and a failure exit code of the subshell. The change proposed in this patch should not be necessary. Something else must be wrong with your tests. I know I started this (or Jeff did, who knows ;) ), but we keep confusing each other more and more: Ah, here it is: @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned do -git show --pretty=short --show-signature $commit actual +git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual exit 1 This is as in the original, it tests invalid signatures, so Good signature should not be in the response. ! grep BAD signature from actual || exit 1 echo $commit OK Notice the ' exit 1'! It should be '|| exit 1', right? -- Hannes In other words, the original tests already had grep foo exit 1 ! grep bar || exit 1 to test that we have neither foo nor bar. The reason is (supposedly) to have this portion of the test mostly analogous to the previous one, where we want foo and do want bar. So this is completely unrelated. I don't think so. What is the outcome of false # simulate a regression grep foo exit 1 ! grep bar || exit 1 assuming that the '! grep bar' happens to be true? Answer: The regression is not diagnosed because the -chain is broken. *That* is what I think you described earlier in this thread as I put something failing on the first line of the original version, and the test succeeded. -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
On Fri, Jun 13, 2014 at 03:21:55PM +0200, Johannes Sixt wrote: I don't think so. What is the outcome of false # simulate a regression grep foo exit 1 ! grep bar || exit 1 assuming that the '! grep bar' happens to be true? Answer: The regression is not diagnosed because the -chain is broken. *That* is what I think you described earlier in this thread as I put something failing on the first line of the original version, and the test succeeded. Yeah, I think that is the bit that I was missing from my original confusion. false anything || exit 1 _does_ work. But that is not what is written. ;) Thanks for pointing it out. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
Johannes Sixt venit, vidit, dixit 13.06.2014 15:21: Am 6/13/2014 15:06, schrieb Michael J Gruber: Johannes Sixt venit, vidit, dixit 13.06.2014 14:54: Am 6/13/2014 14:33, schrieb Michael J Gruber: with this loop, sorry: for X in true false; do for Y in false true; do ($X $Y || exit 1) done echo $X/last inner $Y: $? done gives true/last inner true: 0 false/last inner true: 1 even though on both cases we have at least one failure of Y. (failure of one subtest = failure of the test) Place the loop(s) inside the subshell, and you observe termination on the first failure, and a failure exit code of the subshell. The change proposed in this patch should not be necessary. Something else must be wrong with your tests. I know I started this (or Jeff did, who knows ;) ), but we keep confusing each other more and more: Ah, here it is: @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' ' ( for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned do - git show --pretty=short --show-signature $commit actual + git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual exit 1 This is as in the original, it tests invalid signatures, so Good signature should not be in the response. ! grep BAD signature from actual || exit 1 echo $commit OK Notice the ' exit 1'! It should be '|| exit 1', right? -- Hannes In other words, the original tests already had grep foo exit 1 ! grep bar || exit 1 to test that we have neither foo nor bar. The reason is (supposedly) to have this portion of the test mostly analogous to the previous one, where we want foo and do want bar. So this is completely unrelated. I don't think so. What is the outcome of false # simulate a regression grep foo exit 1 ! grep bar || exit 1 assuming that the '! grep bar' happens to be true? Answer: The regression is not diagnosed because the -chain is broken. *That* is what I think you described earlier in this thread as I put something failing on the first line of the original version, and the test succeeded. -- Hannes If you say that something I have said makes sense I'm happy, because I can't confirm that myself right now. I'll take a break and look into a rewrite of the form a b test_must_fail c d || exit 1 hoping that will make things both readable (by avoiding !) and concise (by avoiding repeated exits). Michael -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
Am 6/13/2014 15:31, schrieb Michael J Gruber: rewrite of the form a b test_must_fail c d || exit 1 hoping that will make things both readable (by avoiding !) and concise (by avoiding repeated exits). Thanks! Please note that we use 'test_must_fail' only for git invocations, but we do write '!' in front of system commands that we expect to fail, e.g., '! grep'. -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/6] t7510: exit for loop with test result
Jeff King p...@peff.net writes: ( for commit in initial second merge fourth-signed fifth-signed sixth-signed master do -git show --pretty=short --show-signature $commit actual +git show --pretty=short --show-signature $commit actual || exit 1 grep Good signature from actual || exit 1 Hrm. The original is: X Y || exit 1 Won't that still exit (i.e., it is already correct)? Doing: for X in true false; do for Y in true false; do ($X $Y || exit 1) echo $X/$Y: $? done done yields: true/true: 0 true/false: 1 false/true: 1 false/false: 1 (and should still short-circuit Y, because we go from left-to-right). I do not mind changing it to keep the style of each line consistent, though. I would have written it as a series of -chains, with a single exit at the end, but I think that is just a matter of preference. Yeah, series of chain with a single exit at the end is good, and the subshell is there only to allow us to do that exit at the end. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html