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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jul 22, 2024 at 11:10 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 22/07/2024 07:59, Eric Sunshine wrote:
> > 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 believe downstream discussion[1][2] established that the behavior is
inconsistent between various shells and versions of shells, and is
considered undefined by POSIX.

[1]: https://lore.kernel.org/git/xmqq34o1cn6b.fsf@gitster.g/
[2]: https://lore.kernel.org/git/xmqqbk2p9lwi.fsf_-_@gitster.g/

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

Junio summarized the problem and explanation[3].

[3]: https://lore.kernel.org/git/xmqq7cdd9l0m.fsf@gitster.g/

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

I didn't know about it either, and only discovered it upon my initial
attempt at making check-non-portable-shell.pl recognize the case Rubén
identified, at which point it started showing false-positives on
`test_env` invocations. Actually, considering that I was involved[4]
in the conversation which led to the introduction[5] of `test_env` by
Peff, it may be that I did know about it but forgot.

[4]: https://lore.kernel.org/git/CAPig+cR989yU4+JNTFREaeXqY61nusUOhufeBGGVCi29tR1P5w@mail.gmail.com/
[5]: https://lore.kernel.org/git/20160601070425.GA13648@sigill.intra.peff.net/