Re: [PATCH 01/10] t: add tool to translate hash-related values

From: Jeff King <peff@peff.net>
Date: Jun 6, 2018, 20:58 UTC
Message-ID: <20180606205845.GB1381@sigill.intra.peff.net>
In-Reply-To: <20180606061927.GA7098@tor.lan>

On Wed, Jun 06, 2018 at 08:19:27AM +0200, Torsten Bögershausen wrote:

> > +test_translate_f_ () {
> > +	local file="$TEST_DIRECTORY/translate/$2" &&
> 
> Unless I'm wrong, we don't use the "local" keyword ?

We've got a test balloon out; see 01d3a526ad (t0000: check whether the shell supports the "local" keyword, 2017-10-26). I think it's reasonable to consider starting its use.

> > +	perl -e '
> 
> The bare "perl" is better spelled as "$PERL_PATH"

This use is OK. Since a0e0ec9f7d (t: provide a perl() function which uses $PERL_PATH, 2013-10-28), most common uses handle this automatically (there are some exceptions, covered in t/README).

> > +	if [ "$1" = "-f" ]
> 
> Style nit, please avoid [] and use test:
> 	if test "$1" = "-f"

This I agree with. :)

-Peff