Re: [PATCH v3] fetch: add new config option fetch.all

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 6, 2024, 23:37 UTC
Message-ID: <CAPig+cSe5QjbMq6aKqM1=0ZBQFize=WvrNtTnvy6GQ4mYKkn+g@mail.gmail.com>
In-Reply-To: <CAPig+cQ3y7rGbrNjMZB_HpMAWYOhDg8qKSoR5EF=T5+jc6rgvA@mail.gmail.com>

On Sat, Jan 6, 2024 at 6:32 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 12 quoted lines

> On Sat, Jan 6, 2024 at 3:25 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> > +create_fetch_all_expect () {
> > +       cat >expect <<-\EOF || return 1
> > +         ...
> > +       EOF
> > +}
>
> This is really minor, but the `|| return 1` is superfluous. The `cat`
> command itself will exit with success or failure, and since it's the
> last command in the function, its return value will be the value
> returned by the function. Thus, there is no need to use `|| return 1`
> to signal failure when the `cat` command itself will do so anyhow.

Just for completeness, the other `|| return 1` in your patch...

> > +       for r in one two three
> > +       do
> > +               git -C "$test_dir" remote add "$r" "../$r" || return 1
> > +       done

... is necessary, thus correct, since shell for-loops don't terminate automatically when a command in the loop body fails; the loop continues regardless. Thus, this `|| return 1` ensures that we correctly abort (signalling a failure) as soon as the error is discovered.