Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 1/5] t3701: stop using `env` in force_color()
Date: Tue, 30 Jun 2020 17:48:44 -0400
Message-ID: <CAPig+cTYJwDL_uGfSR0OmU4fYANbqQ5jLkyN29WXDvq6gWG_Zg@mail.gmail.com> (raw)
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:
> 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.

  reply index

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 [this message]
2020-06-30 22:06     ` Junio C Hamano
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=CAPig+cTYJwDL_uGfSR0OmU4fYANbqQ5jLkyN29WXDvq6gWG_Zg@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.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

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git