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

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

Hi,

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

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

Johannes Schindelin schrieb:

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

Johannes Sixt schrieb:

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

On Fri, Jan 16, 2009 at 05:50:45PM +0100, Johannes Sixt wrote:

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

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

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,

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

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

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

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

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

On Sun, Jan 18, 2009 at 01:27:44PM -0800, Junio C Hamano wrote:

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

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

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.

SZEDER Gábor schrieb:

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

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