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

[Jeff King <peff@peff.net>]

On Mon, Apr 08, 2024 at 10:31:34AM -0700, Junio C Hamano wrote:

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

Yeah, I think a regex is probably going to end up with either false
positives or false negatives. It probably does not matter too much which
way we err, if we expect them to be rare on either side.

My thinking was mostly that false negatives are worse, because they only
matter on old buggy versions of dash (and only if the tests actually
pass a value with spaces). And so most developers will not notice them
immediately. Whereas false positives, while annoying, are reported to
them immediately by the linter. And generally, dealing with problems
closer to the time of writing means less work overall.

But I am happy to take your series as-is and we can see which cases (if
any!) we miss in practice.

I do hope that eventually we could just say "that buggy version of dash
does not matter anymore", but I think it is too soon for that (it sounds
like it is still being used in CI).

-Peff