Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 10, 2016, 19:48 UTC
Message-ID: <CAPig+cTtU9_3=2eu0boaPbXKXh2gngEe7byDpJuSFAR4rcbrMA@mail.gmail.com>
In-Reply-To: <CAPig+cRR49iJOkaLfynkvch4zUHVkpcJwVah0rvaEOeR7aY2Nw@mail.gmail.com>

On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 26 quoted lines

> On Tue, May 10, 2016 at 2:39 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:
>>>       while :
>>>       do
>>>               case "$1" in
>>>               -C) dir="-C $2"; shift; shift ;;
>>>               -b) bare="$2"; shift; shift ;;
>>> +             -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
>>> @@ -36,6 +38,8 @@ test_rev_parse () {
>>>       do
>>>               expect="$1"
>>>               test_expect_success "$name: $o" '
>>> +                     test_when_finished "sane_unset GIT_DIR" &&
>>> +                     eval $env &&
>>
>> This will set up the sane_unset regardless of whether $env does
>> anything. Would it make more sense to stick the test_when_finished
>> inside $env? You could use regular unset then, too, since you know the
>> variable would be set.
>
> I didn't worry about it too much because the end result is effectively
> the same and, with all the 'case' arms being short one-liners, I think
> the code is a bit easier to read as-is; bundling 'test_when_finished'
> into the 'env' assignment line would probably require wrapping the
> line. I do like the improved encapsulation of your suggestion but
> don't otherwise feel strongly about it.

Actually, I think we can have improved encapsulation and maintain readability like this:

    case "$1" in
    ...
    -g) env="$2"; shift;  shift ;;
    ...
    esac

    ...
    test_expect_success "..." '
        if test -n "$env"
        do
            test_when_finished "unset GIT_DIR"
            GIT_DIR="$env"
            export GIT_DIR
        fi
        ...
    '