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

[Phillip Wood <phillip.wood123@gmail.com>]

On 22/07/2024 16:09, Phillip Wood wrote:
> Hi Eric
> 
> On 22/07/2024 07:59, Eric Sunshine wrote:
>> From: Eric Sunshine <sunshine@sunshineco.com>
>>
>> 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@sunshineco.com>
>> ---
>>   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