{"thread":{"id":"61277","subject":"[PATCH 0/6] local VAR=\"VAL\"","startedAt":"2024-04-06T00:09:05Z","lastAt":"2024-04-08T20:40:37Z","messageCount":23,"participants":["Junio C Hamano","rsbecker@nexbridge.com","Eric Sunshine","Andreas Schwab","Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"492331","messageId":"20240406000902.3082301-1-gitster@pobox.com","threadId":"61277","inReplyTo":null,"subject":"[PATCH 0/6] local VAR=\"VAL\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T00:08:56Z","receivedAt":"2024-04-06T00:09:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"* Update coding guidelines and test-lint script so that we quote the\n right hand side of assignment used with \"local\", which is buggy on\n certain versions of dash.\n\nThe first patch is not about the theme of the topic, but to document\na rule enforced by the test-lint script that is not written down in\nthe coding guidelines.\n\nThe second patch gives guidance to avoid the dash bug.\n\nPatches [3/6], [4/6], and [5/6] are to adjust the existing tests. I\nthink many of them are currently safe because the values they assign\nare $IFS safe, but some may be real workarounds for the dash bug.\n\nThe last patch introduces the test-lint pattern.\n\nJunio C Hamano (6):\n CodingGuidelines: describe \"export VAR=VAL\" rule\n CodingGuidelines: quote assigned value in 'local var=$val'\n t: local VAR=\"VAL\" (quote positional parameters)\n t: local VAR=\"VAL\" (quote command substitution)\n t: local VAR=\"VAL\" (quote ${magic-reference})\n t: teach lint that RHS of 'local VAR=VAL' needs to be quoted\n\n Documentation/CodingGuidelines | 20 ++++++++++++++++++++\n t/check-non-portable-shell.pl | 2 ++\n t/lib-parallel-checkout.sh | 2 +-\n t/t2400-worktree-add.sh | 2 +-\n t/t4011-diff-symlink.sh | 4 ++--\n t/t4210-log-i18n.sh | 4 ++--\n t/test-lib-functions.sh | 12 ++++++------\n 7 files changed, 34 insertions(+), 12 deletions(-)\n\n-- \n2.44.0-501-g19981daefd\n\n"},{"id":"492332","messageId":"20240406000902.3082301-2-gitster@pobox.com","threadId":"61277","inReplyTo":"20240406000902.3082301-1-gitster@pobox.com","subject":"[PATCH 1/6] CodingGuidelines: describe \"export VAR=VAL\" rule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T00:08:57Z","receivedAt":"2024-04-06T00:09:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/\nresulted in 9968ffff (test-lint: detect 'export FOO=bar',\n2013-07-08) to add a rule to t/check-non-portable-shell.pl script to\nreject\n\n\texport VAR=VAL\n\nand suggest us to instead write it as \"export VAR\" followed by\n\"VAR=VAL\". This however was not spelled out in the CodingGuidelines\ndocument.\n\nWe may want to re-evaluate the rule since it is from ages ago, but\nfor now, let's make the written rule and what the automation enforces\nconsistent.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 9495df835d..0a39205c48 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -188,6 +188,12 @@ For shell scripts specifically (not exhaustive):\n hopefully nobody starts using \"local\" before they are reimplemented\n in C ;-)\n \n+ - Some versions of shell do not understand \"export variable=value\",\n+ so we write \"export variable\" and \"variable=value\" on separae\n+ lines. Note that this was reported in 2013 and the situation might\n+ have changed since then. We'd need to re-evaluate this rule,\n+ together with the rule in t/check-non-portable-shell.pl script.\n+\n - Use octal escape sequences (e.g. \"\\302\\242\"), not hexadecimal (e.g.\n \"\\xc2\\xa2\") in printf format strings, since hexadecimal escape\n sequences are not portable.\n-- \n2.44.0-501-g19981daefd\n\n"},{"id":"492333","messageId":"20240406000902.3082301-4-gitster@pobox.com","threadId":"61277","inReplyTo":"20240406000902.3082301-1-gitster@pobox.com","subject":"[PATCH 3/6] t: local VAR=\"VAL\" (quote positional parameters)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T00:08:59Z","receivedAt":"2024-04-06T00:09:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Future-proof test scripts that do\n\n\tlocal VAR=VAL\n\nwithout quoting VAL (which is OK in POSIX but broken in some shells)\nthat is a positional parameter, e.g. $4.\n\nSigned-off-by: Junio C Hamano \n---\n t/lib-parallel-checkout.sh | 2 +-\n t/t2400-worktree-add.sh | 2 +-\n t/t4210-log-i18n.sh | 4 ++--\n t/test-lib-functions.sh | 2 +-\n 4 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex acaee9cbb6..8324d6c96d 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -20,7 +20,7 @@ test_checkout_workers () {\n \t\tBUG \"too few arguments to test_checkout_workers\"\n \tfi &&\n \n-\tlocal expected_workers=$1 &&\n+\tlocal expected_workers=\"$1\" &&\n \tshift &&\n \n \tlocal trace_file=trace-test-checkout-workers &&\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 051363acbb..5851e07290 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -404,7 +404,7 @@ test_expect_success '\"add\" worktree with orphan branch, lock, and reason' '\n # Note: Quoted arguments containing spaces are not supported.\n test_wt_add_orphan_hint () {\n \tlocal context=\"$1\" &&\n-\tlocal use_branch=$2 &&\n+\tlocal use_branch=\"$2\" &&\n \tshift 2 &&\n \tlocal opts=\"$*\" &&\n \ttest_expect_success \"'worktree add' show orphan hint in bad/orphan HEAD w/ $context\" '\ndiff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\nindex d2dfcf164e..75216f19ce 100755\n--- a/t/t4210-log-i18n.sh\n+++ b/t/t4210-log-i18n.sh\n@@ -64,7 +64,7 @@ test_expect_success 'log --grep does not find non-reencoded values (latin1)' '\n '\n \n triggers_undefined_behaviour () {\n-\tlocal engine=$1\n+\tlocal engine=\"$1\"\n \n \tcase $engine in\n \tfixed)\n@@ -85,7 +85,7 @@ triggers_undefined_behaviour () {\n }\n \n mismatched_git_log () {\n-\tlocal pattern=$1\n+\tlocal pattern=\"$1\"\n \n \tLC_ALL=$is_IS_locale git log --encoding=ISO-8859-1 --format=%s \\\n \t\t--grep=$pattern\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 2f8868caa1..3204afbafb 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1689,7 +1689,7 @@ test_parse_ls_tree_oids () {\n # Choose a port number based on the test script's number and store it in\n # the given variable name, unless that variable already contains a number.\n test_set_port () {\n-\tlocal var=$1 port\n+\tlocal var=\"$1\" port\n \n \tif test $# -ne 1 || test -z \"$var\"\n \tthen\n-- \n2.44.0-501-g19981daefd\n\n"},{"id":"492334","messageId":"20240406000902.3082301-3-gitster@pobox.com","threadId":"61277","inReplyTo":"20240406000902.3082301-1-gitster@pobox.com","subject":"[PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T00:08:58Z","receivedAt":"2024-04-06T00:09:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097\nlets the shell erroneously perform field splitting on the expansion\nof a command substitution during declaration of a local or an extern\nvariable.\n\nThe explanation was stolen from ebee5580 (parallel-checkout: avoid\ndash local bug in tests, 2021-06-06).\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 0a39205c48..1cb77a871b 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -194,6 +194,20 @@ For shell scripts specifically (not exhaustive):\n have changed since then. We'd need to re-evaluate this rule,\n together with the rule in t/check-non-portable-shell.pl script.\n \n+ - Some versions of dash have broken variable assignment when prefixed\n+ with \"local\", \"export\", and \"readonly\", in that the value to be\n+ assigned goes through field splitting at $IFS unless quoted.\n+\n+ DO NOT write:\n+\n+ local variable=$value ;# wrong\n+ local variable=$(command args) ;# wrong\n+\n+ and instead write:\n+\n+ local variable=\"$value\"\n+ local variable=\"$(command args)\"\n+\n - Use octal escape sequences (e.g. \"\\302\\242\"), not hexadecimal (e.g.\n \"\\xc2\\xa2\") in printf format strings, since hexadecimal escape\n sequences are not portable.\n-- \n2.44.0-501-g19981daefd\n\n"},{"id":"492335","messageId":"20240406000902.3082301-5-gitster@pobox.com","threadId":"61277","inReplyTo":"20240406000902.3082301-1-gitster@pobox.com","subject":"[PATCH 4/6] t: local VAR=\"VAL\" (quote command substitution)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T00:09:00Z","receivedAt":"2024-04-06T00:09:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Future-proof test scripts that do\n\n\tlocal VAR=VAL\n\nwithout quoting VAL (which is OK in POSIX but broken in some shells)\nthat is a $(command substitution).\n\nSigned-off-by: Junio C Hamano \n---\n t/t4011-diff-symlink.sh | 4 ++--\n t/test-lib-functions.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\nindex d7a5f7ae78..bc8ba88719 100755\n--- a/t/t4011-diff-symlink.sh\n+++ b/t/t4011-diff-symlink.sh\n@@ -13,13 +13,13 @@ TEST_PASSES_SANITIZE_LEAK=true\n \n # Print the short OID of a symlink with the given name.\n symlink_oid () {\n-\tlocal oid=$(printf \"%s\" \"$1\" | git hash-object --stdin) &&\n+\tlocal oid=\"$(printf \"%s\" \"$1\" | git hash-object --stdin)\" &&\n \tgit rev-parse --short \"$oid\"\n }\n \n # Print the short OID of the given file.\n short_oid () {\n-\tlocal oid=$(git hash-object \"$1\") &&\n+\tlocal oid=\"$(git hash-object \"$1\")\" &&\n \tgit rev-parse --short \"$oid\"\n }\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 3204afbafb..3dc638f7dc 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1764,7 +1764,7 @@ test_subcommand () {\n \t\tshift\n \tfi\n \n-\tlocal expr=$(printf '\"%s\",' \"$@\")\n+\tlocal expr=\"$(printf '\"%s\",' \"$@\")\"\n \texpr=\"${expr%,}\"\n \n \tif test -n \"$negate\"\n-- \n2.44.0-501-g19981daefd\n\n"},{"id":"492336","messageId":"20240406000902.3082301-6-gitster@pobox.com","threadId":"61277","inReplyTo":"20240406000902.3082301-1-gitster@pobox.com","subject":"[PATCH 5/6] t: local VAR=\"VAL\" (quote ${magic-reference})","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T00:09:01Z","receivedAt":"2024-04-06T00:09:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Future-proof test scripts that do\n\n\tlocal VAR=VAL\n\nwithout quoting VAL (which is OK in POSIX but broken in some shells)\nthat is ${magic-\"reference to a parameter\"}.\n\nSigned-off-by: Junio C Hamano \n---\n t/test-lib-functions.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 3dc638f7dc..029cb31ffe 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -330,7 +330,7 @@ test_commit () {\n \t\tshift\n \tdone &&\n \tindir=${indir:+\"$indir\"/} &&\n-\tlocal file=${2:-\"$1.t\"} &&\n+\tlocal file=\"${2:-\"$1.t\"}\" &&\n \tif test -n \"$append\"\n \tthen\n \t\t$echo \"${3-$1}\" >>\"$indir$file\"\n@@ -1672,7 +1672,7 @@ test_oid () {\n # Insert a slash into an object ID so it can be used to reference a location\n # under \".git/objects\". For example, \"deadbeef...\" becomes \"de/adbeef..\".\n test_oid_to_path () {\n-\tlocal basename=${1#??}\n+\tlocal basename=\"${1#??}\"\n \techo \"${1%$basename}/$basename\"\n }\n \n@@ -1840,7 +1840,7 @@ test_readlink () {\n # An optional increment to the magic timestamp may be specified as second\n # argument.\n test_set_magic_mtime () {\n-\tlocal inc=${2:-0} &&\n+\tlocal inc=\"${2:-0}\" &&\n \tlocal mtime=$((1234567890 + $inc)) &&\n \ttest-tool chmtime =$mtime \"$1\" &&\n \ttest_is_magic_mtime \"$1\" $inc\n@@ -1853,7 +1853,7 @@ test_set_magic_mtime () {\n # argument. Usually, this should be the same increment which was used for\n # the associated test_set_magic_mtime.\n test_is_magic_mtime () {\n-\tlocal inc=${2:-0} &&\n+\tlocal inc=\"${2:-0}\" &&\n \tlocal mtime=$((1234567890 + $inc)) &&\n \techo $mtime >.git/test-mtime-expect &&\n \ttest-tool chmtime --get \"$1\" >.git/test-mtime-actual &&\n-- \n2.44.0-501-g19981daefd\n\n"},{"id":"492337","messageId":"20240406000902.3082301-7-gitster@pobox.com","threadId":"61277","inReplyTo":"20240406000902.3082301-1-gitster@pobox.com","subject":"[PATCH 6/6] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T00:09:02Z","receivedAt":"2024-04-06T00:09:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teach t/check-non-portable-shell.pl that right hand side of the\nassignment done with \"local VAR=VAL\" need to be quoted. We\ndeliberately target only VAL that begins with $ so that we can catch\n\n - $variable_reference and positional parameter reference like $4\n - $(command substitution)\n - ${variable_reference-with_magic}\n\nwhile excluding\n\n - $'\\n' that is a bash-ism freely usable in t990[23]\n - $(( arithmetic )) whose result should be $IFS safe.\n - $? that also is $IFS safe\n\nSigned-off-by: Junio C Hamano \n---\n t/check-non-portable-shell.pl | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex dd8107cd7d..b2b28c2ced 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -47,6 +47,8 @@ sub err {\n \t/\\bgrep\\b.*--file\\b/ and err 'grep --file FILE is not portable (use grep -f FILE)';\n \t/\\b[ef]grep\\b/ and err 'egrep/fgrep obsolescent (use grep -E/-F)';\n \t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (use FOO=bar && export FOO)';\n+\t/\\blocal\\s+[A-Za-z0-9_]*=\\$([A-Za-z0-9_{]|[(][^(])/ and\n+\t\terr q(quote \"$val\" in 'local var=$val');\n \t/^\\s*([A-Z0-9_]+=(\\w*|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n \t\terr '\"FOO=bar shell_func\" assignment extends beyond \"shell_func\"';\n \t$line = '';\n-- \n2.44.0-501-g19981daefd\n\n"},{"id":"492339","messageId":"xmqqttkfs5jl.fsf@gitster.g","threadId":"61277","inReplyTo":"20240406000902.3082301-1-gitster@pobox.com","subject":"[PATCH 7/6] t0610: local VAR=\"VAL\" fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T00:23:10Z","receivedAt":"2024-04-06T00:23:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The series was based on maint and fixes all the tests that exist\nthere, but we have acquired a few more.\n\nSigned-off-by: Junio C Hamano \n---\n t/t0610-reftable-basics.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git i/t/t0610-reftable-basics.sh w/t/t0610-reftable-basics.sh\nindex 686781192e..c8074ebab2 100755\n--- i/t/t0610-reftable-basics.sh\n+++ w/t/t0610-reftable-basics.sh\n@@ -83,7 +83,7 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '\n test_expect_perms () {\n \tlocal perms=\"$1\"\n \tlocal file=\"$2\"\n-\tlocal actual=$(ls -l \"$file\") &&\n+\tlocal actual=\"$(ls -l \"$file\")\" &&\n \n \tcase \"$actual\" in\n \t$perms*)\n"},{"id":"492340","messageId":"xmqqil0vs5b3.fsf@gitster.g","threadId":"61277","inReplyTo":"20240406000902.3082301-1-gitster@pobox.com","subject":"[PATCH 8/6] t1016: local VAR=\"VAL\" fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T00:28:16Z","receivedAt":"2024-04-06T00:28:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The series was based on maint and fixes all the tests that exist\nthere, but we have acquired a few more.\n\nI suspect that the values assigned in many of these places are $IFS\nsafe, and this is primarily to squelch the linter than adding a\nnecessary workaround for buggy dash.\n\nSigned-off-by: Junio C Hamano \n---\n t/t1016-compatObjectFormat.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git c/t/t1016-compatObjectFormat.sh w/t/t1016-compatObjectFormat.sh\nindex 8132cd37b8..be3206a16f 100755\n--- c/t/t1016-compatObjectFormat.sh\n+++ w/t/t1016-compatObjectFormat.sh\n@@ -79,7 +79,7 @@ commit2_oid () {\n }\n \n del_sigcommit () {\n- local delete=$1\n+ local delete=\"$1\"\n \n if test \"$delete\" = \"sha256\" ; then\n \tlocal pattern=\"gpgsig-sha256\"\n@@ -91,8 +91,8 @@ del_sigcommit () {\n \n \n del_sigtag () {\n- local storage=$1\n- local delete=$2\n+ local storage=\"$1\"\n+ local delete=\"$2\"\n \n if test \"$storage\" = \"$delete\" ; then\n \tlocal pattern=\"trailer\"\n@@ -181,7 +181,7 @@ done\n cd \"$base\"\n \n compare_oids () {\n- test \"$#\" = 5 && { local PREREQ=$1; shift; } || PREREQ=\n+ test \"$#\" = 5 && { local PREREQ=\"$1\"; shift; } || PREREQ=\n local type=\"$1\"\n local name=\"$2\"\n local sha1_oid=\"$3\"\n@@ -193,8 +193,8 @@ compare_oids () {\n \n git --git-dir=repo-sha1/.git rev-parse --output-object-format=sha256 ${sha1_oid} > ${name}_sha1_sha256_found\n git --git-dir=repo-sha256/.git rev-parse --output-object-format=sha1 ${sha256_oid} > ${name}_sha256_sha1_found\n- local sha1_sha256_oid=$(cat ${name}_sha1_sha256_found)\n- local sha256_sha1_oid=$(cat ${name}_sha256_sha1_found)\n+ local sha1_sha256_oid=\"$(cat ${name}_sha1_sha256_found)\"\n+ local sha256_sha1_oid=\"$(cat ${name}_sha256_sha1_found)\"\n \n test_expect_success $PREREQ \"Verify ${type} ${name}'s sha1 oid\" '\n \tgit --git-dir=repo-sha256/.git rev-parse --output-object-format=sha1 ${sha256_oid} > ${name}_sha1 &&\n"},{"id":"492356","messageId":"02c801da87c1$eac025f0$c04071d0$@nexbridge.com","threadId":"61277","inReplyTo":"20240406000902.3082301-3-gitster@pobox.com","subject":"RE: [PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-04-06T01:29:44Z","receivedAt":"2024-04-06T01:29:58Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Friday, April 5, 2024 8:09 PM, Junio C Hamano wrote:\n>Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097\n>lets the shell erroneously perform field splitting on the expansion of a\ncommand\n>substitution during declaration of a local or an extern variable.\n>\n>The explanation was stolen from ebee5580 (parallel-checkout: avoid dash\nlocal bug\n>in tests, 2021-06-06).\n>\n>Signed-off-by: Junio C Hamano \n>---\n> Documentation/CodingGuidelines | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n>\n>diff --git a/Documentation/CodingGuidelines\nb/Documentation/CodingGuidelines\n>index 0a39205c48..1cb77a871b 100644\n>--- a/Documentation/CodingGuidelines\n>+++ b/Documentation/CodingGuidelines\n>@@ -194,6 +194,20 @@ For shell scripts specifically (not exhaustive):\n> have changed since then. We'd need to re-evaluate this rule,\n> together with the rule in t/check-non-portable-shell.pl script.\n>\n>+ - Some versions of dash have broken variable assignment when prefixed\n>+ with \"local\", \"export\", and \"readonly\", in that the value to be\n>+ assigned goes through field splitting at $IFS unless quoted.\n>+\n>+ DO NOT write:\n>+\n>+ local variable=$value ;# wrong\n>+ local variable=$(command args) ;# wrong\n>+\n>+ and instead write:\n>+\n>+ local variable=\"$value\"\n>+ local variable=\"$(command args)\"\n>+\n> - Use octal escape sequences (e.g. \"\\302\\242\"), not hexadecimal (e.g.\n> \"\\xc2\\xa2\") in printf format strings, since hexadecimal escape\n> sequences are not portable.\n\nI can confirm, at least for the set of platforms I work on, that printf with\nhex values is definitely not portable.\n--Randall\n\n"},{"id":"492366","messageId":"xmqqplv3p6ji.fsf@gitster.g","threadId":"61277","inReplyTo":"02c801da87c1$eac025f0$c04071d0$@nexbridge.com","subject":"Re: [PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T02:29:53Z","receivedAt":"2024-04-06T02:30:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n>> - Use octal escape sequences (e.g. \"\\302\\242\"), not hexadecimal (e.g.\n>> \"\\xc2\\xa2\") in printf format strings, since hexadecimal escape\n>> sequences are not portable.\n>\n> I can confirm, at least for the set of platforms I work on, that printf with\n> hex values is definitely not portable.\n\nSure, that is why we have that rule that we see in the context.\n"},{"id":"492369","messageId":"CAPig+cRjqe-rgYf5UZr9KXmfSw98ZoYjPo5PKhwzRaC-svwshA@mail.gmail.com","threadId":"61277","inReplyTo":"20240406000902.3082301-2-gitster@pobox.com","subject":"Re: [PATCH 1/6] CodingGuidelines: describe \"export VAR=VAL\" rule","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-06T05:11:29Z","receivedAt":"2024-04-06T05:11:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano wrote:\n> https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/\n> resulted in 9968ffff (test-lint: detect 'export FOO=bar',\n> 2013-07-08) to add a rule to t/check-non-portable-shell.pl script to\n> reject\n>\n> export VAR=VAL\n>\n> and suggest us to instead write it as \"export VAR\" followed by\n> \"VAR=VAL\". This however was not spelled out in the CodingGuidelines\n> document.\n\nI suspect you meant:\n\n ... and suggest us to instead write it as \"VAR=VAL\" followed by\n \"export VAR\".\n\n> We may want to re-evaluate the rule since it is from ages ago, but\n> for now, let's make the written rule and what the automation enforces\n> consistent.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> @@ -188,6 +188,12 @@ For shell scripts specifically (not exhaustive):\n> hopefully nobody starts using \"local\" before they are reimplemented\n> in C ;-)\n>\n> + - Some versions of shell do not understand \"export variable=value\",\n> + so we write \"export variable\" and \"variable=value\" on separae\n\ns/separae/separate/\n\nHere too, it might be clearer to swap around the pieces:\n\n ... so we write \"variable=value\" and \"export variable\" on...\n\n> + lines. Note that this was reported in 2013 and the situation might\n> + have changed since then. We'd need to re-evaluate this rule,\n> + together with the rule in t/check-non-portable-shell.pl script.\n\nThe bit starting at \"Note that...\" seems more appropriate for the\ncommit message (which is already the case) or a To-Do list. People\nreading this document are likely newcomers looking for concrete\ninstructions about how to code for this project, and this sort of\nTo-Do item isn't going to help them. (If anything, it might confuse\nthem into ignoring the advice to split `export foo=bar` into two\nstatements, which will result in reviewers asking them to reroll.)\n"},{"id":"492370","messageId":"CAPig+cR8HxO5ZeZrJQ4PtpgsqM__cvieZ6g37F1m_=ng6xvSPA@mail.gmail.com","threadId":"61277","inReplyTo":"20240406000902.3082301-3-gitster@pobox.com","subject":"Re: [PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-06T05:16:17Z","receivedAt":"2024-04-06T05:16:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano wrote:\n> Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097\n> lets the shell erroneously perform field splitting on the expansion\n> of a command substitution during declaration of a local or an extern\n> variable.\n>\n> The explanation was stolen from ebee5580 (parallel-checkout: avoid\n> dash local bug in tests, 2021-06-06).\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> @@ -194,6 +194,20 @@ For shell scripts specifically (not exhaustive):\n> + - Some versions of dash have broken variable assignment when prefixed\n> + with \"local\", \"export\", and \"readonly\", in that the value to be\n> + assigned goes through field splitting at $IFS unless quoted.\n> +\n> + DO NOT write:\n> +\n> + local variable=$value ;# wrong\n> + local variable=$(command args) ;# wrong\n> +\n> + and instead write:\n> +\n> + local variable=\"$value\"\n> + local variable=\"$(command args)\"\n> +\n\nEvery other example in the shell-script section of this document is\nwritten like this:\n\n (incorrect)\n local variable=$value\n local variable=$(command args)\n\n (correct)\n local variable=\"$value\"\n local variable=\"$(command args)\"\n\nShould this patch follow suit for consistency?\n"},{"id":"492372","messageId":"xmqqsezzm4kg.fsf@gitster.g","threadId":"61277","inReplyTo":"CAPig+cR8HxO5ZeZrJQ4PtpgsqM__cvieZ6g37F1m_=ng6xvSPA@mail.gmail.com","subject":"Re: [PATCH 2/6] CodingGuidelines: quote assigned value in 'local var=$val'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T05:40:47Z","receivedAt":"2024-04-06T05:40:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Every other example in the shell-script section of this document is\n> written like this:\n>\n> (incorrect)\n> local variable=$value\n> local variable=$(command args)\n>\n> (correct)\n> local variable=\"$value\"\n> local variable=\"$(command args)\"\n>\n> Should this patch follow suit for consistency?\n\nSure. That sounds like a good idea.\n"},{"id":"492373","messageId":"xmqqo7anm4a0.fsf@gitster.g","threadId":"61277","inReplyTo":"CAPig+cRjqe-rgYf5UZr9KXmfSw98ZoYjPo5PKhwzRaC-svwshA@mail.gmail.com","subject":"Re: [PATCH 1/6] CodingGuidelines: describe \"export VAR=VAL\" rule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T05:47:03Z","receivedAt":"2024-04-06T05:47:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + lines. Note that this was reported in 2013 and the situation might\n>> + have changed since then. We'd need to re-evaluate this rule,\n>> + together with the rule in t/check-non-portable-shell.pl script.\n>\n> The bit starting at \"Note that...\" seems more appropriate for the\n> commit message (which is already the case) or a To-Do list. People\n> reading this document are likely newcomers looking for concrete\n> instructions about how to code for this project,...\n\nVery true. I thought I'd move some to the log message, but it turns\nout that enough is already described there.\n\nThanks.\n"},{"id":"492377","messageId":"87bk6mc0nj.fsf@linux-m68k.org","threadId":"61277","inReplyTo":"CAPig+cRjqe-rgYf5UZr9KXmfSw98ZoYjPo5PKhwzRaC-svwshA@mail.gmail.com","subject":"Re: [PATCH 1/6] CodingGuidelines: describe \"export VAR=VAL\" rule","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2024-04-06T09:15:28Z","receivedAt":"2024-04-06T09:23:04Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Apr 06 2024, Eric Sunshine wrote:\n\n> On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano wrote:\n>> https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/\n>> resulted in 9968ffff (test-lint: detect 'export FOO=bar',\n>> 2013-07-08) to add a rule to t/check-non-portable-shell.pl script to\n>> reject\n>>\n>> export VAR=VAL\n>>\n>> and suggest us to instead write it as \"export VAR\" followed by\n>> \"VAR=VAL\". This however was not spelled out in the CodingGuidelines\n>> document.\n>\n> I suspect you meant:\n>\n> ... and suggest us to instead write it as \"VAR=VAL\" followed by\n> \"export VAR\".\n\nThere is no difference between them. The export command only marks the\nvariable for export, independent of the current or future value of the\nvariable. The exported value is always the last assigned one.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"492396","messageId":"xmqqa5m6l8y9.fsf@gitster.g","threadId":"61277","inReplyTo":"87bk6mc0nj.fsf@linux-m68k.org","subject":"Re: [PATCH 1/6] CodingGuidelines: describe \"export VAR=VAL\" rule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T17:03:42Z","receivedAt":"2024-04-06T17:03:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab writes:\n\n>> I suspect you meant:\n>>\n>> ... and suggest us to instead write it as \"VAR=VAL\" followed by\n>> \"export VAR\".\n>\n> There is no difference between them. The export command only marks the\n> variable for export, independent of the current or future value of the\n> variable. The exported value is always the last assigned one.\n\nCorrect.\n\nBut we are talking about working around sub-standard (read: buggy)\nimplementations and it is of dubious value to assume a compliant\nimplementation when devising a workaround.\n\nIt is easily imaginable that a sub-standard implementation uses a\nsymbol table with a single \"is it exported?\" bit in addition to\n(name, value), without a way to say \"this parameter is not set\n(yet)\" (IOW, never value==NULL), and such an implementation would\nnot be capable to have \"this name is exported but nobody set the\nvalue to it yet\". Using an assignment to make sure it is known\nbefore setting the exported bit is safer to protect against such an\nimplementation.\n"},{"id":"492400","messageId":"CAPig+cQurykHFWvPY7jRKSPARMDyUhJJHH8fL6zffE6ke8b1mA@mail.gmail.com","threadId":"61277","inReplyTo":"87bk6mc0nj.fsf@linux-m68k.org","subject":"Re: [PATCH 1/6] CodingGuidelines: describe \"export VAR=VAL\" rule","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-06T17:34:55Z","receivedAt":"2024-04-06T17:35:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 6, 2024 at 5:15 AM Andreas Schwab wrote:\n> On Apr 06 2024, Eric Sunshine wrote:\n> > On Fri, Apr 5, 2024 at 8:09 PM Junio C Hamano wrote:\n> >> https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/\n> >> resulted in 9968ffff (test-lint: detect 'export FOO=bar',\n> >> 2013-07-08) to add a rule to t/check-non-portable-shell.pl script to\n> >> reject\n> >>\n> >> export VAR=VAL\n> >>\n> >> and suggest us to instead write it as \"export VAR\" followed by\n> >> \"VAR=VAL\". This however was not spelled out in the CodingGuidelines\n> >> document.\n> >\n> > I suspect you meant:\n> >\n> > ... and suggest us to instead write it as \"VAR=VAL\" followed by\n> > \"export VAR\".\n>\n> There is no difference between them. The export command only marks the\n> variable for export, independent of the current or future value of the\n> variable. The exported value is always the last assigned one.\n\nYes, I know, but it is customary in this code-base to write it as:\n\n VAR=VAL &&\n export VAR\n\nnot the other way around, so it makes sense for CodingGuidelines to\nillustrate it in a fashion consistent with its use in the project.\n"},{"id":"492428","messageId":"20240407014344.GF1085004@coredump.intra.peff.net","threadId":"61277","inReplyTo":"20240406000902.3082301-7-gitster@pobox.com","subject":"Re: [PATCH 6/6] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:43:44Z","receivedAt":"2024-04-07T01:43:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2024 at 05:09:02PM -0700, Junio C Hamano wrote:\n\n> Teach t/check-non-portable-shell.pl that right hand side of the\n> assignment done with \"local VAR=VAL\" need to be quoted. We\n> deliberately target only VAL that begins with $ so that we can catch\n> \n> - $variable_reference and positional parameter reference like $4\n> - $(command substitution)\n> - ${variable_reference-with_magic}\n> \n> while excluding\n> \n> - $'\\n' that is a bash-ism freely usable in t990[23]\n> - $(( arithmetic )) whose result should be $IFS safe.\n> - $? that also is $IFS safe\n\nHmm. Just porting over my comment from the other thread (before I\nrealized you'd written this series), this misses:\n\n local foo=bar/$1\n\netc. Should we look for the \"$\" anywhere on the line? I doubt we can get\nthings foolproof, but requiring somebody to quote:\n\n local foo=$((1+2))\n\ndoes not seem like the worst outcome. I dunno.\n\n-Peff\n"},{"id":"492542","messageId":"ZhQNq4ITp68ikVVy@tanuki","threadId":"61277","inReplyTo":"20240406000902.3082301-4-gitster@pobox.com","subject":"Re: [PATCH 3/6] t: local VAR=\"VAL\" (quote positional parameters)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T15:30:51Z","receivedAt":"2024-04-08T15:30:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Apr 05, 2024 at 05:08:59PM -0700, Junio C Hamano wrote:\n> Future-proof test scripts that do\n> \n> \tlocal VAR=VAL\n> \n> without quoting VAL (which is OK in POSIX but broken in some shells)\n> that is a positional parameter, e.g. $4.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> t/lib-parallel-checkout.sh | 2 +-\n> t/t2400-worktree-add.sh | 2 +-\n> t/t4210-log-i18n.sh | 4 ++--\n> t/test-lib-functions.sh | 2 +-\n> 4 files changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n> index acaee9cbb6..8324d6c96d 100644\n> --- a/t/lib-parallel-checkout.sh\n> +++ b/t/lib-parallel-checkout.sh\n> @@ -20,7 +20,7 @@ test_checkout_workers () {\n> \t\tBUG \"too few arguments to test_checkout_workers\"\n> \tfi &&\n> \n> -\tlocal expected_workers=$1 &&\n> +\tlocal expected_workers=\"$1\" &&\n> \tshift &&\n\nI was wondering a bit why this is a problem in t0610, but not over here.\nAs far as I understand it these statements are fine in practice because\nthe expanded values cannot be split, right? So if \"$1\" expanded to\nsomething with spaces in between things would start to break.\n\nIn any case, changing all of these to be quoted feels like the right\nthing to do regardless of whether or not it happens to work with the\ncurrent values of \"$1\". Otherwise it's simply a confusing failure\nwaiting to happen.\n\nPatrick\n\n> \tlocal trace_file=trace-test-checkout-workers &&\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> index 051363acbb..5851e07290 100755\n> --- a/t/t2400-worktree-add.sh\n> +++ b/t/t2400-worktree-add.sh\n> @@ -404,7 +404,7 @@ test_expect_success '\"add\" worktree with orphan branch, lock, and reason' '\n> # Note: Quoted arguments containing spaces are not supported.\n> test_wt_add_orphan_hint () {\n> \tlocal context=\"$1\" &&\n> -\tlocal use_branch=$2 &&\n> +\tlocal use_branch=\"$2\" &&\n> \tshift 2 &&\n> \tlocal opts=\"$*\" &&\n> \ttest_expect_success \"'worktree add' show orphan hint in bad/orphan HEAD w/ $context\" '\n> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh\n> index d2dfcf164e..75216f19ce 100755\n> --- a/t/t4210-log-i18n.sh\n> +++ b/t/t4210-log-i18n.sh\n> @@ -64,7 +64,7 @@ test_expect_success 'log --grep does not find non-reencoded values (latin1)' '\n> '\n> \n> triggers_undefined_behaviour () {\n> -\tlocal engine=$1\n> +\tlocal engine=\"$1\"\n> \n> \tcase $engine in\n> \tfixed)\n> @@ -85,7 +85,7 @@ triggers_undefined_behaviour () {\n> }\n> \n> mismatched_git_log () {\n> -\tlocal pattern=$1\n> +\tlocal pattern=\"$1\"\n> \n> \tLC_ALL=$is_IS_locale git log --encoding=ISO-8859-1 --format=%s \\\n> \t\t--grep=$pattern\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 2f8868caa1..3204afbafb 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1689,7 +1689,7 @@ test_parse_ls_tree_oids () {\n> # Choose a port number based on the test script's number and store it in\n> # the given variable name, unless that variable already contains a number.\n> test_set_port () {\n> -\tlocal var=$1 port\n> +\tlocal var=\"$1\" port\n> \n> \tif test $# -ne 1 || test -z \"$var\"\n> \tthen\n> -- \n> 2.44.0-501-g19981daefd\n> \n> \n"},{"id":"492565","messageId":"xmqqil0rdazm.fsf@gitster.g","threadId":"61277","inReplyTo":"ZhQNq4ITp68ikVVy@tanuki","subject":"Re: [PATCH 3/6] t: local VAR=\"VAL\" (quote positional parameters)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T17:23:41Z","receivedAt":"2024-04-08T17:23:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Apr 05, 2024 at 05:08:59PM -0700, Junio C Hamano wrote:\n>> Future-proof test scripts that do\n>> \n>> \tlocal VAR=VAL\n>> \n>> without quoting VAL (which is OK in POSIX but broken in some shells)\n>> that is a positional parameter, e.g. $4.\n>> \n>> Signed-off-by: Junio C Hamano \n>> ---\n>> t/lib-parallel-checkout.sh | 2 +-\n>> t/t2400-worktree-add.sh | 2 +-\n>> t/t4210-log-i18n.sh | 4 ++--\n>> t/test-lib-functions.sh | 2 +-\n>> 4 files changed, 5 insertions(+), 5 deletions(-)\n>> \n>> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n>> index acaee9cbb6..8324d6c96d 100644\n>> --- a/t/lib-parallel-checkout.sh\n>> +++ b/t/lib-parallel-checkout.sh\n>> @@ -20,7 +20,7 @@ test_checkout_workers () {\n>> \t\tBUG \"too few arguments to test_checkout_workers\"\n>> \tfi &&\n>> \n>> -\tlocal expected_workers=$1 &&\n>> +\tlocal expected_workers=\"$1\" &&\n>> \tshift &&\n>\n> I was wondering a bit why this is a problem in t0610, but not over here.\n> As far as I understand it these statements are fine in practice because\n> the expanded values cannot be split, right? So if \"$1\" expanded to\n> something with spaces in between things would start to break.\n\nCorrect.\n\n> In any case, changing all of these to be quoted feels like the right\n> thing to do regardless of whether or not it happens to work with the\n> current values of \"$1\". Otherwise it's simply a confusing failure\n> waiting to happen.\n\nAgain, agreed. That is where my \"Future-proof\" comes from.\n\nThe true objective of this change is so that the last patch does not\nhave to learn too much exceptions ;-) As long as expected_workers\nis expected to be a number (an unbroken sequence of digits), even if\nwe add more callers to this helper in the future, $1 we see here is\nexpected to be $IFS safe. So in that sense my \"future-proof\" is a\nwhite lie.\n"},{"id":"492567","messageId":"xmqqa5m3damh.fsf@gitster.g","threadId":"61277","inReplyTo":"20240407014344.GF1085004@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T17:31:34Z","receivedAt":"2024-04-08T17:31:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Apr 05, 2024 at 05:09:02PM -0700, Junio C Hamano wrote:\n>\n>> Teach t/check-non-portable-shell.pl that right hand side of the\n>> assignment done with \"local VAR=VAL\" need to be quoted. We\n>> deliberately target only VAL that begins with $ so that we can catch\n>> \n>> - $variable_reference and positional parameter reference like $4\n>> - $(command substitution)\n>> - ${variable_reference-with_magic}\n>> \n>> while excluding\n>> \n>> - $'\\n' that is a bash-ism freely usable in t990[23]\n>> - $(( arithmetic )) whose result should be $IFS safe.\n>> - $? that also is $IFS safe\n>\n> Hmm. Just porting over my comment from the other thread (before I\n> realized you'd written this series), this misses:\n>\n> local foo=bar/$1\n>\n> etc. Should we look for the \"$\" anywhere on the line? I doubt we can get\n> things foolproof, but requiring somebody to quote:\n>\n> local foo=$((1+2))\n>\n> does not seem like the worst outcome. I dunno.\n\nLooking at the output from\n\n $ git grep -E -e 'local [a-zA-Z0-9_]+=[^\"]*[$]' t/\n\nthe listed ones in the proposed commit log message are the false\npositives. Luckily we didn't have anything that tries to\nconcatenate parameter reference to something else.\n\nBut with the pattern we do miss\n\n local var=$*\n\nand possibly many others. So I am not sure. The false positives\ndo look moderately bad, so I'd rather start with the simplest one\nproposed in the patch.\n\n"},{"id":"492580","messageId":"20240408204036.GA1639295@coredump.intra.peff.net","threadId":"61277","inReplyTo":"xmqqa5m3damh.fsf@gitster.g","subject":"Re: [PATCH 6/6] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-08T20:40:36Z","receivedAt":"2024-04-08T20:40:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 08, 2024 at 10:31:34AM -0700, Junio C Hamano wrote:\n\n> > Hmm. Just porting over my comment from the other thread (before I\n> > realized you'd written this series), this misses:\n> >\n> > local foo=bar/$1\n> >\n> > etc. Should we look for the \"$\" anywhere on the line? I doubt we can get\n> > things foolproof, but requiring somebody to quote:\n> >\n> > local foo=$((1+2))\n> >\n> > does not seem like the worst outcome. I dunno.\n> \n> Looking at the output from\n> \n> $ git grep -E -e 'local [a-zA-Z0-9_]+=[^\"]*[$]' t/\n> \n> the listed ones in the proposed commit log message are the false\n> positives. Luckily we didn't have anything that tries to\n> concatenate parameter reference to something else.\n> \n> But with the pattern we do miss\n> \n> local var=$*\n> \n> and possibly many others. So I am not sure. The false positives\n> do look moderately bad, so I'd rather start with the simplest one\n> proposed in the patch.\n\nYeah, I think a regex is probably going to end up with either false\npositives or false negatives. It probably does not matter too much which\nway we err, if we expect them to be rare on either side.\n\nMy thinking was mostly that false negatives are worse, because they only\nmatter on old buggy versions of dash (and only if the tests actually\npass a value with spaces). And so most developers will not notice them\nimmediately. Whereas false positives, while annoying, are reported to\nthem immediately by the linter. And generally, dealing with problems\ncloser to the time of writing means less work overall.\n\nBut I am happy to take your series as-is and we can see which cases (if\nany!) we miss in practice.\n\nI do hope that eventually we could just say \"that buggy version of dash\ndoes not matter anymore\", but I think it is too soon for that (it sounds\nlike it is still being used in CI).\n\n-Peff\n"}]}