[PATCH] rev-parse: fix --git-common-dir when executed from subpath of main tree

[PATCH] rev-parse: fix --git-common-dir when executed from subpath of main tree

[Michael Rappazzo]

Executing `git-rev-parse --git-common-dir` from the root of the main
worktree results in '.git', which is the relative path to the git dir.
When executed from a subpath of the main tree it returned somthing like:
'sub/path/.git'.  Change this to return the proper relative path to the
git directory (similar to `--show-cdup`).

Add as test to t1500-rev-parse.sh for this case and adjust another test
in t2027-worktree-list.sh to use this expectation.

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

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c961b74..c2918e1 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -787,8 +787,18 @@ 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()));
+				const char *git_common_dir = get_git_common_dir();
+				if (prefix && !is_absolute_path(git_common_dir)) {
+					const char *pfx = prefix;
+					while (pfx) {
+						pfx = strchr(pfx, '/');
+						if (pfx) {
+							pfx++;
+							printf("../");
+						}
+					}
+				}
+				printf("%s\n", git_common_dir);
 				continue;
 			}
 			if (!strcmp(arg, "--is-inside-git-dir")) {
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..2023208 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,6 +3,16 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
+test_expect_success 'git-common-dir inside sub-dir' '
+   (
+		mkdir -p path/to/child &&
+		cd path/to/child &&
+		echo "$(git rev-parse --show-cdup).git" >expect &&
+		git rev-parse --git-common-dir >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_rev_parse() {
 	name=$1
 	shift
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 1b1b65a..3780b14 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -14,7 +14,7 @@ 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
 '
 
-- 
2.8.0

Re: [PATCH] rev-parse: fix --git-common-dir when executed from subpath of main tree

[Eric Sunshine]

On Sun, Apr 3, 2016 at 9:42 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> Executing `git-rev-parse --git-common-dir` from the root of the main
> worktree results in '.git', which is the relative path to the git dir.
> When executed from a subpath of the main tree it returned somthing like:

s/returned/returns/
s/somthing/something/

It would be clearer if you stated explicitly that the subpath result
is incorrect. For instance:

    When executed from a subdirectory of the main tree,
    however, it incorrectly returns:

More below...

> 'sub/path/.git'.  Change this to return the proper relative path to the
> git directory (similar to `--show-cdup`).
>
> Add as test to t1500-rev-parse.sh for this case and adjust another test
> in t2027-worktree-list.sh to use this expectation.
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> @@ -787,8 +787,18 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>                         if (!strcmp(arg, "--git-common-dir")) {
> -                               const char *pfx = prefix ? prefix : "";
> -                               puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
> +                               const char *git_common_dir = get_git_common_dir();
> +                               if (prefix && !is_absolute_path(git_common_dir)) {
> +                                       const char *pfx = prefix;
> +                                       while (pfx) {
> +                                               pfx = strchr(pfx, '/');
> +                                               if (pfx) {
> +                                                       pfx++;
> +                                                       printf("../");
> +                                               }
> +                                       }
> +                               }
> +                               printf("%s\n", git_common_dir);

How about simplifying this entire chunk of code to?

    struct strbuf sb = STRBUF_INIT;
    puts(relative_path(get_git_common_dir(), prefix, &sb));
    strbuf_release(&sb);

No need to check NULL prefix or absolute common dir.

>                                 continue;
>                         }
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> @@ -3,6 +3,16 @@
>  test_description='test git rev-parse'
>  . ./test-lib.sh
>
> +test_expect_success 'git-common-dir inside sub-dir' '
> +   (

Strange indentation. Use TAB rather than spaces.

> +               mkdir -p path/to/child &&

Nit: Although functionally the same, typically 'mkdir' is outside the subshell.

> +               cd path/to/child &&
> +               echo "$(git rev-parse --show-cdup).git" >expect &&
> +               git rev-parse --git-common-dir >actual &&
> +               test_cmp expect actual
> +       )
> +'

I guess you added this new test to the top of the script rather than
the bottom (as is more customary) because this script is ancient and
cd's all around the place with wild abandon and leaks environment
variables; thus you avoided having to prefix this new test with:

    cd .. || exit 1
    sane_unset GIT_DIR GIT_CONFIG

which would have been needed had it been added to at the bottom of the script.

It probably wouldn't hurt to add tests to also verify correct behavior
at the root of the main tree, as well as at the root and within a
subdirectory of a linked worktree (though the latter two tests would
probably go in a worktree-related test script).

Thanks.

Re: [PATCH] rev-parse: fix --git-common-dir when executed from subpath of main tree

[Duy Nguyen]

On Mon, Apr 4, 2016 at 8:42 AM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> Executing `git-rev-parse --git-common-dir` from the root of the main
> worktree results in '.git', which is the relative path to the git dir.
> When executed from a subpath of the main tree it returned somthing like:
> 'sub/path/.git'.  Change this to return the proper relative path to the
> git directory (similar to `--show-cdup`).

I faced a similar problem just a couple days ago, I expected "git
rev-parse --git-path" to return a path relative to cwd too, but it
returned relative to git dir. The same solution (or Eric's, which is
cleaner in my opinion) applies. --shared-index-path also does
puts(git_path(... and has the same problem. Do you want to fix them
too?
-- 
Duy

Re: [PATCH] rev-parse: fix --git-common-dir when executed from subpath of main tree

[Mike Rappazzo]

On Fri, Apr 8, 2016 at 7:47 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, Apr 4, 2016 at 8:42 AM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>> Executing `git-rev-parse --git-common-dir` from the root of the main
>> worktree results in '.git', which is the relative path to the git dir.
>> When executed from a subpath of the main tree it returned somthing like:
>> 'sub/path/.git'.  Change this to return the proper relative path to the
>> git directory (similar to `--show-cdup`).
>
> I faced a similar problem just a couple days ago, I expected "git
> rev-parse --git-path" to return a path relative to cwd too, but it
> returned relative to git dir. The same solution (or Eric's, which is
> cleaner in my opinion) applies. --shared-index-path also does
> puts(git_path(... and has the same problem. Do you want to fix them
> too?

Sure, I can do that.  I will try to get it up soon.

Re: [PATCH] rev-parse: fix --git-common-dir when executed from subpath of main tree

[Mike Hommey]

On Fri, Apr 08, 2016 at 08:35:51AM -0400, Mike Rappazzo wrote:
> On Fri, Apr 8, 2016 at 7:47 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> > On Mon, Apr 4, 2016 at 8:42 AM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> >> Executing `git-rev-parse --git-common-dir` from the root of the main
> >> worktree results in '.git', which is the relative path to the git dir.
> >> When executed from a subpath of the main tree it returned somthing like:
> >> 'sub/path/.git'.  Change this to return the proper relative path to the
> >> git directory (similar to `--show-cdup`).
> >
> > I faced a similar problem just a couple days ago, I expected "git
> > rev-parse --git-path" to return a path relative to cwd too, but it
> > returned relative to git dir. The same solution (or Eric's, which is
> > cleaner in my opinion) applies. --shared-index-path also does
> > puts(git_path(... and has the same problem. Do you want to fix them
> > too?
> 
> Sure, I can do that.  I will try to get it up soon.

If I'm not mistaken, it looks like this fell off your radar. (I haven't
seen an updated patch, and it doesn't look like the fix made it to any
git branch). Would you mind updating?

Cheers,

Mike

Re: [PATCH] rev-parse: fix --git-common-dir when executed from subpath of main tree

[Mike Hommey]

On Sun, Apr 03, 2016 at 09:42:23PM -0400, Michael Rappazzo wrote:
> Executing `git-rev-parse --git-common-dir` from the root of the main
> worktree results in '.git', which is the relative path to the git dir.
> When executed from a subpath of the main tree it returned somthing like:
> 'sub/path/.git'.  Change this to return the proper relative path to the
> git directory (similar to `--show-cdup`).

Note that `git rev-parse --git-dir` returns an absolute path, not a
relative one. Shouldn't --git-common-dir "simply" return the same as
--git-dir when not in a worktree?

Mike

Re: [PATCH] rev-parse: fix --git-common-dir when executed from subpath of main tree

[Mike Rappazzo]

On Thu, May 19, 2016 at 3:49 AM, Mike Hommey <mh@glandium.org> wrote:
> On Fri, Apr 08, 2016 at 08:35:51AM -0400, Mike Rappazzo wrote:
>> On Fri, Apr 8, 2016 at 7:47 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> > On Mon, Apr 4, 2016 at 8:42 AM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>> >> Executing `git-rev-parse --git-common-dir` from the root of the main
>> >> worktree results in '.git', which is the relative path to the git dir.
>> >> When executed from a subpath of the main tree it returned somthing like:
>> >> 'sub/path/.git'.  Change this to return the proper relative path to the
>> >> git directory (similar to `--show-cdup`).
>> >
>> > I faced a similar problem just a couple days ago, I expected "git
>> > rev-parse --git-path" to return a path relative to cwd too, but it
>> > returned relative to git dir. The same solution (or Eric's, which is
>> > cleaner in my opinion) applies. --shared-index-path also does
>> > puts(git_path(... and has the same problem. Do you want to fix them
>> > too?
>>
>> Sure, I can do that.  I will try to get it up soon.
>
> If I'm not mistaken, it looks like this fell off your radar. (I haven't
> seen an updated patch, and it doesn't look like the fix made it to any
> git branch). Would you mind updating?
>
> Cheers,
>
> Mike

There is a newer version submitted on May 6th[1].  Eric Sunshine has
submitted a patch [2] which fixes
up t1500.  It looks like that is in a stable form, so I will rebase my
v2 onto those changes, and resubmit
in the near future.

[1] http://thread.gmane.org/gmane.comp.version-control.git/293778
[2] http://thread.gmane.org/gmane.comp.version-control.git/294999