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:

quoted

On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:

quoted

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