[PATCH] Preserve cwd in setup_git_directory()

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 <redacted>
---
 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

Nguyễn Thái Ngọc Duy schrieb:

This needs an error check.

-- Hannes

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 <redacted>
---
  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

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

On 7/24/08, Johannes Schindelin [off-list ref] 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

Hi,

On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:

On 7/24/08, Johannes Schindelin [off-list ref] wrote:

quoted

 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

On 7/24/08, Johannes Schindelin [off-list ref] wrote:

Hi,

 On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:

 > On 7/24/08, Johannes Schindelin [off-list ref] wrote:
 >

quoted

quoted

 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

Hi,

On 7/25/08, Nguyen Thai Ngoc Duy [off-list ref] wrote:

On 7/24/08, Johannes Schindelin [off-list ref] wrote:
 > Hi,
 >
 >  On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:
 >
 >  > On 7/24/08, Johannes Schindelin [off-list ref] 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

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