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

[Jeff King <peff@peff.net>]

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

> 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.

> 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