[PATCH] Preserve cwd in setup_git_directory()

[PATCH] Preserve cwd in setup_git_directory()

[Nguyễn Thái Ngọc Duy]

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 <pclouds@gmail.com>
---
 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

Re: [PATCH] Preserve cwd in setup_git_directory()

[Johannes Sixt]

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

[PATCH] Preserve cwd in setup_git_directory()

[Nguyen Thai Ngoc Duy]

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 <pclouds@gmail.com>
---
  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

Re: [PATCH] Preserve cwd in setup_git_directory()

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 888 bytes --]

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

Re: [PATCH] Preserve cwd in setup_git_directory()

[Nguyen Thai Ngoc Duy]

On 7/24/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] Preserve cwd in setup_git_directory()

[Johannes Schindelin]

Hi,

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

> On 7/24/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] Preserve cwd in setup_git_directory()

[Nguyen Thai Ngoc Duy]

On 7/24/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
>  On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:
>
>  > On 7/24/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] Preserve cwd in setup_git_directory()

[Geoff Russell]

Hi,

On 7/25/08, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On 7/24/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>  > Hi,
>  >
>  >  On Thu, 24 Jul 2008, Nguyen Thai Ngoc Duy wrote:
>  >
>  >  > On 7/24/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] Preserve cwd in setup_git_directory()

[Johannes Schindelin]

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