Re: [PATCH 3/3] t: detect and signal failure within loop

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Aug 22, 2022 at 11:05 PM Elijah Newren <newren@gmail.com> wrote:
> On Mon, Aug 22, 2022 at 11:26 AM Eric Sunshine via GitGitGadget
> <gitgitgadget@gmail.com> 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@sunshineco.com/

> 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.