From: Phillip Wood Date: Tue, 23 Jul 2024 09:26:50 GMT Subject: Re: [PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation Message-ID: <1b672247-c05d-44f5-967a-9861d715040b@gmail.com> In-Reply-To: On 22/07/2024 16:09, Phillip Wood wrote: > Hi Eric > > On 22/07/2024 07:59, Eric Sunshine wrote: >> From: Eric Sunshine >> >> 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 >> --- >>   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@example.com" \ >> -     test_commit second-root) && >> +    test_env GIT_AUTHOR_NAME="Parsnip" >> GIT_AUTHOR_EMAIL="root@example.com" \ >> +        test_commit second-root && >>       test_commit third-root && >>       cat >script-from-scratch <<-\EOF && >>       pick third-root