From: Junio C Hamano Date: Tue, 10 May 2016 19:19:54 GMT Subject: Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests Message-ID: In-Reply-To: Eric Sunshine writes: >> 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... This is a good change, including the quoting of $env. > 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. Which means the two eval's give different results: $ e='echo "a b"' $ eval $e $ eval "$e" >> 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. Yeah, this is OK.