[PATCH v3 0/2] [PATCH v3 0/2] rev-parse: fix some options when executed from subpath of main tree

[PATCH v3 0/2] [PATCH v3 0/2] rev-parse: fix some options when executed from subpath of main tree

[Michael Rappazzo]

Differenced from v2[1]:
 - Rewrote the commits to just two; one that introduces tests, and 
   one which fixes the problem.

[1] http://thread.gmane.org/gmane.comp.version-control.git/292272

Michael Rappazzo (2):
  rev-parse tests: add tests executed from a subdirectory
  rev-parse: fix some options when executed from subpath of main tree

 builtin/rev-parse.c      | 19 ++++++++++++++-----
 t/t1500-rev-parse.sh     | 38 ++++++++++++++++++++++++++++++++++++++
 t/t1700-split-index.sh   | 17 +++++++++++++++++
 t/t2027-worktree-list.sh | 10 +++++++++-
 4 files changed, 78 insertions(+), 6 deletions(-)

-- 
2.8.0

[PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Michael Rappazzo]

t1500-rev-parse contains envrionment leaks (changing dir without
changing back, setting config variables, etc).  Add a test to clean this
up up so that future tests can be added without worry of any setting
from a previous test.

t2027-worktree-list has an incorrect expectation for --git-common-dir
which has been adjusted and marked to expect failure.

Some of the tests added have been marked to expect failure.  These
demonstrate a problem with the way that some options to git rev-parse
behave when executed from a subdirectory of the main worktree.

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 t/t1500-rev-parse.sh     | 38 ++++++++++++++++++++++++++++++++++++++
 t/t1700-split-index.sh   | 17 +++++++++++++++++
 t/t2027-worktree-list.sh | 12 ++++++++++--
 3 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..442ca46 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -36,6 +36,7 @@ test_rev_parse() {
 # label is-bare is-inside-git is-inside-work prefix git-dir
 
 ROOT=$(pwd)
+original_core_bare=$(git config core.bare)
 
 test_rev_parse toplevel false false true '' .git
 
@@ -84,4 +85,41 @@ test_rev_parse '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_expect_success 'cleanup from the previous tests' '
+	cd .. &&
+	rm -r work &&
+	mv repo.git .git &&
+	unset GIT_DIR &&
+	unset GIT_CONFIG &&
+	git config core.bare $original_core_bare
+'
+
+test_expect_success 'git-common-dir from worktree root' '
+	echo .git >expect &&
+	git rev-parse --git-common-dir >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'git-common-dir inside sub-dir' '
+	mkdir -p path/to/child &&
+	test_when_finished "rm -rf path" &&
+	echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
+	git -C path/to/child rev-parse --git-common-dir >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git-path from worktree root' '
+	echo .git/objects >expect &&
+	git rev-parse --git-path objects >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'git-path inside sub-dir' '
+	mkdir -p path/to/child &&
+	test_when_finished "rm -rf path" &&
+	echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
+	git -C path/to/child rev-parse --git-path objects >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8aef49f..8ca21bd 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,21 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_failure 'rev-parse --shared-index-path' '
+	rm -rf .git &&
+	test_create_repo . &&
+	git update-index --split-index &&
+	ls -t .git/sharedindex* | tail -n 1 >expect &&
+	git rev-parse --shared-index-path >actual &&
+	test_cmp expect actual &&
+	mkdir work &&
+	test_when_finished "rm -rf work" &&
+	(
+		cd work &&
+		ls -t ../.git/sharedindex* | tail -n 1 >expect &&
+		git rev-parse --shared-index-path >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 1b1b65a..53cc5d3 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,16 +8,24 @@ test_expect_success 'setup' '
 	test_commit init
 '
 
-test_expect_success 'rev-parse --git-common-dir on main worktree' '
+test_expect_failure 'rev-parse --git-common-dir on main worktree' '
 	git rev-parse --git-common-dir >actual &&
 	echo .git >expected &&
 	test_cmp expected actual &&
 	mkdir sub &&
 	git -C sub rev-parse --git-common-dir >actual2 &&
-	echo sub/.git >expected2 &&
+	echo ../.git >expected2 &&
 	test_cmp expected2 actual2
 '
 
+test_expect_failure 'rev-parse --git-path objects linked worktree' '
+	echo "$(git rev-parse --show-toplevel)/.git/worktrees/linked-tree/objects" >expect &&
+	test_when_finished "rm -rf linked-tree && git worktree prune" &&
+	git worktree add --detach linked-tree master &&
+	git -C linked-tree rev-parse --git-path objects >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '"list" all worktrees from main' '
 	echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
 	test_when_finished "rm -rf here && git worktree prune" &&
-- 
2.8.0

[PATCH v3 2/2] rev-parse: fix some options when executed from subpath of main tree

[Michael Rappazzo]

Executing `git-rev-parse` with `--git-common-dir`, `--git-path <path>`,
or `--shared-index-path` from the root of the main worktree results in
a relative path to the git dir.

When executed from a subdirectory of the main tree, it can incorrectly
return a path which starts 'sub/path/.git'.  Change this to return the
proper relative path to the git directory.

Related tests marked to expect failure are updated to expect success

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 builtin/rev-parse.c      | 19 ++++++++++++++-----
 t/t1500-rev-parse.sh     |  4 ++--
 t/t1700-split-index.sh   |  2 +-
 t/t2027-worktree-list.sh |  4 ++--
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c961b74..1da2e10 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -564,10 +564,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!strcmp(arg, "--git-path")) {
+			struct strbuf sb = STRBUF_INIT;
 			if (!argv[i + 1])
 				die("--git-path requires an argument");
-			puts(git_path("%s", argv[i + 1]));
-			i++;
+
+			puts(relative_path(xstrfmt("%s/%s", get_git_dir(), argv[++i]),
+				prefix, &sb));
+			strbuf_release(&sb);
 			continue;
 		}
 		if (as_is) {
@@ -787,8 +790,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--git-common-dir")) {
-				const char *pfx = prefix ? prefix : "";
-				puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
+				struct strbuf sb = STRBUF_INIT;
+				puts(relative_path(get_git_common_dir(), prefix, &sb));
+				strbuf_release(&sb);
 				continue;
 			}
 			if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -811,7 +815,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					die(_("Could not read the index"));
 				if (the_index.split_index) {
 					const unsigned char *sha1 = the_index.split_index->base_sha1;
-					puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
+					struct strbuf sb = STRBUF_INIT;
+
+					puts(relative_path(
+						xstrfmt("%s/sharedindex.%s", get_git_dir(), sha1_to_hex(sha1)),
+						prefix, &sb));
+					strbuf_release(&sb);
 				}
 				continue;
 			}
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 442ca46..cc89392 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -100,7 +100,7 @@ test_expect_success 'git-common-dir from worktree root' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'git-common-dir inside sub-dir' '
+test_expect_success 'git-common-dir inside sub-dir' '
 	mkdir -p path/to/child &&
 	test_when_finished "rm -rf path" &&
 	echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
