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

[Eric Sunshine]

On Tue, May 10, 2016 at 5:11 PM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>> 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?
>
> Then you are testing two different things that may go through
> different codepaths.
>
> Adding yet another test to check "git --git-dir=" in addition is
> fine, but that is not a replacement.  We do want to make sure that
> "GIT_DIR=there git" form keeps giving us the expected outcome.

When working on this, I did test with --git-dir= in place of GIT_DIR
and some tests failed, but I didn't follow through to see what the
actual problem was, partly because the code was in flux and I may have
messed up something else, but primarily for the reason Junio gives
above: I wanted this modernization series to be faithful to the
original; additional testing can be added later.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

SZEDER Gábor  writes:

> 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?

Then you are testing two different things that may go through
different codepaths.

Adding yet another test to check "git --git-dir=" in addition is
fine, but that is not a replacement.  We do want to make sure that
"GIT_DIR=there git" form keeps giving us the expected outcome.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[SZEDER Gábor]

Quoting Eric Sunshine :

On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine  
 wrote:

On Tue, May 10, 2016 at 2:39 PM, Jeff King  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 ...


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Tue, May 10, 2016 at 03:59:42PM -0400, Eric Sunshine wrote:

> On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine  
> wrote:
> > 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
> > ...
> > '
> 
> At this point, I'd also rename 'env' to 'gitdir' to be more meaningful.

Yeah, I like this even better. As much as I enjoy eval tricks, I think
this case is more readable with the condition written out.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Eric Sunshine  writes:

> On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine  
> wrote:
>> On Tue, May 10, 2016 at 2:39 PM, Jeff King  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
> ...
> '

That's even better.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine  wrote:
> 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
> ...
> '

At this point, I'd also rename 'env' to 'gitdir' to be more meaningful.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine  wrote:
> On Tue, May 10, 2016 at 2:39 PM, Jeff King  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
...
'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Tue, May 10, 2016 at 2:39 PM, Jeff King  wrote:
> 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...

>> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-g " option to allow callers to
specify the value of the GIT_DIR environment variable explicitly, and
take advantage of this new option to avoid polluting the global scope
with GIT_DIR assignments.

Regarding the implementation: Typically, tests avoid polluting the
global state by wrapping transient environment variable assignments
within a subshell, however, this technique can not be used here since
test_config() and test_unconfig() need to know GIT_DIR, as well, but
neither function can be used within a subshell. Consequently, GIT_DIR is
cleared manually via sane_unset().

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

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 ;;
-*) error "test_rev_parse: unrecognized option '$1'" ;;
*) break ;;
esac
@@ -36,6 +38,8 @@ test_rev_parse () {
do
expect="$1"
test_expect_success "$name: $o" '
+   test_when_finished "sane_unset GIT_DIR" &&
+   eval $env &&
$bare &&
echo "$expect" >expect &&
git $dir rev-parse --$o >actual &&
@@ -61,22 +65,19 @@ test_rev_parse -b t 'core.bare = true' true false false
 test_rev_parse -b u 'core.bare undefined' false false true
 
 test_expect_success 'setup non-local database ../.git' 'mkdir work'
-GIT_DIR=../.git
-export GIT_DIR
 
-test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false 
true ''
+test_rev_parse -C work -g ../.git -b f 'GIT_DIR=../.git, core.bare = false' 
false false true ''
 
-test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false 
false ''
+test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' 
true false false ''
 
-test_rev_parse -C work -b u 'GIT_DIR=../.git, core.bare undefined' false false 
true ''
+test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' 
false false true ''
 
 test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
-GIT_DIR=../repo.git
 
-test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false 
false true ''
+test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = 
false' false false true ''
 
-test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false 
false ''
+test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = 
true' true false false ''
 
-test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false 
false true ''
+test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare 
undefined' false false true ''
 
 test_done
-- 
2.8.2.530.g51d527d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html