[1/5] t3701: stop using `env` in force_color()
diff mbox series

Message ID 67d5b93fdaab7f73f352293372ee3d71fb7c1409.1593529394.git.liu.denton@gmail.com
State New
Headers show
Series
  • t: replace incorrect test_must_fail usage (part 6)
Related show

Commit Message

Denton Liu June 30, 2020, 3:03 p.m. UTC
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>
---
 t/t3701-add-interactive.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Eric Sunshine June 30, 2020, 9:48 p.m. UTC | #1
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.
Junio C Hamano June 30, 2020, 10:06 p.m. UTC | #2
Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

Patch
diff mbox series

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 49decbac71..c4c1e9b603 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -31,7 +31,13 @@  diff_cmp () {
 # indicates a dumb terminal, so we set that variable, too.
 
 force_color () {
-	env GIT_PAGER_IN_USE=true TERM=vt100 "$@"
+	(
+		GIT_PAGER_IN_USE=true &&
+		export GIT_PAGER_IN_USE &&
+		TERM=vt100 &&
+		export TERM &&
+		"$@"
+	)
 }
 
 test_expect_success 'setup (initial)' '
@@ -604,7 +610,7 @@  test_expect_success 'detect bogus diffFilter output' '
 	echo content >test &&
 	test_config interactive.diffFilter "sed 1d" &&
 	printf y >y &&
-	test_must_fail force_color git add -p <y
+	force_color test_must_fail git add -p <y
 '
 
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '