Re: [PATCH 6/6] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 8, 2024, 17:31 UTC
Message-ID: <xmqqa5m3damh.fsf@gitster.g>
In-Reply-To: <20240407014344.GF1085004@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 27 quoted lines

> On Fri, Apr 05, 2024 at 05:09:02PM -0700, Junio C Hamano wrote:
>
>> Teach t/check-non-portable-shell.pl that right hand side of the
>> assignment done with "local VAR=VAL" need to be quoted.  We
>> deliberately target only VAL that begins with $ so that we can catch
>> 
>>  - $variable_reference and positional parameter reference like $4
>>  - $(command substitution)
>>  - ${variable_reference-with_magic}
>> 
>> while excluding
>> 
>>  - $'\n' that is a bash-ism freely usable in t990[23]
>>  - $(( arithmetic )) whose result should be $IFS safe.
>>  - $? that also is $IFS safe
>
> Hmm. Just porting over my comment from the other thread (before I
> realized you'd written this series), this misses:
>
>   local foo=bar/$1
>
> etc. Should we look for the "$" anywhere on the line? I doubt we can get
> things foolproof, but requiring somebody to quote:
>
>   local foo=$((1+2))
>
> does not seem like the worst outcome. I dunno.

Looking at the output from

    $ git grep -E -e 'local [a-zA-Z0-9_]+=[^"]*[$]' t/

the listed ones in the proposed commit log message are the false positives. Luckily we didn't have anything that tries to concatenate parameter reference to something else.

But with the pattern we do miss

    local var=$*

and possibly many others. So I am not sure. The false positives do look moderately bad, so I'd rather start with the simplest one proposed in the patch.