* [PATCH 1/6] t1500: test_rev_parse: facilitate future test enhancements
2016-05-10 5:20 [PATCH 0/6] modernize t1500 Eric Sunshine
@ 2016-05-10 5:20 ` Eric Sunshine
2016-05-10 5:20 ` [PATCH 2/6] t1500: reduce dependence upon global state Eric Sunshine
` (5 subsequent siblings)
6 siblings, 0 replies; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 5:20 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt, Eric Sunshine
Tests run by test_rev_parse() are nearly identical; each invokes
git-rev-parse with a single option and compares the result against an
expected value. Such duplication makes it onerous to extend the tests
since any change needs to be repeated in each test. Reduce the
duplication by parameterizing the test and driving it via a for-loop.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
t/t1500-rev-parse.sh | 44 +++++++++++++++++---------------------------
1 file changed, 17 insertions(+), 27 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..03f22fe 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,38 +3,28 @@
test_description='test git rev-parse'
. ./test-lib.sh
-test_rev_parse() {
+# usage: label is-bare is-inside-git is-inside-work prefix git-dir
+test_rev_parse () {
name=$1
shift
- test_expect_success "$name: is-bare-repository" \
- "test '$1' = \"\$(git rev-parse --is-bare-repository)\""
- shift
- [ $# -eq 0 ] && return
-
- test_expect_success "$name: is-inside-git-dir" \
- "test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
- shift
- [ $# -eq 0 ] && return
-
- test_expect_success "$name: is-inside-work-tree" \
- "test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
- shift
- [ $# -eq 0 ] && return
-
- test_expect_success "$name: prefix" \
- "test '$1' = \"\$(git rev-parse --show-prefix)\""
- shift
- [ $# -eq 0 ] && return
-
- test_expect_success "$name: git-dir" \
- "test '$1' = \"\$(git rev-parse --git-dir)\""
- shift
- [ $# -eq 0 ] && return
+ for o in is-bare-repository \
+ is-inside-git-dir \
+ is-inside-work-tree \
+ show-prefix \
+ git-dir
+ do
+ expect="$1"
+ test_expect_success "$name: $o" '
+ echo "$expect" >expect &&
+ git rev-parse --$o >actual &&
+ test_cmp expect actual
+ '
+ shift
+ test $# -eq 0 && break
+ done
}
-# label is-bare is-inside-git is-inside-work prefix git-dir
-
ROOT=$(pwd)
test_rev_parse toplevel false false true '' .git
--
2.8.2.530.g51d527d
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/6] t1500: reduce dependence upon global state
2016-05-10 5:20 [PATCH 0/6] modernize t1500 Eric Sunshine
2016-05-10 5:20 ` [PATCH 1/6] t1500: test_rev_parse: facilitate future test enhancements Eric Sunshine
@ 2016-05-10 5:20 ` Eric Sunshine
2016-05-10 5:20 ` [PATCH 3/6] t1500: avoid changing working directory outside of tests Eric Sunshine
` (4 subsequent siblings)
6 siblings, 0 replies; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 5:20 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt, Eric Sunshine
This script pollutes its global state by changing its working directory
and capturing that state via $(pwd) in the GIT_CONFIG environment
variable. This makes it difficult to modernize the script since tests
ideally should be self-contained and not rely upon such transient global
state.
With the ultimate goal of eliminating working directory changes, as a
first step, avoid capturing global state by instead taking advantage of
$ROOT which captured $(pwd) prior to any working directory changes, and
compose GIT_CONFIG from it and local knowledge of the working directory.
Subsequent patches will eliminate changes of working directory and drop
GIT_CONFIG altogether.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
t/t1500-rev-parse.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 03f22fe..3d79568 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -49,7 +49,7 @@ test_rev_parse 'core.bare undefined' false false true
mkdir work || exit 1
cd work || exit 1
GIT_DIR=../.git
-GIT_CONFIG="$(pwd)"/../.git/config
+GIT_CONFIG="$ROOT/work/../.git/config"
export GIT_DIR GIT_CONFIG
git config core.bare false
@@ -63,7 +63,7 @@ test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
mv ../.git ../repo.git || exit 1
GIT_DIR=../repo.git
-GIT_CONFIG="$(pwd)"/../repo.git/config
+GIT_CONFIG="$ROOT/work/../repo.git/config"
git config core.bare false
test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''
--
2.8.2.530.g51d527d
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 3/6] t1500: avoid changing working directory outside of tests
2016-05-10 5:20 [PATCH 0/6] modernize t1500 Eric Sunshine
2016-05-10 5:20 ` [PATCH 1/6] t1500: test_rev_parse: facilitate future test enhancements Eric Sunshine
2016-05-10 5:20 ` [PATCH 2/6] t1500: reduce dependence upon global state Eric Sunshine
@ 2016-05-10 5:20 ` Eric Sunshine
2016-05-10 5:20 ` [PATCH 4/6] t1500: avoid setting configuration options " Eric Sunshine
` (3 subsequent siblings)
6 siblings, 0 replies; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 5:20 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt, 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 "-C <dir>" option to allow callers to
instruct it explicitly in which directory its tests should be run, and
take advantage of this new option to avoid changing the working
directory outside of tests.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
t/t1500-rev-parse.sh | 44 ++++++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 3d79568..f294ecc 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,8 +3,18 @@
test_description='test git rev-parse'
. ./test-lib.sh
-# usage: label is-bare is-inside-git is-inside-work prefix git-dir
+# usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir
test_rev_parse () {
+ dir=
+ while :
+ do
+ case "$1" in
+ -C) dir="-C $2"; shift; shift ;;
+ -*) error "test_rev_parse: unrecognized option '$1'" ;;
+ *) break ;;
+ esac
+ done
+
name=$1
shift
@@ -17,7 +27,7 @@ test_rev_parse () {
expect="$1"
test_expect_success "$name: $o" '
echo "$expect" >expect &&
- git rev-parse --$o >actual &&
+ git $dir rev-parse --$o >actual &&
test_cmp expect actual
'
shift
@@ -29,16 +39,11 @@ ROOT=$(pwd)
test_rev_parse toplevel false false true '' .git
-cd .git || exit 1
-test_rev_parse .git/ false true false '' .
-cd objects || exit 1
-test_rev_parse .git/objects/ false true false '' "$ROOT/.git"
-cd ../.. || exit 1
+test_rev_parse -C .git .git/ false true false '' .
+test_rev_parse -C .git/objects .git/objects/ false true false '' "$ROOT/.git"
-mkdir -p sub/dir || exit 1
-cd sub/dir || exit 1
-test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git"
-cd ../.. || exit 1
+test_expect_success 'setup untracked sub/dir' 'mkdir -p sub/dir'
+test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git"
git config core.bare true
test_rev_parse 'core.bare = true' true false false
@@ -46,32 +51,31 @@ test_rev_parse 'core.bare = true' true false false
git config --unset core.bare
test_rev_parse 'core.bare undefined' false false true
-mkdir work || exit 1
-cd work || exit 1
+test_expect_success 'setup non-local database ../.git' 'mkdir work'
GIT_DIR=../.git
GIT_CONFIG="$ROOT/work/../.git/config"
export GIT_DIR GIT_CONFIG
git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''
git config core.bare true
-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''
git config --unset core.bare
-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true ''
-mv ../.git ../repo.git || exit 1
+test_expect_success 'setup non-local database ../repo.git' 'mv .git repo.git'
GIT_DIR=../repo.git
GIT_CONFIG="$ROOT/work/../repo.git/config"
git config core.bare false
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false true ''
git config core.bare true
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false false ''
git config --unset core.bare
-test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
test_done
--
2.8.2.530.g51d527d
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 4/6] t1500: avoid setting configuration options outside of tests
2016-05-10 5:20 [PATCH 0/6] modernize t1500 Eric Sunshine
` (2 preceding siblings ...)
2016-05-10 5:20 ` [PATCH 3/6] t1500: avoid changing working directory outside of tests Eric Sunshine
@ 2016-05-10 5:20 ` Eric Sunshine
2016-05-10 6:34 ` Eric Sunshine
2016-05-10 5:20 ` [PATCH 5/6] t1500: avoid setting environment variables " Eric Sunshine
` (2 subsequent siblings)
6 siblings, 1 reply; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 5:20 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt, 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 "-b <value>" option to allow callers to set
"core.bare" explicitly or undefine it, and take advantage of this new
option to avoid setting "core.bare" outside of tests.
Under the hood, "-b <value>" invokes "test_config -C <dir>" (or
"test_unconfig -C <dir>"), which means that git-config knows explicitly
where to find its configuration file. Consequently, the global
GIT_CONFIG environment variable, which was needed by the manual
git-config invocations outside of tests, is no longer needed, and is
thus dropped.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
t/t1500-rev-parse.sh | 39 ++++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index f294ecc..c058aa4 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -6,15 +6,25 @@ test_description='test git rev-parse'
# usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir
test_rev_parse () {
dir=
+ bare=
while :
do
case "$1" in
-C) dir="-C $2"; shift; shift ;;
+ -b) bare="$2"; shift; shift ;;
-*) error "test_rev_parse: unrecognized option '$1'" ;;
*) break ;;
esac
done
+ case "$bare" in
+ '') ;;
+ t*) bare="test_config $dir core.bare true" ;;
+ f*) bare="test_config $dir core.bare false" ;;
+ u*) bare="test_unconfig $dir core.bare" ;;
+ *) error "test_rev_parse: unrecognized core.bare value '$bare'"
+ esac
+
name=$1
shift
@@ -26,6 +36,7 @@ test_rev_parse () {
do
expect="$1"
test_expect_success "$name: $o" '
+ $bare &&
echo "$expect" >expect &&
git $dir rev-parse --$o >actual &&
test_cmp expect actual
@@ -45,37 +56,27 @@ test_rev_parse -C .git/objects .git/objects/ false true false '' "$ROOT/.git"
test_expect_success 'setup untracked sub/dir' 'mkdir -p sub/dir'
test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git"
-git config core.bare true
-test_rev_parse 'core.bare = true' true false false
+test_rev_parse -b t 'core.bare = true' true false false
-git config --unset core.bare
-test_rev_parse 'core.bare undefined' false false true
+test_rev_parse -b u 'core.bare undefined' false false true
test_expect_success 'setup non-local database ../.git' 'mkdir work'
GIT_DIR=../.git
-GIT_CONFIG="$ROOT/work/../.git/config"
-export GIT_DIR GIT_CONFIG
+export GIT_DIR
-git config core.bare false
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false true ''
-git config core.bare true
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false false ''
-git config --unset core.bare
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_rev_parse -C work -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
-GIT_CONFIG="$ROOT/work/../repo.git/config"
-git config core.bare false
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false true ''
+test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''
-git config core.bare true
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false false ''
+test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''
-git config --unset core.bare
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
test_done
--
2.8.2.530.g51d527d
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 4/6] t1500: avoid setting configuration options outside of tests
2016-05-10 5:20 ` [PATCH 4/6] t1500: avoid setting configuration options " Eric Sunshine
@ 2016-05-10 6:34 ` Eric Sunshine
2016-05-10 18:02 ` Junio C Hamano
0 siblings, 1 reply; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 6:34 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt, Eric Sunshine
On Tue, May 10, 2016 at 1:20 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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 "-b <value>" option to allow callers to set
> "core.bare" explicitly or undefine it, and take advantage of this new
> option to avoid setting "core.bare" outside of tests.
> [...snip...]
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> @@ -6,15 +6,25 @@ test_description='test git rev-parse'
> + case "$bare" in
> + '') ;;
> + t*) bare="test_config $dir core.bare true" ;;
> + f*) bare="test_config $dir core.bare false" ;;
> + u*) bare="test_unconfig $dir core.bare" ;;
> + *) error "test_rev_parse: unrecognized core.bare value '$bare'"
Oops, this line lost its ;; at some point while refining the code.
> + esac
> +
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 4/6] t1500: avoid setting configuration options outside of tests
2016-05-10 6:34 ` Eric Sunshine
@ 2016-05-10 18:02 ` Junio C Hamano
0 siblings, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2016-05-10 18:02 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
Eric Sunshine <sunshine@sunshineco.com> writes:
>> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
>> @@ -6,15 +6,25 @@ test_description='test git rev-parse'
>> + case "$bare" in
>> ...
>> + u*) bare="test_unconfig $dir core.bare" ;;
>> + *) error "test_rev_parse: unrecognized core.bare value '$bare'"
>
> Oops, this line lost its ;; at some point while refining the code.
>
>> + esac
>> +
Strictly speaking, you do not have to have one. I'll squeeze it in
anyways.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 5:20 [PATCH 0/6] modernize t1500 Eric Sunshine
` (3 preceding siblings ...)
2016-05-10 5:20 ` [PATCH 4/6] t1500: avoid setting configuration options " Eric Sunshine
@ 2016-05-10 5:20 ` Eric Sunshine
2016-05-10 18:39 ` Jeff King
2016-05-10 5:20 ` [PATCH 6/6] t1500: be considerate to future potential tests Eric Sunshine
2016-05-10 6:07 ` [PATCH 0/6] modernize t1500 Junio C Hamano
6 siblings, 1 reply; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 5:20 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt, 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 <dir>" 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 <sunshine@sunshineco.com>
---
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
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 5:20 ` [PATCH 5/6] t1500: avoid setting environment variables " Eric Sunshine
@ 2016-05-10 18:39 ` Jeff King
2016-05-10 19:12 ` Eric Sunshine
0 siblings, 1 reply; 42+ messages in thread
From: Jeff King @ 2016-05-10 18:39 UTC (permalink / raw)
To: Eric Sunshine
Cc: git, Junio C Hamano, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 18:39 ` Jeff King
@ 2016-05-10 19:12 ` Eric Sunshine
2016-05-10 19:19 ` Junio C Hamano
2016-05-10 19:48 ` Eric Sunshine
0 siblings, 2 replies; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 19:12 UTC (permalink / raw)
To: Jeff King
Cc: Git List, Junio C Hamano, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
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:
>> 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.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 19:12 ` Eric Sunshine
@ 2016-05-10 19:19 ` Junio C Hamano
2016-05-10 19:48 ` Eric Sunshine
1 sibling, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2016-05-10 19:19 UTC (permalink / raw)
To: Eric Sunshine
Cc: Jeff King, Git List, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
Eric Sunshine <sunshine@sunshineco.com> 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.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 19:12 ` Eric Sunshine
2016-05-10 19:19 ` Junio C Hamano
@ 2016-05-10 19:48 ` Eric Sunshine
2016-05-10 19:59 ` Eric Sunshine
` (2 more replies)
1 sibling, 3 replies; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 19:48 UTC (permalink / raw)
To: Jeff King
Cc: Git List, Junio C Hamano, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
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
...
'
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 19:48 ` Eric Sunshine
@ 2016-05-10 19:59 ` Eric Sunshine
2016-05-10 20:41 ` Jeff King
2016-05-10 20:07 ` Junio C Hamano
2016-05-10 21:01 ` SZEDER Gábor
2 siblings, 1 reply; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 19:59 UTC (permalink / raw)
To: Jeff King
Cc: Git List, Junio C Hamano, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine <sunshine@sunshineco.com> 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.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 19:59 ` Eric Sunshine
@ 2016-05-10 20:41 ` Jeff King
0 siblings, 0 replies; 42+ messages in thread
From: Jeff King @ 2016-05-10 20:41 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Junio C Hamano, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
On Tue, May 10, 2016 at 03:59:42PM -0400, Eric Sunshine wrote:
> On Tue, May 10, 2016 at 3:48 PM, Eric Sunshine <sunshine@sunshineco.com> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 19:48 ` Eric Sunshine
2016-05-10 19:59 ` Eric Sunshine
@ 2016-05-10 20:07 ` Junio C Hamano
2016-05-10 21:01 ` SZEDER Gábor
2 siblings, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2016-05-10 20:07 UTC (permalink / raw)
To: Eric Sunshine
Cc: Jeff King, Git List, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
Eric Sunshine <sunshine@sunshineco.com> writes:
> 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
> ...
> '
That's even better. Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 19:48 ` Eric Sunshine
2016-05-10 19:59 ` Eric Sunshine
2016-05-10 20:07 ` Junio C Hamano
@ 2016-05-10 21:01 ` SZEDER Gábor
2016-05-10 21:11 ` Junio C Hamano
2 siblings, 1 reply; 42+ messages in thread
From: SZEDER Gábor @ 2016-05-10 21:01 UTC (permalink / raw)
To: Eric Sunshine
Cc: Jeff King, Git List, Junio C Hamano, Michael Rappazzo,
Duy Nguyen, Johannes Sixt
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 ...
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 21:01 ` SZEDER Gábor
@ 2016-05-10 21:11 ` Junio C Hamano
2016-05-10 21:19 ` Eric Sunshine
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2016-05-10 21:11 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Eric Sunshine, Jeff King, Git List, Michael Rappazzo, Duy Nguyen,
Johannes Sixt
SZEDER Gábor <szeder@ira.uka.de> 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.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 5/6] t1500: avoid setting environment variables outside of tests
2016-05-10 21:11 ` Junio C Hamano
@ 2016-05-10 21:19 ` Eric Sunshine
0 siblings, 0 replies; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 21:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, Jeff King, Git List, Michael Rappazzo,
Duy Nguyen, Johannes Sixt
On Tue, May 10, 2016 at 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder@ira.uka.de> 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.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 6/6] t1500: be considerate to future potential tests
2016-05-10 5:20 [PATCH 0/6] modernize t1500 Eric Sunshine
` (4 preceding siblings ...)
2016-05-10 5:20 ` [PATCH 5/6] t1500: avoid setting environment variables " Eric Sunshine
@ 2016-05-10 5:20 ` Eric Sunshine
2016-05-10 6:07 ` [PATCH 0/6] modernize t1500 Junio C Hamano
6 siblings, 0 replies; 42+ messages in thread
From: Eric Sunshine @ 2016-05-10 5:20 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt, Eric Sunshine
The final batch of git-rev-parse tests work against a non-local object
database named ../repo.git rather than the typically-named ../.git. It
prepares by renaming .git/ to repo.git/ and pointing GIT_DIR at
../repo.git, but never restores the name to .git/, which can be
problematic for tests added in the future. Be more friendly by instead
making repo.git/ a copy of .git/.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
t/t1500-rev-parse.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 525e6d3..af223ed 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -72,7 +72,7 @@ test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' 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'
+test_expect_success 'setup non-local database ../repo.git' 'cp -R .git repo.git'
test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''
--
2.8.2.530.g51d527d
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 0/6] modernize t1500
2016-05-10 5:20 [PATCH 0/6] modernize t1500 Eric Sunshine
` (5 preceding siblings ...)
2016-05-10 5:20 ` [PATCH 6/6] t1500: be considerate to future potential tests Eric Sunshine
@ 2016-05-10 6:07 ` Junio C Hamano
2016-05-10 18:10 ` Junio C Hamano
6 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2016-05-10 6:07 UTC (permalink / raw)
To: Eric Sunshine
Cc: git, Jeff King, Michael Rappazzo, Duy Nguyen, SZEDER Gábor,
Johannes Sixt
Eric Sunshine <sunshine@sunshineco.com> writes:
> This series modernizes t1500; it takes an entirely different approach
> than [1][2] and is intended to replace that series.
Turns out that it wasn't so painful after all.
The only small niggle I have is on 6/6; my preference would be,
because once we set up .git we do not add objects and refs to it, to
make a copy a lot earlier before the tests start.
I'll let it simmer on the list overnight and take a fresh look in
the morning, but I think I like these better than what I had to
tweak yesterday.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/6] modernize t1500
2016-05-10 6:07 ` [PATCH 0/6] modernize t1500 Junio C Hamano
@ 2016-05-10 18:10 ` Junio C Hamano
2016-05-10 18:26 ` Junio C Hamano
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2016-05-10 18:10 UTC (permalink / raw)
To: Eric Sunshine
Cc: git, Jeff King, Michael Rappazzo, Duy Nguyen, SZEDER Gábor,
Johannes Sixt
Junio C Hamano <gitster@pobox.com> writes:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> This series modernizes t1500; it takes an entirely different approach
>> than [1][2] and is intended to replace that series.
>
> Turns out that it wasn't so painful after all.
>
> The only small niggle I have is on 6/6; my preference would be,
> because once we set up .git we do not add objects and refs to it, to
> make a copy a lot earlier before the tests start.
-- >8 --
Subject: [PATCH 7/6] t1500: finish preparation upfront
The earlier tests do not attempt to modify the contents of .git (by
creating objects or refs, for example), which means that even if
some earlier tests before "cp -R" fail, the tests in repo.git will
run in an environment that we can expect them to succeed in.
Make it clear that tests in repo.git will be independent from the
results of earlier tests done in .git by moving its initialization
"cp -R .git repo.git" much higher in the test sequence.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t1500-rev-parse.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 811c70f2..cb08677 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -51,6 +51,7 @@ test_rev_parse () {
}
ROOT=$(pwd)
+test_expect_success 'setup non-local database ../repo.git' 'cp -R .git repo.git'
test_rev_parse toplevel false false true '' .git
@@ -72,8 +73,6 @@ test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' 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' 'cp -R .git repo.git'
-
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 -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''
--
2.8.2-623-gacdd3f1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 0/6] modernize t1500
2016-05-10 18:10 ` Junio C Hamano
@ 2016-05-10 18:26 ` Junio C Hamano
2016-05-16 17:39 ` Eric Sunshine
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2016-05-10 18:26 UTC (permalink / raw)
To: Eric Sunshine
Cc: git, Jeff King, Michael Rappazzo, Duy Nguyen, SZEDER Gábor,
Johannes Sixt
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> This series modernizes t1500; it takes an entirely different approach
>>> than [1][2] and is intended to replace that series.
>>
>> Turns out that it wasn't so painful after all.
>>
>> The only small niggle I have is on 6/6; my preference would be,
>> because once we set up .git we do not add objects and refs to it, to
>> make a copy a lot earlier before the tests start.
>
> -- >8 --
> Subject: [PATCH 7/6] t1500: finish preparation upfront
>
> The earlier tests do not attempt to modify the contents of .git (by
> creating objects or refs, for example), which means that even if
> some earlier tests before "cp -R" fail, the tests in repo.git will
> run in an environment that we can expect them to succeed in.
>
> Make it clear that tests in repo.git will be independent from the
> results of earlier tests done in .git by moving its initialization
> "cp -R .git repo.git" much higher in the test sequence.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
I think the same logic applies to other preparatory things like
creation of sub/dir in the working tree etc.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/6] modernize t1500
2016-05-10 18:26 ` Junio C Hamano
@ 2016-05-16 17:39 ` Eric Sunshine
2016-05-16 18:52 ` Junio C Hamano
0 siblings, 1 reply; 42+ messages in thread
From: Eric Sunshine @ 2016-05-16 17:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
On Tue, May 10, 2016 at 2:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Junio C Hamano <gitster@pobox.com> writes:
>> Subject: [PATCH 7/6] t1500: finish preparation upfront
>>
>> The earlier tests do not attempt to modify the contents of .git (by
>> creating objects or refs, for example), which means that even if
>> some earlier tests before "cp -R" fail, the tests in repo.git will
>> run in an environment that we can expect them to succeed in.
>>
>> Make it clear that tests in repo.git will be independent from the
>> results of earlier tests done in .git by moving its initialization
>> "cp -R .git repo.git" much higher in the test sequence.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>
> I think the same logic applies to other preparatory things like
> creation of sub/dir in the working tree etc.
Hmm, so are you suggesting a single 'setup' test at the start of
script which does the 'cp -R' and creates those other directories
needed by later tests?
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/6] modernize t1500
2016-05-16 17:39 ` Eric Sunshine
@ 2016-05-16 18:52 ` Junio C Hamano
0 siblings, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2016-05-16 18:52 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Jeff King, Michael Rappazzo, Duy Nguyen,
SZEDER Gábor, Johannes Sixt
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Tue, May 10, 2016 at 2:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> Subject: [PATCH 7/6] t1500: finish preparation upfront
>>>
>>> The earlier tests do not attempt to modify the contents of .git (by
>>> creating objects or refs, for example), which means that even if
>>> some earlier tests before "cp -R" fail, the tests in repo.git will
>>> run in an environment that we can expect them to succeed in.
>>>
>>> Make it clear that tests in repo.git will be independent from the
>>> results of earlier tests done in .git by moving its initialization
>>> "cp -R .git repo.git" much higher in the test sequence.
>>>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> ---
>>
>> I think the same logic applies to other preparatory things like
>> creation of sub/dir in the working tree etc.
>
> Hmm, so are you suggesting a single 'setup' test at the start of
> script which does the 'cp -R' and creates those other directories
> needed by later tests?
Exactly.
^ permalink raw reply [flat|nested] 42+ messages in thread