[PATCH] setup: do not change to work tree prematurely

[PATCH] setup: do not change to work tree prematurely

[Clemens Buchacher]

If the work tree is known and a git command is invoked from within
a git directory, git_setup_directory() will try to find the
relative path from the work tree to the git dir. After doing so it
changes directories to the work tree. It fails to update the
relative path to the git directory, however.

Instead, do not change the working directory at this point and wait
for git_setup_work_tree() to handle this correctly.

This fixes the following bug.

$ cd .git
$ git --work-tree=/tmp/git symbolic-ref HEAD
fatal: ref HEAD is not a symbolic ref

Reported-by: Frédéric Brière <fbriere@fbriere.net>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

I am not 100% certain that my analysis is correct, since I still do
not understand the setup code. But as far as setup_git_directory()
is concerned, I think this is the intended behavior.

Clemens

 dir.c   |   33 +++++++++++++++++++--------------
 dir.h   |    1 +
 setup.c |   14 +++++++++-----
 3 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/dir.c b/dir.c
index cb83332..7bf208d 100644
--- a/dir.c
+++ b/dir.c
@@ -926,6 +926,24 @@ int file_exists(const char *f)
 	return lstat(f, &sb) == 0;
 }
 
+char *get_relative_path(char *cwd, const char *dir)
+{
+	if (!dir)
+		return NULL;
+	if (!is_absolute_path(dir))
+		dir = make_absolute_path(dir);
+
+	while (*dir && *dir == *cwd) {
+		dir++;
+		cwd++;
+	}
+	if (*dir)
+		return NULL;
+	if (*cwd == '/')
+		return cwd + 1;
+	return cwd;
+}
+
 /*
  * get_relative_cwd() gets the prefix of the current working directory
  * relative to 'dir'.  If we are not inside 'dir', it returns NULL.
@@ -942,25 +960,12 @@ int file_exists(const char *f)
  */
 char *get_relative_cwd(char *buffer, int size, const char *dir)
 {
-	char *cwd = buffer;
-
 	if (!dir)
 		return NULL;
 	if (!getcwd(buffer, size))
 		die_errno("can't find the current directory");
 
-	if (!is_absolute_path(dir))
-		dir = make_absolute_path(dir);
-
-	while (*dir && *dir == *cwd) {
-		dir++;
-		cwd++;
-	}
-	if (*dir)
-		return NULL;
-	if (*cwd == '/')
-		return cwd + 1;
-	return cwd;
+	return get_relative_path(buffer, dir);
 }
 
 int is_inside_dir(const char *dir)
diff --git a/dir.h b/dir.h
index 3bead5f..3bcda1f 100644
--- a/dir.h
+++ b/dir.h
@@ -79,6 +79,7 @@ extern void add_exclude(const char *string, const char *base,
 			int baselen, struct exclude_list *which);
 extern int file_exists(const char *);
 
+extern char *get_relative_path(char *cwd, const char *dir);
 extern char *get_relative_cwd(char *buffer, int size, const char *dir);
 extern int is_inside_dir(const char *dir);
 
diff --git a/setup.c b/setup.c
index 5716d90..67b5122 100644
--- a/setup.c
+++ b/setup.c
@@ -525,13 +525,17 @@ const char *setup_git_directory(void)
 
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
+		const char *work_tree = get_git_work_tree();
 		static char buffer[PATH_MAX + 1];
 		char *rel;
-		if (retval && chdir(retval))
-			die_errno ("Could not jump back into original cwd");
-		rel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());
-		if (rel && *rel && chdir(get_git_work_tree()))
-			die_errno ("Could not jump to working directory");
+		if (retval) {
+			if (!is_absolute_path(retval))
+				retval = make_absolute_path(retval);
+			strncpy(buffer, retval, PATH_MAX);
+			buffer[PATH_MAX] = '\0';
+			rel = get_relative_path(buffer, work_tree);
+		} else
+			rel = get_relative_cwd(buffer, PATH_MAX, work_tree);
 		return rel && *rel ? strcat(rel, "/") : NULL;
 	}
 
