Quantcast

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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <[hidden email]> 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 <[hidden email]>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Duy Nguyen
In reply to this post by Michael Rappazzo
On Mon, Apr 4, 2016 at 8:42 AM, Michael Rappazzo <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Michael Rappazzo
On Fri, Apr 8, 2016 at 7:47 AM, Duy Nguyen <[hidden email]> wrote:

> On Mon, Apr 4, 2016 at 8:42 AM, Michael Rappazzo <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mike Hommey-3
On Fri, Apr 08, 2016 at 08:35:51AM -0400, Mike Rappazzo wrote:

> On Fri, Apr 8, 2016 at 7:47 AM, Duy Nguyen <[hidden email]> wrote:
> > On Mon, Apr 4, 2016 at 8:42 AM, Michael Rappazzo <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mike Hommey-3
In reply to this post by Michael Rappazzo
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Michael Rappazzo
In reply to this post by Mike Hommey-3
On Thu, May 19, 2016 at 3:49 AM, Mike Hommey <[hidden email]> 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 <[hidden email]> wrote:
>> > On Mon, Apr 4, 2016 at 8:42 AM, Michael Rappazzo <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Loading...