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

[Jeff King <peff@peff.net>]

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
> index c058aa4..525e6d3 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -7,11 +7,13 @@ test_description='test git rev-parse'
>  test_rev_parse () {
>  	dir=
>  	bare=
> +	env=
>  	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"

> @@ -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).

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.

-Peff