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

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

On Wed, Jun 06, 2018 at 04:58:46PM -0400, Jeff King wrote:
> 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.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]