Re: [PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 22/07/2024 16:09, Phillip Wood wrote:
Hi Eric

On 22/07/2024 07:59, Eric Sunshine wrote:
From: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>

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).

Having seen the parallel discussion about the behavior of hash this construct is non-portable because the behavior differs between shells so perhaps the commit message could say something like

Unlike "VAR=val cmd" one-shot environment variable assignments which only exist for the invocation of external command "cmd", the behavior of "VAR=val func" where "func" is a shell function or builtin command varies between shells and so we should not use it in our test suite.

Best Wishes

Phillip

I'm not sure I follow. If I run

sh -c 'f() {
     echo "f: HELLO=$HELLO"
     env | grep HELLO
}
HELLO=x f; echo "HELLO=$HELLO"'

Then I see

f: HELLO=x
HELLO=x
HELLO=

which seems to contradict the commit message as $HELLO is unset when the function returns. I see the same result if I replace "sh" (which is bash on my system) with an explicit "bash", "dash" or "zsh".

I'm also confused as to why this caused a problem for Rubén's test as $HELLO is set in the environment so I'm don't understand why git wasn't picking up the right pager.

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.

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.

Oh, that sounds useful, I didn't know it existed.

Best Wishes

Phillip

Signed-off-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
---
  t/t3430-rebase-merges.sh | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 36ca126bcd..e851ede4f9 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -392,8 +392,8 @@ 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@xxxxxxxxxxx" \
-     test_commit second-root) &&
+    test_env GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="root@xxxxxxxxxxx" \
+        test_commit second-root &&
      test_commit third-root &&
      cat >script-from-scratch <<-\EOF &&
      pick third-root





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux