From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 15 Sep 2025 11:41:18 GMT
Subject: Re: [PATCH 1/5] t1300: write test expectations in the test's body
Message-ID: <aMf7XvXasOiUAaxo@pks.im>
In-Reply-To: <94eb9052-18e1-4565-8f33-42fdf136e2a4@app.fastmail.com>

On Thu, Sep 11, 2025 at 06:50:29PM +0200, Kristoffer Haugsbakk wrote:
> On Thu, Sep 11, 2025, at 15:24, Patrick Steinhardt wrote:
> > There are a bunch of tests in t1300 where we write the test expectation
> > handed over to `test_cmp ()` outside of the test body. This does not
> > match our modern test style, and there isn't really a reason why this
> > would need to happen outside of the test bodies.
> >
> > Convert those to instead do so as part of the test itself.
> >
> > Note that there are two exceptions that we don't convert. In both of
> > these cases the expectation is reused across multiple tests, and thus a
> > conversion where we'd move that into the first test that uses the
> > expectation would be invalid. Those are simply left as-is for now.
> 
> This is just a suggestion (which everything is):
> 
>     Note that there are two exceptions that we leave as-is for now since
>     they are reused across tests.

Yup, that reads way better.

> > index f856821839..bde9bda234 100755
> > --- a/t/t1300-config.sh
> > +++ b/t/t1300-config.sh
> > @@ -134,38 +134,39 @@ test_expect_success 'clear default config' '
> >  	rm -f .git/config
> >  '
> >
> > -cat > expect << EOF > +test_expect_success 'initial' '
> > +	cat >expect <<EOF &&
> >  [section]
> >  	penguin = little blue
> >  EOF
> > -test_expect_success 'initial' '
> 
> Ok.  Correct.  But I see that you are also overall doing a sort of
> normalization.
> 
> • Remove-tabs if it makes sense
> • No variable expansion if it makes sense
> 
> And here `<<\EOF` would work too.  Or is that worse style?
> 
> I will keep mentioning this throughout the rest.

No, it's not. I already did it sometimes, but not consistently. I'll
mention this in the commit message and adapt as possible.

> > @@ -214,7 +213,9 @@ test_expect_success 'Prohibited LF in comment' '
> >  	test_must_fail git config ${mode_set} --comment="a${LF}b" section.k v
> >  '
> >
> > -test_expect_success 'non-match result' 'test_cmp expect .git/config'
> > +test_expect_success 'non-match result' '
> > +	test_cmp expect .git/config
> > +'
> 
> Okay.  You normalize the one line to
> 
>     test_expect_success <name> '
>          <body>
>     '
> 
> The same kind of change done in the next patch `t1300: small
> style fixups`.

True, will move into the next patch.

Thanks for going through all of these!

Patrick