All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] rev-parse: adjust results when they should be relative
@ 2016-04-22 21:53 Michael Rappazzo
  2016-04-22 21:53 ` [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree Michael Rappazzo
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Michael Rappazzo @ 2016-04-22 21:53 UTC (permalink / raw)
  To: git; +Cc: gitster, sunshine, pclouds, Michael Rappazzo

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree
  2016-04-22 21:53 [PATCH v2 0/4] rev-parse: adjust results when they should be relative Michael Rappazzo
@ 2016-04-22 21:53 ` Michael Rappazzo
  2016-04-29 14:21   ` SZEDER Gábor
       [not found]   ` <20160429135051.15492-1-szeder@ira.uka.de>
  2016-04-22 21:53 ` [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree Michael Rappazzo
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 12+ messages in thread
From: Michael Rappazzo @ 2016-04-22 21:53 UTC (permalink / raw)
  To: git; +Cc: gitster, sunshine, pclouds, 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, 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 <rappazzo@gmail.com>
---
 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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree
  2016-04-22 21:53 [PATCH v2 0/4] rev-parse: adjust results when they should be relative Michael Rappazzo
  2016-04-22 21:53 ` [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree Michael Rappazzo
@ 2016-04-22 21:53 ` Michael Rappazzo
  2016-04-29 14:22   ` SZEDER Gábor
  2016-04-22 21:53 ` [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse Michael Rappazzo
  2016-04-22 21:53 ` [PATCH v2 4/4] t1700-split-index: add test for rev-parse --shared-index-path Michael Rappazzo
  3 siblings, 1 reply; 12+ messages in thread
From: Michael Rappazzo @ 2016-04-22 21:53 UTC (permalink / raw)
  To: git; +Cc: gitster, sunshine, pclouds, Michael Rappazzo

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse
  2016-04-22 21:53 [PATCH v2 0/4] rev-parse: adjust results when they should be relative Michael Rappazzo
  2016-04-22 21:53 ` [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree Michael Rappazzo
  2016-04-22 21:53 ` [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree Michael Rappazzo
@ 2016-04-22 21:53 ` Michael Rappazzo
  2016-04-29 14:22   ` SZEDER Gábor
  2016-04-22 21:53 ` [PATCH v2 4/4] t1700-split-index: add test for rev-parse --shared-index-path Michael Rappazzo
  3 siblings, 1 reply; 12+ messages in thread
From: Michael Rappazzo @ 2016-04-22 21:53 UTC (permalink / raw)
  To: git; +Cc: gitster, sunshine, pclouds, Michael Rappazzo

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 <rappazzo@gmail.com>
---
 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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/4] t1700-split-index: add test for rev-parse --shared-index-path
  2016-04-22 21:53 [PATCH v2 0/4] rev-parse: adjust results when they should be relative Michael Rappazzo
                   ` (2 preceding siblings ...)
  2016-04-22 21:53 ` [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse Michael Rappazzo
@ 2016-04-22 21:53 ` Michael Rappazzo
  3 siblings, 0 replies; 12+ messages in thread
From: Michael Rappazzo @ 2016-04-22 21:53 UTC (permalink / raw)
  To: git; +Cc: gitster, sunshine, pclouds, Michael Rappazzo

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree
  2016-04-22 21:53 ` [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree Michael Rappazzo
@ 2016-04-29 14:21   ` SZEDER Gábor
       [not found]   ` <20160429135051.15492-1-szeder@ira.uka.de>
  1 sibling, 0 replies; 12+ messages in thread
From: SZEDER Gábor @ 2016-04-29 14:21 UTC (permalink / raw)
  To: Michael Rappazzo; +Cc: SZEDER Gábor, gitster, sunshine, pclouds, git

[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 <rappazzo@gmail.com>
> ---
>  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, ...

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree
  2016-04-22 21:53 ` [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree Michael Rappazzo
@ 2016-04-29 14:22   ` SZEDER Gábor
  0 siblings, 0 replies; 12+ messages in thread
From: SZEDER Gábor @ 2016-04-29 14:22 UTC (permalink / raw)
  To: Michael Rappazzo; +Cc: SZEDER Gábor, gitster, sunshine, pclouds, git

[Resend to list, sorry for the duplicates.]

> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
>  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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse
  2016-04-22 21:53 ` [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse Michael Rappazzo
@ 2016-04-29 14:22   ` SZEDER Gábor
  0 siblings, 0 replies; 12+ messages in thread
From: SZEDER Gábor @ 2016-04-29 14:22 UTC (permalink / raw)
  To: Michael Rappazzo; +Cc: SZEDER Gábor, gitster, sunshine, pclouds, git

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

> 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 <rappazzo@gmail.com>
> ---
>  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
>  '

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree
       [not found]   ` <20160429135051.15492-1-szeder@ira.uka.de>
@ 2016-05-06 13:02     ` Mike Rappazzo
  2016-05-06 14:13       ` SZEDER Gábor
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Rappazzo @ 2016-05-06 13:02 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Junio C Hamano, Eric Sunshine, Nguyễn Thái Ngọc,
	Git List

On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor <szeder@ira.uka.de> 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, 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.

I'll try to reword this to make it indicate that the value isn't
always incorrect.

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

I agree with this, but at one point Junio suggested that it should
return the relative path[1],
so I went with that.  Maybe I could RFC a separate patch that changes
all of these to
absolute.

>> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
>> ---
>>  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, ...
>
>

Thanks, have a re-roll ready to go based on you input.  I went with
option 2 - add
tests with expect failure and fix them with the next.

[1] http://article.gmane.org/gmane.comp.version-control.git/290061

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree
  2016-05-06 13:02     ` Mike Rappazzo
@ 2016-05-06 14:13       ` SZEDER Gábor
  2016-05-06 17:08         ` Junio C Hamano
  2016-05-10  7:49         ` Eric Sunshine
  0 siblings, 2 replies; 12+ messages in thread
From: SZEDER Gábor @ 2016-05-06 14:13 UTC (permalink / raw)
  To: Mike Rappazzo
  Cc: Junio C Hamano, Eric Sunshine, Nguyễn Thái Ngọc,
	Git List


Quoting Mike Rappazzo <rappazzo@gmail.com>:

> On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor <szeder@ira.uka.de> 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, 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.
>
> I'll try to reword this to make it indicate that the value isn't
> always incorrect.

Not sure I understand your intention about rewording, in particular that
"isn't always incorrect" part.  Just to make sure there is no
misunderstanding let's have a look at the two broken cases:

    $ git -C t/ rev-parse --git-common-dir
    t/.git

Obviously wrong.
This is what you correctly described in the commit message as
"incorrectly returns a path which starts 'sub/path/.git'.

    $ git -C t/ rev-parse --git-path objects
    .git/objects

Wrong as well.  It would be correct if we were at the top of the working
tree, but we are not.  It must be relative to the directory where '-C t/'
brought us.
Your description in the commit message implies that '--git-path <path>'
is wrong in the same way as '--git-common-dir' is, i.e. in this case
would also return the relative path starting with the subdirectory.
That is apparently not the case.

My point in the previous email was that both are wrong when executed in
a subdir of the working tree, but they are wrong in different ways.  And
they are always wrong when executed from a subdir of the working tree.
I would have described the current wrong behavior as:

    When executed from a subdirectory of the working tree, however,
    '--git-common-dir' incorrectly returns a path which starts
    'sub/path/.git', while '--git-path <path>' incorrectly returns a path
    relative to the top of the working tree.

(Still hasn't looked at shared index...)

>>> 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
>>
>
> I agree with this, but at one point Junio suggested that it should
> return the relative path[1],

I wasn't aware of Junio's suggestion.

It shouldn't really matter in practice, because both the absolute and
relative paths will ultimately lead to the same place.  However, I
still think that for consistency's sake absolute paths would be better
when executed in a subdir of the working tree.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree
  2016-05-06 14:13       ` SZEDER Gábor
@ 2016-05-06 17:08         ` Junio C Hamano
  2016-05-10  7:49         ` Eric Sunshine
  1 sibling, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2016-05-06 17:08 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Mike Rappazzo, Eric Sunshine, Nguyễn Thái Ngọc,
	Git List

SZEDER Gábor <szeder@ira.uka.de> writes:

>> I agree with this, but at one point Junio suggested that it should
>> return the relative path[1],
>
> I wasn't aware of Junio's suggestion.

Mike justified his position to change everything to absolute, citing
that it is what "rev-parse --show-toplevel" does when run from a
subdirectory.  I just gave a counter-example that rev-parse does not
always talk in absolute with "rev-parse -C t --show-cdup".

It hinted that his justification may not be strong enough to choose
absolute over relative, but wasn't meant to be a suggestion to
choose relative at all.  At most, I was saying "either is equally
plausible".

> It shouldn't really matter in practice, because both the absolute and
> relative paths will ultimately lead to the same place.  However, I
> still think that for consistency's sake absolute paths would be better
> when executed in a subdir of the working tree.

Sure.

One thing I'd be worried about is that people may be using it to
compute paths and store them in $GIT_DIR/somewhere, and expecting
that they can later do a wholesale "mv" of directory hierarchy and
relative paths still work.  I wouldn't be one of those who does the
"mv" so I personally do not care, but I suspect some do.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree
  2016-05-06 14:13       ` SZEDER Gábor
  2016-05-06 17:08         ` Junio C Hamano
@ 2016-05-10  7:49         ` Eric Sunshine
  1 sibling, 0 replies; 12+ messages in thread
From: Eric Sunshine @ 2016-05-10  7:49 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Mike Rappazzo, Junio C Hamano, Nguyễn Thái Ngọc,
	Git List

On Fri, May 6, 2016 at 10:13 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Quoting Mike Rappazzo <rappazzo@gmail.com>:
>> I'll try to reword this to make it indicate that the value isn't
>> always incorrect.
>
> Not sure I understand your intention about rewording, in particular that
> "isn't always incorrect" part.  Just to make sure there is no
> misunderstanding let's have a look at the two broken cases:
>
>    $ git -C t/ rev-parse --git-common-dir
>    t/.git
>
> Obviously wrong.
> This is what you correctly described in the commit message as
> "incorrectly returns a path which starts 'sub/path/.git'.
>
>    $ git -C t/ rev-parse --git-path objects
>    .git/objects
>
> Wrong as well.  It would be correct if we were at the top of the working
> tree, but we are not.  It must be relative to the directory where '-C t/'
> brought us.
> Your description in the commit message implies that '--git-path <path>'
> is wrong in the same way as '--git-common-dir' is, i.e. in this case
> would also return the relative path starting with the subdirectory.
> That is apparently not the case.
>
> My point in the previous email was that both are wrong when executed in
> a subdir of the working tree, but they are wrong in different ways.  And
> they are always wrong when executed from a subdir of the working tree.

This explanation argues strongly in favor of placing each fix in a
separate patch[1] so that each commit message can be tailored to
precisely match the problem being fixed. Bundling the corresponding
tests with the fix would be a bonus; no need for a lead-in patch
introducing the tests en masse, and no need for test_expect_failure.

[1]: http://article.gmane.org/gmane.comp.version-control.git/293001/

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-05-10  7:49 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-22 21:53 [PATCH v2 0/4] rev-parse: adjust results when they should be relative Michael Rappazzo
2016-04-22 21:53 ` [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree Michael Rappazzo
2016-04-29 14:21   ` SZEDER Gábor
     [not found]   ` <20160429135051.15492-1-szeder@ira.uka.de>
2016-05-06 13:02     ` Mike Rappazzo
2016-05-06 14:13       ` SZEDER Gábor
2016-05-06 17:08         ` Junio C Hamano
2016-05-10  7:49         ` Eric Sunshine
2016-04-22 21:53 ` [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree Michael Rappazzo
2016-04-29 14:22   ` SZEDER Gábor
2016-04-22 21:53 ` [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse Michael Rappazzo
2016-04-29 14:22   ` SZEDER Gábor
2016-04-22 21:53 ` [PATCH v2 4/4] t1700-split-index: add test for rev-parse --shared-index-path Michael Rappazzo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.