[PATCH v2 0/5] improve one-shot variable detection with shell function

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`