{"thread":{"id":"14649","subject":"[PATCH] Preserve cwd in setup_git_directory()","startedAt":"2008-07-24T03:14:41Z","lastAt":"2008-07-25T10:04:51Z","messageCount":9,"participants":["Nguyễn Thái Ngọc Duy","Johannes Sixt","Nguyen Thai Ngoc Duy","Johannes Schindelin","Geoff Russell"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"84667","messageId":"20080724031441.GA26072@laptop","threadId":"14649","inReplyTo":null,"subject":"[PATCH] Preserve cwd in setup_git_directory()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-07-24T03:14:41Z","receivedAt":"2008-07-24T03:14:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When GIT_DIR is not set, cwd is used to determine where .git is.\nIf core.worktree is set, setup_git_directory() needs to jump back\nto the original cwd in order to calculate worktree, this leads to\nincorrect .git location later in setup_work_tree().\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n setup.c | 4 ++++\n t/t1501-worktree.sh | 13 ++++++++++++-\n 2 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 6cf9094..ef58761 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -577,10 +577,14 @@ const char *setup_git_directory(void)\n \t/* If the work tree is not the default one, recompute prefix */\n \tif (inside_work_tree < 0) {\n \t\tstatic char buffer[PATH_MAX + 1];\n+\t\tstatic char cwd[PATH_MAX + 1];\n \t\tchar *rel;\n+\t\tgetcwd(cwd, PATH_MAX);\n \t\tif (retval && chdir(retval))\n \t\t\tdie (\"Could not jump back into original cwd\");\n \t\trel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());\n+\t\tif (retval && chdir(cwd))\n+\t\t\tdie (\"Could not jump back into original cwd\");\n \t\treturn rel && *rel ? strcat(rel, \"/\") : NULL;\n \t}\n \ndiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\nindex 2ee88d8..64f8dea 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -29,7 +29,18 @@ test_rev_parse() {\n }\n \n mkdir -p work/sub/dir || exit 1\n-mv .git repo.git || exit 1\n+\n+git config core.worktree \"$(pwd)\"/work\n+mv .git work || exit 1\n+test_expect_success '--git-dir with relative .git' '\n+\t(\n+\tMYPWD=\"$(pwd)\"\n+\tcd work/sub/dir &&\n+\ttest \"$MYPWD\"/work/.git = \"$(git rev-parse --git-dir)\"\n+\t)\n+'\n+\n+mv work/.git repo.git || exit 1\n \n say \"core.worktree = relative path\"\n GIT_DIR=repo.git\n-- \n1.5.5.GIT\n"},{"id":"84676","messageId":"48882628.7030305@viscovery.net","threadId":"14649","inReplyTo":"20080724031441.GA26072@laptop","subject":"Re: [PATCH] Preserve cwd in setup_git_directory()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-07-24T06:50:16Z","receivedAt":"2008-07-24T06:50:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Nguyễn Thái Ngọc Duy schrieb:\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -577,10 +577,14 @@ const char *setup_git_directory(void)\n> \t/* If the work tree is not the default one, recompute prefix */\n> \tif (inside_work_tree < 0) {\n> \t\tstatic char buffer[PATH_MAX + 1];\n> +\t\tstatic char cwd[PATH_MAX + 1];\n> \t\tchar *rel;\n> +\t\tgetcwd(cwd, PATH_MAX);\n\nThis needs an error check.\n\n-- Hannes\n"},{"id":"84682","messageId":"20080724081222.GA32354@laptop","threadId":"14649","inReplyTo":"48882628.7030305@viscovery.net","subject":"[PATCH] Preserve cwd in setup_git_directory()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-07-24T08:12:22Z","receivedAt":"2008-07-24T08:12:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When GIT_DIR is not set, cwd is used to determine where .git is.\nIf core.worktree is set, setup_git_directory() needs to jump back\nto the original cwd in order to setup worktree, this leads to\nincorrect .git location later in setup_work_tree().\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n On Thu, Jul 24, 2008 at 08:50:16AM +0200, Johannes Sixt wrote:\n > Nguyễn Thái Ngọc Duy schrieb:\n > > --- a/setup.c\n > > +++ b/setup.c\n > > @@ -577,10 +577,14 @@ const char *setup_git_directory(void)\n > > \t/* If the work tree is not the default one, recompute prefix */\n > > \tif (inside_work_tree < 0) {\n > > \t\tstatic char buffer[PATH_MAX + 1];\n > > +\t\tstatic char cwd[PATH_MAX + 1];\n > > \t\tchar *rel;\n > > +\t\tgetcwd(cwd, PATH_MAX);\n > \n > This needs an error check.\n\n Check added.\n\n setup.c | 5 +++++\n t/t1501-worktree.sh | 13 ++++++++++++-\n 2 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 6cf9094..fa1d696 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -577,10 +577,15 @@ const char *setup_git_directory(void)\n \t/* If the work tree is not the default one, recompute prefix */\n \tif (inside_work_tree < 0) {\n \t\tstatic char buffer[PATH_MAX + 1];\n+\t\tstatic char cwd[PATH_MAX + 1];\n \t\tchar *rel;\n+\t\tif (!getcwd(cwd, PATH_MAX))\n+\t\t\tdie (\"Could not get the current working directory\");\n \t\tif (retval && chdir(retval))\n \t\t\tdie (\"Could not jump back into original cwd\");\n \t\trel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());\n+\t\tif (retval && chdir(cwd))\n+\t\t\tdie (\"Could not jump back into original cwd\");\n \t\treturn rel && *rel ? strcat(rel, \"/\") : NULL;\n \t}\n \ndiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\nindex 2ee88d8..d53d66a 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -29,7 +29,18 @@ test_rev_parse() {\n }\n \n mkdir -p work/sub/dir || exit 1\n-mv .git repo.git || exit 1\n+\n+git config core.worktree \"$(pwd)\"/work\n+mv .git work || exit 1\n+test_expect_success '--git-dir with relative .git' '\n+\t(\n+\tMYPWD=\"$(pwd)\"\n+\tcd work/sub/dir &&\n+\ttest \"$MYPWD\"/work/.git = \"$(git rev-parse --git-dir)\"\n+\t)\n+'\n+\n+mv work/.git repo.git || exit 1\n \n say \"core.worktree = relative path\"\n GIT_DIR=repo.git\n-- \n1.5.5.GIT\n"},{"id":"84705","messageId":"alpine.DEB.1.00.0807241324040.8986@racer","threadId":"14649","inReplyTo":"20080724031441.GA26072@laptop","subject":"Re: [PATCH] Preserve cwd in setup_git_directory()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-24T12:26:40Z","receivedAt":"2008-07-24T12:26:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 24 Jul 2008, Nguyễn Thái Ngọc Duy wrote:\n\n> When GIT_DIR is not set, cwd is used to determine where .git is. If \n> core.worktree is set, setup_git_directory() needs to jump back to the \n> original cwd in order to calculate worktree, this leads to incorrect \n> .git location later in setup_work_tree().\n\nI do not understand. core.worktree is either absolute, in which case \nthere is no problem. Or it is relative to where the config lives, no?\n\nBesides, this touches a _very_ delicate part of Git. I'd rather not touch \nit during the -rc cycle.\n\nI remember I was opposed to the whole worktree crap, and judging by the \nsheer amount of bug reports, next to nobody uses it anyway.\n\nIt was implemented in a really ugly manner, too, and my attempt to fix it \nwas still messy. That is why we have _only_ problems with it.\n\nJust thinking of worktree makes me uneasy,\nDscho\n"},{"id":"84708","messageId":"fcaeb9bf0807240540i400fe0d1s7ea8efe72203471d@mail.gmail.com","threadId":"14649","inReplyTo":"alpine.DEB.1.00.0807241324040.8986@racer","subject":"Re: [PATCH] Preserve cwd in setup_git_directory()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-07-24T12:40:18Z","receivedAt":"2008-07-24T12:40:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 7/24/08, Johannes Schindelin wrote:\n> Hi,\n>\n>\n> On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:\n>\n> > When GIT_DIR is not set, cwd is used to determine where .git is. If\n> > core.worktree is set, setup_git_directory() needs to jump back to the\n> > original cwd in order to calculate worktree, this leads to incorrect\n> > .git location later in setup_work_tree().\n>\n>\n> I do not understand. core.worktree is either absolute, in which case\n> there is no problem. Or it is relative to where the config lives, no?\n\nThe problem is GIT_DIR is not absolute in this case. So cwd must stay\nwhere git dir is until it is absolute-ized by setup_work_tree().\n\n-- \nDuy\n"},{"id":"84719","messageId":"alpine.DEB.1.00.0807241506340.8986@racer","threadId":"14649","inReplyTo":"fcaeb9bf0807240540i400fe0d1s7ea8efe72203471d@mail.gmail.com","subject":"Re: [PATCH] Preserve cwd in setup_git_directory()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-24T14:09:58Z","receivedAt":"2008-07-24T14:09:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:\n\n> On 7/24/08, Johannes Schindelin wrote:\n>\n> > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:\n> >\n> > > When GIT_DIR is not set, cwd is used to determine where .git is. If \n> > > core.worktree is set, setup_git_directory() needs to jump back to \n> > > the original cwd in order to calculate worktree, this leads to \n> > > incorrect .git location later in setup_work_tree().\n> >\n> > I do not understand. core.worktree is either absolute, in which case \n> > there is no problem. Or it is relative to where the config lives, no?\n> \n> The problem is GIT_DIR is not absolute in this case. So cwd must stay\n> where git dir is until it is absolute-ized by setup_work_tree().\n\nI do not see GIT_DIR being set in your test case at all.\n\nI do not see how get_git_work_tree() ro get_relative_cwd() should ever be \nallowed to chdir().\n\n_If_ they were (which I strongly doubt), they should chdir() back \nthemselves.\n\nI now wasted easily 30 minutes just trying to make sense of your patch and \nyour response. And I am still puzzled.\n\nYour commit message was of no help.\n\nThis patch is definitely the opposite of \"obviously correct\".\n\nCiao,\nDscho\n"},{"id":"84766","messageId":"fcaeb9bf0807241137h65292d7egad8cc5f797114607@mail.gmail.com","threadId":"14649","inReplyTo":"alpine.DEB.1.00.0807241506340.8986@racer","subject":"Re: [PATCH] Preserve cwd in setup_git_directory()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-07-24T18:37:55Z","receivedAt":"2008-07-24T18:37:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 7/24/08, Johannes Schindelin wrote:\n> Hi,\n>\n> On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:\n>\n> > On 7/24/08, Johannes Schindelin wrote:\n> >\n>\n> > > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:\n> > >\n> > > > When GIT_DIR is not set, cwd is used to determine where .git is. If\n> > > > core.worktree is set, setup_git_directory() needs to jump back to\n> > > > the original cwd in order to calculate worktree, this leads to\n> > > > incorrect .git location later in setup_work_tree().\n> > >\n> > > I do not understand. core.worktree is either absolute, in which case\n> > > there is no problem. Or it is relative to where the config lives, no?\n> >\n> > The problem is GIT_DIR is not absolute in this case. So cwd must stay\n> > where git dir is until it is absolute-ized by setup_work_tree().\n>\n>\n> I do not see GIT_DIR being set in your test case at all.\n>\n> I do not see how get_git_work_tree() ro get_relative_cwd() should ever be\n> allowed to chdir().\n>\n> _If_ they were (which I strongly doubt), they should chdir() back\n> themselves.\n>\n> I now wasted easily 30 minutes just trying to make sense of your patch and\n> your response. And I am still puzzled.\n>\n> Your commit message was of no help.\n\nAlright, let's look at the code.\n\n 1. cwd is moved to toplevel working directory by setup_git_directory_gently()\n 2. setup_git_env() by default will set git_dir variable as \".git\" as\npart of check_repository_format_gently()\n 3. now in setup_git_directory() finds out core.worktree, it chdir()\nto get relative prefix\n 4. setup_work_tree() sees that git_dir is not absolute path, it makes\ngit_dir absolute\n\nIf in step 3, it does not chdir(), step 4 will be right. In this case,\nstep 3 does chdir() and not going back, access to git repository will\nfail as Geoff Russell discovered.\n-- \nDuy\n"},{"id":"84853","messageId":"93c3eada0807250248l60c81090sd1953df24cca1421@mail.gmail.com","threadId":"14649","inReplyTo":"fcaeb9bf0807241137h65292d7egad8cc5f797114607@mail.gmail.com","subject":"Re: [PATCH] Preserve cwd in setup_git_directory()","fromName":"Geoff Russell","fromEmail":"geoffrey.russell@gmail.com","sentAt":"2008-07-25T09:48:57Z","receivedAt":"2008-07-25T09:48:57Z","isPatch":true,"sender":{"key":"geoffrey.russell@gmail.com","avatar":"https://gravatar.com/avatar/c30f497ccfa6bf06d86f30bd2ba092a2dd124c61c6bc902f7cb5c3f6486947de?d=mp&s=160"},"body":"Hi,\n\nOn 7/25/08, Nguyen Thai Ngoc Duy wrote:\n> On 7/24/08, Johannes Schindelin wrote:\n> > Hi,\n> >\n> > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:\n> >\n> > > On 7/24/08, Johannes Schindelin wrote:\n> > >\n> >\n> > > > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:\n> > > >\n> > > > > When GIT_DIR is not set, cwd is used to determine where .git is. If\n> > > > > core.worktree is set, setup_git_directory() needs to jump back to\n> > > > > the original cwd in order to calculate worktree, this leads to\n> > > > > incorrect .git location later in setup_work_tree().\n> > > >\n> > > > I do not understand. core.worktree is either absolute, in which case\n> > > > there is no problem. Or it is relative to where the config lives, no?\n> > >\n> > > The problem is GIT_DIR is not absolute in this case. So cwd must stay\n> > > where git dir is until it is absolute-ized by setup_work_tree().\n> >\n> >\n> > I do not see GIT_DIR being set in your test case at all.\n> >\n> > I do not see how get_git_work_tree() ro get_relative_cwd() should ever be\n> > allowed to chdir().\n> >\n> > _If_ they were (which I strongly doubt), they should chdir() back\n> > themselves.\n> >\n> > I now wasted easily 30 minutes just trying to make sense of your patch and\n> > your response. And I am still puzzled.\n> >\n> > Your commit message was of no help.\n>\n>\n> Alright, let's look at the code.\n>\n> 1. cwd is moved to toplevel working directory by setup_git_directory_gently()\n> 2. setup_git_env() by default will set git_dir variable as \".git\" as\n> part of check_repository_format_gently()\n> 3. now in setup_git_directory() finds out core.worktree, it chdir()\n> to get relative prefix\n> 4. setup_work_tree() sees that git_dir is not absolute path, it makes\n> git_dir absolute\n>\n> If in step 3, it does not chdir(), step 4 will be right. In this case,\n> step 3 does chdir() and not going back, access to git repository will\n> fail as Geoff Russell discovered.\n> --\n>\n> Duy\n>\n\n\nThere seem to be plenty of things happening here which I know nothing\nabout, which\nis really why I hit the problem in the first place. I was pretty\nsurprised to find that\nmy command line switch (--work-tree) was being stored in the config file, which\nended up causing the problem. Duy thinks he can fix this, which is\nfine, but as a matter of principle, I'm not keen on command line switches\nbeing magically saved for me when I didn't ask for this to happen.\nThe reason for\nusing a command line switch is because I want to override default\nbehaviour on this execution, if I wanted to change the default behaviour, then\nI'd set values in the config file.\n\nWhatever you decide, I reckon its pretty magical to have a software\nproblem, send\nan email to a list and get the solution in 45 minutes. I've never got that\nservice from any other piece of software (or hardware)!\n\nCheers,\nGeoff Russell\n"},{"id":"84855","messageId":"alpine.DEB.1.00.0807251203001.11976@eeepc-johanness","threadId":"14649","inReplyTo":"93c3eada0807250248l60c81090sd1953df24cca1421@mail.gmail.com","subject":"Re: [PATCH] Preserve cwd in setup_git_directory()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-25T10:04:51Z","receivedAt":"2008-07-25T10:04:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 25 Jul 2008, Geoff Russell wrote:\n\n> I was pretty surprised to find that my command line switch (--work-tree) \n> was being stored in the config file, which ended up causing the problem. \n> Duy thinks he can fix this, which is fine, but as a matter of principle, \n> I'm not keen on command line switches being magically saved for me when \n> I didn't ask for this to happen.\n\nYou were initializing a repository. How on earth could you be surprised \nwhen _important_ things like work-tree git-dir or if it is bare are stored \ninside the freshly initialized repository?\n\n> The reason for using a command line switch is because I want to override \n> default behaviour on this execution, if I wanted to change the default \n> behaviour, then I'd set values in the config file.\n\nNo, you would not. Because there is no config file yet.\n\nHtrh,\nDscho\n"}]}