From: SZEDER Gábor <szeder@ira.uka.de>
Date: Tue, 10 May 2016 21:01:11 GMT
Subject: Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
Message-ID: <20160510230111.Horde.JpLRouwWbSCYp7yuvqhy5SS@webmail.informatik.kit.edu>
In-Reply-To: <CAPig+cTtU9_3=2eu0boaPbXKXh2gngEe7byDpJuSFAR4rcbrMA@mail.gmail.com>


Quoting Eric Sunshine <sunshine@sunshineco.com>:

> On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine  
> <sunshine@sunshineco.com> wrote:
>> 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
>         ...
>     '

I wonder if is it really necessary to specify the path to the .git
directory via $GIT_DIR.  Would 'git --git-dir=/over/there' be just as
good?  If yes, then how about

   git ${gitdir:+--git-dir="$gitdir"} rev-parse ...