@@ -114,7 +114,7 @@ test_expect_success 'git-path from worktree root' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'git-path inside sub-dir' '
+test_expect_success 'git-path inside sub-dir' '
 	mkdir -p path/to/child &&
 	test_when_finished "rm -rf path" &&
 	echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8ca21bd..d2d9e02 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,7 +200,7 @@ EOF
 	test_cmp expect actual
 '
 
-test_expect_failure 'rev-parse --shared-index-path' '
+test_expect_success 'rev-parse --shared-index-path' '
 	rm -rf .git &&
 	test_create_repo . &&
 	git update-index --split-index &&
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 53cc5d3..16eec6e 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,7 +8,7 @@ test_expect_success 'setup' '
 	test_commit init
 '
 
-test_expect_failure 'rev-parse --git-common-dir on main worktree' '
+test_expect_success 'rev-parse --git-common-dir on main worktree' '
 	git rev-parse --git-common-dir >actual &&
 	echo .git >expected &&
 	test_cmp expected actual &&
@@ -18,7 +18,7 @@ test_expect_failure 'rev-parse --git-common-dir on main worktree' '
 	test_cmp expected2 actual2
 '
 
-test_expect_failure 'rev-parse --git-path objects linked worktree' '
+test_expect_success 'rev-parse --git-path objects linked worktree' '
 	echo "$(git rev-parse --show-toplevel)/.git/worktrees/linked-tree/objects" >expect &&
 	test_when_finished "rm -rf linked-tree && git worktree prune" &&
 	git worktree add --detach linked-tree master &&
