Fix gitdir detection when in subdir of gitdir

If the current working directory is a subdirectory of the gitdir (e.g. <repo>/.git/refs/), then setup_git_directory_gently() will climb its parent directories until it finds itself in a gitdir. However, no matter how many parent directories it climbs, it sets 'GIT_DIR_ENVIRONMENT' to ".", which is obviously wrong.

This behaviour affected at least 'git rev-parse --git-dir' and hence caused some errors in bash completion (e.g. customized command prompt when on a detached head and completion of refs).

To fix this, we set the absolute path of the found gitdir instead.

---

I'm not sure about setting an absolut path instead of a relative one (hence the RFC), although I think it should not make any difference. Of course I could have count the number of chdir("..") calls and then construct a "../../..", but that would have been more intrusive than this two-liner.

 setup.c             |    3 ++-
 t/t1501-worktree.sh |    7 +++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

index 6b277b6..b787a54 100644
@@ -456,7 +456,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			inside_git_dir = 1;
 			if (!work_tree_env)
 				inside_work_tree = 0;
-			setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+			cwd[offset] = '\0';
+			setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
 			check_repository_format_gently(nongit_ok);
 			return NULL;
 		}

index f6a6f83..27dc6c5 100755
@@ -92,6 +92,13 @@ cd sub/dir || exit 1
 test_rev_parse 'in repo.git/sub/dir' false true true sub/dir/
 cd ../../../.. || exit 1
 
+test_expect_success 'detecting gitdir when cwd is in a subdir of gitdir' '
+	(expected=$(pwd)/repo.git &&
+	 cd repo.git/refs &&
+	 unset GIT_DIR &&
+	 test "$expected" = "$(git rev-parse --git-dir)")
+'
+
 test_expect_success 'repo finds its work tree' '
 	(cd repo.git &&
 	 : > work/sub/dir/untracked &&

1.6.1.153.g15508

Hi,

IIRC the absolute paths were shot down already... for performance reasons.

So we try very hard to keep relative paths instead of absolute ones.

Ciao,
Dscho

This is a different matter.

The question is basically: How should git behave if $PWD is inside a bare repository? And if you are inside .git/refs, than for git this looks as if it were a bare repository.

The current behavior is that we chdir() up into .git, but do not set a prefix. Nor do we chdir() back where we started after the discovery.

Gábor's patch needs a better justification which misbehavior it tries to fix, and the spot that it changes:

		if (is_git_directory(".")) {
			inside_git_dir = 1;
			if (!work_tree_env)
				inside_work_tree = 0;
			setenv(GIT_DIR_ENVIRONMENT, ".", 1);
			check_repository_format_gently(nongit_ok);
			return NULL;
		}

needs a comment why it does what it does (and that this if-branch is only about bare repositories).

-- Hannes

I actually meant: "which use-case the patch tries to help". Because the current behavior can hardly be classified as bug. ("You have no business cd-ing around in .git." ;)

-- Hannes

I agree that fiddling around in '.git' is a quite rare use case.

I did it while I was working on bash completion support for the upcoming 'git sequencer' to see where it stores its temporary files and what is in those files. And I got errors from the completion script after each executed command, which quickly made me upset enough to look after it.

I thought it worths fixing, but it's even better if it's not a bug, because then I don't have to fix my fix (;

Regards,
Gábor

Thanks.

I think (1) the solution (almost) makes sense, (2) the patch needs to be explained a lot better as you mentioned in your two messages, and (3) if it does not affect any other case than when you are in a subdirectory of the .git/ directory, then you are doing something funny anyway and performance issue Dscho mentions, if any, is not a concern.

My "(almost)" in (1) above is because the patch uses this new behaviour even when you are inside the .git/ directory itself (or at the root of a bare repository), which is a very common case that we do not have to nor want to change the behaviour. It also invalidates the precondition of (3) above.

Dscho, what performance issues do you have in mind, by the way?

Hi,

Back when I tried to fix the worktree issue (still the subject of some of my nightmares), I set the GIT_DIR (IIRC) to the absolute path, just to make sure that it works in all cases, even when the work tree is far away from the GIT_DIR (think DOS drives, blech).

I could be mistaken, but I think it was there that somebody did some timing and found that lstats on hundreds of absolute paths were substantially slower than on relative paths.

Now, think of git-gc in a large number of bare repositories, such as repo.or.cz. It does matter there.

Ciao,
Dscho

And this is a trivial follow-up on top of Szeder's patch.

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

index 4049298..dd7c039 100644
@@ -456,8 +456,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			inside_git_dir = 1;
 			if (!work_tree_env)
 				inside_work_tree = 0;
-			cwd[offset] = '\0';
-			setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
+			if (offset != len) {
+				cwd[offset] = '\0';
+				setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
+			} else
+				setenv(GIT_DIR_ENVIRONMENT, ".", 1);
 			check_repository_format_gently(nongit_ok);
 			return NULL;
 		}

