Re: [PATCH 1/5] t3701: stop using `env` in force_color()

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2020, 22:06 UTC
Message-ID: <xmqqlfk4dx9h.fsf@gitster.c.googlers.com>
In-Reply-To: <CAPig+cTYJwDL_uGfSR0OmU4fYANbqQ5jLkyN29WXDvq6gWG_Zg@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 37 quoted lines

> On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote:
>> In a future patch, we plan on making the test_must_fail()-family of
>> functions accept only git commands. Even though force_color() wraps an
>> invocation of `env git`, test_must_fail() will not be able to figure
>> this out since it will assume that force_color() is just some random
>> function which is disallowed.
>>
>> Instead of using `env` in force_color() (which does not support shell
>> functions), export the environment variables in a subshell. Write the
>> invocation as `force_color test_must_fail git ...` since shell functions
>> are now supported.
>>
>> Signed-off-by: Denton Liu <liu.denton@gmail.com>
>> ---
>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
>> @@ -31,7 +31,13 @@ diff_cmp () {
>>  force_color () {
>> -       env GIT_PAGER_IN_USE=true TERM=vt100 "$@"
>> +       (
>> +               GIT_PAGER_IN_USE=true &&
>> +               export GIT_PAGER_IN_USE &&
>> +               TERM=vt100 &&
>> +               export TERM &&
>> +               "$@"
>> +       )
>>  }
>
> I'm having trouble understanding why this function was transformed the
> way it was. I presume the subshell is to ensure that the variable
> assignments don't escape the function context since you're dropping
> 'env', however, it seems the following would be simpler:
>
>     force_color ()  {
>         (GIT_PAGER_IN_USE=true TERM=vt100 "$@")
>     }
>
> Or, is there something non-obvious going on that I'm missing?

Denton wants to be able to write

	force_color test_must_fail git foo blah

The only known kind of POSIX breaking behaviour by some shells makes the assignment persist after the "$@" returns, and the subshell would be a good way to protect the caller of force_color from the effect of the assignment leaking, so if we only care about that known breakage, both Denton's and your simplification should work OK, but it is probably clearer to see that we are *not* relying on any one-shot environment assignment at all in Denton's version.

I'd list two variables on a single export command, though ;-)

Show 10 quoted lines

> By the way, I'm wondering if the subshell deserves an in-code comment.
> Whereas, we have somewhat settled upon the idiom 'test_must_fail env
> FOO=bar ...' when we need to make sure variable assignments don't
> escape the local context -- since 'FOO=bar test_must_fail ...' doesn't
> make that guarantee under all shells -- the use of a subshell here to
> achieve the same (if I'm understanding correctly) is not nearly so
> obvious. The non-obviousness is due to "$@" being abstract -- someone
> reading the code won't necessarily realize that the first element of
> "$@" might be a shell function, thus would not necessarily understand
> the use of a subshell.

Probably.