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
