| Message ID | xmqqbk6nyej1.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CodingGuidelines: quote assigned value with "local" and "export" | expand |
Somebody may want to inspect the output from
$ git grep -E -i -e '^ *(local|export) [a-z0-9_]*=\$'
and fix those that can be used with dash. I made a cursory scan and
removed obviously safe ones whose RHS will never have $IFS whitespaces,
and the following are remainders. #leftoverbits
t/t0610-reftable-basics.sh: local actual=$(ls -l "$file") &&
t/test-lib-functions.sh: local file=${2:-"$1.t"} &&
t/test-lib-functions.sh: local basename=${1#??}
t/test-lib-functions.sh: local var=$1 port
t/test-lib-functions.sh: local expr=$(printf '"%s",' "$@")
t/test-lib-functions.sh: local inc=${2:-0} &&
t/test-lib-functions.sh: local inc=${2:-0} &&
t/test-lib-functions.sh: local ret=$?
On Fri, Apr 05, 2024 at 09:12:34AM -0700, Junio C Hamano wrote: > Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097 > lets the shell erroneously perform field splitting on the expansion > of a command substitution during declaration of a local or an extern > variable. > > The explanation was stolen from ebee5580 (parallel-checkout: avoid > dash local bug in tests, 2021-06-06). Thanks for digging up that commit. I read the earlier part of the thread and went off on a wild goose chase in the archive. :) > + - Some versions of dash has broken variable assignment when prefixed > + with "local", "export", and "readonly", in that the value to be > + assigned goes through field splitting at $IFS unless quoted. > + > + DO NOT write: > + > + local variable=$value ;# wrong > + export variable=$(command args) ;# wrong > + > + and instead write: > + > + local variable="$value" > + export variable="$(command args)" 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. -Peff
Jeff King <peff@peff.net> writes: >> + - Some versions of dash has broken variable assignment when prefixed >> + with "local", "export", and "readonly", in that the value to be >> + assigned goes through field splitting at $IFS unless quoted. >> + >> + DO NOT write: >> + >> + local variable=$value ;# wrong >> + export variable=$(command args) ;# wrong >> + >> + and instead write: >> + >> + local variable="$value" >> + export variable="$(command args)" > > 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. As we are not showing "readonly" in the "DO NOT/DO" example above, we should probably drop the "export" example and discuss it separately and decide if it makes sense to loosen the "export var" vs "export var=val" rule. Thanks.
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/ 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.
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
diff --git i/Documentation/CodingGuidelines w/Documentation/CodingGuidelines index 32e69f798e..8aa76094ca 100644 --- i/Documentation/CodingGuidelines +++ w/Documentation/CodingGuidelines @@ -188,6 +188,20 @@ For shell scripts specifically (not exhaustive): hopefully nobody starts using "local" before they are reimplemented in C ;-) + - Some versions of dash has broken variable assignment when prefixed + with "local", "export", and "readonly", in that the value to be + assigned goes through field splitting at $IFS unless quoted. + + DO NOT write: + + local variable=$value ;# wrong + export variable=$(command args) ;# wrong + + and instead write: + + local variable="$value" + export variable="$(command args)" + - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g. "\xc2\xa2") in printf format strings, since hexadecimal escape sequences are not portable.