Re: [PATCH 2/3] t4216: fix broken '&&'-chain
From: Jeff King <hidden>
Date: 2020-06-30 19:03:28
On Tue, Jun 30, 2020 at 02:39:28PM -0400, Taylor Blau wrote:
quoted
quoted
This ends up working fine when the file already exists, in which case 'rm' exits cleanly and the rest of the function executes normally. When the file does _not_ exist, however, 'rm' returns an unclean exit code, causing the function to terminate.This explanation makes no sense. Since this command was not part of the &&-chain, its failure would not cause the function to terminate prematurely nor would it affect the return value of the function. This explanation would make sense, however, if you're talking about the behavior _after_ fixing the broken &&-chain.Fair enough. For what it's worth, this explanation *does* make sense if you 'set -e' beforehand, which I am accustomed to (and had incorrectly assumed that tests in 't' also have 'set -e', when they do not).
If we _really_ want to nitpick, it probably wouldn't terminate under "set -e" because the call to "setup" is itself part of an &&-chain, which suppresses "-e" handling (which is one of the many confusing "set -e" behaviors that led us to avoid it in the first place). But definitely your revised commit message below is more accurate. However...
quoted hunk ↗ jump to hunk
--- >8 ---Subject: [PATCH] t4216: fix broken '&&'-chain The 'rm' added in a759bfa9ee (t4216: add end to end tests for git log with Bloom filters, 2020-04-06) should be placed within the function's '&&'-chain. The file being removed may not exist (for eg., in the case of '--run', in which case it may not be generated beforehand by a skipped test), and so add '-f' to account for the file's optional existence.
Is the &&-chain really broken, or is the first command simply not part
of that chain? Perhaps a question for philosophers, but the more applied
question here is: what are we improving, and why?
The original code handled the fact that the file might not exist by not
including its exit code in the &&-chain which leads to the function's
return value. Your new code does so by putting it in the &&-chain but
asking "rm" to ignore errors. Is one better than the other?
I think so, but my argument would be more along the lines of:
- without "-f", "rm" will complain about a missing file, which is
distracting noise in the test log
- once "-f" is added in to suppress that, we might as well add the
command to the &&-chain. That's our normal style, so readers don't
have to wonder if it's important or not. Plus it would help avoid a
broken chain if more commands are added at the beginning of the
function.
-Peff