# [PATCH] Preserve cwd in setup_git_directory() 9 messages from 2008-07-24 to 2008-07-25. Participants: Nguyễn Thái Ngọc Duy, Johannes Sixt, Nguyen Thai Ngoc Duy, Johannes Schindelin, Geoff Russell. Thread: https://gitlist.dev/t/14649 ## Nguyễn Thái Ngọc Duy, 2008-07-24 03:14 Subject: [PATCH] Preserve cwd in setup_git_directory() Message-ID: <20080724031441.GA26072@laptop> URL: https://gitlist.dev/e/20080724031441.GA26072%40laptop ``` When GIT_DIR is not set, cwd is used to determine where .git is. If core.worktree is set, setup_git_directory() needs to jump back to the original cwd in order to calculate worktree, this leads to incorrect .git location later in setup_work_tree(). Signed-off-by: Nguyễn Thái Ngọc Duy --- setup.c | 4 ++++ t/t1501-worktree.sh | 13 ++++++++++++- 2 files changed, 16 insertions(+), 1 deletions(-) diff --git a/setup.c b/setup.c index 6cf9094..ef58761 100644 --- a/setup.c +++ b/setup.c @@ -577,10 +577,14 @@ const char *setup_git_directory(void) /* If the work tree is not the default one, recompute prefix */ if (inside_work_tree < 0) { static char buffer[PATH_MAX + 1]; + static char cwd[PATH_MAX + 1]; char *rel; + getcwd(cwd, PATH_MAX); if (retval && chdir(retval)) die ("Could not jump back into original cwd"); rel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree()); + if (retval && chdir(cwd)) + die ("Could not jump back into original cwd"); return rel && *rel ? strcat(rel, "/") : NULL; } diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh index 2ee88d8..64f8dea 100755 --- a/t/t1501-worktree.sh +++ b/t/t1501-worktree.sh @@ -29,7 +29,18 @@ test_rev_parse() { } mkdir -p work/sub/dir || exit 1 -mv .git repo.git || exit 1 + +git config core.worktree "$(pwd)"/work +mv .git work || exit 1 +test_expect_success '--git-dir with relative .git' ' + ( + MYPWD="$(pwd)" + cd work/sub/dir && + test "$MYPWD"/work/.git = "$(git rev-parse --git-dir)" + ) +' + +mv work/.git repo.git || exit 1 say "core.worktree = relative path" GIT_DIR=repo.git -- 1.5.5.GIT ``` ## Johannes Sixt, 2008-07-24 06:50 Subject: Re: [PATCH] Preserve cwd in setup_git_directory() Message-ID: <48882628.7030305@viscovery.net> URL: https://gitlist.dev/e/48882628.7030305%40viscovery.net In-Reply-To: <20080724031441.GA26072@laptop> ``` Nguyễn Thái Ngọc Duy schrieb: > --- a/setup.c > +++ b/setup.c > @@ -577,10 +577,14 @@ const char *setup_git_directory(void) > /* If the work tree is not the default one, recompute prefix */ > if (inside_work_tree < 0) { > static char buffer[PATH_MAX + 1]; > + static char cwd[PATH_MAX + 1]; > char *rel; > + getcwd(cwd, PATH_MAX); This needs an error check. -- Hannes ``` ## Nguyen Thai Ngoc Duy, 2008-07-24 08:12 Subject: [PATCH] Preserve cwd in setup_git_directory() Message-ID: <20080724081222.GA32354@laptop> URL: https://gitlist.dev/e/20080724081222.GA32354%40laptop In-Reply-To: <48882628.7030305@viscovery.net> ``` When GIT_DIR is not set, cwd is used to determine where .git is. If core.worktree is set, setup_git_directory() needs to jump back to the original cwd in order to setup worktree, this leads to incorrect .git location later in setup_work_tree(). Signed-off-by: Nguyễn Thái Ngọc Duy --- On Thu, Jul 24, 2008 at 08:50:16AM +0200, Johannes Sixt wrote: > Nguyễn Thái Ngọc Duy schrieb: > > --- a/setup.c > > +++ b/setup.c > > @@ -577,10 +577,14 @@ const char *setup_git_directory(void) > > /* If the work tree is not the default one, recompute prefix */ > > if (inside_work_tree < 0) { > > static char buffer[PATH_MAX + 1]; > > + static char cwd[PATH_MAX + 1]; > > char *rel; > > + getcwd(cwd, PATH_MAX); > > This needs an error check. Check added. setup.c | 5 +++++ t/t1501-worktree.sh | 13 ++++++++++++- 2 files changed, 17 insertions(+), 1 deletions(-) diff --git a/setup.c b/setup.c index 6cf9094..fa1d696 100644 --- a/setup.c +++ b/setup.c @@ -577,10 +577,15 @@ const char *setup_git_directory(void) /* If the work tree is not the default one, recompute prefix */ if (inside_work_tree < 0) { static char buffer[PATH_MAX + 1]; + static char cwd[PATH_MAX + 1]; char *rel; + if (!getcwd(cwd, PATH_MAX)) + die ("Could not get the current working directory"); if (retval && chdir(retval)) die ("Could not jump back into original cwd"); rel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree()); + if (retval && chdir(cwd)) + die ("Could not jump back into original cwd"); return rel && *rel ? strcat(rel, "/") : NULL; } diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh index 2ee88d8..d53d66a 100755 --- a/t/t1501-worktree.sh +++ b/t/t1501-worktree.sh @@ -29,7 +29,18 @@ test_rev_parse() { } mkdir -p work/sub/dir || exit 1 -mv .git repo.git || exit 1 + +git config core.worktree "$(pwd)"/work +mv .git work || exit 1 +test_expect_success '--git-dir with relative .git' ' + ( + MYPWD="$(pwd)" + cd work/sub/dir && + test "$MYPWD"/work/.git = "$(git rev-parse --git-dir)" + ) +' + +mv work/.git repo.git || exit 1 say "core.worktree = relative path" GIT_DIR=repo.git -- 1.5.5.GIT ``` ## Johannes Schindelin, 2008-07-24 12:26 Subject: Re: [PATCH] Preserve cwd in setup_git_directory() Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807241324040.8986%40racer In-Reply-To: <20080724031441.GA26072@laptop> ``` Hi, On Thu, 24 Jul 2008, Nguyễn Thái Ngọc Duy wrote: > When GIT_DIR is not set, cwd is used to determine where .git is. If > core.worktree is set, setup_git_directory() needs to jump back to the > original cwd in order to calculate worktree, this leads to incorrect > .git location later in setup_work_tree(). I do not understand. core.worktree is either absolute, in which case there is no problem. Or it is relative to where the config lives, no? Besides, this touches a _very_ delicate part of Git. I'd rather not touch it during the -rc cycle. I remember I was opposed to the whole worktree crap, and judging by the sheer amount of bug reports, next to nobody uses it anyway. It was implemented in a really ugly manner, too, and my attempt to fix it was still messy. That is why we have _only_ problems with it. Just thinking of worktree makes me uneasy, Dscho ``` ## Nguyen Thai Ngoc Duy, 2008-07-24 12:40 Subject: Re: [PATCH] Preserve cwd in setup_git_directory() Message-ID: URL: https://gitlist.dev/e/fcaeb9bf0807240540i400fe0d1s7ea8efe72203471d%40mail.gmail.com In-Reply-To: ``` On 7/24/08, Johannes Schindelin wrote: > Hi, > > > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote: > > > When GIT_DIR is not set, cwd is used to determine where .git is. If > > core.worktree is set, setup_git_directory() needs to jump back to the > > original cwd in order to calculate worktree, this leads to incorrect > > .git location later in setup_work_tree(). > > > I do not understand. core.worktree is either absolute, in which case > there is no problem. Or it is relative to where the config lives, no? The problem is GIT_DIR is not absolute in this case. So cwd must stay where git dir is until it is absolute-ized by setup_work_tree(). -- Duy ``` ## Johannes Schindelin, 2008-07-24 14:09 Subject: Re: [PATCH] Preserve cwd in setup_git_directory() Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807241506340.8986%40racer In-Reply-To: ``` Hi, On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote: > On 7/24/08, Johannes Schindelin wrote: > > > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote: > > > > > When GIT_DIR is not set, cwd is used to determine where .git is. If > > > core.worktree is set, setup_git_directory() needs to jump back to > > > the original cwd in order to calculate worktree, this leads to > > > incorrect .git location later in setup_work_tree(). > > > > I do not understand. core.worktree is either absolute, in which case > > there is no problem. Or it is relative to where the config lives, no? > > The problem is GIT_DIR is not absolute in this case. So cwd must stay > where git dir is until it is absolute-ized by setup_work_tree(). I do not see GIT_DIR being set in your test case at all. I do not see how get_git_work_tree() ro get_relative_cwd() should ever be allowed to chdir(). _If_ they were (which I strongly doubt), they should chdir() back themselves. I now wasted easily 30 minutes just trying to make sense of your patch and your response. And I am still puzzled. Your commit message was of no help. This patch is definitely the opposite of "obviously correct". Ciao, Dscho ``` ## Nguyen Thai Ngoc Duy, 2008-07-24 18:37 Subject: Re: [PATCH] Preserve cwd in setup_git_directory() Message-ID: URL: https://gitlist.dev/e/fcaeb9bf0807241137h65292d7egad8cc5f797114607%40mail.gmail.com In-Reply-To: ``` On 7/24/08, Johannes Schindelin wrote: > Hi, > > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote: > > > On 7/24/08, Johannes Schindelin wrote: > > > > > > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote: > > > > > > > When GIT_DIR is not set, cwd is used to determine where .git is. If > > > > core.worktree is set, setup_git_directory() needs to jump back to > > > > the original cwd in order to calculate worktree, this leads to > > > > incorrect .git location later in setup_work_tree(). > > > > > > I do not understand. core.worktree is either absolute, in which case > > > there is no problem. Or it is relative to where the config lives, no? > > > > The problem is GIT_DIR is not absolute in this case. So cwd must stay > > where git dir is until it is absolute-ized by setup_work_tree(). > > > I do not see GIT_DIR being set in your test case at all. > > I do not see how get_git_work_tree() ro get_relative_cwd() should ever be > allowed to chdir(). > > _If_ they were (which I strongly doubt), they should chdir() back > themselves. > > I now wasted easily 30 minutes just trying to make sense of your patch and > your response. And I am still puzzled. > > Your commit message was of no help. Alright, let's look at the code. 1. cwd is moved to toplevel working directory by setup_git_directory_gently() 2. setup_git_env() by default will set git_dir variable as ".git" as part of check_repository_format_gently() 3. now in setup_git_directory() finds out core.worktree, it chdir() to get relative prefix 4. setup_work_tree() sees that git_dir is not absolute path, it makes git_dir absolute If in step 3, it does not chdir(), step 4 will be right. In this case, step 3 does chdir() and not going back, access to git repository will fail as Geoff Russell discovered. -- Duy ``` ## Geoff Russell, 2008-07-25 09:48 Subject: Re: [PATCH] Preserve cwd in setup_git_directory() Message-ID: <93c3eada0807250248l60c81090sd1953df24cca1421@mail.gmail.com> URL: https://gitlist.dev/e/93c3eada0807250248l60c81090sd1953df24cca1421%40mail.gmail.com In-Reply-To: ``` Hi, On 7/25/08, Nguyen Thai Ngoc Duy wrote: > On 7/24/08, Johannes Schindelin wrote: > > Hi, > > > > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote: > > > > > On 7/24/08, Johannes Schindelin wrote: > > > > > > > > > On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote: > > > > > > > > > When GIT_DIR is not set, cwd is used to determine where .git is. If > > > > > core.worktree is set, setup_git_directory() needs to jump back to > > > > > the original cwd in order to calculate worktree, this leads to > > > > > incorrect .git location later in setup_work_tree(). > > > > > > > > I do not understand. core.worktree is either absolute, in which case > > > > there is no problem. Or it is relative to where the config lives, no? > > > > > > The problem is GIT_DIR is not absolute in this case. So cwd must stay > > > where git dir is until it is absolute-ized by setup_work_tree(). > > > > > > I do not see GIT_DIR being set in your test case at all. > > > > I do not see how get_git_work_tree() ro get_relative_cwd() should ever be > > allowed to chdir(). > > > > _If_ they were (which I strongly doubt), they should chdir() back > > themselves. > > > > I now wasted easily 30 minutes just trying to make sense of your patch and > > your response. And I am still puzzled. > > > > Your commit message was of no help. > > > Alright, let's look at the code. > > 1. cwd is moved to toplevel working directory by setup_git_directory_gently() > 2. setup_git_env() by default will set git_dir variable as ".git" as > part of check_repository_format_gently() > 3. now in setup_git_directory() finds out core.worktree, it chdir() > to get relative prefix > 4. setup_work_tree() sees that git_dir is not absolute path, it makes > git_dir absolute > > If in step 3, it does not chdir(), step 4 will be right. In this case, > step 3 does chdir() and not going back, access to git repository will > fail as Geoff Russell discovered. > -- > > Duy > There seem to be plenty of things happening here which I know nothing about, which is really why I hit the problem in the first place. I was pretty surprised to find that my command line switch (--work-tree) was being stored in the config file, which ended up causing the problem. Duy thinks he can fix this, which is fine, but as a matter of principle, I'm not keen on command line switches being magically saved for me when I didn't ask for this to happen. The reason for using a command line switch is because I want to override default behaviour on this execution, if I wanted to change the default behaviour, then I'd set values in the config file. Whatever you decide, I reckon its pretty magical to have a software problem, send an email to a list and get the solution in 45 minutes. I've never got that service from any other piece of software (or hardware)! Cheers, Geoff Russell ``` ## Johannes Schindelin, 2008-07-25 10:04 Subject: Re: [PATCH] Preserve cwd in setup_git_directory() Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807251203001.11976%40eeepc-johanness In-Reply-To: <93c3eada0807250248l60c81090sd1953df24cca1421@mail.gmail.com> ``` Hi, On Fri, 25 Jul 2008, Geoff Russell wrote: > I was pretty surprised to find that my command line switch (--work-tree) > was being stored in the config file, which ended up causing the problem. > Duy thinks he can fix this, which is fine, but as a matter of principle, > I'm not keen on command line switches being magically saved for me when > I didn't ask for this to happen. You were initializing a repository. How on earth could you be surprised when _important_ things like work-tree git-dir or if it is bare are stored inside the freshly initialized repository? > The reason for using a command line switch is because I want to override > default behaviour on this execution, if I wanted to change the default > behaviour, then I'd set values in the config file. No, you would not. Because there is no config file yet. Htrh, Dscho ```