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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html