Thanks. In the meantime I was working on a patch that sets relative path in this case, too. I got it almost working: all tests passed except '.git/objects/: is-bare-repository' in 't1500-rev-parse'. I couldn't figure it out why this test failed, however.

In case somebody might be interested for such an uncommon case, the patch is below.

Best,
Gábor

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

index 6b277b6..b4d37d7 100644
@@ -375,7 +375,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	static char cwd[PATH_MAX+1];
 	const char *gitdirenv;
 	const char *gitfile_dir;
-	int len, offset, ceil_offset;
+	int len, offset, ceil_offset, cdup_count = 0;
 
 	/*
 	 * Let's assume that we are in a git repository.

@@ -453,10 +453,22 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
 			break;
 		if (is_git_directory(".")) {
+			char gd_rel_path[PATH_MAX];
 			inside_git_dir = 1;
 			if (!work_tree_env)
 				inside_work_tree = 0;
-			setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+			if (cdup_count) {
+				char *p = gd_rel_path;
+				while (cdup_count-- > 1) {
+					*p++ = '.'; *p++ = '.'; *p++ = '/';
+				}
+				*p++ = '.'; *p++ = '.';
+				*p = '\0';
+			} else {
+				gd_rel_path[0] = '.';
+				gd_rel_path[1] = '\0';
+			}
+			setenv(GIT_DIR_ENVIRONMENT, gd_rel_path, 1);
 			check_repository_format_gently(nongit_ok);
 			return NULL;
 		}

@@ -472,6 +484,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		}
 		if (chdir(".."))
 			die("Cannot change to %s/..: %s", cwd, strerror(errno));
+		cdup_count++;
 	}
 
 	inside_git_dir = 0;

Does it make sense to use relative path in such a case?

If it is for "rev-parse --git-dir", the calling script may learn the correct location of the GIT_DIR with either relative or absolute, but if it is for the internal consumption of git process itself and any subprocess forked from us that look at GIT_DIR we export, the process already runs at the repository root (because you do not chdir back) and using relative path does not make much sense. Exported GIT_DIR has to be either "." or the full path from the root to make sense to such a user, I think.

---

These will pass with Junio's follow-up.

 t/t1500-rev-parse.sh |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

index 85da4ca..48ee077 100755
@@ -26,21 +26,28 @@ test_rev_parse() {
 	"test '$1' = \"\$(git rev-parse --show-prefix)\""
 	shift
 	[ $# -eq 0 ] && return
+
+	test_expect_success "$name: git-dir" \
+	"test '$1' = \"\$(git rev-parse --git-dir)\""
+	shift
+	[ $# -eq 0 ] && return
 }
 
-# label is-bare is-inside-git is-inside-work prefix
+# label is-bare is-inside-git is-inside-work prefix git-dir
+
+ROOT=$(pwd)
 
-test_rev_parse toplevel false false true ''
+test_rev_parse toplevel false false true '' .git
 
 cd .git || exit 1
-test_rev_parse .git/ false true false ''
+test_rev_parse .git/ false true false '' .
 cd objects || exit 1
-test_rev_parse .git/objects/ false true false ''
+test_rev_parse .git/objects/ false true false '' "$ROOT/.git"
 cd ../.. || exit 1
 
 mkdir -p sub/dir || exit 1
 cd sub/dir || exit 1
-test_rev_parse subdirectory false false true sub/dir/
+test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git"
 cd ../.. || exit 1
 
 git config core.bare true

1.6.1.201.g0e7e.dirty