From 7e3a9c23d670347454c6b95e3e6448c9d77fbdc4 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 5 Apr 2024 17:08:57 -0700 Subject: [PATCH 1/8] CodingGuidelines: describe "export VAR=VAL" rule https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/ resulted in 9968ffff (test-lint: detect 'export FOO=bar', 2013-07-08) to add a rule to t/check-non-portable-shell.pl script to reject export VAR=VAL and suggest us to instead write it as two statements, i.e., VAR=VAL export VAR This however was not spelled out in the CodingGuidelines document. We may want to re-evaluate the rule since it is from ages ago, but for now, let's make the written rule and what the automation enforces consistent. Signed-off-by: Junio C Hamano --- Documentation/CodingGuidelines | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 32e69f798e..96eaeee205 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -188,6 +188,10 @@ For shell scripts specifically (not exhaustive): hopefully nobody starts using "local" before they are reimplemented in C ;-) + - Some versions of shell do not understand "export variable=value", + so we write "variable=value" and then "export variable" on two + separate lines. + - 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. From be34b51049d1628d1ba4f17e3c087fd01f15f988 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 5 Apr 2024 17:08:58 -0700 Subject: [PATCH 2/8] CodingGuidelines: quote assigned value in 'local var=$val' 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). Signed-off-by: Junio C Hamano --- Documentation/CodingGuidelines | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 96eaeee205..30bf290418 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -192,6 +192,18 @@ For shell scripts specifically (not exhaustive): so we write "variable=value" and then "export variable" on two separate lines. + - Some versions of dash have 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. + + (incorrect) + local variable=$value + local variable=$(command args) + + (correct) + local variable="$value" + local 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. From 341aad8d41ca8321d826e1ce012e4faf1a8be2a4 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 5 Apr 2024 17:08:59 -0700 Subject: [PATCH 3/8] t: local VAR="VAL" (quote positional parameters) Future-proof test scripts that do local VAR=VAL without quoting VAL (which is OK in POSIX but broken in some shells) that is a positional parameter, e.g. $4. Signed-off-by: Junio C Hamano --- t/lib-parallel-checkout.sh | 2 +- t/t2400-worktree-add.sh | 2 +- t/t4210-log-i18n.sh | 4 ++-- t/test-lib-functions.sh | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh index acaee9cbb6..8324d6c96d 100644 --- a/t/lib-parallel-checkout.sh +++ b/t/lib-parallel-checkout.sh @@ -20,7 +20,7 @@ test_checkout_workers () { BUG "too few arguments to test_checkout_workers" fi && - local expected_workers=$1 && + local expected_workers="$1" && shift && local trace_file=trace-test-checkout-workers && diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index c28c04133c..ba320dc417 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -427,7 +427,7 @@ test_expect_success '"add" worktree with orphan branch, lock, and reason' ' # Note: Quoted arguments containing spaces are not supported. test_wt_add_orphan_hint () { local context="$1" && - local use_branch=$2 && + local use_branch="$2" && shift 2 && local opts="$*" && test_expect_success "'worktree add' show orphan hint in bad/orphan HEAD w/ $context" ' diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh index d2dfcf164e..75216f19ce 100755 --- a/t/t4210-log-i18n.sh +++ b/t/t4210-log-i18n.sh @@ -64,7 +64,7 @@ test_expect_success 'log --grep does not find non-reencoded values (latin1)' ' ' triggers_undefined_behaviour () { - local engine=$1 + local engine="$1" case $engine in fixed) @@ -85,7 +85,7 @@ triggers_undefined_behaviour () { } mismatched_git_log () { - local pattern=$1 + local pattern="$1" LC_ALL=$is_IS_locale git log --encoding=ISO-8859-1 --format=%s \ --grep=$pattern diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 2eccf100c0..fc60708471 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1765,7 +1765,7 @@ test_parse_ls_tree_oids () { # Choose a port number based on the test script's number and store it in # the given variable name, unless that variable already contains a number. test_set_port () { - local var=$1 port + local var="$1" port if test $# -ne 1 || test -z "$var" then From 7f9f230b7fcc1bfeb352216930f704075bca713d Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 5 Apr 2024 17:09:00 -0700 Subject: [PATCH 4/8] t: local VAR="VAL" (quote command substitution) Future-proof test scripts that do local VAR=VAL without quoting VAL (which is OK in POSIX but broken in some shells) that is a $(command substitution). Signed-off-by: Junio C Hamano --- t/t4011-diff-symlink.sh | 4 ++-- t/test-lib-functions.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh index d7a5f7ae78..bc8ba88719 100755 --- a/t/t4011-diff-symlink.sh +++ b/t/t4011-diff-symlink.sh @@ -13,13 +13,13 @@ TEST_PASSES_SANITIZE_LEAK=true # Print the short OID of a symlink with the given name. symlink_oid () { - local oid=$(printf "%s" "$1" | git hash-object --stdin) && + local oid="$(printf "%s" "$1" | git hash-object --stdin)" && git rev-parse --short "$oid" } # Print the short OID of the given file. short_oid () { - local oid=$(git hash-object "$1") && + local oid="$(git hash-object "$1")" && git rev-parse --short "$oid" } diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index fc60708471..4cc7d74f11 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1840,7 +1840,7 @@ test_subcommand () { shift fi - local expr=$(printf '"%s",' "$@") + local expr="$(printf '"%s",' "$@")" expr="${expr%,}" if test -n "$negate" From e97f4a6d94103c43a08c864c6ab63e6086812998 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 5 Apr 2024 17:09:01 -0700 Subject: [PATCH 5/8] t: local VAR="VAL" (quote ${magic-reference}) Future-proof test scripts that do local VAR=VAL without quoting VAL (which is OK in POSIX but broken in some shells) that is ${magic-"reference to a parameter"}. Signed-off-by: Junio C Hamano --- t/test-lib-functions.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 4cc7d74f11..862d80c974 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -385,7 +385,7 @@ test_commit () { shift done && indir=${indir:+"$indir"/} && - local file=${2:-"$1.t"} && + local file="${2:-"$1.t"}" && if test -n "$append" then $echo "${3-$1}" >>"$indir$file" @@ -1748,7 +1748,7 @@ test_oid () { # Insert a slash into an object ID so it can be used to reference a location # under ".git/objects". For example, "deadbeef..." becomes "de/adbeef..". test_oid_to_path () { - local basename=${1#??} + local basename="${1#??}" echo "${1%$basename}/$basename" } @@ -1930,7 +1930,7 @@ test_readlink () { # An optional increment to the magic timestamp may be specified as second # argument. test_set_magic_mtime () { - local inc=${2:-0} && + local inc="${2:-0}" && local mtime=$((1234567890 + $inc)) && test-tool chmtime =$mtime "$1" && test_is_magic_mtime "$1" $inc @@ -1943,7 +1943,7 @@ test_set_magic_mtime () { # argument. Usually, this should be the same increment which was used for # the associated test_set_magic_mtime. test_is_magic_mtime () { - local inc=${2:-0} && + local inc="${2:-0}" && local mtime=$((1234567890 + $inc)) && echo $mtime >.git/test-mtime-expect && test-tool chmtime --get "$1" >.git/test-mtime-actual && From 8bfe4861913843b6aac8656aabfd43ac405362e8 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 5 Apr 2024 17:09:02 -0700 Subject: [PATCH 6/8] t: teach lint that RHS of 'local VAR=VAL' needs to be quoted Teach t/check-non-portable-shell.pl that right hand side of the assignment done with "local VAR=VAL" need to be quoted. We deliberately target only VAL that begins with $ so that we can catch - $variable_reference and positional parameter reference like $4 - $(command substitution) - ${variable_reference-with_magic} while excluding - $'\n' that is a bash-ism freely usable in t990[23] - $(( arithmetic )) whose result should be $IFS safe. - $? that also is $IFS safe Signed-off-by: Junio C Hamano --- t/check-non-portable-shell.pl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index dd8107cd7d..b2b28c2ced 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -47,6 +47,8 @@ while (<>) { /\bgrep\b.*--file\b/ and err 'grep --file FILE is not portable (use grep -f FILE)'; /\b[ef]grep\b/ and err 'egrep/fgrep obsolescent (use grep -E/-F)'; /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)'; + /\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and + err q(quote "$val" in 'local var=$val'); /^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and err '"FOO=bar shell_func" assignment extends beyond "shell_func"'; $line = ''; From 26ba7477d9815f82ad37c5a5499af2e236cbef25 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 5 Apr 2024 17:23:10 -0700 Subject: [PATCH 7/8] t0610: local VAR="VAL" fix The series was based on maint and fixes all the tests that exist there, but we have acquired a few more. Signed-off-by: Junio C Hamano --- t/t0610-reftable-basics.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 686781192e..c8074ebab2 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -83,7 +83,7 @@ test_expect_success 'init: reinitializing reftable with files backend fails' ' test_expect_perms () { local perms="$1" local file="$2" - local actual=$(ls -l "$file") && + local actual="$(ls -l "$file")" && case "$actual" in $perms*) From 836b22139115f92812479ff6a92ffad09f8a6b8c Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 5 Apr 2024 17:28:16 -0700 Subject: [PATCH 8/8] t1016: local VAR="VAL" fix The series was based on maint and fixes all the tests that exist there, but we have acquired a few more. I suspect that the values assigned in many of these places are $IFS safe, and this is primarily to squelch the linter than adding a necessary workaround for buggy dash. Signed-off-by: Junio C Hamano --- t/t1016-compatObjectFormat.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/t/t1016-compatObjectFormat.sh b/t/t1016-compatObjectFormat.sh index 8132cd37b8..be3206a16f 100755 --- a/t/t1016-compatObjectFormat.sh +++ b/t/t1016-compatObjectFormat.sh @@ -79,7 +79,7 @@ commit2_oid () { } del_sigcommit () { - local delete=$1 + local delete="$1" if test "$delete" = "sha256" ; then local pattern="gpgsig-sha256" @@ -91,8 +91,8 @@ del_sigcommit () { del_sigtag () { - local storage=$1 - local delete=$2 + local storage="$1" + local delete="$2" if test "$storage" = "$delete" ; then local pattern="trailer" @@ -181,7 +181,7 @@ done cd "$base" compare_oids () { - test "$#" = 5 && { local PREREQ=$1; shift; } || PREREQ= + test "$#" = 5 && { local PREREQ="$1"; shift; } || PREREQ= local type="$1" local name="$2" local sha1_oid="$3" @@ -193,8 +193,8 @@ compare_oids () { git --git-dir=repo-sha1/.git rev-parse --output-object-format=sha256 ${sha1_oid} > ${name}_sha1_sha256_found git --git-dir=repo-sha256/.git rev-parse --output-object-format=sha1 ${sha256_oid} > ${name}_sha256_sha1_found - local sha1_sha256_oid=$(cat ${name}_sha1_sha256_found) - local sha256_sha1_oid=$(cat ${name}_sha256_sha1_found) + local sha1_sha256_oid="$(cat ${name}_sha1_sha256_found)" + local sha256_sha1_oid="$(cat ${name}_sha256_sha1_found)" test_expect_success $PREREQ "Verify ${type} ${name}'s sha1 oid" ' git --git-dir=repo-sha256/.git rev-parse --output-object-format=sha1 ${sha256_oid} > ${name}_sha1 &&