Re: [PATCH v2] tests: set temp variables using 'env' in test function instead of subshell

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 18, 2014, 20:52 UTC
Message-ID: <CAPig+cS5UDxvCGRm4d840tOfG6pwjHbARuyAWOR+D_Aht79Gzw@mail.gmail.com>
In-Reply-To: <1395144518-2489-1-git-send-email-unsignedzero@gmail.com>

On Tue, Mar 18, 2014 at 8:08 AM, David Tran <unsignedzero@gmail.com> wrote:

Show 13 quoted lines

> Originally, the code used subshells instead of FOO=BAR command because
> the variable would otherwise leak into the surrounding context of the POSIX
> shell when 'command' is a shell function. The subshell was used to hold the
> context for the test. Using 'env' in the test function sets the temp variables
> without leaking, removing the need of a subshell.
>
> Signed-off-by: David Tran <unsignedzero@gmail.com>
> ---
>> Oh, really ;-)?
> Missed that.
>
>> Thanks.  Getting closer, I think.
> Slowly but surely.

Getting better. See below.

Show 15 quoted lines

> Signed-off-by: David Tran <unsignedzero@gmail.com>
> ---
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 50e22b1..4c7364a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -104,9 +104,7 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>         git checkout master &&
>         (
>         set_fake_editor &&
> -       FAKE_LINES="exec_echo_foo_>file1 1" &&
> -       export FAKE_LINES &&
> -       test_must_fail git rebase -i HEAD^
> +       test_must_fail env FAKE_LINES="exec_echo_foo_>file1 1" git rebase -i HEAD^
>         ) &&

In a previous review, I asked if this subshell could be dropped or if it was required for set_fake_editor. I didn't quite understand your response, so I tested it myself, and found that the subshell can be eliminated safely without breaking this or later tests.

Show 12 quoted lines

>         test_cmp_rev master^ HEAD &&
>         git reset --hard &&
> @@ -118,9 +116,8 @@ test_expect_success 'rebase -i with exec of inexistent command' '
>         test_when_finished "git rebase --abort" &&
>         (
>         set_fake_editor &&
> -       FAKE_LINES="exec_this-command-does-not-exist 1" &&
> -       export FAKE_LINES &&
> -       test_must_fail git rebase -i HEAD^ >actual 2>&1
> +       test_must_fail env FAKE_LINES="exec_this-command-does-not-exist 1" \
> +       git rebase -i HEAD^ >actual 2>&1
>         ) &&

Ditto for this subshell.

Show 12 quoted lines

>         ! grep "Maybe git-rebase is broken" actual
>  '
> @@ -528,11 +509,7 @@ test_expect_success 'aborted --continue does not squash commits after "edit"' '
>         FAKE_LINES="edit 1" git rebase -i HEAD^ &&
>         echo "edited again" > file7 &&
>         git add file7 &&
> -       (
> -               FAKE_COMMIT_MESSAGE=" " &&
> -               export FAKE_COMMIT_MESSAGE &&
> -               test_must_fail git rebase --continue
> -       ) &&
> +       test_must_fail env FAKE_COMMIT_MESSAGE=" " git rebase --continue

Broken &&-chain.

>         test $old = $(git rev-parse HEAD) &&
>         git rebase --abort
>  '