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

[Eric Sunshine <sunshine@sunshineco.com>]

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?

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.