[PATCH v2 0/4] rev-parse: adjust results when they should be relative

Differences from v1[1]:
 - Simplify implementation based on review comments
 - Included similar changes in rev-parse for --git-path and --shared-index-path
 - Added tests and separated them into individual commits

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

Michael Rappazzo (4):
  rev-parse: fix some options when executed from subpath of main tree
  t1500-rev-parse: add tests executed from sub path of the main worktree
  t2027-worktree-list: add and adjust tests related to git-rev-parse
  t1700-split-index: add test for rev-parse --shared-index-path

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

-- 
2.8.0

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, however, it incorrectly
returns a path which starts 'sub/path/.git'.  Change this to return the
proper relative path to the git directory.

Signed-off-by: Michael Rappazzo <redacted>
---
 builtin/rev-parse.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 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;
 			}

-- 
2.8.0

Adjust the incorrect expectation for `rev-parse --git-common-dir`.

Add a test for `git rev-parse --git-path` executed from a linked
worktree.

Signed-off-by: Michael Rappazzo <redacted>
---
 t/t2027-worktree-list.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 1b1b65a..16eec6e 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh

@@ -14,10 +14,18 @@ test_expect_success 'rev-parse --git-common-dir on main worktree' '
 	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_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 &&
+	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

Signed-off-by: Michael Rappazzo <redacted>
---
 t/t1700-split-index.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8aef49f..d2d9e02 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh

@@ -200,4 +200,21 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_success '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

-- 
2.8.0

Signed-off-by: Michael Rappazzo <redacted>
---
 t/t1500-rev-parse.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..1e220f7 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,40 @@ 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 ''
 
+#cleanup from the above
+cd ..
+rm -r work
+mv repo.git .git || exit 1
+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_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 &&
+	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_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 &&
+	git -C path/to/child rev-parse --git-path objects >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.8.0

[Resend to list, sorry for the duplicates.]

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, however, it incorrectly
returns a path which starts 'sub/path/.git'.

This is not completely true, because '--git-path ...' returns a
relative path starting with '.git':

  $ git -C t/ rev-parse --git-dir --git-path objects --git-common-dir
  /home/szeder/src/git/.git
  .git/objects
  t/.git

It's still wrong, of course.

Change this to return the
proper relative path to the git directory.

I think returning absolute paths would be better.  It is consistent
with the already properly working '--git-dir' option, which returns an
absolute path in this case.  Furthermore, both '--git-path ...' and
'--git-common-dir' already return absolute paths when run from a
subdirectory of the .git directory:

  $ git -C .git/refs rev-parse --git-dir --git-path objects --git-common-dir
  /home/szeder/src/git/.git
  /home/szeder/src/git/.git/objects
  /home/szeder/src/git/.git

Signed-off-by: Michael Rappazzo <redacted>
---
 builtin/rev-parse.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

This patch doesn't add any new tests, while subsequent patches of the
series do nothing but add more tests.  Splitting up your changes this
way doesn't add any value, it only increases the number of commits.  I
think either:

  - all those new tests could be added with this patch, or

  - if you want to add the test separately, then add them before
    this patch and mark them with 'test_expect_failure' to clearly
    demonstrate what the series is about to fix, and flip them to
    'test_expect_success' in this patch.

  - An alternative way to split this series, following the "Make
    separate commits for logically separate changes" guideline, would
    be to fix and test these options in separate patches, i.e. fix and
    test '--git-path ...' in one patch, then fix and test
    '--git-common-dir' in the next, ...

[Resend to list, sorry for the duplicates.]

You can't just 'exit 1' mid-script, because terminating the test script
abruptly makes the test harness unhappy.

+unset GIT_DIR
+unset GIT_CONFIG

Both variables are set at this point, so calling plain 'unset' is OK.
Still, I would suggest using 'sane_unset' instead, so the next person
looking at this test doesn't have to spend brain cycles on figuring
out whether plain 'unset' is indeed safe or not.

+git config core.bare $original_core_bare

This whole '#cleanup from the above' block is just ugly.  Not your
fault, of course, but the consequence of how the preceeding tests were
written in the past.  I think it would be best if this series were
scheduled on top of the 't1500 cleanup & modernization' patch I saw a
few days ago, then this block wouldn't be necessary at all.

+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_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 &&
+	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_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 &&
+	git -C path/to/child rev-parse --git-path objects >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.8.0

[Resend to list, sorry for the duplicates...]

This hunk must go into patch 1/4.

The full test suite should pass after every single commit.  As patch
1/4 fixes things, the changing behavior makes this test case fail,
resulting in two commits in which 'make test' would fail.

+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 &&
+	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