Here's what I found looking for loops like:
for i in a b c; do
something_important $i || break
done &&
something_else
which presumably expect the chain to stop when something_important fails
for any loop element. The solutions are one of (depending on the
surrounding code):
1. Switch the break to "return 1". The tests are all &&-chained, so
the effect of a failed command is to exit the test immediately
anyway. And we wrap our eval'd test snippets inside an extra layer
of function call to explicitly allow early returns like this.
2. Switch the break to "exit 1". Calling "return" from a subshell
inside a function is a bit weird. It doesn't exit the function at
all, but rather just the subshell (in both bash and dash). But if
you are not in a function, calling "return" at all is an error in
bash (subshell or no), and OK in dash (where it acts like "exit").
POSIX explicitly marks the "outside of a function" behavior as
unspecified, but I couldn't find anything about the subshell
behavior.
So I'm loathe to depend on it, even though it does seem to do what
we want, as I do not want to even think what some more obscure
shells might do with it. And especially because we know that "exit
1" portably does what we want (the only downside is that you have
to recognize which situation you are in and use "exit" versus
"return").
3. Unroll the loops. In some cases the result is actually shorter, and
(IMHO) more readable.
These sites were all found with:
git grep -E '(^|\|\|)[ ]*break' t/t*.sh
(that's a space and a tab in the brackets). There are some matches
there that I did not touch, because they were already fine. E.g., t5302
and t7700 use loops to assign a value to a variable and break out early,
and then check the value of the variable.
That's just the tip of the iceberg, though. Searching for
git grep 'for .* in '
yields hundreds of hits. Most of which are probably fine (quite a few
are outside &&-chains entirely). I focused on the ones that called
break, because that indicated to me that the author was trying to
address the &&-chain. Certainly anybody else is welcome to take a stab
at the rest, but I'm also happy to fix them up as we touch nearby code
and notice them. Most of the loops are in setup code that we do not
expect to fail anyway, so examining them is a lot of work for a little
gain.
There were a few legitimate problems, though. I've ordered the patches
below by descending severity. These apply on top of jk/test-chain-lint.
[1/8]: perf-lib: fix ignored exit code inside loop
[2/8]: t0020: fix ignored exit code inside loops
[3/8]: t3305: fix ignored exit code inside loop
[4/8]: t7701: fix ignored exit code inside loop
These four are actual bugs.
[5/8]: t: fix some trivial cases of ignored exit codes in loops
These ones are in setup code, and so would almost certainly never
fail.
[6/8]: t: simplify loop exit-code status variables
[7/8]: t0020: use test_* helpers instead of hand-rolled messages
[8/8]: t9001: drop save_confirm helper
These last three are pure cleanup, no behavior changes. The last two
are not even strictly related to the same topic, but I noticed them
while in the area.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
