Re: [PATCHv2 4/6] t7510: exit for loop with test result

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Jun 13, 2014, 12:33 UTC
Message-ID: <539AEF7E.1050402@drmicha.warpmail.net>
In-Reply-To: <539AED0C.8050107@drmicha.warpmail.net>

Michael J Gruber venit, vidit, dixit 13.06.2014 14:22:

Show 71 quoted lines

> 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 <git@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.