Re: [PATCH] CodingGuidelines: quote assigned value with "local" and "export"

[Jeff King <peff@peff.net>]

On Fri, Apr 05, 2024 at 04:15:57PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> I think that is a good rule for "local", but I thought we did not allow
> >> "export foo=bar" at all, and required:
> >>
> >>   foo=bar
> >>   export foo
> >>
> >> If that was only because of this bug, it would be nice to loosen the
> >> rules a bit.
> >
> > That rule in Documentation/CodingGuidelines predates the discovery
> > of this bug.  I have this vague feeling that it was for the shell on
> > old Solaris, which would not matter to us anymore, but I do not
> > remember.
> 
> Heh, I do not even see any such rule in the guidelines.  What
> enforces it is actually in t/check-non-portable-shell.pl script.  It
> came from 9968ffff (test-lint: detect 'export FOO=bar', 2013-07-08),
> which in turn comes from https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/

Yeah, I noticed it was not in CodingGuidelines, but did not even realize
it was in the linter. Thanks for digging up the link, though sadly it
does not say which shell had a problem. I could very well believe there
is no such modern shell, but I don't know how to test that without a
weather balloon patch.

> We make multiple uses of it in ci/*.sh but the environments ci/
> scripts are used in are rather sterile, so they do not quite count
> as a proof that the problematic shells no longer exist.
> 
> We may instead want to add a separate rule e.g.,
> 
> 	/\blocal\s+[a-zA-z0-9_]*=\$/ and err q(quote "$val" in 'local var=$val');
> 
> to the check script.

Yeah. I think matching \$ is probably enough to catch most relevant
cases without complaining about the innocuous:

  local foo=bar

It would miss:

  local foo="bar/$1"

I guess "=[^"]*\$" would be a bit more aggressive.

-Peff