I was working on a simple bug fix[1], and I wanted to add a test to t1500. I put
the new test at the beginning rather than try to decipher what state the test run
would be in at the end. In the review, Eric Sunshine described the test:
this script is ancient and cd's all around the place with wild abandon and
leaks environment variables
I decided to rewrite this test in (what I hope is) the modern test format. In
doing so, I expanded the size of the test file greatly, but I think it is much
clearer to read and understand as you go through it. This also has the advantage
of allowing a tester to use verbose options when testing. As noted in the commit
comment, tests which compare text (rather than booleans) were also adjusted to use
the test_cmp function.
This patch is based on 'pu' because of a feature to add `--absolute-git-dir` to
rev-parse[2].
I considered including this patch as the first commit in the aforementioned bug
fix, but I wanted to get it out for review to make sure that it is acceptible.
[1]http://thread.gmane.org/gmane.comp.version-control.git/290669
[2]http://article.gmane.org/gmane.comp.version-control.git/287462
Michael Rappazzo (1):
t1500-rev-parse: rewrite each test to run in isolation
t/t1500-rev-parse.sh | 607 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 545 insertions(+), 62 deletions(-)
--
2.8.0
t1500-rev-parse has many tests which change directories and leak
environment variables. This makes it difficult to add new tests without
minding the environment variables and current directory.
Each test is now setup, executed, and cleaned up without leaving anything
behind. Tests which have textual expectations have been converted to use
test_cmp (which will show a diff when the test is run with --verbose).
Signed-off-by: Michael Rappazzo <redacted>
---
t/t1500-rev-parse.sh | 607 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 545 insertions(+), 62 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 617fcd8..dffa9f3 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,88 +3,571 @@
test_description='test git rev-parse'
. ./test-lib.sh
-test_rev_parse() {
- name=$1
- shift
+test_expect_success 'toplevel: is-bare-repository' '
+ test false = "$(git rev-parse --is-bare-repository)"
+'
- test_expect_success "$name: is-bare-repository" \
- "test '$1' = \"\$(git rev-parse --is-bare-repository)\""
- shift
- [ $# -eq 0 ] && return
+test_expect_success 'toplevel: is-inside-git-dir' '
+ test false = "$(git rev-parse --is-inside-git-dir)"
+'
- test_expect_success "$name: is-inside-git-dir" \
- "test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
- shift
- [ $# -eq 0 ] && return
+test_expect_success 'toplevel: is-inside-work-tree' '
+ test true = "$(git rev-parse --is-inside-work-tree)"
+'
- test_expect_success "$name: is-inside-work-tree" \
- "test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
- shift
- [ $# -eq 0 ] && return
+test_expect_success 'toplevel: prefix' '
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+'
- test_expect_success "$name: prefix" \
- "test '$1' = \"\$(git rev-parse --show-prefix)\""
- shift
- [ $# -eq 0 ] && return
+test_expect_success 'toplevel: git-dir' '
+ echo .git >expected &&
+ git rev-parse --git-dir >actual &&
+ test_cmp expected actual
+'
- test_expect_success "$name: git-dir" \
- "test '$1' = \"\$(git rev-parse --git-dir)\""
- shift
- [ $# -eq 0 ] && return
+test_expect_success 'toplevel: absolute-git-dir' '
+ echo "$(pwd)/.git" >expected &&
+ git rev-parse --absolute-git-dir >actual &&
+ test_cmp expected actual
+'
- test_expect_success "$name: absolute-git-dir" \
- "verbose test '$1' = \"\$(git rev-parse --absolute-git-dir)\""
-}
+test_expect_success '.git/: is-bare-repository' '
+ (cd .git && test false = "$(git rev-parse --is-bare-repository)")
+'
-# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
+test_expect_success '.git/: is-inside-git-dir' '
+ (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")
+'
-ROOT=$(pwd)
+test_expect_success '.git/: is-inside-work-tree' '
+ (cd .git && test false = "$(git rev-parse --is-inside-work-tree)")
+'
-test_rev_parse toplevel false false true '' .git "$ROOT/.git"
+test_expect_success '.git/: prefix' '
+ (
+ cd .git &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
+'
-cd .git || exit 1
-test_rev_parse .git/ false true false '' . "$ROOT/.git"
-cd objects || exit 1
-test_rev_parse .git/objects/ false true false '' "$ROOT/.git" "$ROOT/.git"
-cd ../.. || exit 1
+test_expect_success '.git/: git-dir' '
+ (
+ cd .git &&
+ echo . >expected &&
+ git rev-parse --git-dir >actual &&
+ test_cmp expected actual
+ )
+'
-mkdir -p sub/dir || exit 1
-cd sub/dir || exit 1
-test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git" "$ROOT/.git"
-cd ../.. || exit 1
+test_expect_success '.git/: absolute-git-dir' '
+ (
+ ROOT=$(pwd) &&
+ cd .git &&
+ echo "$ROOT/.git" >expected &&
+ git rev-parse --absolute-git-dir >actual &&
+ test_cmp expected actual
+ )
+'
-git config core.bare true
-test_rev_parse 'core.bare = true' true false false
+test_expect_success '.git/objects/: is-bare-repository' '
+ (cd .git/objects && test false = "$(git rev-parse --is-bare-repository)")
+'
-git config --unset core.bare
-test_rev_parse 'core.bare undefined' false false true
+test_expect_success '.git/objects/: is-inside-git-dir' '
+ (cd .git/objects && test true = "$(git rev-parse --is-inside-git-dir)")
+'
-mkdir work || exit 1
-cd work || exit 1
-GIT_DIR=../.git
-GIT_CONFIG="$(pwd)"/../.git/config
-export GIT_DIR GIT_CONFIG
+test_expect_success '.git/objects/: is-inside-work-tree' '
+ (cd .git/objects && test false = "$(git rev-parse --is-inside-work-tree)")
+'
-git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true '' "../.git" "$ROOT/.git"
+test_expect_success '.git/objects/: prefix' '
+ (
+ cd .git/objects &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
+'
-git config core.bare true
-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_expect_success '.git/objects/: git-dir' '
+ (
+ ROOT=$(pwd) &&
+ cd .git/objects &&
+ echo $ROOT/.git >expected &&
+ git rev-parse --git-dir >actual &&
+ test_cmp expected actual
+ )
+'
-git config --unset core.bare
-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_expect_success '.git/objects/: absolute-git-dir' '
+ (
+ ROOT=$(pwd) &&
+ cd .git/objects &&
+ echo "$ROOT/.git" >expected &&
+ git rev-parse --absolute-git-dir >actual &&
+ test_cmp expected actual
+ )
+'
-mv ../.git ../repo.git || exit 1
-GIT_DIR=../repo.git
-GIT_CONFIG="$(pwd)"/../repo.git/config
+test_expect_success 'subdirectory: is-bare-repository' '
+ mkdir -p sub/dir &&
+ test_when_finished "rm -rf sub" &&
+ (cd sub/dir && test false = "$(git rev-parse --is-bare-repository)")
+'
-git config core.bare false
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true '' "../repo.git" "$ROOT/repo.git"
+test_expect_success 'subdirectory: is-inside-git-dir' '
+ mkdir -p sub/dir &&
+ test_when_finished "rm -rf sub" &&
+ (cd sub/dir && test false = "$(git rev-parse --is-inside-git-dir)")
+'
-git config core.bare true
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
+test_expect_success 'subdirectory: is-inside-work-tree' '
+ mkdir -p sub/dir &&
+ test_when_finished "rm -rf sub" &&
+ (cd sub/dir && test true = "$(git rev-parse --is-inside-work-tree)")
+'
-git config --unset core.bare
-test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_expect_success 'subdirectory: prefix' '
+ mkdir -p sub/dir &&
+ test_when_finished "rm -rf sub" &&
+ (cd sub/dir && test sub/dir/ = "$(git rev-parse --show-prefix)")
+'
+
+test_expect_success 'subdirectory: git-dir' '
+ mkdir -p sub/dir &&
+ test_when_finished "rm -rf sub" &&
+ (
+ ROOT=$(pwd) &&
+ cd sub/dir &&
+ echo $ROOT/.git >expected &&
+ git rev-parse --git-dir >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'subdirectory: absolute-git-dir' '
+ mkdir -p sub/dir &&
+ test_when_finished "rm -rf sub" &&
+ (
+ ROOT=$(pwd) &&
+ cd sub/dir &&
+ echo $ROOT/.git >expected &&
+ git rev-parse --absolute-git-dir >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'core.bare = true: is-bare-repository' '
+ git config core.bare true &&
+ test_when_finished "git config core.bare false" &&
+ test true = "$(git rev-parse --is-bare-repository)"
+'
+
+test_expect_success 'core.bare = true: is-inside-git-dir' '
+ git config core.bare true &&
+ test_when_finished "git config core.bare false" &&
+ test false = "$(git rev-parse --is-inside-git-dir)"
+'
+
+test_expect_success 'core.bare = true: is-inside-work-tree' '
+ git config core.bare true &&
+ test_when_finished "git config core.bare false" &&
+ test false = "$(git rev-parse --is-inside-work-tree)"
+'
+
+test_expect_success 'core.bare undefined: is-bare-repository' '
+ git config --unset core.bare &&
+ test_when_finished "git config core.bare false" &&
+ test false = "$(git rev-parse --is-bare-repository)"
+'
+
+test_expect_success 'core.bare undefined: is-inside-git-dir' '
+ git config --unset core.bare &&
+ test_when_finished "git config core.bare false" &&
+ test false = "$(git rev-parse --is-inside-git-dir)"
+'
+
+test_expect_success 'core.bare undefined: is-inside-work-tree' '
+ git config --unset core.bare &&
+ test_when_finished "git config core.bare false" &&
+ test true = "$(git rev-parse --is-inside-work-tree)"
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config
+ git config core.bare false &&
+ test false = "$(git rev-parse --is-bare-repository)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: is-inside-git-dir' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config core.bare false &&
+ test false = "$(git rev-parse --is-inside-git-dir)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: is-inside-work-tree' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config core.bare false &&
+ test true = "$(git rev-parse --is-inside-work-tree)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: prefix' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config core.bare false &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: git-dir' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config core.bare false &&
+ echo ../.git >expected &&
+ git rev-parse --git-dir >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: absolute-git-dir' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ ROOT=$(pwd) &&
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config core.bare false &&
+ echo $ROOT/.git >expected &&
+ git rev-parse --absolute-git-dir >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = true: is-bare-repository' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config core.bare true &&
+ test true = "$(git rev-parse --is-bare-repository)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = true: is-inside-git-dir' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config core.bare true &&
+ test false = "$(git rev-parse --is-inside-git-dir)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = true: is-inside-work-tree' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config core.bare true &&
+ test false = "$(git rev-parse --is-inside-work-tree)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = true: prefix' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config core.bare true &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-bare-repository' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config --unset core.bare &&
+ test false = "$(git rev-parse --is-bare-repository)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-inside-git-dir' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config --unset core.bare &&
+ test false = "$(git rev-parse --is-inside-git-dir)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-inside-work-tree' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config --unset core.bare &&
+ test true = "$(git rev-parse --is-inside-work-tree)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare undefined: prefix' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config &&
+ git config --unset core.bare &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config
+ git config core.bare false &&
+ test false = "$(git rev-parse --is-bare-repository)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-git-dir' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config core.bare false &&
+ test false = "$(git rev-parse --is-inside-git-dir)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-work-tree' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config core.bare false &&
+ test true = "$(git rev-parse --is-inside-work-tree)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: prefix' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config core.bare false &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: git-dir' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config core.bare false &&
+ echo ../repo.git >expected &&
+ git rev-parse --git-dir >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: absolute-git-dir' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ ROOT=$(pwd) &&
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config core.bare false &&
+ echo $ROOT/repo.git >expected &&
+ git rev-parse --absolute-git-dir >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-bare-repository' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config core.bare true &&
+ test true = "$(git rev-parse --is-bare-repository)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-git-dir' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config core.bare true &&
+ test false = "$(git rev-parse --is-inside-git-dir)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-work-tree' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config core.bare true &&
+ test false = "$(git rev-parse --is-inside-work-tree)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: prefix' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config core.bare true &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-bare-repository' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config --unset core.bare &&
+ test false = "$(git rev-parse --is-bare-repository)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-git-dir' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config --unset core.bare &&
+ test false = "$(git rev-parse --is-inside-git-dir)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-work-tree' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config --unset core.bare &&
+ test true = "$(git rev-parse --is-inside-work-tree)"
+ )
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: prefix' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+ git config --unset core.bare &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
+'
test_done--
2.8.0
On Sat, Apr 9, 2016 at 7:19 AM, Michael Rappazzo [off-list ref] wrote:
t1500-rev-parse has many tests which change directories and leak
environment variables. This makes it difficult to add new tests without
minding the environment variables and current directory.
Each test is now setup, executed, and cleaned up without leaving anything
behind. Tests which have textual expectations have been converted to use
test_cmp (which will show a diff when the test is run with --verbose).
It might be easier to review this if broken into several cleanup and
modernization patches, however, some comments below...
quoted hunk
Signed-off-by: Michael Rappazzo <redacted>
---
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
@@ -3,88 +3,571 @@
+test_expect_success '.git/: is-bare-repository' '
+ (cd .git && test false = "$(git rev-parse --is-bare-repository)")
+'
-# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
+test_expect_success '.git/: is-inside-git-dir' '
+ (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")
Simpler:
test true = "$(git -C .git rev-parse --is-inside-git-dir)"
+'
-ROOT=$(pwd)
+test_expect_success '.git/: is-inside-work-tree' '
+ (cd .git && test false = "$(git rev-parse --is-inside-work-tree)")
Ditto.
+'
-test_rev_parse toplevel false false true '' .git "$ROOT/.git"
+test_expect_success '.git/: prefix' '
+ (
+ cd .git &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
Likewise, you could drop the entire subshell:
echo >expected &&
git -C .git rev-parse --show-prefix >actual &&
test_cmp expected actual
+'
+test_expect_success '.git/: git-dir' '
+ (
+ cd .git &&
+ echo . >expected &&
+ git rev-parse --git-dir >actual &&
+ test_cmp expected actual
+ )
Same here and for many subsequent tests (which I won't quote).
+'
+test_expect_success 'core.bare = true: is-bare-repository' '
+ git config core.bare true &&
+ test_when_finished "git config core.bare false" &&
+ test true = "$(git rev-parse --is-bare-repository)"
Simpler:
test_config core.bare true
and then you can drop 'test_when_finished' altogether. However, even simpler:
test true = "$(git -c core.bare=false rev-parse --is-bare-repository)"
which allows you to drop 'test_config', as well.
Ditto for subsequent tests (which I won't quote).
+'
+test_expect_success 'core.bare undefined: is-bare-repository' '
+ git config --unset core.bare &&
test_unconfig core.bare
+ test_when_finished "git config core.bare false" &&
Why does this need to re-instate core.bare?
Same comments for subsequent tests.
+ test false = "$(git rev-parse --is-bare-repository)"
+'
+test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config
+ git config core.bare false &&
+ test false = "$(git rev-parse --is-bare-repository)"
+ )
+'
Same comments about -C to avoid the subshell and -c for configuration.
Also, you can do one-shot environment variable setting for the command
invocation, so the subshell goes away, and everything inside the
subshell collapses to:
test false = "$(GIT_DIR=... GIT_CONFIG=...
git -C work -c core.bare=false rev-parse ...)"
Additionally, I'm confused about why this test "reverts" the
core.bare=false by setting core.bare=false in 'test_when_finished'.
Ditto for subsequent tests.
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
If 'cp' fails, then 'test_when_finished' will never be invoked, which
means that the cleanup will never happen; thus 'test_when_finished'
needs to be called earlier. Ditto for subsequent tests.
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config
+ git config core.bare false &&
+ test false = "$(git rev-parse --is-bare-repository)"
+ )
+'
Closing comments:
By using -C, -c, and one-shot environment variables, you can ditch the
subshells, and most of these tests should collapse to one or two
lines.
There seems to be a lot of repetition here. To reduce the repetition,
have you considered encoding the state which varies between tests into
a table and making the tests table-driven. Or, by encoding the varying
state in some nested for-loops? The nice thing about driving the tests
from a table or for-loops is that it is easier to see at a glance if
all cases are covered.
On Wed, Apr 13, 2016 at 12:54 AM, Eric Sunshine [off-list ref] wrote:
On Sat, Apr 9, 2016 at 7:19 AM, Michael Rappazzo [off-list ref] wrote:
quoted
t1500-rev-parse has many tests which change directories and leak
environment variables. This makes it difficult to add new tests without
minding the environment variables and current directory.
Each test is now setup, executed, and cleaned up without leaving anything
behind. Tests which have textual expectations have been converted to use
test_cmp (which will show a diff when the test is run with --verbose).
It might be easier to review this if broken into several cleanup and
modernization patches, however, some comments below...
I felt that the changes are repetitive enough that it did not
necessitate separate patches. Perhaps after simplifying based on your
suggestions, it will be even smaller.
quoted
Signed-off-by: Michael Rappazzo <redacted>
---
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
@@ -3,88 +3,571 @@
+test_expect_success '.git/: is-bare-repository' '
+ (cd .git && test false = "$(git rev-parse --is-bare-repository)")
+'
-# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
+test_expect_success '.git/: is-inside-git-dir' '
+ (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")
Simpler:
test true = "$(git -C .git rev-parse --is-inside-git-dir)"
quoted
+'
-ROOT=$(pwd)
+test_expect_success '.git/: is-inside-work-tree' '
+ (cd .git && test false = "$(git rev-parse --is-inside-work-tree)")
Ditto.
quoted
+'
-test_rev_parse toplevel false false true '' .git "$ROOT/.git"
+test_expect_success '.git/: prefix' '
+ (
+ cd .git &&
+ echo >expected &&
+ git rev-parse --show-prefix >actual &&
+ test_cmp expected actual
+ )
Likewise, you could drop the entire subshell:
echo >expected &&
git -C .git rev-parse --show-prefix >actual &&
test_cmp expected actual
quoted
+'
+test_expect_success '.git/: git-dir' '
+ (
+ cd .git &&
+ echo . >expected &&
+ git rev-parse --git-dir >actual &&
+ test_cmp expected actual
+ )
Same here and for many subsequent tests (which I won't quote).
quoted
+'
+test_expect_success 'core.bare = true: is-bare-repository' '
+ git config core.bare true &&
+ test_when_finished "git config core.bare false" &&
+ test true = "$(git rev-parse --is-bare-repository)"
Simpler:
test_config core.bare true
and then you can drop 'test_when_finished' altogether. However, even simpler:
test true = "$(git -c core.bare=false rev-parse --is-bare-repository)"
which allows you to drop 'test_config', as well.
Ditto for subsequent tests (which I won't quote).
quoted
+'
+test_expect_success 'core.bare undefined: is-bare-repository' '
+ git config --unset core.bare &&
test_unconfig core.bare
quoted
+ test_when_finished "git config core.bare false" &&
Why does this need to re-instate core.bare?
Same comments for subsequent tests.
quoted
+ test false = "$(git rev-parse --is-bare-repository)"
+'
+test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' '
+ mkdir work &&
+ test_when_finished "rm -rf work && git config core.bare false" &&
+ (
+ cd work &&
+ export GIT_DIR=../.git &&
+ export GIT_CONFIG="$(pwd)"/../.git/config
+ git config core.bare false &&
+ test false = "$(git rev-parse --is-bare-repository)"
+ )
+'
Same comments about -C to avoid the subshell and -c for configuration.
Also, you can do one-shot environment variable setting for the command
invocation, so the subshell goes away, and everything inside the
subshell collapses to:
test false = "$(GIT_DIR=... GIT_CONFIG=...
git -C work -c core.bare=false rev-parse ...)"
Additionally, I'm confused about why this test "reverts" the
core.bare=false by setting core.bare=false in 'test_when_finished'.
Ditto for subsequent tests.
quoted
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
+ mkdir work &&
+ cp -r .git repo.git &&
+ test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
If 'cp' fails, then 'test_when_finished' will never be invoked, which
means that the cleanup will never happen; thus 'test_when_finished'
needs to be called earlier. Ditto for subsequent tests.
quoted
+ (
+ cd work &&
+ export GIT_DIR=../repo.git &&
+ export GIT_CONFIG="$(pwd)"/../repo.git/config
+ git config core.bare false &&
+ test false = "$(git rev-parse --is-bare-repository)"
+ )
+'
Closing comments:
By using -C, -c, and one-shot environment variables, you can ditch the
subshells, and most of these tests should collapse to one or two
lines.
I follow, and I can make that adjustment. Thanks for the review.
There seems to be a lot of repetition here. To reduce the repetition,
have you considered encoding the state which varies between tests into
a table and making the tests table-driven. Or, by encoding the varying
state in some nested for-loops? The nice thing about driving the tests
from a table or for-loops is that it is easier to see at a glance if
all cases are covered.
I was aiming for readability and the ability to test an individual
case. The previous version was driven by a function, which did reduce
the repetition, but it made it not simple to run an individual test in
the case of a failure. Would your table implementation be much
different from the function, in terms of readability?
On Wed, Apr 13, 2016 at 12:54:16AM -0400, Eric Sunshine wrote:
quoted
-# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
+test_expect_success '.git/: is-inside-git-dir' '
+ (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")
Simpler:
test true = "$(git -C .git rev-parse --is-inside-git-dir)"
While we are modernizing, I wonder if it is a good idea to drop the use
of "test" for string comparisons here. We usually save output to a file
and use test_cmp, for two reasons:
1. It tests the exit code of the command substitution.
2. It's easier to debug when it goes wrong (the test produces useful
output, and the state is left in the filesystem).
It is more verbose to do so, but we could easily wrap that in:
test_stdout "true" git -C .git rev-parse --is-inside-git-dir
or something. The other problem is that it uses an extra process to do
the test_cmp, which might make the tests slower (especially on Windows).
We could do something like:
test_stdout () {
expect=$1; shift
if ! actual=$("$@")
then
echo >&2 "test_stdout: command failed: $*"
return 1
fi
if test "$expect" != "$actual"
then
echo >&2 "test_stdout: unexpected output for $*"
echo >&2 "test_stdout: $expect != $actual"
return 1
fi
return 0
}
which is more thorough without incurring extra processes (we lose the
nice diff output of test_cmp, but since this interface is only useful
for short outputs anyway, I don't think that's a big deal.
-Peff