Re: [PATCH 11/19] tests: fix broken &&-chains in `$(...)` command substitutions

From: Jeff King <peff@peff.net>
Date: Dec 10, 2021, 09:27 UTC
Message-ID: <YbMddynehrDgsSWO@coredump.intra.peff.net>
In-Reply-To: <CAPig+cQzTnQiwT4Nmpp8qYJOaRpy2pK7DDOu42Wk-1TfmbUxSg@mail.gmail.com>

On Thu, Dec 09, 2021 at 11:53:47AM -0500, Eric Sunshine wrote:

Show 16 quoted lines

> On Thu, Dec 9, 2021 at 11:44 AM Elijah Newren <newren@gmail.com> wrote:
> > On Wed, Dec 8, 2021 at 11:39 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > >  test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' '
> > > -       OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) &&
> > > +       OUT=$( ((trap "" PIPE && large_git; echo $? 1>&3) | :) 3>&1 ) &&
> >
> > Shouldn't the second ';' be replaced with '&&' as well?
> 
> Thanks for reading so carefully. In this case, the answer is "no", the
> semicolon is correct. This code legitimately wants to capture in the
> OUT variable the numeric exit status of the command preceding `echo
> $?`. If the semicolon is replaced with `&&`, then the echo won't be
> executed if the exit status is non-zero, but we want `echo` to be
> executed regardless of the exit status. So, the code is correct with
> the semicolon, and would be incorrect with `&&`. (I hope I'm
> explaining this well enough to make sense.)

That makes sense to me. I wondered why it was even worth changing the earlier semi-colon in that case, then, but...

> It's this sort of special case which accounts for why the new linter
> (as mentioned in the cover letter) has special understanding that a
> broken &&-chain can be legitimate in certain circumstances, such as
> explicit handling of `$?`.

...your unseen magic script explains it. :)

All of the changes here look reasonable. We'd either want to know about failure (e.g., "cd") or don't expect it to fail (e.g., "echo").

These "trap" calls are probably fine. I can't imagine why they'd fail, but being a weird shell builtin I wonder if it's possible for them to fail in odd circumstances. I'm happy to leave that as a hypothetical until we see it in practice.

-Peff