[PATCH] Change git-rev-parse --show-cdup to output an absolute path

[PATCH] Change git-rev-parse --show-cdup to output an absolute path

[koreth]

Add a test case for deep symlinking into a repo, which breaks with the
old relative output of --show-cdup.

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---

I have a symlink pointing to a subdirectory of a repo of mine. If I cd
to that symlink, some shell-script git commands don't work.  Shell scripts
use the PWD environment variable as the working directory, and the chain
of "../"s from git-rev-parse --show-cdup just peels entries off that
variable. That's fine when PWD is a real path, but not so great when it's
a reference to a symlink from an outside directory.

A typical failure case:

$ git clone git://whatever.git foobar
$ ln -s foobar/src/tools/misc/myapp myapp
$ cd myapp
$ git pull
fatal: Not a git repository

This patch adds a test for the problem (fails in the current git build)
and a proposed fix: making git-rev-parse --show-cdup output an absolute
path. It's possible that'll break something somewhere, but all the tests
in the test suite pass, so I think the breakage can't be *too* severe if
there's any at all.

 builtin-rev-parse.c       |   18 +++++++++---------
 t/t6102-rev-parse-cdup.sh |   24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 9 deletions(-)
 create mode 100755 t/t6102-rev-parse-cdup.sh

diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 37addb2..99a95eb 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -320,15 +320,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--show-cdup")) {
-				const char *pfx = prefix;
-				while (pfx) {
-					pfx = strchr(pfx, '/');
-					if (pfx) {
-						pfx++;
-						printf("../");
-					}
-				}
-				putchar('\n');
+				char cwd[PATH_MAX];
+				/*
+				 * The prefix computation does chdir("..") until it
+				 * reaches the top level, so the current directory
+				 * is actually the thing we want to output.
+				 */
+				if (!getcwd(cwd, PATH_MAX))
+					die("unable to get current working directory");
+				printf("%s/\n", cwd);
 				continue;
 			}
 			if (!strcmp(arg, "--git-dir")) {
diff --git a/t/t6102-rev-parse-cdup.sh b/t/t6102-rev-parse-cdup.sh
new file mode 100755
index 0000000..233d9cf
--- /dev/null
+++ b/t/t6102-rev-parse-cdup.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Steven Grimm
+#
+
+test_description='Test git-rev-parse --show-cdup'
+
+. ./test-lib.sh
+. ../t6000lib.sh # t6xxx specific functions
+
+# Make a subdirectory and a symbolic link pointing to it from outside the repo.
+mkdir subdir
+ln -s `pwd`/subdir /tmp/git-test.$$
+cd /tmp/git-test.$$
+
+test_expect_success 'shell pwd references symlink' "pwd | grep git-test.$$"
+test_expect_success 'cdup goes to top level of repo' '
+	cd `git-rev-parse --show-cdup` &&
+	test -d .git'
+
+rm /tmp/git-test.$$
+
+test_done
+
-- 
1.5.2.rc0.1.g2cc31

Re: [PATCH] Change git-rev-parse --show-cdup to output an absolute path

[Alex Riesen]

On 4/26/07, koreth@midwinter.com <koreth@midwinter.com> wrote:
> I have a symlink pointing to a subdirectory of a repo of mine. If I cd
> to that symlink, some shell-script git commands don't work.  Shell scripts
> use the PWD environment variable as the working directory, and the chain
> of "../"s from git-rev-parse --show-cdup just peels entries off that
> variable. That's fine when PWD is a real path, but not so great when it's
> a reference to a symlink from an outside directory.

Your implementation will fail if cwd is longer than PATH_MAX.
Does not happen often, though.

> A typical failure case:
>
> $ git clone git://whatever.git foobar
> $ ln -s foobar/src/tools/misc/myapp myapp
> $ cd myapp

Which is a strange thing to do. What is that for?
myapp is kind of outside the git repo foobar.

Re: [PATCH] Change git-rev-parse --show-cdup to output an absolute path

[Steven Grimm]

Alex Riesen wrote:
> Your implementation will fail if cwd is longer than PATH_MAX.
> Does not happen often, though.

That limitation is already littered through the code, e.g. in setup.c 
which would already be failing in the existing implementation. Actually 
setup.c goes one better: is_inside_git_dir() hardwires a 1024-character 
limit into the code rather than using PATH_MAX. I didn't think I was 
introducing a new limit here.

>> A typical failure case:
>>
>> $ git clone git://whatever.git foobar
>> $ ln -s foobar/src/tools/misc/myapp myapp
>> $ cd myapp
>
> Which is a strange thing to do. What is that for?
> myapp is kind of outside the git repo foobar.

For convenience, mostly; obviously that example was a bit contrived but 
I do have several symlinks to subdirectories of my repository and it's 
faster to type "cd ~/xyz" than "cd ~/repo/src/server/xyz" all the time. 
And as you say, you're only "kind of" outside the repo when going 
through the symlink; one could argue that cd-ing into a symlink should 
be the semantic equivalent of cd-ing into the thing the link points to, 
and that's certainly the way I use it.

But actually my big objection isn't that it fails, per se, but rather 
that it fails inconsistently. All the C commands work just fine since 
they do getcwd() which returns the real path. It's only the shell 
scripts that fail, e.g. git-pull. With the existing implementation I 
have to remember which commands are shell scripts and which are C 
programs, so I can do "cd `/bin/pwd`" to reset my $PWD before running 
any of the former.

That was actually my initial approach to fixing this -- I put "cd 
`/bin/pwd`" at the top of the "cd_to_toplevel" function in 
git-sh-setup.sh. But it felt cleaner to make git-rev-parse return the 
actual correct path so it'd work for shell scripts that didn't happen to 
use git-sh-setup.sh. I'm happy to go either way, or of course to keep 
this as a local modification if folks find it too distasteful to include 
in the official source.

-Steve

[PATCH] Use PATH_MAX rather than a hardwired constant maximum length.

[koreth]

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---

Is the +1 really needed? The existing code is doing this in other places
but I'm not sure it's necessary since we're also doing sizeof(buffer)-1
in the getcwd() call. I figured it was best to be consistent with the
existing code, e.g. setup_git_directory_gently().

 setup.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index a45ea83..84d3c4a 100644
--- a/setup.c
+++ b/setup.c
@@ -175,11 +175,11 @@ static int inside_git_dir = -1;
 int is_inside_git_dir(void)
 {
 	if (inside_git_dir < 0) {
-		char buffer[1024];
+		char buffer[PATH_MAX+1];
 
 		if (is_bare_repository())
 			return (inside_git_dir = 1);
-		if (getcwd(buffer, sizeof(buffer))) {
+		if (getcwd(buffer, sizeof(buffer)-1)) {
 			const char *git_dir = get_git_dir(), *cwd = buffer;
 			while (*git_dir && *git_dir == *cwd) {
 				git_dir++;
-- 
1.5.2.rc0.35.gf41c8

Re: [PATCH] Use PATH_MAX rather than a hardwired constant maximum length.

[Alex Riesen]

On 4/26/07, koreth@midwinter.com <koreth@midwinter.com> wrote:
> Signed-off-by: Steven Grimm <koreth@midwinter.com>
> ---
>
> Is the +1 really needed? The existing code is doing this in other places

No, not according SUSv3, where PATH_MAX is defined as
"Maximum number of bytes in a pathname, including the terminating null
character."
http://www.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html#tag_13_24

getcwd expects the size of the buffer, so it can store the cwd _and_ NUL.
See ERANGE in in SUSv3:
http://www.opengroup.org/onlinepubs/009695399/functions/getcwd.html

> but I'm not sure it's necessary since we're also doing sizeof(buffer)-1

The -1 is wrong too.

> in the getcwd() call. I figured it was best to be consistent with the
> existing code, e.g. setup_git_directory_gently().

setup_git_directory_gently is wrong. But it does not really
matter in practice.

Re: [PATCH] Change git-rev-parse --show-cdup to output an absolute path

[Alex Riesen]

On 4/26/07, Steven Grimm <koreth@midwinter.com> wrote:
> Alex Riesen wrote:
> > Your implementation will fail if cwd is longer than PATH_MAX.
> > Does not happen often, though.
>
> That limitation is already littered through the code, e.g. in setup.c
> which would already be failing in the existing implementation. Actually
> setup.c goes one better: is_inside_git_dir() hardwires a 1024-character
> limit into the code rather than using PATH_MAX. I didn't think I was
> introducing a new limit here.

It is yet another place to fix when it breaks.

> >> A typical failure case:
> >>
> >> $ git clone git://whatever.git foobar
> >> $ ln -s foobar/src/tools/misc/myapp myapp
> >> $ cd myapp
> >
> > Which is a strange thing to do. What is that for?
> > myapp is kind of outside the git repo foobar.
>
> For convenience, mostly; obviously that example was a bit contrived but
> I do have several symlinks to subdirectories of my repository and it's
> faster to type "cd ~/xyz" than "cd ~/repo/src/server/xyz" all the time.

Can't you use your shells variables or aliases (which is even faster to
type)?

> That was actually my initial approach to fixing this -- I put "cd
> `/bin/pwd`" at the top of the "cd_to_toplevel" function in
> git-sh-setup.sh. But it felt cleaner to make git-rev-parse return the
> actual correct path so it'd work for shell scripts that didn't happen to
> use git-sh-setup.sh. I'm happy to go either way, or of course to keep
> this as a local modification if folks find it too distasteful to include
> in the official source.

I'm not really objecting. It's just a bit unusual so I suspect unusual
problems coming out of it. Kind of when you try running with your
eyes closed.