Re: [PATCH 12/16] test-reach: test reduce_heads

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 16, 2018, 21:59 UTC
Message-ID: <CAPig+cQ+pDrhYmsUPe69MzGK8YiH_cGOp_cd592ZEBC4i2Lkhw@mail.gmail.com>
In-Reply-To: <CAGZ79kbV-ahEYdCPyp6-mo+hVYr1c2+Xf-F5xYD3mCwBX9g16g@mail.gmail.com>

On Mon, Jul 16, 2018 at 5:30 PM Stefan Beller <sbeller@google.com> wrote:

Show 28 quoted lines

> > +test_expect_success 'reduce_heads' '
> > +       cat >input <<-\EOF &&
> > +               X:commit-1-10
> > +               X:commit-2-8
> > +               X:commit-3-6
> > +               X:commit-4-4
> > +               X:commit-1-7
> > +               X:commit-2-5
> > +               X:commit-3-3
> > +               X:commit-5-1
> > +       EOF
> > +       {
> > +               printf "reduce_heads(X):\n" &&
> > +               git rev-parse commit-5-1 &&
> > +               git rev-parse commit-4-4 &&
> > +               git rev-parse commit-3-6 &&
> > +               git rev-parse commit-2-8 &&
> > +               git rev-parse commit-1-10
> > +      } >expect &&
>
> Please use rev-parse only once.
>
> I am not sure about the usage of { braces } in the test suite,
> +cc Eric who sent a test suite linting series recently.
> Do we need to em-'brace' the statements that describe the
> expected behavior? (Or is it supposed to be easier to read
> for the reviewers? I found these very readable so far... but
> this question just came up)

Grouping the commands for redirection via a "{...}>expect" block is less noisy than redirecting each command separately, thus more reviewer-friendly. And, {...} blocks are used regularly in the test suite, so no issue there.

I do agree that a single git-rev-parse with all 5 arguments makes more sense (and would be appreciated by Windows folk). Also, the 'printf' could be replaced by a simple 'echo' if we want to get nit-picky.

Finally, a style nit: We don't normally indent the content of a here-doc like that. Instead, the content is normally aligned with the closing EOF, not indented beyond it.