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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Jun 7, 2018, 00:57 UTC
Message-ID: <20180607005704.GA862596@genre.crustytoothpaste.net>
In-Reply-To: <20180606205845.GB1381@sigill.intra.peff.net>

On Wed, Jun 06, 2018 at 04:58:46PM -0400, Jeff King wrote:

Show 10 quoted lines

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

I used it because it's already in use earlier in the file in some of the mingw_* functions. Perhaps we happen to know that our mingw systems will always have a suitable /bin/sh, but I suppose some less capable shells would still have choked on it by now.

I can remove it if necessary, but it didn't seem necessary.

Show 7 quoted lines

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

This was exactly my reasoning.

Show 6 quoted lines

> > > +	if [ "$1" = "-f" ]
> > 
> > Style nit, please avoid [] and use test:
> > 	if test "$1" = "-f"
> 
> This I agree with. :)

Yeah, I forgot that that's our style.  I'll fix that.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204