threads / patch / 23880

patchsetup: do not change to work tree prematurely

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

## tl;dr

3 messages between May 23, 2010 and May 23, 2010. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Clemens Buchacher· May 23, 2010, 00:07 UTC · lore

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(-)
Show changes to 3 files +29 −19

dir.c, dir.h, setup.c

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
Jonathan Nieder· May 23, 2010, 01:35 UTC · re: Clemens Buchacher · lore

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

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.
Show 9 quoted lines
> 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
Show changes to t/t1501-worktree.sh +9 −0
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= \
Clemens Buchacher· May 23, 2010, 09:14 UTC · re: Jonathan Nieder · lore

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

On Sat, May 22, 2010 at 08:35:39PM -0500, Jonathan Nieder wrote:
Show 12 quoted lines
> 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().

Show 15 quoted lines
> > 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

← back to recent threads