{"thread":{"id":"23880","subject":"[PATCH] setup: do not change to work tree prematurely","startedAt":"2010-05-23T00:07:19Z","lastAt":"2010-05-23T09:14:42Z","messageCount":3,"participants":["Clemens Buchacher","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"142126","messageId":"20100523000719.GA32380@localhost","threadId":"23880","inReplyTo":null,"subject":"[PATCH] setup: do not change to work tree prematurely","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2010-05-23T00:07:19Z","receivedAt":"2010-05-23T00:07:19Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If the work tree is known and a git command is invoked from within\na git directory, git_setup_directory() will try to find the\nrelative path from the work tree to the git dir. After doing so it\nchanges directories to the work tree. It fails to update the\nrelative path to the git directory, however.\n\nInstead, do not change the working directory at this point and wait\nfor git_setup_work_tree() to handle this correctly.\n\nThis fixes the following bug.\n\n$ cd .git\n$ git --work-tree=/tmp/git symbolic-ref HEAD\nfatal: ref HEAD is not a symbolic ref\n\nReported-by: Frédéric Brière <fbriere@fbriere.net>\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nI am not 100% certain that my analysis is correct, since I still do\nnot understand the setup code. But as far as setup_git_directory()\nis concerned, I think this is the intended behavior.\n\nClemens\n\n dir.c   |   33 +++++++++++++++++++--------------\n dir.h   |    1 +\n setup.c |   14 +++++++++-----\n 3 files changed, 29 insertions(+), 19 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex cb83332..7bf208d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -926,6 +926,24 @@ int file_exists(const char *f)\n \treturn lstat(f, &sb) == 0;\n }\n \n+char *get_relative_path(char *cwd, const char *dir)\n+{\n+\tif (!dir)\n+\t\treturn NULL;\n+\tif (!is_absolute_path(dir))\n+\t\tdir = make_absolute_path(dir);\n+\n+\twhile (*dir && *dir == *cwd) {\n+\t\tdir++;\n+\t\tcwd++;\n+\t}\n+\tif (*dir)\n+\t\treturn NULL;\n+\tif (*cwd == '/')\n+\t\treturn cwd + 1;\n+\treturn cwd;\n+}\n+\n /*\n  * get_relative_cwd() gets the prefix of the current working directory\n  * relative to 'dir'.  If we are not inside 'dir', it returns NULL.\n@@ -942,25 +960,12 @@ int file_exists(const char *f)\n  */\n char *get_relative_cwd(char *buffer, int size, const char *dir)\n {\n-\tchar *cwd = buffer;\n-\n \tif (!dir)\n \t\treturn NULL;\n \tif (!getcwd(buffer, size))\n \t\tdie_errno(\"can't find the current directory\");\n \n-\tif (!is_absolute_path(dir))\n-\t\tdir = make_absolute_path(dir);\n-\n-\twhile (*dir && *dir == *cwd) {\n-\t\tdir++;\n-\t\tcwd++;\n-\t}\n-\tif (*dir)\n-\t\treturn NULL;\n-\tif (*cwd == '/')\n-\t\treturn cwd + 1;\n-\treturn cwd;\n+\treturn get_relative_path(buffer, dir);\n }\n \n int is_inside_dir(const char *dir)\ndiff --git a/dir.h b/dir.h\nindex 3bead5f..3bcda1f 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -79,6 +79,7 @@ extern void add_exclude(const char *string, const char *base,\n \t\t\tint baselen, struct exclude_list *which);\n extern int file_exists(const char *);\n \n+extern char *get_relative_path(char *cwd, const char *dir);\n extern char *get_relative_cwd(char *buffer, int size, const char *dir);\n extern int is_inside_dir(const char *dir);\n \ndiff --git a/setup.c b/setup.c\nindex 5716d90..67b5122 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -525,13 +525,17 @@ const char *setup_git_directory(void)\n \n \t/* If the work tree is not the default one, recompute prefix */\n \tif (inside_work_tree < 0) {\n+\t\tconst char *work_tree = get_git_work_tree();\n \t\tstatic char buffer[PATH_MAX + 1];\n \t\tchar *rel;\n-\t\tif (retval && chdir(retval))\n-\t\t\tdie_errno (\"Could not jump back into original cwd\");\n-\t\trel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());\n-\t\tif (rel && *rel && chdir(get_git_work_tree()))\n-\t\t\tdie_errno (\"Could not jump to working directory\");\n+\t\tif (retval) {\n+\t\t\tif (!is_absolute_path(retval))\n+\t\t\t\tretval = make_absolute_path(retval);\n+\t\t\tstrncpy(buffer, retval, PATH_MAX);\n+\t\t\tbuffer[PATH_MAX] = '\\0';\n+\t\t\trel = get_relative_path(buffer, work_tree);\n+\t\t} else\n+\t\t\trel = get_relative_cwd(buffer, PATH_MAX, work_tree);\n \t\treturn rel && *rel ? strcat(rel, \"/\") : NULL;\n \t}\n \n-- \n1.7.0.5.3.ga76e\n"},{"id":"142127","messageId":"20100523013539.GA14403@progeny.tock","threadId":"23880","inReplyTo":"20100523000719.GA32380@localhost","subject":"Re: [PATCH] setup: do not change to work tree prematurely","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-23T01:35:39Z","receivedAt":"2010-05-23T01:35:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Clemens,\n\nClemens Buchacher wrote:\n\n> $ cd .git\n> $ git --work-tree=/tmp/git symbolic-ref HEAD\n> fatal: ref HEAD is not a symbolic ref\n\nWithout your patch applied, I get\n\n $ cd .git\n $ git --work-tree=/tmp/git symbolic-ref HEAD\n refs/heads/hello\n\nI should have done the following instead:\n\n $ worktree=$(pwd)\n $ cd .git\n $ git --work-tree=\"$worktree\" symbolic-ref HEAD\n fatal: ref HEAD is not a symbolic ref\n\n> +char *get_relative_path(char *cwd, const char *dir)\n\nLooks reasonable.  Should be const char *cwd.\n\n> diff --git a/setup.c b/setup.c\n> index 5716d90..67b5122 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -524,14 +524,18 @@ const char *setup_git_directory(void)\n> \tconst char *retval = setup_git_directory_gently(NULL);\n>  \n>  \t/* If the work tree is not the default one, recompute prefix */\n>  \tif (inside_work_tree < 0) {\n\nIf I understand correctly, you made two changes here:\n\n - interpret GIT_WORK_TREE and friends relative to .git instead of\n   the original cwd (by not calling chdir(retval) before\n   get_git_work_tree())\n\n - make setup_git_directory stay in the last directory searched for a\n   .git directory instead of chdir-ing into the toplevel of the work tree.\n\nAre these safe changes to make?\n\nJonathan\n\ndiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\nindex 9df3012..93bf92c 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -106,6 +106,15 @@ test_expect_success 'repo finds its work tree from work tree, too' '\n \t test sub/dir/tracked = \"$(git ls-files)\")\n '\n \n+test -d work || cp -R repo.git/work .\n+test_expect_success 'repo finds its .git dir with separate worktree' '\n+\t(unset GIT_DIR GIT_WORK_TREE GIT_CONFIG &&\n+\t worktree=$(pwd) &&\n+\t cd repo.git &&\n+\t git symbolic-ref HEAD &&\n+\t git --work-tree=\"$worktree\" symbolic-ref HEAD)\n+'\n+\n test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '\n \t(cd repo.git/work/sub/dir &&\n \tGIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \\\n-- \n"},{"id":"142133","messageId":"20100523091442.GA8128@localhost","threadId":"23880","inReplyTo":"20100523013539.GA14403@progeny.tock","subject":"Re: [PATCH] setup: do not change to work tree prematurely","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2010-05-23T09:14:42Z","receivedAt":"2010-05-23T09:14:42Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sat, May 22, 2010 at 08:35:39PM -0500, Jonathan Nieder wrote:\n\n> Without your patch applied, I get\n> \n>  $ cd .git\n>  $ git --work-tree=/tmp/git symbolic-ref HEAD\n>  refs/heads/hello\n> \n> I should have done the following instead:\n> \n>  $ worktree=$(pwd)\n>  $ cd .git\n>  $ git --work-tree=\"$worktree\" symbolic-ref HEAD\n>  fatal: ref HEAD is not a symbolic ref\n\nThanks\n\n> > +char *get_relative_path(char *cwd, const char *dir)\n> \n> Looks reasonable.  Should be const char *cwd.\n\nThat is not possible, because we return a pointer to cwd plus an\noffset, which will be subsequently modified by strcat().\n\n> > diff --git a/setup.c b/setup.c\n> > index 5716d90..67b5122 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -524,14 +524,18 @@ const char *setup_git_directory(void)\n> > \tconst char *retval = setup_git_directory_gently(NULL);\n> >  \n> >  \t/* If the work tree is not the default one, recompute prefix */\n> >  \tif (inside_work_tree < 0) {\n> \n> If I understand correctly, you made two changes here:\n> \n>  - interpret GIT_WORK_TREE and friends relative to .git instead of\n>    the original cwd (by not calling chdir(retval) before\n>    get_git_work_tree())\n\nOops\n\n>  - make setup_git_directory stay in the last directory searched for a\n>    .git directory instead of chdir-ing into the toplevel of the work tree.\n>\n> Are these safe changes to make?\n\nIndeed, if you put it that way, this change does not look good.\nMaybe I'll have a look at Nguyen's patches.\n\n> diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\n> index 9df3012..93bf92c 100755\n> --- a/t/t1501-worktree.sh\n> +++ b/t/t1501-worktree.sh\n\nThanks\n"}]}