From: Jeff King <peff@peff.net>
Date: Wed, 25 Mar 2015 02:56:35 GMT
Subject: Re: [PATCH 17/25] t0020: use modern test_* helpers
Message-ID: <20150325025635.GC15051@peff.net>
In-Reply-To: <20150325012323.Horde.zCWvV1mF8OBE1PxYPuuEFg8@webmail.informatik.kit.edu>

On Wed, Mar 25, 2015 at 01:23:23AM +0100, SZEDER Gábor wrote:

> >  	for f in one dir/two
> >  	do
> >  		append_cr <$f >tmp && mv -f tmp $f &&
> >-		git update-index -- $f || {
> >-			echo Oops
> >-			false
> >-			break
> >-		}
> >+		git update-index -- $f ||
> >+		break
> >  	done &&
> 
> Ah, these tests are evil, I remember them from the time when I was fiddling
> with Jonathan's patch.  They can fail silently without testing what they
> were supposed to test.
> 
> If something in the loop fails, the break will leave the loop but it will do
> so with zero return value and, consequently, the test will continue as if
> everything were OK.
> And unless it was 'git update-index' that failed in a way that left a borked
> index behind, the 'git diff-index --cached' below will not error out or
> produce some output that would cause the test to fail.  i.e. I tried e.g.
> 
>   append_cr <$f >tmp && mv -f tmp $f && false &&
> 
> in the loop and the test succeeded.

Ugh, you're right. I remembered that for loops were tricky in &&-chains,
but for some reason was thinking that "break" would give you the last
exit code, But I just re-tested, and of course it does not work.

> I think the best fix would be to unroll the loop: after this patch the loop
> body consists of only two significant lines and we iterate through the loop
> only twice, so the test would be even shorter.

Yeah, unrolling may be the best thing here, given the size of the loops.
As a general rule, I think it has to be a subshell with an exit, like:

  (
	for i in one two three; do
		echo $i &&
		test $i = one ||
		exit 1
	done
  )
  echo exit=$?

which should yield one, two, and exit=1. 7b1732c (t7510: use consistent
&&-chains in loop, 2014-06-16) deals with this in another test.

-Peff