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

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

---

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

index 37addb2..99a95eb 100644
@@ -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")) {

new file mode 100755
index 0000000..233d9cf
@@ -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

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

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

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.

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

---

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

index a45ea83..84d3c4a 100644
@@ -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

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

The -1 is wrong too.

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

It is yet another place to fix when it breaks.

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

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.