Re: [PATCH v2 8/9] t: validate 'test_i18ngrep's parameters

From: Jeff King <peff@peff.net>
Date: Feb 8, 2018, 16:34 UTC
Message-ID: <20180208163416.GA13078@sigill.intra.peff.net>
In-Reply-To: <20180208155656.9831-9-szeder.dev@gmail.com>

On Thu, Feb 08, 2018 at 04:56:55PM +0100, SZEDER Gábor wrote:

Show 5 quoted lines

> Prevent similar mistakes in the future by validating 'test_i18ngrep's
> parameters requiring that
> 
>   - The last parameter names an existing file to be read, effectively
>     forbiding piping into 'test_i18ngrep'.

s/forbiding/forbidding/

Show 22 quoted lines

>     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 the piping antipattern is arguably more important than
>     supporting these cases, which can be easily worked around by
>     opening the file directly or using an intermediate file anyway.
> 
>   - There are at least two parameters, not including the optional '!'
>     to negate the pattern.  This ought to catch corner cases when
>     'test_i18ngrep' looks for the name of an existing file on its
>     standard input; the above check would miss this case becase the
>     filename as pattern would be the last parameter.
> 
>     Note that this is not quite perfect, as it doesn't account for any
>     'grep --options' given as parameters.  However, doing so would be
>     far too complicated, considering that patterns can start with
>     dashes as well, and in the majority of the cases we don't use any
>     such options anyway.

And most importantly, we never err on the side of complaining unnecessarily. So our safety might not kick in, but as long as it kicks in most of the time, we're fine.

I like this approach much better than the previous round.

Show 9 quoted lines

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 92ed029371..a1676e0386 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -719,6 +719,18 @@ test_i18ncmp () {
>  # under GETTEXT_POISON this pretends that the command produced expected
>  # results.
>  test_i18ngrep () {
> +	eval "last_arg=\"\${$#}\""

These embedded double-quotes are unnecessary, I think, because it's a variable assignment: E.g.:

  set -- one two 'foo bar'
  eval "last_arg=\${$#}"
  echo $last_arg

should produce "foo bar".

Usually not a big deal, but because of the extra quoting it may make the whole thing a bit more readable to drop them.

-Peff