Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 10, 2016, 19:12 UTC
- Message-ID
- <CAPig+cRR49iJOkaLfynkvch4zUHVkpcJwVah0rvaEOeR7aY2Nw@mail.gmail.com>
- In-Reply-To
- <20160510183955.GA16211@sigill.intra.peff.net>
On Tue, May 10, 2016 at 2:39 PM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:
>> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
>> @@ -7,11 +7,13 @@ test_description='test git rev-parse'
>> 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 ;;
>
> This will expand $2 inside $env, which is later eval'd. So funny things
> happen if there are spaces or metacharacters. It looks like you only use
> it with short relative paths ("../repo.git", etc), which is OK, but this
> would probably break badly if we ever used absolute paths.
>
> I don't know if it's worth worrying about or not. The usual solution is
> something like:
>
> env_git_dir=$2
> env='GIT_DIR=$env_git_dir; export GIT_DIR'
> ...
> eval "$env"Makes sense; I wasn't quite happy with having $2 interpolated unquoted. Like you, though, I don't know if it's worth worrying about...
Show 10 quoted lines
>> @@ -36,6 +38,8 @@ test_rev_parse () {
>> do
>> expect="$1"
>> test_expect_success "$name: $o" '
>> + test_when_finished "sane_unset GIT_DIR" &&
>> + eval $env &&
>
> I was surprised not to see quoting around $env here, but it probably
> doesn't matter (I think it may affect how some whitespace is treated,
> but the contents of $env are pretty tame).I flip-flopped on this one several times, quoting, and not quoting. Documentation for 'eval' says:
The args are read and concatenated together into a single
command.so, I ultimately left it unquoted, but don't feel strongly about it.
> 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.
Nevertheless, I can re-roll with these changes if you feel more strongly than I about it.