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

quoted

 	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