On Mon, Aug 22, 2022 at 11:05 PM Elijah Newren <newren@xxxxxxxxx> wrote: > On Mon, Aug 22, 2022 at 11:26 AM Eric Sunshine via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: > > Failures within `for` and `while` loops can go unnoticed if not detected > > and signaled manually since the loop itself does not abort when a > > contained command fails, nor will a failure necessarily be detected when > > the loop finishes since the loop returns the exit code of the last > > command it ran on the final iteration, which may not be the command > > which failed. Therefore, detect and signal failures manually within > > loops using the idiom `|| return 1` (or `|| exit 1` within subshells). > > > > diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh > > for i in $(test_seq 1 10) > > do > > - >unrelated/$i > > + >unrelated/$i || exit 1 > > done && > > That's not something I'm likely ever going to remember to think of as > capable of failing and needing this special care. Is this a > preliminary series before you send chainlint improvements that finds > this kind of thing for us? Or did you notice this some other way? This could fail due to lack of space on the filesystem or "inode" exhaustion (indeed, I've seen out-of-space failures when I've underallocated the ramdisk on which I run tests). But there are some cases of added `|| return` or `|| exit` in the original series[1] which are just churn because the code inside the loop wouldn't / couldn't / shouldn't fail. I wasn't happy about adding those simply to pacify a not-smart-enough linter, but I eventually convinced myself that the small amount of inconvenience of those pointless cases was greatly outweighed by the vast number of cases in which adding `|| return` or `|| exit` was the correct thing to do since those cases could genuinely have allowed errors in Git or in the tests themselves to go unnoticed. Yes, this is another preliminary series before sending the new `chainlint` series, and it was the new linter which found these cases. [1]: https://lore.kernel.org/git/20211209051115.52629-1-sunshine@xxxxxxxxxxxxxx/ > Change is fine, of course, I'm just curious how it was found (and how > I can avoid adding more of these that you'll need to later fix up). Although the implementation of the new linter has been complete for over a year, I finally found time to work on polishing the patch series itself. I had hoped to get it submitted this past week, but time constraints prevented it. So, the answer is that the new linter (once I submit it and once Junio accepts it -- if he does) should help you avoid introducing more such cases.