-- 
2.8.0

Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Junio C Hamano]

Michael Rappazzo <rappazzo@gmail.com> writes:

> t1500-rev-parse contains envrionment leaks (changing dir without
> changing back, setting config variables, etc).  Add a test to clean this
> up up so that future tests can be added without worry of any setting
> from a previous test.

This is a wonderful thing to do, but...

>  test_rev_parse toplevel false false true '' .git
>  
> @@ -84,4 +85,41 @@ test_rev_parse '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_expect_success 'cleanup from the previous tests' '
> +	cd .. &&
> +	rm -r work &&

Instead of cleaning things up like this, could you please please
please fix these existing tests that chdir around without being in a
subshell?  If the "previous tests" failed before going down as this
step expects, the "cd .. && rm -r" can make things worse.

> +	mv repo.git .git &&
> +	unset GIT_DIR &&
> +	unset GIT_CONFIG &&

The spirit of this change is to make the test more independent from
the effects of what happened previously.  Use sane_unset so that
we do not have to worry about previous step that may have failed
before it has a chance to set GIT_DIR and GIT_CONFIG (which would
cause these unset to fail).

> +	git config core.bare $original_core_bare

Is this (rather, the capturing of $original_core_bare up above)
necessary?  We are in the default 'trash' repository when the
capturing happens, and we know it is not a bare repository, right?

Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Mike Rappazzo]

On Fri, May 6, 2016 at 6:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Rappazzo <rappazzo@gmail.com> writes:
>
>> t1500-rev-parse contains envrionment leaks (changing dir without
>> changing back, setting config variables, etc).  Add a test to clean this
>> up up so that future tests can be added without worry of any setting
>> from a previous test.
>
> This is a wonderful thing to do, but...
>
>>  test_rev_parse toplevel false false true '' .git
>>
>> @@ -84,4 +85,41 @@ test_rev_parse '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_expect_success 'cleanup from the previous tests' '
>> +     cd .. &&
>> +     rm -r work &&
>
> Instead of cleaning things up like this, could you please please
> please fix these existing tests that chdir around without being in a
> subshell?  If the "previous tests" failed before going down as this
> step expects, the "cd .. && rm -r" can make things worse.

I still have fixing this test up on my do-to list.  My previous
attempt[1] had some flaws
in addition to some objection to the approach I took to expand each test. Eric
Sunshine suggested using a table approach, but I am not sure if that can be done
cleanly.

I figured that a fix to the rev-parse code would supersede test
cleanup, so I separated
my efforts.

I originally copied the pattern from above this code:

> +#cleanup from the above
> +cd ..
> +rm -r work
> +mv repo.git .git || exit 1

but Gábor had an objection to it [2].  So I went with this simple cleanup test.

I could move it back to outside of a test, and do some checks around
it.  Something
like:

    dir=$(pwd)
    target=${dir##*/}
    if [ "$target" == "work" ]
    then
        cd ..
        rm -r "work"
    fi

>
>> +     mv repo.git .git &&
>> +     unset GIT_DIR &&
>> +     unset GIT_CONFIG &&
>
> The spirit of this change is to make the test more independent from
> the effects of what happened previously.  Use sane_unset so that
> we do not have to worry about previous step that may have failed
> before it has a chance to set GIT_DIR and GIT_CONFIG (which would
> cause these unset to fail).
>
>> +     git config core.bare $original_core_bare
>
> Is this (rather, the capturing of $original_core_bare up above)
> necessary?  We are in the default 'trash' repository when the
> capturing happens, and we know it is not a bare repository, right?

My goal was to have the test be in the state exactly as it was at the
beginning of
the test.  Right above my cleanup test this line is executed:

> git config --unset core.bare

I just wanted to be absolutely sure that the value was the same.  I
could certainly
simplify it to assume core.bare is "true" though.

Thanks,
_Mike


[1] http://thread.gmane.org/gmane.comp.version-control.git/291729
[2] http://article.gmane.org/gmane.comp.version-control.git/293003

Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Junio C Hamano]

Mike Rappazzo <rappazzo@gmail.com> writes:

