Re: [PATCH 08/10] t: forbid piping into 'test_i18ngrep'

From: Jeff King <peff@peff.net>
Date: Jan 26, 2018, 18:41 UTC
Message-ID: <20180126184151.GD27618@sigill.intra.peff.net>
In-Reply-To: <20180126123708.21722-9-szeder.dev@gmail.com>

On Fri, Jan 26, 2018 at 01:37:06PM +0100, SZEDER Gábor wrote:

Show 23 quoted lines

> When checking a git command's output with 'test_i18ngrep', it's
> tempting to conveniently pipe the git command's standard output into
> 'test_i18ngrep'.  Unfortunately, this is an anti-pattern, because it
> hides the git command's exit code, and the test could continue even if
> the command exited with error.
> 
> Add a bit of linting to 'test_i18ngrep' to detect when data is fed to
> its standard input and to error out with a "bug in the test script"
> message.
> 
> Note that this change will also forbid cases where 'test_i18ngrep'
> would legitimately read its standard input, e.g.
> 
>   - when its standard input is redirected from a file, or
> 
>   - when a git command's standard output is first written to an
>     intermediate file, which is then preprocessed by a non-git command
>     before the results are piped into 'test_i18ngrep'.
> 
> See two of the previous patches for the only such cases we had in our
> test suite.  However, reliably preventing this antipattern is arguably
> more important than supporting these cases, which can be worked around
> by only minor inconveniences.

The idea seems reasonable to me. Let's think about what the escape hatch looks like to work around it if you need to.

I guess you've got:

  cat >file &&
  test_i18ngrep ... file

which is not too bad.

You've also got:

  test_i18ngrep ... -

though that relies on the underlying grep understanding "-" (which is in POSIX, though with a rather vague "if the implementations supports it"). And it wouldn't work with the "read" test in this patch.

Show 12 quoted lines

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 92ed02937..e381d50d0 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -719,6 +719,10 @@ test_i18ncmp () {
>  # under GETTEXT_POISON this pretends that the command produced expected
>  # results.
>  test_i18ngrep () {
> +	( read line ) &&
> +	error "bug in the test script: data on test_i18ngrep's stdin;" \
> +	      "perhaps a git command's output is piped into it?"
> +

This seems kind of hacky compared to just seeing if there is a file argument. But I suppose that is hard to do, since we just pass through the arguments to grep.

Though looking at our test_18ngrep invocations, they are simple enough that would just ask "are there two non-option arguments at the end of the command line". The exception is "-e", but IMHO we could just drop that. It serves no purpose unless you're trying to hide a "-" at the start of your pattern, and in fact we used to ban it since sysv grep didn't understand it (e.g., aadbe44f883).

-Peff