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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 22, 2024, 15:09 UTC
Message-ID: <c586f7dc-636b-45a3-acb2-faedfe1068e6@gmail.com>
In-Reply-To: <20240722065915.80760-2-ericsunshine@charter.net>

Hi Eric

On 22/07/2024 07:59, Eric Sunshine wrote:

Show 6 quoted lines

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

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.

Show 9 quoted lines

> 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

Show 20 quoted lines

> 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