Re: [PATCH 1/4] t3430: modernize one-shot "VAR=val shell-func" invocation
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 23, 2024, 09:26 UTC
- Message-ID
- <1b672247-c05d-44f5-967a-9861d715040b@gmail.com>
- In-Reply-To
- <c586f7dc-636b-45a3-acb2-faedfe1068e6@gmail.com>
On 22/07/2024 16:09, Phillip Wood wrote:
Show 9 quoted lines
> 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
Show 59 quoted lines
> 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