Re: [PATCH v6] format-patch --signature-file <file>

From: Jeff King <peff@peff.net>
Date: May 21, 2014, 22:02 UTC
Message-ID: <20140521220250.GA11970@sigill.intra.peff.net>
In-Reply-To: <xmqqoayqoktp.fsf@gitster.dls.corp.google.com>

On Wed, May 21, 2014 at 02:58:42PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> >> >  	! grep "^-- \$" output
> > ...
> >> 
> >> We have been trying not to do the above in recent test updates.  It
> >> would be nice if this set-up did not have to be outside of the usual
> >> test_expect_success structure.
> >> 
> >
> > Jeff caught those "! grep" instances in my patch.
> 
> Hmm, I didn't mean that one, and I do not offhand what is wrong
> about "! grep" that says "output should not contain this string".

I think he is responding to my earlier request to use test_must_fail instead of "!". But there is a subtlety there he does not know, which is that we typically only use the former for git programs, and rely on "!" for normal Unix commands.

Show 30 quoted lines

> The problem is a "cat" you added outside test_expect_*; the recent
> push is to have as little executable outside them, especially the
> "set-up" code to prepare for the real tests.
> 
> i.e. we have been trying to write new tests (and convert old ones)
> like this:
> 
>         test_expect_success 'I test such and such ' '
>                 cat >input-for-test <<-\EOF &&
>                 here comes input
>                 EOF
>                 git command-to-be-tested <input-for-test >actual &&
>                 cat >expected <<-\EOF &&
>                 here comes expected output
>                 EOF
>                 test_cmp expected actual
>         '
> 
> not like this:
> 
>         cat >input-for-test <<-\EOF &&
>         here comes input
>         EOF
>         test_expect_success 'I test such and such ' '
>                 git command-to-be-tested <input-for-test >actual &&
>                 cat >expected <<-\EOF &&
>                 here comes expected output
>                 EOF
>                 test_cmp expected actual
>         '

Yeah, I noticed and gave a pass on this in earlier review, because the file is used across many tests. So burying it in the first test that uses it is probably a bad thing. However, it could go in its own setup test.

-Peff