Re: [PATCH v2 02/11] cache-tree tests: use a sub-shell with less indirection

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 17, 2021, 22:23 UTC
Message-ID: <CAPig+cToj8nQmyBCqC1k7DXF2vXaonCEA-fCJ4x7JBZG2ixYBw@mail.gmail.com>
In-Reply-To: <YARsCsgXuiXr4uFX@coredump.intra.peff.net>

On Sun, Jan 17, 2021 at 11:55 AM Jeff King <peff@peff.net> wrote:

Show 11 quoted lines

> On Sat, Jan 16, 2021 at 04:35:45PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >       for subtree in $subtrees
> >       do
> > +             (
> > +                     cd "$subtree"
> > +                     generate_expected_cache_tree_rec "$dir$subtree" || return 1
> > +             )
> >       done
>
> We don't check that "cd" worked either before or after your patch.
> Should we?

I'd say "yes".

Show 13 quoted lines

> After your patch, we "return" from inside a subshell. Is that portable?
> ISTR issues around that before, but it just have been when we are not in
> a function at all. Still, I wonder if:
>
>   for ...
>   do
>         (
>                 cd "$subtree" &&
>                 generate_expected_cache_tree_rec "$dir$subtree"
>         ) || return 1
>   done
>
> might be more obvious.

Yes, a good recommendation. Normally, we `exit 1` from within subshells[1], but that wouldn't help us exit this loop early, so the `|| return 1` outside the subshell seems a good solution.

[1]: https://lore.kernel.org/git/20150325052952.GE31924@peff.net/