>> Instead of cleaning things up like this, could you please please
>> please fix these existing tests that chdir around without being in a
>> subshell?  If the "previous tests" failed before going down as this
>> step expects, the "cd .. && rm -r" can make things worse.

> I originally copied the pattern from above this code:
> ...
> but Gábor had an objection to it [2].  So I went with this simple cleanup test.

The "|| exit 1" you see everywhere is a sure sign that these tests
are not designed correctly.  These existing tests that do too many
things outside test_expect_success block needs to be redone, so that
each of them can do what it wants to test even when previous ones
failed.

What I had in mind was more a long the lines of this change. I only
did the first several just for illustration (I added an 'exit' to
mark where I stopped), but I think you get the idea.  The point is
that by doing things in subprocess inside test_expect_success you
can avoid disrupting the main test process even when a test fails as
much as possible, and this illustrates how you would deal with the
"cd" and "export".  What I didn't handle was the updates to
.git/config whose effects by earlier tests are relied on later tests
in this illustration.

As to "table driven" vs "explicitly spelling out scripts, accepting
some code repetition", I tend to favor the latter slightly, unless
the table driven approach is done in such a way that each of its
tests are truly independent from one another, i.e. if a row of the
table represents one test, the tests would not be disrupted by
reordering the rows, inserting a new row in the middle, or removing
an existing row.  From my experience, "table-driven" sets of tests
whose rows have inter-dependency has been much harder to debug when
I had to hunt down a bug that manifests itself only in one test in
the middle.

