{"thread":{"id":"17205","subject":"[RFC PATCH] Fix gitdir detection when in subdir of gitdir","startedAt":"2009-01-16T15:37:33Z","lastAt":"2009-01-19T07:17:17Z","messageCount":12,"participants":["SZEDER Gábor","Johannes Schindelin","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"100744","messageId":"1232120253-1551-1-git-send-email-szeder@ira.uka.de","threadId":"17205","inReplyTo":null,"subject":"[RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2009-01-16T15:37:33Z","receivedAt":"2009-01-16T15:37:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"If the current working directory is a subdirectory of the gitdir (e.g.\n/.git/refs/), then setup_git_directory_gently() will climb its\nparent directories until it finds itself in a gitdir. However, no\nmatter how many parent directories it climbs, it sets\n'GIT_DIR_ENVIRONMENT' to \".\", which is obviously wrong.\n\nThis behaviour affected at least 'git rev-parse --git-dir' and hence\ncaused some errors in bash completion (e.g. customized command prompt\nwhen on a detached head and completion of refs).\n\nTo fix this, we set the absolute path of the found gitdir instead.\n\nSigned-off-by: SZEDER Gábor \n---\n\n I'm not sure about setting an absolut path instead of a relative one\n (hence the RFC), although I think it should not make any difference.\n Of course I could have count the number of chdir(\"..\") calls and then\n construct a \"../../..\", but that would have been more intrusive than\n this two-liner.\n\n setup.c | 3 ++-\n t/t1501-worktree.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 6b277b6..b787a54 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -456,7 +456,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\tinside_git_dir = 1;\n \t\t\tif (!work_tree_env)\n \t\t\t\tinside_work_tree = 0;\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n+\t\t\tcwd[offset] = '\\0';\n+\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cwd, 1);\n \t\t\tcheck_repository_format_gently(nongit_ok);\n \t\t\treturn NULL;\n \t\t}\ndiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\nindex f6a6f83..27dc6c5 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -92,6 +92,13 @@ cd sub/dir || exit 1\n test_rev_parse 'in repo.git/sub/dir' false true true sub/dir/\n cd ../../../.. || exit 1\n \n+test_expect_success 'detecting gitdir when cwd is in a subdir of gitdir' '\n+\t(expected=$(pwd)/repo.git &&\n+\t cd repo.git/refs &&\n+\t unset GIT_DIR &&\n+\t test \"$expected\" = \"$(git rev-parse --git-dir)\")\n+'\n+\n test_expect_success 'repo finds its work tree' '\n \t(cd repo.git &&\n \t : > work/sub/dir/untracked &&\n-- \n1.6.1.153.g15508\n"},{"id":"100751","messageId":"alpine.DEB.1.00.0901161729070.3586@pacific.mpi-cbg.de","threadId":"17205","inReplyTo":"1232120253-1551-1-git-send-email-szeder@ira.uka.de","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-16T16:29:55Z","receivedAt":"2009-01-16T16:29:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 Jan 2009, SZEDER Gábor wrote:\n\n> I'm not sure about setting an absolut path instead of a relative one \n> (hence the RFC), although I think it should not make any difference. \n> Of course I could have count the number of chdir(\"..\") calls and then \n> construct a \"../../..\", but that would have been more intrusive than \n> this two-liner.\n\nIIRC the absolute paths were shot down already... for performance reasons.\n\nSo we try very hard to keep relative paths instead of absolute ones.\n\nCiao,\nDscho\n"},{"id":"100752","messageId":"4970BA2B.7090807@viscovery.net","threadId":"17205","inReplyTo":"alpine.DEB.1.00.0901161729070.3586@pacific.mpi-cbg.de","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-01-16T16:47:39Z","receivedAt":"2009-01-16T16:47:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin schrieb:\n> Hi,\n> \n> On Fri, 16 Jan 2009, SZEDER Gábor wrote:\n> \n>> I'm not sure about setting an absolut path instead of a relative one \n>> (hence the RFC), although I think it should not make any difference. \n>> Of course I could have count the number of chdir(\"..\") calls and then \n>> construct a \"../../..\", but that would have been more intrusive than \n>> this two-liner.\n> \n> IIRC the absolute paths were shot down already... for performance reasons.\n> \n> So we try very hard to keep relative paths instead of absolute ones.\n\nThis is a different matter.\n\nThe question is basically: How should git behave if $PWD is inside a bare\nrepository? And if you are inside .git/refs, than for git this looks as if\nit were a bare repository.\n\nThe current behavior is that we chdir() up into .git, but do not set a\nprefix. Nor do we chdir() back where we started after the discovery.\n\nGábor's patch needs a better justification which misbehavior it tries to\nfix, and the spot that it changes:\n\n\t\tif (is_git_directory(\".\")) {\n\t\t\tinside_git_dir = 1;\n\t\t\tif (!work_tree_env)\n\t\t\t\tinside_work_tree = 0;\n\t\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n\t\t\tcheck_repository_format_gently(nongit_ok);\n\t\t\treturn NULL;\n\t\t}\n\nneeds a comment why it does what it does (and that this if-branch is only\nabout bare repositories).\n\n-- Hannes\n"},{"id":"100754","messageId":"4970BAE5.8080006@viscovery.net","threadId":"17205","inReplyTo":"4970BA2B.7090807@viscovery.net","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-01-16T16:50:45Z","receivedAt":"2009-01-16T16:50:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Sixt schrieb:\n> Gábor's patch needs a better justification which misbehavior it tries to\n> fix, and the spot that it changes:\n\nI actually meant: \"which use-case the patch tries to help\". Because the\ncurrent behavior can hardly be classified as bug. (\"You have no business\ncd-ing around in .git.\" ;)\n\n-- Hannes\n"},{"id":"100758","messageId":"20090116172346.GA15804@neumann","threadId":"17205","inReplyTo":"4970BAE5.8080006@viscovery.net","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2009-01-16T17:23:46Z","receivedAt":"2009-01-16T17:23:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 16, 2009 at 05:50:45PM +0100, Johannes Sixt wrote:\n> Johannes Sixt schrieb:\n> > Gábor's patch needs a better justification which misbehavior it tries to\n> > fix, and the spot that it changes:\n> \n> I actually meant: \"which use-case the patch tries to help\". Because the\n> current behavior can hardly be classified as bug. (\"You have no business\n> cd-ing around in .git.\" ;)\n\nI agree that fiddling around in '.git' is a quite rare use case.\n\nI did it while I was working on bash completion support for the\nupcoming 'git sequencer' to see where it stores its temporary files\nand what is in those files. And I got errors from the completion\nscript after each executed command, which quickly made me upset enough\nto look after it.\n\nI thought it worths fixing, but it's even better if it's not a bug,\nbecause then I don't have to fix my fix (;\n\nRegards,\nGábor\n"},{"id":"100761","messageId":"7vr63386rc.fsf@gitster.siamese.dyndns.org","threadId":"17205","inReplyTo":"4970BAE5.8080006@viscovery.net","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-16T18:07:03Z","receivedAt":"2009-01-16T18:07:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Johannes Sixt schrieb:\n>> Gábor's patch needs a better justification which misbehavior it tries to\n>> fix, and the spot that it changes:\n>\n> I actually meant: \"which use-case the patch tries to help\". Because the\n> current behavior can hardly be classified as bug. (\"You have no business\n> cd-ing around in .git.\" ;)\n\nThanks.\n\nI think (1) the solution (almost) makes sense, (2) the patch needs to be\nexplained a lot better as you mentioned in your two messages, and (3) if\nit does not affect any other case than when you are in a subdirectory of\nthe .git/ directory, then you are doing something funny anyway and\nperformance issue Dscho mentions, if any, is not a concern.\n\nMy \"(almost)\" in (1) above is because the patch uses this new behaviour\neven when you are inside the .git/ directory itself (or at the root of a\nbare repository), which is a very common case that we do not have to nor\nwant to change the behaviour. It also invalidates the precondition of (3)\nabove.\n\nDscho, what performance issues do you have in mind, by the way?\n"},{"id":"100771","messageId":"alpine.DEB.1.00.0901162150550.3586@pacific.mpi-cbg.de","threadId":"17205","inReplyTo":"7vr63386rc.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-16T20:55:04Z","receivedAt":"2009-01-16T20:55:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 Jan 2009, Junio C Hamano wrote:\n\n> Dscho, what performance issues do you have in mind, by the way?\n\nBack when I tried to fix the worktree issue (still the subject of some of \nmy nightmares), I set the GIT_DIR (IIRC) to the absolute path, just to \nmake sure that it works in all cases, even when the work tree is far away \nfrom the GIT_DIR (think DOS drives, blech).\n\nI could be mistaken, but I think it was there that somebody did some \ntiming and found that lstats on hundreds of absolute paths were \nsubstantially slower than on relative paths.\n\nNow, think of git-gc in a large number of bare repositories, such as \nrepo.or.cz. It does matter there.\n\nCiao,\nDscho\n"},{"id":"101061","messageId":"7vhc3wuwxb.fsf@gitster.siamese.dyndns.org","threadId":"17205","inReplyTo":"7vr63386rc.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-18T21:27:44Z","receivedAt":"2009-01-18T21:27:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I think (1) the solution (almost) makes sense, (2) the patch needs to be\n> explained a lot better as you mentioned in your two messages, and (3) if\n> it does not affect any other case than when you are in a subdirectory of\n> the .git/ directory, then you are doing something funny anyway and\n> performance issue Dscho mentions, if any, is not a concern.\n>\n> My \"(almost)\" in (1) above is because the patch uses this new behaviour\n> even when you are inside the .git/ directory itself (or at the root of a\n> bare repository), which is a very common case that we do not have to nor\n> want to change the behaviour. It also invalidates the precondition of (3)\n> above.\n\nAnd this is a trivial follow-up on top of Szeder's patch.\n\n setup.c | 7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git c/setup.c w/setup.c\nindex 4049298..dd7c039 100644\n--- c/setup.c\n+++ w/setup.c\n@@ -456,8 +456,11 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\tinside_git_dir = 1;\n \t\t\tif (!work_tree_env)\n \t\t\t\tinside_work_tree = 0;\n-\t\t\tcwd[offset] = '\\0';\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cwd, 1);\n+\t\t\tif (offset != len) {\n+\t\t\t\tcwd[offset] = '\\0';\n+\t\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cwd, 1);\n+\t\t\t} else\n+\t\t\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n \t\t\tcheck_repository_format_gently(nongit_ok);\n \t\t\treturn NULL;\n \t\t}\n"},{"id":"101079","messageId":"20090119020311.GA8753@neumann","threadId":"17205","inReplyTo":"7vhc3wuwxb.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"SZEDER Gábor","fromEmail":"szeder@fzi.de","sentAt":"2009-01-19T02:03:11Z","receivedAt":"2009-01-19T02:03:11Z","isPatch":true,"sender":{"key":"szeder@fzi.de","avatar":null},"body":"On Sun, Jan 18, 2009 at 01:27:44PM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > I think (1) the solution (almost) makes sense, (2) the patch needs to be\n> > explained a lot better as you mentioned in your two messages, and (3) if\n> > it does not affect any other case than when you are in a subdirectory of\n> > the .git/ directory, then you are doing something funny anyway and\n> > performance issue Dscho mentions, if any, is not a concern.\n> >\n> > My \"(almost)\" in (1) above is because the patch uses this new behaviour\n> > even when you are inside the .git/ directory itself (or at the root of a\n> > bare repository), which is a very common case that we do not have to nor\n> > want to change the behaviour. It also invalidates the precondition of (3)\n> > above.\n> \n> And this is a trivial follow-up on top of Szeder's patch.\n\nThanks. In the meantime I was working on a patch that sets relative\npath in this case, too. I got it almost working: all tests passed\nexcept '.git/objects/: is-bare-repository' in 't1500-rev-parse'. I\ncouldn't figure it out why this test failed, however.\n\nIn case somebody might be interested for such an uncommon case, the\npatch is below.\n\n\nBest,\nGábor\n\n\n setup.c | 17 +++++++++++++++--\n 1 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 6b277b6..b4d37d7 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -375,7 +375,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tstatic char cwd[PATH_MAX+1];\n \tconst char *gitdirenv;\n \tconst char *gitfile_dir;\n-\tint len, offset, ceil_offset;\n+\tint len, offset, ceil_offset, cdup_count = 0;\n \n \t/*\n \t * Let's assume that we are in a git repository.\n@@ -453,10 +453,22 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tif (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))\n \t\t\tbreak;\n \t\tif (is_git_directory(\".\")) {\n+\t\t\tchar gd_rel_path[PATH_MAX];\n \t\t\tinside_git_dir = 1;\n \t\t\tif (!work_tree_env)\n \t\t\t\tinside_work_tree = 0;\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n+\t\t\tif (cdup_count) {\n+\t\t\t\tchar *p = gd_rel_path;\n+\t\t\t\twhile (cdup_count-- > 1) {\n+\t\t\t\t\t*p++ = '.'; *p++ = '.'; *p++ = '/';\n+\t\t\t\t}\n+\t\t\t\t*p++ = '.'; *p++ = '.';\n+\t\t\t\t*p = '\\0';\n+\t\t\t} else {\n+\t\t\t\tgd_rel_path[0] = '.';\n+\t\t\t\tgd_rel_path[1] = '\\0';\n+\t\t\t}\n+\t\t\tsetenv(GIT_DIR_ENVIRONMENT, gd_rel_path, 1);\n \t\t\tcheck_repository_format_gently(nongit_ok);\n \t\t\treturn NULL;\n \t\t}\n@@ -472,6 +484,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t}\n \t\tif (chdir(\"..\"))\n \t\t\tdie(\"Cannot change to %s/..: %s\", cwd, strerror(errno));\n+\t\tcdup_count++;\n \t}\n \n \tinside_git_dir = 0;\n"},{"id":"101080","messageId":"20090119020841.GA9798@neumann","threadId":"17205","inReplyTo":"7vhc3wuwxb.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] t1500: extend with tests of 'git rev-parse --git-dir'","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2009-01-19T02:08:41Z","receivedAt":"2009-01-19T02:08:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Signed-off-by: SZEDER Gábor \n---\n\n These will pass with Junio's follow-up.\n\n\n t/t1500-rev-parse.sh | 17 ++++++++++++-----\n 1 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 85da4ca..48ee077 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -26,21 +26,28 @@ test_rev_parse() {\n \t\"test '$1' = \\\"\\$(git rev-parse --show-prefix)\\\"\"\n \tshift\n \t[ $# -eq 0 ] && return\n+\n+\ttest_expect_success \"$name: git-dir\" \\\n+\t\"test '$1' = \\\"\\$(git rev-parse --git-dir)\\\"\"\n+\tshift\n+\t[ $# -eq 0 ] && return\n }\n \n-# label is-bare is-inside-git is-inside-work prefix\n+# label is-bare is-inside-git is-inside-work prefix git-dir\n+\n+ROOT=$(pwd)\n \n-test_rev_parse toplevel false false true ''\n+test_rev_parse toplevel false false true '' .git\n \n cd .git || exit 1\n-test_rev_parse .git/ false true false ''\n+test_rev_parse .git/ false true false '' .\n cd objects || exit 1\n-test_rev_parse .git/objects/ false true false ''\n+test_rev_parse .git/objects/ false true false '' \"$ROOT/.git\"\n cd ../.. || exit 1\n \n mkdir -p sub/dir || exit 1\n cd sub/dir || exit 1\n-test_rev_parse subdirectory false false true sub/dir/\n+test_rev_parse subdirectory false false true sub/dir/ \"$ROOT/.git\"\n cd ../.. || exit 1\n \n git config core.bare true\n-- \n1.6.1.201.g0e7e.dirty\n"},{"id":"101088","messageId":"7vy6x8rnpk.fsf@gitster.siamese.dyndns.org","threadId":"17205","inReplyTo":"20090119020311.GA8753@neumann","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-19T03:15:03Z","receivedAt":"2009-01-19T03:15:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Thanks. In the meantime I was working on a patch that sets relative\n> path in this case, too.\n\nDoes it make sense to use relative path in such a case?\n\nIf it is for \"rev-parse --git-dir\", the calling script may learn the\ncorrect location of the GIT_DIR with either relative or absolute, but if\nit is for the internal consumption of git process itself and any\nsubprocess forked from us that look at GIT_DIR we export, the process\nalready runs at the repository root (because you do not chdir back) and\nusing relative path does not make much sense. Exported GIT_DIR has to be\neither \".\" or the full path from the root to make sense to such a user, I\nthink.\n"},{"id":"101107","messageId":"497428FD.7050301@viscovery.net","threadId":"17205","inReplyTo":"20090119020311.GA8753@neumann","subject":"Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-01-19T07:17:17Z","receivedAt":"2009-01-19T07:17:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"SZEDER Gábor schrieb:\n> \t\tif (is_git_directory(\".\")) {\n> +\t\t\tchar gd_rel_path[PATH_MAX];\n> \t\t\tinside_git_dir = 1;\n> \t\t\tif (!work_tree_env)\n> \t\t\t\tinside_work_tree = 0;\n> -\t\t\tsetenv(GIT_DIR_ENVIRONMENT, \".\", 1);\n> +\t\t\tif (cdup_count) {\n> +\t\t\t\tchar *p = gd_rel_path;\n> +\t\t\t\twhile (cdup_count-- > 1) {\n> +\t\t\t\t\t*p++ = '.'; *p++ = '.'; *p++ = '/';\n> +\t\t\t\t}\n> +\t\t\t\t*p++ = '.'; *p++ = '.';\n> +\t\t\t\t*p = '\\0';\n> +\t\t\t} else {\n> +\t\t\t\tgd_rel_path[0] = '.';\n> +\t\t\t\tgd_rel_path[1] = '\\0';\n> +\t\t\t}\n> +\t\t\tsetenv(GIT_DIR_ENVIRONMENT, gd_rel_path, 1);\n> \t\t\tcheck_repository_format_gently(nongit_ok);\n> \t\t\treturn NULL;\n> \t\t}\n\nThis does not make sense because you don't chdir back to where you\nstarted, so the relative path would be incorrect.\n\nI have the feeling that it is not worth to support this particular\nuse-case with so many lines of code.\n\n-- Hannes\n"}]}