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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 26, 2018, 18:39 UTC
Message-ID: <xmqqshas79di.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqq1sic8omp.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 23 quoted lines

> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> 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.
>
> I am not sure if that inconveniences will be minor.  Is this too
> contrived an example, for example?
>
>   check () {
>         pattern=$1 file=$2 script=./runme
>
>         test_i18ngrep "$pattern" "$file" &&
>         write_script "$script" &&
>         test_expect_success "check $pattern" '
>                 "$script"
>         '
>   }
>
>   check foo file <<-EOF
>   ... test script comes here ...
>   EOF

Is there a case where test_i18ngrep (after your clean-ups in this series up to 06/10) needs to read from more than one file?

I actually think that the kind of inconveniences we *can* work with, without risking breakage to legitimate test, would be to allow and require test_i18ngrep to name and read only from one file that appears at the end of its command line. IOW, instead of doing a probing "read" that you cannot undo and break legitimate test, I think it is OK to see if the last token names a file that is on the filesystem, e.g.

	test_i18ngrep () {
		eval test -f \"\${$#}\" ||
		error "bug in the test sript: test_i18ngrep must" \
		      "name a file to read as the last token on the command line"
		...