-- 
1.7.0.5.3.ga76e

Re: [PATCH] setup: do not change to work tree prematurely

[Jonathan Nieder]

Hi Clemens,

Clemens Buchacher wrote:

> $ cd .git
> $ git --work-tree=/tmp/git symbolic-ref HEAD
> fatal: ref HEAD is not a symbolic ref

Without your patch applied, I get

 $ cd .git
 $ git --work-tree=/tmp/git symbolic-ref HEAD
 refs/heads/hello

I should have done the following instead:

 $ worktree=$(pwd)
 $ cd .git
 $ git --work-tree="$worktree" symbolic-ref HEAD
 fatal: ref HEAD is not a symbolic ref

> +char *get_relative_path(char *cwd, const char *dir)

Looks reasonable.  Should be const char *cwd.

> diff --git a/setup.c b/setup.c
> index 5716d90..67b5122 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -524,14 +524,18 @@ const char *setup_git_directory(void)
> 	const char *retval = setup_git_directory_gently(NULL);
>  
>  	/* If the work tree is not the default one, recompute prefix */
>  	if (inside_work_tree < 0) {

If I understand correctly, you made two changes here:

 - interpret GIT_WORK_TREE and friends relative to .git instead of
   the original cwd (by not calling chdir(retval) before
   get_git_work_tree())

 - make setup_git_directory stay in the last directory searched for a
   .git directory instead of chdir-ing into the toplevel of the work tree.

Are these safe changes to make?

Jonathan

diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 9df3012..93bf92c 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -106,6 +106,15 @@ test_expect_success 'repo finds its work tree from work tree, too' '
 	 test sub/dir/tracked = "$(git ls-files)")
 '
 
+test -d work || cp -R repo.git/work .
+test_expect_success 'repo finds its .git dir with separate worktree' '
+	(unset GIT_DIR GIT_WORK_TREE GIT_CONFIG &&
+	 worktree=$(pwd) &&
+	 cd repo.git &&
+	 git symbolic-ref HEAD &&
+	 git --work-tree="$worktree" symbolic-ref HEAD)
+'
+
 test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '
 	(cd repo.git/work/sub/dir &&
 	GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \
-- 

Re: [PATCH] setup: do not change to work tree prematurely

[Clemens Buchacher]

On Sat, May 22, 2010 at 08:35:39PM -0500, Jonathan Nieder wrote:

> Without your patch applied, I get
> 
>  $ cd .git
>  $ git --work-tree=/tmp/git symbolic-ref HEAD
>  refs/heads/hello
> 
> I should have done the following instead:
> 
>  $ worktree=$(pwd)
>  $ cd .git
>  $ git --work-tree="$worktree" symbolic-ref HEAD
>  fatal: ref HEAD is not a symbolic ref

Thanks

> > +char *get_relative_path(char *cwd, const char *dir)
> 
> Looks reasonable.  Should be const char *cwd.

That is not possible, because we return a pointer to cwd plus an
offset, which will be subsequently modified by strcat().

> > diff --git a/setup.c b/setup.c
> > index 5716d90..67b5122 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -524,14 +524,18 @@ const char *setup_git_directory(void)
> > 	const char *retval = setup_git_directory_gently(NULL);
> >  
> >  	/* If the work tree is not the default one, recompute prefix */
> >  	if (inside_work_tree < 0) {
> 
> If I understand correctly, you made two changes here:
> 
>  - interpret GIT_WORK_TREE and friends relative to .git instead of
>    the original cwd (by not calling chdir(retval) before
>    get_git_work_tree())

Oops

>  - make setup_git_directory stay in the last directory searched for a
>    .git directory instead of chdir-ing into the toplevel of the work tree.
>
> Are these safe changes to make?

Indeed, if you put it that way, this change does not look good.
Maybe I'll have a look at Nguyen's patches.

> diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
> index 9df3012..93bf92c 100755
> --- a/t/t1501-worktree.sh
> +++ b/t/t1501-worktree.sh

Thanks