git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Denton Liu <liu.denton@gmail.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 1/5] t3701: stop using `env` in force_color()
Date: Tue, 30 Jun 2020 15:06:18 -0700	[thread overview]
Message-ID: <xmqqlfk4dx9h.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CAPig+cTYJwDL_uGfSR0OmU4fYANbqQ5jLkyN29WXDvq6gWG_Zg@mail.gmail.com> (Eric Sunshine's message of "Tue, 30 Jun 2020 17:48:44 -0400")

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.

  reply	other threads:[~2020-06-30 22:06 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30 15:03 [PATCH 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
2020-06-30 15:03 ` [PATCH 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-06-30 21:48   ` Eric Sunshine
2020-06-30 22:06     ` Junio C Hamano [this message]
2020-06-30 15:03 ` [PATCH 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-06-30 15:03 ` [PATCH 3/5] t7107: don't use test_must_fail() Denton Liu
2020-06-30 15:03 ` [PATCH 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-06-30 15:03 ` [PATCH 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu
2020-06-30 20:56   ` Eric Sunshine
2020-06-30 21:59     ` Eric Sunshine
2020-06-30 23:39     ` Denton Liu
2020-07-01  4:27 ` [PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
2020-07-01  4:27   ` [PATCH v2 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-07-01  4:27   ` [PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-07-01  4:27   ` [PATCH v2 3/5] t7107: don't use test_must_fail() Denton Liu
2020-07-01  4:27   ` [PATCH v2 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-07-01  4:27   ` [PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu
2020-07-07  6:04   ` [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 3/5] t7107: don't use test_must_fail() Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu
2020-07-07 20:08     ` [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Junio C Hamano
2020-07-07 20:57       ` Junio C Hamano
2020-07-07 22:08         ` [PATCH] t9400: don't use test_must_fail with cvs Denton Liu
2020-07-07 22:11         ` [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Junio C Hamano
2020-07-07 22:21           ` Denton Liu
2020-07-07 22:29             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqlfk4dx9h.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).