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