Hope this helps clarify what I meant.

 t/t1500-rev-parse.sh | 82 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 34 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..22b52c0 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -5,65 +5,79 @@ test_description='test git rev-parse'
 
 test_rev_parse() {
 	name=$1
-	shift
-
-	test_expect_success "$name: is-bare-repository" \
-	"test '$1' = \"\$(git rev-parse --is-bare-repository)\""
+	prep=$2
+	shift 2
+
+	test_expect_success "$name: is-bare-repository" "
+	(
+		$prep &&
+		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)\""
+	test_expect_success "$name: is-inside-git-dir" "
+	(
+		$prep &&
+		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)\""
+	test_expect_success "$name: is-inside-work-tree" "
+	(
+		$prep &&
+		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)\""
+	test_expect_success "$name: prefix" "
+	(
+		$prep &&
+		test '$1' = \"\$(git rev-parse --show-prefix)\"
+	)"
 	shift
 	[ $# -eq 0 ] && return
 
-	test_expect_success "$name: git-dir" \
-	"test '$1' = \"\$(git rev-parse --git-dir)\""
+	test_expect_success "$name: git-dir" "
+	(
+		$prep &&
+		test '$1' = \"\$(git rev-parse --git-dir)\"
+	)"
 	shift
 	[ $# -eq 0 ] && return
 }
 
-# label is-bare is-inside-git is-inside-work prefix git-dir
+# label prep is-bare is-inside-git is-inside-work prefix git-dir
 
 ROOT=$(pwd)
+mkdir -p sub/dir work
 
-test_rev_parse toplevel false false true '' .git
+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 .git 'cd .git' false true false '' .
 
-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_rev_parse .git/objects/ 'cd .git/objects' false true false '' "$ROOT/.git"
 
-git config core.bare true
-test_rev_parse 'core.bare = true' true false false
+test_rev_parse subdirectory 'cd sub/dir' \
+	false false true sub/dir/ "$ROOT/.git"
 
-git config --unset core.bare
-test_rev_parse 'core.bare undefined' false false true
+test_rev_parse 'core.bare = true' 'git config core.bare true' \
+	true false false
 
-mkdir work || exit 1
-cd work || exit 1
-GIT_DIR=../.git
-GIT_CONFIG="$(pwd)"/../.git/config
-export GIT_DIR GIT_CONFIG
+test_rev_parse 'core.bare undefined' 'git config --unset core.bare || :' \
+	false false true
 
-git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse 'GIT_DIR=../.git, core.bare = false' '
+	cd work && 
+	GIT_DIR=../.git &&
+	GIT_CONFIG="$(pwd)"/../.git/config &&
+	export GIT_DIR GIT_CONFIG &&
+	git config core.bare false' \
+	false false true ''
+
+exit
 
 git config core.bare true
 test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''

Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> What I had in mind was more a long the lines of this change. I only
> did the first several just for illustration (I added an 'exit' to
> mark where I stopped), but I think you get the idea.  The point is
> that by doing things in subprocess inside test_expect_success you
> can avoid disrupting the main test process even when a test fails as
> much as possible, and this illustrates how you would deal with the
> "cd" and "export".  What I didn't handle was the updates to
> .git/config whose effects by earlier tests are relied on later tests
> in this illustration.
>
> As to "table driven" vs "explicitly spelling out scripts, accepting
> some code repetition", I tend to favor the latter slightly, unless
> the table driven approach is done in such a way that each of its
> tests are truly independent from one another, i.e. if a row of the
> table represents one test, the tests would not be disrupted by
> reordering the rows, inserting a new row in the middle, or removing
> an existing row.  From my experience, "table-driven" sets of tests
> whose rows have inter-dependency has been much harder to debug when
> I had to hunt down a bug that manifests itself only in one test in
> the middle.
>
> Hope this helps clarify what I meant.


If you want to go forward in this direction, three are a few
additional things to note.

> +	test_expect_success "$name: is-bare-repository" "
> +	(
> +		$prep &&
> +		test '$1' = \"\$(git rev-parse --is-bare-repository)\"
> +	)"

This is not a new problem, but quoting the test body with dq instead
of sq pair make it ugly to read.  We might want to do something like

	expect=$1
	test_expect_success "$name: is-bare-repository" '
	(
		$prep &&
		test "$outcome" = "$(git rev-parse --is-bare-repository)"
	)'

>  	shift
>  	[ $# -eq 0 ] && return

Also, let's avoid []; i.e.

	test $# = 0 && return

> -# label is-bare is-inside-git is-inside-work prefix git-dir
> +# label prep is-bare is-inside-git is-inside-work prefix git-dir
>  
>  ROOT=$(pwd)
> +mkdir -p sub/dir work

Remember this place in the script, I may refer to it later.

> -git config core.bare true
> -test_rev_parse 'core.bare = true' true false false
> +test_rev_parse subdirectory 'cd sub/dir' \
> +	false false true sub/dir/ "$ROOT/.git"
>  
> -git config --unset core.bare
> -test_rev_parse 'core.bare undefined' false false true
> +test_rev_parse 'core.bare = true' 'git config core.bare true' \
> +	true false false

I didn't use "test_config" and "test_unconfig" here, because you
cannot use them in a subprocess.  If we truly want to make these
tests independent from each other, we'd need to come up with a way
for the tests to revert the configuration state back to where it was
before they started.
>  
> -mkdir work || exit 1
> -cd work || exit 1
> -GIT_DIR=../.git
> -GIT_CONFIG="$(pwd)"/../.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 'GIT_DIR=../.git, core.bare = false' '
> +	cd work && 
> +	GIT_DIR=../.git &&
> +	GIT_CONFIG="$(pwd)"/../.git/config &&
> +	export GIT_DIR GIT_CONFIG &&
> +	git config core.bare false' \
> +	false false true ''

The "prep" step for this test is long and I'd assume that the later
tests would need to do something similar themselves.  To avoid
repetition, create a helper shell function to reduce boilerplate
before the series of these tests start (i.e. the place above) and
call it from the prep parts with different arguments, e.g. making
the above test into

test_rev_parse 'GIT_DIR=../.git, core.bare = false' '
	cd work && 
	set_common_env ../.git false' \
	false false true ''

by having something like

set_dir_and_core_bare () {
	GIT_DIR=$1 &&
	GIT_CONFIG="$(pwd)/$GIT_DIR/config" &&
	export GIT_DIR GIT_CONFIG &&
	git config core.bare "$2"
}

> +exit
>  
>  git config core.bare true
>  test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''

Re: [PATCH v3 2/2] rev-parse: fix some options when executed from subpath of main tree

[Eric Sunshine]

On Fri, May 6, 2016 at 9:35 AM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> Executing `git-rev-parse` with `--git-common-dir`, `--git-path <path>`,
> or `--shared-index-path` from the root of the main worktree results in
> a relative path to the git dir.
>
> When executed from a subdirectory of the main tree, it can incorrectly
> return a path which starts 'sub/path/.git'.  Change this to return the
> proper relative path to the git directory.
>
> Related tests marked to expect failure are updated to expect success
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> @@ -564,10 +564,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>                 if (!strcmp(arg, "--git-path")) {
> +                       struct strbuf sb = STRBUF_INIT;
>                         if (!argv[i + 1])
>                                 die("--git-path requires an argument");
> -                       puts(git_path("%s", argv[i + 1]));
> -                       i++;
> +
> +                       puts(relative_path(xstrfmt("%s/%s", get_git_dir(), argv[++i]),
> +                               prefix, &sb));

This is leaking the result of xstrfmt().

> +                       strbuf_release(&sb);
>                         continue;
>                 }
> @@ -811,7 +815,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>                                 if (the_index.split_index) {
>                                         const unsigned char *sha1 = the_index.split_index->base_sha1;
> -                                       puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
> +                                       struct strbuf sb = STRBUF_INIT;
> +
> +                                       puts(relative_path(
> +                                               xstrfmt("%s/sharedindex.%s", get_git_dir(), sha1_to_hex(sha1)),
> +                                               prefix, &sb));

Likewise leaking xstrfmt().

> +                                       strbuf_release(&sb);
>                                 }
>                                 continue;
>                         }

[PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Johannes Schindelin]

From: Michael Rappazzo <rappazzo@gmail.com>

t2027-worktree-list has an incorrect expectation for --git-common-dir
which has been adjusted and marked to expect failure.

Some of the tests added have been marked to expect failure.  These
demonstrate a problem with the way that some options to git rev-parse
behave when executed from a subdirectory of the main worktree.

[jes: fixed incorrect assumption that objects/ lives in the
worktree-specific git-dir (it lives in the common dir instead). Also
adjusted t1700 so that the test case does not *need* to be the last
one in that script.]

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1500-rev-parse.sh     | 28 ++++++++++++++++++++++++++++
 t/t1700-split-index.sh   | 16 ++++++++++++++++
 t/t2027-worktree-list.sh | 12 ++++++++++--
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 038e24c4014..f39f783f2db 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru
 
 test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
 
+test_expect_success 'git-common-dir from worktree root' '
+	echo .git >expect &&
+	git rev-parse --git-common-dir >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'git-common-dir inside sub-dir' '
+	mkdir -p path/to/child &&
+	test_when_finished "rm -rf path" &&
+	echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
+	git -C path/to/child rev-parse --git-common-dir >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git-path from worktree root' '
+	echo .git/objects >expect &&
+	git rev-parse --git-path objects >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'git-path inside sub-dir' '
+	mkdir -p path/to/child &&
+	test_when_finished "rm -rf path" &&
+	echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
+	git -C path/to/child rev-parse --git-path objects >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 292a0720fcc..b754865a618 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,20 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_failure 'rev-parse --shared-index-path' '
+	test_create_repo split-index &&
+	(
+		cd split-index &&
+		git update-index --split-index &&
+		echo .git/sharedindex* >expect &&
+		git rev-parse --shared-index-path >actual &&
+		test_cmp expect actual &&
+		mkdir subdirectory &&
+		cd subdirectory &&
+		echo ../.git/sharedindex* >expect &&
+		git rev-parse --shared-index-path >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 465eeeacd3d..c1a072348e7 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,16 +8,24 @@ test_expect_success 'setup' '
 	test_commit init
 '
 
-test_expect_success 'rev-parse --git-common-dir on main worktree' '
+test_expect_failure 'rev-parse --git-common-dir on main worktree' '
 	git rev-parse --git-common-dir >actual &&
 	echo .git >expected &&
 	test_cmp expected actual &&
 	mkdir sub &&
 	git -C sub rev-parse --git-common-dir >actual2 &&
-	echo sub/.git >expected2 &&
+	echo ../.git >expected2 &&
 	test_cmp expected2 actual2
 '
 
+test_expect_failure 'rev-parse --git-path objects linked worktree' '
+	echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
+	test_when_finished "rm -rf linked-tree && git worktree prune" &&
+	git worktree add --detach linked-tree master &&
+	git -C linked-tree rev-parse --git-path objects >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '"list" all worktrees from main' '
 	echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
 	test_when_finished "rm -rf here && git worktree prune" &&
-- 
2.11.1.windows.1.2.g87ad093.dirty