From a26d7004f73abb1005acd98114faec3aa75f5090 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Sat, 27 Jul 2024 01:35:05 -0400 Subject: [PATCH 1/5] t3430: drop unnecessary one-shot "VAR=val shell-func" invocation The behavior of a one-shot environment variable assignment of the form "VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell function. Indeed the behavior differs between shell implementations and even different versions of the same shell. One such problematic behavior is that, with some shells, the assignment will outlive the invocation of the function, thus may potentially impact subsequent commands in the test, as well as subsequent tests. A common way to work around the problem is to wrap a subshell around the one-shot assignment, thus ensuring that the assignment is short-lived. In this test, the subshell is employed precisely for this purpose; other side-effects of the subshell, such as losing the effect of `test_tick` which is invoked by `test_commit`, are immaterial. These days, we can take advantage of `test_commit --author` to more clearly convey that the test is interested only in overriding the author of the commit. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/t3430-rebase-merges.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 36ca126bcd..2aa8593f77 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -392,8 +392,7 @@ test_expect_success 'refuse to merge ancestors of HEAD' ' test_expect_success 'root commits' ' git checkout --orphan unrelated && - (GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \ - test_commit second-root) && + test_commit --author "Parsnip " second-root && test_commit third-root && cat >script-from-scratch <<-\EOF && pick third-root From 5e91056a1b0d38c6b2149c2c3a72198720a1867f Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Sat, 27 Jul 2024 01:35:06 -0400 Subject: [PATCH 2/5] t4034: fix use of one-shot variable assignment with shell function The behavior of a one-shot environment variable assignment of the form "VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell function. Indeed the behavior differs between shell implementations and even different versions of the same shell, thus should be avoided. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/t4034-diff-words.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh index 74586f3813..4dcd7e9925 100755 --- a/t/t4034-diff-words.sh +++ b/t/t4034-diff-words.sh @@ -70,7 +70,7 @@ test_language_driver () { word_diff --color-words ' test_expect_success "diff driver '$lang' in Islandic" ' - LANG=is_IS.UTF-8 LANGUAGE=is LC_ALL="$is_IS_locale" \ + test_env LANG=is_IS.UTF-8 LANGUAGE=is LC_ALL="$is_IS_locale" \ word_diff --color-words ' } From a7fa6097473e42b3547cf59fb52fd5ff34e9bd48 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Sat, 27 Jul 2024 01:35:07 -0400 Subject: [PATCH 3/5] check-non-portable-shell: loosen one-shot assignment error message When a0a630192d (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13) added the check for one-shot environment variable assignment for shell functions, the primary reason given for avoiding them was that, under some shells, the assignment outlives the invocation of the shell function, thus could potentially negatively impact subsequent commands in the same test, as well as subsequent tests. However, it has recently become apparent that this is not the only potential problem with one-shot assignments and shell functions. Another problem is that some shells do not actually export the variable to commands which the function invokes[1]. More significantly, however, the behavior of one-shot assignments with shell functions is not specified by POSIX[2]. Given this new understanding, the presented error message ("assignment extends beyond 'shell_func'") is too specific and potentially misleading. Address this by emitting a less specific error message. (Note that the wording "is not portable" is chosen over the more specific "behavior not specified by POSIX" for consistency with almost all other error message issued by this "lint" script.) [1]: https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/ [2]: https://lore.kernel.org/git/xmqq34o19jj1.fsf@gitster.g/ Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/check-non-portable-shell.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index b2b28c2ced..179efaa39d 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -50,7 +50,7 @@ while (<>) { /\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"'; + err '"FOO=bar shell_func" is not portable'; $line = ''; # this resets our $. for each file close ARGV if eof; From 7bd0cd0e7b29da382fcf2821c1bf087190ab2ffe Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Sat, 27 Jul 2024 01:35:08 -0400 Subject: [PATCH 4/5] check-non-portable-shell: suggest alternative for `VAR=val shell-func` Most problems reported by check-non-portable-shell are accompanied by advice suggesting how the test author can repair the problem. For instance: error: egrep/fgrep obsolescent (use grep -E/-F) However, when one-shot variable assignment is detected when calling a shell function (i.e. `VAR=val shell-func`), the problem is reported, but no advice is given. The lack of advice is particularly egregious since neither the problem nor the workaround are likely well-known by newcomers to the project writing tests for the first time. Address this shortcoming by recommending the use of `test_env` which is tailor made for this specific use-case. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/check-non-portable-shell.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index 179efaa39d..903af14294 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -50,7 +50,7 @@ while (<>) { /\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" is not portable'; + err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)'; $line = ''; # this resets our $. for each file close ARGV if eof; From 7b9e54714a1c0b6f284cc7d8241e9ea25d149476 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Sat, 27 Jul 2024 01:35:09 -0400 Subject: [PATCH 5/5] check-non-portable-shell: improve `VAR=val shell-func` detection The behavior of a one-shot environment variable assignment of the form "VAR=val cmd" is unspecified according to POSIX when "cmd" is a shell function. Indeed the behavior differs between shell implementations and even different versions of the same shell, thus should be avoided. As such, check-non-portable-shell.pl warns when it detects such usage. However, a limitation of the check is that it only detects such invocations when variable assignment (i.e. `VAR=val`) is the first thing on the line. Thus, it can easily be fooled by an invocation such as: echo X | VAR=val shell-func Address this shortcoming by loosening the check so that the variable assignment can be recognized even when not at the beginning of the line. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/check-non-portable-shell.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index 903af14294..6ee7700eb4 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -49,7 +49,7 @@ while (<>) { /\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 + /\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)'; $line = ''; # this resets our $. for each file