From: Eric Sunshine <sunshine@sunshineco.com>
This is a reroll of [1] which improves check-non-portable-shell's detection of one-shot environment variable assignment with shell functions. Changes since v1:
* commit messages now state the behavior is undefined according to POSIX
rather than focusing only on the original reason given (that the
assignments could outlive the shell function invocation)
* t3430 simplified by dropping the subshell altogether in favor of
`test_commit --author`
* new commit to improve the error message when one-shot assignment with
shell function is detected
[1]: https://lore.kernel.org/git/20240722065915.80760-1-ericsunshine@charter.net/
Eric Sunshine (5):
t3430: drop unnecessary one-shot "VAR=val shell-func" invocation
t4034: fix use of one-shot variable assignment with shell function
check-non-portable-shell: loosen one-shot assignment error message
check-non-portable-shell: suggest alternative for `VAR=val shell-func`
check-non-portable-shell: improve `VAR=val shell-func` detection
t/check-non-portable-shell.pl | 4 ++--
t/t3430-rebase-merges.sh | 3 +--
t/t4034-diff-words.sh | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
Range-diff against v1:
1: 5bb6811f68 ! 1: 0d3c0593c9 t3430: modernize one-shot "VAR=val shell-func" invocation
@@ Metadata
Author: Eric Sunshine <sunshine@sunshineco.com>
## Commit message ##
- t3430: modernize one-shot "VAR=val shell-func" invocation
+ t3430: drop unnecessary one-shot "VAR=val shell-func" invocation
- Unlike "VAR=val cmd" one-shot environment variable assignments which
- exist only for the invocation of 'cmd', those assigned by "VAR=val
- shell-func" exist within the running shell and continue to do so until
- the process exits (or are explicitly unset). check-non-portable-shell.pl
- warns when it detects such usage since, more often than not, the author
- who writes such an invocation is unaware of the undesirable behavior.
+ The behavior of a one-shot environment variable assignment of the form
+ "VAR=val cmd" is undefined 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 ill-defined 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.
- A common way to work around the problem is to wrap a subshell around the
- variable assignments and function call, thus ensuring that the
- assignments are short-lived. However, these days, a more ergonomic
- approach is to employ test_env() which is tailor-made for this specific
- use-case.
+ 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 <sunshine@sunshineco.com>
@@ t/t3430-rebase-merges.sh: test_expect_success 'refuse to merge ancestors of HEAD
git checkout --orphan unrelated &&
- (GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
- test_commit second-root) &&
-+ test_env GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@example.com" \
-+ test_commit second-root &&
++ test_commit --author "Parsnip <root@example.com>" second-root &&
test_commit third-root &&
cat >script-from-scratch <<-\EOF &&
pick third-root
2: 1f35449847 ! 2: 19ee8295ef t4034: fix use of one-shot variable assignment with shell function
@@ Metadata
## Commit message ##
t4034: fix use of one-shot variable assignment with shell function
- Unlike "VAR=val cmd" one-shot environment variable assignments which
- exist only for the invocation of 'cmd', those assigned by "VAR=val
- shell-func" exist within the running shell and continue to do so until
- the process exits (or are explicitly unset). In most cases, it is
- unlikely that this behavior was intended by the test author, and, even
- if those leaked assignments do not impact other tests today, they can
- negatively impact tests added later by authors unaware that the variable
- assignments are still hanging around. Address this shortcoming by
- ensuring that the assignments are short-lived.
+ The behavior of a one-shot environment variable assignment of the form
+ "VAR=val cmd" is undefined 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 <sunshine@sunshineco.com>
-: ---------- > 3: 220ca26d4f check-non-portable-shell: loosen one-shot assignment error message
-: ---------- > 4: 4910756aab check-non-portable-shell: suggest alternative for `VAR=val shell-func`
3: 89621f72a2 ! 5: 7a15553a5a check-non-portable-shell: improve `VAR=val shell-func` detection
@@ Metadata
## Commit message ##
check-non-portable-shell: improve `VAR=val shell-func` detection
- Unlike "VAR=val cmd" one-shot environment variable assignments which
- exist only for the invocation of 'cmd', those assigned by "VAR=val
- shell-func" exist within the running shell and continue to do so until
- the process exits. check-non-portable-shell.pl warns when it detects
- such usage since, more often than not, the author who writes such an
- invocation is unaware of the undesirable behavior.
+ The behavior of a one-shot environment variable assignment of the form
+ "VAR=val cmd" is undefined 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:
+ 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
@@ t/check-non-portable-shell.pl: sub err {
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" assignment extends beyond "shell_func"';
+ err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';
$line = '';
# this resets our $. for each file
4: 7b2e1dd895 < -: ---------- check-non-portable-shell: suggest alternative for `VAR=val shell-func`
