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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 30, 2020, 21:48 UTC
Message-ID: <CAPig+cTYJwDL_uGfSR0OmU4fYANbqQ5jLkyN29WXDvq6gWG_Zg@mail.gmail.com>
In-Reply-To: <67d5b93fdaab7f73f352293372ee3d71fb7c1409.1593529394.git.liu.denton@gmail.com>

On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote:

Show 25 quoted lines

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

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.