[RFC PATCH] Fix gitdir detection when in subdir of gitdir

[RFC PATCH] Fix gitdir detection when in subdir of gitdir

[SZEDER Gábor]

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.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

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

diff --git a/setup.c b/setup.c
index 6b277b6..b787a54 100644
--- a/setup.c
+++ b/setup.c
@@ -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;
 		}
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index f6a6f83..27dc6c5 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -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

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 524 bytes --]

Hi,

On Fri, 16 Jan 2009, SZEDER Gábor wrote:

>   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.

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

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[Johannes Sixt]

Johannes Schindelin schrieb:
> Hi,
> 
> On Fri, 16 Jan 2009, SZEDER Gábor wrote:
> 
>>   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.
> 
> IIRC the absolute paths were shot down already... for performance reasons.
> 
> So we try very hard to keep relative paths instead of absolute ones.

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

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[Johannes Sixt]

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

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

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[SZEDER Gábor]

On Fri, Jan 16, 2009 at 05:50:45PM +0100, Johannes Sixt wrote:
> Johannes Sixt schrieb:
> > Gábor's patch needs a better justification which misbehavior it tries to
> > fix, and the spot that it changes:
> 
> 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." ;)

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

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Johannes Sixt schrieb:
>> Gábor's patch needs a better justification which misbehavior it tries to
>> fix, and the spot that it changes:
>
> 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." ;)

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?

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[Johannes Schindelin]

Hi,

On Fri, 16 Jan 2009, Junio C Hamano wrote:

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

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

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

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

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

diff --git c/setup.c w/setup.c
index 4049298..dd7c039 100644
--- c/setup.c
+++ w/setup.c
@@ -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;
 		}

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[SZEDER Gábor]

On Sun, Jan 18, 2009 at 01:27:44PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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.
> 
> And this is a trivial follow-up on top of Szeder's patch.

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

diff --git a/setup.c b/setup.c
index 6b277b6..b4d37d7 100644
--- a/setup.c
+++ b/setup.c
@@ -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;

[PATCH] t1500: extend with tests of 'git rev-parse --git-dir'

[SZEDER Gábor]

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

  These will pass with Junio's follow-up.


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

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 85da4ca..48ee077 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -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

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[Junio C Hamano]

SZEDER Gábor <szeder@fzi.de> writes:

> Thanks.  In the meantime I was working on a patch that sets relative
> path in this case, too.

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.

Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir

[Johannes Sixt]

SZEDER Gábor schrieb:
>  		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;
>  		}

This does not make sense because you don't chdir back to where you
started, so the relative path would be incorrect.

I have the feeling that it is not worth to support this particular
use-case with so many lines of code.

-- Hannes